Re: [PATCH v2 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant
From: AngeloGioacchino Del Regno
Date: Tue Jul 29 2025 - 06:45:04 EST
Il 22/07/25 16:09, Jonathan Cameron ha scritto:
On Tue, 22 Jul 2025 12:13:11 +0200
AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
---
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