Re: [PATCH v2] iio: iadc: Qualcomm SPMI PMIC current ADC driver

Ivan T. Ivanov
2014-09-24T16:00:44Z

On Wed, 2014-09-24 at 15:55 +0100, Mark Rutland wrote:
> On Wed, Sep 24, 2014 at 02:58:23PM +0100, Ivan T. Ivanov wrote:
> > The current ADC is peripheral of Qualcomm SPMI PMIC chips. It has
> > 16 bits resolution and register space inside PMIC accessible across
> > SPMI bus.
> >
> > The driver registers itself through IIO interface.
> >
> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > ---
> > Changes:
> > - Fix Kconfig dependencies
> > - Reword Kconfig help text
> > - Replace IIO_CHAN_INFO_PROCESSED with IIO_CHAN_INFO_RAW and IIO_CHAN_INFO_SCALE,
> > This enable client drivers to use microampers precission, instead miliamps.
> > - Use const array for iio_schan_spec-fiers.
> > - Fix spelling errors and adress review comments.
> >
> > Previous version:
> > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg728360.html
> >
> > .../devicetree/bindings/iio/adc/qcom,spmi-iadc.txt | 61 ++
> > drivers/iio/adc/Kconfig | 11 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/qcom-spmi-iadc.c | 691 +++++++++++++++++++++
> > 4 files changed, 764 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-iadc.txt
> > create mode 100644 drivers/iio/adc/qcom-spmi-iadc.c
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-iadc.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-iadc.txt
> > new file mode 100644
> > index 0000000..06e58b6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-iadc.txt
> > @@ -0,0 +1,61 @@
> > +Qualcomm's SPMI PMIC current ADC
> > +
> > +QPNP PMIC current ADC (IADC) provides interface to clients to read current.
> > +A 16 bit ADC is used for current measurements.
> > +
> > +IADC node:
> > +
> > +- compatible:
> > + Usage: required
> > + Value type: <string>
> > + Definition: Should contain "qcom,spmi-iadc".
> > +
> > +- reg:
> > + Usage: required
> > + Value type: <prop-encoded-array>
> > + Definition: IADC base address and length in the SPMI PMIC register map.
> > + TRIM_CNST_RDS register address and length(1)
>
> Are these two register regions? Please make the order explicit somehow
> (either say first/second/third/etc, or use reg-names).

Will make order explicit.

>
> Are they both part of the IADC, or is one part of an external/shared
> component?

I think that this is shared component.

>
> > +
> > +- interrupts:
> > + Usage: optional
> > + Value type: <prop-encoded-array>
> > + Definition: End of conversion interrupt number. If property is
> > + missing driver will use polling to detect end of conversion
> > + completion.
>
> Driver details shouldn't be in the binding. If the driver can poll,
> that's good, but it should be dropped form this description.
>

Will remove driver details.

> Is this the only interrupt?
>

Yes.

> What do you mean be "End of conversion interrupt number"? Just describe
> what the interrupt logically is from the PoV of the device -- interrupts
> is a standard property so we don't need to be too explicit about the
> type.

Badly worded. Just, "End of conversion interrupt"?

>
> > +
> > +- qcom,rsense:
> > + Usage: optional
> > + Value type: <u32>
> > + Definition: External sense register value in Ohm. Defining this
> > + propery will instruct ADC to use external ADC sense channel.
> > + If property is not defined ADC will use internal sense channel.
>
> The latter two sentences sound like a driver description.
>
> What exactly is the difference between the internal and external
> channels, and what exactly is the value in the sense register used for?

Internal - when using chip build-in resistor
External - when using off-chip resistor

>
> The binding should describe the logical properties of the device rather
> than the specific programming model details.
>
> > +
> > +- qcom,rds-trim:
> > + Usage: optional
> > + Value type: <u32>
> > + Definition: Calculate internal sense resistor value based TRIM_CNST_RDS,
> > + IADC RDS and manufacturer.
> > + 0: Read the IADC and SMBB trim register and apply the default
> > + RSENSE if conditions are met.
> > + 1: Read the IADC, SMBB trim register and manufacturer type and
> > + apply the default RSENSE if conditions are met.
> > + 2: Read the IADC, SMBB trim register and apply the default RSENSE
> > + if conditions are met.
> > + If property is not defined driver will use qcom,rsense value if
> > + defined or internal sense resistor value trimmed into register.
>
> Having read this a few times, I still don't understand which I would use
> and when.
>
> Which conditions need to be met in each of these cases?
>
> When does the manufacturer need to be taken into account and what does
> it even mean for that to be the case? That sounds very much like a
> driver detail rather than a HW property.
>
> Cases 0 and 2 seem to be the same, just with s/, / and /. I cannot
> figure out the intended difference between the two.
>
> The last sentence is very much a driver description.

Sorry. I have tried to make it better. Now looking again DT files
in codeaurora tree I see that 0 is used in pm8941, 1 is used in
pm8110 and 2 is used for pm8226. So I will remove this property
and handle this inside driver based on chip version.

>
> > +
> > +Example:
> > + /* IADC node */
> > + pmic_iadc: iadc@3600 {
> > + compatible = "qcom,spmi-iadc";
> > + reg = <0x3600 0x100>,
> > + <0x12f1 0x1>;
> > + interrupts = <0x0 0x36 0x0 IRQ_TYPE_EDGE_RISING>;
> > + qcom,rds-trim = <0>;
> > + };
> > +
> > + /* IIO client node */
> > + bat {
> > + io-channels = <&pmic_iadc 0>;
> > + io-channel-names = "iadc";
> > + };
>
> Surely you need #iio-cells on the IADC node?

Yes, I need to add it.

>
> Given the client seems to pass a specifier value, does this have
> multiple channels, or only a single channel?

Driver register only one IIO channel, so #io-channel-cells
should be <0>.

Thank you for the comments.

Regards,
Ivan