[PATCH 0/3] arm64: dts: qcom: pm8998: Add ADC node and die temperature channel

[PATCH 0/3] arm64: dts: qcom: pm8998: Add ADC node and die temperature channel

[Matthias Kaehlcke]

This series adds the DT node for the QCOM SPMI PMIC5 ADC and a channel
for the die temperature.

The die temperature is going to be used by the temperature alarm driver
(https://lore.kernel.org/patchwork/project/lkml/list/?series=361416).

My understanding is that some of the ADC channels are/can be universally
useful on devices with a pm8998, while the use of others is device specific
(e.g. AMUX). Siddartha / QCA folks, are there other generally useful ADC
channels that should be added here?

The driver for the QCOM SPMI PMIC5 ADC landed in the 'testing' branch of the
'iio' tree:

https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=2c909be15f45c96d3dc6a84e56a76e5c3c7a60ae

(This series has a trivial conflict with the temp alarm series mentioned
above, if desired I can rebase it on that, I wasn't sure whether it's a
good idea to create a dependency on it)

Matthias Kaehlcke (3):
  dt-bindings: iio: vadc: Fix documentation of 'reg'
  arm64: dts: qcom: pm8998: Add adc node
  arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC

 .../bindings/iio/adc/qcom,spmi-vadc.txt        |  4 ++--
 arch/arm64/boot/dts/qcom/pm8998.dtsi           | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 3 deletions(-)

-- 
2.18.0.597.ga71716f1ad-goog

[PATCH 1/3] dt-bindings: iio: vadc: Fix documentation of 'reg'

[Matthias Kaehlcke]

The documentation of Qualcomm's SPMI PMIC voltage ADC claims that the
'reg' property consists of two values, the SPMI address and the length
of the controller's registers. However the SPMI bus to which it is added
specifies "#size-cells = <0>;". Remove the controller register length
from the documentation of the field and the example.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt
index 0fb46137f936..d0c188e5c922 100644
--- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt
@@ -13,7 +13,7 @@ VADC node:
 - reg:
     Usage: required
     Value type: <prop-encoded-array>
-    Definition: VADC base address and length in the SPMI PMIC register map.
+    Definition: VADC base address in the SPMI PMIC register map.
 
 - #address-cells:
     Usage: required
@@ -104,7 +104,7 @@ Example:
 	/* VADC node */
 	pmic_vadc: vadc@3100 {
 		compatible = "qcom,spmi-vadc";
-		reg = <0x3100 0x100>;
+		reg = <0x3100>;
 		interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
2.18.0.597.ga71716f1ad-goog

Re: [PATCH 1/3] dt-bindings: iio: vadc: Fix documentation of 'reg'

[Doug Anderson]

Hi,

On Wed, Aug 8, 2018 at 12:13 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> The documentation of Qualcomm's SPMI PMIC voltage ADC claims that the
> 'reg' property consists of two values, the SPMI address and the length
> of the controller's registers. However the SPMI bus to which it is added
> specifies "#size-cells = <0>;". Remove the controller register length
> from the documentation of the field and the example.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 1/3] dt-bindings: iio: vadc: Fix documentation of 'reg'

[Rob Herring]

On Wed,  8 Aug 2018 12:13:37 -0700, Matthias Kaehlcke wrote:
> The documentation of Qualcomm's SPMI PMIC voltage ADC claims that the
> 'reg' property consists of two values, the SPMI address and the length
> of the controller's registers. However the SPMI bus to which it is added
> specifies "#size-cells = <0>;". Remove the controller register length
> from the documentation of the field and the example.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 2/3] arm64: dts: qcom: pm8998: Add adc node

[Matthias Kaehlcke]

This adds the adc node to pm8998 based on the examples in the
bindings. It also fixes the order of the included headers.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 arch/arm64/boot/dts/qcom/pm8998.dtsi | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
index 92bed1e7d4bb..f70f6101bceb 100644
--- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
@@ -1,8 +1,9 @@
 // SPDX-License-Identifier: (GPL-2.0+ OR MIT)
 /* Copyright 2018 Google LLC. */
 
-#include <dt-bindings/spmi/spmi.h>
+#include <dt-bindings/iio/qcom,spmi-vadc.h>
 #include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/spmi/spmi.h>
 
 &spmi_bus {
 	pm8998_lsid0: pmic@0 {
@@ -11,6 +12,16 @@
 		#address-cells = <1>;
 		#size-cells = <0>;
 
+		pm8998_adc: adc@3100 {
+			compatible = "qcom,spmi-adc-rev2";
+			reg = <0x3100>;
+			interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			#io-channel-cells = <1>;
+			io-channel-ranges;
+		};
+
 		pm8998_gpio: gpios@c000 {
 			compatible = "qcom,pm8998-gpio", "qcom,spmi-gpio";
 			reg = <0xc000>;
-- 
2.18.0.597.ga71716f1ad-goog

Re: [PATCH 2/3] arm64: dts: qcom: pm8998: Add adc node

[Doug Anderson]

Hi,

On Wed, Aug 8, 2018 at 12:13 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> This adds the adc node to pm8998 based on the examples in the
> bindings. It also fixes the order of the included headers.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  arch/arm64/boot/dts/qcom/pm8998.dtsi | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> index 92bed1e7d4bb..f70f6101bceb 100644
> --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> @@ -1,8 +1,9 @@
>  // SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>  /* Copyright 2018 Google LLC. */
>
> -#include <dt-bindings/spmi/spmi.h>
> +#include <dt-bindings/iio/qcom,spmi-vadc.h>
>  #include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/spmi/spmi.h>
>
>  &spmi_bus {
>         pm8998_lsid0: pmic@0 {
> @@ -11,6 +12,16 @@
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
> +               pm8998_adc: adc@3100 {
> +                       compatible = "qcom,spmi-adc-rev2";
> +                       reg = <0x3100>;
> +                       interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       #io-channel-cells = <1>;
> +                       io-channel-ranges;
> +               };

I'm a little confused about what the "io-channel-ranges" does here.
The documentation isn't clear at all to me for it.  If I'm reading it
right it's also supposed to be for iio-consumers, but you're using it
in a provider.  I see you copied this from the example.  Maybe the
example is wrong?  ...or I'm just confused...

Other than that question, this looks fine to me.


-Doug

Re: [PATCH 2/3] arm64: dts: qcom: pm8998: Add adc node

[Matthias Kaehlcke]

On Fri, Aug 10, 2018 at 05:09:17PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Aug 8, 2018 at 12:13 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> > This adds the adc node to pm8998 based on the examples in the
> > bindings. It also fixes the order of the included headers.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> >  arch/arm64/boot/dts/qcom/pm8998.dtsi | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> > index 92bed1e7d4bb..f70f6101bceb 100644
> > --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> > @@ -1,8 +1,9 @@
> >  // SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >  /* Copyright 2018 Google LLC. */
> >
> > -#include <dt-bindings/spmi/spmi.h>
> > +#include <dt-bindings/iio/qcom,spmi-vadc.h>
> >  #include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/spmi/spmi.h>
> >
> >  &spmi_bus {
> >         pm8998_lsid0: pmic@0 {
> > @@ -11,6 +12,16 @@
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >
> > +               pm8998_adc: adc@3100 {
> > +                       compatible = "qcom,spmi-adc-rev2";
> > +                       reg = <0x3100>;
> > +                       interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
> > +                       #address-cells = <1>;
> > +                       #size-cells = <0>;
> > +                       #io-channel-cells = <1>;
> > +                       io-channel-ranges;
> > +               };
> 
> I'm a little confused about what the "io-channel-ranges" does here.
> The documentation isn't clear at all to me for it.  If I'm reading it
> right it's also supposed to be for iio-consumers, but you're using it
> in a provider.  I see you copied this from the example.  Maybe the
> example is wrong?  ...or I'm just confused...

Yes, I copied it from the example, its use here is also not clear to
me, other ADC providers like adc@126c0000 in exynos3250.dtsi or
adc@180a6000 in bcm-cygnus.dtsi also specify it ...

Siddartha/Jonathan, could you help to clarify if "io-channel-ranges"
should really be specified here as the DT example suggests?

Thanks

Matthias

Re: [PATCH 2/3] arm64: dts: qcom: pm8998: Add adc node

[Doug Anderson]

Hi,

On Mon, Aug 27, 2018 at 10:10 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
> On Fri, Aug 10, 2018 at 05:09:17PM -0700, Doug Anderson wrote:
>> Hi,
>>
>> On Wed, Aug 8, 2018 at 12:13 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
>> > This adds the adc node to pm8998 based on the examples in the
>> > bindings. It also fixes the order of the included headers.
>> >
>> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> > ---
>> >  arch/arm64/boot/dts/qcom/pm8998.dtsi | 13 ++++++++++++-
>> >  1 file changed, 12 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
>> > index 92bed1e7d4bb..f70f6101bceb 100644
>> > --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
>> > +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
>> > @@ -1,8 +1,9 @@
>> >  // SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>> >  /* Copyright 2018 Google LLC. */
>> >
>> > -#include <dt-bindings/spmi/spmi.h>
>> > +#include <dt-bindings/iio/qcom,spmi-vadc.h>
>> >  #include <dt-bindings/interrupt-controller/irq.h>
>> > +#include <dt-bindings/spmi/spmi.h>
>> >
>> >  &spmi_bus {
>> >         pm8998_lsid0: pmic@0 {
>> > @@ -11,6 +12,16 @@
>> >                 #address-cells = <1>;
>> >                 #size-cells = <0>;
>> >
>> > +               pm8998_adc: adc@3100 {
>> > +                       compatible = "qcom,spmi-adc-rev2";
>> > +                       reg = <0x3100>;
>> > +                       interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
>> > +                       #address-cells = <1>;
>> > +                       #size-cells = <0>;
>> > +                       #io-channel-cells = <1>;
>> > +                       io-channel-ranges;
>> > +               };
>>
>> I'm a little confused about what the "io-channel-ranges" does here.
>> The documentation isn't clear at all to me for it.  If I'm reading it
>> right it's also supposed to be for iio-consumers, but you're using it
>> in a provider.  I see you copied this from the example.  Maybe the
>> example is wrong?  ...or I'm just confused...
>
> Yes, I copied it from the example, its use here is also not clear to
> me, other ADC providers like adc@126c0000 in exynos3250.dtsi or
> adc@180a6000 in bcm-cygnus.dtsi also specify it ...
>
> Siddartha/Jonathan, could you help to clarify if "io-channel-ranges"
> should really be specified here as the DT example suggests?

Does everything work if you just remove the "io-channel-ranges"?  We
could remove it and always add it back in later if someone could
explain what it's for or if we find a reason why it was needed?  ...or
we have any other ideas for how to get this resolved?  :(

-Doug

Re: [PATCH 2/3] arm64: dts: qcom: pm8998: Add adc node

[Matthias Kaehlcke]

On Thu, Sep 06, 2018 at 11:34:32AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Aug 27, 2018 at 10:10 AM, Matthias Kaehlcke <mka@chromium.org> wrote:
> > On Fri, Aug 10, 2018 at 05:09:17PM -0700, Doug Anderson wrote:
> >> Hi,
> >>
> >> On Wed, Aug 8, 2018 at 12:13 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> >> > This adds the adc node to pm8998 based on the examples in the
> >> > bindings. It also fixes the order of the included headers.
> >> >
> >> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >> > ---
> >> >  arch/arm64/boot/dts/qcom/pm8998.dtsi | 13 ++++++++++++-
> >> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> >> > index 92bed1e7d4bb..f70f6101bceb 100644
> >> > --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> >> > +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> >> > @@ -1,8 +1,9 @@
> >> >  // SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> >> >  /* Copyright 2018 Google LLC. */
> >> >
> >> > -#include <dt-bindings/spmi/spmi.h>
> >> > +#include <dt-bindings/iio/qcom,spmi-vadc.h>
> >> >  #include <dt-bindings/interrupt-controller/irq.h>
> >> > +#include <dt-bindings/spmi/spmi.h>
> >> >
> >> >  &spmi_bus {
> >> >         pm8998_lsid0: pmic@0 {
> >> > @@ -11,6 +12,16 @@
> >> >                 #address-cells = <1>;
> >> >                 #size-cells = <0>;
> >> >
> >> > +               pm8998_adc: adc@3100 {
> >> > +                       compatible = "qcom,spmi-adc-rev2";
> >> > +                       reg = <0x3100>;
> >> > +                       interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
> >> > +                       #address-cells = <1>;
> >> > +                       #size-cells = <0>;
> >> > +                       #io-channel-cells = <1>;
> >> > +                       io-channel-ranges;
> >> > +               };
> >>
> >> I'm a little confused about what the "io-channel-ranges" does here.
> >> The documentation isn't clear at all to me for it.  If I'm reading it
> >> right it's also supposed to be for iio-consumers, but you're using it
> >> in a provider.  I see you copied this from the example.  Maybe the
> >> example is wrong?  ...or I'm just confused...
> >
> > Yes, I copied it from the example, its use here is also not clear to
> > me, other ADC providers like adc@126c0000 in exynos3250.dtsi or
> > adc@180a6000 in bcm-cygnus.dtsi also specify it ...
> >
> > Siddartha/Jonathan, could you help to clarify if "io-channel-ranges"
> > should really be specified here as the DT example suggests?
> 
> Does everything work if you just remove the "io-channel-ranges"?

Yes, the ADC channels are still available within the kernel and
through sysfs and provide reasonable values.

> We could remove it and always add it back in later if someone could
> explain what it's for or if we find a reason why it was needed?  ...or
> we have any other ideas for how to get this resolved?  :(

Sounds good, I'll respin with 'io-channel-ranges' removed.

[PATCH 3/3] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC

[Matthias Kaehlcke]

Add a channel node for the die temperature to the ADC.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
 arch/arm64/boot/dts/qcom/pm8998.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
index f70f6101bceb..499bae0afb7f 100644
--- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
@@ -20,6 +20,11 @@
 			#size-cells = <0>;
 			#io-channel-cells = <1>;
 			io-channel-ranges;
+
+			die-temp {
+				reg = <ADC5_DIE_TEMP>;
+				label = "die_temp";
+			};
 		};
 
 		pm8998_gpio: gpios@c000 {
-- 
2.18.0.597.ga71716f1ad-goog

Re: [PATCH 3/3] arm64: dts: qcom: pm8998: Add die temperature channel node to the ADC

[Doug Anderson]

Hi,

On Wed, Aug 8, 2018 at 12:13 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> Add a channel node for the die temperature to the ADC.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  arch/arm64/boot/dts/qcom/pm8998.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> index f70f6101bceb..499bae0afb7f 100644
> --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> @@ -20,6 +20,11 @@
>                         #size-cells = <0>;
>                         #io-channel-cells = <1>;
>                         io-channel-ranges;
> +
> +                       die-temp {
> +                               reg = <ADC5_DIE_TEMP>;
> +                               label = "die_temp";
> +                       };

Seems to match the binding.

I'm no expert, but FWIW:

Reviewed-by: Douglas Anderson <dianders@chromium.org>