All of lore.kernel.org
 help / color / mirror / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Jonathan Cameron <jic23@kernel.org>
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 08:33:46 -0800	[thread overview]
Message-ID: <20201229163346.GA57378@roeck-us.net> (raw)
In-Reply-To: <20201229142531.3a4ceedc@archlinux>

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

  reply	other threads:[~2020-12-29 16:34 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-24  1:16 Linus Walleij
2020-12-24  1:39 ` Chris Lesiak
2020-12-24  3:15   ` Chris Lesiak
2020-12-25 23:01     ` Linus Walleij
2020-12-25 22:55   ` Linus Walleij
2020-12-26  1:45     ` Chris Lesiak
2020-12-27 13:46       ` Linus Walleij
2020-12-27 18:54         ` Chris Lesiak
2020-12-27 21:47           ` Linus Walleij
2020-12-27 22:08             ` Chris Lesiak
2020-12-29 14:25               ` Jonathan Cameron
2020-12-29 16:33                 ` Guenter Roeck [this message]
2020-12-29 16:53                   ` Jonathan Cameron

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201229163346.GA57378@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=chris.lesiak@licor.com \
    --cc=jdelvare@suse.com \
    --cc=jic23@cam.ac.uk \
    --cc=jic23@kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-hwmon@vger.kernel.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=peda@axentia.se \
    --subject='Re: [PATCH v2] hwmon: (ntc_thermistor): try reading processed' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.