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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 29 Aug 2025 00:17:41 +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>
Hi Duje

A few quick comments. I've tried not to overlap too much with David.

Jonathan

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

> +static int gpadc_get_raw(struct iio_dev *iio, enum pm886_gpadc_channel chan)
> +{
> +	struct regmap **map = iio_priv(iio);
> +	int val, ret;
> +	u8 buf[2];
> +
> +	if (chan >= GPADC0_RES_CHAN)
> +		/* Resistor voltage drops are read from the corresponding voltage channel */
> +		chan -= GPADC0_RES_CHAN - GPADC0_CHAN;
> +
> +	ret = regmap_bulk_read(*map, regs[chan], buf, 2);
> +
> +	if (ret)
> +		return ret;
> +
> +	val = ((buf[0] & 0xff) << 4) | (buf[1] & 0xf);

No point in masking a u8 by 0xff.

> +	val &= 0xfff;
Can't have any other bits set that I can see so no need for this
final mask.
> +
> +	return val;
> +}
>

> +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 raw, ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
Probably worth a wrapper that handles the pm runtime stuff and
then calls the rest of this code just to allow simple returns
on errors.  I keep promising to spin a series done ACQUIRE() magic
for pm_runtime_resume_and_get with autosuspend but not getting time
to write it :(  For now, wrapper is the way to go.

https://elixir.bootlin.com/linux/v6.17-rc3/source/include/linux/cleanup.h#L330

> +	if (ret)
> +		return ret;
> +
> +	if (chan->type == IIO_RESISTANCE) {
> +		raw = gpadc_get_resistor(iio, chan);
> +		if (raw < 0) {
> +			ret = raw;
> +			goto out;
> +		}
> +
> +		*val = raw;
> +		dev_dbg(&iio->dev, "chan: %d, %d Ohm\n", chan->channel, *val);
> +		ret = IIO_VAL_INT;
> +		goto out;
> +	}
> +
> +	raw = gpadc_get_raw(iio, chan->channel);
> +	if (raw < 0) {
> +		ret = raw;
> +		goto out;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = raw;
> +		dev_dbg(&iio->dev, "chan: %d, raw: %d\n", chan->channel, *val);
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_PROCESSED: {
> +		*val = raw * chan->address;
> +		ret = IIO_VAL_INT;
> +
> +		/*
> +		 * Voltage measurements are scaled into uV. Scale them back
> +		 * into the mV dimension.
> +		 */
> +		if (chan->type == IIO_VOLTAGE)
> +			*val = DIV_ROUND_CLOSEST(*val, 1000);
> +
> +		dev_dbg(&iio->dev, "chan: %d, raw: %d, processed: %d\n", chan->channel, raw, *val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	}
> +
> +out:
> +	pm_runtime_mark_last_busy(dev);

Drop this mark_last_busy.  The put_autosuspend now always calls it
so should never be any reason to call that any more.


> +	pm_runtime_put_autosuspend(dev);
> +	return ret;
> +}

> +
> +static int pm886_gpadc_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev, *parent = dev->parent;
> +	struct pm886_chip *chip = dev_get_drvdata(parent);
> +	struct i2c_client *client = chip->client, *page;
> +	struct regmap **map;
> +	struct iio_dev *iio;
> +	int ret;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*map));
> +	if (!iio)
> +		return -ENOMEM;
> +	map = 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");
> +
> +	*map = devm_regmap_init_i2c(page, &pm886_gpadc_regmap_config);
> +	if (IS_ERR(*map))
> +		return dev_err_probe(dev, PTR_ERR(*map),
> +				     "Failed to initialize GPADC regmap\n");
> +
> +	iio->name = "88pm886-gpadc";
> +	iio->dev.parent = dev;
> +	iio->dev.of_node = parent->of_node;
Done in core code.
https://elixir.bootlin.com/linux/v6.16.3/source/drivers/iio/industrialio-core.c#L2044
__iio_device_register() so unless you use it before that call shouldn't need this.

I'm not sure what it is used for though.

> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->info = &pm886_gpadc_iio_info;
> +	iio->channels = pm886_adc_channels;
> +	iio->num_channels = ARRAY_SIZE(pm886_adc_channels);
> +
> +	devm_pm_runtime_enable(dev);
> +	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;
> +}

>