On Sun, 31 Aug 2025 12:33:05 +0200
Duje Mihanović <d...@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ć <d...@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);
> +}
