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