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]
Date:	Thu, 18 Sep 2014 12:57:20 +0300
From:	Stanimir Varbanov <svarbanov@...sol.com>
To:	Jonathan Cameron <jic23@...nel.org>
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>
Subject: Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver

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().

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?

>>
>>>>> +		rc = IIO_VAL_INT;
>>>> return IIO_VAL_INT;
>>>>> +		break;
>>>>> +	default:
>>>>> +		rc = -EINVAL;
>>>>> +		break;
>>>> Drop default case, or leave empty.
>>>>> +	}
>>>>> +
>>>>> +	return rc;
>>>> return -EINVAL;
>>>>> +}


-- 
regards,
Stan
--
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/