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 [off-list ref] wrote:

quoted

Hi Jonathan,

Thanks for the review!

On 09/13/2014 08:27 PM, Jonathan Cameron wrote:

quoted

On 13/09/14 00:27, Hartmut Knaack wrote:

quoted

Stanimir Varbanov schrieb, Am 11.09.2014 17:13:

quoted

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.

quoted

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

quoted

tree reading code rather than doing it in lots of other places. 

Hence

quoted

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 :)

quoted

quoted

Signed-off-by: Stanimir Varbanov <redacted>
Signed-off-by: Ivan T. Ivanov <redacted>
---
 drivers/iio/adc/Kconfig                       |   11 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/qcom-spmi-vadc.c              |  999

+++++++++++++++++++++++++

quoted

quoted

quoted

 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>

quoted

quoted

quoted

quoted

+	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

quoted

all the channels?  Very handy if so, but seems a little unlikely with

15 bit

quoted

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?

quoted

quoted

quoted

quoted

+		rc = IIO_VAL_INT;

return IIO_VAL_INT;

quoted

+		break;
+	default:
+		rc = -EINVAL;
+		break;

Drop default case, or leave empty.

quoted

+	}
+
+	return rc;

return -EINVAL;

quoted

+}

-- 
regards,
Stan