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

[Jonathan Cameron]

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?  

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.

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.

> +};
> +
> +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,
	[...]

> +	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.


> +	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);

> +		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.

> +
> +		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.


> +	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.

> +
> +	/* 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


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