Re: [PATCH v2 4/4] arm64: dts: qcom: sdm845: mtp: Add vadc channels and thermal zones

Matthias Kaehlcke
2021-09-29T18:40:42Z

On Thu, Sep 23, 2021 at 02:23:11PM -0700, Bjorn Andersson wrote:
> Downstream defines four ADC channels related to thermal sensors external
> to the PM8998 and two channels for internal voltage measurements.
>
> Add these to the upstream SDM845 MTP, describe the thermal monitor
> channels and add thermal_zones for these.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> In addition to the iio channels exposed by v1, Daniel wanted thermal_zones...
>
> Changes since v1:
> - Enable the pm8998_adc_tm and describe the ADC channels
> - Add thermal-zones for the new channels
>
> arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 128 ++++++++++++++++++++++++
> 1 file changed, 128 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index 52dd7a858231..e3b40daef801 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -10,6 +10,8 @@
> #include <dt-bindings/gpio/gpio.h>
> #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> #include "sdm845.dtsi"
> +#include "pm8998.dtsi"
> +#include "pmi8998.dtsi"
>
> / {
> model = "Qualcomm Technologies, Inc. SDM845 MTP";
> @@ -46,6 +48,68 @@ vreg_s4a_1p8: pm8998-smps4 {
>
> vin-supply = <&vph_pwr>;
> };
> +
> + thermal-zones {
> + xo_thermal: xo-thermal {
> + polling-delay-passive = <0>;
> + polling-delay = <0>;
> +
> + thermal-sensors = <&pm8998_adc_tm 1>;
> +
> + trips {
> + trip-point {
> + temperature = <125000>;
> + hysteresis = <10000>;
> + type = "passive";
> + };
> + };
> + };
> +
> + msm_thermal: msm-thermal {
> + polling-delay-passive = <0>;
> + polling-delay = <0>;
> +
> + thermal-sensors = <&pm8998_adc_tm 2>;
> +
> + trips {
> + trip-point {
> + temperature = <125000>;
> + hysteresis = <10000>;
> + type = "passive";
> + };
> + };
> + };
> +
> + pa_thermal: pa-thermal {
> + polling-delay-passive = <0>;
> + polling-delay = <0>;
> +
> + thermal-sensors = <&pm8998_adc_tm 3>;
> +
> + trips {
> + trip-point {
> + temperature = <125000>;
> + hysteresis = <10000>;
> + type = "passive";
> + };
> + };
> + };
> +
> + quiet_thermal: quiet-thermal {
> + polling-delay-passive = <0>;
> + polling-delay = <0>;
> +
> + thermal-sensors = <&pm8998_adc_tm 4>;
> +
> + trips {
> + trip-point {
> + temperature = <125000>;
> + hysteresis = <10000>;
> + type = "passive";
> + };
> + };
> + };
> + };
> };
>
> &adsp_pas {
> @@ -469,6 +533,70 @@ &mss_pil {
> firmware-name = "qcom/sdm845/mba.mbn", "qcom/sdm845/modem.mbn";
> };
>
> +&pm8998_adc {
> + adc-chan@4c {
> + reg = <ADC5_XO_THERM_100K_PU>;
> + label = "xo_therm";
> + };
> +
> + adc-chan@4d {
> + reg = <ADC5_AMUX_THM1_100K_PU>;
> + label = "msm_therm";
> + };
> +
> + adc-chan@4f {
> + reg = <ADC5_AMUX_THM3_100K_PU>;
> + label = "pa_therm1";
> + };
> +
> + adc-chan@51 {
> + reg = <ADC5_AMUX_THM5_100K_PU>;
> + label = "quiet_therm";
> + };
> +
> + adc-chan@83 {
> + reg = <ADC5_VPH_PWR>;
> + label = "vph_pwr";
> + };
> +
> + adc-chan@85 {
> + reg = <ADC5_VCOIN>;
> + label = "vcoin";
> + };
> +};
> +
> +&pm8998_adc_tm {
> + status = "okay";
> +
> + xo-thermistor@1 {
> + reg = <1>;
> + io-channels = <&pm8998_adc ADC5_XO_THERM_100K_PU>;
> + qcom,ratiometric;
> + qcom,hw-settle-time-us = <200>;
> + };
> +
> + msm-thermistor@2 {
> + reg = <2>;
> + io-channels = <&pm8998_adc ADC5_AMUX_THM1_100K_PU>;
> + qcom,ratiometric;
> + qcom,hw-settle-time-us = <200>;
> + };
> +
> + pa-thermistor@3 {
> + reg = <3>;
> + io-channels = <&pm8998_adc ADC5_AMUX_THM3_100K_PU>;
> + qcom,ratiometric;
> + qcom,hw-settle-time-us = <200>;
> + };
> +
> + quiet-thermistor@4 {
> + reg = <4>;
> + io-channels = <&pm8998_adc ADC5_AMUX_THM5_100K_PU>;
> + qcom,ratiometric;
> + qcom,hw-settle-time-us = <200>;
> + };
> +};
> +

The example in the 'qcom,spmi-adc-tm5' binding specifies 'qcom,ratiometric'
and 'qcom,hw-settle-time-us' for both the ADC and the thermal monitor, so do
several board files (e.g. sm8250-mtp.dts and qrb5165-rb5.dts). This apparent
redundancy bothered me earlier, it's not really clear to me whether it's
needed/recommended or not. Do you happen to have any insights on this?