On Sun, Aug 31, 2025 at 12:33:05PM +0200, Duje Mihanović 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.
...
> +#define ADC_CHANNEL_RESISTANCE(index, lsb, name) { \
Please, make those { on the separate lines
{
> + .type = IIO_RESISTANCE, \
> + .indexed = 1, \
> + .channel = index, \
> + .address = lsb, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> + .datasheet_name = name, \
> +}
Also it's easier to read when \:s are TABbed to the same column.
...
> +static const struct regmap_config pm886_gpadc_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = PM886_REG_GPADC_VBAT_SLP + 1,
What is this + 1 register? Why is it not defined / documented?
> +};
...
> +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;
This is hard to read. Note, the more LoCs is fine as long as it helps (and
actually a lot in this case) readability.
> +}
...
> +static int
> +gpadc_find_bias_current(struct iio_dev *iio, struct iio_chan_spec const
> *chan,
> + unsigned int *raw_uv, unsigned int *raw_ua)
> +{
> + struct pm886_gpadc *gpadc = iio_priv(iio);
> + unsigned int gpadc_num = chan->channel - GPADC0_CHAN;
> + unsigned int reg = PM886_REG_GPADC_CONFIG(0xb + gpadc_num);
> + unsigned long lsb = chan->address;
> + int ret;
> +
> + for (unsigned int i = 0; i < PM886_GPADC_BIAS_LEVELS; i++) {
> + ret = regmap_update_bits(gpadc->map, reg, GENMASK(3, 0), i);
> + if (ret)
> + return ret;
Sleep needs to be explained.
> + fsleep(5000);
5 * USEC_PER_MSEC
> + *raw_ua = PM886_GPADC_INDEX_TO_BIAS_UA(i);
> + *raw_uv = gpadc_get_raw(iio, chan->channel) * lsb;
Can we use uA and uV suffixes?
> + /*
> + * Vendor kernel errors out above 1.25V, but testing shows that
> + * the resistance of the battery detection channel (GPADC2 on
> + * coreprimevelte) reaches about 1.4Mohm when the battery is
> + * removed, which can't be measured with such a low upper
> + * limit. Therefore, to be able to detect the battery without
> + * ugly externs as used in the vendor fuelgauge driver,
> + * increase this limit a bit.
> + */
> + if (WARN_ON(*raw_uv > 1500 * (MICRO / MILLI)))
> + return -EIO;
> +
> + /*
> + * Vendor kernel errors out under 300mV, but for the same
> + * reason as above (except the channel hovers around 3.5kohm
> + * with battery present) reduce this limit.
> + */
> + if (*raw_uv < 200 * (MICRO / MILLI)) {
> + dev_dbg(&iio->dev, "bad bias for chan %d: %duA @
> %duV\n", chan->channel,
> + *raw_ua, *raw_uv);
> + continue;
> + }
> +
> + dev_dbg(&iio->dev, "good bias for chan %d: %duA @ %duV\n",
> chan->channel,
> + *raw_ua, *raw_uv);
> + return 0;
> + }
> +
> + dev_err(&iio->dev, "failed to find good bias for chan %d\n",
> chan->channel);
> + return -EINVAL;
> +}
...
> +static int
> +gpadc_get_resistance_ohm(struct iio_dev *iio, struct iio_chan_spec const
> *chan)
> +{
> + struct pm886_gpadc *gpadc = iio_priv(iio);
> + unsigned int raw_uv, raw_ua;
> + int ret;
> +
> + ret = gpadc_set_bias(gpadc, chan->channel, true);
> + if (ret)
> + goto err;
> +
> + ret = gpadc_find_bias_current(iio, chan, &raw_uv, &raw_ua);
> + if (ret)
> + goto err;
> +
> + ret = gpadc_set_bias(gpadc, chan->channel, false);
> + if (ret)
> + return ret;
> +
> + return DIV_ROUND_CLOSEST(raw_uv, raw_ua);
> +err:
> + gpadc_set_bias(gpadc, chan->channel, false);
You do the same in the other branch and checking there for an error. Why this
one is so special?
> + return ret;
> +}
...
> + dev_dbg(&iio->dev, "chan: %d, raw: %d\n", chan->channel, *val);
How is this useful? The userspace gets the same value. Do you expect problems
on its way to the user space?
...
> + case IIO_CHAN_INFO_SCALE:
> + *val = lsb;
> + *val2 = (MICRO / MILLI);
> + return chan->type == IIO_VOLTAGE
> + ? IIO_VAL_FRACTIONAL
> + : IIO_VAL_INT;
Make it one line, it is much easier to follow or even split to a regular 'if'.
> + case IIO_CHAN_INFO_OFFSET:
> + /* Raw value is 104 millikelvin/LSB, convert it to 104
> millicelsius/LSB */
> + *val = ABSOLUTE_ZERO_MILLICELSIUS;
> + *val2 = lsb;
> + return IIO_VAL_FRACTIONAL;
...
> +static int pm886_gpadc_setup(struct regmap *map, bool enable)
> +{
> + const u8 config[] = {
> + PM886_GPADC_CONFIG1_EN_ALL,
> + PM886_GPADC_CONFIG2_EN_ALL,
> + PM886_GPADC_GND_DET2_EN
Leave trailing comma. This doesn't look like a terminator entry.
> + };
> + 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_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;
No, use device_set_node() with the respective parameters.
But rather debug why firmware node (or OF in your case) is not propagated from
the parent device.
> + 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);
Hmm... Shouldn't this be better after enabling PM?
> + 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;
> +}
> +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);
> +}
Okay, Now I see that the _setup() is better to be split to two functions with
the proper naming, like _gpadc_hw_enable() and disable.
...
> +#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)
Better formatting is
#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_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)
Ditto.
...
> +#define PM886_GPADC_INDEX_TO_BIAS_UA(i) (1 + (i) * 5)
_uA (yes, it's okay to use small 'u' for clarity).
--
With Best Regards,
Andy Shevchenko
