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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D47Q9E5EW08V.2JP0X6EFQMFBT@baylibre.com>
Date: Mon, 16 Sep 2024 13:03:08 +0000
From: "Esteban Blanc" <eblanc@...libre.com>
To: "Esteban Blanc" <eblanc@...libre.com>, "Jonathan Cameron"
 <jic23@...nel.org>
Cc: "Lars-Peter Clausen" <lars@...afoo.de>, "Michael Hennerich"
 <Michael.Hennerich@...log.com>, "Rob Herring" <robh@...nel.org>, "Krzysztof
 Kozlowski" <krzk+dt@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>,
 "Nuno Sa" <nuno.sa@...log.com>, "Jonathan Corbet" <corbet@....net>,
 <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, "David Lechner" <dlechner@...libre.com>,
 <linux-doc@...r.kernel.org>
Subject: Re: [PATCH 4/6] iio: adc: ad4030: add support for ad4630-24 and
 ad4630-16

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@...libre.com> 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).