linux-kernel - Re: [PATCH 4/8] iio: adc: ad7476: Use correct channel for bit info

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: <b1023916-23bb-455f-816a-1ca9412a6e32@gmail.com>
Date: Thu, 7 Aug 2025 09:46:26 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>,
 Nuno Sá <nuno.sa@...log.com>,
 Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Liam Girdwood <lgirdwood@...il.com>,
 Mark Brown <broonie@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/8] iio: adc: ad7476: Use correct channel for bit info

On 06/08/2025 18:04, Jonathan Cameron wrote:
> On Wed, 6 Aug 2025 10:03:43 +0300
> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> 
>> The ad7476 supports ADCs which use separate GPIO for starting the
>> conversion. For such devices, the driver uses different channel
>> information if the GPIO is found. The bit information is still always
>> used from the original (non 'convstart') channels.
>>
>> This has not been causing problems because the bit information for the
>> 'convstart' -channel and the 'normal' -channel is identical. It,
>> however, will cause issues if an IC has different characteristics for an
>> 'convstart' -channel and regular channel. Furthermore, this will cause
>> problems if a device always requires the convstart GPIO and thus only
>> defines the convstart channel.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
>> ---
>> It appears that the _only_ difference between the 'convstart' -channel
>> and the 'normal' channel is a lack of the 'raw-read' support. I might
>> prefer seeing the _same_ channel information being used for 'convstart'
>> and 'normal' channels, just setting the IIO_CHAN_INFO_RAW -bit when the
>> CONVSTART GPIO is found. This would allow getting rid of the 'convstart'
>> -channel spec altogeher. Having only one channel info spec would also
>> help the code-reader to understand that the driver really provides only
>> one data channel to the users. Currently a quick reader may assume the
>> driver for some reason provides both the 'convstart' and the 'normal'
>> channels.
>>
>> Adding the IIO_CHAN_INFO_RAW when CONVSTART GPIO is obtained would
>> however require the channel information structs to be mutable - which may
>> be seen as a "no, no" by some. Hence this minimally intrusive patch.
> If you duplicate them before updating that is probably fine, just keep the
> ones in the chip info static const.

This will mean allocating a new channel spec for each instance of this 
driver. Tradeoff seems to be clarity Vs memory consumption then. Well, I 
suppose systems don't have that many of these ADCs, right?

Well, I'll do this if no-one objects then.

>> ---
>>   drivers/iio/adc/ad7476.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7476.c b/drivers/iio/adc/ad7476.c
>> index 7b6d36999afc..fc701267358e 100644
>> --- a/drivers/iio/adc/ad7476.c
>> +++ b/drivers/iio/adc/ad7476.c
>> @@ -121,8 +121,8 @@ static int ad7476_read_raw(struct iio_dev *indio_dev,
>>   
>>   		if (ret < 0)
>>   			return ret;
>> -		*val = (ret >> st->chip_info->channel[0].scan_type.shift) &
>> -			GENMASK(st->chip_info->channel[0].scan_type.realbits - 1, 0);
>> +		*val = (ret >> chan->scan_type.shift) &
>> +			GENMASK(chan->scan_type.realbits - 1, 0);
>>   		return IIO_VAL_INT;
>>   	case IIO_CHAN_INFO_SCALE:
>>   		*val = st->scale_mv;
>> @@ -345,7 +345,7 @@ static int ad7476_probe(struct spi_device *spi)
>>   	/* Setup default message */
>>   
>>   	st->xfer.rx_buf = &st->data;
>> -	st->xfer.len = st->chip_info->channel[0].scan_type.storagebits / 8;
>> +	st->xfer.len = indio_dev->channels[0].scan_type.storagebits / 8;
>>   
>>   	spi_message_init(&st->msg);
>>   	spi_message_add_tail(&st->xfer, &st->msg);
>