Message-ID: <aOMg87Mds-Ww3IT4@debian-BULLSEYE-live-builder-AMD64>
Date: Sun, 5 Oct 2025 22:52:51 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Marilene Andrade Garcia <marilene.agarcia@...il.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org,
	Kim Seer Paller <kimseer.paller@...log.com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Jonathan Cameron <jic23@...nel.org>,
	David Lechner <dlechner@...libre.com>,
	Nuno Sá <nuno.sa@...log.com>,
	Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Marcelo Schmitt <Marcelo.Schmitt@...log.com>,
	Ceclan Dumitru <dumitru.ceclan@...log.com>,
	Jonathan Santos <Jonathan.Santos@...log.com>,
	Dragos Bogdan <dragos.bogdan@...log.com>
Subject: Re: [PATCH v12 2/3] iio: adc: max14001: New driver

Hi Marilene,

...
> I have one remaining question related to the max_register attribute of the
> regmap. The register regions that can be accessed are 0x00–0x0c and
> 0x13–0x1a. As suggested, I used max_register to set the upper limit of the
> register region that can be accessed (0x1a). Beyond this address, there is
> a reserved region that should not be used (0x1b–0x1f). However, there is 
> also a reserved region that should not be used between addresses 0x0d–0x12.
> Should I use something to mark this region in the regmap?
> 

Yes, use rd_table and wr_table as suggested by David.
If you haven't implemented it already, I think the reg ranges would look like

static const struct regmap_range max14001_regmap_rd_range[] = {
	regmap_reg_range(MAX14001_REG_ADC, MAX14001_REG_ENBL),
	regmap_reg_range(MAX14001_REG_WEN, MAX14001_REG_WEN),
	regmap_reg_range(MAX14001_REG_VERIFICATION(MAX14001_REG_FLTEN),
			 MAX14001_REG_VERIFICATION(MAX14001_REG_ENBL)),
};

static const struct regmap_range max14001_regmap_wr_range[] = {
	regmap_reg_range(MAX14001_REG_FLTEN, MAX14001_REG_WEN),
	regmap_reg_range(MAX14001_REG_VERIFICATION(MAX14001_REG_FLTEN),
			 MAX14001_REG_VERIFICATION(MAX14001_REG_ENBL)),
};

see drivers/iio/adc/ad4030.c for an example.

> 
> I tested it on the Raspberry Pi modified kernel version rpi-6.12 with
> Raspberry Pi 5 hardware, using the MAX14001PMB evaluation board, and it
> seems to work fine.
> 

I finally managed to give this driver a try.

max14001_disable_mv_fault() does make the memory fault flag (MV) clear out.
Both register read and register write work fine through debugfs.
Though, even after I plugged a bench top power supply to the ADC inputs, I read
0x80 from the FLAGS (0x02) register. That flag indicates "input voltage detected
without input current" so maybe I missed or miss-connected some inputs.

The _scale looks good. Eval board data sheet says each ADC input is DC offset by
VREF/2 (1.25 / 2 == 0.625 Volts), and I do get readings that float between
0.623779 and 0.629883 volts. We know that the PMB board also significantly
attenuates the signal. At 10V input, I start getting data that scales to
0.631104 volts (_raw == 517). And it looks the same if I go to 20V. Interesting
thing is, the readings peak only after I disable the power supply output. One of
the eval board user guides [1] has an example connection diagram to an AC
voltage/current circuit, but no example connecting to a DC circuit. So, maybe
I'm just being silly trying to test this with a DC supply. Though, despite the
outcome of my rudimentary tests [2], the driver provides a proper _scale and
grabs sample data from the correct register.

[1]: https://www.analog.com/media/en/reference-design-documentation/design-notes/ds25-isolated-adc-with-integrated-dc-dc-converter-simplifies-field-side-circuitry.pdf
[2]: https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/max14001

Thus,
Tested-by: Marcelo Schmitt <marcelo.schmitt1@...il.com>


About the data averaging, my suggestion is to remove _AVERAGE_RAW stuff from the
initial driver support patch and (optionally) implement it on a follow up patch.
It will make it easier, faster, and safer to get this driver merged without the
averaging thing. max14001 averaging feature is uncommon. Even though we have
in_Y_mean_raw documented in IIO ABI, adding more interfaces is a commitment.
Once it's there and people start using it, we cannot change. 'We don't break
user space'. So, the Linux kernel community may want to think about it
thoroughly before committing to new ABI. The max14001 driver don't need to be on
hold until a decision about max14001 averaging interface is made.

> +static int max14001_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct max14001_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_AVERAGE_RAW:
Drop/remove IIO_CHAN_INFO_AVERAGE_RAW from the initial driver patch.
Optionally, add IIO_CHAN_INFO_AVERAGE_RAW on a follow up patch.

> +		ret = regmap_read(st->regmap, MAX14001_REG_FADC, val);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
...
> +static const struct iio_chan_spec max14001_channel[] = {
> +	{
> +		.type = IIO_VOLTAGE,
> +		.indexed = 1,
> +		.channel = 0,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_AVERAGE_RAW) |
Same reasoning here. Can be introduced in a separate patch.

> +				      BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +};

With the averaging related stuff dropped from the initial driver support,
Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@...il.com>

btw, well done.

Powered by blists - more mailing lists