[PATCH] hwmon: (ntc_thermistor): try reading processed

[PATCH] hwmon: (ntc_thermistor): try reading processed

[Linus Walleij]

Before trying the custom method of reading the sensor
as raw and then converting assuming 1000 scaling, just
use iio_read_channel_processed() which first tries to
see if the ADC can provide a processed value directly,
else reads raw and applies scaling inside of IIO
using the scale attributes of the ADC.

The code that hardcodes scaling to 1000 and assumes
a 12bit ADC is very dubious. I keep it around here
but I have a strong urge to just delete it.

This gives correct readings on the AB8500 thermistor
inputs used in the Ux500 HREFP520 platform for reading
battery and board temperature.

Cc: Peter Rosin <peda@axentia.se>
Cc: Chris Lesiak <chris.lesiak@licor.com>
Cc: Jonathan Cameron <jic23@cam.ac.uk>
Cc: linux-iio@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/hwmon/ntc_thermistor.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/ntc_thermistor.c b/drivers/hwmon/ntc_thermistor.c
index 3aad62a0e661..ac0d80faddf6 100644
--- a/drivers/hwmon/ntc_thermistor.c
+++ b/drivers/hwmon/ntc_thermistor.c
@@ -326,18 +326,29 @@ struct ntc_data {
 static int ntc_adc_iio_read(struct ntc_thermistor_platform_data *pdata)
 {
 	struct iio_channel *channel = pdata->chan;
-	int raw, uv, ret;
+	int uv, ret;
 
-	ret = iio_read_channel_raw(channel, &raw);
+	/* A processed voltage channel will return microvolts */
+	ret = iio_read_channel_processed(channel, &uv);
 	if (ret < 0) {
-		pr_err("read channel() error: %d\n", ret);
-		return ret;
-	}
+		int raw;
 
-	ret = iio_convert_raw_to_processed(channel, raw, &uv, 1000);
-	if (ret < 0) {
-		/* Assume 12 bit ADC with vref at pullup_uv */
-		uv = (pdata->pullup_uv * (s64)raw) >> 12;
+		/*
+		 * FIXME: This fallback to using a raw read and then right
+		 * out assume the ADC is 12 bits and hard-coding scale
+		 * to 1000 seems a bit dangerous. Should it simply be
+		 * deleted?
+		 */
+		ret = iio_read_channel_raw(channel, &raw);
+		if (ret < 0) {
+			pr_err("read channel() error: %d\n", ret);
+			return ret;
+		}
+		ret = iio_convert_raw_to_processed(channel, raw, &uv, 1000);
+		if (ret < 0) {
+			/* Assume 12 bit ADC with vref at pullup_uv */
+			uv = (pdata->pullup_uv * (s64)raw) >> 12;
+		}
 	}
 
 	return uv;
-- 
2.29.2

Re: [PATCH] hwmon: (ntc_thermistor): try reading processed

[Guenter Roeck]

On 12/19/20 2:41 PM, Linus Walleij wrote:
> Before trying the custom method of reading the sensor
> as raw and then converting assuming 1000 scaling, just
> use iio_read_channel_processed() which first tries to
> see if the ADC can provide a processed value directly,
> else reads raw and applies scaling inside of IIO
> using the scale attributes of the ADC.
> 
> The code that hardcodes scaling to 1000 and assumes
> a 12bit ADC is very dubious. I keep it around here
> but I have a strong urge to just delete it.
> 
> This gives correct readings on the AB8500 thermistor
> inputs used in the Ux500 HREFP520 platform for reading
> battery and board temperature.
> 
> Cc: Peter Rosin <peda@axentia.se>
> Cc: Chris Lesiak <chris.lesiak@licor.com>
> Cc: Jonathan Cameron <jic23@cam.ac.uk>
> Cc: linux-iio@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/hwmon/ntc_thermistor.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hwmon/ntc_thermistor.c b/drivers/hwmon/ntc_thermistor.c
> index 3aad62a0e661..ac0d80faddf6 100644
> --- a/drivers/hwmon/ntc_thermistor.c
> +++ b/drivers/hwmon/ntc_thermistor.c
> @@ -326,18 +326,29 @@ struct ntc_data {
>  static int ntc_adc_iio_read(struct ntc_thermistor_platform_data *pdata)
>  {
>  	struct iio_channel *channel = pdata->chan;
> -	int raw, uv, ret;
> +	int uv, ret;
>  
> -	ret = iio_read_channel_raw(channel, &raw);
> +	/* A processed voltage channel will return microvolts */
> +	ret = iio_read_channel_processed(channel, &uv);
>  	if (ret < 0) {
> -		pr_err("read channel() error: %d\n", ret);
> -		return ret;
> -	}
> +		int raw;
>  
> -	ret = iio_convert_raw_to_processed(channel, raw, &uv, 1000);
> -	if (ret < 0) {
> -		/* Assume 12 bit ADC with vref at pullup_uv */
> -		uv = (pdata->pullup_uv * (s64)raw) >> 12;
> +		/*
> +		 * FIXME: This fallback to using a raw read and then right
> +		 * out assume the ADC is 12 bits and hard-coding scale
> +		 * to 1000 seems a bit dangerous. Should it simply be
> +		 * deleted?
> +		 */

The hwmon ABI specifically supports unscaled values, which can then be
scaled in userspace using the sensors configuration file.
Given that we return the pseudo-scaled value to userspace today,
it seems to me that it would do more harm to change that instead of just
leaving it in place.

Either case, calling iio_convert_raw_to_processed() does not seem to add
value here. This is already done by iio_read_channel_processed().
The best we can do is to use the original fallback.

Guenter

> +		ret = iio_read_channel_raw(channel, &raw);
> +		if (ret < 0) {
> +			pr_err("read channel() error: %d\n", ret);
> +			return ret;
> +		}
> +		ret = iio_convert_raw_to_processed(channel, raw, &uv, 1000);
> +		if (ret < 0) {
> +			/* Assume 12 bit ADC with vref at pullup_uv */
> +			uv = (pdata->pullup_uv * (s64)raw) >> 12;
> +		}
>  	}
>  
>  	return uv;
> 

Re: [PATCH] hwmon: (ntc_thermistor): try reading processed

[Linus Walleij]

On Mon, Dec 21, 2020 at 5:15 PM Guenter Roeck <linux@roeck-us.net> wrote:

> > -     ret = iio_convert_raw_to_processed(channel, raw, &uv, 1000);
> > -     if (ret < 0) {
> > -             /* Assume 12 bit ADC with vref at pullup_uv */
> > -             uv = (pdata->pullup_uv * (s64)raw) >> 12;
> > +             /*
> > +              * FIXME: This fallback to using a raw read and then right
> > +              * out assume the ADC is 12 bits and hard-coding scale
> > +              * to 1000 seems a bit dangerous. Should it simply be
> > +              * deleted?
> > +              */
>
> The hwmon ABI specifically supports unscaled values, which can then be
> scaled in userspace using the sensors configuration file.
> Given that we return the pseudo-scaled value to userspace today,
> it seems to me that it would do more harm to change that instead of just
> leaving it in place.

I see.

I tried to drill down and see the history of the driver and in the
original commit all values are scaled with the function
get_temp_mC() which indicates that the driver has always
intended to return millicentigrades, not unscaled values (as
far as I can tell).

First commit 9e8269de100dd0be1199778dc175ff22417aebd2
"hwmon: (ntc_thermistor) Add DT with IIO support to NTC thermistor driver"
adds the result  >>= 12 thing on top of a raw IIO read,
so here is a silent assumption of a 12 bit ADC.

Then commit 0315253b19bbc63eedad2f6125c21e280c76e29b
"hwmon: (ntc_thermistor) fix iio raw to microvolts conversion"
calls iio_convert_raw_to_processed() to get around the 12
bit assumption, instead adding a 1000x scale assumption
on the value passed in from the raw read. This just looks
wrong, why would it be 1000 and not 1 like the IIO core does
when we call iio_read_channel_processed()? It looks like it is
actually compensating for a bug in the ADC returning
the wrong scale: the author may have used a buggy ADC
driver return a scaling to volts instead of millivolts and then this
was a trial-and-error solution to that bug in the ADC
driver.

In that case it would be nice to know which ADC driver,
so we can fix it! I suspect maybe an out-of-tree ADC?

So there is first a hardcoded solution to handle raw 12 bit
values and then a trial-and-error fix for 1000x scaling in the
code that was supposed to make it generic.

The actual situation I have is that I used the thermistor for a thermal
zone, and AFAICT these require that the temperature be in millicelsius,
at least it will be very hard to handle the thermal zone in the device
tree that define the trip points in millicelsius, so since the driver sets
HWMON_C_REGISTER_TZ it might break
the thermal zone ABI rather than the hwmon/sensors ABI, right?

At least commit c08860ffe5c0e986e208e8217dae8191c0b40b24
"hwmon: (ntc_thermistor) Add ntc thermistor to thermal subsystem as a sensor"
adds a thermal zone calling the function get_temp_mc()
clearly requiring a millicentigrade measure.

I also think it is indeed resulting in millicelsius on the platform that
commit 0315253b19bbc63eedad2f6125c21e280c76e29b
is intended for, it's just that it fixes a bug in the wrong place...

> Either case, calling iio_convert_raw_to_processed() does not seem to add
> value here. This is already done by iio_read_channel_processed().
> The best we can do is to use the original fallback.

I don't know if the patch is especially messy but this part inside
the if (ret < 0) clause:

> > +             ret = iio_read_channel_raw(channel, &raw);
> > +             if (ret < 0) {
> > +                     pr_err("read channel() error: %d\n", ret);
> > +                     return ret;
> > +             }
> > +             ret = iio_convert_raw_to_processed(channel, raw, &uv, 1000);
> > +             if (ret < 0) {
> > +                     /* Assume 12 bit ADC with vref at pullup_uv */
> > +                     uv = (pdata->pullup_uv * (s64)raw) >> 12;
> > +             }

shold be identical to the original fallback.

If you want me to revise the patch in some way, I can fix, to me
it looks like the fallback will just work on a certain platform with
erroneous 1000x offset scaling, it would be good to know which
one so we can fix the actual problem.

Yours,
Linus Walleij

Re: [PATCH] hwmon: (ntc_thermistor): try reading processed

[Guenter Roeck]

On 12/23/20 1:08 PM, Linus Walleij wrote:
> On Mon, Dec 21, 2020 at 5:15 PM Guenter Roeck <linux@roeck-us.net> wrote:
> 
>>> -     ret = iio_convert_raw_to_processed(channel, raw, &uv, 1000);
>>> -     if (ret < 0) {
>>> -             /* Assume 12 bit ADC with vref at pullup_uv */
>>> -             uv = (pdata->pullup_uv * (s64)raw) >> 12;
>>> +             /*
>>> +              * FIXME: This fallback to using a raw read and then right
>>> +              * out assume the ADC is 12 bits and hard-coding scale
>>> +              * to 1000 seems a bit dangerous. Should it simply be
>>> +              * deleted?
>>> +              */
>>
>> The hwmon ABI specifically supports unscaled values, which can then be
>> scaled in userspace using the sensors configuration file.
>> Given that we return the pseudo-scaled value to userspace today,
>> it seems to me that it would do more harm to change that instead of just
>> leaving it in place.
> 
> I see.
> 
> I tried to drill down and see the history of the driver and in the
> original commit all values are scaled with the function
> get_temp_mC() which indicates that the driver has always
> intended to return millicentigrades, not unscaled values (as
> far as I can tell).
> 

Sure. That isn't the problem. I didn't say that the value reported
to userspace _shall_ be unscaled, I said that the ABI supports it.

We are not discussing your patch here, just the comment. You don't
answer the basic question: What should the driver do if the iio
subsystem only delivers raw values ? I suggested we should just keep
the current assumption in this case, ie do the fixed conversion,
and let userspace handle any necessary adjustments. You seem to object.
With your patch, we get a comment in the driver suggesting that some
code should be removed. In my opinion, such a comment comment does
not add any value. Ether we drop the code or we don't, and I dislike
removing it.

That results in my usual fallback: When in doubt, do nothing.

Guenter

Re: [PATCH] hwmon: (ntc_thermistor): try reading processed

[Linus Walleij]

On Wed, Dec 23, 2020 at 10:42 PM Guenter Roeck <linux@roeck-us.net> wrote:

> You don't
> answer the basic question: What should the driver do if the iio
> subsystem only delivers raw values ? I suggested we should just keep
> the current assumption in this case, ie do the fixed conversion,
> and let userspace handle any necessary adjustments. You seem to object.
> With your patch, we get a comment in the driver suggesting that some
> code should be removed. In my opinion, such a comment comment does
> not add any value. Ether we drop the code or we don't, and I dislike
> removing it.
>
> That results in my usual fallback: When in doubt, do nothing.

OK I get it, I'll fix it up along with the confused bug pointed out by
Chris and resend.

Thanks!
Linus Walleij

Re: [PATCH] hwmon: (ntc_thermistor): try reading processed

[Chris Lesiak]

First, let me apologize if some of you may have received an
earlier version as HTML.

I forgot that I was in my web email client.


On 12/23/20 3:08 PM, Linus Walleij wrote:
> Then commit 0315253b19bbc63eedad2f6125c21e280c76e29b
> "hwmon: (ntc_thermistor) fix iio raw to microvolts conversion"
> calls iio_convert_raw_to_processed() to get around the 12
> bit assumption, instead adding a 1000x scale assumption
> on the value passed in from the raw read. This just looks
> wrong, why would it be 1000 and not 1 like the IIO core does
> when we call iio_read_channel_processed()? It looks like it is
> actually compensating for a bug in the ADC returning
> the wrong scale: the author may have used a buggy ADC
> driver return a scaling to volts instead of millivolts and then this
> was a trial-and-error solution to that bug in the ADC
> driver.
>
> In that case it would be nice to know which ADC driver,
> so we can fix it! I suspect maybe an out-of-tree ADC?

I'm the author ofcommit 0315253b19bbc63eedad2f6125c21e280c76e29b
"hwmon: (ntc_thermistor) fix iio raw to microvolts conversion".

The 1000X scaling was not to get around a bad ADC driver that returns
volts instead of millivolts; it was to convert millivolts to microvolts.
The function ntc_adc_iio_read needs to return microvolts.

Re: [PATCH] hwmon: (ntc_thermistor): try reading processed

[Linus Walleij]

Hi Chris!

Thanks for stepping in!

On Wed, Dec 23, 2020 at 11:24 PM Chris Lesiak <chris.lesiak@licor.com> wrote:

> I'm the author of commit 0315253b19bbc63eedad2f6125c21e280c76e29b
> "hwmon: (ntc_thermistor) fix iio raw to microvolts conversion".
>
> The 1000X scaling was not to get around a bad ADC driver that returns volts instead of
> millivolts; it was to convert millivolts to microvolts.  The function ntc_adc_iio_read
> needs to return microvolts.

Sorry for my confusion of terms.

I see my mistake, I'll update the patch to scale the returned value
from millivolts to microvolts and respin!

Yours,
Linus Walleij