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

[Stanimir Varbanov <svarbanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>]

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-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org> 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-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
>>>>> Signed-off-by: Ivan T. Ivanov <iivanov-NEYub+7Iv8PQT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>>  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