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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 27 Jun 2024 13:59:15 +0200
Esteban Blanc <eblanc@baylibre.com> 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 | 130 +++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 126 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4030.c b/drivers/iio/adc/ad4030.c
> index 1bcbcbd40a45..09d2f6d8cfe6 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
> @@ -391,7 +393,10 @@ 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)
>  {
> -	return mask & BIT(st->chip->num_channels);
> +	if (st->chip->num_channels == 1)
> +		return mask & BIT(st->chip->num_channels);
> +
> +	return mask & GENMASK(st->chip->num_channels + 1, st->chip->num_channels);
>  }
>  
>  static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
> @@ -412,6 +417,45 @@ static int ad4030_set_mode(struct iio_dev *indio_dev, unsigned long mask)
>  				st->mode);
>  }
>  
> +/*
> + * @brief Descramble 2 32bits numbers out of a 64bits. The bits are interleaved: 1 bit for first
line wrap at 80 chars unless good reason to be longer.
> + * number, 1 bit for the second, and so on...

Do you have a reference for the alg used? 
Google fed me a bunch of options for a perfect unshuffle 
though it is probably microarch dependent.


> + */
> +static void ad4030_extract_interleaved(u8 *src, u32 *out)
> +{
> +	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;
> +	}
> +
> +	out[0] = out0;
> +	out[1] = out1;
> +}
> +
>  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) +
> @@ -437,10 +481,16 @@ static int ad4030_conversion(struct ad4030_state *st, const struct iio_chan_spec
>  	ndelay(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))
> +	if (ret)
>  		return ret;
>  
> +	if (st->chip->num_channels == 2)
> +		ad4030_extract_interleaved(st->rx_data.raw, st->rx_data.val);
> +
> +	if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
> +	    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM)
> +		return 0;

If you make the suggested split of the error and good paths from patch 2 review
this will be a simpler diff.

> +
>  	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];
> @@ -776,12 +826,25 @@ static int ad4030_detect_chip_info(const struct ad4030_state *st)
>  
>  static int ad4030_config(struct ad4030_state *st)
>  {
> +	int ret;
> +	u8 reg_modes = 0;
Seems you can just use = below instead of |=

> +
>  	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->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);

wrap that line.  Aim for sub 80 chars.

> +
> +	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);
> @@ -863,8 +926,14 @@ static const unsigned long ad4030_channel_masks[] = {
>  	0,
>  };
>  

Thanks,

Jonathan