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

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

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