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ć <d...@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.
