Re: [PATCH 2/9] hwrng: arm-smccc-trng - transition to the faux device interface

[Sudeep Holla <sudeep.holla@arm.com>]

On Mon, Mar 17, 2025 at 03:30:15PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Mar 17, 2025 at 02:22:45PM +0000, Sudeep Holla wrote:
> > On Mon, Mar 17, 2025 at 02:04:27PM +0100, Greg Kroah-Hartman wrote:
> > > On Mon, Mar 17, 2025 at 10:13:14AM +0000, Sudeep Holla wrote:
> > > > +MODULE_ALIAS("faux:smccc_trng");
> > > 
> > > Why do you need a branch new alias you just made up?  Please don't add
> > > that for these types of devices, that's not going to work at all (just
> > > like the platform alias really doesn't work well.
> > > 
> > 
> > Sure I will drop all of those alias. One question I have if the idea of
> > creating a macro for this is good or bad ? I need this initial condition
> > flag to make use of such a macro, so I didn't go for it, but it does
> > remove some boiler-plate code.
> > 
> > Let me know what do you think of it ?
> > 
> > Regards,
> > Sudeep
> > 
> > -->8 
> > diff --git i/include/linux/device/faux.h w/include/linux/device/faux.h
> > index 9f43c0e46aa4..8af3eaef281a 100644
> > --- i/include/linux/device/faux.h
> > +++ w/include/linux/device/faux.h
> > @@ -66,4 +66,30 @@ static inline void faux_device_set_drvdata(struct faux_device *faux_dev, void *d
> >  	dev_set_drvdata(&faux_dev->dev, data);
> >  }
> >  
> > +#define module_faux_driver(name, tag, init_cond)			\
> > +static struct faux_device_ops tag##_ops = {				\
> > +	.probe = tag##_probe,						\
> > +	.remove = tag##_remove,						\
> > +};									\
> > +									\
> > +static struct faux_device *tag##_dev;					\
> > +									\
> > +static int __init tag##_init(void)					\
> > +{									\
> > +	if (!(init_cond))						\
> > +		return 0;						\
> > +	tag##_dev = faux_device_create(name, NULL, &tag##_ops);		\
> > +	if (!tag##_dev)							\
> > +		return -ENODEV;						\
> > +									\
> > +	return 0;							\
> > +}									\
> > +module_init(tag##_init);						\
> > +									\
> > +static void __exit tag##_exit(void)					\
> > +{									\
> > +	faux_device_destroy(tag##_dev);					\
> > +}									\
> > +module_exit(tag##_exit);						\
> 
> Yes, I see that some of your changes could be moved to use this, so I
> think it is worth it.
> 
> But why can't you use module_driver() here?  Ah, that init_cond?  And
> the device.  Hm, why not put the init_cond in the probe callback?  That
> should make this macro simpler.
> 

I tried to keep the creation of the device itself conditional the way
it is today. Deferring the check to probe means the device gets created
unconditionally but won't be probed which is fine I guess. I was thinking
that device shouldn't show up on the bus if the condition is not met to
match the current scenario. I might be overthinking there.

> And don't use "tag", that's an odd term here, just copy what
> module_driver() does instead please.
> 

Sure, I will not use it. It was just a name that came to my mind.

Also creating the macro builds the dependency. Do you prefer to push the
changes as is and the macro in one release and make use of the macro later.

-- 
Regards,
Sudeep