Message-ID: <20140924170522.GL5729@leverpostej>
Date:	Wed, 24 Sep 2014 18:05:22 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	"Ivan T. Ivanov" <iivanov@...sol.com>
Cc:	Rob Herring <robh+dt@...nel.org>, Pawel Moll <Pawel.Moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Jonathan Cameron <jic23@...nel.org>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	Hartmut Knaack <knaack.h@....de>,
	Lee Jones <lee.jones@...aro.org>,
	Philippe Reynes <tremyfr@...oo.fr>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v2] iio: iadc: Qualcomm SPMI PMIC current ADC driver

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@...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.

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?

> 
> > 
> > > +
> > > +- 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.

> 
> > 
> > > +
> > > +- 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?

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

> 
> > 
> > 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?

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@...0 {
> > > +               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?

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

Powered by blists - more mailing lists