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

Andy Shevchenko
2025-09-16T07:58:18Z

On Tue, Sep 16, 2025 at 1:16 AM 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.

This version looks almost good to me, a few nit-picks below.

...

> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -87,6 +87,7 @@ obj-$(CONFIG_MAX11100) += max11100.o
> obj-$(CONFIG_MAX1118) += max1118.o
> obj-$(CONFIG_MAX11205) += max11205.o
> obj-$(CONFIG_MAX11410) += max11410.o
> +obj-$(CONFIG_MAX14001) += max14001.o
> obj-$(CONFIG_MAX1241) += max1241.o
> obj-$(CONFIG_MAX1363) += max1363.o
> obj-$(CONFIG_MAX34408) += max34408.o

Please, keep it ordered.

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitrev.h>
> +#include <linux/bits.h>

> +#include <linux/byteorder/generic.h>

This is wrong, should be asm/byteorder.h going after linux/* but
before linux/iio/* ones...

> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +#include <linux/units.h>

...here

asm/byteorder.h

> +#include <linux/iio/iio.h>
> +#include <linux/iio/types.h>

...

> +struct max14001_state {
> + const struct max14001_chip_info *chip_info;
> + struct spi_device *spi;
> + struct regmap *regmap;
> + int vref_mV;
> + /*
> + * lock protect against multiple concurrent accesses, RMW sequence,

Lock to protect...

> + * and SPI transfer.
> + */
> + struct mutex lock;
> + /*
> + * The following buffers will be bit-reversed during device
> + * communication, because the device transmits and receives data
> + * LSB-first.
> + * DMA (thus cache coherency maintenance) requires the transfer
> + * buffers to live in their own cache lines.
> + */
> + __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
> + __be16 spi_rx_buffer;
> +};

...

> +static struct max14001_chip_info max14001_chip_info = {
> + .name = "max14001",
> +};
> +
> +static struct max14001_chip_info max14002_chip_info = {
> + .name = "max14002",
> +};

These can be moved closer to their first user (ID table?).

...

> +static int max14001_write(struct max14001_state *st, unsigned int reg, unsigned int val)
> +{
> + /*
> + * Prepare SPI transmit buffer 16 bit-value big-endian format and
> + * reverses bit order to align with the LSB-first input on SDI port

reverse

> + * in order to meet the device communication requirements.
> + */
> + st->spi_tx_buffer = cpu_to_be16(bitrev16(FIELD_PREP(MAX14001_MASK_ADDR, reg) |
> + FIELD_PREP(MAX14001_MASK_WR, 1) |
> + FIELD_PREP(MAX14001_MASK_DATA, val)));
> +
> + return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
> +}

...

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

bool use_ext_vrefin = false;

> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> + st->chip_info = spi_get_device_match_data(spi);

> + if (!st->chip_info)
> + return dev_err_probe(dev, -ENODEV, "Failed to get match data\n");

Remove this check, it will be almost always a dead code. The developer
implementing a new device support won't be able to test the driver
anyway with properly given chip_info.

> + indio_dev->name = st->chip_info->name;
> + indio_dev->info = &max14001_info;
> + indio_dev->channels = max14001_channel;
> + indio_dev->num_channels = ARRAY_SIZE(max14001_channel);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + st->regmap = devm_regmap_init(dev, NULL, st, &max14001_regmap_config);
> + if (IS_ERR(st->regmap))
> + return dev_err_probe(dev, PTR_ERR(st->regmap), "Failed to initialize regmap\n");
> +
> + ret = devm_mutex_init(dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable Vdd supply\n");
> +
> + ret = devm_regulator_get_enable(dev, "vddl");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable Vddl supply\n");
> +
> + ret = devm_regulator_get_enable_read_voltage(dev, "refin");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "Failed to get REFIN voltage\n");
> +
> + if (ret < 0)

if (ret == -ENODEV) ?
It would be interesting out of curiosity to see bloat-o-meter output
for the original check and my proposal. I would choose one which gives
less code, but in case of equality, I would rather go with a more
explicit (my proposal) check.

> + ret = 1250000;
> + else
> + ext_vrefin = 1;

use_ext_vrefin = true;

> + st->vref_mV = ret / (MICRO / MILLI);
> +
> + if (ext_vrefin) {

if (use_ext_vrefin) {

> + /*
> + * Configure the MAX14001/MAX14002 to use an external voltage reference source
> + * by setting the bit 5 of the configuration register

Missing period.

> + */
> + ret = regmap_update_bits(st->regmap, MAX14001_REG_CFG, MAX14001_REG_CFG_BIT_EXRF, MAX14001_REG_CFG_BIT_EXRF);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to set External REFIN in Configuration Register\n");
> + }
> +
> + ret = max14001_disable_mv_fault(st);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to disable MV Fault\n");
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}

--
With Best Regards,
Andy Shevchenko