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

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 06/08/2025 18:04, Jonathan Cameron wrote:
> On Wed, 6 Aug 2025 10:03:43 +0300
> Matti Vaittinen <mazziesaccount@gmail.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@gmail.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);
>