| Message ID | 20201219224143.686074-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (ntc_thermistor): try reading processed | expand |
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; >
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
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
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.
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
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
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;
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(-)