[PATCH v13 05/12] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[sashiko-bot at kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The new `spmi_subdevice` acts as a dummy parent device but does not enable runtime PM, breaking PM propagation from child devices.
- [High] `spmi_subdevice_alloc_and_add()` conflates device allocation and registration, preventing callers from safely configuring matching properties (e.g., `of_node`) before `device_add()`.
--

--- Patch [5]: [PATCH v13 05/12] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant ---
commit 09395afcc62824fab7f184ceae90afb8ac488cec
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno at collabora.com>

spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

This commit introduces new functions to allocate and add sub-devices to an
SPMI device. It is intended for complex PMICs with multiple sub-devices on
the SPMI bus, and includes devm variants exported under the SPMI namespace.

> diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
> index ba4a980a35ebb..bafb4c3e6c174 100644
> --- a/drivers/spmi/spmi.c
> +++ b/drivers/spmi/spmi.c

[ ... ]

> @@ -429,6 +455,58 @@ struct spmi_device *spmi_device_alloc(struct spmi_controller *ctrl)
>  }
>  EXPORT_SYMBOL_GPL(spmi_device_alloc);
>  
> +/**
> + * 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 error pointer.
> + */
> +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_obj(*sub_sdev);
> +	if (!sub_sdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	sdev = &sub_sdev->sdev;
> +	sdev->ctrl = sparent->ctrl;
> +	sdev->usid = sparent->usid;
> +
> +	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;
> +
> +	ret = dev_set_name(&sdev->dev, "%u-%02x.%u.auto",
> +			   sdev->ctrl->nr, sdev->usid, sub_sdev->devid);
> +	if (ret)
> +		goto err_put_dev;
> +
> +	ret = device_add(&sdev->dev);

[Severity: High]
Does this allocation and registration sequence prevent callers from configuring
device properties before calling device_add?

Because device_initialize and device_add are conflated into a single function
without any opportunity to configure properties like of_node or fwnode, it
seems this might prevent bus matching and sysfs symlink creation.

Without an of_node, wouldn't the MFD core fail to assign DT nodes to child
cells, causing module autoloading to fail for statically named drivers?

[Severity: High]
Also, does this new subdevice properly propagate runtime PM requests?

When a child device (like an MFD cell) calls pm_runtime_get_sync, the driver
core's __pm_runtime_resume checks the parent. Since this subdevice is added
without pm_runtime_enable, its disable_depth will be 1.

Will the driver core see the disabled state, skip resuming the subdevice, and
stop propagating the resume request up to the actual parent PMIC, causing
hardware accesses to fail or time out?

Should this call pm_runtime_no_callbacks and pm_runtime_enable before
device_add?

> +	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);
> +}
> +EXPORT_SYMBOL_NS_GPL(spmi_subdevice_alloc_and_add, "SPMI");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721093626.96264-1-angelogioacchino.delregno@collabora.com?part=5