Re: [PATCH v1 3/4] iio: adc: Add support for MAX8907 ADC cell

[Svyatoslav Ryhel]

пн, 9 лист. 2026 р. о 01:47 Jonathan Cameron <jic23@xxxxxxxxxx> пише:
>
> On Sun,  8 Nov 2026 11:56:33 +0200
> Svyatoslav Ryhel <clamor95@xxxxxxxxx> wrote:
>
> > The MAX8907 PMIC has a dedicated ADC cell, which provides 8 ADC channels
> > with information about different parts of the power system, including the
> > battery, charger/plug, jack and thermal readings.
>
> This is an unusual driver structure.  Seems like we really have
> a number of different i2c devices inside one package.  Is the only
> unified bit the power control?
>

This is a usual structure for MFD device, two and tree address devices
is not smth unique in this subsystem.

> I appreciate you just want to add a new feature to this ancient
> driver but it does feel like things would much simpler if the
> i2c regmap was registered in here rather than the parent using
> a platform device to wrap what seems to be a largely independent
> i2c device.
>

It is not independent. Parent still has control over all i2c addresses
physically. Additionally, this i2c client also has unimplemented
touchscreen capabilities.

> I'll be looking from some input from Lee for the MFD aspects and
> whether he is happy with adding stuff to this driver with its
> current structure.  Doesn't make that much difference to the IIO
> part either way.
>
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
>
> Other than that various minor things inline.
>
>
> > diff --git a/drivers/iio/adc/max8907-adc.c b/drivers/iio/adc/max8907-adc.c
> > new file mode 100644
> > index 0000000000000..d1acb5bc95c15
> > --- /dev/null
> > +++ b/drivers/iio/adc/max8907-adc.c
> > @@ -0,0 +1,195 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/kernel.h>
>
> Don't include that.  Figure out which more specific
> headers in are in use and include those instead.
>
> > +#include <linux/module.h>
> > +#include <linux/mfd/max8907.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +
> > +/* MAX8907_ADC_RES_CNFG1 */
> > +#define RES_AUX1                     BIT(7)
> > +#define RES_AUX2                     BIT(6)
> > +#define RES_VCHG                     BIT(5)
> > +#define RES_VBBATT                   BIT(4)
> > +#define RES_VMBATT                   BIT(3)
> > +#define RES_ISNS                     BIT(2)
> > +#define RES_THM                              BIT(1)
> > +#define RES_TDIE                     BIT(0)
> > +
> > +/* MAX8907_ADC_AVG_CNFG1 */
> > +#define AVG_AUX1_EN                  BIT(7)
> > +#define AVG_AUX2_EN                  BIT(6)
> > +#define AVG_VCHG_EN                  BIT(5)
> > +#define AVG_VBBATT_EN                        BIT(4)
> > +#define AVG_VMBATT_EN                        BIT(3)
> > +#define AVG_ISNS_EN                  BIT(2)
> > +#define AVG_THM_EN                   BIT(1)
> > +#define AVG_TDIE_EN                  BIT(0)
> > +
> > +/* MAX8907_ADC_ACQ_CNFG1 */
> > +#define T_ACQ_AUX1                   BIT(7)
> > +#define T_ACQ_AUX2                   BIT(5)
> > +#define T_ACQ_VCHG                   BIT(3)
> > +#define T_ACQ_VBBATT                 BIT(1)
> > +
> > +/* MAX8907_ADC_ACQ_CNFG2 */
> > +#define T_ACQ_VMBATT                 BIT(6)
> > +#define T_ACQ_ISNS                   BIT(4)
> > +#define T_ACQ_THM                    BIT(2)
> > +#define T_ACQ_TDIE                   BIT(0)
> > +
> > +/* MAX8907_ADC_SCHED */
> > +#define AUTOSCH_ENABLE                       BIT(1)
> > +#define AUTOSCH_TIME                 BIT(0)
> > +
> > +#define TO_REG(n)                    ((n) & 0xff)
> > +#define TO_MEASURE(n)                        (((n) >> 8) & 0xff)
>
> Similar to below, Give this a prefix.  Same for all the defines
> above.
>
> > +
> > +enum {
> > +     TDIE, THM, ISNS, VMBATT, VBBATT, VCHG, AUX2, AUX1
>
> I'd prefix these. Chances of a naming clash in the long
> run are high enough that usual IIO style is to prefix pretty
> much everything with the driver name.
>

I have not prefixed there intentionally to be able to use them as both
index and in the register naming in the channel macros later in the
code.

> > +};
> > +
> > +static const u32 max8907_default_ref[] = {
> Index these off the enum. Makes it easier to check what
> the value is for a specific channel.
>         [MAX8907_TDIE] = 4096,
>         [...]
>

Sure.

> > +     4096, 4096, 4096, 8192, 8192, 8192, 2500, 4096
> > +};
> > +
> > +#define MAX8907_CHAN(_type, _idname, _datasheet_name) {              \
> > +     .type = (_type),                                        \
> > +     .address = MAX8907_REG_##_idname##_MSB |                \
> > +                MAX8907_REG_##_idname##_MEASURE << 8,        \
> > +     .indexed = 1,                                           \
> > +     .channel = (_idname),                                   \
> > +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |          \
> > +                           BIT(IIO_CHAN_INFO_SCALE),         \
> > +     .scan_index = (_idname),                                \
> > +     .scan_type = {                                          \
> > +             .format = IIO_SCAN_FORMAT_UNSIGNED_INT,         \
> > +             .realbits = 12,                                 \
> > +             .storagebits = 16,                              \
> > +             .endianness = IIO_CPU,                          \
> > +     },                                                      \
> > +     .datasheet_name = (_datasheet_name),                    \
> > +}
> > +
> > +static const struct iio_chan_spec max8907_adc_channels[] = {
> > +     MAX8907_CHAN(IIO_VOLTAGE, TDIE,   "tdiode"),
> > +     MAX8907_CHAN(IIO_VOLTAGE, THM,    "thermistor"),
> > +     MAX8907_CHAN(IIO_CURRENT, ISNS,   "isense"),
> > +     MAX8907_CHAN(IIO_VOLTAGE, VMBATT, "vmbatt"),
> > +     MAX8907_CHAN(IIO_VOLTAGE, VBBATT, "vbbatt"),
> > +     MAX8907_CHAN(IIO_VOLTAGE, VCHG,   "vcharge"),
> > +     MAX8907_CHAN(IIO_VOLTAGE, AUX2,   "aux2"),
> > +     MAX8907_CHAN(IIO_VOLTAGE, AUX1,   "aux1"),
> > +};
> > +
> > +static int max8907_adc_read(struct iio_dev *indio_dev,
> > +                         struct iio_chan_spec const *chan,
> > +                         int *val, int *val2, long mask)
> > +{
> > +     struct regmap **regmap = iio_priv(indio_dev);
> > +     unsigned char data[2] = { 0, 0 };
>
> No need for those 0.  = { }; is defined to do the same thing.
> There are some subtleties around that, but the kernel has used
> build options for a long time (and self tests to ensure that form
> is fine).  It is also part of more recent C specs.
>
> Use u8 rather than char.
>

Noted

>
> > +     int ret;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             ret = regmap_write(*regmap, TO_MEASURE(chan->address), 0);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = regmap_bulk_read(*regmap, TO_REG(chan->address), data, 2);
>
> sizeof(data);
>

Noted

> > +             if (ret)
> > +                     return ret;
> > +
> > +             *val = ((data[0] << 8) | data[1]) >> 4;
>
> Use get_unaligned_be16() then a shift.
> better still use a __be16 as the buffer.
>

Noted

> > +
> > +             return IIO_VAL_INT;
> > +
> > +     case IIO_CHAN_INFO_SCALE:
> > +             *val = max8907_default_ref[chan->channel];
> > +             *val2 = chan->scan_type.realbits;
> > +
> > +             return IIO_VAL_FRACTIONAL_LOG2;
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +
> > +static const struct iio_info max8907_adc_info = {
> > +     .read_raw = &max8907_adc_read,
> > +};
> > +
> > +static int max8907_adc_init(struct regmap *regmap)
> > +{
> > +     u32 value;
> > +     int ret;
> > +
> > +     /* Enable internal voltage reference. Exact purpose of bits is not known. */
> > +     ret = regmap_update_bits(regmap, MAX8907_REG_TSC_CNFG1, GENMASK(1, 0), BIT(1));
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Check for internal reference IRQ */
> > +     ret = regmap_read_poll_timeout(regmap, MAX8907_REG_TSC_STA_INT, value,
> > +                                    !(value & MAX8907_MASK_NREF_OK), 0, 1000);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Enable ADC scheduler with 1 second interval */
> > +     ret = regmap_update_bits(regmap, MAX8907_REG_ADC_SCHED,
> > +                              AUTOSCH_ENABLE | AUTOSCH_TIME, AUTOSCH_TIME);
>
> return regmap_update_bits()
>
> Bit odd to unset AUTOSCH_ENABLE in something that is commented as turning
> it on.  That is probably a field that needs a rename.
> I'd also like those field names to reflect that they are in ADC_SCHED.
>

I agree, but max8907 unfortunately has no available datasheet, and
these names are from downstream kernel.

>
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> > +}
> > +
> > +static int max8907_adc_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev = &pdev->dev;
> > +     struct max8907 *max8907 = dev_get_drvdata(dev->parent);
> > +     struct iio_dev *indio_dev;
> > +     struct regmap **regmap;
> > +     int ret;
> > +
> > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*regmap));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     regmap = iio_priv(indio_dev);
> > +
> > +     *regmap = max8907->regmap_adc;
> > +
> > +     indio_dev->modes = INDIO_DIRECT_MODE;
> > +     indio_dev->channels = max8907_adc_channels;
> > +     indio_dev->num_channels = ARRAY_SIZE(max8907_adc_channels);
> > +     indio_dev->name = dev_name(dev);
> > +     indio_dev->info = &max8907_adc_info;
> > +
> > +     iio_device_set_parent(indio_dev, dev->parent);
>
> Why this choice for the parent?  What is wrong with having the
> platform device in between?  Needs a comment.
>

To be able to use pmic phandle as adc reference since adc has no
dedicated node. I have tried without, it failed. I will add comment
above.

> > +
> > +     /* Enable ADCREF */
> If it's called ADCREF can we just include that as part of the field
> name and remove need for the comment?
> > +     ret = regmap_set_bits(max8907->regmap_gen, MAX8907_REG_RESET_CNFG,
> > +                           MAX8907_MASK_RST_CFG_INT_REF_EN);
>
> Need to be more consistent with register vs field names. I'm fine
> with either form but it should be the same for how you shorten
> RESET_CONFIG
>

As with ADC_SCHED

>
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = max8907_adc_init(max8907->regmap_adc);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return devm_iio_device_register(dev, indio_dev);
> > +}
>
>