Message-ID: <6ea0495e-21d8-41a8-b1b0-1c99c2929de5@collabora.com>
Date: Tue, 29 Jul 2025 12:44:34 +0200
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: sboyd@...nel.org, jic23@...nel.org, dlechner@...libre.com,
 nuno.sa@...log.com, andy@...nel.org, arnd@...db.de,
 gregkh@...uxfoundation.org, srini@...nel.org, vkoul@...nel.org,
 kishon@...nel.org, sre@...nel.org, krzysztof.kozlowski@...aro.org,
 u.kleine-koenig@...libre.com, linux-arm-msm@...r.kernel.org,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-phy@...ts.infradead.org, linux-pm@...r.kernel.org,
 kernel@...labora.com, wenst@...omium.org, casey.connolly@...aro.org
Subject: Re: [PATCH v2 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and
 devm variant

Il 22/07/25 16:09, Jonathan Cameron ha scritto:
> On Tue, 22 Jul 2025 12:13:11 +0200
> AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.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@...labora.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

Powered by blists - more mailing lists