[PATCH] iio: adc: ad7124: fix temperature channel

[PATCH] iio: adc: ad7124: fix temperature channel

[David Lechner]

Fix temperature channel not working due to gain and offset not being
initialized. This was causing the raw temperature readings to be always
8388608 (0x800000).

To fix it, we just make sure the gain and offset values are set to the
default values and still return early without doing an internal
calibration.

While here, add a comment explaining why we don't bother calibrating
the temperature channel.

Fixes: 47036a03a303 ("iio: adc: ad7124: Implement internal calibration at probe time")
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
 drivers/iio/adc/ad7124.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index 374e39736584f55c1290db3e257dff2c60f884d2..94d90a63987c0f9886586db0c4bc1690855be2c1 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -1518,10 +1518,6 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
 	int ret, i;
 
 	for (i = 0; i < st->num_channels; i++) {
-
-		if (indio_dev->channels[i].type != IIO_VOLTAGE)
-			continue;
-
 		/*
 		 * For calibration the OFFSET register should hold its reset default
 		 * value. For the GAIN register there is no such requirement but
@@ -1531,6 +1527,13 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
 		st->channels[i].cfg.calibration_offset = 0x800000;
 		st->channels[i].cfg.calibration_gain = st->gain_default;
 
+		/*
+		 * Only the main voltage input channels are important enough
+		 * to be automatically calibrated here.
+		 */
+		if (indio_dev->channels[i].type != IIO_VOLTAGE)
+			continue;
+
 		/*
 		 * Full-scale calibration isn't supported at gain 1, so skip in
 		 * that case. Note that untypically full-scale calibration has

---
base-commit: 411e8b72c181e4f49352c12ced0fd8426eb683aa
change-id: 20250923-iio-adc-ad7124-fix-temperature-channel-5900f7302886

Best regards,
-- 
David Lechner <dlechner@baylibre.com>

Re: [PATCH] iio: adc: ad7124: fix temperature channel

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2390 bytes --]

Hello David,

On Tue, Sep 23, 2025 at 03:18:02PM -0500, David Lechner wrote:
> Fix temperature channel not working due to gain and offset not being
> initialized. This was causing the raw temperature readings to be always
> 8388608 (0x800000).
> 
> To fix it, we just make sure the gain and offset values are set to the
> default values and still return early without doing an internal
> calibration.
> 
> While here, add a comment explaining why we don't bother calibrating
> the temperature channel.
> 
> Fixes: 47036a03a303 ("iio: adc: ad7124: Implement internal calibration at probe time")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>  drivers/iio/adc/ad7124.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> index 374e39736584f55c1290db3e257dff2c60f884d2..94d90a63987c0f9886586db0c4bc1690855be2c1 100644
> --- a/drivers/iio/adc/ad7124.c
> +++ b/drivers/iio/adc/ad7124.c
> @@ -1518,10 +1518,6 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
>  	int ret, i;
>  
>  	for (i = 0; i < st->num_channels; i++) {
> -
> -		if (indio_dev->channels[i].type != IIO_VOLTAGE)
> -			continue;
> -
>  		/*
>  		 * For calibration the OFFSET register should hold its reset default
>  		 * value. For the GAIN register there is no such requirement but
> @@ -1531,6 +1527,13 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
>  		st->channels[i].cfg.calibration_offset = 0x800000;
>  		st->channels[i].cfg.calibration_gain = st->gain_default;
>  
> +		/*
> +		 * Only the main voltage input channels are important enough
> +		 * to be automatically calibrated here.
> +		 */
> +		if (indio_dev->channels[i].type != IIO_VOLTAGE)
> +			continue;
> +

I don't understand a detail of the problem. The commit log suggests that
there is a general problem, but looking at the patch I suspect there is
only a problem if at probe time the OFFSET and GAIN register for the
temperature channel are different from their reset default setting.

I think the patch is fine, but if my understanding is right the commit
log is more dramatic than the issue really is, as it needs some fiddling
with the ADC's registers between poweron and driver loading to trigger.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] iio: adc: ad7124: fix temperature channel

[David Lechner]

On 9/24/25 6:03 AM, Uwe Kleine-König wrote:
> Hello David,
> 
> On Tue, Sep 23, 2025 at 03:18:02PM -0500, David Lechner wrote:
>> Fix temperature channel not working due to gain and offset not being
>> initialized. This was causing the raw temperature readings to be always
>> 8388608 (0x800000).
>>
>> To fix it, we just make sure the gain and offset values are set to the
>> default values and still return early without doing an internal
>> calibration.
>>
>> While here, add a comment explaining why we don't bother calibrating
>> the temperature channel.
>>
>> Fixes: 47036a03a303 ("iio: adc: ad7124: Implement internal calibration at probe time")
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
>>  drivers/iio/adc/ad7124.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
>> index 374e39736584f55c1290db3e257dff2c60f884d2..94d90a63987c0f9886586db0c4bc1690855be2c1 100644
>> --- a/drivers/iio/adc/ad7124.c
>> +++ b/drivers/iio/adc/ad7124.c
>> @@ -1518,10 +1518,6 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
>>  	int ret, i;
>>  
>>  	for (i = 0; i < st->num_channels; i++) {
>> -
>> -		if (indio_dev->channels[i].type != IIO_VOLTAGE)
>> -			continue;
>> -
>>  		/*
>>  		 * For calibration the OFFSET register should hold its reset default
>>  		 * value. For the GAIN register there is no such requirement but
>> @@ -1531,6 +1527,13 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
>>  		st->channels[i].cfg.calibration_offset = 0x800000;
>>  		st->channels[i].cfg.calibration_gain = st->gain_default;
>>  
>> +		/*
>> +		 * Only the main voltage input channels are important enough
>> +		 * to be automatically calibrated here.
>> +		 */
>> +		if (indio_dev->channels[i].type != IIO_VOLTAGE)
>> +			continue;
>> +
> 
> I don't understand a detail of the problem. The commit log suggests that
> there is a general problem, but looking at the patch I suspect there is
> only a problem if at probe time the OFFSET and GAIN register for the
> temperature channel are different from their reset default setting.

What I failed to mention is that st->channels[i].cfg.calibration_offset
and st->channels[i].cfg.calibration_gain are zero-initialized. And that
these values are later programmed into the ADC. Programming these to
zero is what caused reading the raw value value to always return a fixed
value because the real value got multiplied by 0 in the ADC.

Is that enough to clear it up?

> 
> I think the patch is fine, but if my understanding is right the commit
> log is more dramatic than the issue really is, as it needs some fiddling
> with the ADC's registers between poweron and driver loading to trigger.
> 
> Best regards
> Uwe

Re: [PATCH] iio: adc: ad7124: fix temperature channel

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3381 bytes --]

Hello David,

On Wed, Sep 24, 2025 at 08:17:21AM -0500, David Lechner wrote:
> On 9/24/25 6:03 AM, Uwe Kleine-König wrote:
> > On Tue, Sep 23, 2025 at 03:18:02PM -0500, David Lechner wrote:
> >> Fix temperature channel not working due to gain and offset not being
> >> initialized. This was causing the raw temperature readings to be always
> >> 8388608 (0x800000).
> >>
> >> To fix it, we just make sure the gain and offset values are set to the
> >> default values and still return early without doing an internal
> >> calibration.
> >>
> >> While here, add a comment explaining why we don't bother calibrating
> >> the temperature channel.
> >>
> >> Fixes: 47036a03a303 ("iio: adc: ad7124: Implement internal calibration at probe time")
> >> Signed-off-by: David Lechner <dlechner@baylibre.com>
> >> ---
> >>  drivers/iio/adc/ad7124.c | 11 +++++++----
> >>  1 file changed, 7 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
> >> index 374e39736584f55c1290db3e257dff2c60f884d2..94d90a63987c0f9886586db0c4bc1690855be2c1 100644
> >> --- a/drivers/iio/adc/ad7124.c
> >> +++ b/drivers/iio/adc/ad7124.c
> >> @@ -1518,10 +1518,6 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
> >>  	int ret, i;
> >>  
> >>  	for (i = 0; i < st->num_channels; i++) {
> >> -
> >> -		if (indio_dev->channels[i].type != IIO_VOLTAGE)
> >> -			continue;
> >> -
> >>  		/*
> >>  		 * For calibration the OFFSET register should hold its reset default
> >>  		 * value. For the GAIN register there is no such requirement but
> >> @@ -1531,6 +1527,13 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
> >>  		st->channels[i].cfg.calibration_offset = 0x800000;
> >>  		st->channels[i].cfg.calibration_gain = st->gain_default;
> >>  
> >> +		/*
> >> +		 * Only the main voltage input channels are important enough
> >> +		 * to be automatically calibrated here.
> >> +		 */
> >> +		if (indio_dev->channels[i].type != IIO_VOLTAGE)
> >> +			continue;
> >> +
> > 
> > I don't understand a detail of the problem. The commit log suggests that
> > there is a general problem, but looking at the patch I suspect there is
> > only a problem if at probe time the OFFSET and GAIN register for the
> > temperature channel are different from their reset default setting.
> 
> What I failed to mention is that st->channels[i].cfg.calibration_offset
> and st->channels[i].cfg.calibration_gain are zero-initialized. And that
> these values are later programmed into the ADC. Programming these to
> zero is what caused reading the raw value value to always return a fixed
> value because the real value got multiplied by 0 in the ADC.
> 
> Is that enough to clear it up?

Yes, got the whole picture now. Thanks for explaining.

So maybe:

	For channels other than the voltage ones calibration is skipped (which
	is OK). However that results in the calibration register values
	tracked in st->channels[i].cfg all being zero. These zeros are
	then written to hardware before a measurement and thus resulting
	in temperature readings to be always 8388608 (0x800000). This is
	easily fixed by postponing the channel type check until
	st->channels[i].cfg is initialized with the uncalibrated default
	values.

Best regards
Uwe


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] iio: adc: ad7124: fix temperature channel

[Marcelo Schmitt]

Hi,

On 09/23, David Lechner wrote:
> Fix temperature channel not working due to gain and offset not being
> initialized. This was causing the raw temperature readings to be always
> 8388608 (0x800000).

Would
'Fix temperature channel not working due to gain and offset not being
initialized to their default values.'
be a more accurate description?


> 
> To fix it, we just make sure the gain and offset values are set to the
> default values and still return early without doing an internal
> calibration.
> 
> While here, add a comment explaining why we don't bother calibrating
> the temperature channel.
> 
> Fixes: 47036a03a303 ("iio: adc: ad7124: Implement internal calibration at probe time")
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
...
>  	for (i = 0; i < st->num_channels; i++) {
> -
> -		if (indio_dev->channels[i].type != IIO_VOLTAGE)
> -			continue;
> -
>  		/*
>  		 * For calibration the OFFSET register should hold its reset default
>  		 * value. For the GAIN register there is no such requirement but
> @@ -1531,6 +1527,13 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
>  		st->channels[i].cfg.calibration_offset = 0x800000;
>  		st->channels[i].cfg.calibration_gain = st->gain_default;
>  
> +		/*
> +		 * Only the main voltage input channels are important enough
> +		 * to be automatically calibrated here.
I think it would be more accurate to just say the offset and callibscale
for temperature channel need to be at default values for the data sheet's
equation for the temperature sensor to be accurate.


> +		 */
> +		if (indio_dev->channels[i].type != IIO_VOLTAGE)
> +			continue;
> +
>  		/*
>  		 * Full-scale calibration isn't supported at gain 1, so skip in
>  		 * that case. Note that untypically full-scale calibration has

Maybe, instead of moving the 'if(... IIO_VOLTAGE)' check, this could alternatively
be set when initializing the temperature channel at ad7124_parse_channel_config().

 	if (num_channels < AD7124_MAX_CHANNELS) {
 		st->channels[num_channels] = (struct ad7124_channel) {
 			.nr = num_channels,
 			.ain = FIELD_PREP(AD7124_CHANNEL_AINP, AD7124_CHANNEL_AINx_TEMPSENSOR) |
 				FIELD_PREP(AD7124_CHANNEL_AINM, AD7124_CHANNEL_AINx_AVSS),
 			.cfg = {
 				.bipolar = true,
+				.calibration_offset = 0x800000,
+				.calibration_gain = st->gain_default,
 			},
 		};
 
 		chan[num_channels] = (struct iio_chan_spec) {
 			.type = IIO_TEMP,


Nevertheless, the current fix looks good to me as it is, so
Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

> 
> ---
> base-commit: 411e8b72c181e4f49352c12ced0fd8426eb683aa
> change-id: 20250923-iio-adc-ad7124-fix-temperature-channel-5900f7302886
> 
> Best regards,
> -- 
> David Lechner <dlechner@baylibre.com>
> 
> 

Re: [PATCH] iio: adc: ad7124: fix temperature channel

[David Lechner]

On 9/24/25 8:01 AM, Marcelo Schmitt wrote:
> Hi,
> 
> On 09/23, David Lechner wrote:
>> Fix temperature channel not working due to gain and offset not being
>> initialized. This was causing the raw temperature readings to be always
>> 8388608 (0x800000).
> 
> Would
> 'Fix temperature channel not working due to gain and offset not being
> initialized to their default values.'
> be a more accurate description?
> 
> 
>>
>> To fix it, we just make sure the gain and offset values are set to the
>> default values and still return early without doing an internal
>> calibration.
>>
>> While here, add a comment explaining why we don't bother calibrating
>> the temperature channel.
>>
>> Fixes: 47036a03a303 ("iio: adc: ad7124: Implement internal calibration at probe time")
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>> ---
> ...
>>  	for (i = 0; i < st->num_channels; i++) {
>> -
>> -		if (indio_dev->channels[i].type != IIO_VOLTAGE)
>> -			continue;
>> -
>>  		/*
>>  		 * For calibration the OFFSET register should hold its reset default
>>  		 * value. For the GAIN register there is no such requirement but
>> @@ -1531,6 +1527,13 @@ static int __ad7124_calibrate_all(struct ad7124_state *st, struct iio_dev *indio
>>  		st->channels[i].cfg.calibration_offset = 0x800000;
>>  		st->channels[i].cfg.calibration_gain = st->gain_default;
>>  
>> +		/*
>> +		 * Only the main voltage input channels are important enough
>> +		 * to be automatically calibrated here.
> I think it would be more accurate to just say the offset and callibscale
> for temperature channel need to be at default values for the data sheet's
> equation for the temperature sensor to be accurate.

This is true for all channels, not just the temperature channel and
there is an existing comment (partially visible above) that says
something like this already.

> 
> 
>> +		 */
>> +		if (indio_dev->channels[i].type != IIO_VOLTAGE)
>> +			continue;
>> +
>>  		/*
>>  		 * Full-scale calibration isn't supported at gain 1, so skip in
>>  		 * that case. Note that untypically full-scale calibration has
> 
> Maybe, instead of moving the 'if(... IIO_VOLTAGE)' check, this could alternatively
> be set when initializing the temperature channel at ad7124_parse_channel_config().
> 
>  	if (num_channels < AD7124_MAX_CHANNELS) {
>  		st->channels[num_channels] = (struct ad7124_channel) {
>  			.nr = num_channels,
>  			.ain = FIELD_PREP(AD7124_CHANNEL_AINP, AD7124_CHANNEL_AINx_TEMPSENSOR) |
>  				FIELD_PREP(AD7124_CHANNEL_AINM, AD7124_CHANNEL_AINx_AVSS),
>  			.cfg = {
>  				.bipolar = true,
> +				.calibration_offset = 0x800000,
> +				.calibration_gain = st->gain_default,

st->gain_default has not been initialized at this point, so this would
not work without more rearranging.

>  			},
>  		};
>  
>  		chan[num_channels] = (struct iio_chan_spec) {
>  			.type = IIO_TEMP,
> 
> 
> Nevertheless, the current fix looks good to me as it is, so
> Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
> 
>>
>> ---
>> base-commit: 411e8b72c181e4f49352c12ced0fd8426eb683aa
>> change-id: 20250923-iio-adc-ad7124-fix-temperature-channel-5900f7302886
>>
>> Best regards,
>> -- 
>> David Lechner <dlechner@baylibre.com>
>>
>>