Re: [PATCH v2 2/2] arm64: dts: qcom: sm8350: Add thermal zones and throttling support

[Robert Foss]

Hey Konrad,

Thanks for the review!


On Mon, 22 Mar 2021 at 18:27, Konrad Dybcio
<konrad.dybcio@xxxxxxxxxxxxxx> 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.