[PATCH v2 0/4] arm64: dts: qcom: sdm845: Add thermal zones for PM8998 ADC

[PATCH v2 0/4] arm64: dts: qcom: sdm845: Add thermal zones for PM8998 ADC

[Bjorn Andersson]

Describe ADC channels, related thermal monitor channels and add thermal zones for these.

Bjorn Andersson (4):
  dt-bindings: thermal: qcom: add HC variant of adc-thermal monitor
    bindings
  thermal/drivers/qcom/spmi-adc-tm5: Add support for HC variant
  arm64: dts: qcom: pm8998: Add ADC Thermal Monitor node
  arm64: dts: qcom: sdm845: mtp: Add vadc channels and thermal zones

 .../bindings/thermal/qcom-spmi-adc-tm-hc.yaml | 149 ++++++++++++++++++
 arch/arm64/boot/dts/qcom/pm8998.dtsi          |  10 ++
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts       | 128 +++++++++++++++
 drivers/thermal/qcom/qcom-spmi-adc-tm5.c      |  43 ++++-
 4 files changed, 329 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm-hc.yaml

-- 
2.29.2

[PATCH v2 1/4] dt-bindings: thermal: qcom: add HC variant of adc-thermal monitor bindings

[Bjorn Andersson]

The HC generation of the ADC Thermal Monitor is quite similar to the 5th
generation, but differs in valid values for a few properties. Create a
new binding for the HC version of the harware, rather than sprinkle
conditionals throughout the existing binding.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

I tried to represent the enum differences from adc5 using conditionals, but the
binding became a complete mess. So hopefully this is acceptable instead.

Changes since v1:
- New patch

 .../bindings/thermal/qcom-spmi-adc-tm-hc.yaml | 149 ++++++++++++++++++
 1 file changed, 149 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm-hc.yaml

diff --git a/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm-hc.yaml b/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm-hc.yaml
new file mode 100644
index 000000000000..8273ac55b63f
--- /dev/null
+++ b/Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm-hc.yaml
@@ -0,0 +1,149 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/thermal/qcom-spmi-adc-tm-hc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm's SPMI PMIC ADC HC Thermal Monitoring
+maintainers:
+  - Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
+
+properties:
+  compatible:
+    const: qcom,spmi-adc-tm-hc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#thermal-sensor-cells":
+    const: 1
+    description:
+      Number of cells required to uniquely identify the thermal sensors. Since
+      we have multiple sensors this is set to 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  qcom,avg-samples:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Number of samples to be used for measurement.
+    enum:
+      - 1
+      - 2
+      - 4
+      - 8
+      - 16
+    default: 1
+
+  qcom,decimation:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: This parameter is used to decrease ADC sampling rate.
+            Quicker measurements can be made by reducing decimation ratio.
+    enum:
+      - 256
+      - 512
+      - 1024
+    default: 1024
+
+patternProperties:
+  "^([-a-z0-9]*)@[0-7]$":
+    type: object
+    description:
+      Represent one thermal sensor.
+
+    properties:
+      reg:
+        description: Specify the sensor channel. There are 8 channels in PMIC5's ADC TM
+        minimum: 0
+        maximum: 7
+
+      io-channels:
+        description:
+          From common IIO binding. Used to pipe PMIC ADC channel to thermal monitor
+
+      qcom,ratiometric:
+        $ref: /schemas/types.yaml#/definitions/flag
+        description:
+          Channel calibration type.
+          If this property is specified VADC will use the VDD reference
+          (1.875V) and GND for channel calibration. If property is not found,
+          channel will be calibrated with 0V and 1.25V reference channels,
+          also known as absolute calibration.
+
+      qcom,hw-settle-time-us:
+        description: Time between AMUX getting configured and the ADC starting conversion.
+        enum: [0, 100, 200, 300, 400, 500, 600, 700, 1000, 2000, 4000, 6000, 8000, 10000]
+
+      qcom,pre-scaling:
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        description: Used for scaling the channel input signal before the
+          signal is fed to VADC. The configuration for this node is to know the
+          pre-determined ratio and use it for post scaling. It is a pair of
+          integers, denoting the numerator and denominator of the fraction by
+          which input signal is multiplied. For example, <1 3> indicates the
+          signal is scaled down to 1/3 of its value before ADC measurement.  If
+          property is not found default value depending on chip will be used.
+        items:
+          - const: 1
+          - enum: [ 1, 3, 4, 6, 20, 8, 10 ]
+
+    required:
+      - reg
+      - io-channels
+
+    additionalProperties:
+      false
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#address-cells"
+  - "#size-cells"
+  - "#thermal-sensor-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/iio/qcom,spmi-vadc.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spmi_bus {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        pm8998_adc: adc@3100 {
+            reg = <0x3100>;
+            compatible = "qcom,spmi-adc-rev2";
+            #address-cells = <1>;
+            #size-cells = <0>;
+            #io-channel-cells = <1>;
+
+            /* Other propreties are omitted */
+            adc-chan@4c {
+                reg = <ADC5_XO_THERM_100K_PU>;
+            };
+        };
+
+        pm8998_adc_tm: adc-tm@3400 {
+            compatible = "qcom,spmi-adc-tm-hc";
+            reg = <0x3400>;
+            interrupts = <0x2 0x34 0x0 IRQ_TYPE_EDGE_RISING>;
+            #thermal-sensor-cells = <1>;
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            thermistor@1 {
+                reg = <1>;
+                io-channels = <&pm8998_adc ADC5_XO_THERM_100K_PU>;
+                qcom,ratiometric;
+                qcom,hw-settle-time-us = <200>;
+            };
+        };
+    };
+...
-- 
2.29.2

[PATCH v2 2/4] thermal/drivers/qcom/spmi-adc-tm5: Add support for HC variant

[Bjorn Andersson]

The variant of the ADC Thermal Monitor block found in e.g. PM8998 is
"HC", add support for this variant to the ADC TM5 driver in order to
support using VADC channels as thermal_zones on SDM845 et al.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- New patch

 drivers/thermal/qcom/qcom-spmi-adc-tm5.c | 43 +++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/qcom/qcom-spmi-adc-tm5.c b/drivers/thermal/qcom/qcom-spmi-adc-tm5.c
index 8494cc04aa21..7fe5cf28ae15 100644
--- a/drivers/thermal/qcom/qcom-spmi-adc-tm5.c
+++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5.c
@@ -82,6 +82,7 @@ struct adc_tm5_data {
 	const u32	full_scale_code_volt;
 	unsigned int	*decimation;
 	unsigned int	*hw_settle;
+	bool		is_hc;
 };
 
 enum adc_tm5_cal_method {
@@ -146,6 +147,14 @@ static const struct adc_tm5_data adc_tm5_data_pmic = {
 					 64000, 128000 },
 };
 
+static const struct adc_tm5_data adc_tm_hc_data_pmic = {
+	.full_scale_code_volt = 0x70e4,
+	.decimation = (unsigned int []) { 256, 512, 1024 },
+	.hw_settle = (unsigned int []) { 0, 100, 200, 300, 400, 500, 600, 700,
+					 1000, 2000, 4000, 6000, 8000, 10000 },
+	.is_hc = true,
+};
+
 static int adc_tm5_read(struct adc_tm5_chip *adc_tm, u16 offset, u8 *data, int len)
 {
 	return regmap_bulk_read(adc_tm->regmap, adc_tm->base + offset, data, len);
@@ -375,6 +384,31 @@ static int adc_tm5_register_tzd(struct adc_tm5_chip *adc_tm)
 	return 0;
 }
 
+static int adc_tm_hc_init(struct adc_tm5_chip *chip)
+{
+	unsigned int i;
+	u8 buf[2];
+	int ret;
+
+	for (i = 0; i < chip->nchannels; i++) {
+		if (chip->channels[i].channel >= ADC_TM5_NUM_CHANNELS) {
+			dev_err(chip->dev, "Invalid channel %d\n", chip->channels[i].channel);
+			return -EINVAL;
+		}
+	}
+
+	buf[0] = chip->decimation;
+	buf[1] = chip->avg_samples | ADC_TM5_FAST_AVG_EN;
+
+	ret = adc_tm5_write(chip, ADC_TM5_ADC_DIG_PARAM, buf, sizeof(buf));
+	if (ret) {
+		dev_err(chip->dev, "block write failed: %d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
 static int adc_tm5_init(struct adc_tm5_chip *chip)
 {
 	u8 buf[4], channels_available;
@@ -591,7 +625,10 @@ static int adc_tm5_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = adc_tm5_init(adc_tm);
+	if (adc_tm->data->is_hc)
+		ret = adc_tm_hc_init(adc_tm);
+	else
+		ret = adc_tm5_init(adc_tm);
 	if (ret) {
 		dev_err(dev, "adc-tm init failed\n");
 		return ret;
@@ -612,6 +649,10 @@ static const struct of_device_id adc_tm5_match_table[] = {
 		.compatible = "qcom,spmi-adc-tm5",
 		.data = &adc_tm5_data_pmic,
 	},
+	{
+		.compatible = "qcom,spmi-adc-tm-hc",
+		.data = &adc_tm_hc_data_pmic,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adc_tm5_match_table);
-- 
2.29.2

[PATCH v2 3/4] arm64: dts: qcom: pm8998: Add ADC Thermal Monitor node

[Bjorn Andersson]

Add a node for the ADC Thermal Monitor found in the PM8998 PMIC. This is
used to connect thermal zones with ADC channels.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- New patch

 arch/arm64/boot/dts/qcom/pm8998.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
index 6f5bb6b37ec2..d09f2954b6f9 100644
--- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
@@ -84,6 +84,16 @@ adc-chan@6 {
 			};
 		};
 
+		pm8998_adc_tm: adc-tm@3400 {
+			compatible = "qcom,spmi-adc-tm-hc";
+			reg = <0x3400>;
+			interrupts = <0x0 0x34 0x0 IRQ_TYPE_EDGE_RISING>;
+			#thermal-sensor-cells = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			status = "disabled";
+		};
+
 		rtc@6000 {
 			compatible = "qcom,pm8941-rtc";
 			reg = <0x6000>, <0x6100>;
-- 
2.29.2

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

[Bjorn Andersson]

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>;
+	};
+};
+
 &qupv3_id_1 {
 	status = "okay";
 };
-- 
2.29.2

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

[Matthias Kaehlcke]

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?

Re: [PATCH v2 2/4] thermal/drivers/qcom/spmi-adc-tm5: Add support for HC variant

[Vladimir Zapolskiy]

Hi Bjorn,

On 9/24/21 12:23 AM, Bjorn Andersson wrote:
> The variant of the ADC Thermal Monitor block found in e.g. PM8998 is
> "HC", add support for this variant to the ADC TM5 driver in order to
> support using VADC channels as thermal_zones on SDM845 et al.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - New patch
> 
>   drivers/thermal/qcom/qcom-spmi-adc-tm5.c | 43 +++++++++++++++++++++++-
>   1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/qcom/qcom-spmi-adc-tm5.c b/drivers/thermal/qcom/qcom-spmi-adc-tm5.c
> index 8494cc04aa21..7fe5cf28ae15 100644
> --- a/drivers/thermal/qcom/qcom-spmi-adc-tm5.c
> +++ b/drivers/thermal/qcom/qcom-spmi-adc-tm5.c
> @@ -82,6 +82,7 @@ struct adc_tm5_data {
>   	const u32	full_scale_code_volt;
>   	unsigned int	*decimation;
>   	unsigned int	*hw_settle;
> +	bool		is_hc;
>   };
>   
>   enum adc_tm5_cal_method {
> @@ -146,6 +147,14 @@ static const struct adc_tm5_data adc_tm5_data_pmic = {
>   					 64000, 128000 },
>   };
>   
> +static const struct adc_tm5_data adc_tm_hc_data_pmic = {
> +	.full_scale_code_volt = 0x70e4,
> +	.decimation = (unsigned int []) { 256, 512, 1024 },
> +	.hw_settle = (unsigned int []) { 0, 100, 200, 300, 400, 500, 600, 700,
> +					 1000, 2000, 4000, 6000, 8000, 10000 },
> +	.is_hc = true,
> +};
> +
>   static int adc_tm5_read(struct adc_tm5_chip *adc_tm, u16 offset, u8 *data, int len)
>   {
>   	return regmap_bulk_read(adc_tm->regmap, adc_tm->base + offset, data, len);
> @@ -375,6 +384,31 @@ static int adc_tm5_register_tzd(struct adc_tm5_chip *adc_tm)
>   	return 0;
>   }
>   
> +static int adc_tm_hc_init(struct adc_tm5_chip *chip)
> +{
> +	unsigned int i;
> +	u8 buf[2];
> +	int ret;
> +
> +	for (i = 0; i < chip->nchannels; i++) {
> +		if (chip->channels[i].channel >= ADC_TM5_NUM_CHANNELS) {
> +			dev_err(chip->dev, "Invalid channel %d\n", chip->channels[i].channel);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	buf[0] = chip->decimation;
> +	buf[1] = chip->avg_samples | ADC_TM5_FAST_AVG_EN;
> +
> +	ret = adc_tm5_write(chip, ADC_TM5_ADC_DIG_PARAM, buf, sizeof(buf));
> +	if (ret) {
> +		dev_err(chip->dev, "block write failed: %d\n", ret);
> +		return ret;
> +	}

	if (ret)
		dev_err(chip->dev, "block write failed: %d\n", ret);

above should be sufficient here and similarly in adc_tm5_init().

> +
> +	return ret;
> +}
> +
>   static int adc_tm5_init(struct adc_tm5_chip *chip)
>   {
>   	u8 buf[4], channels_available;
> @@ -591,7 +625,10 @@ static int adc_tm5_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> -	ret = adc_tm5_init(adc_tm);
> +	if (adc_tm->data->is_hc)
> +		ret = adc_tm_hc_init(adc_tm);
> +	else
> +		ret = adc_tm5_init(adc_tm);
>   	if (ret) {
>   		dev_err(dev, "adc-tm init failed\n");
>   		return ret;
> @@ -612,6 +649,10 @@ static const struct of_device_id adc_tm5_match_table[] = {
>   		.compatible = "qcom,spmi-adc-tm5",
>   		.data = &adc_tm5_data_pmic,
>   	},
> +	{
> +		.compatible = "qcom,spmi-adc-tm-hc",
> +		.data = &adc_tm_hc_data_pmic,
> +	},
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(of, adc_tm5_match_table);
> 

--
Best wishes,
Vladimir

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

[Bjorn Andersson]

On Wed 29 Sep 11:40 PDT 2021, Matthias Kaehlcke wrote:

> 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
[..]
> > +&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?

Hmm, you're right and I missed this in defining my channels. I've not
looked at this detail, just got reasonable readings from my thermal
zones and was happy about that.

Dmitry, do you have any further insights why these properties are
supposed to be duplicated between the adc channel and the thermal zones?

Thanks,
Bjorn

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

[Dmitry Baryshkov]

On Mon, 4 Oct 2021 at 23:13, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>
> On Wed 29 Sep 11:40 PDT 2021, Matthias Kaehlcke wrote:
>
> > 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
> [..]
> > > +&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?
>
> Hmm, you're right and I missed this in defining my channels. I've not
> looked at this detail, just got reasonable readings from my thermal
> zones and was happy about that.
>
> Dmitry, do you have any further insights why these properties are
> supposed to be duplicated between the adc channel and the thermal zones?

Because both ADC channel and thermal zone registers should be
programmed accordingly.

One not-so-perfect approach would be to use io-channels property to
locate the adc's adc-chan node and to parse it. However this way
thermal driver would have to know the exact structure of adc's device
tree nodes.
Another (even worse) solution would be to introduce qcom-specific API
to query these properties from the IIO channel.

Selecting between these two options I decided to follow the downstream
path and just to duplicate these few properties.

-- 
With best wishes
Dmitry

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

[Bjorn Andersson]

On Mon 04 Oct 15:56 CDT 2021, Dmitry Baryshkov wrote:

> On Mon, 4 Oct 2021 at 23:13, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> >
> > On Wed 29 Sep 11:40 PDT 2021, Matthias Kaehlcke wrote:
> >
> > > 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
> > [..]
> > > > +&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?
> >
> > Hmm, you're right and I missed this in defining my channels. I've not
> > looked at this detail, just got reasonable readings from my thermal
> > zones and was happy about that.
> >
> > Dmitry, do you have any further insights why these properties are
> > supposed to be duplicated between the adc channel and the thermal zones?
> 
> Because both ADC channel and thermal zone registers should be
> programmed accordingly.
> 

I presume our question is "why" to this particular part. The actual
implementation thereof seems reasonable.

Regards,
Bjorn

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

[Thara Gopinath]

On 10/4/21 4:56 PM, Dmitry Baryshkov wrote:
> On Mon, 4 Oct 2021 at 23:13, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>>
>> On Wed 29 Sep 11:40 PDT 2021, Matthias Kaehlcke wrote:
>>
>>> 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
>> [..]
>>>> +&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?
>>
>> Hmm, you're right and I missed this in defining my channels. I've not
>> looked at this detail, just got reasonable readings from my thermal
>> zones and was happy about that.
>>
>> Dmitry, do you have any further insights why these properties are
>> supposed to be duplicated between the adc channel and the thermal zones?
> 
> Because both ADC channel and thermal zone registers should be
> programmed accordingly.
> 
> One not-so-perfect approach would be to use io-channels property to
> locate the adc's adc-chan node and to parse it. However this way
> thermal driver would have to know the exact structure of adc's device
> tree nodes.
> Another (even worse) solution would be to introduce qcom-specific API
> to query these properties from the IIO channel.
> 
> Selecting between these two options I decided to follow the downstream
> path and just to duplicate these few properties.

I know I am joining late to this party, since v3 is out. But before I 
send my acked-by, wanted to check/state a few things.
To me, the not-so-perfect approach that you mention here is actually not 
so bad, considering the thermal sensor driver already is aware of 
internals of the adc device. The duplicating of node is kind of confusing.
Also did you explore using one of _channel_write apis in iio framework 
to move all the writes into the adc device instead of thermal sensor 
driver ?

> 

-- 
Warm Regards
Thara (She/Her/Hers)

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

[Bjorn Andersson]

On Tue 05 Oct 13:30 PDT 2021, Thara Gopinath wrote:

> 
> 
> On 10/4/21 4:56 PM, Dmitry Baryshkov wrote:
> > On Mon, 4 Oct 2021 at 23:13, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> > > 
> > > On Wed 29 Sep 11:40 PDT 2021, Matthias Kaehlcke wrote:
> > > 
> > > > 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
> > > [..]
> > > > > +&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?
> > > 
> > > Hmm, you're right and I missed this in defining my channels. I've not
> > > looked at this detail, just got reasonable readings from my thermal
> > > zones and was happy about that.
> > > 
> > > Dmitry, do you have any further insights why these properties are
> > > supposed to be duplicated between the adc channel and the thermal zones?
> > 
> > Because both ADC channel and thermal zone registers should be
> > programmed accordingly.
> > 
> > One not-so-perfect approach would be to use io-channels property to
> > locate the adc's adc-chan node and to parse it. However this way
> > thermal driver would have to know the exact structure of adc's device
> > tree nodes.
> > Another (even worse) solution would be to introduce qcom-specific API
> > to query these properties from the IIO channel.
> > 
> > Selecting between these two options I decided to follow the downstream
> > path and just to duplicate these few properties.
> 
> I know I am joining late to this party, since v3 is out. But before I send
> my acked-by, wanted to check/state a few things.
> To me, the not-so-perfect approach that you mention here is actually not so
> bad, considering the thermal sensor driver already is aware of internals of
> the adc device. The duplicating of node is kind of confusing.
> Also did you explore using one of _channel_write apis in iio framework to
> move all the writes into the adc device instead of thermal sensor driver ?
> 

To me the TM hardware block seems to just duplicate some parts of the
ADC block. Without understanding why the two would need to be configured
in sync, I don't think we should tie them together further.

In particular, I get the same readings with or without configuring the
ADC channels, does this perhaps imply that the TM configuration is used
to configure the ADC during it's reads (i.e. we don't need to define the
ADC channels after all), or is this just a coincidence?

There are other things in the PMIC, using the ADC seemingly without you
having to explicitly configure the ADC and there's definitely things
pointing to the ADC being "timeshared" between various users.

Regards,
Bjorn

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

[Thara Gopinath]

On 10/5/21 5:52 PM, Bjorn Andersson wrote:
> On Tue 05 Oct 13:30 PDT 2021, Thara Gopinath wrote:
> 
>>
>>
>> On 10/4/21 4:56 PM, Dmitry Baryshkov wrote:
>>> On Mon, 4 Oct 2021 at 23:13, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>>>>
>>>> On Wed 29 Sep 11:40 PDT 2021, Matthias Kaehlcke wrote:
>>>>
>>>>> 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
>>>> [..]
>>>>>> +&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?
>>>>
>>>> Hmm, you're right and I missed this in defining my channels. I've not
>>>> looked at this detail, just got reasonable readings from my thermal
>>>> zones and was happy about that.
>>>>
>>>> Dmitry, do you have any further insights why these properties are
>>>> supposed to be duplicated between the adc channel and the thermal zones?
>>>
>>> Because both ADC channel and thermal zone registers should be
>>> programmed accordingly.
>>>
>>> One not-so-perfect approach would be to use io-channels property to
>>> locate the adc's adc-chan node and to parse it. However this way
>>> thermal driver would have to know the exact structure of adc's device
>>> tree nodes.
>>> Another (even worse) solution would be to introduce qcom-specific API
>>> to query these properties from the IIO channel.
>>>
>>> Selecting between these two options I decided to follow the downstream
>>> path and just to duplicate these few properties.
>>
>> I know I am joining late to this party, since v3 is out. But before I send
>> my acked-by, wanted to check/state a few things.
>> To me, the not-so-perfect approach that you mention here is actually not so
>> bad, considering the thermal sensor driver already is aware of internals of
>> the adc device. The duplicating of node is kind of confusing.
>> Also did you explore using one of _channel_write apis in iio framework to
>> move all the writes into the adc device instead of thermal sensor driver ?
>>
> 
> To me the TM hardware block seems to just duplicate some parts of the
> ADC block. Without understanding why the two would need to be configured
> in sync, I don't think we should tie them together further.

You are right. It is duplicating most of the ADC parts except for 
temperature read for which it calls into the adc device. And I don't see 
any interface from the ADC driver that can be used to do the 
configuration TM needs. Hence the question, have we ever explored this 
path ?


> 
> In particular, I get the same readings with or without configuring the
> ADC channels, does this perhaps imply that the TM configuration is used
> to configure the ADC during it's reads (i.e. we don't need to define the
> ADC channels after all), or is this just a coincidence?

Maybe because of what we do in adc_tm5_init ?

If needed, we should be able to duplicate the read in the TM as well and 
get rid of ADC channels , right ? I don't like this half configuration 
in TM and half in ADC device unless there is a reason behind this.

> 
> There are other things in the PMIC, using the ADC seemingly without you
> having to explicitly configure the ADC and there's definitely things
> pointing to the ADC being "timeshared" between various users.
> 
> Regards,
> Bjorn
> 

-- 
Warm Regards
Thara (She/Her/Hers)

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

[Bjorn Andersson]

On Tue 05 Oct 15:21 PDT 2021, Thara Gopinath wrote:

> 
> 
> On 10/5/21 5:52 PM, Bjorn Andersson wrote:
> > On Tue 05 Oct 13:30 PDT 2021, Thara Gopinath wrote:
> > 
> > > 
> > > 
> > > On 10/4/21 4:56 PM, Dmitry Baryshkov wrote:
> > > > On Mon, 4 Oct 2021 at 23:13, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> > > > > 
> > > > > On Wed 29 Sep 11:40 PDT 2021, Matthias Kaehlcke wrote:
> > > > > 
> > > > > > 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
> > > > > [..]
> > > > > > > +&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?
> > > > > 
> > > > > Hmm, you're right and I missed this in defining my channels. I've not
> > > > > looked at this detail, just got reasonable readings from my thermal
> > > > > zones and was happy about that.
> > > > > 
> > > > > Dmitry, do you have any further insights why these properties are
> > > > > supposed to be duplicated between the adc channel and the thermal zones?
> > > > 
> > > > Because both ADC channel and thermal zone registers should be
> > > > programmed accordingly.
> > > > 
> > > > One not-so-perfect approach would be to use io-channels property to
> > > > locate the adc's adc-chan node and to parse it. However this way
> > > > thermal driver would have to know the exact structure of adc's device
> > > > tree nodes.
> > > > Another (even worse) solution would be to introduce qcom-specific API
> > > > to query these properties from the IIO channel.
> > > > 
> > > > Selecting between these two options I decided to follow the downstream
> > > > path and just to duplicate these few properties.
> > > 
> > > I know I am joining late to this party, since v3 is out. But before I send
> > > my acked-by, wanted to check/state a few things.
> > > To me, the not-so-perfect approach that you mention here is actually not so
> > > bad, considering the thermal sensor driver already is aware of internals of
> > > the adc device. The duplicating of node is kind of confusing.
> > > Also did you explore using one of _channel_write apis in iio framework to
> > > move all the writes into the adc device instead of thermal sensor driver ?
> > > 
> > 
> > To me the TM hardware block seems to just duplicate some parts of the
> > ADC block. Without understanding why the two would need to be configured
> > in sync, I don't think we should tie them together further.
> 
> You are right. It is duplicating most of the ADC parts except for
> temperature read for which it calls into the adc device. And I don't see any
> interface from the ADC driver that can be used to do the configuration TM
> needs. Hence the question, have we ever explored this path ?
> 
> 
> > 
> > In particular, I get the same readings with or without configuring the
> > ADC channels, does this perhaps imply that the TM configuration is used
> > to configure the ADC during it's reads (i.e. we don't need to define the
> > ADC channels after all), or is this just a coincidence?
> 
> Maybe because of what we do in adc_tm5_init ?
> 
> If needed, we should be able to duplicate the read in the TM as well and get
> rid of ADC channels , right ? I don't like this half configuration in TM and
> half in ADC device unless there is a reason behind this.
> 

Looking at the registers once more makes me more convinced that the PMIC
is using the ADC hardware behind the back of the ADC driver - for the
purpose of detecting if we pass the trip points.

And then we use the ADC driver to read the temperature, separate of
that. To expose a thermal_zone with both trip points and temperature.

That way it would be in our best interest to configure the TM and the
ADC in the same way, so the trips and temperature readings are matching,
but it really is two different interfaces to the underlying ADC
hardware.

Regards,
Bjorn

> > 
> > There are other things in the PMIC, using the ADC seemingly without you
> > having to explicitly configure the ADC and there's definitely things
> > pointing to the ADC being "timeshared" between various users.
> > 
> > Regards,
> > Bjorn
> > 
> 
> -- 
> Warm Regards
> Thara (She/Her/Hers)

Re: [PATCH v2 3/4] arm64: dts: qcom: pm8998: Add ADC Thermal Monitor node

[Dmitry Baryshkov]

On 24/09/2021 00:23, Bjorn Andersson wrote:
> Add a node for the ADC Thermal Monitor found in the PM8998 PMIC. This is
> used to connect thermal zones with ADC channels.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
> 
> Changes since v1:
> - New patch
> 
>   arch/arm64/boot/dts/qcom/pm8998.dtsi | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/pm8998.dtsi b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> index 6f5bb6b37ec2..d09f2954b6f9 100644
> --- a/arch/arm64/boot/dts/qcom/pm8998.dtsi
> +++ b/arch/arm64/boot/dts/qcom/pm8998.dtsi
> @@ -84,6 +84,16 @@ adc-chan@6 {
>   			};
>   		};
>   
> +		pm8998_adc_tm: adc-tm@3400 {
> +			compatible = "qcom,spmi-adc-tm-hc";
> +			reg = <0x3400>;
> +			interrupts = <0x0 0x34 0x0 IRQ_TYPE_EDGE_RISING>;
> +			#thermal-sensor-cells = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			status = "disabled";
> +		};
> +
>   		rtc@6000 {
>   			compatible = "qcom,pm8941-rtc";
>   			reg = <0x6000>, <0x6100>;
> 


-- 
With best wishes
Dmitry