Re: [PATCH v12 2/3] iio: adc: max14001: New driver

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 29 Sep 2025 02:59:37 -0300
Marilene Andrade Garcia <marilene.agarcia@gmail.com> wrote:

> The MAX14001/MAX14002 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs. In addition to ADC readings, the MAX14001/MAX14002 offers
> more features, like a binary comparator, a filtered reading that can
> provide the average of the last 2, 4, or 8 ADC readings, and an inrush
> comparator that triggers the inrush current. There is also a fault feature
> that can diagnose seven possible fault conditions. And an option to select
> an external or internal ADC voltage reference.
> 
> MAX14001/MAX14002 features implemented so far:
> - Raw ADC reading.
> - Filtered ADC average reading with the default configuration.
> - MV fault disable.
> - Selection of external or internal ADC voltage reference, depending on
> whether it is declared in the device tree.
> 
> Co-developed-by: Kim Seer Paller <kimseer.paller@analog.com>
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> Signed-off-by: Marilene Andrade Garcia <marilene.agarcia@gmail.com>
> ---
> 
> Hello maintainers,
> Thank you for reviewing v11 and for your suggestions.
> I believe I have addressed most of the requested code changes in this v12.
> There were some discussions about a few of them, and I tried to follow the
> path that you seemed to agree with.
> 
> 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?
regmap allows specification of which registers readable and which writeable.
If this is a concern then you could do that.  I'd not worry too much though
as those regions are only accessed by the debugfs interface and that provides
many other foot guns!

Just one trivial comment to add to David's more detailed review and questions.

> 
> Notes:
> As suggested by Andy, I have chosen to use the code "if (ret == -ENODEV)" 
> rather than "if (ret < 0)" on line 312, because it produces a slightly smaller
> max14001.o file compared to the other approach (10640 bytes vs. 10648 bytes).
> Additionally, as mentioned, it is more explicit check.
> 
> As suggested by David, I added support for SPI_LSB_FIRST, and I also used a
> union to avoid clang compiler warnings related to casts between __le16,
> __be16, and u16. Thank you for the code examples.
> 
> 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.
> 
> Main changes since v11:
> - I think I fixed the alphabetical order in the files pointed.
> - Fixed small issues in the include files.
> - Removed the mutex since regmap has a lock mechanism (also removed the 
> mutex include).
> - Added support for SPI_LSB_FIRST in case it is used in a device tree file.


> diff --git a/drivers/iio/adc/max14001.c b/drivers/iio/adc/max14001.c
> new file mode 100644
> index 000000000000..52584c24fb08
> --- /dev/null
> +++ b/drivers/iio/adc/max14001.c

...


> +static int max14001_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct max14001_state *st;
> +	int ret = 0;

Set before use I think in all paths below. So can drop init here.

> +	bool use_ext_vrefin = false;