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