Re: [PATCH v7 04/10] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[Andy Shevchenko]

On Wed, Jan 14, 2026 at 09:39:51AM +0100, AngeloGioacchino Del Regno wrote:
> Some devices connected over the SPMI bus may be big, in the sense
> that those may be a complex of devices managed by a single chip
> over the SPMI bus, reachable through a single SID.
> 
> Add new functions aimed at managing sub-devices of a SPMI device
> spmi_subdevice_alloc_and_add() and a spmi_subdevice_put_and_remove()
> for adding a new subdevice and removing it respectively, and also
> add their devm_* variants.
> 
> The need for such functions comes from the existence of	those
> complex Power Management ICs (PMICs), which feature one or many
> sub-devices, in some cases with these being even addressable on
> the chip in form of SPMI register ranges.
> 
> Examples of those devices can be found in both Qualcomm platforms
> with their PMICs having PON, RTC, SDAM, GPIO controller, and other
> sub-devices, and in newer MediaTek platforms showing similar HW
> features and a similar layout with those also having many subdevs.
> 
> Also, instead of generally exporting symbols, export them with a
> new "SPMI" namespace: all users will have to import this namespace
> to make use of the newly introduced exports.

...

> +static void devm_spmi_subdevice_remove(void *res)

For better readability we usually call the variable with a meaningful name, and
sub_sdev here seems the best.

> +{
> +	spmi_subdevice_remove(res);
> +}

...

> + * Pointer to newly allocated SPMI sub-device for success or negative ERR_PTR.

This is "negative ERR_PTR" nonsense. Either "negative errno" or "error pointer".
Or something alike.

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

Too far, may be prone to mistakes in the future. The rewritten one would look
like

	sub_sdev = kzalloc(sizeof(*sub_sdev), GFP_KERNEL);
	if (!sub_sdev)
		return ERR_PTR(-ENOMEM);

	sdev = &sub_sdev->sdev;
	sdev->ctrl = sparent->ctrl;

	ret = ida_alloc(&spmi_subdevice_ida, GFP_KERNEL);
	if (ret < 0) {
		kfree(sub_sdev);
		return ERR_PTR(ret);
	}
	sub_sdev->devid = ret;

	device_initialize(&sdev->dev);
	sdev->dev.parent = &sparent->dev;
	sdev->dev.bus = &spmi_bus_type;
	sdev->dev.type = &spmi_subdev_type;


> +	sdev->usid = sparent->usid;
> +
> +	ret = dev_set_name(&sdev->dev, "%d-%02x.%d.auto",
> +			   sdev->ctrl->nr, sdev->usid, sub_sdev->devid);
> +	if (ret)
> +		goto err_put_dev;
> +
> +	ret = device_add(&sdev->dev);
> +	if (ret) {
> +		dev_err(&sdev->dev, "Can't add device, status %pe\n", ERR_PTR(ret));
> +		goto err_put_dev;
> +	}
> +
> +	return sub_sdev;
> +
> +err_put_dev:
> +	put_device(&sdev->dev);
> +	return ERR_PTR(ret);
> +}

-- 
With Best Regards,
Andy Shevchenko