Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and ad4630-16

[David Lechner <dlechner@baylibre.com>]

On 8/22/24 7:45 AM, Esteban Blanc wrote:
> AD4630-24 and AD4630-16 are 2 channels ADCs. Both channels are
> interleaved bit per bit on SDO line.
> 
> Signed-off-by: Esteban Blanc <eblanc@baylibre.com>
> ---
>  drivers/iio/adc/ad4030.c | 197 +++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 173 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> index e1e1dbf0565c..dbba5287b630 100644
> --- a/drivers/iio/adc/ad4030.c
> +++ b/drivers/iio/adc/ad4030.c
> @@ -32,6 +32,8 @@
>  #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_AD4630_16_GRADE	0x03
> +#define     AD4030_REG_CHIP_GRADE_AD4630_24_GRADE	0x00
>  #define     AD4030_REG_CHIP_GRADE_MASK_CHIP_GRADE	GENMASK(7, 3)
>  #define AD4030_REG_SCRATCH_PAD			0x0A
>  #define AD4030_REG_SPI_REVISION			0x0B
> @@ -159,10 +161,14 @@ struct ad4030_state {
>  	struct {
>  		union {
>  			u8 raw[AD4030_MAXIMUM_RX_BUFFER_SIZE];
> -			struct {
> -				s32 val;
> -				u32 common;
> -			} __packed buffered[AD4030_MAX_HARDWARE_CHANNEL_NB];
> +			union {
> +				s32 diff[AD4030_MAX_HARDWARE_CHANNEL_NB];
> +				struct {
> +					s32 diff;
> +					u32 common;
> +				} __packed
> +				buffered_common[AD4030_MAX_HARDWARE_CHANNEL_NB];
> +			};
>  		};
>  	} rx_data __aligned(IIO_DMA_MINALIGN);
>  };
> @@ -171,7 +177,7 @@ struct ad4030_state {
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
>  	.type = IIO_VOLTAGE,						\
>  	.indexed = 1,							\
> -	.channel = _idx * 2 + 2,					\
> +	.channel = _idx * 3 + 2,					\

I'm guessing * 3 should have actually been in the first patch?
Seems odd to change it now.

>  	.scan_index = _idx * 2 + 1,					\
>  	.extend_name = "Channel" #_idx " common byte part",		\
>  	.scan_type = {							\
> @@ -194,8 +200,8 @@ struct ad4030_state {
>  		BIT(IIO_CHAN_INFO_CALIBSCALE),				\
>  	.type = IIO_VOLTAGE,						\
>  	.indexed = 1,							\
> -	.channel = _idx * 2,						\
> -	.channel2 = _idx * 2 + 1,					\
> +	.channel = _idx * 3,						\
> +	.channel2 = _idx * 3 + 1,					\
>  	.scan_index = _idx * 2,						\
>  	.extend_name = "Channel" #_idx " differential part",		\
>  	.differential = true,						\
> @@ -412,7 +418,7 @@ static int ad4030_set_avg_frame_len(struct iio_dev *dev, unsigned int avg_len)
>  static bool ad4030_is_common_byte_asked(struct ad4030_state *st,
>  					unsigned int mask)
>  {
> -	/* Common byte channel is after the "real" differential sample channel */
> +	/* Common byte channels are after each differential channel */
>  	return mask & AD4030_COMMON_BYTE_CHANNELS_FILTER;
>  }
>  
> @@ -420,18 +426,69 @@ static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
>  {
>  	struct ad4030_state *st = iio_priv(indio_dev);
>  
> -	if (st->avg_len)
> +	if (st->avg_len) {
>  		st->mode = AD4030_OUT_DATA_MD_30_AVERAGED_DIFF;
> -	else if (ad4030_is_common_byte_asked(st, mask))
> -		st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
> -	else
> +	} else if (ad4030_is_common_byte_asked(st, mask)) {
> +		switch (st->chip->precision_bits) {
> +		case 16:
> +			st->mode = AD4030_OUT_DATA_MD_16_DIFF_8_COM;
> +			break;
> +
> +		case 24:
> +			st->mode = AD4030_OUT_DATA_MD_24_DIFF_8_COM;
> +			break;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +	} else {
>  		st->mode = AD4030_OUT_DATA_MD_24_DIFF;

nit: maybe better to rename this one to AD4030_OUT_DATA_MD_DIFF
or AD4030_OUT_DATA_MD_DIFF_ONLY since it can be 16 or 24 bits
depending on the chip?

> +	}
>  
>  	return regmap_update_bits(st->regmap, AD4030_REG_MODES,
>  				AD4030_REG_MODES_MASK_OUT_DATA_MODE,
>  				st->mode);
>  }
>  
> +/*
> + * @brief Descramble 2 32bits numbers out of a 64bits. The bits are interleaved:
> + * 1 bit for first number, 1 bit for the second, and so on...
> + */
> +static void ad4030_extract_interleaved(u8 *src, u32 *ch0, u32 *ch1)
> +{
> +	u8 h0, h1, l0, l1;
> +	u32 out0, out1;
> +	u8 *out0_raw = (u8 *)&out0;
> +	u8 *out1_raw = (u8 *)&out1;
> +
> +	for (int i = 0; i < 4; i++) {
> +		h0 = src[i * 2];
> +		l1 = src[i * 2 + 1];
> +		h1 = h0 << 1;
> +		l0 = l1 >> 1;
> +
> +		h0 &= 0xAA;
> +		l0 &= 0x55;
> +		h1 &= 0xAA;
> +		l1 &= 0x55;
> +
> +		h0 = (h0 | h0 << 001) & 0xCC;
> +		h1 = (h1 | h1 << 001) & 0xCC;
> +		l0 = (l0 | l0 >> 001) & 0x33;
> +		l1 = (l1 | l1 >> 001) & 0x33;
> +		h0 = (h0 | h0 << 002) & 0xF0;
> +		h1 = (h1 | h1 << 002) & 0xF0;
> +		l0 = (l0 | l0 >> 002) & 0x0F;
> +		l1 = (l1 | l1 >> 002) & 0x0F;
> +
> +		out0_raw[i] = h0 | l0;
> +		out1_raw[i] = h1 | l1;
> +	}
> +
> +	*ch0 = out0;
> +	*ch1 = out1;
> +}
> +
>  static int ad4030_conversion(struct ad4030_state *st,
>  			     const struct iio_chan_spec *chan)
>  {
> @@ -460,12 +517,21 @@ static int ad4030_conversion(struct ad4030_state *st,
>  	if (ret)
>  		return ret;
>  
> -	if (st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> +	if (st->chip->num_channels == 2)
> +		ad4030_extract_interleaved(st->rx_data.raw,
> +					   &st->rx_data.diff[0],
> +					   &st->rx_data.diff[1]);
> +
> +	if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> +	    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
>  		return 0;
>  
>  	byte_index = BITS_TO_BYTES(chan->scan_type.realbits);
> -	for (i = 0; i < st->chip->num_channels; i++)
> -		st->rx_data.buffered[i].common = ((u8 *)&st->rx_data.buffered[i].val)[byte_index];
> +	/* Doing it backward to avoid overlap when reordering */
> +	for (i = st->chip->num_channels - 1; i > 0; i--) {
> +		st->rx_data.buffered_common[i].diff = st->rx_data.diff[i];
> +		st->rx_data.buffered_common[i].common = ((u8 *)&st->rx_data.diff[i])[byte_index];
> +	}
>  
>  	return 0;
>  }
> @@ -489,9 +555,9 @@ static int ad4030_single_conversion(struct iio_dev *indio_dev,
>  		goto out_error;
>  
>  	if (chan->channel % 2)
> -		*val = st->rx_data.buffered[chan->channel / 2].common;
> +		*val = st->rx_data.buffered_common[chan->channel / 2].common;
>  	else
> -		*val = st->rx_data.buffered[chan->channel / 2].val;
> +		*val = st->rx_data.diff[chan->channel / 2];

Doesn't this need to change since `* 2` was changed to `* 3` for the channel
value?

Better would be to make use of chan->address to store the actual number we need
and use that directly instead of reverse engineering the relation of chan->channel
to input pin number.

>  
>  out_error:
>  	ad4030_enter_config_mode(st);
> @@ -582,14 +648,17 @@ static int ad4030_read_raw(struct iio_dev *indio_dev,
>  			return IIO_VAL_FRACTIONAL_LOG2;
>  
>  		case IIO_CHAN_INFO_CALIBSCALE:
> -			ret = ad4030_get_chan_gain(indio_dev, chan->channel,
> -						   val, val2);
> +			ret = ad4030_get_chan_gain(indio_dev,
> +						   chan->scan_index / 2,
> +						   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,
> +			ret = ad4030_get_chan_offset(indio_dev,
> +						     chan->scan_index / 2,

Similar to above, we could use chan->address here instead of having to
know the relationship between scan_index and input pin.

>  						     val);
>  			if (ret)
>  				return ret;
> @@ -614,11 +683,14 @@ static int ad4030_write_raw(struct iio_dev *indio_dev,
>  	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);
> +			return ad4030_set_chan_gain(indio_dev,
> +						    chan->scan_index / 2,
> +						    val,
> +						    val2);
>  
>  		case IIO_CHAN_INFO_CALIBBIAS:
> -			return ad4030_set_chan_offset(indio_dev, chan->channel,
> +			return ad4030_set_chan_offset(indio_dev,
> +						      chan->scan_index / 2,
>  						      val);
>  
>  		case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> @@ -801,10 +873,24 @@ static int ad4030_detect_chip_info(const struct ad4030_state *st)
>  
>  static int ad4030_config(struct ad4030_state *st)
>  {
> +	int ret;
> +	u8 reg_modes;
> +
>  	st->offset_avail[0] = (int)BIT(st->chip->precision_bits - 1) * -1;
>  	st->offset_avail[1] = 1;
>  	st->offset_avail[2] = BIT(st->chip->precision_bits - 1) - 1;
>  
> +	if (st->chip->num_channels > 1)
> +		reg_modes = FIELD_PREP(AD4030_REG_MODES_MASK_LANE_MODE,
> +				       AD4030_LANE_MD_INTERLEAVED);
> +	else
> +		reg_modes = FIELD_PREP(AD4030_REG_MODES_MASK_LANE_MODE,
> +				       AD4030_LANE_MD_1_PER_CH);
> +
> +	ret = regmap_write(st->regmap, AD4030_REG_MODES, reg_modes);
> +	if (ret)
> +		return ret;
> +
>  	if (st->vio_uv < AD4030_VIO_THRESHOLD_UV)
>  		return regmap_write(st->regmap, AD4030_REG_IO,
>  				AD4030_REG_IO_MASK_IO2X);
> @@ -891,8 +977,16 @@ static const unsigned long ad4030_channel_masks[] = {
>  	0,
>  };
>  
> +static const unsigned long ad4630_channel_masks[] = {
> +	/* Differential only */
> +	BIT(0) | BIT(2),

nit: for order consistency with GENMASK

	BIT(2) | BIT(0),

> +	/* Differential with common byte */
> +	GENMASK(3, 0),
> +	0,
> +};
> +
>  static const struct iio_scan_type ad4030_24_scan_types[] = {
> -	[AD4030_SCAN_TYPE_NORMAL] = {
> +	[AD4030_OUT_DATA_MD_24_DIFF] = {

Accidental replacement?

>  		.sign = 's',
>  		.storagebits = 32,
>  		.realbits = 24,
> @@ -908,6 +1002,23 @@ static const struct iio_scan_type ad4030_24_scan_types[] = {
>  	},
>  };
>  
> +static const struct iio_scan_type ad4030_16_scan_types[] = {
> +	[AD4030_SCAN_TYPE_NORMAL] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 16,
> +		.shift = 16,
> +		.endianness = IIO_BE,
> +	},
> +	[AD4030_SCAN_TYPE_AVG] = {
> +		.sign = 's',
> +		.storagebits = 32,
> +		.realbits = 30,
> +		.shift = 2,
> +		.endianness = IIO_BE,
> +	}
> +};
> +
>  static const struct ad4030_chip_info ad4030_24_chip_info = {
>  	.name = "ad4030-24",
>  	.available_masks = ad4030_channel_masks,
> @@ -923,14 +1034,52 @@ static const struct ad4030_chip_info ad4030_24_chip_info = {
>  	.tcyc = AD4030_TCYC_ADJUSTED_NS,
>  };
>  
> +static const struct ad4030_chip_info ad4630_16_chip_info = {
> +	.name = "ad4630-16",
> +	.available_masks = ad4630_channel_masks,
> +	.available_masks_len = ARRAY_SIZE(ad4630_channel_masks),
> +	.channels = {
> +		AD4030_CHAN_IN(0, ad4030_16_scan_types),
> +		AD4030_CHAN_CMO(0),
> +		AD4030_CHAN_IN(1, ad4030_16_scan_types),
> +		AD4030_CHAN_CMO(1),
> +		IIO_CHAN_SOFT_TIMESTAMP(4),
> +	},
> +	.grade = AD4030_REG_CHIP_GRADE_AD4630_16_GRADE,
> +	.precision_bits = 16,
> +	.num_channels = 2,
> +	.tcyc = AD4030_TCYC_ADJUSTED_NS,
> +};
> +
> +static const struct ad4030_chip_info ad4630_24_chip_info = {
> +	.name = "ad4630-24",
> +	.available_masks = ad4630_channel_masks,
> +	.available_masks_len = ARRAY_SIZE(ad4630_channel_masks),
> +	.channels = {
> +		AD4030_CHAN_IN(0, ad4030_24_scan_types),
> +		AD4030_CHAN_CMO(0),
> +		AD4030_CHAN_IN(1, ad4030_24_scan_types),
> +		AD4030_CHAN_CMO(1),
> +		IIO_CHAN_SOFT_TIMESTAMP(4),
> +	},
> +	.grade = AD4030_REG_CHIP_GRADE_AD4630_24_GRADE,
> +	.precision_bits = 24,
> +	.num_channels = 2,
> +	.tcyc = AD4030_TCYC_ADJUSTED_NS,
> +};
> +
>  static const struct spi_device_id ad4030_id_table[] = {
>  	{ "ad4030-24", (kernel_ulong_t)&ad4030_24_chip_info },
> +	{ "ad4630-16", (kernel_ulong_t)&ad4630_16_chip_info },
> +	{ "ad4630-24", (kernel_ulong_t)&ad4630_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 },
> +	{ .compatible = "adi,ad4630-16", .data = &ad4630_16_chip_info },
> +	{ .compatible = "adi,ad4630-24", .data = &ad4630_24_chip_info },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, ad4030_of_match);
>