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>
---
v3:
- Add indices to pm886_gpadc_regs[] table
- Tidy up macro definitions
- Define MAX_REGISTER
- Drop some ternary operators
- raw_u{v,a} -> raw_u{V,A}
- Reorder gpadc_get_resistance_ohm()
- Split pm886_gpadc_setup() into hw_{en,dis}able()
- Set firmware node the right way

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 | 388 ++++++++++++++++++++++++++++++++++++++++
 drivers/iio/adc/Kconfig         |  13 ++
 drivers/iio/adc/Makefile        |   1 +
 include/linux/mfd/88pm886.h     |  58 ++++++
 5 files changed, 465 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6dcfbd11efef87927041f5cf58d70633dbb4b18d..0f7b78865c77967683ffc92f4822e722a4f723b6 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..8c1fa99b94415f5f193ec65d313fab5001f40d58
--- /dev/null
+++ b/drivers/iio/adc/88pm886-gpadc.c
@@ -0,0 +1,388 @@
+// 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;
+};
+
+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 const int pm886_gpadc_regs[] = {
+	[VSC_CHAN] = PM886_REG_GPADC_VSC,
+	[VCHG_PWR_CHAN] = PM886_REG_GPADC_VCHG_PWR,
+	[VCF_OUT_CHAN] = PM886_REG_GPADC_VCF_OUT,
+	[VBAT_CHAN] = PM886_REG_GPADC_VBAT,
+	[VBAT_SLP_CHAN] = PM886_REG_GPADC_VBAT_SLP,
+	[VBUS_CHAN] = PM886_REG_GPADC_VBUS,
+
+	[GPADC0_CHAN] = PM886_REG_GPADC_GPADC0,
+	[GPADC1_CHAN] = PM886_REG_GPADC_GPADC1,
+	[GPADC2_CHAN] = PM886_REG_GPADC_GPADC2,
+	[GPADC3_CHAN] = PM886_REG_GPADC_GPADC3,
+
+	[GND_DET1_CHAN] = PM886_REG_GPADC_GND_DET1,
+	[GND_DET2_CHAN] = PM886_REG_GPADC_GND_DET2,
+	[MIC_DET_CHAN] = PM886_REG_GPADC_MIC_DET,
+
+	[TINT_CHAN] = PM886_REG_GPADC_TINT,
+};
+
+#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_GPADC_MAX_REGISTER,
+};
+
+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));
+	if (ret)
+		return ret;
+
+	return be16_to_cpu(buf) >> 4;
+}
+
+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;
+
+		/* Wait for the new bias level to apply. */
+		fsleep(5 * USEC_PER_MSEC);
+
+		*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 out;
+
+	ret = gpadc_find_bias_current(iio, chan, &raw_uV, &raw_uA);
+	if (ret)
+		goto out;
+
+	ret = DIV_ROUND_CLOSEST(raw_uV, raw_uA);
+out:
+	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;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = lsb;
+
+		if (chan->type == IIO_VOLTAGE) {
+			*val2 = MILLI;
+			return IIO_VAL_FRACTIONAL;
+		} else {
+			return 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;
+
+		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_hw_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_hw_disable(struct regmap *map)
+{
+	return regmap_clear_bits(map, PM886_REG_GPADC_CONFIG(0x6), BIT(0));
+}
+
+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->modes = INDIO_DIRECT_MODE;
+	iio->info = &pm886_gpadc_iio_info;
+	iio->channels = pm886_gpadc_channels;
+	iio->num_channels = ARRAY_SIZE(pm886_gpadc_channels);
+	device_set_node(&iio->dev, dev_fwnode(dev->parent));
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
+
+	pm_runtime_set_autosuspend_delay(dev, 50);
+	pm_runtime_use_autosuspend(dev);
+	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_hw_enable(gpadc->map);
+}
+
+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_hw_disable(gpadc->map);
+}
+
+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..38892ba7b8a42bbecb53621a891a52a2fd70fd43 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
 
@@ -70,6 +71,63 @@
 #define PM886_LDO_VSEL_MASK		0x0f
 #define PM886_BUCK_VSEL_MASK		0x7f
 
+/* GPADC enable/disable registers */
+#define PM886_REG_GPADC_CONFIG(n)	(n)
+
+#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)
+
+/* 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
+
+/* VBAT_SLP is the last register and is 2 bytes wide like other channels. */
+#define PM886_GPADC_MAX_REGISTER	(PM886_REG_GPADC_VBAT_SLP + 1)
+
+#define PM886_GPADC_BIAS_LEVELS		16
+#define PM886_GPADC_INDEX_TO_BIAS_uA(i)	(1 + (i) * 5)
+
 struct pm886_chip {
 	struct i2c_client *client;
 	unsigned int chip_id;

-- 
2.51.0

Duje Mihanović, 2025-09-05T13:00:55+02:00:
> diff --git a/include/linux/mfd/88pm886.h b/include/linux/mfd/88pm886.h
> index 85eca44f39ab58ba4cb9ec4216118ee9604d021f..38892ba7b8a42bbecb53621a891a52a2fd70fd43 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
>  
> @@ -70,6 +71,63 @@
>  #define PM886_LDO_VSEL_MASK		0x0f
>  #define PM886_BUCK_VSEL_MASK		0x7f
>  
> +/* GPADC enable/disable registers */
> +#define PM886_REG_GPADC_CONFIG(n)	(n)
> +
> +#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)
> +
> +/* 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
> +
> +/* VBAT_SLP is the last register and is 2 bytes wide like other channels. */
> +#define PM886_GPADC_MAX_REGISTER	(PM886_REG_GPADC_VBAT_SLP + 1)
> +
> +#define PM886_GPADC_BIAS_LEVELS		16
> +#define PM886_GPADC_INDEX_TO_BIAS_uA(i)	(1 + (i) * 5)
> +
>  struct pm886_chip {
>  	struct i2c_client *client;
>  	unsigned int chip_id;

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

On Fri, Sep 5, 2025 at 2:01 PM 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.

Jonathan et al. The Q here is do we want to somehow make values
spelling (in the comments and documentation) be unified with the
Scientific Style [1]? Personally I find the article very useful and
one style for the whole subsystem would be good to enforce (in my
humble opinion). Thoughts?

[1]: https://poynton.ca/notes/units/

...

> +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));
> +       if (ret)
> +               return ret;
> +
> +       return be16_to_cpu(buf) >> 4;

+ asm/byteorder.h (after linux/* but before linux/iio/*)

> +}

...

> +               /*
> +                * Vendor kernel errors out above 1.25V, but testing shows that

1.25 V

> +                * the resistance of the battery detection channel (GPADC2 on
> +                * coreprimevelte) reaches about 1.4Mohm when the battery is

1.4 MOhm or even 1.4 MΩ


> +                * 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,

fuel gauge

> +                * increase this limit a bit.
> +                */
> +               if (WARN_ON(*raw_uV > 1500 * (MICRO / MILLI)))

+ bug.h

> +                       return -EIO;

...

> +               /*
> +                * Vendor kernel errors out under 300mV, but for the same

300 mV

> +                * reason as above (except the channel hovers around 3.5kohm

3.5 kOhm / 3.5 kΩ

> +                * 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,

Also add spaces before units.

> +                               *raw_uA, *raw_uV);
> +                       continue;
> +               }
> +
> +               dev_dbg(&iio->dev, "good bias for chan %d: %duA @ %duV\n", chan->channel,
> +                       *raw_uA, *raw_uV);

Ditto.

> +               return 0;
> +       }
> +
> +       dev_err(&iio->dev, "failed to find good bias for chan %d\n", chan->channel);
> +       return -EINVAL;
> +}

...

> +       ret = DIV_ROUND_CLOSEST(raw_uV, raw_uA);

+ math.h

...

> +       page = devm_i2c_new_dummy_device(dev, client->adapter,
> +                                        client->addr + PM886_PAGE_OFFSET_GPADC);
> +       if (IS_ERR(page))

+ err.h

> +               return dev_err_probe(dev, PTR_ERR(page), "Failed to initialize GPADC page\n");


-- 
With Best Regards,
Andy Shevchenko

On Fri, 5 Sep 2025 16:39:05 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Sep 5, 2025 at 2:01 PM 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.  
> 
> Jonathan et al. The Q here is do we want to somehow make values
> spelling (in the comments and documentation) be unified with the
> Scientific Style [1]? Personally I find the article very useful and
> one style for the whole subsystem would be good to enforce (in my
> humble opinion). Thoughts?
> 
> [1]: https://poynton.ca/notes/units/

I should have included in previous reply that I would not want this
driver stalled on that discussion, so let us guess the outcome and
go with the unit formatting suggestions Andy makes in this review.

The question of standardising on this aspect and documenting it can be resolved
in parallel.

Jonathan

On Fri, 5 Sep 2025 16:39:05 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Sep 5, 2025 at 2:01 PM 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.  
> 
> Jonathan et al. The Q here is do we want to somehow make values
> spelling (in the comments and documentation) be unified with the
> Scientific Style [1]? Personally I find the article very useful and
> one style for the whole subsystem would be good to enforce (in my
> humble opinion). Thoughts?
> 
> [1]: https://poynton.ca/notes/units/

Consistency is indeed good, but I'm always careful about putting up
barriers to submission. So I'd prefer starting with guidance and general
review comments about consistent style, but probably not pushing back too
hard initially at least on new code where a consistent slightly different
style is applied. e.g. there is inconsistency in this patch as Volt and
Ampere get capitals but not Ohm (which should given all 3 are named
after people)

To actually move to such a style we'd need to fix up at least some existing
comments but that would be a trade off against churn.

I've been useless for years about putting an IIO maintainer entry profile in place.
https://docs.kernel.org/doc-guide/maintainer-profile.html but that is where
probably where this sort of extra documentation of specific requirements would belong
unless we can get broad agreement across subsystems where comments with units
are common.

Before moving to any standard on this I think we'd want to make sure
we at least don't 'clash' with what is accepted in other subsystems where
these units are common.  +CC some relevant maintainers.

Jonathan

> 
> ...
> 
> > +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));
> > +       if (ret)
> > +               return ret;
> > +
> > +       return be16_to_cpu(buf) >> 4;  
> 
> + asm/byteorder.h (after linux/* but before linux/iio/*)
> 
> > +}  
> 
> ...
> 
> > +               /*
> > +                * Vendor kernel errors out above 1.25V, but testing shows that  
> 
> 1.25 V
> 
> > +                * the resistance of the battery detection channel (GPADC2 on
> > +                * coreprimevelte) reaches about 1.4Mohm when the battery is  
> 
> 1.4 MOhm or even 1.4 MΩ
> 
> 
> > +                * 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,  
> 
> fuel gauge
> 
> > +                * increase this limit a bit.
> > +                */
> > +               if (WARN_ON(*raw_uV > 1500 * (MICRO / MILLI)))  
> 
> + bug.h
> 
> > +                       return -EIO;  
> 
> ...
> 
> > +               /*
> > +                * Vendor kernel errors out under 300mV, but for the same  
> 
> 300 mV
> 
> > +                * reason as above (except the channel hovers around 3.5kohm  
> 
> 3.5 kOhm / 3.5 kΩ
> 
> > +                * 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,  
> 
> Also add spaces before units.
> 
> > +                               *raw_uA, *raw_uV);
> > +                       continue;
> > +               }
> > +
> > +               dev_dbg(&iio->dev, "good bias for chan %d: %duA @ %duV\n", chan->channel,
> > +                       *raw_uA, *raw_uV);  
> 
> Ditto.
> 
> > +               return 0;
> > +       }
> > +
> > +       dev_err(&iio->dev, "failed to find good bias for chan %d\n", chan->channel);
> > +       return -EINVAL;
> > +}  
> 
> ...
> 
> > +       ret = DIV_ROUND_CLOSEST(raw_uV, raw_uA);  
> 
> + math.h
> 
> ...
> 
> > +       page = devm_i2c_new_dummy_device(dev, client->adapter,
> > +                                        client->addr + PM886_PAGE_OFFSET_GPADC);
> > +       if (IS_ERR(page))  
> 
> + err.h
> 
> > +               return dev_err_probe(dev, PTR_ERR(page), "Failed to initialize GPADC page\n");  
> 
>