[PATCH v4 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[Uwe Kleine-König]

Hello AngeloGioacchino,

On Wed, Sep 17, 2025 at 01:41:40PM +0200, AngeloGioacchino Del Regno wrote:
> Il 16/09/25 15:25, Uwe Kleine-König ha scritto:
> > Hello AngeloGioacchino,
> > 
> > On Tue, Sep 16, 2025 at 10:44:39AM +0200, AngeloGioacchino Del Regno wrote:
> > > +/**
> > > + * spmi_subdevice_alloc_and_add(): Allocate and add a new SPMI sub-device
> > > + * @sparent:	SPMI parent device with previously registered SPMI controller
> > > + *
> > > + * Returns:
> > > + * Pointer to newly allocated SPMI sub-device for success or negative ERR_PTR.
> > > + */
> > > +struct spmi_subdevice *spmi_subdevice_alloc_and_add(struct spmi_device *sparent)
> > > +{
> > > +	struct spmi_subdevice *sub_sdev;
> > > +	struct spmi_device *sdev;
> > > +	int ret;
> > > +
> > > +	sub_sdev = kzalloc(sizeof(*sub_sdev), GFP_KERNEL);
> > > +	if (!sub_sdev)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	ret = ida_alloc(&spmi_subdevice_ida, GFP_KERNEL);
> > > +	if (ret < 0) {
> > > +		kfree(sub_sdev);
> > > +		return ERR_PTR(ret);
> > > +	}
> > > +
> > > +	sdev = &sub_sdev->sdev;
> > > +	sdev->ctrl = sparent->ctrl;
> > > +	device_initialize(&sdev->dev);
> > > +	sdev->dev.parent = &sparent->dev;
> > > +	sdev->dev.bus = &spmi_bus_type;
> > > +	sdev->dev.type = &spmi_subdev_type;
> > > +
> > > +	sub_sdev->devid = ret;
> > > +	sdev->usid = sparent->usid;
> > > +
> > > +	ret = dev_set_name(&sdev->dev, "%d-%02x.%d.auto",
> > > +			   sdev->ctrl->nr, sdev->usid, sub_sdev->devid);
> > 
> > If I understand correctly sub_sdev->devid is globally unique. I wonder
> > if a namespace that is specific to the parent spmi device would be more
> > sensible?!
> 
> Only in the context of the children of sdev. I'm not sure of what you're proposing
> here, looks like it would complicate the code for no big reason - unless I am
> misunderstanding something here.

The thing that I wondered about is: Why use sdev->usid if
sub_sdev->devid is already a unique description of the subdevice? And
for other device types (platform devices, mfd) the device identifiers
are not globally unique. So I just wondered why spmi is different here.

> > > +	if (ret)
> > > +		goto err_put_dev;
> > > +
> > > +	ret = device_add(&sdev->dev);
> > > +	if (ret) {
> > > +		dev_err(&sdev->dev, "Can't add %s, status %d\n",
> > 
> > I'd use %pe instead of %d here.
> > 
> 
> The only reason why I am using %d is for consistency with the rest of the code that
> is in SPMI - there is another device_add() call in spmi_device_add() which prints
> the same error in the very same way as I'm doing here.
> 
> I agree that using %pe makes error prints more readable, but perhaps that should be
> done as a later cleanup to keep prints consistent (and perhaps that should not be
> done only in SPMI anyway).
> 
> If you have really strong opinions about doing that right now I can do it, but I
> anyway prefer seeing that as a later commit doing that in the entire SPMI codebase.

My approach would be to first convert the driver to use %pe and then
add the new code. But I don't feel strong.

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-phy/attachments/20250917/a19b599e/attachment-0001.sig>