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

Link: https://lore.kernel.org/r/20250722101317.76729-2-angelogioacchino.delregno@collabora.com
Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Link: https://lore.kernel.org/r/20250730112645.542179-2-angelogioacchino.delregno@collabora.com
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/spmi/spmi-devres.c | 24 ++++++++++++
 drivers/spmi/spmi.c        | 79 ++++++++++++++++++++++++++++++++++++++
 include/linux/spmi.h       | 16 ++++++++
 3 files changed, 119 insertions(+)

diff --git a/drivers/spmi/spmi-devres.c b/drivers/spmi/spmi-devres.c
index 62c4b3f24d06..8feebab0365b 100644
--- a/drivers/spmi/spmi-devres.c
+++ b/drivers/spmi/spmi-devres.c
@@ -60,5 +60,29 @@ int devm_spmi_controller_add(struct device *parent, struct spmi_controller *ctrl
 }
 EXPORT_SYMBOL_GPL(devm_spmi_controller_add);
 
+static void devm_spmi_subdevice_remove(void *res)
+{
+	spmi_subdevice_remove(res);
+}
+
+struct spmi_subdevice *devm_spmi_subdevice_alloc_and_add(struct device *dev,
+							 struct spmi_device *sparent)
+{
+	struct spmi_subdevice *sub_sdev;
+	int ret;
+
+	sub_sdev = spmi_subdevice_alloc_and_add(sparent);
+	if (IS_ERR(sub_sdev))
+		return sub_sdev;
+
+	ret = devm_add_action_or_reset(dev, devm_spmi_subdevice_remove, sub_sdev);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return sub_sdev;
+}
+EXPORT_SYMBOL_NS_GPL(devm_spmi_subdevice_alloc_and_add, "SPMI");
+
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("SPMI devres helpers");
+MODULE_IMPORT_NS("SPMI");
diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
index 3cf8d9bd4566..e011876c3187 100644
--- a/drivers/spmi/spmi.c
+++ b/drivers/spmi/spmi.c
@@ -19,6 +19,7 @@
 
 static bool is_registered;
 static DEFINE_IDA(ctrl_ida);
+static DEFINE_IDA(spmi_subdevice_ida);
 
 static void spmi_dev_release(struct device *dev)
 {
@@ -31,6 +32,19 @@ static const struct device_type spmi_dev_type = {
 	.release	= spmi_dev_release,
 };
 
+static void spmi_subdev_release(struct device *dev)
+{
+	struct spmi_device *sdev = to_spmi_device(dev);
+	struct spmi_subdevice *sub_sdev = container_of(sdev, struct spmi_subdevice, sdev);
+
+	ida_free(&spmi_subdevice_ida, sub_sdev->devid);
+	kfree(sub_sdev);
+}
+
+static const struct device_type spmi_subdev_type = {
+	.release	= spmi_subdev_release,
+};
+
 static void spmi_ctrl_release(struct device *dev)
 {
 	struct spmi_controller *ctrl = to_spmi_controller(dev);
@@ -90,6 +104,18 @@ void spmi_device_remove(struct spmi_device *sdev)
 }
 EXPORT_SYMBOL_GPL(spmi_device_remove);
 
+/**
+ * spmi_subdevice_remove() - Remove an SPMI subdevice
+ * @sub_sdev:	spmi_device to be removed
+ */
+void spmi_subdevice_remove(struct spmi_subdevice *sub_sdev)
+{
+	struct spmi_device *sdev = &sub_sdev->sdev;
+
+	device_unregister(&sdev->dev);
+}
+EXPORT_SYMBOL_NS_GPL(spmi_subdevice_remove, "SPMI");
+
 static inline int
 spmi_cmd(struct spmi_controller *ctrl, u8 opcode, u8 sid)
 {
@@ -431,6 +457,59 @@ 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 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 (ret)
+		goto err_put_dev;
+
+	ret = device_add(&sdev->dev);
+	if (ret) {
+		dev_err(&sdev->dev, "Can't add %s, status %d\n",
+			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");
+
 /**
  * spmi_controller_alloc() - Allocate a new SPMI controller
  * @parent:	parent device
diff --git a/include/linux/spmi.h b/include/linux/spmi.h
index 28e8c8bd3944..7cea0a5b034b 100644
--- a/include/linux/spmi.h
+++ b/include/linux/spmi.h
@@ -69,6 +69,22 @@ int spmi_device_add(struct spmi_device *sdev);
 
 void spmi_device_remove(struct spmi_device *sdev);
 
+/**
+ * struct spmi_subdevice - Basic representation of an SPMI sub-device
+ * @sdev:	Sub-device representation of an SPMI device
+ * @devid:	Platform Device ID of an SPMI sub-device
+ */
+struct spmi_subdevice {
+	struct spmi_device	sdev;
+	unsigned int		devid;
+};
+
+struct spmi_subdevice *spmi_subdevice_alloc_and_add(struct spmi_device *sparent);
+void spmi_subdevice_remove(struct spmi_subdevice *sdev);
+
+struct spmi_subdevice *devm_spmi_subdevice_alloc_and_add(struct device *dev,
+							 struct spmi_device *sparent);
+
 /**
  * struct spmi_controller - interface to the SPMI master controller
  * @dev:	Driver model representation of the device.
-- 
2.51.0

Hi,

On Tue, Sep 16, 2025 at 10:44:39AM +0200, 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 existance 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.
> 
> Link: https://lore.kernel.org/r/20250722101317.76729-2-angelogioacchino.delregno@collabora.com
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
> Link: https://lore.kernel.org/r/20250730112645.542179-2-angelogioacchino.delregno@collabora.com
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---

It would be nice, if this patch lands in the 6.18 merge window. That
way we don't need to do an immutable branch for the different involved
subsystems.

Greetings,

-- Sebastian

>  drivers/spmi/spmi-devres.c | 24 ++++++++++++
>  drivers/spmi/spmi.c        | 79 ++++++++++++++++++++++++++++++++++++++
>  include/linux/spmi.h       | 16 ++++++++
>  3 files changed, 119 insertions(+)
> 
> diff --git a/drivers/spmi/spmi-devres.c b/drivers/spmi/spmi-devres.c
> index 62c4b3f24d06..8feebab0365b 100644
> --- a/drivers/spmi/spmi-devres.c
> +++ b/drivers/spmi/spmi-devres.c
> @@ -60,5 +60,29 @@ int devm_spmi_controller_add(struct device *parent, struct spmi_controller *ctrl
>  }
>  EXPORT_SYMBOL_GPL(devm_spmi_controller_add);
>  
> +static void devm_spmi_subdevice_remove(void *res)
> +{
> +	spmi_subdevice_remove(res);
> +}
> +
> +struct spmi_subdevice *devm_spmi_subdevice_alloc_and_add(struct device *dev,
> +							 struct spmi_device *sparent)
> +{
> +	struct spmi_subdevice *sub_sdev;
> +	int ret;
> +
> +	sub_sdev = spmi_subdevice_alloc_and_add(sparent);
> +	if (IS_ERR(sub_sdev))
> +		return sub_sdev;
> +
> +	ret = devm_add_action_or_reset(dev, devm_spmi_subdevice_remove, sub_sdev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return sub_sdev;
> +}
> +EXPORT_SYMBOL_NS_GPL(devm_spmi_subdevice_alloc_and_add, "SPMI");
> +
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("SPMI devres helpers");
> +MODULE_IMPORT_NS("SPMI");
> diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
> index 3cf8d9bd4566..e011876c3187 100644
> --- a/drivers/spmi/spmi.c
> +++ b/drivers/spmi/spmi.c
> @@ -19,6 +19,7 @@
>  
>  static bool is_registered;
>  static DEFINE_IDA(ctrl_ida);
> +static DEFINE_IDA(spmi_subdevice_ida);
>  
>  static void spmi_dev_release(struct device *dev)
>  {
> @@ -31,6 +32,19 @@ static const struct device_type spmi_dev_type = {
>  	.release	= spmi_dev_release,
>  };
>  
> +static void spmi_subdev_release(struct device *dev)
> +{
> +	struct spmi_device *sdev = to_spmi_device(dev);
> +	struct spmi_subdevice *sub_sdev = container_of(sdev, struct spmi_subdevice, sdev);
> +
> +	ida_free(&spmi_subdevice_ida, sub_sdev->devid);
> +	kfree(sub_sdev);
> +}
> +
> +static const struct device_type spmi_subdev_type = {
> +	.release	= spmi_subdev_release,
> +};
> +
>  static void spmi_ctrl_release(struct device *dev)
>  {
>  	struct spmi_controller *ctrl = to_spmi_controller(dev);
> @@ -90,6 +104,18 @@ void spmi_device_remove(struct spmi_device *sdev)
>  }
>  EXPORT_SYMBOL_GPL(spmi_device_remove);
>  
> +/**
> + * spmi_subdevice_remove() - Remove an SPMI subdevice
> + * @sub_sdev:	spmi_device to be removed
> + */
> +void spmi_subdevice_remove(struct spmi_subdevice *sub_sdev)
> +{
> +	struct spmi_device *sdev = &sub_sdev->sdev;
> +
> +	device_unregister(&sdev->dev);
> +}
> +EXPORT_SYMBOL_NS_GPL(spmi_subdevice_remove, "SPMI");
> +
>  static inline int
>  spmi_cmd(struct spmi_controller *ctrl, u8 opcode, u8 sid)
>  {
> @@ -431,6 +457,59 @@ 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 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 (ret)
> +		goto err_put_dev;
> +
> +	ret = device_add(&sdev->dev);
> +	if (ret) {
> +		dev_err(&sdev->dev, "Can't add %s, status %d\n",
> +			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");
> +
>  /**
>   * spmi_controller_alloc() - Allocate a new SPMI controller
>   * @parent:	parent device
> diff --git a/include/linux/spmi.h b/include/linux/spmi.h
> index 28e8c8bd3944..7cea0a5b034b 100644
> --- a/include/linux/spmi.h
> +++ b/include/linux/spmi.h
> @@ -69,6 +69,22 @@ int spmi_device_add(struct spmi_device *sdev);
>  
>  void spmi_device_remove(struct spmi_device *sdev);
>  
> +/**
> + * struct spmi_subdevice - Basic representation of an SPMI sub-device
> + * @sdev:	Sub-device representation of an SPMI device
> + * @devid:	Platform Device ID of an SPMI sub-device
> + */
> +struct spmi_subdevice {
> +	struct spmi_device	sdev;
> +	unsigned int		devid;
> +};
> +
> +struct spmi_subdevice *spmi_subdevice_alloc_and_add(struct spmi_device *sparent);
> +void spmi_subdevice_remove(struct spmi_subdevice *sdev);
> +
> +struct spmi_subdevice *devm_spmi_subdevice_alloc_and_add(struct device *dev,
> +							 struct spmi_device *sparent);
> +
>  /**
>   * struct spmi_controller - interface to the SPMI master controller
>   * @dev:	Driver model representation of the device.
> -- 
> 2.51.0
> 

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?!

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

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

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

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

Il 17/09/25 16:57, Uwe Kleine-König ha scritto:
> 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.
> 

That gives a clear representation of the tree of devices on a SPMI bus, more
or less like it's done for some other addressable (or discoverable) busses
where you may have a tree like controller->hub->device (just as a fast example eh).

The SPMI devices are anyway already following this naming even before my changes,
as those are simply "%d-%02x" (ctrlid-devaddr), so what I wrote here is aimed to
  1. Not reinvent the wheel
  2. Be consistent with previous naming
  3. Be nice to whoever is trying to understand "where" a device is
     3a. ...And make it immediately easy to see that
     3b. ...And make it easier to debug, in case it's needed
  4. Not exclude a possible future where SPMI may become discoverable somehow
     (...which is questionably kind of a thing already, but then for multiple
      reasons it's not really feasible right now)

...I would be able to go on with more reasons, but let's not open this loophole :-)

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

That'd be right, but %pe being better is (imo..) just an opinion and, while I agree
with you in that it's nicer, it'd be great if this doesn't become a blocker and if
we could get this patch picked in the current merge window.

This is because otherwise the other patches (of which, two are not yet ready as
they need some Kconfig fixes) would need immutable branches between multiple
trees (as those are touching nvmem, power, phy, misc and iio), and that would
get a bit complicated.

Of course while resending the other patches, I can add a %pe conversion for the
whole SPMI framework code. One more line isn't the end of the world anyway.

Mind you - the main reason for this patch is that I am using it for new drivers
for MediaTek PMICs (and a SPMI v2 bus implementation for the new MTK SPMI Arbiter)
that I will introduce after this gets picked.

Cheers,
Angelo