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: <D452E2M75XCM.13OQGAPJ7JJ4A@baylibre.com>
Date: Fri, 13 Sep 2024 09:55:28 +0000
From: "Esteban Blanc" <eblanc@...libre.com>
To: "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 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

> > +static const unsigned long ad4630_channel_masks[] = {
> > +	/* Differential only */
> > +	BIT(0) | BIT(2),
> > +	/* Differential with common byte */
> > +	GENMASK(3, 0),
> The packing of data isn't going to be good. How bad to shuffle
> to put the two small channels next to each other?
> Seems like it means you will want to combine your deinterleave
> and channel specific handling above, which is a bit fiddly but
> not much worse than current code.

I can do it since that was what I had done in the RFC in the first place.
Nuno asked for in this email https://lore.kernel.org/r/0036d44542f8cf45c91c867f0ddd7b45d1904d6b.camel@gmail.com/:

>>> * You're pushing the CM channels into the end. So when we a 2 channel device
>>> we'll have:

>>> in_voltage0 - diff
>>> in_voltage1 - diff
>>> in_voltage2 - CM associated with chan0
>>> in_voltage0 - CM associated with chan1
>>>
>>> I think we could make it so the CM channel comes right after the channel where
>>> it's data belongs too. So for example, odd channels would be CM channels (and
>>> labels could also make sense).

So that's what I did here :D

For the software side off things here it doesn't change a lot of things
since we have to manipulate the data anyway, putting the extra byte at the
end or in between is no extra work.
For the offload engine however, it should be easier to ask for 24 bits
then 8 bits for each channel as it would return two u32 per "hardware
channel".

In order to avoid having two different layouts, I was kind of sold by
Nuno's idea of having the CM in between each diff channel.

Thanks for your time,

-- 
Esteban Blanc
BayLibre