Date:	Wed, 24 Sep 2014 15:55:27 +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 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).

Are they both part of the IADC, or is one part of an external/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.

Is this the only interrupt?

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.

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

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.

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

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

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