[PATCH v2 0/3] Marvell 88PM886 PMIC GPADC driver

[PATCH v2 0/3] Marvell 88PM886 PMIC GPADC driver

[Duje Mihanović]

This series adds a driver for the GPADC found on the Marvell 88PM886
PMIC. The GPADC monitors various system voltages and is a prerequisite
for battery monitoring on boards using the PMIC.

Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
---
Changes in v2:
- Refactor driver according to comments
- Add binding patch
- Link to v1: https://lore.kernel.org/r/20250829-88pm886-gpadc-v1-0-f60262266fea@dujemihanovic.xyz

---
Duje Mihanović (3):
      dt-bindings: mfd: 88pm886: Add #io-channel-cells
      iio: adc: Add driver for Marvell 88PM886 PMIC ADC
      mfd: 88pm886: Add GPADC cell

 .../bindings/mfd/marvell,88pm886-a1.yaml           |   4 +
 MAINTAINERS                                        |   5 +
 drivers/iio/adc/88pm886-gpadc.c                    | 383 +++++++++++++++++++++
 drivers/iio/adc/Kconfig                            |  13 +
 drivers/iio/adc/Makefile                           |   1 +
 drivers/mfd/88pm886.c                              |   1 +
 include/linux/mfd/88pm886.h                        |  54 +++
 7 files changed, 461 insertions(+)
---
base-commit: 1b237f190eb3d36f52dffe07a40b5eb210280e00
change-id: 20250827-88pm886-gpadc-81e2ca1d52ce

Best regards,
-- 
Duje Mihanović <duje@dujemihanovic.xyz>

[PATCH v2 1/3] dt-bindings: mfd: 88pm886: Add #io-channel-cells

[Duje Mihanović]

Add an #io-channel-cells property to the Marvell 88PM886 PMIC binding to
allow referencing the IO channels exposed by its GPADC.

Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
---
v2:
- New patch
---
 Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml b/Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml
index d6a71c912b76f7d24787d346d4b4cd51919b1cf6..92a72a99fd790805e775727e39d457608fa1795d 100644
--- a/Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml
+++ b/Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml
@@ -35,6 +35,9 @@ properties:
         description: LDO or buck regulator.
         unevaluatedProperties: false
 
+  '#io-channel-cells':
+    const: 1
+
 required:
   - compatible
   - reg
@@ -53,6 +56,7 @@ examples:
         reg = <0x30>;
         interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;
         interrupt-parent = <&gic>;
+        #io-channel-cells = <1>;
         wakeup-source;
 
         regulators {

-- 
2.51.0

[PATCH v2 2/3] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

[Duje Mihanović]

Marvell's 88PM886 PMIC has a so-called General Purpose ADC used for
monitoring various system voltages and temperatures. Add the relevant
register definitions to the MFD header and a driver for the ADC.

Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
---
v2:
- default MFD_88PM886_PMIC
- u8[2] -> __be16
- Drop kernel.h include
- Add pm886_gpadc struct
- Reorder channel enum
- Drop GPADC voltage channels
- Drop unnecessary masking in gpadc_get_raw()
- Extend gpadc_enable_bias() to allow disabling bias
- usleep_range() -> fsleep()
- PM wrapper for pm886_gpadc_read_raw()
- Proper channel info: voltage is RAW | SCALE, temperature is RAW |
  OFFSET | SCALE, resistance is PROCESSED
- Explicitly define channels to en/disable in pm886_gpadc_setup()
- Don't explicitly set iio->dev.parent
- Miscellaneous style changes
---
 MAINTAINERS                     |   5 +
 drivers/iio/adc/88pm886-gpadc.c | 383 ++++++++++++++++++++++++++++++++++++++++
 drivers/iio/adc/Kconfig         |  13 ++
 drivers/iio/adc/Makefile        |   1 +
 include/linux/mfd/88pm886.h     |  54 ++++++
 5 files changed, 456 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index fed6cd812d796a08cebc0c1fd540c8901d1bf448..b362d81e9c1532cc7920f9cec65b1fd1f81471c6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14710,6 +14710,11 @@ F:	drivers/regulator/88pm886-regulator.c
 F:	drivers/rtc/rtc-88pm886.c
 F:	include/linux/mfd/88pm886.h
 
+MARVELL 88PM886 PMIC GPADC DRIVER
+M:	Duje Mihanović <duje@dujemihanovic.xyz>
+S:	Maintained
+F:	drivers/iio/adc/88pm886-gpadc.c
+
 MARVELL ARMADA 3700 PHY DRIVERS
 M:	Miquel Raynal <miquel.raynal@bootlin.com>
 S:	Maintained
diff --git a/drivers/iio/adc/88pm886-gpadc.c b/drivers/iio/adc/88pm886-gpadc.c
new file mode 100644
index 0000000000000000000000000000000000000000..4622d2525e0edeed89c6e6d43336b177590aa885
--- /dev/null
+++ b/drivers/iio/adc/88pm886-gpadc.c
@@ -0,0 +1,383 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2025, Duje Mihanović <duje@dujemihanovic.xyz>
+ */
+
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/types.h>
+
+#include <linux/mfd/88pm886.h>
+
+struct pm886_gpadc {
+	struct regmap *map;
+};
+
+static const int pm886_gpadc_regs[] = {
+	PM886_REG_GPADC_VSC,
+	PM886_REG_GPADC_VCHG_PWR,
+	PM886_REG_GPADC_VCF_OUT,
+	PM886_REG_GPADC_VBAT,
+	PM886_REG_GPADC_VBAT_SLP,
+	PM886_REG_GPADC_VBUS,
+
+	PM886_REG_GPADC_GPADC0,
+	PM886_REG_GPADC_GPADC1,
+	PM886_REG_GPADC_GPADC2,
+	PM886_REG_GPADC_GPADC3,
+
+	PM886_REG_GPADC_GND_DET1,
+	PM886_REG_GPADC_GND_DET2,
+	PM886_REG_GPADC_MIC_DET,
+
+	PM886_REG_GPADC_TINT,
+};
+
+/* Must be kept in sync with the table above */
+enum pm886_gpadc_channel {
+	VSC_CHAN,
+	VCHG_PWR_CHAN,
+	VCF_OUT_CHAN,
+	VBAT_CHAN,
+	VBAT_SLP_CHAN,
+	VBUS_CHAN,
+
+	GPADC0_CHAN,
+	GPADC1_CHAN,
+	GPADC2_CHAN,
+	GPADC3_CHAN,
+
+	GND_DET1_CHAN,
+	GND_DET2_CHAN,
+	MIC_DET_CHAN,
+
+	TINT_CHAN,
+};
+
+#define ADC_CHANNEL_VOLTAGE(index, lsb, name) {	\
+	.type = IIO_VOLTAGE, \
+	.indexed = 1, \
+	.channel = index, \
+	.address = lsb, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+			      BIT(IIO_CHAN_INFO_SCALE), \
+	.datasheet_name = name, \
+}
+
+#define ADC_CHANNEL_RESISTANCE(index, lsb, name) { \
+	.type = IIO_RESISTANCE, \
+	.indexed = 1, \
+	.channel = index, \
+	.address = lsb, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
+	.datasheet_name = name, \
+}
+
+#define ADC_CHANNEL_TEMPERATURE(index, lsb, name) { \
+	.type = IIO_TEMP, \
+	.indexed = 1, \
+	.channel = index, \
+	.address = lsb, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+			      BIT(IIO_CHAN_INFO_SCALE) | \
+			      BIT(IIO_CHAN_INFO_OFFSET), \
+	.datasheet_name = name, \
+}
+
+static const struct iio_chan_spec pm886_gpadc_channels[] = {
+	ADC_CHANNEL_VOLTAGE(VSC_CHAN, 1367, "vsc"),
+	ADC_CHANNEL_VOLTAGE(VCHG_PWR_CHAN, 1709, "vchg_pwr"),
+	ADC_CHANNEL_VOLTAGE(VCF_OUT_CHAN, 1367, "vcf_out"),
+	ADC_CHANNEL_VOLTAGE(VBAT_CHAN, 1367, "vbat"),
+	ADC_CHANNEL_VOLTAGE(VBAT_SLP_CHAN, 1367, "vbat_slp"),
+	ADC_CHANNEL_VOLTAGE(VBUS_CHAN, 1709, "vbus"),
+
+	ADC_CHANNEL_RESISTANCE(GPADC0_CHAN, 342, "gpadc0"),
+	ADC_CHANNEL_RESISTANCE(GPADC1_CHAN, 342, "gpadc1"),
+	ADC_CHANNEL_RESISTANCE(GPADC2_CHAN, 342, "gpadc2"),
+	ADC_CHANNEL_RESISTANCE(GPADC3_CHAN, 342, "gpadc3"),
+
+	ADC_CHANNEL_VOLTAGE(GND_DET1_CHAN, 342, "gnddet1"),
+	ADC_CHANNEL_VOLTAGE(GND_DET2_CHAN, 342, "gnddet2"),
+	ADC_CHANNEL_VOLTAGE(MIC_DET_CHAN, 1367, "mic_det"),
+
+	ADC_CHANNEL_TEMPERATURE(TINT_CHAN, 104, "tint"),
+};
+
+static const struct regmap_config pm886_gpadc_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = PM886_REG_GPADC_VBAT_SLP + 1,
+};
+
+static int gpadc_get_raw(struct iio_dev *iio, enum pm886_gpadc_channel chan)
+{
+	struct pm886_gpadc *gpadc = iio_priv(iio);
+	__be16 buf;
+	int ret;
+
+	ret = regmap_bulk_read(gpadc->map, pm886_gpadc_regs[chan], &buf, sizeof(buf));
+	return !ret ? be16_to_cpu(buf) >> 4 : ret;
+}
+
+static int
+gpadc_set_bias(struct pm886_gpadc *gpadc, enum pm886_gpadc_channel chan, bool on)
+{
+	unsigned int gpadc_num = chan - GPADC0_CHAN;
+	unsigned int bits = BIT(gpadc_num + 4) | BIT(gpadc_num);
+
+	return regmap_assign_bits(gpadc->map, PM886_REG_GPADC_CONFIG(0x14), bits, on);
+}
+
+static int
+gpadc_find_bias_current(struct iio_dev *iio, struct iio_chan_spec const *chan,
+			unsigned int *raw_uv, unsigned int *raw_ua)
+{
+	struct pm886_gpadc *gpadc = iio_priv(iio);
+	unsigned int gpadc_num = chan->channel - GPADC0_CHAN;
+	unsigned int reg = PM886_REG_GPADC_CONFIG(0xb + gpadc_num);
+	unsigned long lsb = chan->address;
+	int ret;
+
+	for (unsigned int i = 0; i < PM886_GPADC_BIAS_LEVELS; i++) {
+		ret = regmap_update_bits(gpadc->map, reg, GENMASK(3, 0), i);
+		if (ret)
+			return ret;
+
+		fsleep(5000);
+
+		*raw_ua = PM886_GPADC_INDEX_TO_BIAS_UA(i);
+		*raw_uv = gpadc_get_raw(iio, chan->channel) * lsb;
+
+		/*
+		 * Vendor kernel errors out above 1.25V, but testing shows that
+		 * the resistance of the battery detection channel (GPADC2 on
+		 * coreprimevelte) reaches about 1.4Mohm when the battery is
+		 * removed, which can't be measured with such a low upper
+		 * limit. Therefore, to be able to detect the battery without
+		 * ugly externs as used in the vendor fuelgauge driver,
+		 * increase this limit a bit.
+		 */
+		if (WARN_ON(*raw_uv > 1500 * (MICRO / MILLI)))
+			return -EIO;
+
+		/*
+		 * Vendor kernel errors out under 300mV, but for the same
+		 * reason as above (except the channel hovers around 3.5kohm
+		 * with battery present) reduce this limit.
+		 */
+		if (*raw_uv < 200 * (MICRO / MILLI)) {
+			dev_dbg(&iio->dev, "bad bias for chan %d: %duA @ %duV\n", chan->channel,
+				*raw_ua, *raw_uv);
+			continue;
+		}
+
+		dev_dbg(&iio->dev, "good bias for chan %d: %duA @ %duV\n", chan->channel,
+			*raw_ua, *raw_uv);
+		return 0;
+	}
+
+	dev_err(&iio->dev, "failed to find good bias for chan %d\n", chan->channel);
+	return -EINVAL;
+}
+
+static int
+gpadc_get_resistance_ohm(struct iio_dev *iio, struct iio_chan_spec const *chan)
+{
+	struct pm886_gpadc *gpadc = iio_priv(iio);
+	unsigned int raw_uv, raw_ua;
+	int ret;
+
+	ret = gpadc_set_bias(gpadc, chan->channel, true);
+	if (ret)
+		goto err;
+
+	ret = gpadc_find_bias_current(iio, chan, &raw_uv, &raw_ua);
+	if (ret)
+		goto err;
+
+	ret = gpadc_set_bias(gpadc, chan->channel, false);
+	if (ret)
+		return ret;
+
+	return DIV_ROUND_CLOSEST(raw_uv, raw_ua);
+err:
+	gpadc_set_bias(gpadc, chan->channel, false);
+	return ret;
+}
+
+static int
+__pm886_gpadc_read_raw(struct iio_dev *iio, struct iio_chan_spec const *chan,
+		       int *val, int *val2, long mask)
+{
+	unsigned long lsb = chan->address;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*val = gpadc_get_raw(iio, chan->channel);
+		if (*val < 0)
+			return *val;
+
+		dev_dbg(&iio->dev, "chan: %d, raw: %d\n", chan->channel, *val);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = lsb;
+		*val2 = (MICRO / MILLI);
+		return chan->type == IIO_VOLTAGE
+		       ? IIO_VAL_FRACTIONAL
+		       : IIO_VAL_INT;
+	case IIO_CHAN_INFO_OFFSET:
+		/* Raw value is 104 millikelvin/LSB, convert it to 104 millicelsius/LSB */
+		*val = ABSOLUTE_ZERO_MILLICELSIUS;
+		*val2 = lsb;
+		return IIO_VAL_FRACTIONAL;
+	case IIO_CHAN_INFO_PROCESSED:
+		*val = gpadc_get_resistance_ohm(iio, chan);
+		if (*val < 0)
+			return *val;
+
+		dev_dbg(&iio->dev, "chan: %d, %d Ohm\n", chan->channel, *val);
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int pm886_gpadc_read_raw(struct iio_dev *iio, struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct device *dev = iio->dev.parent;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
+
+	ret = __pm886_gpadc_read_raw(iio, chan, val, val2, mask);
+
+	pm_runtime_put_autosuspend(dev);
+	return ret;
+}
+
+static int pm886_gpadc_setup(struct regmap *map, bool enable)
+{
+	const u8 config[] = {
+		PM886_GPADC_CONFIG1_EN_ALL,
+		PM886_GPADC_CONFIG2_EN_ALL,
+		PM886_GPADC_GND_DET2_EN
+	};
+	int ret;
+
+	/* Enable/disable the ADC block */
+	ret = regmap_assign_bits(map, PM886_REG_GPADC_CONFIG(0x6), BIT(0), enable);
+	if (ret)
+		return ret;
+
+	if (!enable)
+		return 0;
+
+	/* If enabling, enable all channels */
+	return regmap_bulk_write(map, PM886_REG_GPADC_CONFIG(0x1), config, ARRAY_SIZE(config));
+}
+
+static const struct iio_info pm886_gpadc_iio_info = {
+	.read_raw = pm886_gpadc_read_raw,
+};
+
+static int pm886_gpadc_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pm886_chip *chip = dev_get_drvdata(dev->parent);
+	struct i2c_client *client = chip->client;
+	struct pm886_gpadc *gpadc;
+	struct i2c_client *page;
+	struct iio_dev *iio;
+	int ret;
+
+	iio = devm_iio_device_alloc(dev, sizeof(*gpadc));
+	if (!iio)
+		return -ENOMEM;
+
+	gpadc = iio_priv(iio);
+	dev_set_drvdata(dev, iio);
+
+	page = devm_i2c_new_dummy_device(dev, client->adapter,
+					 client->addr + PM886_PAGE_OFFSET_GPADC);
+	if (IS_ERR(page))
+		return dev_err_probe(dev, PTR_ERR(page), "Failed to initialize GPADC page\n");
+
+	gpadc->map = devm_regmap_init_i2c(page, &pm886_gpadc_regmap_config);
+	if (IS_ERR(gpadc->map))
+		return dev_err_probe(dev, PTR_ERR(gpadc->map),
+				     "Failed to initialize GPADC regmap\n");
+
+	iio->name = "88pm886-gpadc";
+	iio->dev.of_node = dev->parent->of_node;
+	iio->modes = INDIO_DIRECT_MODE;
+	iio->info = &pm886_gpadc_iio_info;
+	iio->channels = pm886_gpadc_channels;
+	iio->num_channels = ARRAY_SIZE(pm886_gpadc_channels);
+
+	pm_runtime_set_autosuspend_delay(dev, 50);
+	pm_runtime_use_autosuspend(dev);
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
+
+	ret = devm_iio_device_register(dev, iio);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register ADC\n");
+
+	return 0;
+}
+
+static int pm886_gpadc_runtime_resume(struct device *dev)
+{
+	struct iio_dev *iio = dev_get_drvdata(dev);
+	struct pm886_gpadc *gpadc = iio_priv(iio);
+
+	return pm886_gpadc_setup(gpadc->map, true);
+}
+
+static int pm886_gpadc_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *iio = dev_get_drvdata(dev);
+	struct pm886_gpadc *gpadc = iio_priv(iio);
+
+	return pm886_gpadc_setup(gpadc->map, false);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(pm886_gpadc_pm_ops,
+				 pm886_gpadc_runtime_suspend,
+				 pm886_gpadc_runtime_resume, NULL);
+
+static const struct platform_device_id pm886_gpadc_id[] = {
+	{ "88pm886-gpadc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, pm886_gpadc_id);
+
+static struct platform_driver pm886_gpadc_driver = {
+	.driver = {
+		.name = "88pm886-gpadc",
+		.pm = pm_ptr(&pm886_gpadc_pm_ops),
+	},
+	.probe = pm886_gpadc_probe,
+	.id_table = pm886_gpadc_id,
+};
+module_platform_driver(pm886_gpadc_driver);
+
+MODULE_AUTHOR("Duje Mihanović <duje@dujemihanovic.xyz>");
+MODULE_DESCRIPTION("Marvell 88PM886 GPADC driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 24f2572c487ea3db2abec3283ebd93357c08baab..04c8478ff707dd16ec943674ac7f01f33249acf1 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -9,6 +9,19 @@ menu "Analog to digital converters"
 config IIO_ADC_HELPER
 	tristate
 
+config 88PM886_GPADC
+	tristate "Marvell 88PM886 GPADC driver"
+	depends on MFD_88PM886_PMIC
+	default MFD_88PM886_PMIC
+	help
+	  Say Y here to enable support for the GPADC (General Purpose ADC)
+	  found on the Marvell 88PM886 PMIC. The GPADC measures various
+	  internal voltages and temperatures, including (but not limited to)
+	  system, battery and USB Vbus.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called 88pm886-gpadc.
+
 config AB8500_GPADC
 	bool "ST-Ericsson AB8500 GPADC driver"
 	depends on AB8500_CORE && REGULATOR_AB8500
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 1c6ca5fd4b6db8c4c40a351b231ba0892e8cd70e..64854907bf3bef7da39f95247e4e502d01232af3 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -6,6 +6,7 @@
 obj-$(CONFIG_IIO_ADC_HELPER) += industrialio-adc.o
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_88PM886_GPADC) += 88pm886-gpadc.o
 obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4000) += ad4000.o
diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
index 85eca44f39ab58ba4cb9ec4216118ee9604d021f..85c3c16fb10b7ee6aafdd6e68fd9135d8009eef8 100644
--- a/include/linux/mfd/88pm886.h
+++ b/include/linux/mfd/88pm886.h
@@ -10,6 +10,7 @@
 #define PM886_IRQ_ONKEY			0
 
 #define PM886_PAGE_OFFSET_REGULATORS	1
+#define PM886_PAGE_OFFSET_GPADC		2
 
 #define PM886_REG_ID			0x00
 
@@ -67,6 +68,59 @@
 #define PM886_REG_BUCK4_VOUT		0xcf
 #define PM886_REG_BUCK5_VOUT		0xdd
 
+/* GPADC enable/disable registers */
+#define PM886_REG_GPADC_CONFIG(n)	(n)
+
+/* GPADC channel registers */
+#define PM886_REG_GPADC_VSC		0x40
+#define PM886_REG_GPADC_VCHG_PWR	0x4c
+#define PM886_REG_GPADC_VCF_OUT		0x4e
+#define PM886_REG_GPADC_TINT		0x50
+#define PM886_REG_GPADC_GPADC0		0x54
+#define PM886_REG_GPADC_GPADC1		0x56
+#define PM886_REG_GPADC_GPADC2		0x58
+#define PM886_REG_GPADC_VBAT		0xa0
+#define PM886_REG_GPADC_GND_DET1	0xa4
+#define PM886_REG_GPADC_GND_DET2	0xa6
+#define PM886_REG_GPADC_VBUS		0xa8
+#define PM886_REG_GPADC_GPADC3		0xaa
+#define PM886_REG_GPADC_MIC_DET		0xac
+#define PM886_REG_GPADC_VBAT_SLP	0xb0
+
+/* GPADC channel enable bits */
+#define PM886_GPADC_VSC_EN	BIT(0)
+#define PM886_GPADC_VBAT_EN	BIT(1)
+#define PM886_GPADC_GNDDET1_EN	BIT(3)
+#define PM886_GPADC_VBUS_EN	BIT(4)
+#define PM886_GPADC_VCHG_PWR_EN	BIT(5)
+#define PM886_GPADC_VCF_OUT_EN	BIT(6)
+#define PM886_GPADC_CONFIG1_EN_ALL (PM886_GPADC_VSC_EN | \
+				    PM886_GPADC_VBAT_EN | \
+				    PM886_GPADC_GNDDET1_EN | \
+				    PM886_GPADC_VBUS_EN | \
+				    PM886_GPADC_VCHG_PWR_EN | \
+				    PM886_GPADC_VCF_OUT_EN)
+
+#define PM886_GPADC_TINT_EN	BIT(0)
+#define PM886_GPADC_PMODE_EN	BIT(1)
+#define PM886_GPADC_GPADC0_EN	BIT(2)
+#define PM886_GPADC_GPADC1_EN	BIT(3)
+#define PM886_GPADC_GPADC2_EN	BIT(4)
+#define PM886_GPADC_GPADC3_EN	BIT(5)
+#define PM886_GPADC_MIC_DET_EN	BIT(6)
+#define PM886_GPADC_CONFIG2_EN_ALL (PM886_GPADC_TINT_EN | \
+				    PM886_GPADC_GPADC0_EN | \
+				    PM886_GPADC_GPADC1_EN | \
+				    PM886_GPADC_GPADC2_EN | \
+				    PM886_GPADC_GPADC3_EN | \
+				    PM886_GPADC_MIC_DET_EN)
+
+/* No CONFIG3_EN_ALL because this is the only bit there */
+#define PM886_GPADC_GND_DET2_EN	BIT(0)
+
+#define PM886_GPADC_BIAS_LEVELS		16
+#define PM886_GPADC_INDEX_TO_BIAS_UA(i)	(1 + (i) * 5)
+
 #define PM886_LDO_VSEL_MASK		0x0f
 #define PM886_BUCK_VSEL_MASK		0x7f
 

-- 
2.51.0

[PATCH v2 3/3] mfd: 88pm886: Add GPADC cell

[Duje Mihanović]

Add a cell for the PMIC's onboard General Purpose ADC.

Acked-by: Karel Balej <balejk@matfyz.cz> # for the PMIC
Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
---
v2:
- Sort cell names
---
 drivers/mfd/88pm886.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/88pm886.c b/drivers/mfd/88pm886.c
index 39dd9a818b0f0e1e5839f76768ff54940f4cefa5..e411d8dee55420e10b6d7ad7069576c681360de1 100644
--- a/drivers/mfd/88pm886.c
+++ b/drivers/mfd/88pm886.c
@@ -35,6 +35,7 @@ static const struct resource pm886_onkey_resources[] = {
 };
 
 static const struct mfd_cell pm886_devs[] = {
+	MFD_CELL_NAME("88pm886-gpadc"),
 	MFD_CELL_RES("88pm886-onkey", pm886_onkey_resources),
 	MFD_CELL_NAME("88pm886-regulator"),
 	MFD_CELL_NAME("88pm886-rtc"),

-- 
2.51.0

Re: [PATCH v2 1/3] dt-bindings: mfd: 88pm886: Add #io-channel-cells

[Krzysztof Kozlowski]

On 31/08/2025 12:33, Duje Mihanović wrote:
> Add an #io-channel-cells property to the Marvell 88PM886 PMIC binding to
> allow referencing the IO channels exposed by its GPADC.
> 
> Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 1/3] dt-bindings: mfd: 88pm886: Add #io-channel-cells

[Karel Balej]

Duje Mihanović, 2025-08-31T12:33:04+02:00:
> Add an #io-channel-cells property to the Marvell 88PM886 PMIC binding to
> allow referencing the IO channels exposed by its GPADC.
>
> Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
> ---
> v2:
> - New patch
> ---
>  Documentation/devicetree/bindings/mfd/marvell,88pm886-a1.yaml | 4 ++++
>  1 file changed, 4 insertions(+)

Acked-by: Karel Balej <balejk@matfyz.cz> # for the PMIC

Re: [PATCH v2 2/3] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

[Karel Balej]

Duje Mihanović, 2025-08-31T12:33:05+02:00:
> Marvell's 88PM886 PMIC has a so-called General Purpose ADC used for
> monitoring various system voltages and temperatures. Add the relevant
> register definitions to the MFD header and a driver for the ADC.
>
> Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
> ---
> v2:
> - default MFD_88PM886_PMIC
> - u8[2] -> __be16
> - Drop kernel.h include
> - Add pm886_gpadc struct
> - Reorder channel enum
> - Drop GPADC voltage channels
> - Drop unnecessary masking in gpadc_get_raw()
> - Extend gpadc_enable_bias() to allow disabling bias
> - usleep_range() -> fsleep()
> - PM wrapper for pm886_gpadc_read_raw()
> - Proper channel info: voltage is RAW | SCALE, temperature is RAW |
>   OFFSET | SCALE, resistance is PROCESSED
> - Explicitly define channels to en/disable in pm886_gpadc_setup()
> - Don't explicitly set iio->dev.parent
> - Miscellaneous style changes
> ---
>  MAINTAINERS                     |   5 +
>  drivers/iio/adc/88pm886-gpadc.c | 383 ++++++++++++++++++++++++++++++++++++++++
>  drivers/iio/adc/Kconfig         |  13 ++
>  drivers/iio/adc/Makefile        |   1 +
>  include/linux/mfd/88pm886.h     |  54 ++++++
>  5 files changed, 456 insertions(+)
>

[...]

> diff --git a/drivers/iio/adc/88pm886-gpadc.c b/drivers/iio/adc/88pm886-gpadc.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..4622d2525e0edeed89c6e6d43336b177590aa885
> --- /dev/null
> +++ b/drivers/iio/adc/88pm886-gpadc.c

[...]

> +static int pm886_gpadc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pm886_chip *chip = dev_get_drvdata(dev->parent);
> +	struct i2c_client *client = chip->client;
> +	struct pm886_gpadc *gpadc;
> +	struct i2c_client *page;
> +	struct iio_dev *iio;
> +	int ret;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*gpadc));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	gpadc = iio_priv(iio);
> +	dev_set_drvdata(dev, iio);
> +
> +	page = devm_i2c_new_dummy_device(dev, client->adapter,
> +					 client->addr + PM886_PAGE_OFFSET_GPADC);
> +	if (IS_ERR(page))
> +		return dev_err_probe(dev, PTR_ERR(page), "Failed to initialize GPADC page\n");
> +
> +	gpadc->map = devm_regmap_init_i2c(page, &pm886_gpadc_regmap_config);
> +	if (IS_ERR(gpadc->map))
> +		return dev_err_probe(dev, PTR_ERR(gpadc->map),
> +				     "Failed to initialize GPADC regmap\n");
> +
> +	iio->name = "88pm886-gpadc";
> +	iio->dev.of_node = dev->parent->of_node;

Didn't you mean to drop this since Jonathan pointed out that it's done
by the core?

> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->info = &pm886_gpadc_iio_info;
> +	iio->channels = pm886_gpadc_channels;
> +	iio->num_channels = ARRAY_SIZE(pm886_gpadc_channels);
> +
> +	pm_runtime_set_autosuspend_delay(dev, 50);
> +	pm_runtime_use_autosuspend(dev);
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register ADC\n");
> +
> +	return 0;
> +}

[...]

> diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
> index 85eca44f39ab58ba4cb9ec4216118ee9604d021f..85c3c16fb10b7ee6aafdd6e68fd9135d8009eef8 100644
> --- a/include/linux/mfd/88pm886.h
> +++ b/include/linux/mfd/88pm886.h
> @@ -10,6 +10,7 @@
>  #define PM886_IRQ_ONKEY			0
>  
>  #define PM886_PAGE_OFFSET_REGULATORS	1
> +#define PM886_PAGE_OFFSET_GPADC		2
>  
>  #define PM886_REG_ID			0x00
>  
> @@ -67,6 +68,59 @@
>  #define PM886_REG_BUCK4_VOUT		0xcf
>  #define PM886_REG_BUCK5_VOUT		0xdd
>  
> +/* GPADC enable/disable registers */
> +#define PM886_REG_GPADC_CONFIG(n)	(n)
> +
> +/* GPADC channel registers */
> +#define PM886_REG_GPADC_VSC		0x40
> +#define PM886_REG_GPADC_VCHG_PWR	0x4c
> +#define PM886_REG_GPADC_VCF_OUT		0x4e
> +#define PM886_REG_GPADC_TINT		0x50
> +#define PM886_REG_GPADC_GPADC0		0x54
> +#define PM886_REG_GPADC_GPADC1		0x56
> +#define PM886_REG_GPADC_GPADC2		0x58
> +#define PM886_REG_GPADC_VBAT		0xa0
> +#define PM886_REG_GPADC_GND_DET1	0xa4
> +#define PM886_REG_GPADC_GND_DET2	0xa6
> +#define PM886_REG_GPADC_VBUS		0xa8
> +#define PM886_REG_GPADC_GPADC3		0xaa
> +#define PM886_REG_GPADC_MIC_DET		0xac
> +#define PM886_REG_GPADC_VBAT_SLP	0xb0
> +
> +/* GPADC channel enable bits */
> +#define PM886_GPADC_VSC_EN	BIT(0)
> +#define PM886_GPADC_VBAT_EN	BIT(1)
> +#define PM886_GPADC_GNDDET1_EN	BIT(3)
> +#define PM886_GPADC_VBUS_EN	BIT(4)
> +#define PM886_GPADC_VCHG_PWR_EN	BIT(5)
> +#define PM886_GPADC_VCF_OUT_EN	BIT(6)
> +#define PM886_GPADC_CONFIG1_EN_ALL (PM886_GPADC_VSC_EN | \
> +				    PM886_GPADC_VBAT_EN | \
> +				    PM886_GPADC_GNDDET1_EN | \
> +				    PM886_GPADC_VBUS_EN | \
> +				    PM886_GPADC_VCHG_PWR_EN | \
> +				    PM886_GPADC_VCF_OUT_EN)
> +
> +#define PM886_GPADC_TINT_EN	BIT(0)
> +#define PM886_GPADC_PMODE_EN	BIT(1)
> +#define PM886_GPADC_GPADC0_EN	BIT(2)
> +#define PM886_GPADC_GPADC1_EN	BIT(3)
> +#define PM886_GPADC_GPADC2_EN	BIT(4)
> +#define PM886_GPADC_GPADC3_EN	BIT(5)
> +#define PM886_GPADC_MIC_DET_EN	BIT(6)
> +#define PM886_GPADC_CONFIG2_EN_ALL (PM886_GPADC_TINT_EN | \
> +				    PM886_GPADC_GPADC0_EN | \
> +				    PM886_GPADC_GPADC1_EN | \
> +				    PM886_GPADC_GPADC2_EN | \
> +				    PM886_GPADC_GPADC3_EN | \
> +				    PM886_GPADC_MIC_DET_EN)
> +
> +/* No CONFIG3_EN_ALL because this is the only bit there */
> +#define PM886_GPADC_GND_DET2_EN	BIT(0)

I have previously ordered the definitions here to always have the
individual bits follow the register definition (see above REG_STATUS1
for instance). I don't know if this is a common practice, but it seemed
useful to me to make it clear which register the bits are found in
without having to look at their usage (even though in your case it is
clear from the _EN_ALL definition following them) and without making
them long by stuffing the register name in their names. So if you wanted
to follow this logic, the preceding three paragraphs should be moved
after the GPADC_CONFIG macro (since you used that to condense the
individual register definitions).

Also a nit, the above comment is I believe a full sentence, so it should
have a period at the end (I wouldn't mention it, but I seem to recall
that Lee was keen on this :-).

> +
> +#define PM886_GPADC_BIAS_LEVELS		16
> +#define PM886_GPADC_INDEX_TO_BIAS_UA(i)	(1 + (i) * 5)
> +
>  #define PM886_LDO_VSEL_MASK		0x0f
>  #define PM886_BUCK_VSEL_MASK		0x7f

It would also seem logical to me to keep these last two grouped with the
other regulators-related definitions.

Re: [PATCH v2 2/3] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

[Duje Mihanović]

On Sunday, 31 August 2025 21:24:54 Central European Summer Time Karel Balej wrote:
> Duje Mihanović, 2025-08-31T12:33:05+02:00:
> > +	if (IS_ERR(page))
> > +		return dev_err_probe(dev, PTR_ERR(page), "Failed to initialize GPADC
> > page\n"); +
> > +	gpadc->map = devm_regmap_init_i2c(page, &pm886_gpadc_regmap_config);
> > +	if (IS_ERR(gpadc->map))
> > +		return dev_err_probe(dev, PTR_ERR(gpadc->map),
> > +				     "Failed to initialize GPADC regmap\n");
> > +
> > +	iio->name = "88pm886-gpadc";
> > +	iio->dev.of_node = dev->parent->of_node;
> 
> Didn't you mean to drop this since Jonathan pointed out that it's done
> by the core?

Actually I have found that device tree consumers fail to get their IO
channels without this line, so I left it.

Regards,
--
Duje

Re: [PATCH v2 2/3] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

[Andy Shevchenko]

On Sun, Aug 31, 2025 at 10:19:38PM +0200, Duje Mihanović wrote:
> On Sunday, 31 August 2025 21:24:54 Central European Summer Time Karel Balej wrote:
> > Duje Mihanović, 2025-08-31T12:33:05+02:00:

...

> > > +	iio->dev.of_node = dev->parent->of_node;
> > 
> > Didn't you mean to drop this since Jonathan pointed out that it's done
> > by the core?
> 
> Actually I have found that device tree consumers fail to get their IO
> channels without this line, so I left it.

because the passed device is not parent?

In any case this line is problematic. Use device_set_node() instead with the
proper dev_fwnode() parameter.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

[Andy Shevchenko]

On Sun, Aug 31, 2025 at 12:33:05PM +0200, Duje Mihanović wrote:
> Marvell's 88PM886 PMIC has a so-called General Purpose ADC used for
> monitoring various system voltages and temperatures. Add the relevant
> register definitions to the MFD header and a driver for the ADC.

...

> +#define ADC_CHANNEL_RESISTANCE(index, lsb, name) { \

Please, make those { on the separate lines

{

> +	.type = IIO_RESISTANCE, \
> +	.indexed = 1, \
> +	.channel = index, \
> +	.address = lsb, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> +	.datasheet_name = name, \
> +}

Also it's easier to read when \:s are TABbed to the same column.

...

> +static const struct regmap_config pm886_gpadc_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = PM886_REG_GPADC_VBAT_SLP + 1,

What is this + 1 register? Why is it not defined / documented?

> +};

...

> +static int gpadc_get_raw(struct iio_dev *iio, enum pm886_gpadc_channel chan)
> +{
> +	struct pm886_gpadc *gpadc = iio_priv(iio);
> +	__be16 buf;
> +	int ret;
> +
> +	ret = regmap_bulk_read(gpadc->map, pm886_gpadc_regs[chan], &buf, sizeof(buf));
> +	return !ret ? be16_to_cpu(buf) >> 4 : ret;

This is hard to read. Note, the more LoCs is fine as long as it helps (and
actually a lot in this case) readability.

> +}

...

> +static int
> +gpadc_find_bias_current(struct iio_dev *iio, struct iio_chan_spec const *chan,
> +			unsigned int *raw_uv, unsigned int *raw_ua)
> +{
> +	struct pm886_gpadc *gpadc = iio_priv(iio);
> +	unsigned int gpadc_num = chan->channel - GPADC0_CHAN;
> +	unsigned int reg = PM886_REG_GPADC_CONFIG(0xb + gpadc_num);
> +	unsigned long lsb = chan->address;
> +	int ret;
> +
> +	for (unsigned int i = 0; i < PM886_GPADC_BIAS_LEVELS; i++) {
> +		ret = regmap_update_bits(gpadc->map, reg, GENMASK(3, 0), i);
> +		if (ret)
> +			return ret;

Sleep needs to be explained.

> +		fsleep(5000);

5 * USEC_PER_MSEC

> +		*raw_ua = PM886_GPADC_INDEX_TO_BIAS_UA(i);
> +		*raw_uv = gpadc_get_raw(iio, chan->channel) * lsb;

Can we use uA and uV suffixes?

> +		/*
> +		 * Vendor kernel errors out above 1.25V, but testing shows that
> +		 * the resistance of the battery detection channel (GPADC2 on
> +		 * coreprimevelte) reaches about 1.4Mohm when the battery is
> +		 * removed, which can't be measured with such a low upper
> +		 * limit. Therefore, to be able to detect the battery without
> +		 * ugly externs as used in the vendor fuelgauge driver,
> +		 * increase this limit a bit.
> +		 */
> +		if (WARN_ON(*raw_uv > 1500 * (MICRO / MILLI)))
> +			return -EIO;
> +
> +		/*
> +		 * Vendor kernel errors out under 300mV, but for the same
> +		 * reason as above (except the channel hovers around 3.5kohm
> +		 * with battery present) reduce this limit.
> +		 */
> +		if (*raw_uv < 200 * (MICRO / MILLI)) {
> +			dev_dbg(&iio->dev, "bad bias for chan %d: %duA @ %duV\n", chan->channel,
> +				*raw_ua, *raw_uv);
> +			continue;
> +		}
> +
> +		dev_dbg(&iio->dev, "good bias for chan %d: %duA @ %duV\n", chan->channel,
> +			*raw_ua, *raw_uv);
> +		return 0;
> +	}
> +
> +	dev_err(&iio->dev, "failed to find good bias for chan %d\n", chan->channel);
> +	return -EINVAL;
> +}

...

> +static int
> +gpadc_get_resistance_ohm(struct iio_dev *iio, struct iio_chan_spec const *chan)
> +{
> +	struct pm886_gpadc *gpadc = iio_priv(iio);
> +	unsigned int raw_uv, raw_ua;
> +	int ret;
> +
> +	ret = gpadc_set_bias(gpadc, chan->channel, true);
> +	if (ret)
> +		goto err;
> +
> +	ret = gpadc_find_bias_current(iio, chan, &raw_uv, &raw_ua);
> +	if (ret)
> +		goto err;
> +
> +	ret = gpadc_set_bias(gpadc, chan->channel, false);
> +	if (ret)
> +		return ret;
> +
> +	return DIV_ROUND_CLOSEST(raw_uv, raw_ua);
> +err:
> +	gpadc_set_bias(gpadc, chan->channel, false);

You do the same in the other branch and checking there for an error. Why this
one is so special?

> +	return ret;
> +}

...

> +		dev_dbg(&iio->dev, "chan: %d, raw: %d\n", chan->channel, *val);

How is this useful? The userspace gets the same value. Do you expect problems
on its way to the user space?

...

> +	case IIO_CHAN_INFO_SCALE:
> +		*val = lsb;
> +		*val2 = (MICRO / MILLI);
> +		return chan->type == IIO_VOLTAGE
> +		       ? IIO_VAL_FRACTIONAL
> +		       : IIO_VAL_INT;

Make it one line, it is much easier to follow or even split to a regular 'if'.

> +	case IIO_CHAN_INFO_OFFSET:
> +		/* Raw value is 104 millikelvin/LSB, convert it to 104 millicelsius/LSB */
> +		*val = ABSOLUTE_ZERO_MILLICELSIUS;
> +		*val2 = lsb;
> +		return IIO_VAL_FRACTIONAL;

...

> +static int pm886_gpadc_setup(struct regmap *map, bool enable)
> +{
> +	const u8 config[] = {
> +		PM886_GPADC_CONFIG1_EN_ALL,
> +		PM886_GPADC_CONFIG2_EN_ALL,
> +		PM886_GPADC_GND_DET2_EN

Leave trailing comma. This doesn't look like a terminator entry.

> +	};
> +	int ret;
> +
> +	/* Enable/disable the ADC block */
> +	ret = regmap_assign_bits(map, PM886_REG_GPADC_CONFIG(0x6), BIT(0), enable);
> +	if (ret)
> +		return ret;

> +	if (!enable)
> +		return 0;
> +
> +	/* If enabling, enable all channels */
> +	return regmap_bulk_write(map, PM886_REG_GPADC_CONFIG(0x1), config, ARRAY_SIZE(config));
> +}

...

> +static int pm886_gpadc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pm886_chip *chip = dev_get_drvdata(dev->parent);
> +	struct i2c_client *client = chip->client;
> +	struct pm886_gpadc *gpadc;
> +	struct i2c_client *page;
> +	struct iio_dev *iio;
> +	int ret;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*gpadc));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	gpadc = iio_priv(iio);
> +	dev_set_drvdata(dev, iio);
> +
> +	page = devm_i2c_new_dummy_device(dev, client->adapter,
> +					 client->addr + PM886_PAGE_OFFSET_GPADC);
> +	if (IS_ERR(page))
> +		return dev_err_probe(dev, PTR_ERR(page), "Failed to initialize GPADC page\n");
> +
> +	gpadc->map = devm_regmap_init_i2c(page, &pm886_gpadc_regmap_config);
> +	if (IS_ERR(gpadc->map))
> +		return dev_err_probe(dev, PTR_ERR(gpadc->map),
> +				     "Failed to initialize GPADC regmap\n");
> +
> +	iio->name = "88pm886-gpadc";

> +	iio->dev.of_node = dev->parent->of_node;

No, use device_set_node() with the respective parameters.

But rather debug why firmware node (or OF in your case) is not propagated from
the parent device.

> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->info = &pm886_gpadc_iio_info;
> +	iio->channels = pm886_gpadc_channels;
> +	iio->num_channels = ARRAY_SIZE(pm886_gpadc_channels);
> +
> +	pm_runtime_set_autosuspend_delay(dev, 50);
> +	pm_runtime_use_autosuspend(dev);

Hmm... Shouldn't this be better after enabling PM?

> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register ADC\n");
> +
> +	return 0;
> +}

> +static int pm886_gpadc_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *iio = dev_get_drvdata(dev);
> +	struct pm886_gpadc *gpadc = iio_priv(iio);
> +
> +	return pm886_gpadc_setup(gpadc->map, true);
> +}
> +
> +static int pm886_gpadc_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *iio = dev_get_drvdata(dev);
> +	struct pm886_gpadc *gpadc = iio_priv(iio);
> +
> +	return pm886_gpadc_setup(gpadc->map, false);
> +}

Okay, Now I see that the _setup() is better to be split to two functions with
the proper naming, like _gpadc_hw_enable() and disable.

...

> +#define PM886_GPADC_CONFIG1_EN_ALL (PM886_GPADC_VSC_EN | \
> +				    PM886_GPADC_VBAT_EN | \
> +				    PM886_GPADC_GNDDET1_EN | \
> +				    PM886_GPADC_VBUS_EN | \
> +				    PM886_GPADC_VCHG_PWR_EN | \
> +				    PM886_GPADC_VCF_OUT_EN)

Better formatting is

#define PM886_GPADC_CONFIG1_EN_ALL	\
	(PM886_GPADC_VSC_EN |		\
	 PM886_GPADC_VBAT_EN |		\
	 PM886_GPADC_GNDDET1_EN |	\
	 PM886_GPADC_VBUS_EN |		\
	 PM886_GPADC_VCHG_PWR_EN |	\
	 PM886_GPADC_VCF_OUT_EN)

...

> +#define PM886_GPADC_CONFIG2_EN_ALL (PM886_GPADC_TINT_EN | \
> +				    PM886_GPADC_GPADC0_EN | \
> +				    PM886_GPADC_GPADC1_EN | \
> +				    PM886_GPADC_GPADC2_EN | \
> +				    PM886_GPADC_GPADC3_EN | \
> +				    PM886_GPADC_MIC_DET_EN)

Ditto.

...

> +#define PM886_GPADC_INDEX_TO_BIAS_UA(i)	(1 + (i) * 5)

_uA (yes, it's okay to use small 'u' for clarity).


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

[Duje Mihanović]

On Monday, 1 September 2025 17:35:23 Central European Summer Time Andy Shevchenko wrote:
> On Sun, Aug 31, 2025 at 12:33:05PM +0200, Duje Mihanović wrote:
> > +static const struct regmap_config pm886_gpadc_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = PM886_REG_GPADC_VBAT_SLP + 1,
> 
> What is this + 1 register? Why is it not defined / documented?

It is the second field of the vbat_slp channel.

> > +static int
> > +gpadc_get_resistance_ohm(struct iio_dev *iio, struct iio_chan_spec const
> > *chan) +{
> > +	struct pm886_gpadc *gpadc = iio_priv(iio);
> > +	unsigned int raw_uv, raw_ua;
> > +	int ret;
> > +
> > +	ret = gpadc_set_bias(gpadc, chan->channel, true);
> > +	if (ret)
> > +		goto err;
> > +
> > +	ret = gpadc_find_bias_current(iio, chan, &raw_uv, &raw_ua);
> > +	if (ret)
> > +		goto err;
> > +
> > +	ret = gpadc_set_bias(gpadc, chan->channel, false);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return DIV_ROUND_CLOSEST(raw_uv, raw_ua);
> > +err:
> > +	gpadc_set_bias(gpadc, chan->channel, false);
> 
> You do the same in the other branch and checking there for an error. Why this
> one is so special?

My rationale here was to not override the error from either the first
gpadc_set_bias() call or the subsequent gpadc_find_bias_current() call.

> > +		dev_dbg(&iio->dev, "chan: %d, raw: %d\n", chan->channel, *val);
> 
> How is this useful? The userspace gets the same value. Do you expect problems
> on its way to the user space?

That's a leftover from debugging v1, will drop.

> > +	iio->dev.of_node = dev->parent->of_node;
> 
> No, use device_set_node() with the respective parameters.
> 
> But rather debug why firmware node (or OF in your case) is not propagated from
> the parent device.

I guess it is because the IIO device is registered as a child of the
GPADC platform device, which does not have a node unlike the PMIC
device (GPADC pdev's parent). It seems that the regulator cell
registers its regulators directly under the PMIC dev, so maybe I should
do the same here with the IIO dev?

Regards,
--
Duje

Re: [PATCH v2 2/3] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

[Jonathan Cameron]

On Mon, 1 Sep 2025 15:43:08 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Sun, Aug 31, 2025 at 10:19:38PM +0200, Duje Mihanović wrote:
> > On Sunday, 31 August 2025 21:24:54 Central European Summer Time Karel Balej wrote:  
> > > Duje Mihanović, 2025-08-31T12:33:05+02:00:  
> 
> ...
> 
> > > > +	iio->dev.of_node = dev->parent->of_node;  
> > > 
> > > Didn't you mean to drop this since Jonathan pointed out that it's done
> > > by the core?  
> > 
> > Actually I have found that device tree consumers fail to get their IO
> > channels without this line, so I left it.  
> 
> because the passed device is not parent?

Good point. I missed it seems to be going up another level.
Absolutely fine to override in that case.

> 
> In any case this line is problematic. Use device_set_node() instead with the
> proper dev_fwnode() parameter.
> 

Re: [PATCH v2 2/3] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

[Jonathan Cameron]

On Sun, 31 Aug 2025 12:33:05 +0200
Duje Mihanović <duje@dujemihanovic.xyz> wrote:

> Marvell's 88PM886 PMIC has a so-called General Purpose ADC used for
> monitoring various system voltages and temperatures. Add the relevant
> register definitions to the MFD header and a driver for the ADC.
> 
> Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>

A few additional comments from me inline.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/88pm886-gpadc.c b/drivers/iio/adc/88pm886-gpadc.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..4622d2525e0edeed89c6e6d43336b177590aa885
> --- /dev/null
> +++ b/drivers/iio/adc/88pm886-gpadc.c

> +
> +#include <linux/mfd/88pm886.h>
> +
> +struct pm886_gpadc {
> +	struct regmap *map;
> +};
> +
> +static const int pm886_gpadc_regs[] = {
> +	PM886_REG_GPADC_VSC,
> +	PM886_REG_GPADC_VCHG_PWR,
> +	PM886_REG_GPADC_VCF_OUT,
> +	PM886_REG_GPADC_VBAT,
> +	PM886_REG_GPADC_VBAT_SLP,
> +	PM886_REG_GPADC_VBUS,
> +
> +	PM886_REG_GPADC_GPADC0,
> +	PM886_REG_GPADC_GPADC1,
> +	PM886_REG_GPADC_GPADC2,
> +	PM886_REG_GPADC_GPADC3,
> +
> +	PM886_REG_GPADC_GND_DET1,
> +	PM886_REG_GPADC_GND_DET2,
> +	PM886_REG_GPADC_MIC_DET,
> +
> +	PM886_REG_GPADC_TINT,
> +};
> +
> +/* Must be kept in sync with the table above */
Define this first and use it to assign the table entries
[VSC_CHAN] = PM886_REG_GPADC_VSC, 
etc and then no need for the comment as they will naturally

> +enum pm886_gpadc_channel {
> +	VSC_CHAN,
> +	VCHG_PWR_CHAN,
> +	VCF_OUT_CHAN,
> +	VBAT_CHAN,
> +	VBAT_SLP_CHAN,
> +	VBUS_CHAN,
> +
> +	GPADC0_CHAN,
> +	GPADC1_CHAN,
> +	GPADC2_CHAN,
> +	GPADC3_CHAN,
> +
> +	GND_DET1_CHAN,
> +	GND_DET2_CHAN,
> +	MIC_DET_CHAN,
> +
> +	TINT_CHAN,
> +};

> +static int gpadc_get_raw(struct iio_dev *iio, enum pm886_gpadc_channel chan)
> +{
> +	struct pm886_gpadc *gpadc = iio_priv(iio);
> +	__be16 buf;
> +	int ret;
> +
> +	ret = regmap_bulk_read(gpadc->map, pm886_gpadc_regs[chan], &buf, sizeof(buf));
> +	return !ret ? be16_to_cpu(buf) >> 4 : ret;

At very least flip the logic, probably better not using a ternary at all though.

> +}

>
> +static int
> +__pm886_gpadc_read_raw(struct iio_dev *iio, struct iio_chan_spec const *chan,
> +		       int *val, int *val2, long mask)
> +{
> +	unsigned long lsb = chan->address;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = gpadc_get_raw(iio, chan->channel);
> +		if (*val < 0)
> +			return *val;
> +
> +		dev_dbg(&iio->dev, "chan: %d, raw: %d\n", chan->channel, *val);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = lsb;
> +		*val2 = (MICRO / MILLI);
> +		return chan->type == IIO_VOLTAGE
> +		       ? IIO_VAL_FRACTIONAL
> +		       : IIO_VAL_INT;

Split it so that in the IIO_VAL_INT case you don't set val2.
This is a case where burning a few lines for clearer code is a good idea.
Similar to the other case Andy pointed out.

> +
> +static int pm886_gpadc_setup(struct regmap *map, bool enable)
I wonder if you should just split this as only 1-2 lines are actually shared

static int pm886_gpadc_enable(struct regmap *map)
{
	const u8 config[] = {
		PM886_GPADC_CONFIG1_EN_ALL,
		PM886_GPADC_CONFIG2_EN_ALL,
		PM886_GPADC_GND_DET2_EN
	};
	int ret;

	/* Enable the ADC block */
	ret = regmap_set_bits(map, PM886_REG_GPADC_CONFIG(0x6), BIT(0));
	if (ret)
		return ret;

	/* Enable all channels */
	return regmap_bulk_write(map, PM886_REG_GPADC_CONFIG(0x1), config, ARRAY_SIZE(config));
}

static int pm886_gpadc_disable(struct regmap *map)
{
	return regmap_clear_bits(map, PM886_REG_GPADC_CONFIG(0x6), BIT(0));
}

Which I think ends up only 1 line longer than the more complex combined function.

> +{
> +	const u8 config[] = {
> +		PM886_GPADC_CONFIG1_EN_ALL,
> +		PM886_GPADC_CONFIG2_EN_ALL,
> +		PM886_GPADC_GND_DET2_EN
> +	};
> +	int ret;
> +
> +	/* Enable/disable the ADC block */
> +	ret = regmap_assign_bits(map, PM886_REG_GPADC_CONFIG(0x6), BIT(0), enable);
> +	if (ret)
> +		return ret;
> +
> +	if (!enable)
> +		return 0;
> +
> +	/* If enabling, enable all channels */
> +	return regmap_bulk_write(map, PM886_REG_GPADC_CONFIG(0x1), config, ARRAY_SIZE(config));
> +}

> +static int pm886_gpadc_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *iio = dev_get_drvdata(dev);
> +	struct pm886_gpadc *gpadc = iio_priv(iio);
> +
> +	return pm886_gpadc_setup(gpadc->map, true);
> +}
> +
> +static int pm886_gpadc_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *iio = dev_get_drvdata(dev);
> +	struct pm886_gpadc *gpadc = iio_priv(iio);
> +
> +	return pm886_gpadc_setup(gpadc->map, false);
> +}

Re: [PATCH v2 2/3] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

[Andy Shevchenko]

On Mon, Sep 01, 2025 at 06:51:19PM +0200, Duje Mihanović wrote:
> On Monday, 1 September 2025 17:35:23 Central European Summer Time Andy Shevchenko wrote:
> > On Sun, Aug 31, 2025 at 12:33:05PM +0200, Duje Mihanović wrote:

...

> > > +	.max_register = PM886_REG_GPADC_VBAT_SLP + 1,
> > 
> > What is this + 1 register? Why is it not defined / documented?
> 
> It is the second field of the vbat_slp channel.

Can you define it separately? Or define _MAX_REGISTER to be equal to that and
put a comment that _VBAT_SLP takes two (byte) offsets.

...

> > > +err:
> > > +	gpadc_set_bias(gpadc, chan->channel, false);
> > 
> > You do the same in the other branch and checking there for an error. Why this
> > one is so special?
> 
> My rationale here was to not override the error from either the first
> gpadc_set_bias() call or the subsequent gpadc_find_bias_current() call.

Maybe this needs splitting / refactoring to call bias setting separately?

...

> > > +	iio->dev.of_node = dev->parent->of_node;
> > 
> > No, use device_set_node() with the respective parameters.
> > 
> > But rather debug why firmware node (or OF in your case) is not propagated from
> > the parent device.
> 
> I guess it is because the IIO device is registered as a child of the
> GPADC platform device, which does not have a node unlike the PMIC
> device (GPADC pdev's parent). It seems that the regulator cell
> registers its regulators directly under the PMIC dev, so maybe I should
> do the same here with the IIO dev?

Maybe. Not an expert in DT.

-- 
With Best Regards,
Andy Shevchenko