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

[AngeloGioacchino Del Regno]

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.

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

Cheers,
Angelo

> > +			dev_name(&sdev->dev), ret);
> > +		goto err_put_dev;
> > +	}
> > +
> > +	return sub_sdev;
> > +
> > +err_put_dev:
> > +	put_device(&sdev->dev);
> > +	return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(spmi_subdevice_alloc_and_add, "SPMI");
> > +
>
> Best regards
> Uwe