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.

quoted

-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.

quoted

+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.

quoted

+EXPORT_SYMBOL(device_add_of_disk);

EXPORT_SYMBO_GPL, please.

ack.

-- 
	Ansuel