Re: [PATCH v1] arm64: dts: qcom: sc7180: coachz: Add thermal config for skin temperature

[Doug Anderson <dianders@chromium.org>]

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>