RE: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
Hello Jonathan and Stanimir, > >> 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. 'lp8788-charger' registers not charger-manager but power-supply subsystem. 'lp8788-adc' is the IIO driver. 'lp8788-charger' is the IIO consumer of lp8788-adc. (How to communicate between lp8788-adc and lp8788-charger) 1. Application requests vbatt(battery voltage) by accessing /sys/class/power/ 2. lp8788-charger asks lp8788-adc to get converted vbatt value 3. lp8788-adc get the battery ADC from the device. It returns calculated micro voltage to lp8788-charger. 4. lp8788-charger reports this uV value to the application 5. Application uses uV or converts to mV or V unit. It's up to the app. Please note that battery app accesses not /sys/bus/iio but /sys/class/power/. Best regards, Milo
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
>> 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
>> Signed-off-by: Ivan T. Ivanov
>> ---
>> 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
>
>
>
>> +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, _code);
>> +if (rc)
>> +return rc;
>> +
>> +vadc_calibrate(vadc, vchan, );
>> +
>> +*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:
>> +
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
svarba...@mm-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 svarba...@mm-sol.com
Signed-off-by: Ivan T. Ivanov iiva...@mm-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 majord...@vger.kernel.org
More majordomo info at
RE: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
Hello Jonathan and Stanimir, 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. 'lp8788-charger' registers not charger-manager but power-supply subsystem. 'lp8788-adc' is the IIO driver. 'lp8788-charger' is the IIO consumer of lp8788-adc. (How to communicate between lp8788-adc and lp8788-charger) 1. Application requests vbatt(battery voltage) by accessing /sys/class/power/ 2. lp8788-charger asks lp8788-adc to get converted vbatt value 3. lp8788-adc get the battery ADC from the device. It returns calculated micro voltage to lp8788-charger. 4. lp8788-charger reports this uV value to the application 5. Application uses uV or converts to mV or V unit. It's up to the app. Please note that battery app accesses not /sys/bus/iio but /sys/class/power/. Best regards, Milo
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
> 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
> Signed-off-by: Ivan T. Ivanov
> ---
> 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
> + 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, _code);
> + if (rc)
> + return rc;
> +
> + vadc_calibrate(vadc, vchan, );
> +
> + *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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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
svarba...@mm-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 svarba...@mm-sol.com
Signed-off-by: Ivan T. Ivanov iiva...@mm-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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
On September 15, 2014 3:12:50 PM GMT+01:00, Stanimir Varbanov
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
Signed-off-by: Ivan T. Ivanov
---
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
>
>
>
+
+static int
>>> Don't wrap line here.
+vadc_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec
>const *chan,
+int *val, int *val2, long mask)
+{
+ struct vadc_priv *vadc = iio_priv(indio_dev);
+ struct vadc_channel *vchan;
+ struct vadc_result result;
+ int rc;
+
>> It is a bit of a pitty we can't avoid this lookup. Normally I'd
>suggest
>> putting an index in chan->address but you've already used that.
>> The purpose of this is to (I think) allow you to have the private
>> data stored in a random order... What is the benefit of doing that?
>> (see various comments elsewhere)
>
>So the vadc_channels array describe all possible vadc channels for all
>supported PMICs from this driver. On the other side vadc->channels
>pointer should contain only the channels described in DT.
>
>Thus we need a below function to check is the current channel is active
>for the current DT (current PMIC version). This is because we have in
>vadc_channels the full set of channels but not every supported PMIC
>have
>support for them.
>
>I agree that this peace of code is not so clear. So I will try to
>rework
>this and register to the IIO core only those channels that have channel
>descriptions in DT.
Cool
>
>Also I wonder can I use iio_chan_spec::address field as a pointer to
>private structure with vadc channel properties like decimation,
>prescale
>etc. got from DT or the default values.
Yes or an index into an array of them perhaps?
>
>>
+ 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, _code);
+ if (rc)
+ return rc;
+
+ vadc_calibrate(vadc, vchan, );
+
+ *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.
>
+ rc = IIO_VAL_INT;
>>> return IIO_VAL_INT;
+ break;
+ default:
+ rc = -EINVAL;
+ break;
>>> Drop default case, or leave empty.
+ }
+
+ return rc;
>>> return -EINVAL;
+}
+
+static const struct iio_info vadc_info = {
+ .read_raw = vadc_read_raw,
+ .driver_module = THIS_MODULE,
+};
+
+#define VADC_CHAN(_id, _pre)
\
+ [VADC_##_id] = {
Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
Thanks for the comments! On 09/13/2014 02:27 AM, 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. OK, I will take into account your comments. -- regards, Stan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
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
>>> Signed-off-by: Ivan T. Ivanov
>>> ---
>>> 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
>>> +
>>> +static int
>> Don't wrap line here.
>>> +vadc_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
>>> + int *val, int *val2, long mask)
>>> +{
>>> + struct vadc_priv *vadc = iio_priv(indio_dev);
>>> + struct vadc_channel *vchan;
>>> + struct vadc_result result;
>>> + int rc;
>>> +
> It is a bit of a pitty we can't avoid this lookup. Normally I'd suggest
> putting an index in chan->address but you've already used that.
> The purpose of this is to (I think) allow you to have the private
> data stored in a random order... What is the benefit of doing that?
> (see various comments elsewhere)
So the vadc_channels array describe all possible vadc channels for all
supported PMICs from this driver. On the other side vadc->channels
pointer should contain only the channels described in DT.
Thus we need a below function to check is the current channel is active
for the current DT (current PMIC version). This is because we have in
vadc_channels the full set of channels but not every supported PMIC have
support for them.
I agree that this peace of code is not so clear. So I will try to rework
this and register to the IIO core only those channels that have channel
descriptions in DT.
Also I wonder can I use iio_chan_spec::address field as a pointer to
private structure with vadc channel properties like decimation, prescale
etc. got from DT or the default values.
>
>>> + 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, _code);
>>> + if (rc)
>>> + return rc;
>>> +
>>> + vadc_calibrate(vadc, vchan, );
>>> +
>>> + *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?
>>> + rc = IIO_VAL_INT;
>> return IIO_VAL_INT;
>>> + break;
>>> + default:
>>> + rc = -EINVAL;
>>> + break;
>> Drop default case, or leave empty.
>>> + }
>>> +
>>> + return rc;
>> return -EINVAL;
>>> +}
>>> +
>>> +static const struct iio_info vadc_info = {
>>> + .read_raw = vadc_read_raw,
>>> + .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +#define VADC_CHAN(_id, _pre)
>>> \
>>> + [VADC_##_id] = {\
>>> + .type = IIO_VOLTAGE,\
> A few of the below look to be temp sensors. If they are hardwired
> in some way to this functionality (i.e. is it on chip) then it might be
> nice to reflect this in the channel type.
Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
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 svarba...@mm-sol.com
Signed-off-by: Ivan T. Ivanov iiva...@mm-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
+
+static int
Don't wrap line here.
+vadc_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct vadc_priv *vadc = iio_priv(indio_dev);
+ struct vadc_channel *vchan;
+ struct vadc_result result;
+ int rc;
+
It is a bit of a pitty we can't avoid this lookup. Normally I'd suggest
putting an index in chan-address but you've already used that.
The purpose of this is to (I think) allow you to have the private
data stored in a random order... What is the benefit of doing that?
(see various comments elsewhere)
So the vadc_channels array describe all possible vadc channels for all
supported PMICs from this driver. On the other side vadc-channels
pointer should contain only the channels described in DT.
Thus we need a below function to check is the current channel is active
for the current DT (current PMIC version). This is because we have in
vadc_channels the full set of channels but not every supported PMIC have
support for them.
I agree that this peace of code is not so clear. So I will try to rework
this and register to the IIO core only those channels that have channel
descriptions in DT.
Also I wonder can I use iio_chan_spec::address field as a pointer to
private structure with vadc channel properties like decimation, prescale
etc. got from DT or the default values.
+ 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?
+ rc = IIO_VAL_INT;
return IIO_VAL_INT;
+ break;
+ default:
+ rc = -EINVAL;
+ break;
Drop default case, or leave empty.
+ }
+
+ return rc;
return -EINVAL;
+}
+
+static const struct iio_info vadc_info = {
+ .read_raw = vadc_read_raw,
+ .driver_module = THIS_MODULE,
+};
+
+#define VADC_CHAN(_id, _pre)
\
+ [VADC_##_id] = {\
+ .type = IIO_VOLTAGE,\
A few of the below look to be temp sensors. If they are hardwired
in some way to this functionality (i.e. is it on chip) then it might be
nice to reflect this in the channel type.
There are a dedicated channels to measure temperature. Those channels
have connected thermistor but I don't think it is on chip. So the
returned adc code is in microvolts and we have a translation
Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
Thanks for the comments! On 09/13/2014 02:27 AM, 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. OK, I will take into account your comments. snip -- regards, Stan -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
On September 15, 2014 3:12:50 PM GMT+01:00, Stanimir Varbanov
svarba...@mm-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 svarba...@mm-sol.com
Signed-off-by: Ivan T. Ivanov iiva...@mm-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
+
+static int
Don't wrap line here.
+vadc_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec
const *chan,
+int *val, int *val2, long mask)
+{
+ struct vadc_priv *vadc = iio_priv(indio_dev);
+ struct vadc_channel *vchan;
+ struct vadc_result result;
+ int rc;
+
It is a bit of a pitty we can't avoid this lookup. Normally I'd
suggest
putting an index in chan-address but you've already used that.
The purpose of this is to (I think) allow you to have the private
data stored in a random order... What is the benefit of doing that?
(see various comments elsewhere)
So the vadc_channels array describe all possible vadc channels for all
supported PMICs from this driver. On the other side vadc-channels
pointer should contain only the channels described in DT.
Thus we need a below function to check is the current channel is active
for the current DT (current PMIC version). This is because we have in
vadc_channels the full set of channels but not every supported PMIC
have
support for them.
I agree that this peace of code is not so clear. So I will try to
rework
this and register to the IIO core only those channels that have channel
descriptions in DT.
Cool
Also I wonder can I use iio_chan_spec::address field as a pointer to
private structure with vadc channel properties like decimation,
prescale
etc. got from DT or the default values.
Yes or an index into an array of them perhaps?
+ 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.
+ rc = IIO_VAL_INT;
return IIO_VAL_INT;
+ break;
+ default:
+ rc = -EINVAL;
+ break;
Drop default case, or leave empty.
+ }
+
+ return rc;
return -EINVAL;
+}
+
+static const struct iio_info vadc_info = {
+ .read_raw = vadc_read_raw,
+ .driver_module = THIS_MODULE,
+};
+
+#define VADC_CHAN(_id, _pre)
\
+ [VADC_##_id] = {\
+ .type = IIO_VOLTAGE,\
A few of the below look to be temp sensors. If they are hardwired
in some way to this functionality (i.e. is it on chip) then it might
be
nice to reflect this in the channel type.
There
Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
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 >> Signed-off-by: Ivan T. Ivanov >> --- >> 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 >> >> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig >> index 11b048a..08700d4 100644 >> --- a/drivers/iio/adc/Kconfig >> +++ b/drivers/iio/adc/Kconfig >> @@ -206,6 +206,17 @@ config NAU7802 >>To compile this driver as a module, choose M here: the >>module will be called nau7802. >> >> +config QCOM_SPMI_VADC >> +tristate "Qualcomm SPMI PMIC voltage ADC" >> +depends on SPMI >> +help >> + Say yes here if you want support for the Qualcomm SPMI PMIC voltage >> ADC. >> + >> + The driver supports reading the HKADC, XOADC through the ADC AMUX >> arbiter. >> + The VADC includes support for the conversion sequencer. The driver >> + supports reading the ADC through the AMUX channels for external >> pull-ups >> + simultaneously. >> + >> config TI_ADC081C >> tristate "Texas Instruments ADC081C021/027" >> depends on I2C >> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile >> index ad81b51..d5d18f4 100644 >> --- a/drivers/iio/adc/Makefile >> +++ b/drivers/iio/adc/Makefile >> @@ -22,6 +22,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o >> obj-$(CONFIG_MCP3422) += mcp3422.o >> obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o >> obj-$(CONFIG_NAU7802) += nau7802.o >> +obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o >> obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o >> obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o >> obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o >> diff --git a/drivers/iio/adc/qcom-spmi-vadc.c >> b/drivers/iio/adc/qcom-spmi-vadc.c >> new file mode 100644 >> index 000..e30eb04 >> --- /dev/null >> +++ b/drivers/iio/adc/qcom-spmi-vadc.c >> @@ -0,0 +1,999 @@ >> +/* >> + * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +/* VADC register and bit definition */ >> +#define VADC_REVISION2 0x1 >> +#define VADC_REVISION2_SUPPORTED_VADC 1 >> + >> +#define VADC_PERPH_TYPE 0x4 >> +#define VADC_PERPH_TYPE_ADC 8 >> + >> +#define VADC_PERPH_SUBTYPE 0x5 >> +#define VADC_PERPH_SUBTYPE_VADC 1 >> + >> +#define VADC_STATUS10x8 >> +#define VADC_STATUS1_OP_MODE4 >> +#define VADC_STATUS1_REQ_STSBIT(1) >> +#define VADC_STATUS1_EOCBIT(0) >> +#define VADC_STATUS1_REQ_STS_EOC_MASK 0x3 >> + >> +#define VADC_MODE_CTL 0x40 >> +#define VADC_OP_MODE_SHIFT 3 >> +#define
Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
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 svarba...@mm-sol.com Signed-off-by: Ivan T. Ivanov iiva...@mm-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 diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 11b048a..08700d4 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -206,6 +206,17 @@ config NAU7802 To compile this driver as a module, choose M here: the module will be called nau7802. +config QCOM_SPMI_VADC +tristate Qualcomm SPMI PMIC voltage ADC +depends on SPMI +help + Say yes here if you want support for the Qualcomm SPMI PMIC voltage ADC. + + The driver supports reading the HKADC, XOADC through the ADC AMUX arbiter. + The VADC includes support for the conversion sequencer. The driver + supports reading the ADC through the AMUX channels for external pull-ups + simultaneously. + config TI_ADC081C tristate Texas Instruments ADC081C021/027 depends on I2C diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index ad81b51..d5d18f4 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -22,6 +22,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o obj-$(CONFIG_MCP3422) += mcp3422.o obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o obj-$(CONFIG_NAU7802) += nau7802.o +obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c new file mode 100644 index 000..e30eb04 --- /dev/null +++ b/drivers/iio/adc/qcom-spmi-vadc.c @@ -0,0 +1,999 @@ +/* + * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include linux/bitops.h +#include linux/completion.h +#include linux/delay.h +#include linux/err.h +#include linux/iio/iio.h +#include linux/interrupt.h +#include linux/kernel.h +#include linux/module.h +#include linux/of.h +#include linux/platform_device.h +#include linux/regmap.h +#include linux/slab.h +#include linux/log2.h + +#include dt-bindings/iio/qcom,spmi-pmic-vadc.h + +/* VADC register and bit definition */ +#define VADC_REVISION2 0x1 +#define VADC_REVISION2_SUPPORTED_VADC 1 + +#define VADC_PERPH_TYPE 0x4 +#define VADC_PERPH_TYPE_ADC 8 + +#define VADC_PERPH_SUBTYPE 0x5 +#define VADC_PERPH_SUBTYPE_VADC 1 + +#define VADC_STATUS10x8 +#define VADC_STATUS1_OP_MODE4 +#define VADC_STATUS1_REQ_STSBIT(1) +#define VADC_STATUS1_EOCBIT(0) +#define VADC_STATUS1_REQ_STS_EOC_MASK 0x3 + +#define VADC_MODE_CTL 0x40 +#define VADC_OP_MODE_SHIFT
Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
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. > Signed-off-by: Stanimir Varbanov > Signed-off-by: Ivan T. Ivanov > --- > 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 > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 11b048a..08700d4 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -206,6 +206,17 @@ config NAU7802 > To compile this driver as a module, choose M here: the > module will be called nau7802. > > +config QCOM_SPMI_VADC > + tristate "Qualcomm SPMI PMIC voltage ADC" > + depends on SPMI > + help > + Say yes here if you want support for the Qualcomm SPMI PMIC voltage > ADC. > + > + The driver supports reading the HKADC, XOADC through the ADC AMUX > arbiter. > + The VADC includes support for the conversion sequencer. The driver > + supports reading the ADC through the AMUX channels for external > pull-ups > + simultaneously. > + > config TI_ADC081C > tristate "Texas Instruments ADC081C021/027" > depends on I2C > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index ad81b51..d5d18f4 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -22,6 +22,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o > obj-$(CONFIG_MCP3422) += mcp3422.o > obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o > obj-$(CONFIG_NAU7802) += nau7802.o > +obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o > obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o > obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o > obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o > diff --git a/drivers/iio/adc/qcom-spmi-vadc.c > b/drivers/iio/adc/qcom-spmi-vadc.c > new file mode 100644 > index 000..e30eb04 > --- /dev/null > +++ b/drivers/iio/adc/qcom-spmi-vadc.c > @@ -0,0 +1,999 @@ > +/* > + * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +/* VADC register and bit definition */ > +#define VADC_REVISION2 0x1 > +#define VADC_REVISION2_SUPPORTED_VADC1 > + > +#define VADC_PERPH_TYPE 0x4 > +#define VADC_PERPH_TYPE_ADC 8 > + > +#define VADC_PERPH_SUBTYPE 0x5 > +#define VADC_PERPH_SUBTYPE_VADC 1 > + > +#define VADC_STATUS1 0x8 > +#define VADC_STATUS1_OP_MODE 4 > +#define VADC_STATUS1_REQ_STS BIT(1) > +#define VADC_STATUS1_EOC BIT(0) > +#define VADC_STATUS1_REQ_STS_EOC_MASK0x3 > + > +#define VADC_MODE_CTL0x40 > +#define VADC_OP_MODE_SHIFT 3 > +#define VADC_OP_MODE_NORMAL 0 > +#define VADC_AMUX_TRIM_ENBIT(1) > +#define VADC_ADC_TRIM_EN BIT(0) > + > +#define VADC_EN_CTL1 0x46 > +#define VADC_EN_CTL1_SET BIT(7) > + > +#define VADC_ADC_CH_SEL_CTL 0x48 > + > +#define VADC_ADC_DIG_PARAM 0x50 > +#define VADC_ADC_DIG_DEC_RATIO_SEL_SHIFT 2 > + > +#define VADC_HW_SETTLE_DELAY 0x51 > + > +#define VADC_CONV_REQ0x52 > +#define VADC_CONV_REQ_SETBIT(7) > + > +#define VADC_FAST_AVG_CTL0x5a >
Re: [PATCH v2 1/2] iio: vadc: Qualcomm SPMI PMIC voltage ADC driver
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. Signed-off-by: Stanimir Varbanov svarba...@mm-sol.com Signed-off-by: Ivan T. Ivanov iiva...@mm-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 diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 11b048a..08700d4 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -206,6 +206,17 @@ config NAU7802 To compile this driver as a module, choose M here: the module will be called nau7802. +config QCOM_SPMI_VADC + tristate Qualcomm SPMI PMIC voltage ADC + depends on SPMI + help + Say yes here if you want support for the Qualcomm SPMI PMIC voltage ADC. + + The driver supports reading the HKADC, XOADC through the ADC AMUX arbiter. + The VADC includes support for the conversion sequencer. The driver + supports reading the ADC through the AMUX channels for external pull-ups + simultaneously. + config TI_ADC081C tristate Texas Instruments ADC081C021/027 depends on I2C diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index ad81b51..d5d18f4 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -22,6 +22,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o obj-$(CONFIG_MCP3422) += mcp3422.o obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o obj-$(CONFIG_NAU7802) += nau7802.o +obj-$(CONFIG_QCOM_SPMI_VADC) += qcom-spmi-vadc.o obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o diff --git a/drivers/iio/adc/qcom-spmi-vadc.c b/drivers/iio/adc/qcom-spmi-vadc.c new file mode 100644 index 000..e30eb04 --- /dev/null +++ b/drivers/iio/adc/qcom-spmi-vadc.c @@ -0,0 +1,999 @@ +/* + * Copyright (c) 2012-2014, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include linux/bitops.h +#include linux/completion.h +#include linux/delay.h +#include linux/err.h +#include linux/iio/iio.h +#include linux/interrupt.h +#include linux/kernel.h +#include linux/module.h +#include linux/of.h +#include linux/platform_device.h +#include linux/regmap.h +#include linux/slab.h +#include linux/log2.h + +#include dt-bindings/iio/qcom,spmi-pmic-vadc.h + +/* VADC register and bit definition */ +#define VADC_REVISION2 0x1 +#define VADC_REVISION2_SUPPORTED_VADC1 + +#define VADC_PERPH_TYPE 0x4 +#define VADC_PERPH_TYPE_ADC 8 + +#define VADC_PERPH_SUBTYPE 0x5 +#define VADC_PERPH_SUBTYPE_VADC 1 + +#define VADC_STATUS1 0x8 +#define VADC_STATUS1_OP_MODE 4 +#define VADC_STATUS1_REQ_STS BIT(1) +#define VADC_STATUS1_EOC BIT(0) +#define VADC_STATUS1_REQ_STS_EOC_MASK0x3 + +#define VADC_MODE_CTL0x40 +#define VADC_OP_MODE_SHIFT 3 +#define VADC_OP_MODE_NORMAL 0 +#define VADC_AMUX_TRIM_ENBIT(1) +#define VADC_ADC_TRIM_EN BIT(0) + +#define VADC_EN_CTL1 0x46 +#define VADC_EN_CTL1_SET BIT(7) + +#define VADC_ADC_CH_SEL_CTL 0x48 + +#define VADC_ADC_DIG_PARAM 0x50 +#define VADC_ADC_DIG_DEC_RATIO_SEL_SHIFT 2 + +#define VADC_HW_SETTLE_DELAY 0x51 + +#define VADC_CONV_REQ
