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.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/spmi/spmi-devres.c | 23 +++++++++++
 drivers/spmi/spmi.c        | 83 ++++++++++++++++++++++++++++++++++++++
 include/linux/spmi.h       | 16 ++++++++
 3 files changed, 122 insertions(+)

diff --git a/drivers/spmi/spmi-devres.c b/drivers/spmi/spmi-devres.c
index 62c4b3f24d06..7e00e38be2ff 100644
--- a/drivers/spmi/spmi-devres.c
+++ b/drivers/spmi/spmi-devres.c
@@ -60,5 +60,28 @@ 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((struct spmi_subdevice *)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");
diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
index 3cf8d9bd4566..62bb782b2bbc 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,18 @@ 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);
+
+	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 +103,19 @@ 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);
+	ida_free(&spmi_subdevice_ida, sub_sdev->devid);
+}
+EXPORT_SYMBOL_NS_GPL(spmi_subdevice_remove, "SPMI");
+
 static inline int
 spmi_cmd(struct spmi_controller *ctrl, u8 opcode, u8 sid)
 {
@@ -431,6 +457,63 @@ 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;
+
+	if (!sparent)
+		return ERR_PTR(-EINVAL);
+
+	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)
+		goto err_ida_alloc;
+
+	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_set_name;
+
+	ret = device_add(&sdev->dev);
+	if (ret) {
+		dev_err(&sdev->dev, "Can't add %s, status %d\n",
+			dev_name(&sdev->dev), ret);
+		put_device(&sdev->dev);
+		return ERR_PTR(ret);
+	}
+
+	return sub_sdev;
+
+err_set_name:
+	ida_free(&ctrl_ida, sub_sdev->devid);
+err_ida_alloc:
+	kfree(sub_sdev);
+	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.50.1

Hi AngeloGioacchino,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20250722]
[also build test ERROR on v6.16-rc7]
[cannot apply to jic23-iio/togreg sre-power-supply/for-next char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.16-rc7 v6.16-rc6 v6.16-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/AngeloGioacchino-Del-Regno/spmi-Implement-spmi_subdevice_alloc_and_add-and-devm-variant/20250722-181911
base:   next-20250722
patch link:    https://lore.kernel.org/r/20250722101317.76729-2-angelogioacchino.delregno%40collabora.com
patch subject: [PATCH v2 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant
config: riscv-randconfig-002-20250723 (https://download.01.org/0day-ci/archive/20250723/202507231731.VakdiYEM-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 853c343b45b3e83cc5eeef5a52fc8cc9d8a09252)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250723/202507231731.VakdiYEM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507231731.VakdiYEM-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: module spmi-devres uses symbol spmi_subdevice_alloc_and_add from namespace SPMI, but does not import it.
>> ERROR: modpost: module spmi-devres uses symbol spmi_subdevice_remove from namespace SPMI, but does not import it.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Hi AngeloGioacchino,

kernel test robot noticed the following build warnings:

[auto build test WARNING on next-20250722]
[also build test WARNING on v6.16-rc7]
[cannot apply to jic23-iio/togreg sre-power-supply/for-next char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus linus/master v6.16-rc7 v6.16-rc6 v6.16-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/AngeloGioacchino-Del-Regno/spmi-Implement-spmi_subdevice_alloc_and_add-and-devm-variant/20250722-181911
base:   next-20250722
patch link:    https://lore.kernel.org/r/20250722101317.76729-2-angelogioacchino.delregno%40collabora.com
patch subject: [PATCH v2 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant
config: sparc-randconfig-001-20250723 (https://download.01.org/0day-ci/archive/20250723/202507231529.OH2sdMoF-lkp@intel.com/config)
compiler: sparc-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250723/202507231529.OH2sdMoF-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507231529.OH2sdMoF-lkp@intel.com/

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> WARNING: modpost: module spmi-devres uses symbol spmi_subdevice_remove from namespace SPMI, but does not import it.
>> WARNING: modpost: module spmi-devres uses symbol spmi_subdevice_alloc_and_add from namespace SPMI, but does not import it.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On Tue, 22 Jul 2025 12:13:11 +0200
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> 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.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/spmi/spmi-devres.c | 23 +++++++++++
>  drivers/spmi/spmi.c        | 83 ++++++++++++++++++++++++++++++++++++++
>  include/linux/spmi.h       | 16 ++++++++
>  3 files changed, 122 insertions(+)
> 
> diff --git a/drivers/spmi/spmi-devres.c b/drivers/spmi/spmi-devres.c
> index 62c4b3f24d06..7e00e38be2ff 100644
> --- a/drivers/spmi/spmi-devres.c
> +++ b/drivers/spmi/spmi-devres.c
> @@ -60,5 +60,28 @@ 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((struct spmi_subdevice *)res);

Why the cast?  Implicit casts are fine for void * to any other pointer type
so
	spmi_subdevice_remove(res);
should be fine.


> +}

>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("SPMI devres helpers");
> diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
> index 3cf8d9bd4566..62bb782b2bbc 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,18 @@ 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);
> +
> +	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 +103,19 @@ 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);
> +	ida_free(&spmi_subdevice_ida, sub_sdev->devid);

Why not make the ida free part of the release? If not
the device_unregister could (I think) result in a reference
count drop and freeing of sub_sdev before you dereference it here.


> +}
> +EXPORT_SYMBOL_NS_GPL(spmi_subdevice_remove, "SPMI");
> +
>  static inline int
>  spmi_cmd(struct spmi_controller *ctrl, u8 opcode, u8 sid)
>  {
> @@ -431,6 +457,63 @@ 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;
> +
> +	if (!sparent)
> +		return ERR_PTR(-EINVAL);

Is this protecting against a real possibility? Feels like something went
very wrong if you are allocating a subdevice of 'nothing'.
If it's just defensive programming I'd drop it.

> +
> +	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)
> +		goto err_ida_alloc;
> +
> +	sdev = &sub_sdev->sdev;
> +	sdev->ctrl = sparent->ctrl;
> +	device_initialize(&sdev->dev);

Read the device_initialize() documentation for what you need to do
if an error occurs after this point. Specifically the last 'NOTE'.


> +	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_set_name;
> +
> +	ret = device_add(&sdev->dev);
> +	if (ret) {
> +		dev_err(&sdev->dev, "Can't add %s, status %d\n",
> +			dev_name(&sdev->dev), ret);
> +		put_device(&sdev->dev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return sub_sdev;
> +
> +err_set_name:
> +	ida_free(&ctrl_ida, sub_sdev->devid);
> +err_ida_alloc:
> +	kfree(sub_sdev);
> +	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;

Having something called a subdevice containing an instance of a device
does seem a little odd.  Maybe the spmi_device naming is inappropriate after
this patch?

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

Il 22/07/25 16:09, Jonathan Cameron ha scritto:
> On Tue, 22 Jul 2025 12:13:11 +0200
> AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> 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.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/spmi/spmi-devres.c | 23 +++++++++++
>>   drivers/spmi/spmi.c        | 83 ++++++++++++++++++++++++++++++++++++++
>>   include/linux/spmi.h       | 16 ++++++++
>>   3 files changed, 122 insertions(+)
>>
>> diff --git a/drivers/spmi/spmi-devres.c b/drivers/spmi/spmi-devres.c
>> index 62c4b3f24d06..7e00e38be2ff 100644
>> --- a/drivers/spmi/spmi-devres.c
>> +++ b/drivers/spmi/spmi-devres.c
>> @@ -60,5 +60,28 @@ 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((struct spmi_subdevice *)res);
> 
> Why the cast?  Implicit casts are fine for void * to any other pointer type
> so
> 	spmi_subdevice_remove(res);
> should be fine.
> 

Because style consistency across the file... but yeah, I'm removing the cast.

> 
>> +}
> 
>>   MODULE_LICENSE("GPL");
>>   MODULE_DESCRIPTION("SPMI devres helpers");
>> diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
>> index 3cf8d9bd4566..62bb782b2bbc 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,18 @@ 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);
>> +
>> +	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 +103,19 @@ 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);
>> +	ida_free(&spmi_subdevice_ida, sub_sdev->devid);
> 
> Why not make the ida free part of the release? If not
> the device_unregister could (I think) result in a reference
> count drop and freeing of sub_sdev before you dereference it here.
> 

That's right, I moved it to the release, before the kfree.

> 
>> +}
>> +EXPORT_SYMBOL_NS_GPL(spmi_subdevice_remove, "SPMI");
>> +
>>   static inline int
>>   spmi_cmd(struct spmi_controller *ctrl, u8 opcode, u8 sid)
>>   {
>> @@ -431,6 +457,63 @@ 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;
>> +
>> +	if (!sparent)
>> +		return ERR_PTR(-EINVAL);
> 
> Is this protecting against a real possibility? Feels like something went
> very wrong if you are allocating a subdevice of 'nothing'.
> If it's just defensive programming I'd drop it.
> 

That was defensive programming. Dropping.

>> +
>> +	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)
>> +		goto err_ida_alloc;
>> +
>> +	sdev = &sub_sdev->sdev;
>> +	sdev->ctrl = sparent->ctrl;
>> +	device_initialize(&sdev->dev);
> 
> Read the device_initialize() documentation for what you need to do
> if an error occurs after this point. Specifically the last 'NOTE'.
> 

Sorry. That was a bad miss :-)

> 
>> +	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_set_name;
>> +
>> +	ret = device_add(&sdev->dev);
>> +	if (ret) {
>> +		dev_err(&sdev->dev, "Can't add %s, status %d\n",
>> +			dev_name(&sdev->dev), ret);
>> +		put_device(&sdev->dev);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	return sub_sdev;
>> +
>> +err_set_name:
>> +	ida_free(&ctrl_ida, sub_sdev->devid);
>> +err_ida_alloc:
>> +	kfree(sub_sdev);
>> +	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;
> 
> Having something called a subdevice containing an instance of a device
> does seem a little odd.  Maybe the spmi_device naming is inappropriate after
> this patch?
> 

A SPMI Sub-Device is a SPMI Device on its own, but one that is child of a device.

Controller -> Device -> Sub-Device

Before this version, I initially added devid to spmi_device, but that felt wrong
because:
  1. Sub-devices are children of devices (though, still also devices themselves)
  2. The devid field would be useless in "main" SPMI devices (struct spmi_device)
     and would not only waste (a very small amount of) memory for each device but,
     more importantly, would confuse people with an unused field there.

So, this defines a SPMI Sub-Device as an extension of a SPMI Device, where:
  - Device has controller-device numbers
  - Sub-device has controller-device.subdev_id numbers.

I don't really see any cleaner way of defining this, but I am completely open to
any idea :-)

Cheers,
Angelo

> >> +/**
> >> + * 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;  
> > 
> > Having something called a subdevice containing an instance of a device
> > does seem a little odd.  Maybe the spmi_device naming is inappropriate after
> > this patch?
> >   
> 
> A SPMI Sub-Device is a SPMI Device on its own, but one that is child of a device.
> 
> Controller -> Device -> Sub-Device
> 
> Before this version, I initially added devid to spmi_device, but that felt wrong
> because:
>   1. Sub-devices are children of devices (though, still also devices themselves)
>   2. The devid field would be useless in "main" SPMI devices (struct spmi_device)
>      and would not only waste (a very small amount of) memory for each device but,
>      more importantly, would confuse people with an unused field there.
> 
> So, this defines a SPMI Sub-Device as an extension of a SPMI Device, where:
>   - Device has controller-device numbers
>   - Sub-device has controller-device.subdev_id numbers.
> 
> I don't really see any cleaner way of defining this, but I am completely open to
> any idea :-)
I was thinking it was a specialization at the same level as the old spmi_device
(not it's child). As a child this is fine.

Just showing my complete lack of knowledge of the SPMI code :)

Jonathan

> 
> Cheers,
> Angelo
> 
>