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
