linux-kernel - RE: [PATCH 3/8] iio: Add support for DA9150 GPADC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 9 Oct 2014 14:29:17 +0000
From:	"Opensource [Adam Thomson]" <Adam.Thomson.Opensource@...semi.com>
To:	Jonathan Cameron <jic23@...nel.org>,
	"Opensource [Adam Thomson]" <Adam.Thomson.Opensource@...semi.com>,
	Lee Jones <lee.jones@...aro.org>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	Sebastian Reichel <sre@...nel.org>,
	"Dmitry Eremin-Solenikov" <dbaryshkov@...il.com>,
	David Woodhouse <dwmw2@...radead.org>,
	"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	"Mark Rutland" <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Grant Likely <grant.likely@...aro.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Joe Perches <joe@...ches.com>,
	"linux-api@...r.kernel.org" <linux-api@...r.kernel.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Support Opensource" <Support.Opensource@...semi.com>
Subject: RE: [PATCH 3/8] iio: Add support for DA9150 GPADC

On October 7, 2014 20:37, Jonathan Cameron wrote:

> On October 7, 2014 3:55:55 PM GMT+01:00, "Opensource [Adam Thomson]"
> <Adam.Thomson.Opensource@...semi.com> wrote:
> >On September 27, 2014 11:50, Jonathan Cameron wrote:
> >
> >> On 23/09/14 11:53, Adam Thomson wrote:
> >> > This patch adds support for DA9150 Charger & Fuel-Gauge IC GPADC.
> >
> >> > +
> >> > +static inline int da9150_gpadc_gpio_6v_voltage_now(int raw_val)
> >> > +{
> >> > +	/* Convert to mV */
> >> > +	return (6 * ((raw_val * 1000) + 500)) / 1024;
> >> These could all be expressed as raw values with offsets
> >> and scales (and that would be preferred).
> >> E.g. This one has offset 500000 and scale 6000/1024 or even
> >> better use IIO_VAL_FRACTIONAL_LOG2 for scale with val1 = 6000
> >> and val2 = (log_2 1024) = 10.
> >>
> >
> >What you've suggested isn't correct. The problem here is that the
> >offset is
> >added first to the raw ADC reading, without factoring the ADC value
> >accordingly
> >to match the factor of the offset. If we take the original equation
> >provided for
> >this channel of the ADC, the offset is actually 0.5 which should be
> >added to the
> >raw ADC value. This doesn't fit into the implementation in the kernel
> >as we
> >can't use floating point. If we multiply the offset but not the raw ADC
> >value,
> >then add them before applying the scale factor, then the result is
> >wrong at the
> >end. Basically you need a scale for the raw ADC value to match the
> >offset scale
> >so you can achieve the correct results, which is what my calculation
> >does.
> >But that seems impossible with the current raw|offset|scale method.
> Oops got that wrong.  The fixed point maths to fix the in kernel interface isn't exactly
>  difficult but indeed it does not handle this currently.

I did have a quick look when I had a spare moment, and I guess you could do
something like having an offset scale/factor which can be applied to the raw
reading, prior to adding the offset. May be an option but I think this would
also have to be exposed to user-space as I believe the same problem would reside
there as well.

> >
> >> > +	ret = iio_map_array_register(indio_dev,
> >da9150_gpadc_default_maps);
> >> > +	if (ret) {
> >> > +		dev_err(dev, "Failed to register IIO maps: %d\n", ret);
> >> > +		return ret;
> >> > +	}
> >> I'd suggest doing the devm_request_thread_irq before the
> >iio_map_array
> >> stuff.  This is purely to avoid the order during remove not being
> >> obviously correct as it isn't the reverse of during probe.
> >
> >Ok, should still work ok that way so can update.
> >
> >> > +static int da9150_gpadc_remove(struct platform_device *pdev)
> >> > +{
> >> > +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >> > +
> >> > +	iio_map_array_unregister(indio_dev);
> >> Twice in one day.  I'm definitely thinking we should add a
> >> devm version of iio_map_array_register...
> >
> >I assume you mean here that iio_device_unregister() should come first?
> >Will
> >update.
> Nope just that such a new function might be useful.

:) Ok fair enough.

> 
> --
> Sent from my Android phone with K-9 Mail. Please excuse my brevity.