linux-kernel - Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver

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]
Message-ID: <541ED2C0.4040801@kernel.org>
Date:	Sun, 21 Sep 2014 14:29:36 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Stanimir Varbanov <svarbanov@...sol.com>
CC:	Hartmut Knaack <knaack.h@....de>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Pawel Moll <pawel.moll@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Kumar Gala <galak@...eaurora.org>,
	Mark Rutland <mark.rutland@....com>,
	Grant Likely <grant.likely@...aro.org>,
	Arnd Bergmann <arnd@...db.de>, linux-arm-msm@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Angelo Compagnucci <angelo.compagnucci@...il.com>,
	Doug Anderson <dianders@...omium.org>,
	Fugang Duan <B38611@...escale.com>,
	Johannes Thumshirn <johannes.thumshirn@....de>,
	Jean Delvare <jdelvare@...e.de>,
	Philippe Reynes <tremyfr@...oo.fr>,
	Lee Jones <lee.jones@...aro.org>,
	Josh Cartwright <joshc@...eaurora.org>,
	Stephen Boyd <sboyd@...eaurora.org>,
	David Collins <collinsd@...eaurora.org>,
	"Ivan T. Ivanov" <iivanov@...sol.com>,
	"Kim, Milo" <Milo.Kim@...com>
Subject: Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver

On 18/09/14 10:57, Stanimir Varbanov wrote:
> Hi Jonathan,
> 
> On 09/15/2014 07:11 PM, Jonathan Cameron wrote:
>>
>>
>> On September 15, 2014 3:12:50 PM GMT+01:00, Stanimir Varbanov <svarbanov@...sol.com> wrote:
>>> Hi Jonathan,
>>>
>>> Thanks for the review!
>>>
>>> On 09/13/2014 08:27 PM, Jonathan Cameron wrote:
>>>> On 13/09/14 00:27, Hartmut Knaack wrote:
>>>>> Stanimir Varbanov schrieb, Am 11.09.2014 17:13:
>>>>>> The voltage ADC is peripheral of Qualcomm SPMI PMIC chips. It has
>>>>>> 15bits resolution and register space inside PMIC accessible across
>>>>>> SPMI bus.
>>>>>>
>>>>>> The vadc driver registers itself through IIO interface.
>>>>>>
>>>>> Looks already pretty good. Things you should consider in regard of
>>> common coding style are to use the variable name ret instead of rc,
>>> since it is used in almost all adc drivers and thus makes reviewing a
>>> bit easier. Besides that, you seem to use unsigned as well as unsigned
>>> int, so to be consistent, please stick to one of them. Other comments
>>> in line.
>>>>
>>>> A few additional comments from me.  My biggest question is whether
>>>> you are actually making life difficult for yourself by having
>>>> vadc_channels and vadc->channels (don't like the similar naming btw!)
>>>> in different orders.  I think you can move the ordering into the
>>> device
>>>> tree reading code rather than doing it in lots of other places. 
>>> Hence
>>>> rather than an order based on the device tree description, put the
>>>> data into a fixed ofer in vadc->channels.
>>>>
>>>> Entirely possible I'm missing something though :)
>>>>>> Signed-off-by: Stanimir Varbanov <svarbanov@...sol.com>
>>>>>> Signed-off-by: Ivan T. Ivanov <iivanov@...sol.com>
>>>>>> ---
>>>>>>  drivers/iio/adc/Kconfig                       |   11 +
>>>>>>  drivers/iio/adc/Makefile                      |    1 +
>>>>>>  drivers/iio/adc/qcom-spmi-vadc.c              |  999
>>> +++++++++++++++++++++++++
>>>>>>  include/dt-bindings/iio/qcom,spmi-pmic-vadc.h |  119 +++
>>>>>>  4 files changed, 1130 insertions(+), 0 deletions(-)
>>>>>>  create mode 100644 drivers/iio/adc/qcom-spmi-vadc.c
>>>>>>  create mode 100644 include/dt-bindings/iio/qcom,spmi-pmic-vadc.h
> 
> <snip>
> 
>>>>>> +	vchan = vadc_find_channel(vadc, chan->channel);
>>>>>> +	if (!vchan)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	if (!vadc->is_ref_measured) {
>>>>>> +		rc = vadc_measure_reference_points(vadc);
>>>>>> +		if (rc)
>>>>>> +			return rc;
>>>>>> +
>>>>>> +		vadc->is_ref_measured = true;
>>>>>> +	}
>>>>>> +
>>>>>> +	switch (mask) {
>>>>>> +	case IIO_CHAN_INFO_PROCESSED:
>>>>>> +		rc = vadc_do_conversion(vadc, vchan, &result.adc_code);
>>>>>> +		if (rc)
>>>>>> +			return rc;
>>>>>> +
>>>>>> +		vadc_calibrate(vadc, vchan, &result);
>>>>>> +
>>>>>> +		*val = result.physical;
>>>> I'm a little suspicious here.  Are the resulting values in milivolts
>>> for
>>>> all the channels?  Very handy if so, but seems a little unlikely with
>>> 15 bit
>>>> ADC that you'd have no part of greater accuracy than a milivolt.
>>>
>>> In fact *val is in microvolts. What is the expected unit from IIO ADC
>>> users?
>> See Documentation/ABI/sysfs-bus-iio
>> Millivolts I think... We copied hwmon where possible.
> 
> I'm a bit confused about these units. I searched references of
> iio_read_channel_processed() and found a few.
> 
> The iio_hwmon expecting milivolts. On the other side lp8788-charger.c
> registers a get_property method in charger-manager.c, which expects
> microvolts in get_batt_uV().
It's definitely meant to be millivolts (lifted from hwmon a while back).
See Documentation/ABI/testing/sysfs-bus-iio

Looks like we have a bug in lp8788-charger - it might be matched with
one in lp8788-adc, but then there will be a bug there...

Cc'd Milo Kim.

> 
> I also wonder how to implement IIO read_raw() method so as not to lose
> precision (if assume we must return millivolts). As far I can see it
> should be some combination of IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_SCALE
> masks. Any hints?

Use val and val2 and a return type of IIO_VAL_INT_PLUS_MICRO.
So if you have n microvolts

val = n div 1000;
val2 = n mod 1000 * 1000;
This makes sense if you are returning a processed value.
Otherwise, use a scale infomask element (and support in read raw) to
ensure that rawvalue*scale is in millivolts
So if n is in microvolts it would simply be 1000.
(this would all have been more obvious if we hadn't copied hwmon and
had just used volts, but such is history).

As for in kernel users...
iio_read_channel_raw and iio_read_channel_processed assume we are looking
for an integer value out, so you'll want to call iio_read_channel_raw and
apply iio_convert_raw_to_processed with appropriately adjusted scale value.

> 
>>>
>>>>>> +		rc = IIO_VAL_INT;
>>>>> return IIO_VAL_INT;
>>>>>> +		break;
>>>>>> +	default:
>>>>>> +		rc = -EINVAL;
>>>>>> +		break;
>>>>> Drop default case, or leave empty.
>>>>>> +	}
>>>>>> +
>>>>>> +	return rc;
>>>>> return -EINVAL;
>>>>>> +}
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/