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: <20210414111007.v1.1.I1a438604a79025307f177347d45815987b105cb5@changeid>
In-Reply-To: <20210414111007.v1.1.I1a438604a79025307f177347d45815987b105cb5@changeid>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 16 Apr 2021 07:29:15 -0700
Message-ID: <CAD=FV=XURtypK1G-Q+4FKtii=JnTVTRKQ=Ehr1rggVqxbXDH0w@mail.gmail.com>
Subject: Re: [PATCH v1] arm64: dts: qcom: sc7180: coachz: Add thermal config
 for skin temperature
To:     Matthias Kaehlcke <mka@chromium.org>
Cc:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Wed, Apr 14, 2021 at 11:10 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Add ADC and thermal monitor configuration for skin temperature,
> plus a thermal zone that monitors the skin temperature and uses
> the big cores as cooling devices.
>
> CoachZ rev1 is stuffed with an incompatible thermistor for the
> skin temperature, disable the thermal zone for rev1 to avoid
> the use of bogus temperature values.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>
>  .../dts/qcom/sc7180-trogdor-coachz-r1.dts     |  9 +++
>  .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi  | 63 +++++++++++++++++++
>  2 files changed, 72 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts
> index 86619f6c1134..80bdc4d5b523 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz-r1.dts
> @@ -14,6 +14,15 @@ / {
>         compatible = "google,coachz-rev1", "qcom,sc7180";
>  };
>
> +/*
> + * CoachZ rev1 is stuffed with a 47k NTC as thermistor for skin temperature,
> + * which currently is not supported by the PM6150 ADC driver. Disable the
> + * skin temperature thermal zone to avoid using bogus temperature values.
> + */
> +&skin_temp_thermal {
> +       status = "disabled";
> +};
> +
>  &tlmm {
>         gpio-line-names = "HUB_RST_L",
>                           "AP_RAM_ID0",
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> index e2ffe71c2d52..cabe5d6b981b 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> @@ -25,6 +25,50 @@ adau7002: audio-codec-1 {
>                 IOVDD-supply = <&pp1800_l15a>;
>                 #sound-dai-cells = <0>;
>         };
> +
> +       thermal-zones {
> +               skin_temp_thermal: skin-temp-thermal {
> +                       polling-delay-passive = <250>;
> +                       polling-delay = <0>;
> +
> +                       thermal-sensors = <&pm6150_adc_tm 1>;
> +                       sustainable-power = <814>;
> +
> +                       trips {
> +                               skin_temp_alert0: trip-point0 {
> +                                       temperature = <42000>;
> +                                       hysteresis = <1000>;
> +                                       type = "passive";
> +                               };
> +
> +                               skin_temp_alert1: trip-point1 {
> +                                       temperature = <45000>;
> +                                       hysteresis = <1000>;
> +                                       type = "passive";
> +                               };
> +
> +                               skin-temp-crit {

If it were me I would have added a label to the "crit" too to match
the alert0 and alert1. It's not needed right now, though, so it's not
worth spinning the patch for.

> +                                       temperature = <60000>;
> +                                       hysteresis = <1000>;
> +                                       type = "critical";
> +                               };
> +                       };
> +
> +                       cooling-maps {
> +                               map0 {
> +                                       trip = <&skin_temp_alert0>;
> +                                       cooling-device = <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +                                                        <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +                               };
> +
> +                               map1 {
> +                                       trip = <&skin_temp_alert1>;
> +                                       cooling-device = <&CPU6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +                                                        <&CPU7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +                               };
> +                       };
> +               };
> +       };
>  };
>
>  &ap_spi_fp {
> @@ -77,6 +121,25 @@ &panel {
>         compatible = "boe,nv110wtm-n61";
>  };
>
> +&pm6150_adc {
> +       skin-temp-thermistor@4e {

A part of me wonders if we should just be using the generic name
"thermistor@4e" which seems more common in dts.  ...but what you have
matches what we've already done in pompom so I think it's fine.

FWIW: I'm not an expert on any of the thermistor stuff but it looks
sane and matches how pompom is working with this thermistor...

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