[RFC,2/5] iio: adc: ad4030: add driver for ad4030-24

Commit Message

Esteban Blanc June 27, 2024, 11:59 a.m. UTC

This adds a new driver for the Analog Devices INC. AD4030-24 ADC.

The driver implements basic support for the AD4030-24 1 channel
differential ADC with hardware gain and offset control.

Signed-off-by: Esteban Blanc <[e-mail]>
---
 MAINTAINERS              |   1 +
 drivers/iio/adc/Kconfig  |  13 +
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ad4030.c | 822 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 837 insertions(+)

Comments

Nuno Sá June 28, 2024, 8:35 a.m. UTC | #1

Hi Esteban,

Main thing that I think we should clear and think about is how to expose the
common mode voltage. I kind if agree with it being a single channel but I still
have some concerns... See below.

On Thu, 2024-06-27 at 13:59 +0200, Esteban Blanc wrote:
> This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> 
> The driver implements basic support for the AD4030-24 1 channel
> differential ADC with hardware gain and offset control.
> 
> Signed-off-by: Esteban Blanc <[e-mail]>
> ---
>  MAINTAINERS              |   1 +
>  drivers/iio/adc/Kconfig  |  13 +
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/ad4030.c | 822
> +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 837 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8ca5b2e09b69..8df171c62d37 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -419,6 +419,7 @@ R:	Esteban Blanc <[e-mail]>
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/adc/adi,ad4630.yaml
> +F:	drivers/iio/adc/ad4030.c
>  
>  AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
>  M:	Mugilraj Dhavachelvan <[e-mail]>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 3d91015af6ea..e71ac1e49acb 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -21,6 +21,19 @@ config AD_SIGMA_DELTA
>  	select IIO_BUFFER
>  	select IIO_TRIGGERED_BUFFER
>  
> +config AD4030
> +	tristate "Analog Device AD4630 ADC Driver"
> +	depends on SPI
> +	depends on GPIOLIB
> +	select REGMAP_SPI
> +	select IIO_BUFFER
> +	help
> +	  Say yes here to build support for Analog Devices AD4030 and AD4630
> high speed
> +	  SPI analog to digital converters (ADC).
> +
> +	  To compile this driver as a module, choose M here: the module will
> be
> +	  called ad4030.
> +
>  config AD4130
>  	tristate "Analog Device AD4130 ADC Driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 37ac689a0209..7a8945559589 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -6,6 +6,7 @@
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
>  obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> +obj-$(CONFIG_AD4030) += ad4030.o
>  obj-$(CONFIG_AD4130) += ad4130.o
>  obj-$(CONFIG_AD7091R) += ad7091r-base.o
>  obj-$(CONFIG_AD7091R5) += ad7091r5.o
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> new file mode 100644
> index 000000000000..6d537e531d6f
> --- /dev/null
> +++ b/drivers/iio/adc/ad4030.c
> @@ -0,0 +1,822 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD4030 and AD4630 ADC family driver.
> + *
> + * Copyright 2024 Analog Devices, Inc.
> + * Copyright 2024 BayLibre, SAS
> + *
> + * based on code from:
> + *	Analog Devices, Inc.
> + *	  Sergiu Cuciurean <[e-mail]>
> + *	  Nuno Sa <[e-mail]>
> + *	  Marcelo Schmitt <[e-mail]>
> + *	  Liviu Adace <[e-mail]>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/units.h>
> +#include <linux/clk.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define AD4030_REG_INTERFACE_CONFIG_A				0x00
> +#define     AD4030_REG_INTERFACE_CONFIG_A_SW_RESET		(BIT(0) |
> BIT(7))
> +#define AD4030_REG_INTERFACE_CONFIG_B				0x01
> +#define AD4030_REG_DEVICE_CONFIG				0x02
> +#define AD4030_REG_CHIP_TYPE					0x03
> +#define AD4030_REG_PRODUCT_ID_L					0x04
> +#define AD4030_REG_PRODUCT_ID_H					0x05
> +#define AD4030_REG_CHIP_GRADE					0x06
> +#define     AD4030_REG_CHIP_GRADE_AD4030_24_GRADE		0x10
> +#define     AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE		GENMASK(7, 3)
> +#define AD4030_REG_SCRATCH_PAD					0x0A
> +#define AD4030_REG_SPI_REVISION					0x0B
> +#define AD4030_REG_VENDOR_L					0x0C
> +#define AD4030_REG_VENDOR_H					0x0D
> +#define AD4030_REG_STREAM_MODE					0x0E
> +#define AD4030_REG_INTERFACE_CONFIG_C				0x10
> +#define AD4030_REG_INTERFACE_STATUS_A				0x11
> +#define AD4030_REG_EXIT_CFG_MODE				0x14
> +#define     AD4030_REG_EXIT_CFG_MODE_MASK_EXIT_CONFIG_MD	BIT(0)
> +#define AD4030_REG_AVG						0x15
> +#define     AD4030_REG_AVG_MASK_AVG_SYNC			BIT(7)
> +#define    
> AD4030_REG_AVG_MASK_AVG_VAL				GENMASK(4, 0)
> +#define AD4030_REG_OFFSET_X0_0					0x16
> +#define AD4030_REG_OFFSET_X0_1					0x17
> +#define AD4030_REG_OFFSET_X0_2					0x18
> +#define AD4030_REG_OFFSET_X1_0					0x19
> +#define AD4030_REG_OFFSET_X1_1					0x1A
> +#define AD4030_REG_OFFSET_X1_2					0x1B
> +#define     AD4030_REG_OFFSET_BYTES_NB				3
> +#define    
> AD4030_REG_OFFSET_CHAN(ch)				(AD4030_REG_OFFSET_X0_2 +	\
> +								(AD4030_REG_O
> FFSET_BYTES_NB *	\
> +								(ch)))
> +#define AD4030_REG_GAIN_X0_LSB					0x1C
> +#define AD4030_REG_GAIN_X0_MSB					0x1D
> +#define AD4030_REG_GAIN_X1_LSB					0x1E
> +#define AD4030_REG_GAIN_X1_MSB					0x1F
> +#define     AD4030_REG_GAIN_MAX_GAIN				1999970
> +#define     AD4030_REG_GAIN_BYTES_NB				2
> +#define    
> AD4030_REG_GAIN_CHAN(ch)				(AD4030_REG_GAIN_X0_MSB +	\
> +								(AD4030_REG_G
> AIN_BYTES_NB *	\
> +								(ch)))
> +#define AD4030_REG_MODES					0x20
> +#define    
> AD4030_REG_MODES_MASK_OUT_DATA_MODE			GENMASK(2, 0)
> +#define     AD4030_REG_MODES_MASK_LANE_MODE			GENMASK(7, 6)
> +#define AD4030_REG_OSCILATOR					0x21
> +#define AD4030_REG_IO						0x22
> +#define     AD4030_REG_IO_MASK_IO2X				BIT(1)
> +#define AD4030_REG_PAT0						0x23
> +#define AD4030_REG_PAT1						0x24
> +#define AD4030_REG_PAT2						0x25
> +#define AD4030_REG_PAT3						0x26
> +#define AD4030_REG_DIG_DIAG					0x34
> +#define AD4030_REG_DIG_ERR					0x35
> +
> +/* Sequence starting with "1 0 1" to enable reg access */
> +#define AD4030_REG_ACCESS		0xa0
> +
> +#define AD4030_MAX_DIFF_CHANNEL_NB	2
> +#define AD4030_MAX_COMMON_CHANNEL_NB	AD4030_MAX_DIFF_CHANNEL_NB
> +#define AD4030_MAX_TIMESTAMP_CHANNEL_NB	1
> +#define AD4030_ALL_CHANNELS_NB		(AD4030_MAX_DIFF_CHANNEL_NB +	\
> +					AD4030_MAX_COMMON_CHANNEL_NB +	\
> +					AD4030_MAX_TIMESTAMP_CHANNEL_NB)
> +#define AD4030_VREF_MIN_UV		(4096 * MILLI)
> +#define AD4030_VREF_MAX_UV		(5000 * MILLI)
> +#define AD4030_VIO_THRESHOLD_UV		(1400 * MILLI)
> +
> +#define AD4030_SPI_MAX_XFER_LEN		8
> +#define AD4030_SPI_MAX_REG_XFER_SPEED	(80 * MEGA)
> +#define AD4030_TCNVH_NS			10
> +#define AD4030_TCNVL_NS			20
> +#define AD4030_TCONV_NS			(300 - (AD4030_TCNVH_NS
> +	\
> +					AD4030_TCNVL_NS))
> +#define AD4030_TRESET_PW_NS		50
> +#define AD4030_TRESET_COM_DELAY_MS	750
> +
> +enum ad4030_out_mode {
> +	AD4030_OUT_DATA_MD_16_DIFF = 0x00,
> +	AD4030_OUT_DATA_MD_24_DIFF = 0x00,
> +	AD4030_OUT_DATA_MD_16_DIFF_8_COM = 0x01,
> +	AD4030_OUT_DATA_MD_24_DIFF_8_COM = 0x02,
> +	AD4030_OUT_DATA_MD_30_AVERAGED_DIFF = 0x03,
> +	AD4030_OUT_DATA_MD_32_PATTERN = 0x04
> +};
> +
> +struct ad4030_chip_info {
> +	const char *name;
> +	const unsigned long *available_masks;
> +	unsigned int available_masks_len;
> +	const struct iio_chan_spec channels[AD4030_ALL_CHANNELS_NB];
> +	u8 grade;
> +	u8 precision_bits;
> +	int num_channels;
> +};
> +
> +struct ad4030_state {
> +	struct spi_device *spi;
> +	struct regmap *regmap;
> +	const struct ad4030_chip_info *chip;
> +	struct gpio_desc *cnv_gpio;
> +	int vref_uv;
> +	int vio_uv;
> +	int min_offset;
> +	int max_offset;
> +	int offset_avail[3];
> +	u32 conversion_speed_hz;
> +	enum ad4030_out_mode mode;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the transfer
> buffers
> +	 * to live in their own cache lines.
> +	 */
> +	u8 tx_data[AD4030_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> +	struct {
> +		union {
> +			/*
> +			 * Make the buffer large enough for MAX_NUM_CHANNELS
> 32-bit samples and
> +			 * one 64-bit aligned 64-bit timestamp.
> +			 */
> +			u8 raw[ALIGN(AD4030_MAX_DIFF_CHANNEL_NB * sizeof(u32)
> +
> +			       AD4030_MAX_COMMON_CHANNEL_NB * sizeof(u8),
> +			       sizeof(u64)) + sizeof(u64)];

This is not very readable. I would make this simpler.

> +			struct {
> +				s32 val[AD4030_MAX_DIFF_CHANNEL_NB];
> +				u8 common[AD4030_MAX_COMMON_CHANNEL_NB];

Not sure common makes sense as it comes aggregated with the sample. Maybe this
could as simple as:

struct {
	s32 val;
	u64 timestamp __aligned(8);
} rx_data ...

> +			};
> +		};
> +	} rx_data __aligned(IIO_DMA_MINALIGN);
> +};
> +
> +#define AD4030_CHAN_CMO(_idx)  {					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _idx,						\
> +	.scan_index = _idx,						\
> +	.scan_type = {							\
> +		.sign = 'u',						\
> +		.storagebits = 8,					\
> +		.realbits = 8,						\
> +		.endianness = IIO_BE,					\
> +	},								\
> +}
> +

So, from the datasheet, figure 39 we have something like a multiplexer where we
can have:

- averaged data;
- normal differential;
- test pattern (btw, useful to have it in debugfs - but can come later);
- 8 common mode bits;

While the average, normal and test pattern are really mutual exclusive, the
common mode voltage is different in the way that it's appended to differential
sample. Making it kind of an aggregated thingy. Thus I guess it can make sense
for us to see them as different channels from a SW perspective (even more since
gain and offset only apply to the differential data). But there are a couple of
things I don't like (have concerns):

* You're pushing the CM channels into the end. So when we a 2 channel device
we'll have:

 in_voltage0 - diff
 in_voltage1 - diff
 in_voltage2 - CM associated with chan0
 in_voltage0 - CM associated with chan1

I think we could make it so the CM channel comes right after the channel where
it's data belongs too. So for example, odd channels would be CM channels (and
labels could also make sense).

Other thing that came to mind is if we could somehow use differential = true
here. Having something like:

in_voltage1_in_voltage0_raw - diff data
...
And the only thing for CM would be:

in_voltage1_raw
in_voltage1_scale

(not sure if the above is doable with ext_info - maybe only with device_attrs)

The downside of the above is that we don't have a way to separate the scan
elements. Meaning that we don't have a way to specify the scan_type for both the
common mode and differential voltage. That said, I wonder if it is that useful
to buffer the common mode stuff? Alternatively, we could just have the scan_type
for the diff data and apps really wanting the CM voltage could still access the
raw data. Not pretty, I know...

However, even if we go with the two separate channels there's one thing that
concerns me. Right now we have diff data with 32 for storage bits and CM data
with 8 storage bits which means the sample will be 40 bits and in reality we
just have 32. Sure, now we have SW buffering so we can make it work but the
ultimate goal is to support HW buffering where we won't be able to touch the
sample and thus we can't lie about the sample size. Could you run any test with
this approach on a HW buffer setup? 

I did not gave too much thought on it but I'm not sure there's a sane way to
have multiple scan_types associated with the same channel. 

...

> +#define AD4030_CHAN_IN(_idx, _storage, _real, _shift)
> {			\
> 
> +
> +static int ad4030_get_chan_gain(struct iio_dev *indio_dev, int ch, int *val,
> +				int *val2)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	u16 gain;
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_GAIN_CHAN(ch), &gain,
> +			       AD4030_REG_GAIN_BYTES_NB);

Even though it's just 2 bytes, Jonathan typically has a strict policy regarding
this not being DMA safe.

> +	if (ret)
> +		return ret;
> +
> +	gain = be16_to_cpu(gain);
> +
> +	/* From datasheet: multiplied output = input × gain word/0x8000 */
> +	*val = gain / 0x8000;
> +	*val2 = mul_u64_u32_div(gain % 0x8000, MICRO, 0x8000);
> +
> +	return 0;
> +}
> +
> +/*
> + * @brief Returns the offset where 1 LSB = (VREF/2^precision_bits - 1)/gain
> + */
> +static int ad4030_get_chan_offset(struct iio_dev *indio_dev, int ch, int
> *val)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be32 offset;

ditto...

> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_OFFSET_CHAN(ch),
> &offset,
> +			       AD4030_REG_OFFSET_BYTES_NB);
> +	if (ret)
> +		return ret;
> +
> +	if (st->chip->precision_bits == 16)
> +		offset <<= 16;
> +	else
> +		offset <<= 8;
> +
> +	*val = be32_to_cpu(offset);
> +
> +	return 0;
> +}
> +
> +static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int
> gain_int,
> +				int gain_frac)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be16 val;
> +	u64 gain;
> +
> +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> +
> +	if (gain > AD4030_REG_GAIN_MAX_GAIN)
> +		return -EINVAL;
> +
> +	val = cpu_to_be16(DIV_ROUND_CLOSEST_ULL(gain * 0x8000, MICRO));
> +
> +	return regmap_bulk_write(st->regmap, AD4030_REG_GAIN_CHAN(ch), &val,
> +			  AD4030_REG_GAIN_BYTES_NB);

ditto

> +}
> +
> +static int ad4030_set_chan_offset(struct iio_dev *indio_dev, int ch, int
> offset)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be32 val;
> +
> +	if (offset < st->min_offset || offset > st->max_offset)
> +		return -EINVAL;
> +
> +	val = cpu_to_be32(offset);
> +	if (st->chip->precision_bits == 16)
> +		val >>= 16;
> +	else
> +		val >>= 8;
> +
> +	return regmap_bulk_write(st->regmap, AD4030_REG_OFFSET_CHAN(ch),
> &val, 3);

...

> +}
> +
> +static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
> +					unsigned int mask)
> +{
> +	return mask & BIT(st->chip->num_channels);
> +}
> +
> +static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	if (ad4030_is_common_byte_asked(st, mask))
> +		st->mode = st->chip->precision_bits == 24 ?
> +			AD4030_OUT_DATA_MD_24_DIFF_8_COM :
> +			AD4030_OUT_DATA_MD_16_DIFF_8_COM;

At this point you still don't really have AD4030_OUT_DATA_MD_16_DIFF_8_COM
support so I would keep it simple until you need to support it.

> +	else
> +		st->mode = AD4030_OUT_DATA_MD_24_DIFF;
> +
> +	return regmap_update_bits(st->regmap, AD4030_REG_MODES,
> +				AD4030_REG_MODES_MASK_OUT_DATA_MODE,
> +				st->mode);
> +}
> +
> +static int ad4030_conversion(struct ad4030_state *st, const struct
> iio_chan_spec *chan)
> +{
> +	unsigned int bytes_to_read = (BITS_TO_BYTES(chan->scan_type.realbits)
> +
> +			     ((st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM
> ||
> +			     st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ?
> 1 : 0)) *
> +			     st->chip->num_channels;
> +	struct spi_transfer xfer = {
> +		.rx_buf = st->rx_data.raw,
> +		.len = bytes_to_read,
> +	};
> +	unsigned char byte_index;
> +	unsigned int i;
> +	int ret;
> +
> +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> +	ndelay(AD4030_TCNVH_NS);
> +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +	ndelay(AD4030_TCNVL_NS + AD4030_TCONV_NS);
> +
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	if (ret || (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> +		    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM))

You should guarantee that st->mode is not invalid...

> +		return ret;
> +
> +	byte_index = st->chip->precision_bits == 16 ? 3 : 4;
> +	for (i = 0; i < st->chip->num_channels; i++)

So even for a single channel conversion we are going through all?

> +		st->rx_data.common[i] = ((u8 *)&st-
> >rx_data.val[i])[byte_index];
> +
> +	return 0;
> +}
> +
> +static int ad4030_single_conversion(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan, int
> *val)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4030_set_mode(indio_dev, BIT(chan->channel));
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_exit_config_mode(st);
> +	if (ret)
> +		goto out_error;
> +
> +	ret = ad4030_conversion(st, chan);
> +	if (ret)
> +		goto out_error;
> +
> +	if (chan->channel < st->chip->num_channels)
> +		*val = st->rx_data.val[chan->channel];
> +	else
> +		*val = st->rx_data.common[chan->channel - st->chip-
> >num_channels];
> +
> +out_error:
> +	ad4030_enter_config_mode(st);
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static irqreturn_t ad4030_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4030_conversion(st, indio_dev->channels);
> +	if (ret)
> +		goto out;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_data.raw,
> +					   pf->timestamp);
> +
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static const int *ad4030_get_offset_avail(struct ad4030_state *st)
> +{
> +	return st->offset_avail;
> +}
> +
> +static const int ad4030_gain_avail[3][2] = {
> +	{0, 0},
> +	{0, 30},
> +	{1, 999969},
> +};
> +
> +static int ad4030_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *channel,
> +			     const int **vals, int *type,
> +			     int *length, long mask)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		*vals = ad4030_get_offset_avail(st);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_RANGE;
> +
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		*vals = (void *)ad4030_gain_avail;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_RANGE;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad4030_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long info)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {

Oh this is not neat :(. I guess there's still no work to make conditional guards
to look more as the typical pattern...


> +		switch (info) {
> +		case IIO_CHAN_INFO_RAW:
> +			return ad4030_single_conversion(indio_dev, chan,
> val);
> +
> +		case IIO_CHAN_INFO_SCALE:
> +			*val = (st->vref_uv * 2) / MILLI;
> +			*val2 = st->chip->precision_bits;
> +			return IIO_VAL_FRACTIONAL_LOG2;

I don't think this applies to CM?

...

> 
> +static int ad4030_detect_chip_info(const struct ad4030_state *st)
> +{
> +	unsigned int grade;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
> +	if (ret)
> +		return ret;
> +
> +	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
> +	if (grade != st->chip->grade)
> +		return dev_err_probe(&st->spi->dev, -EINVAL,
> +				"Unknown grade(%u) for %s\n", grade,
> +				st->chip->name);

I think in here we still want to proceed and just print a warning...

- Nuno Sá

Jonathan Cameron June 29, 2024, 4:39 p.m. UTC | #2

On Thu, 27 Jun 2024 13:59:13 +0200
Esteban Blanc <[e-mail]> wrote:

> This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> 
> The driver implements basic support for the AD4030-24 1 channel
> differential ADC with hardware gain and offset control.
> 
> Signed-off-by: Esteban Blanc <[e-mail]>
A few comments inline.

Jonathan

> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> new file mode 100644
> index 000000000000..6d537e531d6f
> --- /dev/null
> +++ b/drivers/iio/adc/ad4030.c
> @@ -0,0 +1,822 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD4030 and AD4630 ADC family driver.
> + *
> + * Copyright 2024 Analog Devices, Inc.
> + * Copyright 2024 BayLibre, SAS
> + *
> + * based on code from:
> + *	Analog Devices, Inc.
> + *	  Sergiu Cuciurean <[e-mail]>
> + *	  Nuno Sa <[e-mail]>
> + *	  Marcelo Schmitt <[e-mail]>
> + *	  Liviu Adace <[e-mail]>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/units.h>
> +#include <linux/clk.h>
> +#include <linux/spi/spi.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define AD4030_REG_INTERFACE_CONFIG_A				0x00
> +#define     AD4030_REG_INTERFACE_CONFIG_A_SW_RESET		(BIT(0) | BIT(7))
> +#define AD4030_REG_INTERFACE_CONFIG_B				0x01
> +#define AD4030_REG_DEVICE_CONFIG				0x02
> +#define AD4030_REG_CHIP_TYPE					0x03
> +#define AD4030_REG_PRODUCT_ID_L					0x04
> +#define AD4030_REG_PRODUCT_ID_H					0x05
> +#define AD4030_REG_CHIP_GRADE					0x06
> +#define     AD4030_REG_CHIP_GRADE_AD4030_24_GRADE		0x10
> +#define     AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE		GENMASK(7, 3)
> +#define AD4030_REG_SCRATCH_PAD					0x0A
> +#define AD4030_REG_SPI_REVISION					0x0B
> +#define AD4030_REG_VENDOR_L					0x0C
> +#define AD4030_REG_VENDOR_H					0x0D
> +#define AD4030_REG_STREAM_MODE					0x0E
> +#define AD4030_REG_INTERFACE_CONFIG_C				0x10
> +#define AD4030_REG_INTERFACE_STATUS_A				0x11
> +#define AD4030_REG_EXIT_CFG_MODE				0x14
> +#define     AD4030_REG_EXIT_CFG_MODE_MASK_EXIT_CONFIG_MD	BIT(0)

Maybe can shorten to AD4030_REG_EXIT_CFG_MODE_EXIT_MSK
or something like that?

> +#define AD4030_REG_AVG						0x15
> +#define     AD4030_REG_AVG_MASK_AVG_SYNC			BIT(7)
> +#define     AD4030_REG_AVG_MASK_AVG_VAL				GENMASK(4, 0)
> +#define AD4030_REG_OFFSET_X0_0					0x16
> +#define AD4030_REG_OFFSET_X0_1					0x17
> +#define AD4030_REG_OFFSET_X0_2					0x18
> +#define AD4030_REG_OFFSET_X1_0					0x19
> +#define AD4030_REG_OFFSET_X1_1					0x1A
> +#define AD4030_REG_OFFSET_X1_2					0x1B
> +#define     AD4030_REG_OFFSET_BYTES_NB				3
> +#define     AD4030_REG_OFFSET_CHAN(ch)				(AD4030_REG_OFFSET_X0_2 +	\
> +								(AD4030_REG_OFFSET_BYTES_NB *	\
> +								(ch)))
Feel free to not have everything perfectly aligned.  So I'd reduce alignment of
the values and just have one two where it gets pushed further right by
a long name.

> +#define AD4030_REG_GAIN_X0_LSB					0x1C
> +#define AD4030_REG_GAIN_X0_MSB					0x1D
> +#define AD4030_REG_GAIN_X1_LSB					0x1E
> +#define AD4030_REG_GAIN_X1_MSB					0x1F
> +#define     AD4030_REG_GAIN_MAX_GAIN				1999970
> +#define     AD4030_REG_GAIN_BYTES_NB				2
> +#define     AD4030_REG_GAIN_CHAN(ch)				(AD4030_REG_GAIN_X0_MSB +	\
> +								(AD4030_REG_GAIN_BYTES_NB *	\
> +								(ch)))
> +#define AD4030_REG_MODES					0x20
> +#define     AD4030_REG_MODES_MASK_OUT_DATA_MODE			GENMASK(2, 0)
> +#define     AD4030_REG_MODES_MASK_LANE_MODE			GENMASK(7, 6)
> +#define AD4030_REG_OSCILATOR					0x21
> +#define AD4030_REG_IO						0x22
> +#define     AD4030_REG_IO_MASK_IO2X				BIT(1)
> +#define AD4030_REG_PAT0						0x23
> +#define AD4030_REG_PAT1						0x24
> +#define AD4030_REG_PAT2						0x25
> +#define AD4030_REG_PAT3						0x26
> +#define AD4030_REG_DIG_DIAG					0x34
> +#define AD4030_REG_DIG_ERR					0x35
> +
> +/* Sequence starting with "1 0 1" to enable reg access */
> +#define AD4030_REG_ACCESS		0xa0
> +
> +#define AD4030_MAX_DIFF_CHANNEL_NB	2
> +#define AD4030_MAX_COMMON_CHANNEL_NB	AD4030_MAX_DIFF_CHANNEL_NB
> +#define AD4030_MAX_TIMESTAMP_CHANNEL_NB	1
> +#define AD4030_ALL_CHANNELS_NB		(AD4030_MAX_DIFF_CHANNEL_NB +	\
> +					AD4030_MAX_COMMON_CHANNEL_NB +	\
> +					AD4030_MAX_TIMESTAMP_CHANNEL_NB)
> +#define AD4030_VREF_MIN_UV		(4096 * MILLI)
> +#define AD4030_VREF_MAX_UV		(5000 * MILLI)
> +#define AD4030_VIO_THRESHOLD_UV		(1400 * MILLI)
> +
> +#define AD4030_SPI_MAX_XFER_LEN		8
> +#define AD4030_SPI_MAX_REG_XFER_SPEED	(80 * MEGA)
> +#define AD4030_TCNVH_NS			10
> +#define AD4030_TCNVL_NS			20
> +#define AD4030_TCONV_NS			(300 - (AD4030_TCNVH_NS +	\
> +					AD4030_TCNVL_NS))
> +#define AD4030_TRESET_PW_NS		50
> +#define AD4030_TRESET_COM_DELAY_MS	750
> +
> +enum ad4030_out_mode {
> +	AD4030_OUT_DATA_MD_16_DIFF = 0x00,
> +	AD4030_OUT_DATA_MD_24_DIFF = 0x00,
> +	AD4030_OUT_DATA_MD_16_DIFF_8_COM = 0x01,
> +	AD4030_OUT_DATA_MD_24_DIFF_8_COM = 0x02,
> +	AD4030_OUT_DATA_MD_30_AVERAGED_DIFF = 0x03,
> +	AD4030_OUT_DATA_MD_32_PATTERN = 0x04
> +};
> +
> +struct ad4030_chip_info {
> +	const char *name;
> +	const unsigned long *available_masks;
> +	unsigned int available_masks_len;
> +	const struct iio_chan_spec channels[AD4030_ALL_CHANNELS_NB];
> +	u8 grade;
> +	u8 precision_bits;
> +	int num_channels;
> +};
> +
> +struct ad4030_state {
> +	struct spi_device *spi;
> +	struct regmap *regmap;
> +	const struct ad4030_chip_info *chip;
> +	struct gpio_desc *cnv_gpio;
> +	int vref_uv;
> +	int vio_uv;
> +	int min_offset;
> +	int max_offset;
> +	int offset_avail[3];
> +	u32 conversion_speed_hz;
> +	enum ad4030_out_mode mode;
> +
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the transfer buffers
> +	 * to live in their own cache lines.
> +	 */
> +	u8 tx_data[AD4030_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
> +	struct {
> +		union {
> +			/*
> +			 * Make the buffer large enough for MAX_NUM_CHANNELS 32-bit samples and
> +			 * one 64-bit aligned 64-bit timestamp.
> +			 */
> +			u8 raw[ALIGN(AD4030_MAX_DIFF_CHANNEL_NB * sizeof(u32) +
> +			       AD4030_MAX_COMMON_CHANNEL_NB * sizeof(u8),
> +			       sizeof(u64)) + sizeof(u64)];
> +			struct {
> +				s32 val[AD4030_MAX_DIFF_CHANNEL_NB];
> +				u8 common[AD4030_MAX_COMMON_CHANNEL_NB];
> +			};
> +		};
> +	} rx_data __aligned(IIO_DMA_MINALIGN);
> +};

> +
> +static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
> +			   void *val, size_t val_size)
> +{
> +	struct ad4030_state *st = context;
> +
> +	struct spi_transfer xfer = {
> +		.tx_buf = st->tx_data,
> +		.rx_buf = st->rx_data.raw,
> +		.len = reg_size + val_size,
> +	};
> +	int ret;
> +
> +	memcpy(st->tx_data, reg, reg_size);
> +
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	memcpy(val, &st->rx_data.raw[reg_size], val_size);

Can you just use spi_write_then_read() here?

> +
> +	return ret;
> +}


> +/*
> + * @brief Returns the offset where 1 LSB = (VREF/2^precision_bits - 1)/gain
> + */
> +static int ad4030_get_chan_offset(struct iio_dev *indio_dev, int ch, int *val)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be32 offset;
> +	int ret;
> +
> +	ret = regmap_bulk_read(st->regmap, AD4030_REG_OFFSET_CHAN(ch), &offset,
> +			       AD4030_REG_OFFSET_BYTES_NB);

As Nuno observed, DMA safe buffers needed.  I don't think there has been any
change since I last checked that we should not assume regmap doesn't need them
(even though it probably doesn't) when run over transports that do.

> +	if (ret)
> +		return ret;
> +
> +	if (st->chip->precision_bits == 16)
> +		offset <<= 16;
> +	else
> +		offset <<= 8;

As below. This is hard tor read. Just use appropriate unaligned gets for the
two cases to extract the write bytes directly.

> +	*val = be32_to_cpu(offset);
> +
> +	return 0;
> +}
> +
> +static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int gain_int,
> +				int gain_frac)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be16 val;
> +	u64 gain;
> +
> +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> +
> +	if (gain > AD4030_REG_GAIN_MAX_GAIN)
> +		return -EINVAL;
> +
> +	val = cpu_to_be16(DIV_ROUND_CLOSEST_ULL(gain * 0x8000, MICRO));
> +
> +	return regmap_bulk_write(st->regmap, AD4030_REG_GAIN_CHAN(ch), &val,
> +			  AD4030_REG_GAIN_BYTES_NB);
> +}
> +
> +static int ad4030_set_chan_offset(struct iio_dev *indio_dev, int ch, int offset)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	__be32 val;
> +
> +	if (offset < st->min_offset || offset > st->max_offset)
> +		return -EINVAL;
> +
> +	val = cpu_to_be32(offset);
> +	if (st->chip->precision_bits == 16)
> +		val >>= 16;
> +	else
> +		val >>= 8;

I 'think' I get what this is doing but not 100% sure as it's a bit too unusual
and I'm not even sure what happens if we shift a __be32 value on a little endian
system. I would instead split this into appropriate cpu_to_be24() and cpu_to_be16()
to put the value directly in the right place rather than shifting in place.

> +
> +	return regmap_bulk_write(st->regmap, AD4030_REG_OFFSET_CHAN(ch), &val, 3);
> +}
> +
> +static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
> +					unsigned int mask)
> +{
> +	return mask & BIT(st->chip->num_channels);

That's not obvious.  So at very least add a comment on why that's the bit to check.

> +}
> +
> +static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	if (ad4030_is_common_byte_asked(st, mask))
> +		st->mode = st->chip->precision_bits == 24 ?
> +			AD4030_OUT_DATA_MD_24_DIFF_8_COM :
> +			AD4030_OUT_DATA_MD_16_DIFF_8_COM;

I'd match on each precision_bits value so it's clear that there is
only 24 and 16.  Probably a small switch statement.
Will be a little more code, but easier to read.


> +	else
> +		st->mode = AD4030_OUT_DATA_MD_24_DIFF;
> +
> +	return regmap_update_bits(st->regmap, AD4030_REG_MODES,
> +				AD4030_REG_MODES_MASK_OUT_DATA_MODE,
> +				st->mode);
> +}
> +
> +static int ad4030_conversion(struct ad4030_state *st, const struct iio_chan_spec *chan)
> +{
> +	unsigned int bytes_to_read = (BITS_TO_BYTES(chan->scan_type.realbits) +
> +			     ((st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM ||
> +			     st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ? 1 : 0)) *
> +			     st->chip->num_channels;

Too fiddly. I'd express this as a series if statements so it is easier to grasp
what each stage is doing.

> +	struct spi_transfer xfer = {
> +		.rx_buf = st->rx_data.raw,
> +		.len = bytes_to_read,
> +	};
> +	unsigned char byte_index;
> +	unsigned int i;
> +	int ret;
> +
> +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> +	ndelay(AD4030_TCNVH_NS);
> +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +	ndelay(AD4030_TCNVL_NS + AD4030_TCONV_NS);
> +
> +	ret = spi_sync_transfer(st->spi, &xfer, 1);

spi_read()?

> +	if (ret || (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> +		    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM))
> +		return ret;
> +

Split this so error is handled then the 'we are done here' condition
is handled.

> +	byte_index = st->chip->precision_bits == 16 ? 3 : 4;

Can we compute this rather than assume knowledge that precision_bits only takes
a few values?

> +	for (i = 0; i < st->chip->num_channels; i++)
> +		st->rx_data.common[i] = ((u8 *)&st->rx_data.val[i])[byte_index];
> +
> +	return 0;
> +}
> +
> +static int ad4030_single_conversion(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan, int *val)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = ad4030_set_mode(indio_dev, BIT(chan->channel));
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_exit_config_mode(st);

Failed register accesses are always a pain, but normal assumption if they fail
is that nothing got written. So should still be in config mode in the
error path and don't need to reset it.  If there is a reason to do otherwise
here add a cmment.

> +	if (ret)
> +		goto out_error;
> +
> +	ret = ad4030_conversion(st, chan);
> +	if (ret)
> +		goto out_error;
> +
> +	if (chan->channel < st->chip->num_channels)
> +		*val = st->rx_data.val[chan->channel];
> +	else
> +		*val = st->rx_data.common[chan->channel - st->chip->num_channels];
> +
> +out_error:
> +	ad4030_enter_config_mode(st);
> +
> +	return IIO_VAL_INT;
> +}

> +
> +static const int ad4030_gain_avail[3][2] = {
> +	{0, 0},
> +	{0, 30},
> +	{1, 999969},
Spaces after { and before } preferred.

> +};
> +

> +
> +static int ad4030_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long info)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> +		switch (info) {
> +		case IIO_CHAN_INFO_RAW:
> +			return ad4030_single_conversion(indio_dev, chan, val);
> +
> +		case IIO_CHAN_INFO_SCALE:
> +			*val = (st->vref_uv * 2) / MILLI;
> +			*val2 = st->chip->precision_bits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +
> +		case IIO_CHAN_INFO_CALIBSCALE:

As per cover letter these seem somewhat pointless, but if I'm wrong
and they are useeful, roll this into scale (maths will get harder
but that's the side effect of keeping an interface simple) and offset.

> +			ret = ad4030_get_chan_gain(indio_dev, chan->channel,
> +						   val, val2);
> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +
> +		case IIO_CHAN_INFO_CALIBBIAS:
> +			ret = ad4030_get_chan_offset(indio_dev, chan->channel,
> +						     val);
> +			if (ret)
> +				return ret;
> +			return IIO_VAL_INT;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	unreachable();
> +}

> +
> +static int ad4030_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	/* Make sure the SPI clock is within range to read register */
> +	if (st->spi->max_speed_hz > AD4030_SPI_MAX_REG_XFER_SPEED)
> +		st->spi->max_speed_hz = AD4030_SPI_MAX_REG_XFER_SPEED;

min

> +
> +	return ad4030_enter_config_mode(st);
> +}
> +
> +static const struct iio_buffer_setup_ops ad4030_buffer_setup_ops = {
> +	.preenable = ad4030_buffer_preenable,
> +	.postdisable = ad4030_buffer_postdisable,
> +};
> +
> +static int ad4030_regulators_get(struct ad4030_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	static const char * const ids[] = {"vdd-5v", "vdd-1v8"};

Trivial, but I prefer spaces always after { and before } for arrays.

> +	int ret;
> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ids), ids);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> +	st->vio_uv = devm_regulator_get_enable_read_voltage(dev, "vio");
> +	if (st->vio_uv < 0)
> +		return dev_err_probe(dev, st->vio_uv,
> +				     "Failed to enable and read vio voltage");
> +
> +	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (st->vref_uv < 0) {
> +		if (st->vref_uv != -ENODEV)
> +			return dev_err_probe(dev, st->vref_uv,
> +					     "Failed to read vref voltage");
> +
> +		/* if not using optional REF, the internal REFIN must be used */
> +		st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "refin");
> +		if (st->vref_uv < 0)
> +			return dev_err_probe(dev, st->vref_uv,
> +					     "Failed to read vrefin voltage");
> +	}
> +
> +	if (st->vref_uv < AD4030_VREF_MIN_UV || st->vref_uv > AD4030_VREF_MAX_UV)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "vref(%d) must be in the range [%lu %lu]\n",
> +				     st->vref_uv, AD4030_VREF_MIN_UV,
> +				     AD4030_VREF_MAX_UV);
> +
> +	return 0;
> +}
> +
> +static int ad4030_reset(struct ad4030_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct gpio_desc *reset;
> +	int ret;
> +
> +	/* Use GPIO if available ... */
> +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		return dev_err_probe(dev, PTR_ERR(reset),
> +				"Failed to get reset GPIO\n");
> +
> +	if (reset) {
> +		ndelay(50);
> +		gpiod_set_value_cansleep(reset, 0);
> +		goto reset_end;

Rather than a goto, I'd use else { here.
I think that will end up as more readable code flow.
I was briefly wondering why this was an error path having misread it.


> +	}
> +
> +	/* ... falback to software reset otherwise */
> +	ret = ad4030_enter_config_mode(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD4030_REG_INTERFACE_CONFIG_A,
> +			   AD4030_REG_INTERFACE_CONFIG_A_SW_RESET);
> +	if (ret)
> +		return ret;
> +
> +reset_end:
> +	/* Wait for reset to complete before communicating to it */
> +	fsleep(AD4030_TRESET_COM_DELAY_MS);
> +
> +	/* After reset, conversion mode is enabled. Switch to reg access */
> +	return ad4030_enter_config_mode(st);
> +}
> +
> +static int ad4030_detect_chip_info(const struct ad4030_state *st)
> +{
> +	unsigned int grade;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
> +	if (ret)
> +		return ret;
> +
> +	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
> +	if (grade != st->chip->grade)
> +		return dev_err_probe(&st->spi->dev, -EINVAL,
> +				"Unknown grade(%u) for %s\n", grade,

Indent to align after ( or if it's a too long a line as a result use
one tab only after the line above.  That tends to end up more
consistent than seen here.

> +				st->chip->name);
> +
> +	return 0;
> +}
> +
> +static int ad4030_config(struct ad4030_state *st)
> +{
> +	st->min_offset = (int)BIT(st->chip->precision_bits) * -1;
> +	st->max_offset = BIT(st->chip->precision_bits) - 1;
> +	st->offset_avail[0] = st->min_offset;
> +	st->offset_avail[1] = 1;
> +	st->offset_avail[2] = st->max_offset;
Perhaps void storing the same set of values twice in st.

> +
> +	if (st->vio_uv < AD4030_VIO_THRESHOLD_UV)
> +		return regmap_write(st->regmap, AD4030_REG_IO,
> +				AD4030_REG_IO_MASK_IO2X);
> +
> +	return 0;
> +}
> +
> +static int ad4030_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad4030_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	/* Make sure the SPI clock is within range to read register */
> +	st->conversion_speed_hz = spi->max_speed_hz;
> +	if (spi->max_speed_hz > AD4030_SPI_MAX_REG_XFER_SPEED)
> +		spi->max_speed_hz = AD4030_SPI_MAX_REG_XFER_SPEED;

	st->conversion_speed_hz = min(spi->max_speed_hz,
				      AD4030_SPI_MAX_REG_XFER_SPEED);

> +
> +	st->regmap = devm_regmap_init(&spi->dev, &ad4030_regmap_bus, st,
> +				      &ad4030_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> +			      "Failed to initialize regmap\n");
> +
> +	st->chip = spi_get_device_match_data(spi);
> +	if (!st->chip)
> +		return -EINVAL;
> +
> +	ret = ad4030_regulators_get(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_reset(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_detect_chip_info(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_config(st);
> +	if (ret)
> +		return ret;
> +
> +	st->cnv_gpio = devm_gpiod_get(dev, "cnv", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->cnv_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
> +				     "Failed to get cnv gpio");
> +
> +	indio_dev->name = st->chip->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ad4030_iio_info;
> +	indio_dev->channels = st->chip->channels;
> +	indio_dev->num_channels =  2 * st->chip->num_channels + 1;

hmm. This is non obviously.  Add a comment to the structure definition
and maybe one here as well.



> +	indio_dev->available_scan_masks = st->chip->available_masks;
> +	indio_dev->masklength = st->chip->available_masks_len;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      ad4030_trigger_handler,
> +					      &ad4030_buffer_setup_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

Jonathan Cameron June 29, 2024, 4:40 p.m. UTC | #3

> > +#define AD4030_CHAN_CMO(_idx)  {					\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = _idx,						\
> > +	.scan_index = _idx,						\
> > +	.scan_type = {							\
> > +		.sign = 'u',						\
> > +		.storagebits = 8,					\
> > +		.realbits = 8,						\
> > +		.endianness = IIO_BE,					\
> > +	},								\
> > +}
> > +  
> 
> So, from the datasheet, figure 39 we have something like a multiplexer where we
> can have:
> 
> - averaged data;
> - normal differential;
> - test pattern (btw, useful to have it in debugfs - but can come later);

Can consider doing test pattern as simply a different channel, data is afterall
coming from a very different place.


> - 8 common mode bits;
> 
> While the average, normal and test pattern are really mutual exclusive, the
> common mode voltage is different in the way that it's appended to differential
> sample. Making it kind of an aggregated thingy. Thus I guess it can make sense
> for us to see them as different channels from a SW perspective (even more since
> gain and offset only apply to the differential data). But there are a couple of
> things I don't like (have concerns):
> 
> * You're pushing the CM channels into the end. So when we a 2 channel device
> we'll have:
> 
>  in_voltage0 - diff
>  in_voltage1 - diff
>  in_voltage2 - CM associated with chan0
>  in_voltage0 - CM associated with chan1
> 
> I think we could make it so the CM channel comes right after the channel where
> it's data belongs too. So for example, odd channels would be CM channels (and
> labels could also make sense).
> 
> Other thing that came to mind is if we could somehow use differential = true
> here. Having something like:
> 
> in_voltage1_in_voltage0_raw - diff data
in_voltage1-voltage0_raw  so normal differential channel.

> ...
> And the only thing for CM would be:
> 
> in_voltage1_raw
> in_voltage1_scale

you can do that with normal channels.

> 
> (not sure if the above is doable with ext_info - maybe only with device_attrs)
We 'could' do the somewhat nasty

in_voltage1-voltage0_raw - so normal differential channel.

in_voltage1+voltage0_raw via a custom attribute.  Don't try to hammer that in
as channel related though.


> 
> The downside of the above is that we don't have a way to separate the scan
> elements. Meaning that we don't have a way to specify the scan_type for both the
> common mode and differential voltage. That said, I wonder if it is that useful
> to buffer the common mode stuff? Alternatively, we could just have the scan_type
> for the diff data and apps really wanting the CM voltage could still access the
> raw data. Not pretty, I know...

Hmm. That is indeed a nasty corner if we don't use a real channel.
This may be a case for just using labels for the relationship.

> 
> However, even if we go with the two separate channels there's one thing that
> concerns me. Right now we have diff data with 32 for storage bits and CM data
> with 8 storage bits which means the sample will be 40 bits and in reality we
> just have 32. Sure, now we have SW buffering so we can make it work but the
> ultimate goal is to support HW buffering where we won't be able to touch the
> sample and thus we can't lie about the sample size. Could you run any test with
> this approach on a HW buffer setup? 
> 
> I did not gave too much thought on it but I'm not sure there's a sane way to
> have multiple scan_types associated with the same channel. 

Not at the same time.

Jonathan

Esteban Blanc July 26, 2024, 1:38 p.m. UTC | #4

Hi Nuno,

> > +			struct {
> > +				s32 val[AD4030_MAX_DIFF_CHANNEL_NB];
> > +				u8 common[AD4030_MAX_COMMON_CHANNEL_NB];
>
> Not sure common makes sense as it comes aggregated with the sample. Maybe this
> could as simple as:
>
> struct {
> 	s32 val;
> 	u64 timestamp __aligned(8);
> } rx_data ...

See below my answer on channels order and storagebits.

> So, from the datasheet, figure 39 we have something like a multiplexer where we
> can have:
>
> - averaged data;
> - normal differential;
> - test pattern (btw, useful to have it in debugfs - but can come later);
> - 8 common mode bits;
>
> While the average, normal and test pattern are really mutual exclusive, the
> common mode voltage is different in the way that it's appended to differential
> sample. Making it kind of an aggregated thingy. Thus I guess it can make sense
> for us to see them as different channels from a SW perspective (even more since
> gain and offset only apply to the differential data). But there are a couple of
> things I don't like (have concerns):
>
> * You're pushing the CM channels into the end. So when we a 2 channel device
> we'll have:
>
>  in_voltage0 - diff
>  in_voltage1 - diff
>  in_voltage2 - CM associated with chan0
>  in_voltage0 - CM associated with chan1
>
> I think we could make it so the CM channel comes right after the channel where
> it's data belongs too. So for example, odd channels would be CM channels (and
> labels could also make sense).

I must agree with you it would make more sense.

> Other thing that came to mind is if we could somehow use differential = true
> here. Having something like:
>
> in_voltage1_in_voltage0_raw - diff data
> ...
> And the only thing for CM would be:
>
> in_voltage1_raw
> in_voltage1_scale
>
> (not sure if the above is doable with ext_info - maybe only with device_attrs)
>
> The downside of the above is that we don't have a way to separate the scan
> elements. Meaning that we don't have a way to specify the scan_type for both the
> common mode and differential voltage. That said, I wonder if it is that useful
> to buffer the common mode stuff? Alternatively, we could just have the scan_type
> for the diff data and apps really wanting the CM voltage could still access the
> raw data. Not pretty, I know...

At the moment the way I "separate" them is by looking at the
`active_scan_mask`. If the user asked for differential channel only, I put the
chip in differential only mode. If all the channels are asked, I put
the chip in differential + common mode. This way there is no need to
separate anything in differential mode. See below for an example where
this started.

> However, even if we go with the two separate channels there's one thing that
> concerns me. Right now we have diff data with 32 for storage bits and CM data
> with 8 storage bits which means the sample will be 40 bits and in reality we
> just have 32. Sure, now we have SW buffering so we can make it work but the
> ultimate goal is to support HW buffering where we won't be able to touch the
> sample and thus we can't lie about the sample size. Could you run any test with
> this approach on a HW buffer setup? 

Let's take AD4630-24 in diff+cm mode as an example. We would have 4 channels:
- Ch0 diff with 24 bits of realbits and 24 bits of storagebits
- Ch0 cm with 8 bits of realbits and 8 bits of storagebits
- Ch1 diff with 24 bits of realbits and 24 bits of storagebits
- Ch1 cm with 8 bits of realbits and 8 bits of storagebits
ChX diff realbits + ChX cm realbits = 32 bits which is one sample as sent
by the chip.

The problem I faced when trying to do this in this series is that IIO doesn't
seem to like 24 storagebits and the data would get garbled. In diff only
mode with the same channel setup (selecting only Ch0 diff and Ch1 diff)
the data is also garbled. I used iio-oscilloscope software to test this setup.
Here is the output with iio_readdev:
```
# iio_readdev -s 1 ad4630-24 voltage0
WARNING: High-speed mode not enabled
Unable to refill buffer: Invalid argument (22)
```

I think this is happening when computing the padding to align ch1 diff.
In `iio_compute_scan_bytes` this line `bytes = ALIGN(bytes, length);`
will be invoked with bytes = 3 and length = 3 when selecting ch0 diff
and ch1 diff (AD4630-24 in differential mode). The output is 5. As
specified in linux/align.h:
> @a is a power of 2
In our case `a` is `length`, and 3 is not a power of 2.

It works fine with Ch0/1 diff with 24 realbits and 32 storagebits with 8
bits shift.

Intrestingly, a similar setup works great on AD4630-16:
- Ch0 diff with 16 bits of realbits and 16 bits of storagebits
- Ch0 cm with 8 bits of realbits and 8 bits of storagebits
- Ch1 diff with 16 bits of realbits and 16 bits of storagebits
- Ch1 cm with 8 bits of realbits and 8 bits of storagebits

In `iio_compute_scan_bytes` we will have ALIGN(3, 2) which will output
4, everything is fine. The output of iio-oscilloscope is as expected,
a clean sinewave and iio_readdev does not throw an error.

All this to say that at the moment, I'm not sure how I will be able to
put the CM byte in a separate channel for AD4630-24 without buffering it.
It would be useful to return a "packed" buffer.

> > +static int ad4030_conversion(struct ad4030_state *st, const struct
> > iio_chan_spec *chan)
> > +{
> > +	unsigned int bytes_to_read = (BITS_TO_BYTES(chan->scan_type.realbits)
> > +
> > +			     ((st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM
> > ||
> > +			     st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ?
> > 1 : 0)) *
> > +			     st->chip->num_channels;
> > +	struct spi_transfer xfer = {
> > +		.rx_buf = st->rx_data.raw,
> > +		.len = bytes_to_read,
> > +	};
> > +	unsigned char byte_index;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 1);
> > +	ndelay(AD4030_TCNVH_NS);
> > +	gpiod_set_value_cansleep(st->cnv_gpio, 0);
> > +	ndelay(AD4030_TCNVL_NS + AD4030_TCONV_NS);
> > +
> > +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> > +	if (ret || (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> > +		    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM))
>
> You should guarantee that st->mode is not invalid...

I don't see a code path where this could be the case. It's initialized
at 0 in the probe (a valid value) and always set before `ad4030_conversion`
in `ad4030_set_mode` called by `ad4030_buffer_preenable`. If the value
is invalid, this is a driver error, not an invalid input from the user.
Should I put an assert then?

> > +		return ret;
> > +
> > +	byte_index = st->chip->precision_bits == 16 ? 3 : 4;
> > +	for (i = 0; i < st->chip->num_channels; i++)
>
> So even for a single channel conversion we are going through all?

Yes. For ad4030-24 (this patch), that ok since num_channels = 1.
For AD4630, we are getting the 2 channels samples anyway. Current
allowed `scan_mask` are ch0 and ch1 (diff mode) or ch0 to ch3 (diff +
common mode). The only case I could optimized currently is if I allowed
scan_mask such as: ch0 & ch2, ch0 & ch1 & ch2, ch0 & ch1 & ch3. I'm not
sure it makes sense to allow such `scan_mask` since the HW does not
support it.
What do you think?

> > +static int ad4030_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan, int *val,
> > +			   int *val2, long info)
> > +{
> > +	struct ad4030_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
>
> Oh this is not neat :(. I guess there's still no work to make conditional guards
> to look more as the typical pattern...

Yeah... At first I put checks inside each function. Then I put guards
inside each function. But at the end, almost every arm of the switch
case needs to be in direct mode so... I can put it back in the function,
as you wish.

> > +		switch (info) {
> > +		case IIO_CHAN_INFO_RAW:
> > +			return ad4030_single_conversion(indio_dev, chan,
> > val);
> > +
> > +		case IIO_CHAN_INFO_SCALE:
> > +			*val = (st->vref_uv * 2) / MILLI;
> > +			*val2 = st->chip->precision_bits;
> > +			return IIO_VAL_FRACTIONAL_LOG2;
>
> I don't think this applies to CM?

Indded. Scale is not available for the CM channels.

> > +static int ad4030_detect_chip_info(const struct ad4030_state *st)
> > +{
> > +	unsigned int grade;
> > +	int ret;
> > +
> > +	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
> > +	if (ret)
> > +		return ret;
> > +
> > +	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
> > +	if (grade != st->chip->grade)
> > +		return dev_err_probe(&st->spi->dev, -EINVAL,
> > +				"Unknown grade(%u) for %s\n", grade,
> > +				st->chip->name);
>
> I think in here we still want to proceed and just print a warning...

I've put this in because it's a quick and easy way to check:
- If the SPI communication is working,
- If the correct chip is installed. If it's not the correct one, we can
  be sure that the parameters used to get any data out the chip will be
  wrong and the data completely useless.

If you feel that there is a use case I missed I will put a warning
instead, no problem.


All the comments I skipped and not replied to will be fixed in a V2.

Best regards,

Jonathan Cameron July 28, 2024, 3:04 p.m. UTC | #5

One quick comment form me inline.

The short version is non power of 2 storage is not an option because
it is a major ABI change and we aren't paying the cost of complexity
that brings to userspace for a very small number of drivers where
there is any real advantage to packing them tighter.

> 
> > So, from the datasheet, figure 39 we have something like a multiplexer where we
> > can have:
> >
> > - averaged data;
> > - normal differential;
> > - test pattern (btw, useful to have it in debugfs - but can come later);
> > - 8 common mode bits;
> >
> > While the average, normal and test pattern are really mutual exclusive, the
> > common mode voltage is different in the way that it's appended to differential
> > sample. Making it kind of an aggregated thingy. Thus I guess it can make sense
> > for us to see them as different channels from a SW perspective (even more since
> > gain and offset only apply to the differential data). But there are a couple of
> > things I don't like (have concerns):
> >
> > * You're pushing the CM channels into the end. So when we a 2 channel device
> > we'll have:
> >
> >  in_voltage0 - diff
> >  in_voltage1 - diff
> >  in_voltage2 - CM associated with chan0
> >  in_voltage0 - CM associated with chan1
> >
> > I think we could make it so the CM channel comes right after the channel where
> > it's data belongs too. So for example, odd channels would be CM channels (and
> > labels could also make sense).  
> 
> I must agree with you it would make more sense.
> 
> > Other thing that came to mind is if we could somehow use differential = true
> > here. Having something like:
> >
> > in_voltage1_in_voltage0_raw - diff data
> > ...
> > And the only thing for CM would be:
> >
> > in_voltage1_raw
> > in_voltage1_scale
> >
> > (not sure if the above is doable with ext_info - maybe only with device_attrs)
> >
> > The downside of the above is that we don't have a way to separate the scan
> > elements. Meaning that we don't have a way to specify the scan_type for both the
> > common mode and differential voltage. That said, I wonder if it is that useful
> > to buffer the common mode stuff? Alternatively, we could just have the scan_type
> > for the diff data and apps really wanting the CM voltage could still access the
> > raw data. Not pretty, I know...  
> 
> At the moment the way I "separate" them is by looking at the
> `active_scan_mask`. If the user asked for differential channel only, I put the
> chip in differential only mode. If all the channels are asked, I put
> the chip in differential + common mode. This way there is no need to
> separate anything in differential mode. See below for an example where
> this started.
> 
> > However, even if we go with the two separate channels there's one thing that
> > concerns me. Right now we have diff data with 32 for storage bits and CM data
> > with 8 storage bits which means the sample will be 40 bits and in reality we
> > just have 32. Sure, now we have SW buffering so we can make it work but the
> > ultimate goal is to support HW buffering where we won't be able to touch the
> > sample and thus we can't lie about the sample size. Could you run any test with
> > this approach on a HW buffer setup?   
> 
> Let's take AD4630-24 in diff+cm mode as an example. We would have 4 channels:
> - Ch0 diff with 24 bits of realbits and 24 bits of storagebits
> - Ch0 cm with 8 bits of realbits and 8 bits of storagebits
> - Ch1 diff with 24 bits of realbits and 24 bits of storagebits
> - Ch1 cm with 8 bits of realbits and 8 bits of storagebits
> ChX diff realbits + ChX cm realbits = 32 bits which is one sample as sent
> by the chip.
> 
> The problem I faced when trying to do this in this series is that IIO doesn't
> seem to like 24 storagebits and the data would get garbled. In diff only
> mode with the same channel setup (selecting only Ch0 diff and Ch1 diff)
> the data is also garbled. I used iio-oscilloscope software to test this setup.
> Here is the output with iio_readdev:
> ```
> # iio_readdev -s 1 ad4630-24 voltage0
> WARNING: High-speed mode not enabled
> Unable to refill buffer: Invalid argument (22)
> ```
> 
> I think this is happening when computing the padding to align ch1 diff.
> In `iio_compute_scan_bytes` this line `bytes = ALIGN(bytes, length);`
> will be invoked with bytes = 3 and length = 3 when selecting ch0 diff
> and ch1 diff (AD4630-24 in differential mode). The output is 5. As
> specified in linux/align.h:
> > @a is a power of 2  
> In our case `a` is `length`, and 3 is not a power of 2.
> 
> It works fine with Ch0/1 diff with 24 realbits and 32 storagebits with 8
> bits shift.
> 
> Intrestingly, a similar setup works great on AD4630-16:
> - Ch0 diff with 16 bits of realbits and 16 bits of storagebits
> - Ch0 cm with 8 bits of realbits and 8 bits of storagebits
> - Ch1 diff with 16 bits of realbits and 16 bits of storagebits
> - Ch1 cm with 8 bits of realbits and 8 bits of storagebits
> 
> In `iio_compute_scan_bytes` we will have ALIGN(3, 2) which will output
> 4, everything is fine. The output of iio-oscilloscope is as expected,
> a clean sinewave and iio_readdev does not throw an error.
> 
> All this to say that at the moment, I'm not sure how I will be able to
> put the CM byte in a separate channel for AD4630-24 without buffering it.
> It would be useful to return a "packed" buffer.

Whilst it might be useful to allow non power of 2 storage formats,
that's a break of the IIO userspace ABI so that isn't an approach to
consider. You must shuffle the data in the driver.

Esteban Blanc July 29, 2024, 2:42 p.m. UTC | #6

On Sat Jun 29, 2024 at 6:39 PM CEST, Jonathan Cameron wrote:
> On Thu, 27 Jun 2024 13:59:13 +0200
> Esteban Blanc <[e-mail]> wrote:
>
> > This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> > 
> > The driver implements basic support for the AD4030-24 1 channel
> > differential ADC with hardware gain and offset control.
> > 
> > Signed-off-by: Esteban Blanc <[e-mail]>

...

> > +static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
> > +			   void *val, size_t val_size)
> > +{
> > +	struct ad4030_state *st = context;
> > +
> > +	struct spi_transfer xfer = {
> > +		.tx_buf = st->tx_data,
> > +		.rx_buf = st->rx_data.raw,
> > +		.len = reg_size + val_size,
> > +	};
> > +	int ret;
> > +
> > +	memcpy(st->tx_data, reg, reg_size);
> > +
> > +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	memcpy(val, &st->rx_data.raw[reg_size], val_size);
>
> Can you just use spi_write_then_read() here?
>

I was planning on doing that. But I'm getting a timeout issue when
using `spi_write_then_read`. I can see the tx_data going out, rx_data
is recived but CS is kept asserted. I need to investigate more but in
the meantime I'm using this as it is working. I will remove this
workaround if I can find a fix and add a comment for now.

> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->chip->precision_bits == 16)
> > +		offset <<= 16;
> > +	else
> > +		offset <<= 8;
>
> As below. This is hard tor read. Just use appropriate unaligned gets for the
> two cases to extract the write bytes directly.
>
> > +	*val = be32_to_cpu(offset);
> > +
> > +	return 0;
> > +}
> > +
> > +static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int gain_int,
> > +				int gain_frac)
> > +{
> > +	struct ad4030_state *st = iio_priv(indio_dev);
> > +	__be16 val;
> > +	u64 gain;
> > +
> > +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> > +
> > +	if (gain > AD4030_REG_GAIN_MAX_GAIN)
> > +		return -EINVAL;
> > +
> > +	val = cpu_to_be16(DIV_ROUND_CLOSEST_ULL(gain * 0x8000, MICRO));
> > +
> > +	return regmap_bulk_write(st->regmap, AD4030_REG_GAIN_CHAN(ch), &val,
> > +			  AD4030_REG_GAIN_BYTES_NB);
> > +}
> > +
> > +static int ad4030_set_chan_offset(struct iio_dev *indio_dev, int ch, int offset)
> > +{
> > +	struct ad4030_state *st = iio_priv(indio_dev);
> > +	__be32 val;
> > +
> > +	if (offset < st->min_offset || offset > st->max_offset)
> > +		return -EINVAL;
> > +
> > +	val = cpu_to_be32(offset);
> > +	if (st->chip->precision_bits == 16)
> > +		val >>= 16;
> > +	else
> > +		val >>= 8;
>
> I 'think' I get what this is doing but not 100% sure as it's a bit too unusual
> and I'm not even sure what happens if we shift a __be32 value on a little endian
> system. I would instead split this into appropriate cpu_to_be24() and cpu_to_be16()
> to put the value directly in the right place rather than shifting in place.

cpu_to_be24 does not exist yet. I will have a look on how to add them.


All the other comments will be addressed in V2.

Best regards,

Jonathan Cameron July 29, 2024, 8:13 p.m. UTC | #7

On Mon, 29 Jul 2024 16:42:16 +0200
"Esteban Blanc" <[e-mail]> wrote:

> On Sat Jun 29, 2024 at 6:39 PM CEST, Jonathan Cameron wrote:
> > On Thu, 27 Jun 2024 13:59:13 +0200
> > Esteban Blanc <[e-mail]> wrote:
> >  
> > > This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> > > 
> > > The driver implements basic support for the AD4030-24 1 channel
> > > differential ADC with hardware gain and offset control.
> > > 
> > > Signed-off-by: Esteban Blanc <[e-mail]>  
> 
> ...
> 
> > > +static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
> > > +			   void *val, size_t val_size)
> > > +{
> > > +	struct ad4030_state *st = context;
> > > +
> > > +	struct spi_transfer xfer = {
> > > +		.tx_buf = st->tx_data,
> > > +		.rx_buf = st->rx_data.raw,
> > > +		.len = reg_size + val_size,
> > > +	};
> > > +	int ret;
> > > +
> > > +	memcpy(st->tx_data, reg, reg_size);
> > > +
> > > +	ret = spi_sync_transfer(st->spi, &xfer, 1);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	memcpy(val, &st->rx_data.raw[reg_size], val_size);  
> >
> > Can you just use spi_write_then_read() here?
> >  
> 
> I was planning on doing that. But I'm getting a timeout issue when
> using `spi_write_then_read`. I can see the tx_data going out, rx_data
> is recived but CS is kept asserted. I need to investigate more but in
> the meantime I'm using this as it is working. I will remove this
> workaround if I can find a fix and add a comment for now.
Fair enough. We've had a few drivers where the timing doesn't work
recently. Definitely good to leave a comment to avoid a 'fix' :)

> 
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (st->chip->precision_bits == 16)
> > > +		offset <<= 16;
> > > +	else
> > > +		offset <<= 8;  
> >
> > As below. This is hard tor read. Just use appropriate unaligned gets for the
> > two cases to extract the write bytes directly.
> >  
> > > +	*val = be32_to_cpu(offset);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int gain_int,
> > > +				int gain_frac)
> > > +{
> > > +	struct ad4030_state *st = iio_priv(indio_dev);
> > > +	__be16 val;
> > > +	u64 gain;
> > > +
> > > +	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
> > > +
> > > +	if (gain > AD4030_REG_GAIN_MAX_GAIN)
> > > +		return -EINVAL;
> > > +
> > > +	val = cpu_to_be16(DIV_ROUND_CLOSEST_ULL(gain * 0x8000, MICRO));
> > > +
> > > +	return regmap_bulk_write(st->regmap, AD4030_REG_GAIN_CHAN(ch), &val,
> > > +			  AD4030_REG_GAIN_BYTES_NB);
> > > +}
> > > +
> > > +static int ad4030_set_chan_offset(struct iio_dev *indio_dev, int ch, int offset)
> > > +{
> > > +	struct ad4030_state *st = iio_priv(indio_dev);
> > > +	__be32 val;
> > > +
> > > +	if (offset < st->min_offset || offset > st->max_offset)
> > > +		return -EINVAL;
> > > +
> > > +	val = cpu_to_be32(offset);
> > > +	if (st->chip->precision_bits == 16)
> > > +		val >>= 16;
> > > +	else
> > > +		val >>= 8;  
> >
> > I 'think' I get what this is doing but not 100% sure as it's a bit too unusual
> > and I'm not even sure what happens if we shift a __be32 value on a little endian
> > system. I would instead split this into appropriate cpu_to_be24() and cpu_to_be16()
> > to put the value directly in the right place rather than shifting in place.  
> 
> cpu_to_be24 does not exist yet. I will have a look on how to add them.
Ah. Almost by definition be24 isn't aligned in some cases.
So put_unaligned_be24() is what you are looking for.
My mistake!

Jonathan

> 
> 
> All the other comments will be addressed in V2.
> 
> Best regards,
>

Christophe JAILLET July 29, 2024, 9:34 p.m. UTC | #8

Le 27/06/2024 à 13:59, Esteban Blanc a écrit :
> This adds a new driver for the Analog Devices INC. AD4030-24 ADC.
> 
> The driver implements basic support for the AD4030-24 1 channel
> differential ADC with hardware gain and offset control.
> 
> Signed-off-by: Esteban Blanc <eblanc-rdvid1DuHRBWk0Htik3J/[e-mail]>
> ---

Hi,

a few nitpick below, should it help.

...

> +static int ad4030_read_avail(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *channel,
> +			     const int **vals, int *type,
> +			     int *length, long mask)
> +{
> +	struct ad4030_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		*vals = ad4030_get_offset_avail(st);
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_RANGE;
> +
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		*vals = (void *)ad4030_gain_avail;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_RANGE;

Nitpick: in other switch below, there is a blank line before default:

> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static int ad4030_regulators_get(struct ad4030_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	static const char * const ids[] = {"vdd-5v", "vdd-1v8"};
> +	int ret;
> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ids), ids);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> +	st->vio_uv = devm_regulator_get_enable_read_voltage(dev, "vio");
> +	if (st->vio_uv < 0)
> +		return dev_err_probe(dev, st->vio_uv,
> +				     "Failed to enable and read vio voltage");

Nitpick: missing \n

> +
> +	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
> +	if (st->vref_uv < 0) {
> +		if (st->vref_uv != -ENODEV)
> +			return dev_err_probe(dev, st->vref_uv,
> +					     "Failed to read vref voltage");

Nitpick: missing \n

> +
> +		/* if not using optional REF, the internal REFIN must be used */
> +		st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "refin");
> +		if (st->vref_uv < 0)
> +			return dev_err_probe(dev, st->vref_uv,
> +					     "Failed to read vrefin voltage");

Nitpick: missing \n

> +	}
> +
> +	if (st->vref_uv < AD4030_VREF_MIN_UV || st->vref_uv > AD4030_VREF_MAX_UV)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "vref(%d) must be in the range [%lu %lu]\n",
> +				     st->vref_uv, AD4030_VREF_MIN_UV,
> +				     AD4030_VREF_MAX_UV);
> +
> +	return 0;
> +}

...

> +static int ad4030_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad4030_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));

Nitpick: dev could be used. It is the same &spi->dev

> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	/* Make sure the SPI clock is within range to read register */
> +	st->conversion_speed_hz = spi->max_speed_hz;
> +	if (spi->max_speed_hz > AD4030_SPI_MAX_REG_XFER_SPEED)
> +		spi->max_speed_hz = AD4030_SPI_MAX_REG_XFER_SPEED;
> +
> +	st->regmap = devm_regmap_init(&spi->dev, &ad4030_regmap_bus, st,

Nitpick: dev could be used. It is the same &spi->dev

> +				      &ad4030_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		dev_err_probe(&spi->dev, PTR_ERR(st->regmap),

Nitpick: dev could be used. It is the same &spi->dev

> +			      "Failed to initialize regmap\n");
> +
> +	st->chip = spi_get_device_match_data(spi);
> +	if (!st->chip)
> +		return -EINVAL;
> +
> +	ret = ad4030_regulators_get(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_reset(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_detect_chip_info(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad4030_config(st);
> +	if (ret)
> +		return ret;
> +
> +	st->cnv_gpio = devm_gpiod_get(dev, "cnv", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->cnv_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
> +				     "Failed to get cnv gpio");

Nitpick: missing \n

> +
> +	indio_dev->name = st->chip->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ad4030_iio_info;
> +	indio_dev->channels = st->chip->channels;
> +	indio_dev->num_channels =  2 * st->chip->num_channels + 1;

Nitpick : 2 spaces after =

> +	indio_dev->available_scan_masks = st->chip->available_masks;
> +	indio_dev->masklength = st->chip->available_masks_len;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +					      iio_pollfunc_store_time,
> +					      ad4030_trigger_handler,
> +					      &ad4030_buffer_setup_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

...

CJ

Nuno Sá July 31, 2024, 8:56 a.m. UTC | #9

On Fri, 2024-07-26 at 15:38 +0200, Esteban Blanc wrote:
> Hi Nuno,
> 
> > > +			struct {
> > > +				s32 val[AD4030_MAX_DIFF_CHANNEL_NB];
> > > +				u8 common[AD4030_MAX_COMMON_CHANNEL_NB];
> > 
> > Not sure common makes sense as it comes aggregated with the sample. Maybe
> > this
> > could as simple as:
> > 
> > struct {
> > 	s32 val;
> > 	u64 timestamp __aligned(8);
> > } rx_data ...
> 
> See below my answer on channels order and storagebits.
> 
> > So, from the datasheet, figure 39 we have something like a multiplexer where
> > we
> > can have:
> > 
> > - averaged data;
> > - normal differential;
> > - test pattern (btw, useful to have it in debugfs - but can come later);
> > - 8 common mode bits;
> > 
> > While the average, normal and test pattern are really mutual exclusive, the
> > common mode voltage is different in the way that it's appended to
> > differential
> > sample. Making it kind of an aggregated thingy. Thus I guess it can make
> > sense
> > for us to see them as different channels from a SW perspective (even more
> > since
> > gain and offset only apply to the differential data). But there are a couple
> > of
> > things I don't like (have concerns):
> > 
> > * You're pushing the CM channels into the end. So when we a 2 channel device
> > we'll have:
> > 
> >  in_voltage0 - diff
> >  in_voltage1 - diff
> >  in_voltage2 - CM associated with chan0
> >  in_voltage0 - CM associated with chan1
> > 
> > I think we could make it so the CM channel comes right after the channel
> > where
> > it's data belongs too. So for example, odd channels would be CM channels
> > (and
> > labels could also make sense).
> 
> I must agree with you it would make more sense.
> 
> > Other thing that came to mind is if we could somehow use differential = true
> > here. Having something like:
> > 
> > in_voltage1_in_voltage0_raw - diff data
> > ...
> > And the only thing for CM would be:
> > 
> > in_voltage1_raw
> > in_voltage1_scale
> > 
> > (not sure if the above is doable with ext_info - maybe only with
> > device_attrs)
> > 
> > The downside of the above is that we don't have a way to separate the scan
> > elements. Meaning that we don't have a way to specify the scan_type for both
> > the
> > common mode and differential voltage. That said, I wonder if it is that
> > useful
> > to buffer the common mode stuff? Alternatively, we could just have the
> > scan_type
> > for the diff data and apps really wanting the CM voltage could still access
> > the
> > raw data. Not pretty, I know...
> 
> At the moment the way I "separate" them is by looking at the
> `active_scan_mask`. If the user asked for differential channel only, I put the
> chip in differential only mode. If all the channels are asked, I put
> the chip in differential + common mode. This way there is no need to
> separate anything in differential mode. See below for an example where
> this started.
> 
> > However, even if we go with the two separate channels there's one thing that
> > concerns me. Right now we have diff data with 32 for storage bits and CM
> > data
> > with 8 storage bits which means the sample will be 40 bits and in reality we
> > just have 32. Sure, now we have SW buffering so we can make it work but the
> > ultimate goal is to support HW buffering where we won't be able to touch the
> > sample and thus we can't lie about the sample size. Could you run any test
> > with
> > this approach on a HW buffer setup? 
> 
> Let's take AD4630-24 in diff+cm mode as an example. We would have 4 channels:
> - Ch0 diff with 24 bits of realbits and 24 bits of storagebits
> - Ch0 cm with 8 bits of realbits and 8 bits of storagebits
> - Ch1 diff with 24 bits of realbits and 24 bits of storagebits
> - Ch1 cm with 8 bits of realbits and 8 bits of storagebits
> ChX diff realbits + ChX cm realbits = 32 bits which is one sample as sent
> by the chip.
> 
> The problem I faced when trying to do this in this series is that IIO doesn't
> seem to like 24 storagebits and the data would get garbled. In diff only
> mode with the same channel setup (selecting only Ch0 diff and Ch1 diff)
> the data is also garbled. I used iio-oscilloscope software to test this setup.
> Here is the output with iio_readdev:
> ```
> # iio_readdev -s 1 ad4630-24 voltage0
> WARNING: High-speed mode not enabled
> Unable to refill buffer: Invalid argument (22)
> ```
> 
> I think this is happening when computing the padding to align ch1 diff.
> In `iio_compute_scan_bytes` this line `bytes = ALIGN(bytes, length);`
> will be invoked with bytes = 3 and length = 3 when selecting ch0 diff
> and ch1 diff (AD4630-24 in differential mode). The output is 5. As
> specified in linux/align.h:
> > @a is a power of 2
> In our case `a` is `length`, and 3 is not a power of 2.
> 
> It works fine with Ch0/1 diff with 24 realbits and 32 storagebits with 8
> bits shift.

Yes, I do understand that and that we need a power of 2 for storage bits. My
concern, as stated, is that if we have HW buffering (High speed enabled) CHO
will have a sample size of (with diff + cm) of 40 which is not really what comes
from HW. I wonder if it will work in that case. Maybe we can (as this often
happens on an FGPA) have the HW guys doing some data shuffling so things work in
the high speed mode as well.

- Nuno Sá

Nuno Sá July 31, 2024, 9:07 a.m. UTC | #10

On Sun, 2024-07-28 at 16:04 +0100, Jonathan Cameron wrote:
> One quick comment form me inline.
> 
> The short version is non power of 2 storage is not an option because
> it is a major ABI change and we aren't paying the cost of complexity
> that brings to userspace for a very small number of drivers where
> there is any real advantage to packing them tighter.
> 
> > 
> > > So, from the datasheet, figure 39 we have something like a multiplexer
> > > where we
> > > can have:
> > > 
> > > - averaged data;
> > > - normal differential;
> > > - test pattern (btw, useful to have it in debugfs - but can come later);
> > > - 8 common mode bits;
> > > 
> > > While the average, normal and test pattern are really mutual exclusive,
> > > the
> > > common mode voltage is different in the way that it's appended to
> > > differential
> > > sample. Making it kind of an aggregated thingy. Thus I guess it can make
> > > sense
> > > for us to see them as different channels from a SW perspective (even more
> > > since
> > > gain and offset only apply to the differential data). But there are a
> > > couple of
> > > things I don't like (have concerns):
> > > 
> > > * You're pushing the CM channels into the end. So when we a 2 channel
> > > device
> > > we'll have:
> > > 
> > >  in_voltage0 - diff
> > >  in_voltage1 - diff
> > >  in_voltage2 - CM associated with chan0
> > >  in_voltage0 - CM associated with chan1
> > > 
> > > I think we could make it so the CM channel comes right after the channel
> > > where
> > > it's data belongs too. So for example, odd channels would be CM channels
> > > (and
> > > labels could also make sense).  
> > 
> > I must agree with you it would make more sense.
> > 
> > > Other thing that came to mind is if we could somehow use differential =
> > > true
> > > here. Having something like:
> > > 
> > > in_voltage1_in_voltage0_raw - diff data
> > > ...
> > > And the only thing for CM would be:
> > > 
> > > in_voltage1_raw
> > > in_voltage1_scale
> > > 
> > > (not sure if the above is doable with ext_info - maybe only with
> > > device_attrs)
> > > 
> > > The downside of the above is that we don't have a way to separate the scan
> > > elements. Meaning that we don't have a way to specify the scan_type for
> > > both the
> > > common mode and differential voltage. That said, I wonder if it is that
> > > useful
> > > to buffer the common mode stuff? Alternatively, we could just have the
> > > scan_type
> > > for the diff data and apps really wanting the CM voltage could still
> > > access the
> > > raw data. Not pretty, I know...  
> > 
> > At the moment the way I "separate" them is by looking at the
> > `active_scan_mask`. If the user asked for differential channel only, I put
> > the
> > chip in differential only mode. If all the channels are asked, I put
> > the chip in differential + common mode. This way there is no need to
> > separate anything in differential mode. See below for an example where
> > this started.
> > 
> > > However, even if we go with the two separate channels there's one thing
> > > that
> > > concerns me. Right now we have diff data with 32 for storage bits and CM
> > > data
> > > with 8 storage bits which means the sample will be 40 bits and in reality
> > > we
> > > just have 32. Sure, now we have SW buffering so we can make it work but
> > > the
> > > ultimate goal is to support HW buffering where we won't be able to touch
> > > the
> > > sample and thus we can't lie about the sample size. Could you run any test
> > > with
> > > this approach on a HW buffer setup?   
> > 
> > Let's take AD4630-24 in diff+cm mode as an example. We would have 4
> > channels:
> > - Ch0 diff with 24 bits of realbits and 24 bits of storagebits
> > - Ch0 cm with 8 bits of realbits and 8 bits of storagebits
> > - Ch1 diff with 24 bits of realbits and 24 bits of storagebits
> > - Ch1 cm with 8 bits of realbits and 8 bits of storagebits
> > ChX diff realbits + ChX cm realbits = 32 bits which is one sample as sent
> > by the chip.
> > 
> > The problem I faced when trying to do this in this series is that IIO
> > doesn't
> > seem to like 24 storagebits and the data would get garbled. In diff only
> > mode with the same channel setup (selecting only Ch0 diff and Ch1 diff)
> > the data is also garbled. I used iio-oscilloscope software to test this
> > setup.
> > Here is the output with iio_readdev:
> > ```
> > # iio_readdev -s 1 ad4630-24 voltage0
> > WARNING: High-speed mode not enabled
> > Unable to refill buffer: Invalid argument (22)
> > ```
> > 
> > I think this is happening when computing the padding to align ch1 diff.
> > In `iio_compute_scan_bytes` this line `bytes = ALIGN(bytes, length);`
> > will be invoked with bytes = 3 and length = 3 when selecting ch0 diff
> > and ch1 diff (AD4630-24 in differential mode). The output is 5. As
> > specified in linux/align.h:
> > > @a is a power of 2  
> > In our case `a` is `length`, and 3 is not a power of 2.
> > 
> > It works fine with Ch0/1 diff with 24 realbits and 32 storagebits with 8
> > bits shift.
> > 
> > Intrestingly, a similar setup works great on AD4630-16:
> > - Ch0 diff with 16 bits of realbits and 16 bits of storagebits
> > - Ch0 cm with 8 bits of realbits and 8 bits of storagebits
> > - Ch1 diff with 16 bits of realbits and 16 bits of storagebits
> > - Ch1 cm with 8 bits of realbits and 8 bits of storagebits
> > 
> > In `iio_compute_scan_bytes` we will have ALIGN(3, 2) which will output
> > 4, everything is fine. The output of iio-oscilloscope is as expected,
> > a clean sinewave and iio_readdev does not throw an error.
> > 
> > All this to say that at the moment, I'm not sure how I will be able to
> > put the CM byte in a separate channel for AD4630-24 without buffering it.
> > It would be useful to return a "packed" buffer.
> 
> Whilst it might be useful to allow non power of 2 storage formats,
> that's a break of the IIO userspace ABI so that isn't an approach to
> consider. You must shuffle the data in the driver.

Yeah, I do agree the breakage is really not the way to go...

OTOH, I'm seeing more and more of these devices with kind of multiplexed
data/channels in one sample (like cm and diff in this case) and while in SW
buffering we can workaround it in the driver, for HW buffering things may be not
that "simple".

Not sure what we can do about it but one concept that came to mind when I was
giving some thinking about this was kind of a virtual scan element that would
essentially map/link to a (physical) scan element (so virtual_scan + scan =
storage_size of the real scan element). Kind of a basic idea for now and I'm not
really sure how much work would this be or even how could we expose this to
userspace without breaking current ABI (basically if it's doable at all :)).

The only other option I see for these kind of devices (if there's nothing we can
do in HW for shuffling data) is to expose a different channel setup that does
not "lie" about the sample size. And it's up to userspace to parse the sample
data. Far from pretty though...

- Nuno Sá

Jonathan Cameron Aug. 3, 2024, 9:48 a.m. UTC | #11

On Wed, 31 Jul 2024 11:07:14 +0200
Nuno Sá <[e-mail]> wrote:

> On Sun, 2024-07-28 at 16:04 +0100, Jonathan Cameron wrote:
> > One quick comment form me inline.
> > 
> > The short version is non power of 2 storage is not an option because
> > it is a major ABI change and we aren't paying the cost of complexity
> > that brings to userspace for a very small number of drivers where
> > there is any real advantage to packing them tighter.
> >   
> > >   
> > > > So, from the datasheet, figure 39 we have something like a multiplexer
> > > > where we
> > > > can have:
> > > > 
> > > > - averaged data;
> > > > - normal differential;
> > > > - test pattern (btw, useful to have it in debugfs - but can come later);
> > > > - 8 common mode bits;
> > > > 
> > > > While the average, normal and test pattern are really mutual exclusive,
> > > > the
> > > > common mode voltage is different in the way that it's appended to
> > > > differential
> > > > sample. Making it kind of an aggregated thingy. Thus I guess it can make
> > > > sense
> > > > for us to see them as different channels from a SW perspective (even more
> > > > since
> > > > gain and offset only apply to the differential data). But there are a
> > > > couple of
> > > > things I don't like (have concerns):
> > > > 
> > > > * You're pushing the CM channels into the end. So when we a 2 channel
> > > > device
> > > > we'll have:
> > > > 
> > > >  in_voltage0 - diff
> > > >  in_voltage1 - diff
> > > >  in_voltage2 - CM associated with chan0
> > > >  in_voltage0 - CM associated with chan1
> > > > 
> > > > I think we could make it so the CM channel comes right after the channel
> > > > where
> > > > it's data belongs too. So for example, odd channels would be CM channels
> > > > (and
> > > > labels could also make sense).    
> > > 
> > > I must agree with you it would make more sense.
> > >   
> > > > Other thing that came to mind is if we could somehow use differential =
> > > > true
> > > > here. Having something like:
> > > > 
> > > > in_voltage1_in_voltage0_raw - diff data
> > > > ...
> > > > And the only thing for CM would be:
> > > > 
> > > > in_voltage1_raw
> > > > in_voltage1_scale
> > > > 
> > > > (not sure if the above is doable with ext_info - maybe only with
> > > > device_attrs)
> > > > 
> > > > The downside of the above is that we don't have a way to separate the scan
> > > > elements. Meaning that we don't have a way to specify the scan_type for
> > > > both the
> > > > common mode and differential voltage. That said, I wonder if it is that
> > > > useful
> > > > to buffer the common mode stuff? Alternatively, we could just have the
> > > > scan_type
> > > > for the diff data and apps really wanting the CM voltage could still
> > > > access the
> > > > raw data. Not pretty, I know...    
> > > 
> > > At the moment the way I "separate" them is by looking at the
> > > `active_scan_mask`. If the user asked for differential channel only, I put
> > > the
> > > chip in differential only mode. If all the channels are asked, I put
> > > the chip in differential + common mode. This way there is no need to
> > > separate anything in differential mode. See below for an example where
> > > this started.
> > >   
> > > > However, even if we go with the two separate channels there's one thing
> > > > that
> > > > concerns me. Right now we have diff data with 32 for storage bits and CM
> > > > data
> > > > with 8 storage bits which means the sample will be 40 bits and in reality
> > > > we
> > > > just have 32. Sure, now we have SW buffering so we can make it work but
> > > > the
> > > > ultimate goal is to support HW buffering where we won't be able to touch
> > > > the
> > > > sample and thus we can't lie about the sample size. Could you run any test
> > > > with
> > > > this approach on a HW buffer setup?     
> > > 
> > > Let's take AD4630-24 in diff+cm mode as an example. We would have 4
> > > channels:
> > > - Ch0 diff with 24 bits of realbits and 24 bits of storagebits
> > > - Ch0 cm with 8 bits of realbits and 8 bits of storagebits
> > > - Ch1 diff with 24 bits of realbits and 24 bits of storagebits
> > > - Ch1 cm with 8 bits of realbits and 8 bits of storagebits
> > > ChX diff realbits + ChX cm realbits = 32 bits which is one sample as sent
> > > by the chip.
> > > 
> > > The problem I faced when trying to do this in this series is that IIO
> > > doesn't
> > > seem to like 24 storagebits and the data would get garbled. In diff only
> > > mode with the same channel setup (selecting only Ch0 diff and Ch1 diff)
> > > the data is also garbled. I used iio-oscilloscope software to test this
> > > setup.
> > > Here is the output with iio_readdev:
> > > ```
> > > # iio_readdev -s 1 ad4630-24 voltage0
> > > WARNING: High-speed mode not enabled
> > > Unable to refill buffer: Invalid argument (22)
> > > ```
> > > 
> > > I think this is happening when computing the padding to align ch1 diff.
> > > In `iio_compute_scan_bytes` this line `bytes = ALIGN(bytes, length);`
> > > will be invoked with bytes = 3 and length = 3 when selecting ch0 diff
> > > and ch1 diff (AD4630-24 in differential mode). The output is 5. As
> > > specified in linux/align.h:  
> > > > @a is a power of 2    
> > > In our case `a` is `length`, and 3 is not a power of 2.
> > > 
> > > It works fine with Ch0/1 diff with 24 realbits and 32 storagebits with 8
> > > bits shift.
> > > 
> > > Intrestingly, a similar setup works great on AD4630-16:
> > > - Ch0 diff with 16 bits of realbits and 16 bits of storagebits
> > > - Ch0 cm with 8 bits of realbits and 8 bits of storagebits
> > > - Ch1 diff with 16 bits of realbits and 16 bits of storagebits
> > > - Ch1 cm with 8 bits of realbits and 8 bits of storagebits
> > > 
> > > In `iio_compute_scan_bytes` we will have ALIGN(3, 2) which will output
> > > 4, everything is fine. The output of iio-oscilloscope is as expected,
> > > a clean sinewave and iio_readdev does not throw an error.
> > > 
> > > All this to say that at the moment, I'm not sure how I will be able to
> > > put the CM byte in a separate channel for AD4630-24 without buffering it.
> > > It would be useful to return a "packed" buffer.  
> > 
> > Whilst it might be useful to allow non power of 2 storage formats,
> > that's a break of the IIO userspace ABI so that isn't an approach to
> > consider. You must shuffle the data in the driver.  
> 
> Yeah, I do agree the breakage is really not the way to go...
> 
> OTOH, I'm seeing more and more of these devices with kind of multiplexed
> data/channels in one sample (like cm and diff in this case) and while in SW
> buffering we can workaround it in the driver, for HW buffering things may be not
> that "simple".
> 
> Not sure what we can do about it but one concept that came to mind when I was
> giving some thinking about this was kind of a virtual scan element that would
> essentially map/link to a (physical) scan element (so virtual_scan + scan =
> storage_size of the real scan element). Kind of a basic idea for now and I'm not
> really sure how much work would this be or even how could we expose this to
> userspace without breaking current ABI (basically if it's doable at all :)).
> 
> The only other option I see for these kind of devices (if there's nothing we can
> do in HW for shuffling data) is to expose a different channel setup that does
> not "lie" about the sample size. And it's up to userspace to parse the sample
> data. Far from pretty though...

For stuff that is coming out of the DMA / bulk interfaces, I'm rather
more flexible on new data layouts.  That tends to run against a narrow
range of tooling + generally users have a better idea of what they
are doing (after all they probably bought some pricey hardware :)

So there I'd be happy to see proposals for other storage format descriptions
or indeed relaxing the requirements on existing description.

If there is a software path to simply reorganize the data, keep
to existing interfaces with the power of 2 aligned data requirements.

Jonathan

> 
> - Nuno Sá

Jonathan Cameron Aug. 3, 2024, 9:51 a.m. UTC | #12

On Wed, 31 Jul 2024 10:56:22 +0200
Nuno Sá <[e-mail]> wrote:

> On Fri, 2024-07-26 at 15:38 +0200, Esteban Blanc wrote:
> > Hi Nuno,
> >   
> > > > +			struct {
> > > > +				s32 val[AD4030_MAX_DIFF_CHANNEL_NB];
> > > > +				u8 common[AD4030_MAX_COMMON_CHANNEL_NB];  
> > > 
> > > Not sure common makes sense as it comes aggregated with the sample. Maybe
> > > this
> > > could as simple as:
> > > 
> > > struct {
> > > 	s32 val;
> > > 	u64 timestamp __aligned(8);
> > > } rx_data ...  
> > 
> > See below my answer on channels order and storagebits.
> >   
> > > So, from the datasheet, figure 39 we have something like a multiplexer where
> > > we
> > > can have:
> > > 
> > > - averaged data;
> > > - normal differential;
> > > - test pattern (btw, useful to have it in debugfs - but can come later);
> > > - 8 common mode bits;
> > > 
> > > While the average, normal and test pattern are really mutual exclusive, the
> > > common mode voltage is different in the way that it's appended to
> > > differential
> > > sample. Making it kind of an aggregated thingy. Thus I guess it can make
> > > sense
> > > for us to see them as different channels from a SW perspective (even more
> > > since
> > > gain and offset only apply to the differential data). But there are a couple
> > > of
> > > things I don't like (have concerns):
> > > 
> > > * You're pushing the CM channels into the end. So when we a 2 channel device
> > > we'll have:
> > > 
> > >  in_voltage0 - diff
> > >  in_voltage1 - diff
> > >  in_voltage2 - CM associated with chan0
> > >  in_voltage0 - CM associated with chan1
> > > 
> > > I think we could make it so the CM channel comes right after the channel
> > > where
> > > it's data belongs too. So for example, odd channels would be CM channels
> > > (and
> > > labels could also make sense).  
> > 
> > I must agree with you it would make more sense.
> >   
> > > Other thing that came to mind is if we could somehow use differential = true
> > > here. Having something like:
> > > 
> > > in_voltage1_in_voltage0_raw - diff data
> > > ...
> > > And the only thing for CM would be:
> > > 
> > > in_voltage1_raw
> > > in_voltage1_scale
> > > 
> > > (not sure if the above is doable with ext_info - maybe only with
> > > device_attrs)
> > > 
> > > The downside of the above is that we don't have a way to separate the scan
> > > elements. Meaning that we don't have a way to specify the scan_type for both
> > > the
> > > common mode and differential voltage. That said, I wonder if it is that
> > > useful
> > > to buffer the common mode stuff? Alternatively, we could just have the
> > > scan_type
> > > for the diff data and apps really wanting the CM voltage could still access
> > > the
> > > raw data. Not pretty, I know...  
> > 
> > At the moment the way I "separate" them is by looking at the
> > `active_scan_mask`. If the user asked for differential channel only, I put the
> > chip in differential only mode. If all the channels are asked, I put
> > the chip in differential + common mode. This way there is no need to
> > separate anything in differential mode. See below for an example where
> > this started.
> >   
> > > However, even if we go with the two separate channels there's one thing that
> > > concerns me. Right now we have diff data with 32 for storage bits and CM
> > > data
> > > with 8 storage bits which means the sample will be 40 bits and in reality we
> > > just have 32. Sure, now we have SW buffering so we can make it work but the
> > > ultimate goal is to support HW buffering where we won't be able to touch the
> > > sample and thus we can't lie about the sample size. Could you run any test
> > > with
> > > this approach on a HW buffer setup?   
> > 
> > Let's take AD4630-24 in diff+cm mode as an example. We would have 4 channels:
> > - Ch0 diff with 24 bits of realbits and 24 bits of storagebits
> > - Ch0 cm with 8 bits of realbits and 8 bits of storagebits
> > - Ch1 diff with 24 bits of realbits and 24 bits of storagebits
> > - Ch1 cm with 8 bits of realbits and 8 bits of storagebits
> > ChX diff realbits + ChX cm realbits = 32 bits which is one sample as sent
> > by the chip.
> > 
> > The problem I faced when trying to do this in this series is that IIO doesn't
> > seem to like 24 storagebits and the data would get garbled. In diff only
> > mode with the same channel setup (selecting only Ch0 diff and Ch1 diff)
> > the data is also garbled. I used iio-oscilloscope software to test this setup.
> > Here is the output with iio_readdev:
> > ```
> > # iio_readdev -s 1 ad4630-24 voltage0
> > WARNING: High-speed mode not enabled
> > Unable to refill buffer: Invalid argument (22)
> > ```
> > 
> > I think this is happening when computing the padding to align ch1 diff.
> > In `iio_compute_scan_bytes` this line `bytes = ALIGN(bytes, length);`
> > will be invoked with bytes = 3 and length = 3 when selecting ch0 diff
> > and ch1 diff (AD4630-24 in differential mode). The output is 5. As
> > specified in linux/align.h:  
> > > @a is a power of 2  
> > In our case `a` is `length`, and 3 is not a power of 2.
> > 
> > It works fine with Ch0/1 diff with 24 realbits and 32 storagebits with 8
> > bits shift.  
> 
> Yes, I do understand that and that we need a power of 2 for storage bits. My
> concern, as stated, is that if we have HW buffering (High speed enabled) CHO
> will have a sample size of (with diff + cm) of 40 which is not really what comes
> from HW. I wonder if it will work in that case. Maybe we can (as this often
> happens on an FGPA) have the HW guys doing some data shuffling so things work in
> the high speed mode as well.

If it's possible to unscramble the data in an fpga, that would make our life easier
though things may get messy if we get multiple versions of that and some
unscramble, others don't.

What I really don't want to see is us having to describe all the crazy transmission
formats hardware can use such as bit interleaving. The descriptions will
get so complex we'll more or less end up with per device handling in userspace.

Jonathan


> 
> - Nuno Sá
> 
>

Esteban Blanc Aug. 5, 2024, 9:38 a.m. UTC | #13

On Sat Aug 3, 2024 at 11:51 AM CEST, Jonathan Cameron wrote:
> On Wed, 31 Jul 2024 10:56:22 +0200
> Nuno Sá <[e-mail]> wrote:
>
> > On Fri, 2024-07-26 at 15:38 +0200, Esteban Blanc wrote:
> > > Hi Nuno,
> > >   
> > > > > +			struct {
> > > > > +				s32 val[AD4030_MAX_DIFF_CHANNEL_NB];
> > > > > +				u8 common[AD4030_MAX_COMMON_CHANNEL_NB];  
> > > > 
> > > > Not sure common makes sense as it comes aggregated with the sample. Maybe
> > > > this
> > > > could as simple as:
> > > > 
> > > > struct {
> > > > 	s32 val;
> > > > 	u64 timestamp __aligned(8);
> > > > } rx_data ...  
> > > 
> > > See below my answer on channels order and storagebits.
> > >   
> > > > So, from the datasheet, figure 39 we have something like a multiplexer where
> > > > we
> > > > can have:
> > > > 
> > > > - averaged data;
> > > > - normal differential;
> > > > - test pattern (btw, useful to have it in debugfs - but can come later);
> > > > - 8 common mode bits;
> > > > 
> > > > While the average, normal and test pattern are really mutual exclusive, the
> > > > common mode voltage is different in the way that it's appended to
> > > > differential
> > > > sample. Making it kind of an aggregated thingy. Thus I guess it can make
> > > > sense
> > > > for us to see them as different channels from a SW perspective (even more
> > > > since
> > > > gain and offset only apply to the differential data). But there are a couple
> > > > of
> > > > things I don't like (have concerns):
> > > > 
> > > > * You're pushing the CM channels into the end. So when we a 2 channel device
> > > > we'll have:
> > > > 
> > > >  in_voltage0 - diff
> > > >  in_voltage1 - diff
> > > >  in_voltage2 - CM associated with chan0
> > > >  in_voltage0 - CM associated with chan1
> > > > 
> > > > I think we could make it so the CM channel comes right after the channel
> > > > where
> > > > it's data belongs too. So for example, odd channels would be CM channels
> > > > (and
> > > > labels could also make sense).  
> > > 
> > > I must agree with you it would make more sense.
> > >   
> > > > Other thing that came to mind is if we could somehow use differential = true
> > > > here. Having something like:
> > > > 
> > > > in_voltage1_in_voltage0_raw - diff data
> > > > ...
> > > > And the only thing for CM would be:
> > > > 
> > > > in_voltage1_raw
> > > > in_voltage1_scale
> > > > 
> > > > (not sure if the above is doable with ext_info - maybe only with
> > > > device_attrs)
> > > > 
> > > > The downside of the above is that we don't have a way to separate the scan
> > > > elements. Meaning that we don't have a way to specify the scan_type for both
> > > > the
> > > > common mode and differential voltage. That said, I wonder if it is that
> > > > useful
> > > > to buffer the common mode stuff? Alternatively, we could just have the
> > > > scan_type
> > > > for the diff data and apps really wanting the CM voltage could still access
> > > > the
> > > > raw data. Not pretty, I know...  
> > > 
> > > At the moment the way I "separate" them is by looking at the
> > > `active_scan_mask`. If the user asked for differential channel only, I put the
> > > chip in differential only mode. If all the channels are asked, I put
> > > the chip in differential + common mode. This way there is no need to
> > > separate anything in differential mode. See below for an example where
> > > this started.
> > >   
> > > > However, even if we go with the two separate channels there's one thing that
> > > > concerns me. Right now we have diff data with 32 for storage bits and CM
> > > > data
> > > > with 8 storage bits which means the sample will be 40 bits and in reality we
> > > > just have 32. Sure, now we have SW buffering so we can make it work but the
> > > > ultimate goal is to support HW buffering where we won't be able to touch the
> > > > sample and thus we can't lie about the sample size. Could you run any test
> > > > with
> > > > this approach on a HW buffer setup?   
> > > 
> > > Let's take AD4630-24 in diff+cm mode as an example. We would have 4 channels:
> > > - Ch0 diff with 24 bits of realbits and 24 bits of storagebits
> > > - Ch0 cm with 8 bits of realbits and 8 bits of storagebits
> > > - Ch1 diff with 24 bits of realbits and 24 bits of storagebits
> > > - Ch1 cm with 8 bits of realbits and 8 bits of storagebits
> > > ChX diff realbits + ChX cm realbits = 32 bits which is one sample as sent
> > > by the chip.
> > > 
> > > The problem I faced when trying to do this in this series is that IIO doesn't
> > > seem to like 24 storagebits and the data would get garbled. In diff only
> > > mode with the same channel setup (selecting only Ch0 diff and Ch1 diff)
> > > the data is also garbled. I used iio-oscilloscope software to test this setup.
> > > Here is the output with iio_readdev:
> > > ```
> > > # iio_readdev -s 1 ad4630-24 voltage0
> > > WARNING: High-speed mode not enabled
> > > Unable to refill buffer: Invalid argument (22)
> > > ```
> > > 
> > > I think this is happening when computing the padding to align ch1 diff.
> > > In `iio_compute_scan_bytes` this line `bytes = ALIGN(bytes, length);`
> > > will be invoked with bytes = 3 and length = 3 when selecting ch0 diff
> > > and ch1 diff (AD4630-24 in differential mode). The output is 5. As
> > > specified in linux/align.h:  
> > > > @a is a power of 2  
> > > In our case `a` is `length`, and 3 is not a power of 2.
> > > 
> > > It works fine with Ch0/1 diff with 24 realbits and 32 storagebits with 8
> > > bits shift.  
> > 
> > Yes, I do understand that and that we need a power of 2 for storage bits. My
> > concern, as stated, is that if we have HW buffering (High speed enabled) CHO
> > will have a sample size of (with diff + cm) of 40 which is not really what comes
> > from HW. I wonder if it will work in that case. Maybe we can (as this often
> > happens on an FGPA) have the HW guys doing some data shuffling so things work in
> > the high speed mode as well.
>
> If it's possible to unscramble the data in an fpga, that would make our life easier
> though things may get messy if we get multiple versions of that and some
> unscramble, others don't.

At the moment the HDL I could test for this chip is unscrambling the
data, so there is nothing to do on the software side on that front.

Best regards,

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 8ca5b2e09b69..8df171c62d37 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -419,6 +419,7 @@  R:	Esteban Blanc <[e-mail]>
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/adc/adi,ad4630.yaml
+F:	drivers/iio/adc/ad4030.c
 
 AD5110 ANALOG DEVICES DIGITAL POTENTIOMETERS DRIVER
 M:	Mugilraj Dhavachelvan <[e-mail]>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 3d91015af6ea..e71ac1e49acb 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -21,6 +21,19 @@  config AD_SIGMA_DELTA
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 
+config AD4030
+	tristate "Analog Device AD4630 ADC Driver"
+	depends on SPI
+	depends on GPIOLIB
+	select REGMAP_SPI
+	select IIO_BUFFER
+	help
+	  Say yes here to build support for Analog Devices AD4030 and AD4630 high speed
+	  SPI analog to digital converters (ADC).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad4030.
+
 config AD4130
 	tristate "Analog Device AD4130 ADC Driver"
 	depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 37ac689a0209..7a8945559589 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -6,6 +6,7 @@ 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
+obj-$(CONFIG_AD4030) += ad4030.o
 obj-$(CONFIG_AD4130) += ad4130.o
 obj-$(CONFIG_AD7091R) += ad7091r-base.o
 obj-$(CONFIG_AD7091R5) += ad7091r5.o
diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
new file mode 100644
index 000000000000..6d537e531d6f
--- /dev/null
+++ b/drivers/iio/adc/ad4030.c
@@ -0,0 +1,822 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD4030 and AD4630 ADC family driver.
+ *
+ * Copyright 2024 Analog Devices, Inc.
+ * Copyright 2024 BayLibre, SAS
+ *
+ * based on code from:
+ *	Analog Devices, Inc.
+ *	  Sergiu Cuciurean <[e-mail]>
+ *	  Nuno Sa <[e-mail]>
+ *	  Marcelo Schmitt <[e-mail]>
+ *	  Liviu Adace <[e-mail]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/units.h>
+#include <linux/clk.h>
+#include <linux/spi/spi.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define AD4030_REG_INTERFACE_CONFIG_A				0x00
+#define     AD4030_REG_INTERFACE_CONFIG_A_SW_RESET		(BIT(0) | BIT(7))
+#define AD4030_REG_INTERFACE_CONFIG_B				0x01
+#define AD4030_REG_DEVICE_CONFIG				0x02
+#define AD4030_REG_CHIP_TYPE					0x03
+#define AD4030_REG_PRODUCT_ID_L					0x04
+#define AD4030_REG_PRODUCT_ID_H					0x05
+#define AD4030_REG_CHIP_GRADE					0x06
+#define     AD4030_REG_CHIP_GRADE_AD4030_24_GRADE		0x10
+#define     AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE		GENMASK(7, 3)
+#define AD4030_REG_SCRATCH_PAD					0x0A
+#define AD4030_REG_SPI_REVISION					0x0B
+#define AD4030_REG_VENDOR_L					0x0C
+#define AD4030_REG_VENDOR_H					0x0D
+#define AD4030_REG_STREAM_MODE					0x0E
+#define AD4030_REG_INTERFACE_CONFIG_C				0x10
+#define AD4030_REG_INTERFACE_STATUS_A				0x11
+#define AD4030_REG_EXIT_CFG_MODE				0x14
+#define     AD4030_REG_EXIT_CFG_MODE_MASK_EXIT_CONFIG_MD	BIT(0)
+#define AD4030_REG_AVG						0x15
+#define     AD4030_REG_AVG_MASK_AVG_SYNC			BIT(7)
+#define     AD4030_REG_AVG_MASK_AVG_VAL				GENMASK(4, 0)
+#define AD4030_REG_OFFSET_X0_0					0x16
+#define AD4030_REG_OFFSET_X0_1					0x17
+#define AD4030_REG_OFFSET_X0_2					0x18
+#define AD4030_REG_OFFSET_X1_0					0x19
+#define AD4030_REG_OFFSET_X1_1					0x1A
+#define AD4030_REG_OFFSET_X1_2					0x1B
+#define     AD4030_REG_OFFSET_BYTES_NB				3
+#define     AD4030_REG_OFFSET_CHAN(ch)				(AD4030_REG_OFFSET_X0_2 +	\
+								(AD4030_REG_OFFSET_BYTES_NB *	\
+								(ch)))
+#define AD4030_REG_GAIN_X0_LSB					0x1C
+#define AD4030_REG_GAIN_X0_MSB					0x1D
+#define AD4030_REG_GAIN_X1_LSB					0x1E
+#define AD4030_REG_GAIN_X1_MSB					0x1F
+#define     AD4030_REG_GAIN_MAX_GAIN				1999970
+#define     AD4030_REG_GAIN_BYTES_NB				2
+#define     AD4030_REG_GAIN_CHAN(ch)				(AD4030_REG_GAIN_X0_MSB +	\
+								(AD4030_REG_GAIN_BYTES_NB *	\
+								(ch)))
+#define AD4030_REG_MODES					0x20
+#define     AD4030_REG_MODES_MASK_OUT_DATA_MODE			GENMASK(2, 0)
+#define     AD4030_REG_MODES_MASK_LANE_MODE			GENMASK(7, 6)
+#define AD4030_REG_OSCILATOR					0x21
+#define AD4030_REG_IO						0x22
+#define     AD4030_REG_IO_MASK_IO2X				BIT(1)
+#define AD4030_REG_PAT0						0x23
+#define AD4030_REG_PAT1						0x24
+#define AD4030_REG_PAT2						0x25
+#define AD4030_REG_PAT3						0x26
+#define AD4030_REG_DIG_DIAG					0x34
+#define AD4030_REG_DIG_ERR					0x35
+
+/* Sequence starting with "1 0 1" to enable reg access */
+#define AD4030_REG_ACCESS		0xa0
+
+#define AD4030_MAX_DIFF_CHANNEL_NB	2
+#define AD4030_MAX_COMMON_CHANNEL_NB	AD4030_MAX_DIFF_CHANNEL_NB
+#define AD4030_MAX_TIMESTAMP_CHANNEL_NB	1
+#define AD4030_ALL_CHANNELS_NB		(AD4030_MAX_DIFF_CHANNEL_NB +	\
+					AD4030_MAX_COMMON_CHANNEL_NB +	\
+					AD4030_MAX_TIMESTAMP_CHANNEL_NB)
+#define AD4030_VREF_MIN_UV		(4096 * MILLI)
+#define AD4030_VREF_MAX_UV		(5000 * MILLI)
+#define AD4030_VIO_THRESHOLD_UV		(1400 * MILLI)
+
+#define AD4030_SPI_MAX_XFER_LEN		8
+#define AD4030_SPI_MAX_REG_XFER_SPEED	(80 * MEGA)
+#define AD4030_TCNVH_NS			10
+#define AD4030_TCNVL_NS			20
+#define AD4030_TCONV_NS			(300 - (AD4030_TCNVH_NS +	\
+					AD4030_TCNVL_NS))
+#define AD4030_TRESET_PW_NS		50
+#define AD4030_TRESET_COM_DELAY_MS	750
+
+enum ad4030_out_mode {
+	AD4030_OUT_DATA_MD_16_DIFF = 0x00,
+	AD4030_OUT_DATA_MD_24_DIFF = 0x00,
+	AD4030_OUT_DATA_MD_16_DIFF_8_COM = 0x01,
+	AD4030_OUT_DATA_MD_24_DIFF_8_COM = 0x02,
+	AD4030_OUT_DATA_MD_30_AVERAGED_DIFF = 0x03,
+	AD4030_OUT_DATA_MD_32_PATTERN = 0x04
+};
+
+struct ad4030_chip_info {
+	const char *name;
+	const unsigned long *available_masks;
+	unsigned int available_masks_len;
+	const struct iio_chan_spec channels[AD4030_ALL_CHANNELS_NB];
+	u8 grade;
+	u8 precision_bits;
+	int num_channels;
+};
+
+struct ad4030_state {
+	struct spi_device *spi;
+	struct regmap *regmap;
+	const struct ad4030_chip_info *chip;
+	struct gpio_desc *cnv_gpio;
+	int vref_uv;
+	int vio_uv;
+	int min_offset;
+	int max_offset;
+	int offset_avail[3];
+	u32 conversion_speed_hz;
+	enum ad4030_out_mode mode;
+
+	/*
+	 * DMA (thus cache coherency maintenance) requires the transfer buffers
+	 * to live in their own cache lines.
+	 */
+	u8 tx_data[AD4030_SPI_MAX_XFER_LEN] __aligned(IIO_DMA_MINALIGN);
+	struct {
+		union {
+			/*
+			 * Make the buffer large enough for MAX_NUM_CHANNELS 32-bit samples and
+			 * one 64-bit aligned 64-bit timestamp.
+			 */
+			u8 raw[ALIGN(AD4030_MAX_DIFF_CHANNEL_NB * sizeof(u32) +
+			       AD4030_MAX_COMMON_CHANNEL_NB * sizeof(u8),
+			       sizeof(u64)) + sizeof(u64)];
+			struct {
+				s32 val[AD4030_MAX_DIFF_CHANNEL_NB];
+				u8 common[AD4030_MAX_COMMON_CHANNEL_NB];
+			};
+		};
+	} rx_data __aligned(IIO_DMA_MINALIGN);
+};
+
+#define AD4030_CHAN_CMO(_idx)  {					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.channel = _idx,						\
+	.scan_index = _idx,						\
+	.scan_type = {							\
+		.sign = 'u',						\
+		.storagebits = 8,					\
+		.realbits = 8,						\
+		.endianness = IIO_BE,					\
+	},								\
+}
+
+#define AD4030_CHAN_IN(_idx, _storage, _real, _shift) {			\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
+		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
+		BIT(IIO_CHAN_INFO_RAW),					\
+	.info_mask_separate_available = BIT(IIO_CHAN_INFO_CALIBBIAS) |	\
+		BIT(IIO_CHAN_INFO_CALIBSCALE),				\
+	.type = IIO_VOLTAGE,						\
+	.indexed = 1,							\
+	.channel = _idx,						\
+	.scan_index = _idx,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.storagebits = _storage,				\
+		.realbits = _real,					\
+		.shift = _shift,					\
+		.endianness = IIO_BE,					\
+	},								\
+}
+
+static int ad4030_spi_read(void *context, const void *reg, size_t reg_size,
+			   void *val, size_t val_size)
+{
+	struct ad4030_state *st = context;
+
+	struct spi_transfer xfer = {
+		.tx_buf = st->tx_data,
+		.rx_buf = st->rx_data.raw,
+		.len = reg_size + val_size,
+	};
+	int ret;
+
+	memcpy(st->tx_data, reg, reg_size);
+
+	ret = spi_sync_transfer(st->spi, &xfer, 1);
+	if (ret)
+		return ret;
+
+	memcpy(val, &st->rx_data.raw[reg_size], val_size);
+
+	return ret;
+}
+
+static int ad4030_spi_write(void *context, const void *data, size_t count)
+{
+	const struct ad4030_state *st = context;
+
+	return spi_write(st->spi, data, count);
+}
+
+static const struct regmap_bus ad4030_regmap_bus = {
+	.read = ad4030_spi_read,
+	.write = ad4030_spi_write,
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+};
+
+static const struct regmap_range ad4030_regmap_rd_range[] = {
+	regmap_reg_range(AD4030_REG_INTERFACE_CONFIG_A, AD4030_REG_CHIP_GRADE),
+	regmap_reg_range(AD4030_REG_SCRATCH_PAD, AD4030_REG_STREAM_MODE),
+	regmap_reg_range(AD4030_REG_INTERFACE_CONFIG_C, AD4030_REG_INTERFACE_STATUS_A),
+	regmap_reg_range(AD4030_REG_EXIT_CFG_MODE, AD4030_REG_PAT3),
+	regmap_reg_range(AD4030_REG_DIG_DIAG, AD4030_REG_DIG_ERR),
+};
+
+static const struct regmap_range ad4030_regmap_wr_range[] = {
+	regmap_reg_range(AD4030_REG_CHIP_TYPE, AD4030_REG_CHIP_GRADE),
+	regmap_reg_range(AD4030_REG_SPI_REVISION, AD4030_REG_VENDOR_H),
+};
+
+static const struct regmap_access_table ad4030_regmap_rd_table = {
+	.yes_ranges = ad4030_regmap_rd_range,
+	.n_yes_ranges = ARRAY_SIZE(ad4030_regmap_rd_range),
+};
+
+static const struct regmap_access_table ad4030_regmap_wr_table = {
+	.no_ranges = ad4030_regmap_wr_range,
+	.n_no_ranges = ARRAY_SIZE(ad4030_regmap_wr_range),
+};
+
+static const struct regmap_config ad4030_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.read_flag_mask = 0x80,
+	.rd_table = &ad4030_regmap_rd_table,
+	.wr_table = &ad4030_regmap_wr_table,
+	.max_register = AD4030_REG_DIG_ERR,
+};
+
+static int ad4030_enter_config_mode(struct ad4030_state *st)
+{
+	st->rx_data.raw[0] = AD4030_REG_ACCESS;
+
+	return spi_write(st->spi, st->rx_data.raw, 1);
+}
+
+static int ad4030_exit_config_mode(struct ad4030_state *st)
+{
+	return regmap_write(st->regmap, AD4030_REG_EXIT_CFG_MODE,
+		     AD4030_REG_EXIT_CFG_MODE_MASK_EXIT_CONFIG_MD);
+}
+
+static int ad4030_get_chan_gain(struct iio_dev *indio_dev, int ch, int *val,
+				int *val2)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	u16 gain;
+	int ret;
+
+	ret = regmap_bulk_read(st->regmap, AD4030_REG_GAIN_CHAN(ch), &gain,
+			       AD4030_REG_GAIN_BYTES_NB);
+	if (ret)
+		return ret;
+
+	gain = be16_to_cpu(gain);
+
+	/* From datasheet: multiplied output = input × gain word/0x8000 */
+	*val = gain / 0x8000;
+	*val2 = mul_u64_u32_div(gain % 0x8000, MICRO, 0x8000);
+
+	return 0;
+}
+
+/*
+ * @brief Returns the offset where 1 LSB = (VREF/2^precision_bits - 1)/gain
+ */
+static int ad4030_get_chan_offset(struct iio_dev *indio_dev, int ch, int *val)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	__be32 offset;
+	int ret;
+
+	ret = regmap_bulk_read(st->regmap, AD4030_REG_OFFSET_CHAN(ch), &offset,
+			       AD4030_REG_OFFSET_BYTES_NB);
+	if (ret)
+		return ret;
+
+	if (st->chip->precision_bits == 16)
+		offset <<= 16;
+	else
+		offset <<= 8;
+
+	*val = be32_to_cpu(offset);
+
+	return 0;
+}
+
+static int ad4030_set_chan_gain(struct iio_dev *indio_dev, int ch, int gain_int,
+				int gain_frac)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	__be16 val;
+	u64 gain;
+
+	gain = mul_u32_u32(gain_int, MICRO) + gain_frac;
+
+	if (gain > AD4030_REG_GAIN_MAX_GAIN)
+		return -EINVAL;
+
+	val = cpu_to_be16(DIV_ROUND_CLOSEST_ULL(gain * 0x8000, MICRO));
+
+	return regmap_bulk_write(st->regmap, AD4030_REG_GAIN_CHAN(ch), &val,
+			  AD4030_REG_GAIN_BYTES_NB);
+}
+
+static int ad4030_set_chan_offset(struct iio_dev *indio_dev, int ch, int offset)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	__be32 val;
+
+	if (offset < st->min_offset || offset > st->max_offset)
+		return -EINVAL;
+
+	val = cpu_to_be32(offset);
+	if (st->chip->precision_bits == 16)
+		val >>= 16;
+	else
+		val >>= 8;
+
+	return regmap_bulk_write(st->regmap, AD4030_REG_OFFSET_CHAN(ch), &val, 3);
+}
+
+static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
+					unsigned int mask)
+{
+	return mask & BIT(st->chip->num_channels);
+}
+
+static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+
+	if (ad4030_is_common_byte_asked(st, mask))
+		st->mode = st->chip->precision_bits == 24 ?
+			AD4030_OUT_DATA_MD_24_DIFF_8_COM :
+			AD4030_OUT_DATA_MD_16_DIFF_8_COM;
+	else
+		st->mode = AD4030_OUT_DATA_MD_24_DIFF;
+
+	return regmap_update_bits(st->regmap, AD4030_REG_MODES,
+				AD4030_REG_MODES_MASK_OUT_DATA_MODE,
+				st->mode);
+}
+
+static int ad4030_conversion(struct ad4030_state *st, const struct iio_chan_spec *chan)
+{
+	unsigned int bytes_to_read = (BITS_TO_BYTES(chan->scan_type.realbits) +
+			     ((st->mode == AD4030_OUT_DATA_MD_24_DIFF_8_COM ||
+			     st->mode == AD4030_OUT_DATA_MD_16_DIFF_8_COM) ? 1 : 0)) *
+			     st->chip->num_channels;
+	struct spi_transfer xfer = {
+		.rx_buf = st->rx_data.raw,
+		.len = bytes_to_read,
+	};
+	unsigned char byte_index;
+	unsigned int i;
+	int ret;
+
+	gpiod_set_value_cansleep(st->cnv_gpio, 1);
+	ndelay(AD4030_TCNVH_NS);
+	gpiod_set_value_cansleep(st->cnv_gpio, 0);
+	ndelay(AD4030_TCNVL_NS + AD4030_TCONV_NS);
+
+	ret = spi_sync_transfer(st->spi, &xfer, 1);
+	if (ret || (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
+		    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM))
+		return ret;
+
+	byte_index = st->chip->precision_bits == 16 ? 3 : 4;
+	for (i = 0; i < st->chip->num_channels; i++)
+		st->rx_data.common[i] = ((u8 *)&st->rx_data.val[i])[byte_index];
+
+	return 0;
+}
+
+static int ad4030_single_conversion(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan, int *val)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4030_set_mode(indio_dev, BIT(chan->channel));
+	if (ret)
+		return ret;
+
+	ret = ad4030_exit_config_mode(st);
+	if (ret)
+		goto out_error;
+
+	ret = ad4030_conversion(st, chan);
+	if (ret)
+		goto out_error;
+
+	if (chan->channel < st->chip->num_channels)
+		*val = st->rx_data.val[chan->channel];
+	else
+		*val = st->rx_data.common[chan->channel - st->chip->num_channels];
+
+out_error:
+	ad4030_enter_config_mode(st);
+
+	return IIO_VAL_INT;
+}
+
+static irqreturn_t ad4030_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct ad4030_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4030_conversion(st, indio_dev->channels);
+	if (ret)
+		goto out;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, st->rx_data.raw,
+					   pf->timestamp);
+
+out:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static const int *ad4030_get_offset_avail(struct ad4030_state *st)
+{
+	return st->offset_avail;
+}
+
+static const int ad4030_gain_avail[3][2] = {
+	{0, 0},
+	{0, 30},
+	{1, 999969},
+};
+
+static int ad4030_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *channel,
+			     const int **vals, int *type,
+			     int *length, long mask)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBBIAS:
+		*vals = ad4030_get_offset_avail(st);
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_RANGE;
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		*vals = (void *)ad4030_gain_avail;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_RANGE;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad4030_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long info)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	int ret;
+
+	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+		switch (info) {
+		case IIO_CHAN_INFO_RAW:
+			return ad4030_single_conversion(indio_dev, chan, val);
+
+		case IIO_CHAN_INFO_SCALE:
+			*val = (st->vref_uv * 2) / MILLI;
+			*val2 = st->chip->precision_bits;
+			return IIO_VAL_FRACTIONAL_LOG2;
+
+		case IIO_CHAN_INFO_CALIBSCALE:
+			ret = ad4030_get_chan_gain(indio_dev, chan->channel,
+						   val, val2);
+			if (ret)
+				return ret;
+			return IIO_VAL_INT_PLUS_MICRO;
+
+		case IIO_CHAN_INFO_CALIBBIAS:
+			ret = ad4030_get_chan_offset(indio_dev, chan->channel,
+						     val);
+			if (ret)
+				return ret;
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
+	}
+
+	unreachable();
+}
+
+static int ad4030_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val,
+			    int val2, long info)
+{
+	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+		switch (info) {
+		case IIO_CHAN_INFO_CALIBSCALE:
+			return ad4030_set_chan_gain(indio_dev, chan->channel,
+						    val, val2);
+
+		case IIO_CHAN_INFO_CALIBBIAS:
+			return ad4030_set_chan_offset(indio_dev, chan->channel,
+						      val);
+
+		default:
+			return -EINVAL;
+		}
+	}
+
+	unreachable();
+}
+
+static int ad4030_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+			     unsigned int writeval, unsigned int *readval)
+{
+	const struct ad4030_state *st = iio_priv(indio_dev);
+
+	iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+		if (readval)
+			return regmap_read(st->regmap, reg, readval);
+
+		return regmap_write(st->regmap, reg, writeval);
+	}
+
+	unreachable();
+}
+
+static const struct iio_info ad4030_iio_info = {
+	.read_avail = ad4030_read_avail,
+	.read_raw = ad4030_read_raw,
+	.write_raw = ad4030_write_raw,
+	.debugfs_reg_access = ad4030_reg_access,
+};
+
+static int ad4030_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+	int ret;
+
+	ret = ad4030_set_mode(indio_dev, *indio_dev->active_scan_mask);
+	if (ret)
+		return ret;
+
+	ret = ad4030_exit_config_mode(st);
+	if (ret)
+		return ret;
+
+	/* Restore SPI max speed for conversion */
+	st->spi->max_speed_hz = st->conversion_speed_hz;
+
+	return 0;
+}
+
+static int ad4030_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct ad4030_state *st = iio_priv(indio_dev);
+
+	/* Make sure the SPI clock is within range to read register */
+	if (st->spi->max_speed_hz > AD4030_SPI_MAX_REG_XFER_SPEED)
+		st->spi->max_speed_hz = AD4030_SPI_MAX_REG_XFER_SPEED;
+
+	return ad4030_enter_config_mode(st);
+}
+
+static const struct iio_buffer_setup_ops ad4030_buffer_setup_ops = {
+	.preenable = ad4030_buffer_preenable,
+	.postdisable = ad4030_buffer_postdisable,
+};
+
+static int ad4030_regulators_get(struct ad4030_state *st)
+{
+	struct device *dev = &st->spi->dev;
+	static const char * const ids[] = {"vdd-5v", "vdd-1v8"};
+	int ret;
+
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ids), ids);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
+
+	st->vio_uv = devm_regulator_get_enable_read_voltage(dev, "vio");
+	if (st->vio_uv < 0)
+		return dev_err_probe(dev, st->vio_uv,
+				     "Failed to enable and read vio voltage");
+
+	st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "ref");
+	if (st->vref_uv < 0) {
+		if (st->vref_uv != -ENODEV)
+			return dev_err_probe(dev, st->vref_uv,
+					     "Failed to read vref voltage");
+
+		/* if not using optional REF, the internal REFIN must be used */
+		st->vref_uv = devm_regulator_get_enable_read_voltage(dev, "refin");
+		if (st->vref_uv < 0)
+			return dev_err_probe(dev, st->vref_uv,
+					     "Failed to read vrefin voltage");
+	}
+
+	if (st->vref_uv < AD4030_VREF_MIN_UV || st->vref_uv > AD4030_VREF_MAX_UV)
+		return dev_err_probe(dev, -EINVAL,
+				     "vref(%d) must be in the range [%lu %lu]\n",
+				     st->vref_uv, AD4030_VREF_MIN_UV,
+				     AD4030_VREF_MAX_UV);
+
+	return 0;
+}
+
+static int ad4030_reset(struct ad4030_state *st)
+{
+	struct device *dev = &st->spi->dev;
+	struct gpio_desc *reset;
+	int ret;
+
+	/* Use GPIO if available ... */
+	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(reset))
+		return dev_err_probe(dev, PTR_ERR(reset),
+				"Failed to get reset GPIO\n");
+
+	if (reset) {
+		ndelay(50);
+		gpiod_set_value_cansleep(reset, 0);
+		goto reset_end;
+	}
+
+	/* ... falback to software reset otherwise */
+	ret = ad4030_enter_config_mode(st);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD4030_REG_INTERFACE_CONFIG_A,
+			   AD4030_REG_INTERFACE_CONFIG_A_SW_RESET);
+	if (ret)
+		return ret;
+
+reset_end:
+	/* Wait for reset to complete before communicating to it */
+	fsleep(AD4030_TRESET_COM_DELAY_MS);
+
+	/* After reset, conversion mode is enabled. Switch to reg access */
+	return ad4030_enter_config_mode(st);
+}
+
+static int ad4030_detect_chip_info(const struct ad4030_state *st)
+{
+	unsigned int grade;
+	int ret;
+
+	ret = regmap_read(st->regmap, AD4030_REG_CHIP_GRADE, &grade);
+	if (ret)
+		return ret;
+
+	grade = FIELD_GET(AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE, grade);
+	if (grade != st->chip->grade)
+		return dev_err_probe(&st->spi->dev, -EINVAL,
+				"Unknown grade(%u) for %s\n", grade,
+				st->chip->name);
+
+	return 0;
+}
+
+static int ad4030_config(struct ad4030_state *st)
+{
+	st->min_offset = (int)BIT(st->chip->precision_bits) * -1;
+	st->max_offset = BIT(st->chip->precision_bits) - 1;
+	st->offset_avail[0] = st->min_offset;
+	st->offset_avail[1] = 1;
+	st->offset_avail[2] = st->max_offset;
+
+	if (st->vio_uv < AD4030_VIO_THRESHOLD_UV)
+		return regmap_write(st->regmap, AD4030_REG_IO,
+				AD4030_REG_IO_MASK_IO2X);
+
+	return 0;
+}
+
+static int ad4030_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct iio_dev *indio_dev;
+	struct ad4030_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+
+	/* Make sure the SPI clock is within range to read register */
+	st->conversion_speed_hz = spi->max_speed_hz;
+	if (spi->max_speed_hz > AD4030_SPI_MAX_REG_XFER_SPEED)
+		spi->max_speed_hz = AD4030_SPI_MAX_REG_XFER_SPEED;
+
+	st->regmap = devm_regmap_init(&spi->dev, &ad4030_regmap_bus, st,
+				      &ad4030_regmap_config);
+	if (IS_ERR(st->regmap))
+		dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
+			      "Failed to initialize regmap\n");
+
+	st->chip = spi_get_device_match_data(spi);
+	if (!st->chip)
+		return -EINVAL;
+
+	ret = ad4030_regulators_get(st);
+	if (ret)
+		return ret;
+
+	ret = ad4030_reset(st);
+	if (ret)
+		return ret;
+
+	ret = ad4030_detect_chip_info(st);
+	if (ret)
+		return ret;
+
+	ret = ad4030_config(st);
+	if (ret)
+		return ret;
+
+	st->cnv_gpio = devm_gpiod_get(dev, "cnv", GPIOD_OUT_LOW);
+	if (IS_ERR(st->cnv_gpio))
+		return dev_err_probe(dev, PTR_ERR(st->cnv_gpio),
+				     "Failed to get cnv gpio");
+
+	indio_dev->name = st->chip->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ad4030_iio_info;
+	indio_dev->channels = st->chip->channels;
+	indio_dev->num_channels =  2 * st->chip->num_channels + 1;
+	indio_dev->available_scan_masks = st->chip->available_masks;
+	indio_dev->masklength = st->chip->available_masks_len;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      ad4030_trigger_handler,
+					      &ad4030_buffer_setup_ops);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const unsigned long ad4030_channel_masks[] = {
+	BIT(0),
+	GENMASK(1, 0),
+	0,
+};
+
+static const struct ad4030_chip_info ad4030_24_chip_info = {
+	.name = "ad4030-24",
+	.available_masks = ad4030_channel_masks,
+	.available_masks_len = ARRAY_SIZE(ad4030_channel_masks),
+	.channels = {
+		AD4030_CHAN_IN(0, 32, 24, 8),
+		AD4030_CHAN_CMO(1),
+		IIO_CHAN_SOFT_TIMESTAMP(2),
+	},
+	.grade = AD4030_REG_CHIP_GRADE_AD4030_24_GRADE,
+	.precision_bits = 24,
+	.num_channels = 1,
+};
+
+static const struct spi_device_id ad4030_id_table[] = {
+	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
+	{}
+};
+MODULE_DEVICE_TABLE(spi, ad4030_id_table);
+
+static const struct of_device_id ad4030_of_match[] = {
+	{ .compatible = "adi,ad4030-24", .data = &ad4030_24_chip_info },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ad4030_of_match);
+
+static struct spi_driver ad4030_driver = {
+	.driver = {
+		.name = "ad4030",
+		.of_match_table = ad4030_of_match,
+	},
+	.probe = ad4030_probe,
+	.id_table = ad4030_id_table,
+};
+module_spi_driver(ad4030_driver);
+
+MODULE_AUTHOR("Esteban Blanc <[e-mail]>");
+MODULE_DESCRIPTION("Analog Devices AD4630 ADC family driver");
+MODULE_LICENSE("GPL");