| Message ID | 20201224011607.1059534-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] hwmon: (ntc_thermistor): try reading processed | expand |
Please don't use iio_read_channel_processed and convert from milliVolts to microVolts by multiplying by 1000. My use case requires the additional precision that iio_read_channel_raw followed by iio_convert_raw_to_processed with the 1000X scaler provides. But I'm unsure about keeping the fallback 12-bit ADC in place. I kept it so as not to break Naveen Krishna Chatradhi's use case. But I'm not sure it still works after commit adc8ec5ff183d09ae7a9d2dd31125401d302ba63 "iio: inkern: pass through raw values if no scaling". Before the commit, iio_convert_raw_to_processed returned a negative number if there was no scaling available. Now, it returns the raw value. Does that mean that the raw value is already scaled to the correct units? Or does that mean that the scale is unknown and all you get is counts?
A closer reading of the "iio: inkern: pass through raw values if no scaling"
commit leads me to believe that even when the sensor provides no scale,
the returned value is expected to be in the correct units.
If that is true, there was a bug in the commit.
It failed to apply the caller supplied scale that ntc_thermistor.c relies on
to convert from milliVolts to microVolts.
Linus, would this change address your original problem?
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index fe30bcb6a57b..79787474d511 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -587,7 +587,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan,
* Just pass raw values as processed if no scaling is
* available.
*/
- *processed = raw;
+ *processed = raw * scale;
return 0;
}
From: Chris Lesiak <chris.lesiak@licor.com>
Sent: Wednesday, December 23, 2020 7:39 PM
To: Linus Walleij <linus.walleij@linaro.org>; Jean Delvare <jdelvare@suse.com>; Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org <linux-hwmon@vger.kernel.org>; Peter Rosin <peda@axentia.se>; Jonathan Cameron <jic23@cam.ac.uk>; linux-iio@vger.kernel.org <linux-iio@vger.kernel.org>
Subject: Re: [PATCH v2] hwmon: (ntc_thermistor): try reading processed
Please don't use iio_read_channel_processed and convert from milliVolts to
microVolts by multiplying by 1000. My use case requires the additional
precision that iio_read_channel_raw followed by iio_convert_raw_to_processed
with the 1000X scaler provides.
But I'm unsure about keeping the fallback 12-bit ADC in place. I kept it so as
not to break Naveen Krishna Chatradhi's use case. But I'm not sure it still works
after commit adc8ec5ff183d09ae7a9d2dd31125401d302ba63
"iio: inkern: pass through raw values if no scaling". Before the commit,
iio_convert_raw_to_processed returned a negative number if there was no
scaling available. Now, it returns the raw value.
Does that mean that the raw value is already scaled to the correct units?
Or does that mean that the scale is unknown and all you get is counts?
On Thu, Dec 24, 2020 at 2:39 AM Chris Lesiak <chris.lesiak@licor.com> wrote: > Please don't use iio_read_channel_processed and convert from milliVolts to > microVolts by multiplying by 1000. My use case requires the additional > precision that iio_read_channel_raw followed by iio_convert_raw_to_processed > with the 1000X scaler provides. I have to do this change because my ADC driver only provides processed channels (drivers/iio/adc/ab8500-gpadc.c). It provides raw values and it provides processed values but no scale. That means your code will not work, sadly. It will result in the raw value being used without scaling. The reason that the ADC cannot provide scaling is that the scale is not linear and based on calibration. IIO scaling is only linear. After this change the driver will use the processed values directly if possible, since these are in millivolts they need to be multiplied by 1000. Notice that actually this NTC driver is the only driver in the entire kernel that uses iio_convert_raw_to_processed(). (Well lmp91000.c calls it to convert its own raw value to a processed one, so will result in a recursive call.) I kind of find the call dubious outside of IIO itself, it feels like calling iio_read_channel_processed() is more natural? Who outside of IIO needs the raw value really? It's what I used in all of my drivers. > But I'm unsure about keeping the fallback 12-bit ADC in place. I kept it so as > not to break Naveen Krishna Chatradhi's use case. But I'm not sure it still works > after commit adc8ec5ff183d09ae7a9d2dd31125401d302ba63 > "iio: inkern: pass through raw values if no scaling". Before the commit, > iio_convert_raw_to_processed returned a negative number if there was no > scaling available. Now, it returns the raw value. > Does that mean that the raw value is already scaled to the correct units? > Or does that mean that the scale is unknown and all you get is counts? As far as I can tell it is the former of these two, as you point out in your second mail. Yours, Linus Walleij
On Thu, Dec 24, 2020 at 4:15 AM Chris Lesiak <chris.lesiak@licor.com> wrote: > A closer reading of the "iio: inkern: pass through raw values if no scaling" > commit leads me to believe that even when the sensor provides no scale, > the returned value is expected to be in the correct units. > > If that is true, there was a bug in the commit. > It failed to apply the caller supplied scale that ntc_thermistor.c relies on > to convert from milliVolts to microVolts. > > Linus, would this change address your original problem? > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c > index fe30bcb6a57b..79787474d511 100644 > --- a/drivers/iio/inkern.c > +++ b/drivers/iio/inkern.c > @@ -587,7 +587,7 @@ static int iio_convert_raw_to_processed_unlocked(struct iio_channel *chan, > * Just pass raw values as processed if no scaling is > * available. > */ > - *processed = raw; > + *processed = raw * scale; > return 0; > } I do not think the raw values have any obligation to be in e.g. millivolts. They may be, by chance. In my case, it is just an unscaled byte [0..255]. So it unfortunately does not solve my problem, because in my driver, that does not support scaling, the result will be the raw value * 1000 which is not going to be microvolts at all, because the raw values are not millivolts, rather a value 0..255. However if I used the processed values, the value will be adjusted non-linearly to millivolts. Yours, Linus Walleij
Linus Walleij <linus.walleij@linaro.org>wrote: > I have to do this change because my ADC driver only provides processed > channels (drivers/iio/adc/ab8500-gpadc.c). It provides raw values and > it provides processed values but no scale. That means your code will > not work, sadly. It will result in the raw value being used without scaling. > The reason that the ADC cannot provide scaling is that the scale is not > linear and based on calibration. IIO scaling is only linear. I haven't been able to find detailed documentation on the ab8500-gpadc, so I have a couple of questions / comments: 1. The driver appears to support temperature output directly. Why do you need ntc_thermistor? 2. I don't understand how the ab8500_gpadc_read_raw output of processed data could possibly be correct. if (mask == IIO_CHAN_INFO_PROCESSED) { processed = ab8500_gpadc_ad_to_voltage(gpadc, ch->id, raw_val); if (processed < 0) return processed; /* Return millivolt or milliamps or millicentigrades */ *val = processed * 1000; return IIO_VAL_INT; } Note that both processed and *val are both of type int. If *val really does end up with milliVolt units, then processed must have had Volt units. And you only have single Volt resolution. Either you are working with a lot higher voltages than I usually see, or something must be wrong.
On Sat, Dec 26, 2020 at 2:45 AM Chris Lesiak <chris.lesiak@licor.com> wrote: > I haven't been able to find detailed documentation on the ab8500-gpadc, > so I have a couple of questions / comments: The documentation is available here: https://web.archive.org/web/20130614115108/http://www.stericsson.com/developers/CD00291561_UM1031_AB8500_user_manual-rev5_CTDS_public.pdf As it is in the wayback machine I put a copy here: https://dflund.se/~triad/krad/CD00291561_UM1031_AB8500_user_manual-rev5_CTDS_public.pdf As with many mixsig products the ADC isn't very well documented. The code is the documentation... > 1. The driver appears to support temperature output directly. Why do > you need ntc_thermistor? Actually these channels (if you mean AB8500_GPADC_CHAN_BAT_TEMP, AB8505_GPADC_CHAN_DIE_TEMP, AB8500_GPADC_CHAN_XTAL_TEMP) are just voltages and they need to be processed because they are some kind of thermistors and not temperatures at all, these are voltages. It's just that the channels are named like this. However in this case (the current patch), the two channels used for the thermistors are AB8500_GPADC_CHAN_ADC_AUX_1 AB8500_GPADC_CHAN_ADC_AUX_2 which are just common arbitrary voltage ADCs, not related to the above, so it doesn't really apply anyways. > 2. I don't understand how the ab8500_gpadc_read_raw output of processed > data could possibly be correct. > > if (mask == IIO_CHAN_INFO_PROCESSED) { > processed = ab8500_gpadc_ad_to_voltage(gpadc, ch->id, raw_val); > if (processed < 0) > return processed; > > /* Return millivolt or milliamps or millicentigrades */ > *val = processed * 1000; > return IIO_VAL_INT; > } > > Note that both processed and *val are both of type int. > > If *val really does end up with milliVolt units, then processed must > have had Volt units. And you only have single Volt resolution. Sorry there is a bug there and I sent a patch the other day: https://lore.kernel.org/linux-iio/20201224011700.1059659-1-linus.walleij@linaro.org/T/#u "processed" contains millivolts, the * 1000 is a bug. This is why reading the processed channel without multiplying with 1000 was working for me before, then I discovered that the contract of the API is to pass millivolts and then I fixed this bug. Sorry about the confusion :/ > Either you are working with a lot higher voltages than I usually see, > or something must be wrong. Yeah something was wrong. Fixed with the patch. But do you agree with the general stance that we should give precedence to using iio_read_channel_processed() and multiply the result with 1000? It should work with any driver I think. Yours, Linus Walleij
Linus Walleij <linus.walleij@linaro.org> wrote: > But do you agree with the general stance that we should > give precedence to using iio_read_channel_processed() > and multiply the result with 1000? It should work with > any driver I think. In order to preserve resolution of the microVolt value for existing code, I'd prefer a solution similar to the existing implementation of iio_read_channel_processed. Sans error handling: int uv; if (iio_channel_has_info(chan->channel, IIO_CHAN_INFO_PROCESSED)) { int mv; iio_read_channel_processed(channel, &mv); uv = 1000 * mv; } else { int raw; iio_read_channel_raw(channel, &raw); iio_convert_raw_to_processed(channel, raw, &uv, 1000); } return uv;
On Sun, Dec 27, 2020 at 7:54 PM Chris Lesiak <chris.lesiak@licor.com> wrote: > Linus Walleij <linus.walleij@linaro.org> wrote: > > But do you agree with the general stance that we should > > give precedence to using iio_read_channel_processed() > > and multiply the result with 1000? It should work with > > any driver I think. > > In order to preserve resolution of the microVolt value for existing code, Aha you mean that iio_read_channel_processed() loses precision when converting raw to scaled? > I'd prefer a solution similar to the existing implementation of > iio_read_channel_processed. That seems like the wrong way to work around a problem in the core. If iio_read_channel_processed() loses precision we should fix iio_read_channel_processed() and not try to work around the problem in the consumers. It's fine to fix all the consumers in the kernel. What about changing the signature of: int iio_read_channel_processed(struct iio_channel *chan, int *val) to: int iio_read_channel_processed(struct iio_channel *chan, int *val, unsigned int scale) And just augment all calls to pass 1 except the ntc driver which then passes 1000 in the last argument? If Jonathan agrees I can fix a patch to alter all the ~50 call sites like this and include the change to this NTC driver. Yours, Linus Walleij
Linus Walleij <linus.walleij@linaro.org> wrote: > Aha you mean that iio_read_channel_processed() loses > precision when converting raw to scaled? Yes. For example, take a 16-bit ADC with 4.096 V reference. The native resolution is 62.5 microVolts. Yet iio_read_channel_processed() can only give integer milliVolts. iio_read_channel_raw() followed by iio_convert_raw_to_processed() with a scale of 1000 will preserve more of the native resolution. User space can do this by using floating point numbers when converting to processed. You are likely to lose precision for all ADCs greater than about 12-bit. Chris Lesiak <chris.leisak@licor.com> wrote: >> I'd prefer a solution similar to the existing implementation of >> iio_read_channel_processed. > That seems like the wrong way to work around a problem in > the core. > If iio_read_channel_processed() loses precision we should > fix iio_read_channel_processed() and not try to work around > the problem in the consumers. > It's fine to fix all the consumers in the kernel. > What about changing the signature of: > int iio_read_channel_processed(struct iio_channel *chan, int *val) > to: > int iio_read_channel_processed(struct iio_channel *chan, int *val, > unsigned int scale) > And just augment all calls to pass 1 except the ntc driver > which then passes 1000 in the last argument? > If Jonathan agrees I can fix a patch to alter all the ~50 > call sites like this and include the change to this NTC > driver. That would meet my needs and does address what I think is a shortcoming in the existing iio_read_channel_processed interface.
On Sun, 27 Dec 2020 22:08:24 +0000 Chris Lesiak <chris.lesiak@licor.com> wrote: > Linus Walleij <linus.walleij@linaro.org> wrote: > > Aha you mean that iio_read_channel_processed() loses > > precision when converting raw to scaled? > > Yes. For example, take a 16-bit ADC with 4.096 V reference. > The native resolution is 62.5 microVolts. > Yet iio_read_channel_processed() can only give integer milliVolts. > iio_read_channel_raw() followed by iio_convert_raw_to_processed() > with a scale of 1000 will preserve more of the native resolution. > User space can do this by using floating point numbers when > converting to processed. > > You are likely to lose precision for all ADCs greater than about 12-bit. > > Chris Lesiak <chris.leisak@licor.com> wrote: > >> I'd prefer a solution similar to the existing implementation of > >> iio_read_channel_processed. > > > That seems like the wrong way to work around a problem in > > the core. > > > If iio_read_channel_processed() loses precision we should > > fix iio_read_channel_processed() and not try to work around > > the problem in the consumers. > > > It's fine to fix all the consumers in the kernel. > > > What about changing the signature of: > > > int iio_read_channel_processed(struct iio_channel *chan, int *val) > > > to: > > > int iio_read_channel_processed(struct iio_channel *chan, int *val, > > unsigned int scale) > > > And just augment all calls to pass 1 except the ntc driver > > which then passes 1000 in the last argument? > > > If Jonathan agrees I can fix a patch to alter all the ~50 > > call sites like this and include the change to this NTC > > driver. > > That would meet my needs and does address what I think is a > shortcoming in the existing iio_read_channel_processed interface. I'm fine with this proposal as well. Makes a lot of sense given there is no particular reason why another subsystem should want to convert to IIO base units (here milivolts). The only other way I could think of doing it would be to have iio_read_channel_processed 'return' a pair of integers and type etc IIO_VAL_INT_PLUS_MICRO much like read_raw etc does inside the actual drivers. It would be a bit clunky to implement however and potentially require some messy maths in the consumers. May want to think about whether we need additional sanity checks for overflow etc. Seems unlikely we'd hit hit them for voltage, but we might for some other types of sensor where the base unit is much smaller (wrt to real world values). Jonathan
On Tue, Dec 29, 2020 at 02:25:31PM +0000, Jonathan Cameron wrote: > On Sun, 27 Dec 2020 22:08:24 +0000 > Chris Lesiak <chris.lesiak@licor.com> wrote: > > > Linus Walleij <linus.walleij@linaro.org> wrote: > > > Aha you mean that iio_read_channel_processed() loses > > > precision when converting raw to scaled? > > > > Yes. For example, take a 16-bit ADC with 4.096 V reference. > > The native resolution is 62.5 microVolts. > > Yet iio_read_channel_processed() can only give integer milliVolts. > > iio_read_channel_raw() followed by iio_convert_raw_to_processed() > > with a scale of 1000 will preserve more of the native resolution. > > User space can do this by using floating point numbers when > > converting to processed. > > > > You are likely to lose precision for all ADCs greater than about 12-bit. > > > > Chris Lesiak <chris.leisak@licor.com> wrote: > > >> I'd prefer a solution similar to the existing implementation of > > >> iio_read_channel_processed. > > > > > That seems like the wrong way to work around a problem in > > > the core. > > > > > If iio_read_channel_processed() loses precision we should > > > fix iio_read_channel_processed() and not try to work around > > > the problem in the consumers. > > > > > It's fine to fix all the consumers in the kernel. > > > > > What about changing the signature of: > > > > > int iio_read_channel_processed(struct iio_channel *chan, int *val) > > > > > to: > > > > > int iio_read_channel_processed(struct iio_channel *chan, int *val, > > > unsigned int scale) > > > > > And just augment all calls to pass 1 except the ntc driver > > > which then passes 1000 in the last argument? > > > > > If Jonathan agrees I can fix a patch to alter all the ~50 > > > call sites like this and include the change to this NTC > > > driver. > > > > That would meet my needs and does address what I think is a > > shortcoming in the existing iio_read_channel_processed interface. > I'm fine with this proposal as well. Makes a lot of sense given > there is no particular reason why another subsystem should want to > convert to IIO base units (here milivolts). > > The only other way I could think of doing it would be to > have iio_read_channel_processed 'return' a pair of integers and > type etc IIO_VAL_INT_PLUS_MICRO much like read_raw etc does inside > the actual drivers. > > It would be a bit clunky to implement however and potentially require > some messy maths in the consumers. > > May want to think about whether we need additional sanity checks for > overflow etc. Seems unlikely we'd hit hit them for voltage, but > we might for some other types of sensor where the base unit is much > smaller (wrt to real world values). > All this makes me wonder if it would be better to add a separate API function (iio_read_channel_processed_scale ?) instead of replacing the existing one. Changing 50+ callers at the same time sounds messy. Guenter
On Tue, 29 Dec 2020 08:33:46 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > On Tue, Dec 29, 2020 at 02:25:31PM +0000, Jonathan Cameron wrote: > > On Sun, 27 Dec 2020 22:08:24 +0000 > > Chris Lesiak <chris.lesiak@licor.com> wrote: > > > > > Linus Walleij <linus.walleij@linaro.org> wrote: > > > > Aha you mean that iio_read_channel_processed() loses > > > > precision when converting raw to scaled? > > > > > > Yes. For example, take a 16-bit ADC with 4.096 V reference. > > > The native resolution is 62.5 microVolts. > > > Yet iio_read_channel_processed() can only give integer milliVolts. > > > iio_read_channel_raw() followed by iio_convert_raw_to_processed() > > > with a scale of 1000 will preserve more of the native resolution. > > > User space can do this by using floating point numbers when > > > converting to processed. > > > > > > You are likely to lose precision for all ADCs greater than about 12-bit. > > > > > > Chris Lesiak <chris.leisak@licor.com> wrote: > > > >> I'd prefer a solution similar to the existing implementation of > > > >> iio_read_channel_processed. > > > > > > > That seems like the wrong way to work around a problem in > > > > the core. > > > > > > > If iio_read_channel_processed() loses precision we should > > > > fix iio_read_channel_processed() and not try to work around > > > > the problem in the consumers. > > > > > > > It's fine to fix all the consumers in the kernel. > > > > > > > What about changing the signature of: > > > > > > > int iio_read_channel_processed(struct iio_channel *chan, int *val) > > > > > > > to: > > > > > > > int iio_read_channel_processed(struct iio_channel *chan, int *val, > > > > unsigned int scale) > > > > > > > And just augment all calls to pass 1 except the ntc driver > > > > which then passes 1000 in the last argument? > > > > > > > If Jonathan agrees I can fix a patch to alter all the ~50 > > > > call sites like this and include the change to this NTC > > > > driver. > > > > > > That would meet my needs and does address what I think is a > > > shortcoming in the existing iio_read_channel_processed interface. > > I'm fine with this proposal as well. Makes a lot of sense given > > there is no particular reason why another subsystem should want to > > convert to IIO base units (here milivolts). > > > > The only other way I could think of doing it would be to > > have iio_read_channel_processed 'return' a pair of integers and > > type etc IIO_VAL_INT_PLUS_MICRO much like read_raw etc does inside > > the actual drivers. > > > > It would be a bit clunky to implement however and potentially require > > some messy maths in the consumers. > > > > May want to think about whether we need additional sanity checks for > > overflow etc. Seems unlikely we'd hit hit them for voltage, but > > we might for some other types of sensor where the base unit is much > > smaller (wrt to real world values). > > > > All this makes me wonder if it would be better to add a separate > API function (iio_read_channel_processed_scale ?) instead of replacing > the existing one. Changing 50+ callers at the same time sounds messy. Agreed - definitely makes more sense to do it that way. Jonathan > > Guenter
diff --git a/drivers/hwmon/ntc_thermistor.c b/drivers/hwmon/ntc_thermistor.c index 3aad62a0e661..c1c02cc454fc 100644 --- a/drivers/hwmon/ntc_thermistor.c +++ b/drivers/hwmon/ntc_thermistor.c @@ -326,18 +326,33 @@ 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); + 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; + /* + * This fallback uses a raw read and then + * assumes the ADC is 12 bits, scaling with + * a factor 1000 to get to microvolts. + */ + 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; + } + } else { + /* + * The processed channel is in millivolts, so scale this + * to microvolts. + */ + uv *= 1000; } return uv;
Before trying the custom method of reading the sensor as raw and then converting, 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. We need to multiply the scaled value with 1000 to get to microvolts from millivolts which is what processed IIO channels returns. Keep the code that assumes 12bit ADC around as a fallback. 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> --- ChangeLog v1->v2: - Fix the patch to multiply the processed value by 1000 to get to microvolts from millivolts. - Fix up the confusion in the commit message. - Drop pointless comments about the code, we keep the original code path around if processed reads don't work, nothing bad with that. --- drivers/hwmon/ntc_thermistor.c | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-)