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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Sep 02, 2025 at 10:16:12AM -0300, Marilene Andrade Garcia 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.

...

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

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

Please, move this group...

> +#include <linux/kernel.h>

This header must not be used in a driver like this.
Please, replace it with the headers that you are actually used.

> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>


...to be here (and add a blank line above).

...

> +#define MAX14001_REG_VERIFICATION(x)	((x) + 0x10)
> +
> +#define MAX14001_REG_CFG_EXRF		BIT(5)

Is it REG? I'm lost in the naming(s) and value(s) in this driver.

...

> +#define MAX14001_WRITE_WEN		0x294

Also, what's this? Shouldn't it be a (WRITE_EN | 0x94) ?

...

> +enum max14001_chip_model {
> +	max14001,
> +	max14002,
> +};

No need, just make the data structures separate.

...

> +struct max14001_state {
> +	const struct max14001_chip_info *chip_info;
> +	struct spi_device *spi;
> +	int vref_mv;

Can it be _mV? This will follow the real unit spelling.
(And yes, we have such suffixes in the variable names ion the kernel.)

> +	/*
> +	 * lock protect against multiple concurrent accesses, RMW sequence,
> +	 * and SPI transfer.
> +	 */
> +	struct mutex lock;

+ mutex.h

> +	/*
> +	 * 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 int max14001_read(struct max14001_state *st, u16 reg_addr, u16 *reg_data)
> +{
> +	struct spi_transfer xfers[] = {
> +		{
> +			.tx_buf = &st->spi_tx_buffer,
> +			.len = sizeof(st->spi_tx_buffer),
> +			.cs_change = 1,
> +		}, {
> +			.rx_buf = &st->spi_rx_buffer,
> +			.len = sizeof(st->spi_rx_buffer),
> +		},
> +	};
> +	int ret;
> +
> +	/*
> +	 * Prepare SPI transmit buffer 16 bit-value big-endian format and
> +	 * reverses bit order to align with the LSB-first input on SDI port
> +	 * in order to meet the device communication requirements.
> +	 */
> +	st->spi_tx_buffer = FIELD_PREP(MAX14001_MASK_ADDR, reg_addr);
> +	st->spi_tx_buffer = bitrev16(cpu_to_be16(st->spi_tx_buffer));

Use temporary variable. There are two issues with the above:

1) the usual pattern is to avoid putting data to the external buffers /
variables until it's ready (this will be more robust against potential
synchronisation issues);

2) it's simply hard to read and follow; also it's prone to mistakes if
something more comes in the future among these lines.

> +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Convert received 16-bit value from big-endian to cpu-endian format
> +	 * and reverses bit order.
> +	 */
> +	st->spi_rx_buffer = bitrev16(be16_to_cpu(st->spi_rx_buffer));
> +	*reg_data = FIELD_GET(MAX14001_MASK_DATA, st->spi_rx_buffer);
> +
> +	return 0;
> +}

...

> +static int max14001_write(struct max14001_state *st, u16 reg_addr, u16 reg_data)
> +{
> +	/*
> +	 * Prepare SPI transmit buffer 16 bit-value big-endian format and
> +	 * reverses bit order to align with the LSB-first input on SDI port
> +	 * in order to meet the device communication requirements.
> +	 */
> +	st->spi_tx_buffer = FIELD_PREP(MAX14001_MASK_ADDR, reg_addr) |
> +			    FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) |
> +			    FIELD_PREP(MAX14001_MASK_DATA, reg_data);
> +	st->spi_tx_buffer = bitrev16(cpu_to_be16(st->spi_tx_buffer));

Ditto.

> +	return spi_write(st->spi, &st->spi_tx_buffer, sizeof(st->spi_tx_buffer));
> +}

...

> +static int max14001_write_single_reg(struct max14001_state *st, u16 reg_addr,
> +				     u16 reg_data)
> +{
> +	int ret;
> +
> +	/* Enable register write */
> +	ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_WRITE_WEN);
> +	if (ret)
> +		return ret;
> +
> +	/* Write data into register */
> +	ret = max14001_write(st, reg_addr, reg_data);
> +	if (ret)
> +		return ret;

> +	/* Disable register write */
> +	ret = max14001_write(st, MAX14001_REG_WEN, 0);
> +	if (ret)
> +		return ret;
> +
> +	return ret;

Are you expecting anything than 0 here? Why?

Also the whole block is simply

	return max14001_write(st, MAX14001_REG_WEN, 0);

> +}

...

> +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);
> +	u16 reg_data;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:

Use scoped_guard() from cleanup.h.

> +		mutex_lock(&st->lock);
> +		ret = max14001_read(st, MAX14001_REG_ADC, &reg_data);
> +		mutex_unlock(&st->lock);
> +		if (ret)
> +			return ret;
> +
> +		*val = reg_data;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_AVERAGE_RAW:
> +		mutex_lock(&st->lock);
> +		ret = max14001_read(st, MAX14001_REG_FADC, &reg_data);
> +		mutex_unlock(&st->lock);

Ditto.

> +		if (ret)
> +			return ret;
> +
> +		*val = reg_data;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_mv;
> +		*val2 = 10;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +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) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +	}

Leave a trailing comma. It's not a terminator entry.

> +};

...

> +static int max14001_disable_mv_fault(struct max14001_state *st)
> +{
> +	u16 reg_addr;

It's enough to call it reg.

> +	int ret;
> +
> +	/* Enable SPI Registers Write */
> +	ret = max14001_write(st, MAX14001_REG_WEN, MAX14001_WRITE_WEN);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Reads all registers and writes the values back to their appropriate
> +	 * verification registers to clear the Memory Validation fault.
> +	 */
> +	for (reg_addr = MAX14001_REG_FLTEN; reg_addr <= MAX14001_REG_ENBL; reg_addr++) {
> +		ret = max14001_write_verification_reg(st, reg_addr);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Disable SPI Registers Write */
> +	return max14001_write(st, MAX14001_REG_WEN, 0);
> +}

...

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

Call it 'value' ('reg' part is redundant, and 'data' may be ambiguous in this function).

> +	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");
> +
> +	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;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +			"Failed to enable specified Vdd supply\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vddl");
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +			"Failed to enable specified Vddl supply\n");
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "vrefin");
> +	if (ret < 0) {
> +		st->vref_mv = 1250000 / 1000;

(MICRO / MILLI)

> +	} else {
> +		st->vref_mv = ret / 1000;

Ditto.

> +		ext_vrefin = 1;
> +	}

And with deduplication refactored code:

	ret = devm_regulator_get_enable_read_voltage(dev, "vrefin");
	if (ret < 0)
		ret = 1250000;
	else
		ext_vrefin = 1;
	st->vref_mv = ret / (MICRO / MILLI);

> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +			"Failed to init the mutex\n");

One line and do not interleave vref related pieces. I.o.w. move this block
upper before this line "ret = devm_regulator_get_enable(dev, "vdd");".

> +	if (ext_vrefin) {
> +		/*
> +		 * Configure the MAX14001/MAX14002 to use an external voltage
> +		 * reference source for the ADC.
> +		 */
> +		ret = max14001_read(st, MAX14001_REG_CFG, &reg_data);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +				"Failed to read Configuration Register\n");

Indentation issue. Note, for the lines ending with string literals there is no
line limit for a long time (10+ years)

> +		reg_data |= FIELD_PREP(MAX14001_REG_CFG_EXRF, 1);

FIELD_MODIFY() ?

> +		ret = max14001_write_single_reg(st, MAX14001_REG_CFG, reg_data);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +				"Failed to set Configuration Register\n");

As per above.

> +	}
> +
> +	ret = max14001_disable_mv_fault(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +			"Failed to disable MV Fault\n");

One line.

> +	return devm_iio_device_register(dev, indio_dev);
> +}

...

> +static const struct of_device_id max14001_of_match[] = {
> +	{ .compatible = "adi,max14001",
> +	  .data = &max14001_chip_info_tbl[max14001], },
> +	{ .compatible = "adi,max14002",
> +	  .data = &max14001_chip_info_tbl[max14002], },
> +	{ }

After splitting this hard-to-read style will be just as

	{ .compatible = "adi,max14001", .data = &max14001_chip_info },
	{ .compatible = "adi,max14002", .data = &max14002_chip_info },

> +};


-- 
With Best Regards,
Andy Shevchenko