[PATCH v4 0/7] SPMI: Implement sub-devices and migrate drivers

[PATCH v4 0/7] SPMI: Implement sub-devices and migrate drivers

[AngeloGioacchino Del Regno]

Changes in v4:
 - Added selection of REGMAP_SPMI in Kconfig for qcom-coincell and
   for phy-qcom-eusb2-repeater to resolve undefined references when
   compiled with some randconfig

Changes in v3:
 - Fixed importing "SPMI" namespace in spmi-devres.c
 - Removed all instances of defensive programming, as pointed out by
   jic23 and Sebastian
 - Removed explicit casting as pointed out by jic23
 - Moved ida_free call to spmi_subdev_release() and simplified error
   handling in spmi_subdevice_alloc_and_add() as pointed out by jic23

Changes in v2:
 - Fixed missing `sparent` initialization in phy-qcom-eusb2-repeater
 - Changed val_bits to 8 in all Qualcomm drivers to ensure
   compatibility as suggested by Casey
 - Added struct device pointer in all conversion commits as suggested
   by Andy
 - Exported newly introduced functions with a new "SPMI" namespace
   and imported the same in all converted drivers as suggested by Andy
 - Added missing error checking for dev_set_name() call in spmi.c
   as suggested by Andy
 - Added comma to last entry of regmap_config as suggested by Andy

While adding support for newer MediaTek platforms, featuring complex
SPMI PMICs, I've seen that those SPMI-connected chips are internally
divided in various IP blocks, reachable in specific contiguous address
ranges... more or less like a MMIO, but over a slow SPMI bus instead.

I recalled that Qualcomm had something similar... and upon checking a
couple of devicetrees, yeah - indeed it's the same over there.

What I've seen then is a common pattern of reading the "reg" property
from devicetree in a struct member and then either
 A. Wrapping regmap_{read/write/etc}() calls in a function that adds
    the register base with "base + ..register", like it's done with
    writel()/readl() calls; or
 B. Doing the same as A. but without wrapper functions.

Even though that works just fine, in my opinion it's wrong.

The regmap API is way more complex than MMIO-only readl()/writel()
functions for multiple reasons (including supporting multiple busses
like SPMI, of course) - but everyone seemed to forget that regmap
can manage register base offsets transparently and automatically in
its API functions by simply adding a `reg_base` to the regmap_config
structure, which is used for initializing a `struct regmap`.

So, here we go: this series implements the software concept of an SPMI
Sub-Device (which, well, also reflects how Qualcomm and MediaTek's
actual hardware is laid out anyway).

               SPMI Controller
                     |                ______
                     |               /       Sub-Device 1
                     V              /
              SPMI Device (PMIC) ----------- Sub-Device 2
                                    \
                                     \______ Sub-Device 3

As per this implementation, an SPMI Sub-Device can be allocated/created
and added in any driver that implements a... well.. subdevice (!) with
an SPMI "main" device as its parent: this allows to create and finally
to correctly configure a regmap that is specific to the sub-device,
operating on its specific address range and reading, and writing, to
its registers with the regmap API taking care of adding the base address
of a sub-device's registers as per regmap API design.

All of the SPMI Sub-Devices are therefore added as children of the SPMI
Device (usually a PMIC), as communication depends on the PMIC's SPMI bus
to be available (and the PMIC to be up and running, of course).

Summarizing the dependency chain (which is obvious to whoever knows what
is going on with Qualcomm and/or MediaTek SPMI PMICs):
    "SPMI Sub-Device x...N" are children "SPMI Device"
    "SPMI Device" is a child of "SPMI Controller"

(that was just another way to say the same thing as the graph above anyway).

Along with the new SPMI Sub-Device registration functions, I have also
performed a conversion of some Qualcomm SPMI drivers and only where the
actual conversion was trivial.

I haven't included any conversion of more complex Qualcomm SPMI drivers
because I don't have the required bandwidth to do so (and besides, I think,
but haven't exactly verified, that some of those require SoCs that I don't
have for testing anyway).

AngeloGioacchino Del Regno (7):
  spmi: Implement spmi_subdevice_alloc_and_add() and devm variant
  nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()
  power: reset: qcom-pon: Migrate to devm_spmi_subdevice_alloc_and_add()
  phy: qualcomm: eusb2-repeater: Migrate to
    devm_spmi_subdevice_alloc_and_add()
  misc: qcom-coincell: Migrate to devm_spmi_subdevice_alloc_and_add()
  iio: adc: qcom-spmi-iadc: Migrate to
    devm_spmi_subdevice_alloc_and_add()
  iio: adc: qcom-spmi-iadc: Remove regmap R/W wrapper functions

 drivers/iio/adc/qcom-spmi-iadc.c              | 109 ++++++++----------
 drivers/misc/Kconfig                          |   1 +
 drivers/misc/qcom-coincell.c                  |  38 ++++--
 drivers/nvmem/qcom-spmi-sdam.c                |  37 ++++--
 drivers/phy/qualcomm/Kconfig                  |   1 +
 .../phy/qualcomm/phy-qcom-eusb2-repeater.c    |  53 ++++++---
 drivers/power/reset/qcom-pon.c                |  34 ++++--
 drivers/spmi/spmi-devres.c                    |  24 ++++
 drivers/spmi/spmi.c                           |  79 +++++++++++++
 include/linux/spmi.h                          |  16 +++
 10 files changed, 281 insertions(+), 111 deletions(-)

-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v4 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[AngeloGioacchino Del Regno]

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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[AngeloGioacchino Del Regno]

Some Qualcomm PMICs integrate a SDAM device, internally located in
a specific address range reachable through SPMI communication.

Instead of using the parent SPMI device (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device for SDAM
and initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.

This allows to stop manually adding the register base address to
every R/W call in this driver, as this can be, and is now, handled
by the regmap API instead.

Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Link: https://lore.kernel.org/r/20250722101317.76729-3-angelogioacchino.delregno@collabora.com
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Link: https://lore.kernel.org/r/20250730112645.542179-3-angelogioacchino.delregno@collabora.com
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/nvmem/qcom-spmi-sdam.c | 37 ++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/nvmem/qcom-spmi-sdam.c b/drivers/nvmem/qcom-spmi-sdam.c
index 4f1cca6eab71..9a4be20dfa9f 100644
--- a/drivers/nvmem/qcom-spmi-sdam.c
+++ b/drivers/nvmem/qcom-spmi-sdam.c
@@ -9,6 +9,7 @@
 #include <linux/nvmem-provider.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/spmi.h>
 
 #define SDAM_MEM_START			0x40
 #define REGISTER_MAP_ID			0x40
@@ -20,7 +21,6 @@
 struct sdam_chip {
 	struct regmap			*regmap;
 	struct nvmem_config		sdam_config;
-	unsigned int			base;
 	unsigned int			size;
 };
 
@@ -73,7 +73,7 @@ static int sdam_read(void *priv, unsigned int offset, void *val,
 		return -EINVAL;
 	}
 
-	rc = regmap_bulk_read(sdam->regmap, sdam->base + offset, val, bytes);
+	rc = regmap_bulk_read(sdam->regmap, offset, val, bytes);
 	if (rc < 0)
 		dev_err(dev, "Failed to read SDAM offset %#x len=%zd, rc=%d\n",
 						offset, bytes, rc);
@@ -100,7 +100,7 @@ static int sdam_write(void *priv, unsigned int offset, void *val,
 		return -EINVAL;
 	}
 
-	rc = regmap_bulk_write(sdam->regmap, sdam->base + offset, val, bytes);
+	rc = regmap_bulk_write(sdam->regmap, offset, val, bytes);
 	if (rc < 0)
 		dev_err(dev, "Failed to write SDAM offset %#x len=%zd, rc=%d\n",
 						offset, bytes, rc);
@@ -110,8 +110,17 @@ static int sdam_write(void *priv, unsigned int offset, void *val,
 
 static int sdam_probe(struct platform_device *pdev)
 {
+	struct regmap_config sdam_regmap_config = {
+		.reg_bits = 16,
+		.val_bits = 8,
+		.max_register = 0x100,
+		.fast_io = true,
+	};
 	struct sdam_chip *sdam;
 	struct nvmem_device *nvmem;
+	struct spmi_device *sparent;
+	struct spmi_subdevice *sub_sdev;
+	struct device *dev = &pdev->dev;
 	unsigned int val;
 	int rc;
 
@@ -119,19 +128,24 @@ static int sdam_probe(struct platform_device *pdev)
 	if (!sdam)
 		return -ENOMEM;
 
-	sdam->regmap = dev_get_regmap(pdev->dev.parent, NULL);
-	if (!sdam->regmap) {
-		dev_err(&pdev->dev, "Failed to get regmap handle\n");
-		return -ENXIO;
-	}
+	sparent = to_spmi_device(dev->parent);
+	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
+	if (IS_ERR(sub_sdev))
+		return PTR_ERR(sub_sdev);
 
-	rc = of_property_read_u32(pdev->dev.of_node, "reg", &sdam->base);
+	rc = of_property_read_u32(dev->of_node, "reg", &sdam_regmap_config.reg_base);
 	if (rc < 0) {
 		dev_err(&pdev->dev, "Failed to get SDAM base, rc=%d\n", rc);
 		return -EINVAL;
 	}
 
-	rc = regmap_read(sdam->regmap, sdam->base + SDAM_SIZE, &val);
+	sdam->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &sdam_regmap_config);
+	if (IS_ERR(sdam->regmap)) {
+		dev_err(&pdev->dev, "Failed to get regmap handle\n");
+		return PTR_ERR(sdam->regmap);
+	}
+
+	rc = regmap_read(sdam->regmap, SDAM_SIZE, &val);
 	if (rc < 0) {
 		dev_err(&pdev->dev, "Failed to read SDAM_SIZE rc=%d\n", rc);
 		return -EINVAL;
@@ -159,7 +173,7 @@ static int sdam_probe(struct platform_device *pdev)
 	}
 	dev_dbg(&pdev->dev,
 		"SDAM base=%#x size=%u registered successfully\n",
-		sdam->base, sdam->size);
+		sdam_regmap_config.reg_base, sdam->size);
 
 	return 0;
 }
@@ -181,3 +195,4 @@ module_platform_driver(sdam_driver);
 
 MODULE_DESCRIPTION("QCOM SPMI SDAM driver");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS("SPMI");
-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v4 3/7] power: reset: qcom-pon: Migrate to devm_spmi_subdevice_alloc_and_add()

[AngeloGioacchino Del Regno]

Some Qualcomm PMICs integrates a Power On device supporting pwrkey
and resin along with the Android reboot reason action identifier.

Instead of using the parent SPMI device (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device for PON
and initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.

This allows to stop manually adding the register base address to
every R/W call in this driver, as this can be, and is now, handled
by the regmap API instead.

Link: https://lore.kernel.org/r/20250722101317.76729-4-angelogioacchino.delregno@collabora.com
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Link: https://lore.kernel.org/r/20250730112645.542179-4-angelogioacchino.delregno@collabora.com
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/power/reset/qcom-pon.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/power/reset/qcom-pon.c b/drivers/power/reset/qcom-pon.c
index 7e108982a582..0e075a2e5e48 100644
--- a/drivers/power/reset/qcom-pon.c
+++ b/drivers/power/reset/qcom-pon.c
@@ -11,6 +11,7 @@
 #include <linux/reboot.h>
 #include <linux/reboot-mode.h>
 #include <linux/regmap.h>
+#include <linux/spmi.h>
 
 #define PON_SOFT_RB_SPARE		0x8f
 
@@ -22,7 +23,6 @@
 struct qcom_pon {
 	struct device *dev;
 	struct regmap *regmap;
-	u32 baseaddr;
 	struct reboot_mode_driver reboot_mode;
 	long reason_shift;
 };
@@ -35,7 +35,7 @@ static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot,
 	int ret;
 
 	ret = regmap_update_bits(pon->regmap,
-				 pon->baseaddr + PON_SOFT_RB_SPARE,
+				 PON_SOFT_RB_SPARE,
 				 GENMASK(7, pon->reason_shift),
 				 magic << pon->reason_shift);
 	if (ret < 0)
@@ -46,27 +46,42 @@ static int qcom_pon_reboot_mode_write(struct reboot_mode_driver *reboot,
 
 static int qcom_pon_probe(struct platform_device *pdev)
 {
+	struct regmap_config qcom_pon_regmap_config = {
+		.reg_bits = 16,
+		.val_bits = 8,
+		.max_register = 0x100,
+		.fast_io = true,
+	};
+	struct device *dev = &pdev->dev;
+	struct spmi_subdevice *sub_sdev;
+	struct spmi_device *sparent;
 	struct qcom_pon *pon;
 	long reason_shift;
 	int error;
 
+	if (!dev->parent)
+		return -ENODEV;
+
 	pon = devm_kzalloc(&pdev->dev, sizeof(*pon), GFP_KERNEL);
 	if (!pon)
 		return -ENOMEM;
 
 	pon->dev = &pdev->dev;
 
-	pon->regmap = dev_get_regmap(pdev->dev.parent, NULL);
-	if (!pon->regmap) {
-		dev_err(&pdev->dev, "failed to locate regmap\n");
-		return -ENODEV;
-	}
+	sparent = to_spmi_device(dev->parent);
+	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
+	if (IS_ERR(sub_sdev))
+		return PTR_ERR(sub_sdev);
 
-	error = of_property_read_u32(pdev->dev.of_node, "reg",
-				     &pon->baseaddr);
+	error = of_property_read_u32(dev->of_node, "reg",
+				     &qcom_pon_regmap_config.reg_base);
 	if (error)
 		return error;
 
+	pon->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &qcom_pon_regmap_config);
+	if (IS_ERR(pon->regmap))
+		return PTR_ERR(pon->regmap);
+
 	reason_shift = (long)of_device_get_match_data(&pdev->dev);
 
 	if (reason_shift != NO_REASON_SHIFT) {
@@ -106,3 +121,4 @@ module_platform_driver(qcom_pon_driver);
 
 MODULE_DESCRIPTION("Qualcomm Power On driver");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS("SPMI");
-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v4 4/7] phy: qualcomm: eusb2-repeater: Migrate to devm_spmi_subdevice_alloc_and_add()

[AngeloGioacchino Del Regno]

Some Qualcomm PMICs integrate an USB Repeater device, used to
convert between eUSB2 and USB 2.0 signaling levels, reachable
in a specific address range over SPMI.

Instead of using the parent SPMI device (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device for EUSB2
and initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.

This allows to stop manually adding the register base address to
every R/W call in this driver, as this can be, and is now, handled
by the regmap API instead.

Link: https://lore.kernel.org/r/20250722101317.76729-5-angelogioacchino.delregno@collabora.com
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Link: https://lore.kernel.org/r/20250730112645.542179-5-angelogioacchino.delregno@collabora.com
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/phy/qualcomm/Kconfig                  |  1 +
 .../phy/qualcomm/phy-qcom-eusb2-repeater.c    | 53 ++++++++++++-------
 2 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
index 60a0ead127fa..b248b7d44d3d 100644
--- a/drivers/phy/qualcomm/Kconfig
+++ b/drivers/phy/qualcomm/Kconfig
@@ -129,6 +129,7 @@ config PHY_QCOM_EUSB2_REPEATER
 	tristate "Qualcomm PMIC eUSB2 Repeater Driver"
 	depends on OF && (ARCH_QCOM || COMPILE_TEST)
 	select GENERIC_PHY
+	select REGMAP_SPMI
 	help
 	  Enable support for the USB high-speed eUSB2 repeater on Qualcomm
 	  PMICs. The repeater is paired with a Synopsys or M31 eUSB2 Phy
diff --git a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
index 651a12b59bc8..bdeb3c72d26a 100644
--- a/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
+++ b/drivers/phy/qualcomm/phy-qcom-eusb2-repeater.c
@@ -9,6 +9,7 @@
 #include <linux/regmap.h>
 #include <linux/of.h>
 #include <linux/phy/phy.h>
+#include <linux/spmi.h>
 
 /* eUSB2 status registers */
 #define EUSB2_RPTR_STATUS		0x08
@@ -55,7 +56,6 @@ struct eusb2_repeater {
 	struct phy *phy;
 	struct regulator_bulk_data *vregs;
 	const struct eusb2_repeater_cfg *cfg;
-	u32 base;
 	enum phy_mode mode;
 };
 
@@ -118,7 +118,6 @@ static int eusb2_repeater_init(struct phy *phy)
 	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
 	struct device_node *np = rptr->dev->of_node;
 	struct regmap *regmap = rptr->regmap;
-	u32 base = rptr->base;
 	u32 poll_val;
 	int ret;
 	u8 val;
@@ -127,28 +126,28 @@ static int eusb2_repeater_init(struct phy *phy)
 	if (ret)
 		return ret;
 
-	regmap_write(regmap, base + EUSB2_EN_CTL1, EUSB2_RPTR_EN);
+	regmap_write(regmap, EUSB2_EN_CTL1, EUSB2_RPTR_EN);
 
 	/* Write registers from init table */
 	for (int i = 0; i < rptr->cfg->init_tbl_num; i++)
-		regmap_write(regmap, base + rptr->cfg->init_tbl[i].reg,
+		regmap_write(regmap, rptr->cfg->init_tbl[i].reg,
 			     rptr->cfg->init_tbl[i].value);
 
 	/* Override registers from devicetree values */
 	if (!of_property_read_u8(np, "qcom,tune-usb2-preem", &val))
-		regmap_write(regmap, base + EUSB2_TUNE_USB2_PREEM, val);
+		regmap_write(regmap, EUSB2_TUNE_USB2_PREEM, val);
 
 	if (!of_property_read_u8(np, "qcom,tune-usb2-disc-thres", &val))
-		regmap_write(regmap, base + EUSB2_TUNE_HSDISC, val);
+		regmap_write(regmap, EUSB2_TUNE_HSDISC, val);
 
 	if (!of_property_read_u8(np, "qcom,tune-usb2-amplitude", &val))
-		regmap_write(regmap, base + EUSB2_TUNE_IUSB2, val);
+		regmap_write(regmap, EUSB2_TUNE_IUSB2, val);
 
 	if (!of_property_read_u8(np, "qcom,tune-res-fsdif", &val))
-		regmap_write(regmap, base + EUSB2_TUNE_RES_FSDIF, val);
+		regmap_write(regmap, EUSB2_TUNE_RES_FSDIF, val);
 
 	/* Wait for status OK */
-	ret = regmap_read_poll_timeout(regmap, base + EUSB2_RPTR_STATUS, poll_val,
+	ret = regmap_read_poll_timeout(regmap, EUSB2_RPTR_STATUS, poll_val,
 				       poll_val & RPTR_OK, 10, 5);
 	if (ret)
 		dev_err(rptr->dev, "initialization timed-out\n");
@@ -161,7 +160,6 @@ static int eusb2_repeater_set_mode(struct phy *phy,
 {
 	struct eusb2_repeater *rptr = phy_get_drvdata(phy);
 	struct regmap *regmap = rptr->regmap;
-	u32 base = rptr->base;
 
 	switch (mode) {
 	case PHY_MODE_USB_HOST:
@@ -170,8 +168,8 @@ static int eusb2_repeater_set_mode(struct phy *phy,
 		 * per eUSB 1.2 Spec. Below implement software workaround until
 		 * PHY and controller is fixing seen observation.
 		 */
-		regmap_write(regmap, base + EUSB2_FORCE_EN_5, F_CLK_19P2M_EN);
-		regmap_write(regmap, base + EUSB2_FORCE_VAL_5, V_CLK_19P2M_EN);
+		regmap_write(regmap, EUSB2_FORCE_EN_5, F_CLK_19P2M_EN);
+		regmap_write(regmap, EUSB2_FORCE_VAL_5, V_CLK_19P2M_EN);
 		break;
 	case PHY_MODE_USB_DEVICE:
 		/*
@@ -180,8 +178,8 @@ static int eusb2_repeater_set_mode(struct phy *phy,
 		 * repeater doesn't clear previous value due to shared
 		 * regulators (say host <-> device mode switch).
 		 */
-		regmap_write(regmap, base + EUSB2_FORCE_EN_5, 0);
-		regmap_write(regmap, base + EUSB2_FORCE_VAL_5, 0);
+		regmap_write(regmap, EUSB2_FORCE_EN_5, 0);
+		regmap_write(regmap, EUSB2_FORCE_VAL_5, 0);
 		break;
 	default:
 		return -EINVAL;
@@ -206,13 +204,23 @@ static const struct phy_ops eusb2_repeater_ops = {
 
 static int eusb2_repeater_probe(struct platform_device *pdev)
 {
+	struct regmap_config eusb2_regmap_config = {
+		.reg_bits = 16,
+		.val_bits = 8,
+		.max_register = 0x100,
+		.fast_io = true,
+	};
+	struct spmi_device *sparent;
 	struct eusb2_repeater *rptr;
+	struct spmi_subdevice *sub_sdev;
 	struct device *dev = &pdev->dev;
 	struct phy_provider *phy_provider;
 	struct device_node *np = dev->of_node;
-	u32 res;
 	int ret;
 
+	if (!dev->parent)
+		return -ENODEV;
+
 	rptr = devm_kzalloc(dev, sizeof(*rptr), GFP_KERNEL);
 	if (!rptr)
 		return -ENOMEM;
@@ -224,15 +232,21 @@ static int eusb2_repeater_probe(struct platform_device *pdev)
 	if (!rptr->cfg)
 		return -EINVAL;
 
-	rptr->regmap = dev_get_regmap(dev->parent, NULL);
-	if (!rptr->regmap)
+	sparent = to_spmi_device(dev->parent);
+	if (!sparent)
 		return -ENODEV;
 
-	ret = of_property_read_u32(np, "reg", &res);
+	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
+	if (IS_ERR(sub_sdev))
+		return PTR_ERR(sub_sdev);
+
+	ret = of_property_read_u32(np, "reg", &eusb2_regmap_config.reg_base);
 	if (ret < 0)
 		return ret;
 
-	rptr->base = res;
+	rptr->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &eusb2_regmap_config);
+	if (IS_ERR(rptr->regmap))
+		return -ENODEV;
 
 	ret = eusb2_repeater_init_vregs(rptr);
 	if (ret < 0) {
@@ -295,3 +309,4 @@ module_platform_driver(eusb2_repeater_driver);
 
 MODULE_DESCRIPTION("Qualcomm PMIC eUSB2 Repeater driver");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("SPMI");
-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v4 5/7] misc: qcom-coincell: Migrate to devm_spmi_subdevice_alloc_and_add()

[AngeloGioacchino Del Regno]

Some Qualcomm PMICs integrate a charger for coincells, usually
powering an RTC when external (or main battery) power is missing.

Instead of using the parent SPMI device (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device and
initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.

This allows to stop manually adding the register base address to
every R/W call in this driver, as this can be, and is now, handled
by the regmap API instead.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Link: https://lore.kernel.org/r/20250722101317.76729-6-angelogioacchino.delregno@collabora.com
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Link: https://lore.kernel.org/r/20250730112645.542179-6-angelogioacchino.delregno@collabora.com
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/misc/Kconfig         |  1 +
 drivers/misc/qcom-coincell.c | 38 +++++++++++++++++++++++++-----------
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b9c11f67315f..6bc14ae52ecb 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -291,6 +291,7 @@ config HP_ILO
 config QCOM_COINCELL
 	tristate "Qualcomm coincell charger support"
 	depends on MFD_SPMI_PMIC || COMPILE_TEST
+	select REGMAP_SPMI
 	help
 	  This driver supports the coincell block found inside of
 	  Qualcomm PMICs.  The coincell charger provides a means to
diff --git a/drivers/misc/qcom-coincell.c b/drivers/misc/qcom-coincell.c
index 3c57f7429147..49e38442b289 100644
--- a/drivers/misc/qcom-coincell.c
+++ b/drivers/misc/qcom-coincell.c
@@ -9,11 +9,11 @@
 #include <linux/of.h>
 #include <linux/regmap.h>
 #include <linux/platform_device.h>
+#include <linux/spmi.h>
 
 struct qcom_coincell {
 	struct device	*dev;
 	struct regmap	*regmap;
-	u32		base_addr;
 };
 
 #define QCOM_COINCELL_REG_RSET		0x44
@@ -35,7 +35,7 @@ static int qcom_coincell_chgr_config(struct qcom_coincell *chgr, int rset,
 	/* if disabling, just do that and skip other operations */
 	if (!enable)
 		return regmap_write(chgr->regmap,
-			  chgr->base_addr + QCOM_COINCELL_REG_ENABLE, 0);
+			  QCOM_COINCELL_REG_ENABLE, 0);
 
 	/* find index for current-limiting resistor */
 	for (i = 0; i < ARRAY_SIZE(qcom_rset_map); i++)
@@ -58,7 +58,7 @@ static int qcom_coincell_chgr_config(struct qcom_coincell *chgr, int rset,
 	}
 
 	rc = regmap_write(chgr->regmap,
-			  chgr->base_addr + QCOM_COINCELL_REG_RSET, i);
+			  QCOM_COINCELL_REG_RSET, i);
 	if (rc) {
 		/*
 		 * This is mainly to flag a bad base_addr (reg) from dts.
@@ -71,19 +71,28 @@ static int qcom_coincell_chgr_config(struct qcom_coincell *chgr, int rset,
 	}
 
 	rc = regmap_write(chgr->regmap,
-		chgr->base_addr + QCOM_COINCELL_REG_VSET, j);
+		QCOM_COINCELL_REG_VSET, j);
 	if (rc)
 		return rc;
 
 	/* set 'enable' register */
 	return regmap_write(chgr->regmap,
-			    chgr->base_addr + QCOM_COINCELL_REG_ENABLE,
+			    QCOM_COINCELL_REG_ENABLE,
 			    QCOM_COINCELL_ENABLE);
 }
 
 static int qcom_coincell_probe(struct platform_device *pdev)
 {
-	struct device_node *node = pdev->dev.of_node;
+	struct regmap_config qcom_coincell_regmap_config = {
+		.reg_bits = 16,
+		.val_bits = 8,
+		.max_register = 0x100,
+		.fast_io = true,
+	};
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct spmi_subdevice *sub_sdev;
+	struct spmi_device *sparent;
 	struct qcom_coincell chgr;
 	u32 rset = 0;
 	u32 vset = 0;
@@ -92,16 +101,22 @@ static int qcom_coincell_probe(struct platform_device *pdev)
 
 	chgr.dev = &pdev->dev;
 
-	chgr.regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	rc = of_property_read_u32(node, "reg", &qcom_coincell_regmap_config.reg_base);
+	if (rc)
+		return rc;
+
+	sparent = to_spmi_device(dev->parent);
+	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
+	if (IS_ERR(sub_sdev))
+		return PTR_ERR(sub_sdev);
+
+	chgr.regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev,
+						&qcom_coincell_regmap_config);
 	if (!chgr.regmap) {
 		dev_err(chgr.dev, "Unable to get regmap\n");
 		return -EINVAL;
 	}
 
-	rc = of_property_read_u32(node, "reg", &chgr.base_addr);
-	if (rc)
-		return rc;
-
 	enable = !of_property_read_bool(node, "qcom,charger-disable");
 
 	if (enable) {
@@ -142,3 +157,4 @@ module_platform_driver(qcom_coincell_driver);
 
 MODULE_DESCRIPTION("Qualcomm PMIC coincell charger driver");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS("SPMI");
-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v4 6/7] iio: adc: qcom-spmi-iadc: Migrate to devm_spmi_subdevice_alloc_and_add()

[AngeloGioacchino Del Regno]

Some Qualcomm PMICs integrate an Current ADC device, reachable
in a specific address range over SPMI.

Instead of using the parent SPMI device (the main PMIC) as a kind
of syscon in this driver, register a new SPMI sub-device and
initialize its own regmap with this sub-device's specific base
address, retrieved from the devicetree.

This allows to stop manually adding the register base address to
every R/W call in this driver, as this can be, and is now, handled
by the regmap API instead.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Link: https://lore.kernel.org/r/20250722101317.76729-7-angelogioacchino.delregno@collabora.com
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Link: https://lore.kernel.org/r/20250730112645.542179-7-angelogioacchino.delregno@collabora.com
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iio/adc/qcom-spmi-iadc.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/qcom-spmi-iadc.c b/drivers/iio/adc/qcom-spmi-iadc.c
index b64a8a407168..67096952b229 100644
--- a/drivers/iio/adc/qcom-spmi-iadc.c
+++ b/drivers/iio/adc/qcom-spmi-iadc.c
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/spmi.h>
 
 /* IADC register and bit definition */
 #define IADC_REVISION2				0x1
@@ -94,7 +95,6 @@
  * struct iadc_chip - IADC Current ADC device structure.
  * @regmap: regmap for register read/write.
  * @dev: This device pointer.
- * @base: base offset for the ADC peripheral.
  * @rsense: Values of the internal and external sense resister in micro Ohms.
  * @poll_eoc: Poll for end of conversion instead of waiting for IRQ.
  * @offset: Raw offset values for the internal and external channels.
@@ -105,7 +105,6 @@
 struct iadc_chip {
 	struct regmap	*regmap;
 	struct device	*dev;
-	u16		base;
 	bool		poll_eoc;
 	u32		rsense[2];
 	u16		offset[2];
@@ -119,7 +118,7 @@ static int iadc_read(struct iadc_chip *iadc, u16 offset, u8 *data)
 	unsigned int val;
 	int ret;
 
-	ret = regmap_read(iadc->regmap, iadc->base + offset, &val);
+	ret = regmap_read(iadc->regmap, offset, &val);
 	if (ret < 0)
 		return ret;
 
@@ -129,7 +128,7 @@ static int iadc_read(struct iadc_chip *iadc, u16 offset, u8 *data)
 
 static int iadc_write(struct iadc_chip *iadc, u16 offset, u8 data)
 {
-	return regmap_write(iadc->regmap, iadc->base + offset, data);
+	return regmap_write(iadc->regmap, offset, data);
 }
 
 static int iadc_reset(struct iadc_chip *iadc)
@@ -270,7 +269,7 @@ static int iadc_poll_wait_eoc(struct iadc_chip *iadc, unsigned int interval_us)
 
 static int iadc_read_result(struct iadc_chip *iadc, u16 *data)
 {
-	return regmap_bulk_read(iadc->regmap, iadc->base + IADC_DATA, data, 2);
+	return regmap_bulk_read(iadc->regmap, IADC_DATA, data, 2);
 }
 
 static int iadc_do_conversion(struct iadc_chip *iadc, int chan, u16 *data)
@@ -483,12 +482,19 @@ static const struct iio_chan_spec iadc_channels[] = {
 
 static int iadc_probe(struct platform_device *pdev)
 {
+	struct regmap_config iadc_regmap_config = {
+		.reg_bits = 16,
+		.val_bits = 8,
+		.max_register = 0x100,
+		.fast_io = true,
+	};
 	struct device_node *node = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
+	struct spmi_subdevice *sub_sdev;
+	struct spmi_device *sparent;
 	struct iio_dev *indio_dev;
 	struct iadc_chip *iadc;
 	int ret, irq_eoc;
-	u32 res;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*iadc));
 	if (!indio_dev)
@@ -497,18 +503,21 @@ static int iadc_probe(struct platform_device *pdev)
 	iadc = iio_priv(indio_dev);
 	iadc->dev = dev;
 
-	iadc->regmap = dev_get_regmap(dev->parent, NULL);
-	if (!iadc->regmap)
-		return -ENODEV;
+	sparent = to_spmi_device(dev->parent);
+	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
+	if (IS_ERR(sub_sdev))
+		return PTR_ERR(sub_sdev);
 
 	init_completion(&iadc->complete);
 	mutex_init(&iadc->lock);
 
-	ret = of_property_read_u32(node, "reg", &res);
+	ret = of_property_read_u32(node, "reg", &iadc_regmap_config.reg_base);
 	if (ret < 0)
 		return -ENODEV;
 
-	iadc->base = res;
+	iadc->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &iadc_regmap_config);
+	if (IS_ERR(iadc->regmap))
+		return PTR_ERR(iadc->regmap);
 
 	ret = iadc_version_check(iadc);
 	if (ret < 0)
@@ -584,3 +593,4 @@ MODULE_ALIAS("platform:qcom-spmi-iadc");
 MODULE_DESCRIPTION("Qualcomm SPMI PMIC current ADC driver");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Ivan T. Ivanov <iivanov@mm-sol.com>");
+MODULE_IMPORT_NS("SPMI");
-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

[PATCH v4 7/7] iio: adc: qcom-spmi-iadc: Remove regmap R/W wrapper functions

[AngeloGioacchino Del Regno]

This driver doesn't need to add any register base address to any
regmap call anymore since it was migrated to register as a SPMI
subdevice with its own regmap reg_base, which makes the regmap
API to automatically add such base address internally.

Since the iadc_{read,write,read_result}() functions now only do
call regmap_{read,write,bulk_read}() and nothing else, simplify
the driver by removing them and by calling regmap APIs directly.

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
Link: https://lore.kernel.org/r/20250722101317.76729-8-angelogioacchino.delregno@collabora.com
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8650-QRD
Link: https://lore.kernel.org/r/20250730112645.542179-8-angelogioacchino.delregno@collabora.com
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iio/adc/qcom-spmi-iadc.c | 83 ++++++++++++--------------------
 1 file changed, 30 insertions(+), 53 deletions(-)

diff --git a/drivers/iio/adc/qcom-spmi-iadc.c b/drivers/iio/adc/qcom-spmi-iadc.c
index 67096952b229..7d46ec2d1a30 100644
--- a/drivers/iio/adc/qcom-spmi-iadc.c
+++ b/drivers/iio/adc/qcom-spmi-iadc.c
@@ -113,77 +113,59 @@ struct iadc_chip {
 	struct completion complete;
 };
 
-static int iadc_read(struct iadc_chip *iadc, u16 offset, u8 *data)
-{
-	unsigned int val;
-	int ret;
-
-	ret = regmap_read(iadc->regmap, offset, &val);
-	if (ret < 0)
-		return ret;
-
-	*data = val;
-	return 0;
-}
-
-static int iadc_write(struct iadc_chip *iadc, u16 offset, u8 data)
-{
-	return regmap_write(iadc->regmap, offset, data);
-}
-
 static int iadc_reset(struct iadc_chip *iadc)
 {
-	u8 data;
+	u32 data;
 	int ret;
 
-	ret = iadc_write(iadc, IADC_SEC_ACCESS, IADC_SEC_ACCESS_DATA);
+	ret = regmap_write(iadc->regmap, IADC_SEC_ACCESS, IADC_SEC_ACCESS_DATA);
 	if (ret < 0)
 		return ret;
 
-	ret = iadc_read(iadc, IADC_PERH_RESET_CTL3, &data);
+	ret = regmap_read(iadc->regmap, IADC_PERH_RESET_CTL3, &data);
 	if (ret < 0)
 		return ret;
 
-	ret = iadc_write(iadc, IADC_SEC_ACCESS, IADC_SEC_ACCESS_DATA);
+	ret = regmap_write(iadc->regmap, IADC_SEC_ACCESS, IADC_SEC_ACCESS_DATA);
 	if (ret < 0)
 		return ret;
 
 	data |= IADC_FOLLOW_WARM_RB;
 
-	return iadc_write(iadc, IADC_PERH_RESET_CTL3, data);
+	return regmap_write(iadc->regmap, IADC_PERH_RESET_CTL3, data);
 }
 
 static int iadc_set_state(struct iadc_chip *iadc, bool state)
 {
-	return iadc_write(iadc, IADC_EN_CTL1, state ? IADC_EN_CTL1_SET : 0);
+	return regmap_write(iadc->regmap, IADC_EN_CTL1, state ? IADC_EN_CTL1_SET : 0);
 }
 
 static void iadc_status_show(struct iadc_chip *iadc)
 {
-	u8 mode, sta1, chan, dig, en, req;
+	u32 mode, sta1, chan, dig, en, req;
 	int ret;
 
-	ret = iadc_read(iadc, IADC_MODE_CTL, &mode);
+	ret = regmap_read(iadc->regmap, IADC_MODE_CTL, &mode);
 	if (ret < 0)
 		return;
 
-	ret = iadc_read(iadc, IADC_DIG_PARAM, &dig);
+	ret = regmap_read(iadc->regmap, IADC_DIG_PARAM, &dig);
 	if (ret < 0)
 		return;
 
-	ret = iadc_read(iadc, IADC_CH_SEL_CTL, &chan);
+	ret = regmap_read(iadc->regmap, IADC_CH_SEL_CTL, &chan);
 	if (ret < 0)
 		return;
 
-	ret = iadc_read(iadc, IADC_CONV_REQ, &req);
+	ret = regmap_read(iadc->regmap, IADC_CONV_REQ, &req);
 	if (ret < 0)
 		return;
 
-	ret = iadc_read(iadc, IADC_STATUS1, &sta1);
+	ret = regmap_read(iadc->regmap, IADC_STATUS1, &sta1);
 	if (ret < 0)
 		return;
 
-	ret = iadc_read(iadc, IADC_EN_CTL1, &en);
+	ret = regmap_read(iadc->regmap, IADC_EN_CTL1, &en);
 	if (ret < 0)
 		return;
 
@@ -199,34 +181,34 @@ static int iadc_configure(struct iadc_chip *iadc, int channel)
 
 	/* Mode selection */
 	mode = (IADC_OP_MODE_NORMAL << IADC_OP_MODE_SHIFT) | IADC_TRIM_EN;
-	ret = iadc_write(iadc, IADC_MODE_CTL, mode);
+	ret = regmap_write(iadc->regmap, IADC_MODE_CTL, mode);
 	if (ret < 0)
 		return ret;
 
 	/* Channel selection */
-	ret = iadc_write(iadc, IADC_CH_SEL_CTL, channel);
+	ret = regmap_write(iadc->regmap, IADC_CH_SEL_CTL, channel);
 	if (ret < 0)
 		return ret;
 
 	/* Digital parameter setup */
 	decim = IADC_DEF_DECIMATION << IADC_DIG_DEC_RATIO_SEL_SHIFT;
-	ret = iadc_write(iadc, IADC_DIG_PARAM, decim);
+	ret = regmap_write(iadc->regmap, IADC_DIG_PARAM, decim);
 	if (ret < 0)
 		return ret;
 
 	/* HW settle time delay */
-	ret = iadc_write(iadc, IADC_HW_SETTLE_DELAY, IADC_DEF_HW_SETTLE_TIME);
+	ret = regmap_write(iadc->regmap, IADC_HW_SETTLE_DELAY, IADC_DEF_HW_SETTLE_TIME);
 	if (ret < 0)
 		return ret;
 
-	ret = iadc_write(iadc, IADC_FAST_AVG_CTL, IADC_DEF_AVG_SAMPLES);
+	ret = regmap_write(iadc->regmap, IADC_FAST_AVG_CTL, IADC_DEF_AVG_SAMPLES);
 	if (ret < 0)
 		return ret;
 
 	if (IADC_DEF_AVG_SAMPLES)
-		ret = iadc_write(iadc, IADC_FAST_AVG_EN, IADC_FAST_AVG_EN_SET);
+		ret = regmap_write(iadc->regmap, IADC_FAST_AVG_EN, IADC_FAST_AVG_EN_SET);
 	else
-		ret = iadc_write(iadc, IADC_FAST_AVG_EN, 0);
+		ret = regmap_write(iadc->regmap, IADC_FAST_AVG_EN, 0);
 
 	if (ret < 0)
 		return ret;
@@ -239,19 +221,19 @@ static int iadc_configure(struct iadc_chip *iadc, int channel)
 		return ret;
 
 	/* Request conversion */
-	return iadc_write(iadc, IADC_CONV_REQ, IADC_CONV_REQ_SET);
+	return regmap_write(iadc->regmap, IADC_CONV_REQ, IADC_CONV_REQ_SET);
 }
 
 static int iadc_poll_wait_eoc(struct iadc_chip *iadc, unsigned int interval_us)
 {
 	unsigned int count, retry;
 	int ret;
-	u8 sta1;
+	u32 sta1;
 
 	retry = interval_us / IADC_CONV_TIME_MIN_US;
 
 	for (count = 0; count < retry; count++) {
-		ret = iadc_read(iadc, IADC_STATUS1, &sta1);
+		ret = regmap_read(iadc->regmap, IADC_STATUS1, &sta1);
 		if (ret < 0)
 			return ret;
 
@@ -267,11 +249,6 @@ static int iadc_poll_wait_eoc(struct iadc_chip *iadc, unsigned int interval_us)
 	return -ETIMEDOUT;
 }
 
-static int iadc_read_result(struct iadc_chip *iadc, u16 *data)
-{
-	return regmap_bulk_read(iadc->regmap, IADC_DATA, data, 2);
-}
-
 static int iadc_do_conversion(struct iadc_chip *iadc, int chan, u16 *data)
 {
 	unsigned int wait;
@@ -296,7 +273,7 @@ static int iadc_do_conversion(struct iadc_chip *iadc, int chan, u16 *data)
 	}
 
 	if (!ret)
-		ret = iadc_read_result(iadc, data);
+		ret = regmap_bulk_read(iadc->regmap, IADC_DATA, data, sizeof(*data));
 exit:
 	iadc_set_state(iadc, false);
 	if (ret < 0)
@@ -392,10 +369,10 @@ static int iadc_update_offset(struct iadc_chip *iadc)
 
 static int iadc_version_check(struct iadc_chip *iadc)
 {
-	u8 val;
+	u32 val;
 	int ret;
 
-	ret = iadc_read(iadc, IADC_PERPH_TYPE, &val);
+	ret = regmap_read(iadc->regmap, IADC_PERPH_TYPE, &val);
 	if (ret < 0)
 		return ret;
 
@@ -404,7 +381,7 @@ static int iadc_version_check(struct iadc_chip *iadc)
 		return -EINVAL;
 	}
 
-	ret = iadc_read(iadc, IADC_PERPH_SUBTYPE, &val);
+	ret = regmap_read(iadc->regmap, IADC_PERPH_SUBTYPE, &val);
 	if (ret < 0)
 		return ret;
 
@@ -413,7 +390,7 @@ static int iadc_version_check(struct iadc_chip *iadc)
 		return -EINVAL;
 	}
 
-	ret = iadc_read(iadc, IADC_REVISION2, &val);
+	ret = regmap_read(iadc->regmap, IADC_REVISION2, &val);
 	if (ret < 0)
 		return ret;
 
@@ -428,7 +405,7 @@ static int iadc_version_check(struct iadc_chip *iadc)
 static int iadc_rsense_read(struct iadc_chip *iadc, struct device_node *node)
 {
 	int ret, sign, int_sense;
-	u8 deviation;
+	u32 deviation;
 
 	ret = of_property_read_u32(node, "qcom,external-resistor-micro-ohms",
 				   &iadc->rsense[IADC_EXT_RSENSE]);
@@ -440,7 +417,7 @@ static int iadc_rsense_read(struct iadc_chip *iadc, struct device_node *node)
 		return -EINVAL;
 	}
 
-	ret = iadc_read(iadc, IADC_NOMINAL_RSENSE, &deviation);
+	ret = regmap_read(iadc->regmap, IADC_NOMINAL_RSENSE, &deviation);
 	if (ret < 0)
 		return ret;
 
-- 
2.51.0


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2116 bytes --]

Hello,

On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:
> @@ -119,19 +128,24 @@ static int sdam_probe(struct platform_device *pdev)
>  	if (!sdam)
>  		return -ENOMEM;
>  
> -	sdam->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> -	if (!sdam->regmap) {
> -		dev_err(&pdev->dev, "Failed to get regmap handle\n");
> -		return -ENXIO;
> -	}
> +	sparent = to_spmi_device(dev->parent);
> +	sub_sdev = devm_spmi_subdevice_alloc_and_add(dev, sparent);
> +	if (IS_ERR(sub_sdev))
> +		return PTR_ERR(sub_sdev);
>  
> -	rc = of_property_read_u32(pdev->dev.of_node, "reg", &sdam->base);
> +	rc = of_property_read_u32(dev->of_node, "reg", &sdam_regmap_config.reg_base);

It's a bit ugly that you pass the address of an unsigned int as u32*.
But this isn't new, so fine for me. (Also for all Linux archs we have
sizeof(unsigned int) == 4, so AFAICT it's safe anyhow.)

>  	if (rc < 0) {
>  		dev_err(&pdev->dev, "Failed to get SDAM base, rc=%d\n", rc);
>  		return -EINVAL;
>  	}
>  
> -	rc = regmap_read(sdam->regmap, sdam->base + SDAM_SIZE, &val);
> +	sdam->regmap = devm_regmap_init_spmi_ext(&sub_sdev->sdev, &sdam_regmap_config);
> +	if (IS_ERR(sdam->regmap)) {
> +		dev_err(&pdev->dev, "Failed to get regmap handle\n");

dev_err_probe()

> +		return PTR_ERR(sdam->regmap);
> +	}
> +
> +	rc = regmap_read(sdam->regmap, SDAM_SIZE, &val);
>  	if (rc < 0) {
>  		dev_err(&pdev->dev, "Failed to read SDAM_SIZE rc=%d\n", rc);
>  		return -EINVAL;
> @@ -159,7 +173,7 @@ static int sdam_probe(struct platform_device *pdev)
>  	}
>  	dev_dbg(&pdev->dev,
>  		"SDAM base=%#x size=%u registered successfully\n",
> -		sdam->base, sdam->size);
> +		sdam_regmap_config.reg_base, sdam->size);
>  
>  	return 0;
>  }
> @@ -181,3 +195,4 @@ module_platform_driver(sdam_driver);
>  
>  MODULE_DESCRIPTION("QCOM SPMI SDAM driver");
>  MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS("SPMI");

If it's exactly the files that #include <linux/spmi.h> should have that
namespace import, you can put the MODULE_IMPORT_NS into that header.

Best regards
Uwe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1754 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Andy Shevchenko]

On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:

...

> > +MODULE_IMPORT_NS("SPMI");
> 
> If it's exactly the files that #include <linux/spmi.h> should have that
> namespace import, you can put the MODULE_IMPORT_NS into that header.

Which makes anyone to import namespace even if they just want to use some types
out of the header. This is not good solution generally speaking. Also this will
diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
invisible that some of the code may become an abuser of the API just by someone
include the header (for a reason or by a mistake).

-- 
With Best Regards,
Andy Shevchenko



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 968 bytes --]

On Tue, Sep 16, 2025 at 04:35:35PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
> > On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:
> 
> ...
> 
> > > +MODULE_IMPORT_NS("SPMI");
> > 
> > If it's exactly the files that #include <linux/spmi.h> should have that
> > namespace import, you can put the MODULE_IMPORT_NS into that header.
> 
> Which makes anyone to import namespace even if they just want to use some types
> out of the header.

Notice that I carefully formulated my suggestion to cope for this case.

> This is not good solution generally speaking. Also this will
> diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
> invisible that some of the code may become an abuser of the API just by someone
> include the header (for a reason or by a mistake).

Yeah, opinions differ. In my eyes it's quite elegant.

Best regards
Uwe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Andy Shevchenko]

On Tue, Sep 16, 2025 at 6:11 PM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
> On Tue, Sep 16, 2025 at 04:35:35PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
> > > On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:

...

> > > > +MODULE_IMPORT_NS("SPMI");
> > >
> > > If it's exactly the files that #include <linux/spmi.h> should have that
> > > namespace import, you can put the MODULE_IMPORT_NS into that header.
> >
> > Which makes anyone to import namespace even if they just want to use some types
> > out of the header.
>
> Notice that I carefully formulated my suggestion to cope for this case.

And I carefully answered. Your proposal won't prevent _other_ files to
use the same header in the future without needing a namespace to be
imported.

> > This is not good solution generally speaking. Also this will
> > diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
> > invisible that some of the code may become an abuser of the API just by someone
> > include the header (for a reason or by a mistake).
>
> Yeah, opinions differ. In my eyes it's quite elegant.

It's not a pure opinion, it has a technical background that I
explained. The explicit usage of MODULE_IMPORT_NS() is better than
some header somewhere that might even be included by another and be
proxied to the code that doesn't need / want to have this namespace to
be present. Puting MODULE_IMPORT_NS() into a _header_ is a minefield
for the future.

-- 
With Best Regards,
Andy Shevchenko

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[AngeloGioacchino Del Regno]

Il 16/09/25 18:20, Andy Shevchenko ha scritto:
> On Tue, Sep 16, 2025 at 6:11 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
>> On Tue, Sep 16, 2025 at 04:35:35PM +0300, Andy Shevchenko wrote:
>>> On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
>>>> On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:
> 
> ...
> 
>>>>> +MODULE_IMPORT_NS("SPMI");
>>>>
>>>> If it's exactly the files that #include <linux/spmi.h> should have that
>>>> namespace import, you can put the MODULE_IMPORT_NS into that header.
>>>
>>> Which makes anyone to import namespace even if they just want to use some types
>>> out of the header.
>>
>> Notice that I carefully formulated my suggestion to cope for this case.
> 
> And I carefully answered. Your proposal won't prevent _other_ files to
> use the same header in the future without needing a namespace to be
> imported.
> 
>>> This is not good solution generally speaking. Also this will
>>> diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
>>> invisible that some of the code may become an abuser of the API just by someone
>>> include the header (for a reason or by a mistake).
>>
>> Yeah, opinions differ. In my eyes it's quite elegant.
> 
> It's not a pure opinion, it has a technical background that I
> explained. The explicit usage of MODULE_IMPORT_NS() is better than
> some header somewhere that might even be included by another and be
> proxied to the code that doesn't need / want to have this namespace to
> be present. Puting MODULE_IMPORT_NS() into a _header_ is a minefield
> for the future.
> 

Uwe, thanks for your review - much appreciated.

Even though I get your point... Sorry, but here I do agree with Andy, and I think
he explained some of the reasons pretty well.

Cheers,
Angelo



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 4/7] phy: qualcomm: eusb2-repeater: Migrate to devm_spmi_subdevice_alloc_and_add()

[kernel test robot]

Hi AngeloGioacchino,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20250915]
[cannot apply to jic23-iio/togreg char-misc/char-misc-testing char-misc/char-misc-next char-misc/char-misc-linus sre-power-supply/for-next linus/master v6.17-rc6 v6.17-rc5 v6.17-rc4 v6.17-rc6]
[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/20250916-164807
base:   next-20250915
patch link:    https://lore.kernel.org/r/20250916084445.96621-5-angelogioacchino.delregno%40collabora.com
patch subject: [PATCH v4 4/7] phy: qualcomm: eusb2-repeater: Migrate to devm_spmi_subdevice_alloc_and_add()
config: i386-buildonly-randconfig-001-20250917 (https://download.01.org/0day-ci/archive/20250917/202509171640.JfR2hhcz-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250917/202509171640.JfR2hhcz-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/202509171640.JfR2hhcz-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/phy/qualcomm/phy-qcom-eusb2-repeater.o: in function `eusb2_repeater_probe':
   phy-qcom-eusb2-repeater.c:(.text+0x315): undefined reference to `devm_spmi_subdevice_alloc_and_add'
   ld: drivers/base/regmap/regmap-spmi.o: in function `regmap_spmi_base_read':
>> regmap-spmi.c:(.text+0x79): undefined reference to `spmi_register_read'
   ld: drivers/base/regmap/regmap-spmi.o: in function `regmap_spmi_base_gather_write':
>> regmap-spmi.c:(.text+0xbe): undefined reference to `spmi_register_zero_write'
>> ld: regmap-spmi.c:(.text+0xe5): undefined reference to `spmi_register_write'
   ld: drivers/base/regmap/regmap-spmi.o: in function `regmap_spmi_ext_read':
>> regmap-spmi.c:(.text+0x1a0): undefined reference to `spmi_ext_register_read'
>> ld: regmap-spmi.c:(.text+0x1c7): undefined reference to `spmi_ext_register_readl'
   ld: drivers/base/regmap/regmap-spmi.o: in function `regmap_spmi_ext_gather_write':
>> regmap-spmi.c:(.text+0x228): undefined reference to `spmi_ext_register_write'
>> ld: regmap-spmi.c:(.text+0x24f): undefined reference to `spmi_ext_register_writel'

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for REGMAP_SPMI
   Depends on [m]: SPMI [=m]
   Selected by [y]:
   - PHY_QCOM_EUSB2_REPEATER [=y] && OF [=y] && (ARCH_QCOM || COMPILE_TEST [=y])
   Selected by [m]:
   - HI6421V600_IRQ [=m] && OF [=y] && SPMI [=m] && HAS_IOMEM [=y]
   - PINCTRL_QCOM_SPMI_PMIC [=m] && PINCTRL [=y] && (ARCH_QCOM || COMPILE_TEST [=y]) && OF [=y] && SPMI [=m]
   - QCOM_SPMI_TEMP_ALARM [=m] && THERMAL [=y] && (ARCH_QCOM && OF [=y] || COMPILE_TEST [=y]) && OF [=y] && SPMI [=m] && IIO [=y]
   - MFD_HI6421_SPMI [=m] && HAS_IOMEM [=y] && OF [=y] && SPMI [=m]
   - REGULATOR_MT6315 [=m] && REGULATOR [=y] && SPMI [=m]
   - QCOM_SPMI_ADC5 [=m] && IIO [=y] && SPMI [=m]
   - NVMEM_APPLE_SPMI [=m] && NVMEM [=y] && (ARCH_APPLE || COMPILE_TEST [=y]) && SPMI [=m]

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[AngeloGioacchino Del Regno]

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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 2389 bytes --]

Hello Andy,

On Tue, Sep 16, 2025 at 07:20:20PM +0300, Andy Shevchenko wrote:
> On Tue, Sep 16, 2025 at 6:11 PM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> > On Tue, Sep 16, 2025 at 04:35:35PM +0300, Andy Shevchenko wrote:
> > > On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
> > > > On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:
> 
> ...
> 
> > > > > +MODULE_IMPORT_NS("SPMI");
> > > >
> > > > If it's exactly the files that #include <linux/spmi.h> should have that
> > > > namespace import, you can put the MODULE_IMPORT_NS into that header.
> > >
> > > Which makes anyone to import namespace even if they just want to use some types
> > > out of the header.
> >
> > Notice that I carefully formulated my suggestion to cope for this case.
> 
> And I carefully answered.

I tend to disagree. If that anyone only wants some type from the header
but not the namespace, the if part of my statement isn't given any more.
Still your reply felt like objection while logically it's not.

> Your proposal won't prevent _other_ files to
> use the same header in the future without needing a namespace to be
> imported.

Oh yes. But that's true for every change: If you change it further you
have to cope for what is already there.

> > > This is not good solution generally speaking. Also this will
> > > diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
> > > invisible that some of the code may become an abuser of the API just by someone
> > > include the header (for a reason or by a mistake).
> >
> > Yeah, opinions differ. In my eyes it's quite elegant.
> 
> It's not a pure opinion,

That's your opinion :-)

> it has a technical background that I
> explained. The explicit usage of MODULE_IMPORT_NS() is better than
> some header somewhere that might even be included by another and be
> proxied to the code that doesn't need / want to have this namespace to
> be present. Puting MODULE_IMPORT_NS() into a _header_ is a minefield
> for the future.

Well, for a deliberate abuser the hurdle to have to add the explicit
MODULE_IMPORT_NS() isn't that big. And a mistaken abuser won't explode,
just generate a few bytes overhead that can be fixed when noticed.

In my opinion that is an ok cost for the added elegance.

Best regards
Uwe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3149 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[Sebastian Reichel]

[-- Attachment #1.1: Type: text/plain, Size: 7485 bytes --]

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
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 1/7] spmi: Implement spmi_subdevice_alloc_and_add() and devm variant

[AngeloGioacchino Del Regno]

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




-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Andy Shevchenko]

On Wed, Sep 17, 2025 at 02:47:22PM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 16, 2025 at 07:20:20PM +0300, Andy Shevchenko wrote:
> > On Tue, Sep 16, 2025 at 6:11 PM Uwe Kleine-König
> > <u.kleine-koenig@baylibre.com> wrote:
> > > On Tue, Sep 16, 2025 at 04:35:35PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Sep 16, 2025 at 03:24:56PM +0200, Uwe Kleine-König wrote:
> > > > > On Tue, Sep 16, 2025 at 10:44:40AM +0200, AngeloGioacchino Del Regno wrote:

...

> > > > > > +MODULE_IMPORT_NS("SPMI");
> > > > >
> > > > > If it's exactly the files that #include <linux/spmi.h> should have that
> > > > > namespace import, you can put the MODULE_IMPORT_NS into that header.
> > > >
> > > > Which makes anyone to import namespace even if they just want to use some types
> > > > out of the header.
> > >
> > > Notice that I carefully formulated my suggestion to cope for this case.
> > 
> > And I carefully answered.
> 
> I tend to disagree. If that anyone only wants some type from the header
> but not the namespace, the if part of my statement isn't given any more.
> Still your reply felt like objection while logically it's not.

You assumed that in case that the header that is *currently* included in the
users, may be the one that used by the same users that needs an imported
namespace. Okay, *now* (or today) it's not a problem, but *in the future* it
might be *when* one wants to use *just* types from it.
I don't think this is likely to happen, but in general including something
"by default" is not a good idea. That's what I'm objecting to.

> > Your proposal won't prevent _other_ files to
> > use the same header in the future without needing a namespace to be
> > imported.
> 
> Oh yes. But that's true for every change: If you change it further you
> have to cope for what is already there.
> 
> > > > This is not good solution generally speaking. Also this will
> > > > diminish one of the purposes of _NS variants of MODULE*/EXPORT*, i.e. make it
> > > > invisible that some of the code may become an abuser of the API just by someone
> > > > include the header (for a reason or by a mistake).
> > >
> > > Yeah, opinions differ. In my eyes it's quite elegant.
> > 
> > It's not a pure opinion,
> 
> That's your opinion :-)

All we said is just set of opinions. Facts are provided by scientific
experiments.

> > it has a technical background that I
> > explained. The explicit usage of MODULE_IMPORT_NS() is better than
> > some header somewhere that might even be included by another and be
> > proxied to the code that doesn't need / want to have this namespace to
> > be present. Puting MODULE_IMPORT_NS() into a _header_ is a minefield
> > for the future.
> 
> Well, for a deliberate abuser the hurdle to have to add the explicit
> MODULE_IMPORT_NS() isn't that big. And a mistaken abuser won't explode,
> just generate a few bytes overhead that can be fixed when noticed.
> 
> In my opinion that is an ok cost for the added elegance.

I tend to disagree. The practice to include (be lazy) something just in case is
a bad practice. Developer has to know what they are doing. We have already too
much bad code in the kernel and opening new ways for more "vibe:ish" coding is
a road to accumulated issues in the future.

I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header
file.

-- 
With Best Regards,
Andy Shevchenko



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Greg KH]

On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote:
> I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header
> file.

Yes, please never do that, it defeats the purpose of module namespaces
completly.  If you don't want to have module namespaces, don't use them
for your subsytem.  Don't use them and then make them moot by putting
MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless.

thanks,

greg k-h

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[David Lechner]

On 9/19/25 8:59 AM, Greg KH wrote:
> On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote:
>> I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header
>> file.
> 
> Yes, please never do that, it defeats the purpose of module namespaces
> completly.  If you don't want to have module namespaces, don't use them
> for your subsytem.  Don't use them and then make them moot by putting
> MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless.
> 
> thanks,
> 
> greg k-h


Could someone suggest some additional explanation to add to
Documentation/core-api/symbol-namespaces.rst to explain the
reasoning behind this?

Right now, the only part of that document that say _why_ we have
module namespces says:

	That is useful for documentation purposes (think of the
	SUBSYSTEM_DEBUG namespace) as well as for limiting the
	availability of a set of symbols for use in other parts
	of the kernel.

So I don't see the connection between this explanation and and:

	[Putting MODULE_IMPORT_NS() into the header] defeats
	the purpose of module namespaces completely.

I am guilty of putting it in a header, so if I need to fix that
I would like to actually understand why first. Andy has mentioned
something about potential abuses, but without any example, I haven't
been able to understand what this would actually actually look like.
Or maybe there is some other reason that Greg is thinking of that
hasn't been mentioned yet?

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Greg KH]

On Fri, Sep 19, 2025 at 10:05:28AM -0500, David Lechner wrote:
> On 9/19/25 8:59 AM, Greg KH wrote:
> > On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote:
> >> I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header
> >> file.
> > 
> > Yes, please never do that, it defeats the purpose of module namespaces
> > completly.  If you don't want to have module namespaces, don't use them
> > for your subsytem.  Don't use them and then make them moot by putting
> > MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless.
> > 
> > thanks,
> > 
> > greg k-h
> 
> 
> Could someone suggest some additional explanation to add to
> Documentation/core-api/symbol-namespaces.rst to explain the
> reasoning behind this?
> 
> Right now, the only part of that document that say _why_ we have
> module namespces says:
> 
> 	That is useful for documentation purposes (think of the
> 	SUBSYSTEM_DEBUG namespace) as well as for limiting the
> 	availability of a set of symbols for use in other parts
> 	of the kernel.
> 
> So I don't see the connection between this explanation and and:
> 
> 	[Putting MODULE_IMPORT_NS() into the header] defeats
> 	the purpose of module namespaces completely.
> 
> I am guilty of putting it in a header, so if I need to fix that
> I would like to actually understand why first. Andy has mentioned
> something about potential abuses, but without any example, I haven't
> been able to understand what this would actually actually look like.
> Or maybe there is some other reason that Greg is thinking of that
> hasn't been mentioned yet?

Let me turn it around, _why_ would you want your exports in a namespace
at all if you just are putting a MODULE_IMPORT_NS() in the .h file at
the same time?  What is this giving you at all compared to just a normal
MODULE_EXPORT() marking for your exports?

I know what it gives me when I don't put it in a .h file, but I think
that might be different from what you are thinking here :)

thanks,

greg k-h

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[David Lechner]

On 9/19/25 10:13 AM, Greg KH wrote:
> On Fri, Sep 19, 2025 at 10:05:28AM -0500, David Lechner wrote:
>> On 9/19/25 8:59 AM, Greg KH wrote:
>>> On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote:
>>>> I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header
>>>> file.
>>>
>>> Yes, please never do that, it defeats the purpose of module namespaces
>>> completly.  If you don't want to have module namespaces, don't use them
>>> for your subsytem.  Don't use them and then make them moot by putting
>>> MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>>
>> Could someone suggest some additional explanation to add to
>> Documentation/core-api/symbol-namespaces.rst to explain the
>> reasoning behind this?
>>
>> Right now, the only part of that document that say _why_ we have
>> module namespces says:
>>
>> 	That is useful for documentation purposes (think of the
>> 	SUBSYSTEM_DEBUG namespace) as well as for limiting the
>> 	availability of a set of symbols for use in other parts
>> 	of the kernel.
>>
>> So I don't see the connection between this explanation and and:
>>
>> 	[Putting MODULE_IMPORT_NS() into the header] defeats
>> 	the purpose of module namespaces completely.
>>
>> I am guilty of putting it in a header, so if I need to fix that
>> I would like to actually understand why first. Andy has mentioned
>> something about potential abuses, but without any example, I haven't
>> been able to understand what this would actually actually look like.
>> Or maybe there is some other reason that Greg is thinking of that
>> hasn't been mentioned yet?
> 
> Let me turn it around, _why_ would you want your exports in a namespace
> at all if you just are putting a MODULE_IMPORT_NS() in the .h file at
> the same time?  What is this giving you at all compared to just a normal
> MODULE_EXPORT() marking for your exports?
> 
> I know what it gives me when I don't put it in a .h file, but I think
> that might be different from what you are thinking here :)
> 
> thanks,
> 
> greg k-h

Up to now, my (naive) understanding was that the point module namespaces
is to reduce the number of symbols in the global namespace because having
too many symbols there was starting to cause problems. So moving symbols
to another namespace was a "good thing".



-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Greg KH]

On Fri, Sep 19, 2025 at 10:20:29AM -0500, David Lechner wrote:
> On 9/19/25 10:13 AM, Greg KH wrote:
> > On Fri, Sep 19, 2025 at 10:05:28AM -0500, David Lechner wrote:
> >> On 9/19/25 8:59 AM, Greg KH wrote:
> >>> On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote:
> >>>> I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header
> >>>> file.
> >>>
> >>> Yes, please never do that, it defeats the purpose of module namespaces
> >>> completly.  If you don't want to have module namespaces, don't use them
> >>> for your subsytem.  Don't use them and then make them moot by putting
> >>> MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless.
> >>>
> >>> thanks,
> >>>
> >>> greg k-h
> >>
> >>
> >> Could someone suggest some additional explanation to add to
> >> Documentation/core-api/symbol-namespaces.rst to explain the
> >> reasoning behind this?
> >>
> >> Right now, the only part of that document that say _why_ we have
> >> module namespces says:
> >>
> >> 	That is useful for documentation purposes (think of the
> >> 	SUBSYSTEM_DEBUG namespace) as well as for limiting the
> >> 	availability of a set of symbols for use in other parts
> >> 	of the kernel.
> >>
> >> So I don't see the connection between this explanation and and:
> >>
> >> 	[Putting MODULE_IMPORT_NS() into the header] defeats
> >> 	the purpose of module namespaces completely.
> >>
> >> I am guilty of putting it in a header, so if I need to fix that
> >> I would like to actually understand why first. Andy has mentioned
> >> something about potential abuses, but without any example, I haven't
> >> been able to understand what this would actually actually look like.
> >> Or maybe there is some other reason that Greg is thinking of that
> >> hasn't been mentioned yet?
> > 
> > Let me turn it around, _why_ would you want your exports in a namespace
> > at all if you just are putting a MODULE_IMPORT_NS() in the .h file at
> > the same time?  What is this giving you at all compared to just a normal
> > MODULE_EXPORT() marking for your exports?
> > 
> > I know what it gives me when I don't put it in a .h file, but I think
> > that might be different from what you are thinking here :)
> > 
> > thanks,
> > 
> > greg k-h
> 
> Up to now, my (naive) understanding was that the point module namespaces
> is to reduce the number of symbols in the global namespace because having
> too many symbols there was starting to cause problems. So moving symbols
> to another namespace was a "good thing".

Yes, it is a "good thing" overall, but by just making all of your
symbols in a namespace, and then including it in the .h file, that does
the same exact thing as before (i.e. anyone that includes that .h file
puts the symbols into the global namespace with that prefix.)

Ideally, the goal was to be able to easily see in a module, what symbol
namespaces they depend on, which requires them to put MODULE_IMPORT_NS()
in the module to get access to those symbols.  dmabuf has done this very
well, making it obvious to the maintainers of that subsystem that they
should be paying attention to those users.

For other "tiny" subsystems, it just slots away their symbols so that no
one else should ever be using them, and it makes it blindingly obvious
if they do.  For example, the usb-storage symbols, anyone that does:
	MODULE_IMPORT_NS("USB_STORAGE");
had better be living in drivers/usb/storage/ otherwise I need to have a
word with those offenders :)

So it's a way of "tidying" up things, and to make things more explicit
than just having to rely on searching a tree and looking for .h include
usage.  Right now, you are kind of defeating that by just allowing a .h
to be included and you don't get any benifit of being able to watch out
for who is actually using those symbols overall.

Hope this helps,

greg k-h

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3744 bytes --]

Hello Greg,

On Fri, Sep 19, 2025 at 05:13:35PM +0200, Greg KH wrote:
> On Fri, Sep 19, 2025 at 10:05:28AM -0500, David Lechner wrote:
> > On 9/19/25 8:59 AM, Greg KH wrote:
> > > On Thu, Sep 18, 2025 at 10:00:29PM +0300, Andy Shevchenko wrote:
> > >> I,o.w. I principally disagree on putting MODULE_IMPORT_NS() into the header
> > >> file.
> > > 
> > > Yes, please never do that, it defeats the purpose of module namespaces
> > > completly.  If you don't want to have module namespaces, don't use them
> > > for your subsytem.  Don't use them and then make them moot by putting
> > > MODULE_IMPORT_NS() in the .h file for the symbols as that's pointless.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > 
> > 
> > Could someone suggest some additional explanation to add to
> > Documentation/core-api/symbol-namespaces.rst to explain the
> > reasoning behind this?
> > 
> > Right now, the only part of that document that say _why_ we have
> > module namespces says:
> > 
> > 	That is useful for documentation purposes (think of the
> > 	SUBSYSTEM_DEBUG namespace) as well as for limiting the
> > 	availability of a set of symbols for use in other parts
> > 	of the kernel.
> > 
> > So I don't see the connection between this explanation and and:
> > 
> > 	[Putting MODULE_IMPORT_NS() into the header] defeats
> > 	the purpose of module namespaces completely.
> > 
> > I am guilty of putting it in a header, so if I need to fix that
> > I would like to actually understand why first. Andy has mentioned
> > something about potential abuses, but without any example, I haven't
> > been able to understand what this would actually actually look like.
> > Or maybe there is some other reason that Greg is thinking of that
> > hasn't been mentioned yet?
> 
> Let me turn it around, _why_ would you want your exports in a namespace
> at all if you just are putting a MODULE_IMPORT_NS() in the .h file at
> the same time?  What is this giving you at all compared to just a normal
> MODULE_EXPORT() marking for your exports?
> 
> I know what it gives me when I don't put it in a .h file, but I think
> that might be different from what you are thinking here :)

For me (who still today thinks it's elegant to put the MODULE_IMPORT_NS
in a header next to the declarations of the symbols in that namespace)
it's the documentation thing quoted above (e.g. modinfo lists the used
namespaces) and to unclutter the global namespace.

The latter was essentially the motivation to introduce symbol
namespaces, see the cover letter from back then[1].

Personally I don't see the relevance of making it harder for abusers.

A non-GPL module should be stopped by the symbol being exported using
EXPORT_SYMBOL_GPL() with or without namespaces. And for a binary
distribution of a module the need to add the MODULE_IMPORT_NS statement
in the source code is a quite low barrier for the evil binary module
distributor that IMHO doesn't justify to burden all regular users of a
namespace to have to do two things (#include + MODULE_IMPORT_NS) instead
of only one (include which implies the MODULE_IMPORT_NS) to make use of
said namespace.

And for GPL code: Who is actually an abuser of say devm_pwm_get()?
In my eyes any module should be free to use that function. And if it's
not sensible to do so, I would expect this to be discovered even without
the needed MODULE_IMPORT_NS("PWM") in the code. And if it's not
discovered that's fine for me, too. For me that's the spirit of Open
Source: If someone finds devm_pwm_get() useful, let them use it. Please
even tell me if you use it in a way I didn't expect, I'm glad to hear
about it.

I'm probably missing something.

Best regards
Uwe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 3367 bytes --]

Hello Greg,

it seems we agree on the advantage of module namespaces that the pool of
global symbols is reduced, right? That is given with and without the
module import in a header.

On Fri, Sep 19, 2025 at 05:37:03PM +0200, Greg KH wrote:
> On Fri, Sep 19, 2025 at 10:20:29AM -0500, David Lechner wrote:
> > Up to now, my (naive) understanding was that the point module namespaces
> > is to reduce the number of symbols in the global namespace because having
> > too many symbols there was starting to cause problems. So moving symbols
> > to another namespace was a "good thing".
> 
> Yes, it is a "good thing" overall, but by just making all of your
> symbols in a namespace, and then including it in the .h file, that does
> the same exact thing as before (i.e. anyone that includes that .h file
> puts the symbols into the global namespace with that prefix.)

I fail to parse the part in parenthesis. The symbols of the pwm
subsystem are defined in the "PWM" namespace (using `#define
DEFAULT_SYMBOL_NAMESPACE "PWM"` in drivers/pwm/core.c). In <linux/pwm.h>
there is a `MODULE_IMPORT_NS("PWM");`, so a consumer (say
`drivers/regulator/pwm-regulator.c`) only needs

	#include <linux/pwm.h>

to import the "PWM" namespace. So pwm-regulator.c puts the symbols
into the global namespace with that prefix. What symbols? What prefix?

The thing that is different is that the pwm functions are in the "PWM"
namespace, so a module without an import for "PWM" has a smaller pool of
global symbols and so loading that module is a tad more effective,
right?

I agree that for the consumer source it doesn't make a difference if pwm
is using a namespace or not. I'd count that as an advantage of the
"import in a header" approach.

> Ideally, the goal was to be able to easily see in a module, what symbol
> namespaces they depend on, which requires them to put MODULE_IMPORT_NS()
> in the module to get access to those symbols.  dmabuf has done this very
> well, making it obvious to the maintainers of that subsystem that they
> should be paying attention to those users.

For me as pwm maintainer it doesn't matter much if I pay attention to
`MODULE_IMPORT_NS("PWM");` or `#include <linux/pwm.h>`.
 
> For other "tiny" subsystems, it just slots away their symbols so that no
> one else should ever be using them, and it makes it blindingly obvious
> if they do.  For example, the usb-storage symbols, anyone that does:
> 	MODULE_IMPORT_NS("USB_STORAGE");
> had better be living in drivers/usb/storage/ otherwise I need to have a
> word with those offenders :)

All symbols in the "USB_STORAGE" namespace (apart from
`fill_inquiry_response`) start with `usb_stor_`. If you grep for that
string you find all the (probably illegitimate) users of the usb-storage
symbols, but also those that define their own symbols with that prefix.

Do you actually look out for such offenders, i.e. have a lei mailbox
with `MODULE_IMPORT_NS("USB_STORAGE")` as search string or a cron job
grepping your tree for that?

> So it's a way of "tidying" up things, and to make things more explicit
> than just having to rely on searching a tree and looking for .h include
> usage. 

For some reason in your eyes grepping for MODULE_IMPORT_NS is superior
to grepping for an #include. Can you explain that?

Best regards
Uwe

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 112 bytes --]

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH v4 2/7] nvmem: qcom-spmi-sdam: Migrate to devm_spmi_subdevice_alloc_and_add()

[Greg KH]

On Sat, Sep 20, 2025 at 06:41:57PM +0200, Uwe Kleine-König wrote:
> On Fri, Sep 19, 2025 at 05:37:03PM +0200, Greg KH wrote:
> > On Fri, Sep 19, 2025 at 10:20:29AM -0500, David Lechner wrote:
> > > Up to now, my (naive) understanding was that the point module namespaces
> > > is to reduce the number of symbols in the global namespace because having
> > > too many symbols there was starting to cause problems. So moving symbols
> > > to another namespace was a "good thing".
> > 
> > Yes, it is a "good thing" overall, but by just making all of your
> > symbols in a namespace, and then including it in the .h file, that does
> > the same exact thing as before (i.e. anyone that includes that .h file
> > puts the symbols into the global namespace with that prefix.)
> 
> I fail to parse the part in parenthesis. The symbols of the pwm
> subsystem are defined in the "PWM" namespace (using `#define
> DEFAULT_SYMBOL_NAMESPACE "PWM"` in drivers/pwm/core.c). In <linux/pwm.h>
> there is a `MODULE_IMPORT_NS("PWM");`, so a consumer (say
> `drivers/regulator/pwm-regulator.c`) only needs
> 
> 	#include <linux/pwm.h>
> 
> to import the "PWM" namespace. So pwm-regulator.c puts the symbols
> into the global namespace with that prefix. What symbols? What prefix?
> 
> The thing that is different is that the pwm functions are in the "PWM"
> namespace, so a module without an import for "PWM" has a smaller pool of
> global symbols and so loading that module is a tad more effective,
> right?
> 
> I agree that for the consumer source it doesn't make a difference if pwm
> is using a namespace or not. I'd count that as an advantage of the
> "import in a header" approach.
> 
> > Ideally, the goal was to be able to easily see in a module, what symbol
> > namespaces they depend on, which requires them to put MODULE_IMPORT_NS()
> > in the module to get access to those symbols.  dmabuf has done this very
> > well, making it obvious to the maintainers of that subsystem that they
> > should be paying attention to those users.
> 
> For me as pwm maintainer it doesn't matter much if I pay attention to
> `MODULE_IMPORT_NS("PWM");` or `#include <linux/pwm.h>`.

I think this is the primary thing here.  It's easier for some
maintainers and reviewers to notice the MODULE_IMPORT_NS() thing than a
simple include line.  Especially as includes are often hidden in other
include files.

So sure, as a maintainer, you are free to do things this way, it's just
not really what we thought about when namespaces were first created.  We
assumed that an explict MODULE_INPORT_NS() was what would be used, not
worked around :)

> > For other "tiny" subsystems, it just slots away their symbols so that no
> > one else should ever be using them, and it makes it blindingly obvious
> > if they do.  For example, the usb-storage symbols, anyone that does:
> > 	MODULE_IMPORT_NS("USB_STORAGE");
> > had better be living in drivers/usb/storage/ otherwise I need to have a
> > word with those offenders :)
> 
> All symbols in the "USB_STORAGE" namespace (apart from
> `fill_inquiry_response`) start with `usb_stor_`. If you grep for that
> string you find all the (probably illegitimate) users of the usb-storage
> symbols, but also those that define their own symbols with that prefix.
> 
> Do you actually look out for such offenders, i.e. have a lei mailbox
> with `MODULE_IMPORT_NS("USB_STORAGE")` as search string or a cron job
> grepping your tree for that?

Some maintainers do just this, yes.  I think the dmabuf maintainers do
as an example.

> > So it's a way of "tidying" up things, and to make things more explicit
> > than just having to rely on searching a tree and looking for .h include
> > usage. 
> 
> For some reason in your eyes grepping for MODULE_IMPORT_NS is superior
> to grepping for an #include. Can you explain that?

#include files are often included in other include files, and can easily
be "hidden" in the chain of what is needed by what .c file.

That's it, nothing major here, if you want to use namespaces this way,
that's fine, just feels kind of counter-productive :)

thanks,

greg k-h

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy