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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>> Signed-off-by: Ivan T. Ivanov <iivanov@xxxxxxxxxx>
>>>>> ---
>>>>> 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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
