Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210322100420.125616-1-robert.foss@linaro.org>
 <20210322100420.125616-2-robert.foss@linaro.org> <b3f17dd3-d4b3-930c-db02-9f67748e4427@somainline.org>
In-Reply-To: <b3f17dd3-d4b3-930c-db02-9f67748e4427@somainline.org>
From:   Robert Foss <robert.foss@linaro.org>
Date:   Tue, 23 Mar 2021 09:36:58 +0100
Message-ID: <CAG3jFysRW7ObLONrcydYKk6XLetNkR1vYfqGNXL=gTdL9O4zhQ@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] arm64: dts: qcom: sm8350: Add thermal zones and
 throttling support
To:     Konrad Dybcio <konrad.dybcio@somainline.org>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>, amitk@kernel.org,
        rui.zhang@intel.com, Daniel Lezcano <daniel.lezcano@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        MSM <linux-arm-msm@vger.kernel.org>, linux-pm@vger.kernel.org,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Vinod Koul <vinod.koul@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hey Konrad,

Thanks for the review!


On Mon, 22 Mar 2021 at 18:27, Konrad Dybcio
<konrad.dybcio@somainline.org> wrote:
>
> Hi!
>
>
> > +             tsens0: thermal-sensor@c222000 {
> > +                     compatible = "qcom,sm8350-tsens", "qcom,tsens-v2";
> > +                     reg = <0 0x0C263000 0 0x1ff>, /* TM */
> > +                           <0 0x0C222000 0 0x8>; /* SROT */
>
> Please use lowercase hex

Ack

>
>
> > +             tsens1: thermal-sensor@c223000 {
> > +                     compatible = "qcom,sm8350-tsens", "qcom,tsens-v2";
> > +                     reg = <0 0x0C265000 0 0x1ff>, /* TM */
> > +                           <0 0x0c223000 0 0x8>; /* SROT */
>
> Ditto

Ack

>
>
> > +                     trips {
> > +                             cpu0_alert0: trip-point0 {
> > +                                     temperature = <90000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "passive";
> > +                             };
> > +
> > +                             cpu0_alert1: trip-point1 {
> > +                                     temperature = <95000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "passive";
>
> Shouldn't this be "hot"? Possibly ditto for all cpu*alert1-labeled nodes.

I based this patch on the upstream DTS for sm8250 & sdm845, and this
is what they use. However, if you think it is incorrect I'm happy to
do a little digging.

>
>
> > +                             };
> > +
> > +                             cpu0_crit: cpu_crit {
> > +                                     temperature = <110000>;
> > +                                     hysteresis = <1000>;
> > +                                     type = "critical";
> > +                             };
> > +                     };
>
> These values seem, err.. scorching hot.. Are they alright?

I agree :) This is what the vendor ships in their downstream DTS.

>
>
>
> > +             // TODO: What is the NSP subsystem?
> Please use C-style comments (/* foo */)

Removing comment.