[PATCH 1/2] dt-bindings: iio: adc: Add DT binding document for PMIC5 ADC

[PATCH 1/2] dt-bindings: iio: adc: Add DT binding document for PMIC5 ADC

[Siddartha Mohanadoss]

PMIC5 ADC has support for clients to measure voltage and current
on inputs connected to the PMIC. Clients include reading voltage
phone power and on board system thermistors for thermal management.
ADC5 on certain PMIC has support to read battery current.

This change adds documentation.

Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
---
 .../devicetree/bindings/iio/adc/qcom,spmi-adc5.txt | 137 +++++++++++++++++++++
 1 file changed, 137 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt

diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
new file mode 100644
index 0000000..c9268ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
@@ -0,0 +1,137 @@
+Qualcomm Technologies Inc. SPMI PMIC5 voltage and current ADC
+
+SPMI PMIC5 voltage ADC (ADC) provides interface to clients to read
+voltage. The VADC is a 16-bit sigma-delta ADC.
+
+ADC node:
+
+- compatible:
+    Usage: required
+    Value type: <string>
+    Definition: Should contain "qcom,spmi-adc5" for PMIC5 ADC driver.
+		Should contain "qcom,spmi-adc-rev2" for PMIC refresh ADC driver.
+
+- reg:
+    Usage: required
+    Value type: <prop-encoded-array>
+    Definition: VADC base address and length in the SPMI PMIC register map.
+
+- #address-cells:
+    Usage: required
+    Value type: <u32>
+    Definition: Must be one. Child node 'reg' property should define ADC
+            channel number.
+
+- #size-cells:
+    Usage: required
+    Value type: <u32>
+    Definition: Must be zero.
+
+- #io-channel-cells:
+    Usage: required
+    Value type: <u32>
+    Definition: Must be one. For details about IIO bindings see:
+            Documentation/devicetree/bindings/iio/iio-bindings.txt
+
+- interrupts:
+    Usage: optional
+    Value type: <prop-encoded-array>
+    Definition: End of conversion interrupt.
+
+Channel node properties:
+
+- reg:
+    Usage: required
+    Value type: <u32>
+    Definition: ADC channel number.
+            See include/dt-bindings/iio/qcom,spmi-vadc.h
+
+- label:
+    Usage: required
+    Value type: <empty>
+    Definition: ADC datasheet channel name.
+            For thermistor inputs connected to generic AMUX or GPIO inputs
+            these can vary across platform for the same pins. Hence select
+            the datasheet name for this channel.
+
+- qcom,pre-scaling:
+    Usage: required
+    Value type: <u32 array>
+    Definition: Used for scaling the channel input signal before the signal is
+            fed to VADC. The configuration for this node is to know the
+            pre-determined ratio and use it for post scaling. Select one from
+            the following options.
+            <1 1>, <1 3>, <1 4>, <1 6>, <1 20>, <1 8>, <10 81>, <1 10>
+            If property is not found default value depending on chip will be used.
+
+- qcom,decimation:
+    Usage: optional
+    Value type: <u32>
+    Definition: This parameter is used to decrease ADC sampling rate.
+            Quicker measurements can be made by reducing decimation ratio.
+            For PMIC5 ADC, combined two step decimation values are 250, 420 and 840.
+            If property is not found, default value of 840 will be used.
+	    For PMIC refresh ADC, supported decimation values are 256, 512, 1024.
+	    If property is not found, default value of 1024 will be used.
+
+- qcom,ratiometric:
+    Usage: optional
+    Value type: <empty>
+    Definition: Channel calibration type. If this property is specified
+            VADC will use the VDD reference (1.875V) and GND for channel
+            calibration. If property is not found, channel will be
+            calibrated with 0V and 1.25V reference channels, also
+            known as absolute calibration.
+
+- qcom,hw-settle-time:
+    Usage: optional
+    Value type: <u32>
+    Definition: Time between AMUX getting configured and the ADC starting
+            conversion.
+	    For PMIC5, delay = 15us for value 0,
+			100us * (value) for values 0 < value < 11, and
+            		2ms * (value - 10) otherwise.
+            Valid values are: 15, 100, 200, 300, 400, 500, 600, 700, 800,
+            900 us and 1, 2, 4, 6, 8, 10 ms
+            If property is not found, channel will use 15us.
+	    For PMIC rev2, delay = 100us * (value) for values 0 < value < 11, and
+			2ms * (value - 10) otherwise.
+            Valid values are: 0, 100, 200, 300, 400, 500, 600, 700, 800,
+            900 us and 1, 2, 4, 6, 8, 10 ms
+            If property is not found, channel will use 0 us.
+
+- qcom,avg-samples:
+    Usage: optional
+    Value type: <u32>
+    Definition: Number of samples to be used for measurement.
+            Averaging provides the option to obtain a single measurement
+            from the ADC that is an average of multiple samples. The value
+            selected is 2^(value).
+            Valid values are: 1, 2, 4, 8, 16
+            If property is not found, 1 sample will be used.
+
+Example:
+
+        /* VADC node */
+        pmic_vadc: vadc@3100 {
+                compatible = "qcom,spmi-adc5";
+                reg = <0x3100 0x100>;
+                interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
+                #address-cells = <1>;
+                #size-cells = <0>;
+                #io-channel-cells = <1>;
+                io-channel-ranges;
+
+                /* Channel node */
+                vph_pwr {
+                        reg = <ADC_VPH_PWR>;
+                        label = "vph_pwr";
+                        qcom,pre-scaling = <1 3>;
+                };
+        };
+
+        /* IIO client node */
+        usb {
+                io-channels = <&pmic_vadc ADC_VPH_PWR>;
+                io-channel-names = "vadc";
+        };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] dt-bindings: iio: adc: Add DT binding document for PMIC5 ADC

[Jonathan Cameron]

On Tue,  8 May 2018 14:38:21 -0700
Siddartha Mohanadoss <smohanad@codeaurora.org> wrote:

> PMIC5 ADC has support for clients to measure voltage and current
> on inputs connected to the PMIC. Clients include reading voltage
> phone power and on board system thermistors for thermal management.
> ADC5 on certain PMIC has support to read battery current.
> 
> This change adds documentation.
> 
> Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
Hi Siddartha,

Some complexity in here!  Anyhow, a few comments inline and we will definitely
be wanting guidance from the devicetree people for this one.

Jonathan

> ---
>  .../devicetree/bindings/iio/adc/qcom,spmi-adc5.txt | 137 +++++++++++++++++++++
>  1 file changed, 137 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
> new file mode 100644
> index 0000000..c9268ba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
> @@ -0,0 +1,137 @@
> +Qualcomm Technologies Inc. SPMI PMIC5 voltage and current ADC
> +
> +SPMI PMIC5 voltage ADC (ADC) provides interface to clients to read
> +voltage. The VADC is a 16-bit sigma-delta ADC.
> +
> +ADC node:
> +
> +- compatible:
> +    Usage: required
> +    Value type: <string>
> +    Definition: Should contain "qcom,spmi-adc5" for PMIC5 ADC driver.
> +		Should contain "qcom,spmi-adc-rev2" for PMIC refresh ADC driver.
> +
> +- reg:
> +    Usage: required
> +    Value type: <prop-encoded-array>
> +    Definition: VADC base address and length in the SPMI PMIC register map.
> +
> +- #address-cells:
> +    Usage: required
> +    Value type: <u32>
> +    Definition: Must be one. Child node 'reg' property should define ADC
> +            channel number.
> +
> +- #size-cells:
> +    Usage: required
> +    Value type: <u32>
> +    Definition: Must be zero.
> +
> +- #io-channel-cells:
> +    Usage: required
> +    Value type: <u32>
> +    Definition: Must be one. For details about IIO bindings see:
> +            Documentation/devicetree/bindings/iio/iio-bindings.txt
> +
> +- interrupts:
> +    Usage: optional
> +    Value type: <prop-encoded-array>
> +    Definition: End of conversion interrupt.
> +
> +Channel node properties:
> +
> +- reg:
> +    Usage: required
> +    Value type: <u32>
> +    Definition: ADC channel number.
> +            See include/dt-bindings/iio/qcom,spmi-vadc.h
> +
> +- label:
> +    Usage: required
> +    Value type: <empty>
> +    Definition: ADC datasheet channel name.
> +            For thermistor inputs connected to generic AMUX or GPIO inputs
> +            these can vary across platform for the same pins. Hence select
> +            the datasheet name for this channel.
> +
> +- qcom,pre-scaling:
> +    Usage: required
> +    Value type: <u32 array>
> +    Definition: Used for scaling the channel input signal before the signal is
> +            fed to VADC. The configuration for this node is to know the
> +            pre-determined ratio and use it for post scaling. Select one from
> +            the following options.
> +            <1 1>, <1 3>, <1 4>, <1 6>, <1 20>, <1 8>, <10 81>, <1 10>
> +            If property is not found default value depending on chip will be used.
> +
> +- qcom,decimation:
> +    Usage: optional
> +    Value type: <u32>
> +    Definition: This parameter is used to decrease ADC sampling rate.
> +            Quicker measurements can be made by reducing decimation ratio.
> +            For PMIC5 ADC, combined two step decimation values are 250, 420 and 840.
> +            If property is not found, default value of 840 will be used.

The odd indenting here needs sorting.  Mixture of spaces and tabs at the moment.

Hmm. In someways this is a policy decision so should be pushed up to userspace,
but given the 'right' value will be somewhat dependent on what you are doing
with the channel and what is wired to it, it could arguably have a 'right' value
for a given circuit.  This is really just the sampling frequency wrapped
up in decimation of something, I'm guessing some input clock?

Let's see what the Device-tree people think on this one!  Personally I have
never really minded devicetree providing sensible defaults.  We can put
control on these things later, if there is a usecase for changing them.

> +	    For PMIC refresh ADC, supported decimation values are 256, 512, 1024.
> +	    If property is not found, default value of 1024 will be used.
> +
> +- qcom,ratiometric:
> +    Usage: optional
> +    Value type: <empty>
> +    Definition: Channel calibration type. If this property is specified
> +            VADC will use the VDD reference (1.875V) and GND for channel
> +            calibration. If property is not found, channel will be
> +            calibrated with 0V and 1.25V reference channels, also
> +            known as absolute calibration.
> +
> +- qcom,hw-settle-time:
> +    Usage: optional
> +    Value type: <u32>
> +    Definition: Time between AMUX getting configured and the ADC starting
> +            conversion.
> +	    For PMIC5, delay = 15us for value 0,
> +			100us * (value) for values 0 < value < 11, and
> +            		2ms * (value - 10) otherwise.
> +            Valid values are: 15, 100, 200, 300, 400, 500, 600, 700, 800,

This description is very confusing given the different uses of 'value'
None of the values you have allowed is less than 11 so the first condition
doesn't apply.

> +            900 us and 1, 2, 4, 6, 8, 10 ms
> +            If property is not found, channel will use 15us.
> +	    For PMIC rev2, delay = 100us * (value) for values 0 < value < 11, and
> +			2ms * (value - 10) otherwise.
> +            Valid values are: 0, 100, 200, 300, 400, 500, 600, 700, 800,
> +            900 us and 1, 2, 4, 6, 8, 10 ms
> +            If property is not found, channel will use 0 us.
> +
> +- qcom,avg-samples:
> +    Usage: optional
> +    Value type: <u32>
> +    Definition: Number of samples to be used for measurement.
> +            Averaging provides the option to obtain a single measurement
> +            from the ADC that is an average of multiple samples. The value
> +            selected is 2^(value).
> +            Valid values are: 1, 2, 4, 8, 16
> +            If property is not found, 1 sample will be used.

As with decimation, this is arguably not a feature of the hardware, but
a software decision...

> +
> +Example:
> +
> +        /* VADC node */
> +        pmic_vadc: vadc@3100 {
> +                compatible = "qcom,spmi-adc5";
> +                reg = <0x3100 0x100>;
> +                interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +                #io-channel-cells = <1>;
> +                io-channel-ranges;
> +
> +                /* Channel node */
> +                vph_pwr {
> +                        reg = <ADC_VPH_PWR>;
> +                        label = "vph_pwr";
> +                        qcom,pre-scaling = <1 3>;
> +                };
> +        };
> +
> +        /* IIO client node */
> +        usb {
> +                io-channels = <&pmic_vadc ADC_VPH_PWR>;
> +                io-channel-names = "vadc";
> +        };

Re: [PATCH 1/2] dt-bindings: iio: adc: Add DT binding document for PMIC5 ADC

[Siddartha Mohanadoss]

Hi Jonathan,

Thanks for the comments.


On 05/12/2018 03:15 AM, Jonathan Cameron wrote:
> On Tue,  8 May 2018 14:38:21 -0700
> Siddartha Mohanadoss <smohanad@codeaurora.org> wrote:
>
>> PMIC5 ADC has support for clients to measure voltage and current
>> on inputs connected to the PMIC. Clients include reading voltage
>> phone power and on board system thermistors for thermal management.
>> ADC5 on certain PMIC has support to read battery current.
>>
>> This change adds documentation.
>>
>> Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> Hi Siddartha,
>
> Some complexity in here!  Anyhow, a few comments inline and we will definitely
> be wanting guidance from the devicetree people for this one.
>
> Jonathan
>
>> ---
>>   .../devicetree/bindings/iio/adc/qcom,spmi-adc5.txt | 137 +++++++++++++++++++++
>>   1 file changed, 137 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
>> new file mode 100644
>> index 0000000..c9268ba
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
>> @@ -0,0 +1,137 @@
>> +Qualcomm Technologies Inc. SPMI PMIC5 voltage and current ADC
>> +
>> +SPMI PMIC5 voltage ADC (ADC) provides interface to clients to read
>> +voltage. The VADC is a 16-bit sigma-delta ADC.
>> +
>> +ADC node:
>> +
>> +- compatible:
>> +    Usage: required
>> +    Value type: <string>
>> +    Definition: Should contain "qcom,spmi-adc5" for PMIC5 ADC driver.
>> +		Should contain "qcom,spmi-adc-rev2" for PMIC refresh ADC driver.
>> +
>> +- reg:
>> +    Usage: required
>> +    Value type: <prop-encoded-array>
>> +    Definition: VADC base address and length in the SPMI PMIC register map.
>> +
>> +- #address-cells:
>> +    Usage: required
>> +    Value type: <u32>
>> +    Definition: Must be one. Child node 'reg' property should define ADC
>> +            channel number.
>> +
>> +- #size-cells:
>> +    Usage: required
>> +    Value type: <u32>
>> +    Definition: Must be zero.
>> +
>> +- #io-channel-cells:
>> +    Usage: required
>> +    Value type: <u32>
>> +    Definition: Must be one. For details about IIO bindings see:
>> +            Documentation/devicetree/bindings/iio/iio-bindings.txt
>> +
>> +- interrupts:
>> +    Usage: optional
>> +    Value type: <prop-encoded-array>
>> +    Definition: End of conversion interrupt.
>> +
>> +Channel node properties:
>> +
>> +- reg:
>> +    Usage: required
>> +    Value type: <u32>
>> +    Definition: ADC channel number.
>> +            See include/dt-bindings/iio/qcom,spmi-vadc.h
>> +
>> +- label:
>> +    Usage: required
>> +    Value type: <empty>
>> +    Definition: ADC datasheet channel name.
>> +            For thermistor inputs connected to generic AMUX or GPIO inputs
>> +            these can vary across platform for the same pins. Hence select
>> +            the datasheet name for this channel.
>> +
>> +- qcom,pre-scaling:
>> +    Usage: required
>> +    Value type: <u32 array>
>> +    Definition: Used for scaling the channel input signal before the signal is
>> +            fed to VADC. The configuration for this node is to know the
>> +            pre-determined ratio and use it for post scaling. Select one from
>> +            the following options.
>> +            <1 1>, <1 3>, <1 4>, <1 6>, <1 20>, <1 8>, <10 81>, <1 10>
>> +            If property is not found default value depending on chip will be used.
>> +
>> +- qcom,decimation:
>> +    Usage: optional
>> +    Value type: <u32>
>> +    Definition: This parameter is used to decrease ADC sampling rate.
>> +            Quicker measurements can be made by reducing decimation ratio.
>> +            For PMIC5 ADC, combined two step decimation values are 250, 420 and 840.
>> +            If property is not found, default value of 840 will be used.
> The odd indenting here needs sorting.  Mixture of spaces and tabs at the moment.
Ok, will take a look.
> Hmm. In someways this is a policy decision so should be pushed up to userspace,
> but given the 'right' value will be somewhat dependent on what you are doing
> with the channel and what is wired to it, it could arguably have a 'right' value
> for a given circuit.  This is really just the sampling frequency wrapped
> up in decimation of something, I'm guessing some input clock?
Yes. It's number of samples collected over a 4.8MHz clock.
The only reason to update this value would be if client wants to get
the conversion results back sooner. Hence left this as an optional property.
>
> Let's see what the Device-tree people think on this one!  Personally I have
> never really minded devicetree providing sensible defaults.  We can put
> control on these things later, if there is a usecase for changing them.
>
>> +	    For PMIC refresh ADC, supported decimation values are 256, 512, 1024.
>> +	    If property is not found, default value of 1024 will be used.
>> +
>> +- qcom,ratiometric:
>> +    Usage: optional
>> +    Value type: <empty>
>> +    Definition: Channel calibration type. If this property is specified
>> +            VADC will use the VDD reference (1.875V) and GND for channel
>> +            calibration. If property is not found, channel will be
>> +            calibrated with 0V and 1.25V reference channels, also
>> +            known as absolute calibration.
>> +
>> +- qcom,hw-settle-time:
>> +    Usage: optional
>> +    Value type: <u32>
>> +    Definition: Time between AMUX getting configured and the ADC starting
>> +            conversion.
>> +	    For PMIC5, delay = 15us for value 0,
>> +			100us * (value) for values 0 < value < 11, and
>> +            		2ms * (value - 10) otherwise.
>> +            Valid values are: 15, 100, 200, 300, 400, 500, 600, 700, 800,
> This description is very confusing given the different uses of 'value'
> None of the values you have allowed is less than 11 so the first condition
> doesn't apply.
The 'value' is an index programmed in the hardware to achieve the
hardware settling delay specified under valid values. I will update the
documentation here.
>
>> +            900 us and 1, 2, 4, 6, 8, 10 ms
>> +            If property is not found, channel will use 15us.
>> +	    For PMIC rev2, delay = 100us * (value) for values 0 < value < 11, and
>> +			2ms * (value - 10) otherwise.
>> +            Valid values are: 0, 100, 200, 300, 400, 500, 600, 700, 800,
>> +            900 us and 1, 2, 4, 6, 8, 10 ms
>> +            If property is not found, channel will use 0 us.
>> +
>> +- qcom,avg-samples:
>> +    Usage: optional
>> +    Value type: <u32>
>> +    Definition: Number of samples to be used for measurement.
>> +            Averaging provides the option to obtain a single measurement
>> +            from the ADC that is an average of multiple samples. The value
>> +            selected is 2^(value).
>> +            Valid values are: 1, 2, 4, 8, 16
>> +            If property is not found, 1 sample will be used.
> As with decimation, this is arguably not a feature of the hardware, but
> a software decision...
>
>> +
>> +Example:
>> +
>> +        /* VADC node */
>> +        pmic_vadc: vadc@3100 {
>> +                compatible = "qcom,spmi-adc5";
>> +                reg = <0x3100 0x100>;
>> +                interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
>> +                #address-cells = <1>;
>> +                #size-cells = <0>;
>> +                #io-channel-cells = <1>;
>> +                io-channel-ranges;
>> +
>> +                /* Channel node */
>> +                vph_pwr {
>> +                        reg = <ADC_VPH_PWR>;
>> +                        label = "vph_pwr";
>> +                        qcom,pre-scaling = <1 3>;
>> +                };
>> +        };
>> +
>> +        /* IIO client node */
>> +        usb {
>> +                io-channels = <&pmic_vadc ADC_VPH_PWR>;
>> +                io-channel-names = "vadc";
>> +        };

Re: [PATCH 1/2] dt-bindings: iio: adc: Add DT binding document for PMIC5 ADC

[Rob Herring]

On Tue, May 15, 2018 at 04:57:03PM -0700, Siddartha Mohanadoss wrote:
> Hi Jonathan,
> 
> Thanks for the comments.
> 
> 
> On 05/12/2018 03:15 AM, Jonathan Cameron wrote:
> > On Tue,  8 May 2018 14:38:21 -0700
> > Siddartha Mohanadoss <smohanad@codeaurora.org> wrote:
> > 
> > > PMIC5 ADC has support for clients to measure voltage and current
> > > on inputs connected to the PMIC. Clients include reading voltage
> > > phone power and on board system thermistors for thermal management.
> > > ADC5 on certain PMIC has support to read battery current.
> > > 
> > > This change adds documentation.
> > > 
> > > Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
> > Hi Siddartha,
> > 
> > Some complexity in here!  Anyhow, a few comments inline and we will definitely
> > be wanting guidance from the devicetree people for this one.
> > 
> > Jonathan
> > 
> > > ---
> > >   .../devicetree/bindings/iio/adc/qcom,spmi-adc5.txt | 137 +++++++++++++++++++++
> > >   1 file changed, 137 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
> > > new file mode 100644
> > > index 0000000..c9268ba
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
> > > @@ -0,0 +1,137 @@
> > > +Qualcomm Technologies Inc. SPMI PMIC5 voltage and current ADC
> > > +
> > > +SPMI PMIC5 voltage ADC (ADC) provides interface to clients to read
> > > +voltage. The VADC is a 16-bit sigma-delta ADC.
> > > +
> > > +ADC node:
> > > +
> > > +- compatible:
> > > +    Usage: required
> > > +    Value type: <string>
> > > +    Definition: Should contain "qcom,spmi-adc5" for PMIC5 ADC driver.
> > > +		Should contain "qcom,spmi-adc-rev2" for PMIC refresh ADC driver.

Chip specific compatible strings please unless you convince me there are 
a large number of chips per above compatible.

Bindings are for hardware, not drivers.

> > > +
> > > +- reg:
> > > +    Usage: required
> > > +    Value type: <prop-encoded-array>
> > > +    Definition: VADC base address and length in the SPMI PMIC register map.
> > > +
> > > +- #address-cells:
> > > +    Usage: required
> > > +    Value type: <u32>
> > > +    Definition: Must be one. Child node 'reg' property should define ADC
> > > +            channel number.
> > > +
> > > +- #size-cells:
> > > +    Usage: required
> > > +    Value type: <u32>
> > > +    Definition: Must be zero.
> > > +
> > > +- #io-channel-cells:
> > > +    Usage: required
> > > +    Value type: <u32>
> > > +    Definition: Must be one. For details about IIO bindings see:
> > > +            Documentation/devicetree/bindings/iio/iio-bindings.txt
> > > +
> > > +- interrupts:
> > > +    Usage: optional
> > > +    Value type: <prop-encoded-array>
> > > +    Definition: End of conversion interrupt.
> > > +
> > > +Channel node properties:
> > > +
> > > +- reg:
> > > +    Usage: required
> > > +    Value type: <u32>
> > > +    Definition: ADC channel number.
> > > +            See include/dt-bindings/iio/qcom,spmi-vadc.h
> > > +
> > > +- label:
> > > +    Usage: required
> > > +    Value type: <empty>
> > > +    Definition: ADC datasheet channel name.
> > > +            For thermistor inputs connected to generic AMUX or GPIO inputs
> > > +            these can vary across platform for the same pins. Hence select
> > > +            the datasheet name for this channel.

Why do you need this? If the name comes from a datasheet list, then 
perhaps you should list the exact string here. Otherwise, there's a lot 
left to the user in terms of capitalization, etc.

> > > +
> > > +- qcom,pre-scaling:
> > > +    Usage: required
> > > +    Value type: <u32 array>
> > > +    Definition: Used for scaling the channel input signal before the signal is
> > > +            fed to VADC. The configuration for this node is to know the
> > > +            pre-determined ratio and use it for post scaling. Select one from
> > > +            the following options.
> > > +            <1 1>, <1 3>, <1 4>, <1 6>, <1 20>, <1 8>, <10 81>, <1 10>
> > > +            If property is not found default value depending on chip will be used.
> > > +
> > > +- qcom,decimation:
> > > +    Usage: optional
> > > +    Value type: <u32>
> > > +    Definition: This parameter is used to decrease ADC sampling rate.
> > > +            Quicker measurements can be made by reducing decimation ratio.
> > > +            For PMIC5 ADC, combined two step decimation values are 250, 420 and 840.
> > > +            If property is not found, default value of 840 will be used.
> > The odd indenting here needs sorting.  Mixture of spaces and tabs at the moment.
> Ok, will take a look.
> > Hmm. In someways this is a policy decision so should be pushed up to userspace,
> > but given the 'right' value will be somewhat dependent on what you are doing
> > with the channel and what is wired to it, it could arguably have a 'right' value
> > for a given circuit.  This is really just the sampling frequency wrapped
> > up in decimation of something, I'm guessing some input clock?
> Yes. It's number of samples collected over a 4.8MHz clock.
> The only reason to update this value would be if client wants to get
> the conversion results back sooner. Hence left this as an optional property.
> > 
> > Let's see what the Device-tree people think on this one!  Personally I have
> > never really minded devicetree providing sensible defaults.  We can put
> > control on these things later, if there is a usecase for changing them.

I don't have a problem with this in DT, though my first thought was it 
should be common. Then after reading some on decimation, I'm not sure it 
would always just be a single 32-bit value?

> > 
> > > +	    For PMIC refresh ADC, supported decimation values are 256, 512, 1024.
> > > +	    If property is not found, default value of 1024 will be used.
> > > +
> > > +- qcom,ratiometric:
> > > +    Usage: optional
> > > +    Value type: <empty>
> > > +    Definition: Channel calibration type. If this property is specified
> > > +            VADC will use the VDD reference (1.875V) and GND for channel
> > > +            calibration. If property is not found, channel will be
> > > +            calibrated with 0V and 1.25V reference channels, also
> > > +            known as absolute calibration.
> > > +
> > > +- qcom,hw-settle-time:
> > > +    Usage: optional
> > > +    Value type: <u32>
> > > +    Definition: Time between AMUX getting configured and the ADC starting
> > > +            conversion.
> > > +	    For PMIC5, delay = 15us for value 0,
> > > +			100us * (value) for values 0 < value < 11, and
> > > +            		2ms * (value - 10) otherwise.
> > > +            Valid values are: 15, 100, 200, 300, 400, 500, 600, 700, 800,
> > This description is very confusing given the different uses of 'value'
> > None of the values you have allowed is less than 11 so the first condition
> > doesn't apply.
> The 'value' is an index programmed in the hardware to achieve the
> hardware settling delay specified under valid values. I will update the
> documentation here.
> > 
> > > +            900 us and 1, 2, 4, 6, 8, 10 ms
> > > +            If property is not found, channel will use 15us.
> > > +	    For PMIC rev2, delay = 100us * (value) for values 0 < value < 11, and
> > > +			2ms * (value - 10) otherwise.
> > > +            Valid values are: 0, 100, 200, 300, 400, 500, 600, 700, 800,
> > > +            900 us and 1, 2, 4, 6, 8, 10 ms
> > > +            If property is not found, channel will use 0 us.
> > > +
> > > +- qcom,avg-samples:
> > > +    Usage: optional
> > > +    Value type: <u32>
> > > +    Definition: Number of samples to be used for measurement.
> > > +            Averaging provides the option to obtain a single measurement
> > > +            from the ADC that is an average of multiple samples. The value
> > > +            selected is 2^(value).
> > > +            Valid values are: 1, 2, 4, 8, 16
> > > +            If property is not found, 1 sample will be used.
> > As with decimation, this is arguably not a feature of the hardware, but
> > a software decision...

We already have a common property for touchscreens and vendor properties 
for a few ADCs, so we should define a common one.

Now, sadly, I've just found that all these properties are already 
defined in bindings/iio/adc/qcom,spmi-vadc.txt. Why didn't you add these 
compatibles to the existing binding. Then we're not reviewing the same 
thing again...

Rob

Re: [PATCH 1/2] dt-bindings: iio: adc: Add DT binding document for PMIC5 ADC

[Siddartha Mohanadoss]

Hi Rob,

Thanks for the comments.


On 05/22/2018 01:54 PM, Rob Herring wrote:
> On Tue, May 15, 2018 at 04:57:03PM -0700, Siddartha Mohanadoss wrote:
>> Hi Jonathan,
>>
>> Thanks for the comments.
>>
>>
>> On 05/12/2018 03:15 AM, Jonathan Cameron wrote:
>>> On Tue,  8 May 2018 14:38:21 -0700
>>> Siddartha Mohanadoss <smohanad@codeaurora.org> wrote:
>>>
>>>> PMIC5 ADC has support for clients to measure voltage and current
>>>> on inputs connected to the PMIC. Clients include reading voltage
>>>> phone power and on board system thermistors for thermal management.
>>>> ADC5 on certain PMIC has support to read battery current.
>>>>
>>>> This change adds documentation.
>>>>
>>>> Signed-off-by: Siddartha Mohanadoss <smohanad@codeaurora.org>
>>> Hi Siddartha,
>>>
>>> Some complexity in here!  Anyhow, a few comments inline and we will definitely
>>> be wanting guidance from the devicetree people for this one.
>>>
>>> Jonathan
>>>
>>>> ---
>>>>    .../devicetree/bindings/iio/adc/qcom,spmi-adc5.txt | 137 +++++++++++++++++++++
>>>>    1 file changed, 137 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
>>>> new file mode 100644
>>>> index 0000000..c9268ba
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-adc5.txt
>>>> @@ -0,0 +1,137 @@
>>>> +Qualcomm Technologies Inc. SPMI PMIC5 voltage and current ADC
>>>> +
>>>> +SPMI PMIC5 voltage ADC (ADC) provides interface to clients to read
>>>> +voltage. The VADC is a 16-bit sigma-delta ADC.
>>>> +
>>>> +ADC node:
>>>> +
>>>> +- compatible:
>>>> +    Usage: required
>>>> +    Value type: <string>
>>>> +    Definition: Should contain "qcom,spmi-adc5" for PMIC5 ADC driver.
>>>> +		Should contain "qcom,spmi-adc-rev2" for PMIC refresh ADC driver.
> Chip specific compatible strings please unless you convince me there are
> a large number of chips per above compatible.

The above compatible property supports at least 5 different PMIC's each 
so far,
hence split it as a separate property to address the differences.

>
> Bindings are for hardware, not drivers.
>
>>>> +
>>>> +- reg:
>>>> +    Usage: required
>>>> +    Value type: <prop-encoded-array>
>>>> +    Definition: VADC base address and length in the SPMI PMIC register map.
>>>> +
>>>> +- #address-cells:
>>>> +    Usage: required
>>>> +    Value type: <u32>
>>>> +    Definition: Must be one. Child node 'reg' property should define ADC
>>>> +            channel number.
>>>> +
>>>> +- #size-cells:
>>>> +    Usage: required
>>>> +    Value type: <u32>
>>>> +    Definition: Must be zero.
>>>> +
>>>> +- #io-channel-cells:
>>>> +    Usage: required
>>>> +    Value type: <u32>
>>>> +    Definition: Must be one. For details about IIO bindings see:
>>>> +            Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>> +
>>>> +- interrupts:
>>>> +    Usage: optional
>>>> +    Value type: <prop-encoded-array>
>>>> +    Definition: End of conversion interrupt.
>>>> +
>>>> +Channel node properties:
>>>> +
>>>> +- reg:
>>>> +    Usage: required
>>>> +    Value type: <u32>
>>>> +    Definition: ADC channel number.
>>>> +            See include/dt-bindings/iio/qcom,spmi-vadc.h
>>>> +
>>>> +- label:
>>>> +    Usage: required
>>>> +    Value type: <empty>
>>>> +    Definition: ADC datasheet channel name.
>>>> +            For thermistor inputs connected to generic AMUX or GPIO inputs
>>>> +            these can vary across platform for the same pins. Hence select
>>>> +            the datasheet name for this channel.
> Why do you need this? If the name comes from a datasheet list, then
> perhaps you should list the exact string here. Otherwise, there's a lot
> left to the user in terms of capitalization, etc.

I will fix the comment. The label property name used is from the 
schematics for the
respective platform and not the data sheet. Some of the channel names on 
the PMIC
datasheet are generic such as ADC_AMUX_THM1 and ADC_GPIO1. Depending on the
platform these pins can be connected to measure different thermistors or 
inputs
across the platforms.

>
>>>> +
>>>> +- qcom,pre-scaling:
>>>> +    Usage: required
>>>> +    Value type: <u32 array>
>>>> +    Definition: Used for scaling the channel input signal before the signal is
>>>> +            fed to VADC. The configuration for this node is to know the
>>>> +            pre-determined ratio and use it for post scaling. Select one from
>>>> +            the following options.
>>>> +            <1 1>, <1 3>, <1 4>, <1 6>, <1 20>, <1 8>, <10 81>, <1 10>
>>>> +            If property is not found default value depending on chip will be used.
>>>> +
>>>> +- qcom,decimation:
>>>> +    Usage: optional
>>>> +    Value type: <u32>
>>>> +    Definition: This parameter is used to decrease ADC sampling rate.
>>>> +            Quicker measurements can be made by reducing decimation ratio.
>>>> +            For PMIC5 ADC, combined two step decimation values are 250, 420 and 840.
>>>> +            If property is not found, default value of 840 will be used.
>>> The odd indenting here needs sorting.  Mixture of spaces and tabs at the moment.
>> Ok, will take a look.
>>> Hmm. In someways this is a policy decision so should be pushed up to userspace,
>>> but given the 'right' value will be somewhat dependent on what you are doing
>>> with the channel and what is wired to it, it could arguably have a 'right' value
>>> for a given circuit.  This is really just the sampling frequency wrapped
>>> up in decimation of something, I'm guessing some input clock?
>> Yes. It's number of samples collected over a 4.8MHz clock.
>> The only reason to update this value would be if client wants to get
>> the conversion results back sooner. Hence left this as an optional property.
>>> Let's see what the Device-tree people think on this one!  Personally I have
>>> never really minded devicetree providing sensible defaults.  We can put
>>> control on these things later, if there is a usecase for changing them.
> I don't have a problem with this in DT, though my first thought was it
> should be common. Then after reading some on decimation, I'm not sure it
> would always just be a single 32-bit value?

For this PMIC family, the decimation and fast averaging are common 
settings used
across all the channels. In this case the decimation ratio values 
specified are
programmed as an index to select the above decimation value. Not sure if 
i answered
the question. Please let me know if this needs further clarification.

>
>>>> +	    For PMIC refresh ADC, supported decimation values are 256, 512, 1024.
>>>> +	    If property is not found, default value of 1024 will be used.
>>>> +
>>>> +- qcom,ratiometric:
>>>> +    Usage: optional
>>>> +    Value type: <empty>
>>>> +    Definition: Channel calibration type. If this property is specified
>>>> +            VADC will use the VDD reference (1.875V) and GND for channel
>>>> +            calibration. If property is not found, channel will be
>>>> +            calibrated with 0V and 1.25V reference channels, also
>>>> +            known as absolute calibration.
>>>> +
>>>> +- qcom,hw-settle-time:
>>>> +    Usage: optional
>>>> +    Value type: <u32>
>>>> +    Definition: Time between AMUX getting configured and the ADC starting
>>>> +            conversion.
>>>> +	    For PMIC5, delay = 15us for value 0,
>>>> +			100us * (value) for values 0 < value < 11, and
>>>> +            		2ms * (value - 10) otherwise.
>>>> +            Valid values are: 15, 100, 200, 300, 400, 500, 600, 700, 800,
>>> This description is very confusing given the different uses of 'value'
>>> None of the values you have allowed is less than 11 so the first condition
>>> doesn't apply.
>> The 'value' is an index programmed in the hardware to achieve the
>> hardware settling delay specified under valid values. I will update the
>> documentation here.
>>>> +            900 us and 1, 2, 4, 6, 8, 10 ms
>>>> +            If property is not found, channel will use 15us.
>>>> +	    For PMIC rev2, delay = 100us * (value) for values 0 < value < 11, and
>>>> +			2ms * (value - 10) otherwise.
>>>> +            Valid values are: 0, 100, 200, 300, 400, 500, 600, 700, 800,
>>>> +            900 us and 1, 2, 4, 6, 8, 10 ms
>>>> +            If property is not found, channel will use 0 us.
>>>> +
>>>> +- qcom,avg-samples:
>>>> +    Usage: optional
>>>> +    Value type: <u32>
>>>> +    Definition: Number of samples to be used for measurement.
>>>> +            Averaging provides the option to obtain a single measurement
>>>> +            from the ADC that is an average of multiple samples. The value
>>>> +            selected is 2^(value).
>>>> +            Valid values are: 1, 2, 4, 8, 16
>>>> +            If property is not found, 1 sample will be used.
>>> As with decimation, this is arguably not a feature of the hardware, but
>>> a software decision...
> We already have a common property for touchscreens and vendor properties
> for a few ADCs, so we should define a common one.
>
> Now, sadly, I've just found that all these properties are already
> defined in bindings/iio/adc/qcom,spmi-vadc.txt. Why didn't you add these
> compatibles to the existing binding. Then we're not reviewing the same
> thing again...

I figured it would be easier for the client to follow and cleaner to 
separate it
out for a per family otherwise it needs a separate definition for each 
of the
property based on the PMIC type.

The earlier PMIC ADC family has capability to specify the decimation
and fast averaging for each channel. On PMIC5 ADC these two
properties are common across all channels and are required to be
programmed only once. The hardware settling delays have
differences within the family. I do not have a preference either way, i can
try and merge these with the existing bindings for the next patch revision
and based on the comments i can either revert or use the existing one.

>
> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html