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

[Sudeep Holla]

On Mon, Mar 17, 2025 at 05:46:46PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Mar 17, 2025 at 02:43:21PM +0000, Sudeep Holla wrote:
> > 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.
> 
> It will not show up anywhere if the probe call fails.

Ah, nice. I somehow didn't realise that. Thanks for that info.

> 
> > > 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.
> 
> Let's see a series that adds the macro and uses it and we can figure it
> out from there.  If the macro is sane, I can just take that now for
> 6.15-rc1 and then you can send the others to the different subsystems
> after that shows up.
> 

Sure, thanks again.

-- 
Regards,
Sudeep