Re: [PATCH v5 3/6] block: introduce device_add_of_disk()

[Christian Marangi]

On Wed, Oct 02, 2024 at 01:40:58AM -0700, Christoph Hellwig wrote:
> Thanks,
> 
> this looks much better.  A few minor nitpicks, though:
>

Very happy you like it, yes I wasn't sure what was the correct way to
introduce the helper. If you notice in the blkdev.h we have also add_disk()
that is a static inline wrapper for device_add_disk().

Wonder if device_add_disk() should have the same treatement? No idea if
it would cause problem with symbol with external modules, that is why I
used the wrapper.

> > -int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
> > -				 const struct attribute_group **groups)
> > +static int __device_add_disk(struct device *parent, struct gendisk *disk,
> > +			     const struct attribute_group **groups,
> > +			     struct fwnode_handle *fwnode)
> 
> I don't think we need a separate helper if device_add_disk simply
> wraps the OF version by passing a NULL fwnode.
> 
> > +int __must_check device_add_of_disk(struct device *parent, struct gendisk *disk,
> > +				    const struct attribute_group **groups,
> > +				    struct fwnode_handle *fwnode)
> > +{
> > +	return __device_add_disk(parent, disk, groups, fwnode);
> > +}
> 
> I'd name this as add_disk_fwnode as the of in device_add_of_disk
> reads as in add the device of the disk, and the fwnode is what gets
> passed.  The device_ is a bit redundant and just there for historic
> reasons as the original add_disk predates the device model.
> 
> Can you also add a kerneldoc comment for the new helper?
> 

sure! I will wait the usual 24h to respin this.

> > +EXPORT_SYMBOL(device_add_of_disk);
> 
> EXPORT_SYMBO_GPL, please.
> 

ack.

-- 
	Ansuel