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

[Ivan T. Ivanov]

Hi Mark,

On Wed, 2014-09-24 at 18:05 +0100, Mark Rutland wrote:
> On Wed, Sep 24, 2014 at 05:00:42PM +0100, Ivan T. Ivanov wrote:
> > 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@xxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxx/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.
> 
> If it's not a portion of the ADC itself, then that should probably be
> described as a separate unit, and referred to by phandle. What else is
> that unit, and what else is said unit used by?

Please read below.

> 
> > 
> > > 
> > > > +
> > > > +- 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"?
> 
> The part I didn't understand was what was meant by "End of conversion",
> but dropping "number" is certainly better.

It is clear now, right? End of ADC conversion.

> 
> > 
> > > 
> > > > +
> > > > +- 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
> 
> Would it be possible to use the internal channel when you have an
> external resistor?
> 
> If so, does the device have a channel per resistor, or can either
> resistor be selected and applied to the single channel the device has?
> 

They are two dedicated channels. Channel zero can only measure current
over internal resistor (MOSFET). Channel two can only measure current
over external resistor. This ADC is part of Battery Monitor System
(BMS), i.e. it is not general purpose ADC. Based on DT files in
codeaurora repository, only one of the channels is used, probably
chosen at schematics design time. In practice, on the systems that
use battery, there is always two resistors, and they are connected
in sequence, just one of them should be zero. External resistor 
could be with higher quality, I suppose.

> This might be better worded as "external-registor-ohms".

Ok.

> 
> > 
> > > 
> > > 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.
> 
> Is this only to determine the value of the internal resistor?

No. Ideal value for internal resistor is supposed to be 10Mohms.
One register(8 bits) hold production time trimmed value, which represent
difference against ideal value. It looks like some times real resistor
values is too far from ideal and register can't hold the difference, so 
some additional hints are needed. In this case value trimmed to
register into BMS peripheral.

> 
> If that isn't probable in a standard way across all variations, would an
> "internal-resistor-ohms" property be sufficient?
> 
> > 
> > > 
> > > > +
> > > > +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>.
> 
> Ok. Regardless of what the driver does, does the hardware have
> multi-channel capability?

Strictly speaking, yes. But I don't see how both of them could
used at the same time in practice.

Regards,
Ivan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/