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).
