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

From: Jonathan Cameron <jic23@kernel.org>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Chris Lesiak <chris.lesiak@licor.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Jean Delvare <jdelvare@suse.com>,
	"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
Date: Tue, 29 Dec 2020 16:53:57 +0000	[thread overview]
Message-ID: <20201229165357.3b833682@archlinux> (raw)
In-Reply-To: <20201229163346.GA57378@roeck-us.net>

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