Re: [PATCH v2 1/2] arm64: dts: qcom: pm8998: Add spmi-temp-alarm node
Hi, On Mon, Jul 2, 2018 at 1:51 PM, Matthias Kaehlcke wrote: >> > + compatible = "qcom,spmi-temp-alarm"; >> > + reg = <0x2400 0x100>; >> >> Why are there two numbers for the "reg"? Should just be 0x2400. > > From /Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt: > > Required properties: > ... > - reg: Specifies the SPMI address and length of the controller's >registers. Hrm, something isn't adding up here. A) Do a "git grep" and you'll see that nobody else has the length. $ git grep -C2 -- qcom,spmi-temp-alarm | grep reg arch/arm/boot/dts/qcom-pm8841.dtsi- reg = <0x2400>; arch/arm/boot/dts/qcom-pm8941.dtsi- reg = <0x2400>; arch/arm/boot/dts/qcom-pma8084.dtsi-reg = <0x2400>; arch/arm64/boot/dts/qcom/pm8916.dtsi- reg = <0x2400>; --- B) The SPMI bus you're adding it to has "#size-cells = <0>;" ...as do all other SPMI busses: $ git grep -C5 qcom,spmi-pmic | grep size-cells arch/arm/boot/dts/qcom-pm8841.dtsi- #size-cells = <0>; arch/arm/boot/dts/qcom-pm8841.dtsi- #size-cells = <0>; arch/arm/boot/dts/qcom-pm8941.dtsi- #size-cells = <0>; arch/arm/boot/dts/qcom-pm8941.dtsi- #size-cells = <0>; arch/arm/boot/dts/qcom-pma8084.dtsi-#size-cells = <0>; arch/arm/boot/dts/qcom-pma8084.dtsi-#size-cells = <0>; arch/arm64/boot/dts/qcom/pm8004.dtsi- #size-cells = <0>; arch/arm64/boot/dts/qcom/pm8004.dtsi- #size-cells = <0>; arch/arm64/boot/dts/qcom/pm8005.dtsi- #size-cells = <0>; arch/arm64/boot/dts/qcom/pm8005.dtsi- #size-cells = <0>; arch/arm64/boot/dts/qcom/pm8916.dtsi- #size-cells = <0>; arch/arm64/boot/dts/qcom/pm8916.dtsi- #size-cells = <0>; arch/arm64/boot/dts/qcom/pm8994.dtsi- #size-cells = <0>; arch/arm64/boot/dts/qcom/pm8994.dtsi- #size-cells = <0>; arch/arm64/boot/dts/qcom/pm8998.dtsi- #size-cells = <0>; arch/arm64/boot/dts/qcom/pm8998.dtsi- #size-cells = <0>; arch/arm64/boot/dts/qcom/pmi8994.dtsi- #size-cells = <0>; arch/arm64/boot/dts/qcom/pmi8994.dtsi- #size-cells = <0>; ...I have no SPMI history, but my guess is that upstream dropped this because all SPMI devices have 256-byte regions (see qcom,spmi-pmic.txt)? So probably the right thing is to fix the qcom,spmi-temp-alarm not to have the size... -Doug
Re: [PATCH v2 1/2] arm64: dts: qcom: pm8998: Add spmi-temp-alarm node
On Mon, Jul 02, 2018 at 01:12:01PM -0700, Doug Anderson wrote:
> Hi,
>
> On Mon, Jul 2, 2018 at 11:10 AM, Matthias Kaehlcke wrote:
> > This adds the spmi-temp-alarm node to pm8998 based on the examples in the
> > bindings.
> >
> > Signed-off-by: Matthias Kaehlcke
> > ---
> > Changes in v2:
> > - none
> >
> > arch/arm64/boot/dts/qcom/pm8998.dtsi | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> > b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> > index 92bed1e7d4bb..2f4989e7ef68 100644
> > --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> > @@ -11,6 +11,13 @@
> > #address-cells = <1>;
> > #size-cells = <0>;
> >
> > + pm8998_temp: qcom,temp-alarm@2400 {
>
> Remove "qcom," from the node name (AKA please change to
> "temp-alarm@2400"). Someone internal in Qualcomm seems to have
> started this trend so you see it on all downstream kernels, but
> upstream device tree isn't supposed to have it.
Ok, thanks
> > + compatible = "qcom,spmi-temp-alarm";
> > + reg = <0x2400 0x100>;
>
> Why are there two numbers for the "reg"? Should just be 0x2400.
>From /Documentation/devicetree/bindings/thermal/qcom-spmi-temp-alarm.txt:
Required properties:
...
- reg: Specifies the SPMI address and length of the controller's
registers.
Re: [PATCH v2 1/2] arm64: dts: qcom: pm8998: Add spmi-temp-alarm node
Hi,
On Mon, Jul 2, 2018 at 11:10 AM, Matthias Kaehlcke wrote:
> This adds the spmi-temp-alarm node to pm8998 based on the examples in the
> bindings.
>
> Signed-off-by: Matthias Kaehlcke
> ---
> Changes in v2:
> - none
>
> arch/arm64/boot/dts/qcom/pm8998.dtsi | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> index 92bed1e7d4bb..2f4989e7ef68 100644
> --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> @@ -11,6 +11,13 @@
> #address-cells = <1>;
> #size-cells = <0>;
>
> + pm8998_temp: qcom,temp-alarm@2400 {
Remove "qcom," from the node name (AKA please change to
"temp-alarm@2400"). Someone internal in Qualcomm seems to have
started this trend so you see it on all downstream kernels, but
upstream device tree isn't supposed to have it.
> + compatible = "qcom,spmi-temp-alarm";
> + reg = <0x2400 0x100>;
Why are there two numbers for the "reg"? Should just be 0x2400.
-Doug
[PATCH v2 1/2] arm64: dts: qcom: pm8998: Add spmi-temp-alarm node
This adds the spmi-temp-alarm node to pm8998 based on the examples in the
bindings.
Signed-off-by: Matthias Kaehlcke
---
Changes in v2:
- none
arch/arm64/boot/dts/qcom/pm8998.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi
b/arch/arm64/boot/dts/qcom/pm8998.dtsi
index 92bed1e7d4bb..2f4989e7ef68 100644
--- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
@@ -11,6 +11,13 @@
#address-cells = <1>;
#size-cells = <0>;
+ pm8998_temp: qcom,temp-alarm@2400 {
+ compatible = "qcom,spmi-temp-alarm";
+ reg = <0x2400 0x100>;
+ interrupts = <0x0 0x24 0x0 IRQ_TYPE_EDGE_RISING>;
+ #thermal-sensor-cells = <0>;
+ };
+
pm8998_gpio: gpios@c000 {
compatible = "qcom,pm8998-gpio", "qcom,spmi-gpio";
reg = <0xc000>;
--
2.18.0.399.gad0ab374a1-goog
