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

[Guenter Roeck <linux@roeck-us.net>]

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