Re: [PATCH 3/3] iio: adc: ad4130: add new supported parts

[Andy Shevchenko]

On Sat, Feb 28, 2026 at 09:39:04AM -0300, Jonathan Santos wrote:
> Add support for AD4129-4/8, AD4130-4, and AD4131-4/8 variants.
> 
> The AD4129 series supports the same FIFO interface as the AD4130 but with
> reduced resolution (16-bit). The AD4131 series lacks FIFO support, so
> triggered buffer functionality is introduced.
> 
> The 4-channel variants feature fewer analog inputs, GPIOs, and sparse pin
> mappings for VBIAS, analog inputs, and excitation currents. The driver now
> handles these differences with chip-specific configurations, including pin
> mappings and GPIO counts.

...

> +static const unsigned int ad4129_reg_size[] = {
> +	[AD4130_STATUS_REG] = 1,
> +	[AD4130_ADC_CONTROL_REG] = 2,
> +	[AD4130_DATA_REG] = 2,
> +	[AD4130_IO_CONTROL_REG] = 2,
> +	[AD4130_VBIAS_REG] = 2,
> +	[AD4130_ID_REG] = 1,
> +	[AD4130_ERROR_REG] = 2,
> +	[AD4130_ERROR_EN_REG] = 2,
> +	[AD4130_MCLK_COUNT_REG] = 1,
> +	[AD4130_CHANNEL_X_REG(0) ... AD4130_CHANNEL_X_REG(AD4130_MAX_CHANNELS - 1)] = 3,
> +	[AD4130_CONFIG_X_REG(0) ... AD4130_CONFIG_X_REG(AD4130_MAX_SETUPS - 1)] = 2,
> +	[AD4130_FILTER_X_REG(0) ... AD4130_FILTER_X_REG(AD4130_MAX_SETUPS - 1)] = 3,
> +	[AD4130_OFFSET_X_REG(0) ... AD4130_OFFSET_X_REG(AD4130_MAX_SETUPS - 1)] = 2,
> +	[AD4130_GAIN_X_REG(0) ... AD4130_GAIN_X_REG(AD4130_MAX_SETUPS - 1)] = 2,
> +	[AD4130_MISC_REG] = 2,
> +	[AD4130_FIFO_CONTROL_REG] = 3,
> +	[AD4130_FIFO_STATUS_REG] = 1,
> +	[AD4130_FIFO_THRESHOLD_REG] = 3,
> +	[AD4130_FIFO_DATA_REG] = 2,
> +};

I hope this passes `make W=1` builds for both clang and GCC.

...

> +static const u8 ad4130_8_pin_map[] = {
> +	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, /* AIN0/VBIAS_0-AIN7/VBIAS_7 */
> +	0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F, /* AIN8/VBIAS_8-AIN15/VBIAS_15 */

These comments are hard to follow. Much better is

> +};

/* Pin mapping for AIN0..AIN15, VBIAS_0..VBIAS_15 */
static const u8 ad4130_8_pin_map[] = {
	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,	/* 0 -  7 */
	0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F,	/* 8 - 15 */
};

...

>  struct ad4130_chip_info {
>  	const char *name;
>  	unsigned int max_analog_pins;
> +	unsigned int num_gpios;
>  	const struct iio_info *info;
>  	const unsigned int *reg_size;
>  	const unsigned int reg_size_length;
> +	const u8 *pin_map;
> +	bool has_fifo;

Ah, okay, so we fill the potential 4-byte gap from the previous patch.

>  };


...

> +	/* Triggered buffer data structure */
> +	struct {
> +		u32 channels[AD4130_MAX_CHANNELS];
> +		s64 timestamp;

Use aligned_s64 type...

> +	} scan __aligned(8);

...instead of this.

...

> +static irqreturn_t ad4130_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4130_state *st = iio_priv(indio_dev);
> +	unsigned int data_reg_size = ad4130_data_reg_size(st);

> +	unsigned int num_en_chn = bitmap_weight(indio_dev->active_scan_mask,
> +						iio_get_masklength(indio_dev));

Split this assignment. It's harder to read in the current form.

> +	struct spi_transfer xfer = {
> +		.rx_buf = st->scan.channels,
> +		.len = data_reg_size * num_en_chn,
> +	};
> +	int ret;
> +
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	if (ret < 0)
> +		goto err_unlock;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
> +					   iio_get_time_ns(indio_dev));
> +
> +err_unlock:
> +	iio_trigger_notify_done(indio_dev->trig);
>  
>  	return IRQ_HANDLED;
>  }

...

> +		/*
> +		 * In continuous read mode, when all samples are read, the data
> +		 * ready signal returns high until the next conversion result is
> +		 * ready. To exit this mode, the command must be sent when data
> +		 * ready is low. In order to ensure that condition, wait for the
> +		 * next interrupt (when the new conversion is finished), allowing
> +		 * data ready to return low before sending the exit command.
> +		 */
> +		st->buffer_wait_for_irq = true;
> +		ret = wait_for_completion_timeout(&st->completion,
> +						  msecs_to_jiffies(1000));
> +		st->buffer_wait_for_irq = false;
> +		if (!ret)
> +			dev_warn(&st->spi->dev, "Conversion timed out\n");

Usage of 'ret' for semantically different (and IIRC unsigned) case is
confusing, perhaps

		st->buffer_wait_for_irq = true;
		if (wait_for_completion_timeout(&st->completion, msecs_to_jiffies(1000))
			dev_warn(&st->spi->dev, "Conversion timed out\n");
		st->buffer_wait_for_irq = false;

-- 
With Best Regards,
Andy Shevchenko