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

[Jonathan Cameron <jic23@kernel.org>]

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);
> +}