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

[Esteban Blanc]

On Fri Sep 13, 2024 at 9:55 AM UTC, Esteban Blanc wrote:
> On Mon Aug 26, 2024 at 9:27 AM UTC, Jonathan Cameron wrote:
> > On Thu, 22 Aug 2024 14:45:20 +0200
> > Esteban Blanc <eblanc@xxxxxxxxxxxx> wrote:
> > > @@ -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];
> > > +	}
> >
> > I wonder if doing it in place is actually worthwhile.  Maybe unpack into a second
> > array? That is still fairly small and may make code easier to read.
>
> Okay sure

Actually I can't consolidate the differential only mode and the common
byte mode without having to create a bunch of if/else or having a
memcpy. The best I can do is this, but I don't like it:

```
static int ad4030_conversion(struct ad4030_state *st,
			     const struct iio_chan_spec *chan)
{
	...
	u32 tmp[AD4030_MAX_HARDWARE_CHANNEL_NB];
	u32 *diff;

	...

	if (st->mode != AD4030_OUT_DATA_MD_16_DIFF_8_COM &&
	    st->mode != AD4030_OUT_DATA_MD_24_DIFF_8_COM) {
		if (st->chip->num_voltage_inputs == 2)
			ad4030_extract_interleaved(st->rx_data.raw,
						   &st->rx_data.diff[0],
						   &st->rx_data.diff[1]);
		return 0;
	}

	if (st->chip->num_voltage_inputs == 2) {
		ad4030_extract_interleaved(st->rx_data.raw,
					   &tmp[0],
					   &tmp[1]);
		diff = tmp;
	} else {
		diff = st->rx_data.diff;
	}

	common_byte_mask = BITS_TO_BYTES(chan->scan_type.realbits);
	for (i = 0; i < st->chip->num_voltage_inputs; i++) {
		st->rx_data.buffered[i].val = diff[i];
		st->rx_data.buffered[i].common =
			((u8 *)(diff + i))[common_byte_mask];
	}

	return 0;
```

The root cause is that when we are in a differential only mode we are
leaving before the for loop and we want the data to be in the rx_data.

It fells clunky and brain consuming IMAO, I prefer a reversed loop with
a good comment (the one we have now is not explicit enough, I will
update it).