[PATCH 0/6] Add IPQ5332 TSENS support

[PATCH 0/6] Add IPQ5332 TSENS support

[Praveenkumar I]

IPQ5332 uses tsens v2.3.3 IP with combined interrupt for
upper/lower and critical. IPQ5332 does not have RPM and
kernel has to take care of TSENS enablement and calibration.
This patch series adds the sensor enablement and calibration
support. On top, adds IPQ5332 TSENS support.

Praveenkumar I (6):
  dt-bindings: thermal: tsens: Add nvmem cells for calibration data
  thermal/drivers/tsens: Add TSENS enable and calibration support for V2
  dt-bindings: thermal: tsens: Add ipq5332 compatible
  arm64: dts: qcom: ipq5332: Add tsens node
  arm64: dts: qcom: ipq5332: Add thermal zone nodes
  thermal/drivers/tsens: Add IPQ5332 support

 .../bindings/thermal/qcom-tsens.yaml          |  34 +++-
 arch/arm64/boot/dts/qcom/ipq5332.dtsi         | 185 ++++++++++++++++++
 drivers/thermal/qcom/tsens-v2.c               | 129 ++++++++++++
 drivers/thermal/qcom/tsens.c                  |  40 +++-
 drivers/thermal/qcom/tsens.h                  |  58 +++++-
 5 files changed, 440 insertions(+), 6 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/6] dt-bindings: thermal: tsens: Add nvmem cells for calibration data

[Praveenkumar I]

Add TSENS V2 calibration nvmem cells for IPQ5332

Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
---
 .../bindings/thermal/qcom-tsens.yaml          | 26 +++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
index 27e9e16e6455..8b7863c3989e 100644
--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
@@ -91,7 +91,7 @@ properties:
     maxItems: 2
 
   nvmem-cells:
-    oneOf:
+    anyOf:
       - minItems: 1
         maxItems: 2
         description:
@@ -106,9 +106,13 @@ properties:
         description: |
           Reference to nvmem cells for the calibration mode, two calibration
           bases and two cells per each sensor, main and backup copies, plus use_backup cell
+      - maxItems: 17
+        description: |
+          V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
+          bases and one cell per each sensor
 
   nvmem-cell-names:
-    oneOf:
+    anyOf:
       - minItems: 1
         items:
           - const: calib
@@ -205,6 +209,24 @@ properties:
           - const: s9_p2_backup
           - const: s10_p1_backup
           - const: s10_p2_backup
+      - items:
+          - const: mode
+          - const: base0
+          - const: base1
+          - const: s0_offset
+          - const: s3_offset
+          - const: s4_offset
+          - const: s5_offset
+          - const: s6_offset
+          - const: s7_offset
+          - const: s8_offset
+          - const: s9_offset
+          - const: s10_offset
+          - const: s11_offset
+          - const: s12_offset
+          - const: s13_offset
+          - const: s14_offset
+          - const: s15_offset
 
   "#qcom,sensors":
     description:
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/6] thermal/drivers/tsens: Add TSENS enable and calibration support for V2

[Praveenkumar I]

SoCs without RPM have to enable sensors and calibrate from the kernel.
Though TSENS IP supports 16 sensors, not all are used. So added
sensors_to_en in tsens data help enable the relevant sensors.

Added new calibration function for V2 as the tsens.c calib function
only supports V1.

Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
---
 drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
 drivers/thermal/qcom/tsens.c    |  37 +++++++++-
 drivers/thermal/qcom/tsens.h    |  56 +++++++++++++++
 3 files changed, 208 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
index 29a61d2d6ca3..db48b1d95348 100644
--- a/drivers/thermal/qcom/tsens-v2.c
+++ b/drivers/thermal/qcom/tsens-v2.c
@@ -6,11 +6,20 @@
 
 #include <linux/bitops.h>
 #include <linux/regmap.h>
+#include <linux/nvmem-consumer.h>
 #include "tsens.h"
 
 /* ----- SROT ------ */
 #define SROT_HW_VER_OFF	0x0000
 #define SROT_CTRL_OFF		0x0004
+#define SROT_MEASURE_PERIOD	0x0008
+#define SROT_Sn_CONVERSION	0x0060
+#define V2_SHIFT_DEFAULT	0x0003
+#define V2_SLOPE_DEFAULT	0x0cd0
+#define V2_CZERO_DEFAULT	0x016a
+#define ONE_PT_SLOPE		0x0cd0
+#define TWO_PT_SHIFTED_GAIN	921600
+#define ONE_PT_CZERO_CONST	94
 
 /* ----- TM ------ */
 #define TM_INT_EN_OFF			0x0004
@@ -59,6 +68,16 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
 	/* CTRL_OFF */
 	[TSENS_EN]     = REG_FIELD(SROT_CTRL_OFF,    0,  0),
 	[TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF,    1,  1),
+	[SENSOR_EN]    = REG_FIELD(SROT_CTRL_OFF,    3,  18),
+	[CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF,    21, 21),
+
+	/* MAIN_MEASURE_PERIOD */
+	[MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
+
+	/* Sn Conversion */
+	REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
+	REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
+	REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
 
 	/* ----- TM ------ */
 	/* INTERRUPT ENABLE */
@@ -104,6 +123,103 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
 	[TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
 };
 
+static int tsens_v2_calibration(struct tsens_priv *priv)
+{
+	struct device *dev = priv->dev;
+	u32 mode, base0, base1;
+	u32 slope, czero;
+	char name[15];
+	int i, j, ret;
+
+	if (priv->num_sensors > MAX_SENSORS)
+		return -EINVAL;
+
+	ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
+	if (ret == -ENOENT)
+		dev_warn(priv->dev, "Calibration data not present in DT\n");
+	if (ret < 0)
+		return ret;
+
+	dev_dbg(priv->dev, "calibration mode is %d\n", mode);
+
+	ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
+	if (ret < 0)
+		return ret;
+
+	ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
+	if (ret < 0)
+		return ret;
+
+	/* Read offset values and allocate SHIFT, SLOPE & CZERO regmap for enabled sensors */
+	for (i = 0; i < priv->num_sensors; i++) {
+		if (!(priv->sensors_to_en & (0x1 << i)))
+			continue;
+
+		ret = snprintf(name, sizeof(name), "s%d_offset", priv->sensor[i].hw_id);
+		if (ret < 0)
+			return ret;
+
+		ret = nvmem_cell_read_variable_le_u32(priv->dev, name, &priv->sensor[i].offset);
+		if (ret)
+			return ret;
+
+		for (j = SHIFT_0; j <= CZERO_0; j++) {
+			int idx = (i * 3) + j;
+
+			priv->rf[idx] = devm_regmap_field_alloc(dev, priv->srot_map,
+								priv->fields[idx]);
+			if (IS_ERR(priv->rf[idx]))
+				return PTR_ERR(priv->rf[idx]);
+		}
+	}
+
+	/* Based on calib mode, program SHIFT, SLOPE and CZERO for enabled sensors */
+	switch (mode) {
+	case TWO_PT_CALIB:
+		slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
+
+		for (i = 0; i < priv->num_sensors; i++) {
+			if (!(priv->sensors_to_en & (0x1 << i)))
+				continue;
+
+			int idx = i * 3;
+
+			czero = (base0 + priv->sensor[i].offset - ((base1 - base0) / 3));
+			regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
+			regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
+			regmap_field_write(priv->rf[CZERO_0 + idx], czero);
+		}
+		fallthrough;
+	case ONE_PT_CALIB2:
+		for (i = 0; i < priv->num_sensors; i++) {
+			if (!(priv->sensors_to_en & (0x1 << i)))
+				continue;
+
+			int idx = i * 3;
+
+			czero = base0 + priv->sensor[i].offset - ONE_PT_CZERO_CONST;
+			regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
+			regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
+			regmap_field_write(priv->rf[CZERO_0 + idx], czero);
+		}
+		break;
+	default:
+		dev_dbg(priv->dev, "calibrationless mode\n");
+		for (i = 0; i < priv->num_sensors; i++) {
+			if (!(priv->sensors_to_en & (0x1 << i)))
+				continue;
+
+			int idx = i * 3;
+
+			regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
+			regmap_field_write(priv->rf[SLOPE_0 + idx], V2_SLOPE_DEFAULT);
+			regmap_field_write(priv->rf[CZERO_0 + idx], V2_CZERO_DEFAULT);
+		}
+	}
+
+	return 0;
+}
+
 static const struct tsens_ops ops_generic_v2 = {
 	.init		= init_common,
 	.get_temp	= get_temp_tsens_valid,
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 98c356acfe98..169690355dad 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
 	ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
 	if (ret)
 		goto err_put_device;
-	if (!enabled) {
+	if (!enabled && !priv->sensors_to_en) {
 		dev_err(dev, "%s: device not enabled\n", __func__);
 		ret = -ENODEV;
 		goto err_put_device;
@@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
 		goto err_put_device;
 	}
 
+	/* Do TSENS initialization if required */
+	if (priv->sensors_to_en) {
+		priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev, priv->srot_map,
+								 priv->fields[CODE_OR_TEMP]);
+		if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
+			ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
+			goto err_put_device;
+		}
+
+		priv->rf[MAIN_MEASURE_PERIOD] =
+			devm_regmap_field_alloc(dev, priv->srot_map,
+						priv->fields[MAIN_MEASURE_PERIOD]);
+		if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
+			ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
+			goto err_put_device;
+		}
+
+		regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
+
+		/* Update measure period to 2ms */
+		regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
+
+		/* Enable available sensors */
+		regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
+
+		/* Real temperature format */
+		regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
+
+		regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
+
+		/* Enable TSENS */
+		regmap_field_write(priv->rf[TSENS_EN], 0x1);
+	}
+
 	/* This loop might need changes if enum regfield_ids is reordered */
 	for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
 		for (i = 0; i < priv->feat->max_sensors; i++) {
@@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device *pdev)
 
 	priv->dev = dev;
 	priv->num_sensors = num_sensors;
+	priv->sensors_to_en = data->sensors_to_en;
 	priv->ops = data->ops;
 	for (i = 0;  i < priv->num_sensors; i++) {
 		if (data->hw_ids)
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index 2805de1c6827..f8897bc8944e 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -168,6 +168,58 @@ enum regfield_ids {
 	TSENS_SW_RST,
 	SENSOR_EN,
 	CODE_OR_TEMP,
+	/* MEASURE_PERIOD */
+	MAIN_MEASURE_PERIOD,
+
+	/* Sn_CONVERSION */
+	SHIFT_0,
+	SLOPE_0,
+	CZERO_0,
+	SHIFT_1,
+	SLOPE_1,
+	CZERO_1,
+	SHIFT_2,
+	SLOPE_2,
+	CZERO_2,
+	SHIFT_3,
+	SLOPE_3,
+	CZERO_3,
+	SHIFT_4,
+	SLOPE_4,
+	CZERO_4,
+	SHIFT_5,
+	SLOPE_5,
+	CZERO_5,
+	SHIFT_6,
+	SLOPE_6,
+	CZERO_6,
+	SHIFT_7,
+	SLOPE_7,
+	CZERO_7,
+	SHIFT_8,
+	SLOPE_8,
+	CZERO_8,
+	SHIFT_9,
+	SLOPE_9,
+	CZERO_9,
+	SHIFT_10,
+	SLOPE_10,
+	CZERO_10,
+	SHIFT_11,
+	SLOPE_11,
+	CZERO_11,
+	SHIFT_12,
+	SLOPE_12,
+	CZERO_12,
+	SHIFT_13,
+	SLOPE_13,
+	CZERO_13,
+	SHIFT_14,
+	SLOPE_14,
+	CZERO_14,
+	SHIFT_15,
+	SLOPE_15,
+	CZERO_15,
 
 	/* ----- TM ------ */
 	/* TRDY */
@@ -524,6 +576,7 @@ struct tsens_features {
 /**
  * struct tsens_plat_data - tsens compile-time platform data
  * @num_sensors: Number of sensors supported by platform
+ * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
  * @ops: operations the tsens instance supports
  * @hw_ids: Subset of sensors ids supported by platform, if not the first n
  * @feat: features of the IP
@@ -531,6 +584,7 @@ struct tsens_features {
  */
 struct tsens_plat_data {
 	const u32		num_sensors;
+	const u16		sensors_to_en;
 	const struct tsens_ops	*ops;
 	unsigned int		*hw_ids;
 	struct tsens_features	*feat;
@@ -551,6 +605,7 @@ struct tsens_context {
  * struct tsens_priv - private data for each instance of the tsens IP
  * @dev: pointer to struct device
  * @num_sensors: number of sensors enabled on this device
+ * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
  * @tm_map: pointer to TM register address space
  * @srot_map: pointer to SROT register address space
  * @tm_offset: deal with old device trees that don't address TM and SROT
@@ -569,6 +624,7 @@ struct tsens_context {
 struct tsens_priv {
 	struct device			*dev;
 	u32				num_sensors;
+	u16				sensors_to_en;
 	struct regmap			*tm_map;
 	struct regmap			*srot_map;
 	u32				tm_offset;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 3/6] dt-bindings: thermal: tsens: Add ipq5332 compatible

[Praveenkumar I]

IPQ5332 uses TSENS v2.3.3 with combined interrupt. RPM is not
available in the SoC, hence adding new compatible to have the
sensor enablement and calibration function.

Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
---
 Documentation/devicetree/bindings/thermal/qcom-tsens.yaml | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
index 8b7863c3989e..ee57713f6131 100644
--- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
+++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
@@ -68,8 +68,10 @@ properties:
           - const: qcom,tsens-v2
 
       - description: v2 of TSENS with combined interrupt
-        enum:
-          - qcom,ipq8074-tsens
+        items:
+          - enum:
+              - qcom,ipq8074-tsens
+              - qcom,ipq5332-tsens
 
       - description: v2 of TSENS with combined interrupt
         items:
@@ -289,6 +291,7 @@ allOf:
           contains:
             enum:
               - qcom,ipq8074-tsens
+              - qcom,ipq5332-tsens
     then:
       properties:
         interrupts:
@@ -304,6 +307,7 @@ allOf:
           contains:
             enum:
               - qcom,ipq8074-tsens
+              - qcom,ipq5332-tsens
               - qcom,tsens-v0_1
               - qcom,tsens-v1
               - qcom,tsens-v2
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 4/6] arm64: dts: qcom: ipq5332: Add tsens node

[Praveenkumar I]

IPQ5332 has tsens v2.3.3 peripheral. This patch adds the tsense
node with nvmem cells for calibration data.

Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq5332.dtsi | 113 ++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
index 8bfc2db44624..a1e3527178c0 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
@@ -150,6 +150,91 @@ qfprom: efuse@a4000 {
 			reg = <0x000a4000 0x721>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+
+			tsens_mode: mode@3e1 {
+				reg = <0x3e1 0x1>;
+				bits = <0 3>;
+			};
+
+			tsens_base0: base0@3e1 {
+				reg = <0x3e1 0x2>;
+				bits = <3 10>;
+			};
+
+			tsens_base1: base1@3e2 {
+				reg = <0x3e2 0x2>;
+				bits = <5 10>;
+			};
+
+			s0_offset: s0_offset@3e4 {
+				reg = <0x3e4 0x1>;
+				bits = <0 4>;
+			};
+
+			s3_offset: s3_offset@3e5 {
+				reg = <0x3e5 0x1>;
+				bits = <4 4>;
+			};
+
+			s4_offset: s4_offset@3e6 {
+				reg = <0x3e6 0x1>;
+				bits = <0 4>;
+			};
+
+			s5_offset: s5_offset@3e6 {
+				reg = <0x3e6 0x1>;
+				bits = <4 4>;
+			};
+
+			s6_offset: s6_offset@3e8 {
+				reg = <0x3e8 0x1>;
+				bits = <0 4>;
+			};
+
+			s7_offset: s7_offset@3e8 {
+				reg = <0x3e8 0x1>;
+				bits = <4 4>;
+			};
+
+			s8_offset: s8_offset@3a4 {
+				reg = <0x3a4 0x1>;
+				bits = <0 4>;
+			};
+
+			s9_offset: s9_offset@3a4 {
+				reg = <0x3a4 0x1>;
+				bits = <4 4>;
+			};
+
+			s10_offset: s10_offset@3a5 {
+				reg = <0x3a5 0x1>;
+				bits = <0 4>;
+			};
+
+			s11_offset: s11_offset@3a5 {
+				reg = <0x3a5 0x1>;
+				bits = <4 4>;
+			};
+
+			s12_offset: s12_offset@3a6 {
+				reg = <0x3a6 0x1>;
+				bits = <0 4>;
+			};
+
+			s13_offset: s13_offset@3a6 {
+				reg = <0x3a6 0x1>;
+				bits = <4 4>;
+			};
+
+			s14_offset: s14_offset@3ad {
+				reg = <0x3ad 0x2>;
+				bits = <7 4>;
+			};
+
+			s15_offset: s0_offset@3ae {
+				reg = <0x3ae 0x1>;
+				bits = <3 4>;
+			};
 		};
 
 		rng: rng@e3000 {
@@ -159,6 +244,34 @@ rng: rng@e3000 {
 			clock-names = "core";
 		};
 
+		tsens: thermal-sensor@4a9000 {
+			compatible = "qcom,ipq5332-tsens";
+			reg = <0x4a9000 0x1000>,
+			      <0x4a8000 0x1000>;
+			nvmem-cells = <&tsens_mode>, <&tsens_base0>,
+					<&tsens_base1>, <&s0_offset>,
+					<&s3_offset>, <&s4_offset>,
+					<&s5_offset>, <&s6_offset>,
+					<&s7_offset>, <&s8_offset>,
+					<&s9_offset>, <&s10_offset>,
+					<&s11_offset>, <&s12_offset>,
+					<&s13_offset>, <&s14_offset>,
+					<&s15_offset>;
+			nvmem-cell-names = "mode", "base0",
+						"base1", "s0_offset",
+						"s3_offset", "s4_offset",
+						"s5_offset", "s6_offset",
+						"s7_offset", "s8_offset",
+						"s9_offset", "s10_offset",
+						"s11_offset", "s12_offset",
+						"s13_offset", "s14_offset",
+						"s15_offset";
+			interrupts = <GIC_SPI 320 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "combined";
+			#qcom,sensors = <16>;
+			#thermal-sensor-cells = <1>;
+		};
+
 		tlmm: pinctrl@1000000 {
 			compatible = "qcom,ipq5332-tlmm";
 			reg = <0x01000000 0x300000>;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 5/6] arm64: dts: qcom: ipq5332: Add thermal zone nodes

[Praveenkumar I]

This patch adds thermal zone nodes for sensors present in
IPQ5332.

Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72 +++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
index a1e3527178c0..8b276aeca53e 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
@@ -527,4 +527,76 @@ timer {
 			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
 			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
 	};
+
+	thermal-zones {
+		rfa-0-thermal{
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&tsens 11>;
+
+			trips {
+				rfa-0-critical {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "critical";
+				};
+			};
+		};
+
+		rfa-1-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&tsens 12>;
+
+			trips {
+				rfa-1-critical {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "critical";
+				};
+			};
+		};
+
+		misc-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&tsens 13>;
+
+			trips {
+				misc-critical {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "critical";
+				};
+			};
+		};
+
+		cpu-top-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&tsens 14>;
+
+			trips {
+				cpu-top-critical {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "critical";
+				};
+			};
+		};
+
+		top-glue-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&tsens 15>;
+
+			trips {
+				top-glue-critical {
+					temperature = <125000>;
+					hysteresis = <1000>;
+					type = "critical";
+				};
+			};
+		};
+	};
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 6/6] thermal/drivers/tsens: Add IPQ5332 support

[Praveenkumar I]

IPQ5332 uses tsens v2.3.3 IP and it is having combined interrupt as
like IPQ8074. But as the SoCs does not have RPM, kernel needs to
take care of sensor enablement and calibration. Hence introduced
new ops and data for IPQ5332 and reused the feature_config from
IPQ8074.

Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
---
 drivers/thermal/qcom/tsens-v2.c | 13 +++++++++++++
 drivers/thermal/qcom/tsens.c    |  3 +++
 drivers/thermal/qcom/tsens.h    |  2 +-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
index db48b1d95348..8b6e3876fd2c 100644
--- a/drivers/thermal/qcom/tsens-v2.c
+++ b/drivers/thermal/qcom/tsens-v2.c
@@ -237,6 +237,19 @@ struct tsens_plat_data data_ipq8074 = {
 	.fields	= tsens_v2_regfields,
 };
 
+static const struct tsens_ops ops_ipq5332_v2 = {
+	.init		= init_common,
+	.get_temp	= get_temp_tsens_valid,
+	.calibrate	= tsens_v2_calibration,
+};
+
+struct tsens_plat_data data_ipq5332 = {
+	.sensors_to_en	= 0xF800,
+	.ops		= &ops_ipq5332_v2,
+	.feat		= &ipq8074_feat,
+	.fields		= tsens_v2_regfields,
+};
+
 /* Kept around for backward compatibility with old msm8996.dtsi */
 struct tsens_plat_data data_8996 = {
 	.num_sensors	= 13,
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index 169690355dad..e8ba2901cda8 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -1140,6 +1140,9 @@ static const struct of_device_id tsens_table[] = {
 	}, {
 		.compatible = "qcom,ipq8074-tsens",
 		.data = &data_ipq8074,
+	}, {
+		.compatible = "qcom,ipq5332-tsens",
+		.data = &data_ipq5332,
 	}, {
 		.compatible = "qcom,mdm9607-tsens",
 		.data = &data_9607,
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index f8897bc8944e..36040f9beebc 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -701,6 +701,6 @@ extern struct tsens_plat_data data_8226, data_8909, data_8916, data_8939, data_8
 extern struct tsens_plat_data data_tsens_v1, data_8976, data_8956;
 
 /* TSENS v2 targets */
-extern struct tsens_plat_data data_8996, data_ipq8074, data_tsens_v2;
+extern struct tsens_plat_data data_8996, data_ipq8074, data_ipq5332, data_tsens_v2;
 
 #endif /* __QCOM_TSENS_H__ */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/6] thermal/drivers/tsens: Add TSENS enable and calibration support for V2

[Dmitry Baryshkov]

On 10/07/2023 13:37, Praveenkumar I wrote:
> SoCs without RPM have to enable sensors and calibrate from the kernel.
> Though TSENS IP supports 16 sensors, not all are used. So added
> sensors_to_en in tsens data help enable the relevant sensors.
> 
> Added new calibration function for V2 as the tsens.c calib function
> only supports V1.
> 
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> ---
>   drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
>   drivers/thermal/qcom/tsens.c    |  37 +++++++++-
>   drivers/thermal/qcom/tsens.h    |  56 +++++++++++++++
>   3 files changed, 208 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index 29a61d2d6ca3..db48b1d95348 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -6,11 +6,20 @@
>   
>   #include <linux/bitops.h>
>   #include <linux/regmap.h>
> +#include <linux/nvmem-consumer.h>
>   #include "tsens.h"
>   
>   /* ----- SROT ------ */
>   #define SROT_HW_VER_OFF	0x0000
>   #define SROT_CTRL_OFF		0x0004
> +#define SROT_MEASURE_PERIOD	0x0008
> +#define SROT_Sn_CONVERSION	0x0060
> +#define V2_SHIFT_DEFAULT	0x0003
> +#define V2_SLOPE_DEFAULT	0x0cd0
> +#define V2_CZERO_DEFAULT	0x016a
> +#define ONE_PT_SLOPE		0x0cd0
> +#define TWO_PT_SHIFTED_GAIN	921600
> +#define ONE_PT_CZERO_CONST	94
>   
>   /* ----- TM ------ */
>   #define TM_INT_EN_OFF			0x0004
> @@ -59,6 +68,16 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
>   	/* CTRL_OFF */
>   	[TSENS_EN]     = REG_FIELD(SROT_CTRL_OFF,    0,  0),
>   	[TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF,    1,  1),
> +	[SENSOR_EN]    = REG_FIELD(SROT_CTRL_OFF,    3,  18),
> +	[CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF,    21, 21),
> +
> +	/* MAIN_MEASURE_PERIOD */
> +	[MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
> +
> +	/* Sn Conversion */
> +	REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
> +	REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
> +	REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
>   
>   	/* ----- TM ------ */
>   	/* INTERRUPT ENABLE */
> @@ -104,6 +123,103 @@ static const struct reg_field tsens_v2_regfields[MAX_REGFIELDS] = {
>   	[TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>   };
>   
> +static int tsens_v2_calibration(struct tsens_priv *priv)
> +{
> +	struct device *dev = priv->dev;
> +	u32 mode, base0, base1;
> +	u32 slope, czero;
> +	char name[15];
> +	int i, j, ret;
> +
> +	if (priv->num_sensors > MAX_SENSORS)
> +		return -EINVAL;
> +
> +	ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
> +	if (ret == -ENOENT)
> +		dev_warn(priv->dev, "Calibration data not present in DT\n");
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_dbg(priv->dev, "calibration mode is %d\n", mode);
> +
> +	ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Read offset values and allocate SHIFT, SLOPE & CZERO regmap for enabled sensors */
> +	for (i = 0; i < priv->num_sensors; i++) {
> +		if (!(priv->sensors_to_en & (0x1 << i)))
> +			continue;
> +
> +		ret = snprintf(name, sizeof(name), "s%d_offset", priv->sensor[i].hw_id);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = nvmem_cell_read_variable_le_u32(priv->dev, name, &priv->sensor[i].offset);
> +		if (ret)
> +			return ret;
> +
> +		for (j = SHIFT_0; j <= CZERO_0; j++) {
> +			int idx = (i * 3) + j;
> +
> +			priv->rf[idx] = devm_regmap_field_alloc(dev, priv->srot_map,
> +								priv->fields[idx]);
> +			if (IS_ERR(priv->rf[idx]))
> +				return PTR_ERR(priv->rf[idx]);

I think, allocating data structures for 48 regfields, which are written 
just once, to be an overkill.

> +		}
> +	}
> +
> +	/* Based on calib mode, program SHIFT, SLOPE and CZERO for enabled sensors */
> +	switch (mode) {
> +	case TWO_PT_CALIB:
> +		slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
> +
> +		for (i = 0; i < priv->num_sensors; i++) {
> +			if (!(priv->sensors_to_en & (0x1 << i)))
> +				continue;
> +
> +			int idx = i * 3;
> +
> +			czero = (base0 + priv->sensor[i].offset - ((base1 - base0) / 3));
> +			regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
> +			regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
> +			regmap_field_write(priv->rf[CZERO_0 + idx], czero);
> +		}
> +		fallthrough;
> +	case ONE_PT_CALIB2:
> +		for (i = 0; i < priv->num_sensors; i++) {
> +			if (!(priv->sensors_to_en & (0x1 << i)))
> +				continue;
> +
> +			int idx = i * 3;
> +
> +			czero = base0 + priv->sensor[i].offset - ONE_PT_CZERO_CONST;
> +			regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
> +			regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
> +			regmap_field_write(priv->rf[CZERO_0 + idx], czero);
> +		}
> +		break;
> +	default:
> +		dev_dbg(priv->dev, "calibrationless mode\n");
> +		for (i = 0; i < priv->num_sensors; i++) {
> +			if (!(priv->sensors_to_en & (0x1 << i)))
> +				continue;
> +
> +			int idx = i * 3;
> +
> +			regmap_field_write(priv->rf[SHIFT_0 + idx], V2_SHIFT_DEFAULT);
> +			regmap_field_write(priv->rf[SLOPE_0 + idx], V2_SLOPE_DEFAULT);
> +			regmap_field_write(priv->rf[CZERO_0 + idx], V2_CZERO_DEFAULT);
> +		}
> +	}

This code iterates over the sensors field several times. Please consider 
extracting a function that handles all setup for a single sensor, then 
calling it in a loop (I should probably do the same for tsens-v0/v1 too).

> +
> +	return 0;
> +}
> +
>   static const struct tsens_ops ops_generic_v2 = {
>   	.init		= init_common,
>   	.get_temp	= get_temp_tsens_valid,
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 98c356acfe98..169690355dad 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
>   	ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
>   	if (ret)
>   		goto err_put_device;
> -	if (!enabled) {
> +	if (!enabled && !priv->sensors_to_en) {
>   		dev_err(dev, "%s: device not enabled\n", __func__);
>   		ret = -ENODEV;
>   		goto err_put_device;
> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
>   		goto err_put_device;
>   	}
>   
> +	/* Do TSENS initialization if required */
> +	if (priv->sensors_to_en) {

Maybe it would be better to explicitly add VER_2_X_NO_RPM and check it here?

> +		priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev, priv->srot_map,
> +								 priv->fields[CODE_OR_TEMP]);
> +		if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
> +			ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
> +			goto err_put_device;
> +		}
> +
> +		priv->rf[MAIN_MEASURE_PERIOD] =
> +			devm_regmap_field_alloc(dev, priv->srot_map,
> +						priv->fields[MAIN_MEASURE_PERIOD]);
> +		if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
> +			ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
> +			goto err_put_device;
> +		}
> +
> +		regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
> +
> +		/* Update measure period to 2ms */
> +		regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
> +
> +		/* Enable available sensors */
> +		regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
> +
> +		/* Real temperature format */
> +		regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
> +
> +		regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
> +
> +		/* Enable TSENS */
> +		regmap_field_write(priv->rf[TSENS_EN], 0x1);
> +	}
> +
>   	/* This loop might need changes if enum regfield_ids is reordered */
>   	for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
>   		for (i = 0; i < priv->feat->max_sensors; i++) {
> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device *pdev)
>   
>   	priv->dev = dev;
>   	priv->num_sensors = num_sensors;
> +	priv->sensors_to_en = data->sensors_to_en;
>   	priv->ops = data->ops;
>   	for (i = 0;  i < priv->num_sensors; i++) {
>   		if (data->hw_ids)
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index 2805de1c6827..f8897bc8944e 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -168,6 +168,58 @@ enum regfield_ids {
>   	TSENS_SW_RST,
>   	SENSOR_EN,
>   	CODE_OR_TEMP,
> +	/* MEASURE_PERIOD */
> +	MAIN_MEASURE_PERIOD,
> +
> +	/* Sn_CONVERSION */
> +	SHIFT_0,
> +	SLOPE_0,
> +	CZERO_0,
> +	SHIFT_1,
> +	SLOPE_1,
> +	CZERO_1,
> +	SHIFT_2,
> +	SLOPE_2,
> +	CZERO_2,
> +	SHIFT_3,
> +	SLOPE_3,
> +	CZERO_3,
> +	SHIFT_4,
> +	SLOPE_4,
> +	CZERO_4,
> +	SHIFT_5,
> +	SLOPE_5,
> +	CZERO_5,
> +	SHIFT_6,
> +	SLOPE_6,
> +	CZERO_6,
> +	SHIFT_7,
> +	SLOPE_7,
> +	CZERO_7,
> +	SHIFT_8,
> +	SLOPE_8,
> +	CZERO_8,
> +	SHIFT_9,
> +	SLOPE_9,
> +	CZERO_9,
> +	SHIFT_10,
> +	SLOPE_10,
> +	CZERO_10,
> +	SHIFT_11,
> +	SLOPE_11,
> +	CZERO_11,
> +	SHIFT_12,
> +	SLOPE_12,
> +	CZERO_12,
> +	SHIFT_13,
> +	SLOPE_13,
> +	CZERO_13,
> +	SHIFT_14,
> +	SLOPE_14,
> +	CZERO_14,
> +	SHIFT_15,
> +	SLOPE_15,
> +	CZERO_15,
>   
>   	/* ----- TM ------ */
>   	/* TRDY */
> @@ -524,6 +576,7 @@ struct tsens_features {
>   /**
>    * struct tsens_plat_data - tsens compile-time platform data
>    * @num_sensors: Number of sensors supported by platform
> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
>    * @ops: operations the tsens instance supports
>    * @hw_ids: Subset of sensors ids supported by platform, if not the first n
>    * @feat: features of the IP
> @@ -531,6 +584,7 @@ struct tsens_features {
>    */
>   struct tsens_plat_data {
>   	const u32		num_sensors;
> +	const u16		sensors_to_en;

There is already a similar field, hw_ids. Can it be used instead?

>   	const struct tsens_ops	*ops;
>   	unsigned int		*hw_ids;
>   	struct tsens_features	*feat;
> @@ -551,6 +605,7 @@ struct tsens_context {
>    * struct tsens_priv - private data for each instance of the tsens IP
>    * @dev: pointer to struct device
>    * @num_sensors: number of sensors enabled on this device
> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
>    * @tm_map: pointer to TM register address space
>    * @srot_map: pointer to SROT register address space
>    * @tm_offset: deal with old device trees that don't address TM and SROT
> @@ -569,6 +624,7 @@ struct tsens_context {
>   struct tsens_priv {
>   	struct device			*dev;
>   	u32				num_sensors;
> +	u16				sensors_to_en;
>   	struct regmap			*tm_map;
>   	struct regmap			*srot_map;
>   	u32				tm_offset;

-- 
With best wishes
Dmitry

Re: [PATCH 4/6] arm64: dts: qcom: ipq5332: Add tsens node

[Dmitry Baryshkov]

On 10/07/2023 13:37, Praveenkumar I wrote:
> IPQ5332 has tsens v2.3.3 peripheral. This patch adds the tsense
> node with nvmem cells for calibration data.
> 
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/ipq5332.dtsi | 113 ++++++++++++++++++++++++++
>   1 file changed, 113 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index 8bfc2db44624..a1e3527178c0 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -150,6 +150,91 @@ qfprom: efuse@a4000 {
>   			reg = <0x000a4000 0x721>;
>   			#address-cells = <1>;
>   			#size-cells = <1>;
> +
> +			tsens_mode: mode@3e1 {
> +				reg = <0x3e1 0x1>;
> +				bits = <0 3>;
> +			};
> +
> +			tsens_base0: base0@3e1 {
> +				reg = <0x3e1 0x2>;
> +				bits = <3 10>;
> +			};
> +
> +			tsens_base1: base1@3e2 {
> +				reg = <0x3e2 0x2>;
> +				bits = <5 10>;
> +			};
> +
> +			s0_offset: s0_offset@3e4 {
> +				reg = <0x3e4 0x1>;
> +				bits = <0 4>;
> +			};
> +
> +			s3_offset: s3_offset@3e5 {
> +				reg = <0x3e5 0x1>;
> +				bits = <4 4>;
> +			};
> +
> +			s4_offset: s4_offset@3e6 {
> +				reg = <0x3e6 0x1>;
> +				bits = <0 4>;
> +			};
> +
> +			s5_offset: s5_offset@3e6 {
> +				reg = <0x3e6 0x1>;
> +				bits = <4 4>;
> +			};
> +
> +			s6_offset: s6_offset@3e8 {
> +				reg = <0x3e8 0x1>;
> +				bits = <0 4>;
> +			};
> +
> +			s7_offset: s7_offset@3e8 {
> +				reg = <0x3e8 0x1>;
> +				bits = <4 4>;
> +			};
> +
> +			s8_offset: s8_offset@3a4 {
> +				reg = <0x3a4 0x1>;
> +				bits = <0 4>;
> +			};
> +
> +			s9_offset: s9_offset@3a4 {
> +				reg = <0x3a4 0x1>;
> +				bits = <4 4>;
> +			};
> +
> +			s10_offset: s10_offset@3a5 {
> +				reg = <0x3a5 0x1>;
> +				bits = <0 4>;
> +			};
> +
> +			s11_offset: s11_offset@3a5 {
> +				reg = <0x3a5 0x1>;
> +				bits = <4 4>;
> +			};
> +
> +			s12_offset: s12_offset@3a6 {
> +				reg = <0x3a6 0x1>;
> +				bits = <0 4>;
> +			};
> +
> +			s13_offset: s13_offset@3a6 {
> +				reg = <0x3a6 0x1>;
> +				bits = <4 4>;
> +			};
> +
> +			s14_offset: s14_offset@3ad {
> +				reg = <0x3ad 0x2>;
> +				bits = <7 4>;
> +			};
> +
> +			s15_offset: s0_offset@3ae {
> +				reg = <0x3ae 0x1>;
> +				bits = <3 4>;
> +			};
>   		};
>   
>   		rng: rng@e3000 {
> @@ -159,6 +244,34 @@ rng: rng@e3000 {
>   			clock-names = "core";
>   		};
>   
> +		tsens: thermal-sensor@4a9000 {
> +			compatible = "qcom,ipq5332-tsens";
> +			reg = <0x4a9000 0x1000>,
> +			      <0x4a8000 0x1000>;
> +			nvmem-cells = <&tsens_mode>, <&tsens_base0>,
> +					<&tsens_base1>, <&s0_offset>,

Please align vertically.

> +					<&s3_offset>, <&s4_offset>,
> +					<&s5_offset>, <&s6_offset>,
> +					<&s7_offset>, <&s8_offset>,
> +					<&s9_offset>, <&s10_offset>,
> +					<&s11_offset>, <&s12_offset>,
> +					<&s13_offset>, <&s14_offset>,
> +					<&s15_offset>;
> +			nvmem-cell-names = "mode", "base0",
> +						"base1", "s0_offset",

And here.

> +						"s3_offset", "s4_offset",
> +						"s5_offset", "s6_offset",
> +						"s7_offset", "s8_offset",
> +						"s9_offset", "s10_offset",
> +						"s11_offset", "s12_offset",
> +						"s13_offset", "s14_offset",
> +						"s15_offset";
> +			interrupts = <GIC_SPI 320 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "combined";
> +			#qcom,sensors = <16>;
> +			#thermal-sensor-cells = <1>;
> +		};
> +
>   		tlmm: pinctrl@1000000 {
>   			compatible = "qcom,ipq5332-tlmm";
>   			reg = <0x01000000 0x300000>;

-- 
With best wishes
Dmitry

Re: [PATCH 2/6] thermal/drivers/tsens: Add TSENS enable and calibration support for V2

[Dmitry Baryshkov]

On 10/07/2023 14:19, Dmitry Baryshkov wrote:
> On 10/07/2023 13:37, Praveenkumar I wrote:
>> SoCs without RPM have to enable sensors and calibrate from the kernel.
>> Though TSENS IP supports 16 sensors, not all are used. So added
>> sensors_to_en in tsens data help enable the relevant sensors.
>>
>> Added new calibration function for V2 as the tsens.c calib function
>> only supports V1.
>>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> ---
>>   drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
>>   drivers/thermal/qcom/tsens.c    |  37 +++++++++-
>>   drivers/thermal/qcom/tsens.h    |  56 +++++++++++++++
>>   3 files changed, 208 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-v2.c 
>> b/drivers/thermal/qcom/tsens-v2.c
>> index 29a61d2d6ca3..db48b1d95348 100644
>> --- a/drivers/thermal/qcom/tsens-v2.c
>> +++ b/drivers/thermal/qcom/tsens-v2.c
>> @@ -6,11 +6,20 @@
>>   #include <linux/bitops.h>
>>   #include <linux/regmap.h>
>> +#include <linux/nvmem-consumer.h>
>>   #include "tsens.h"
>>   /* ----- SROT ------ */
>>   #define SROT_HW_VER_OFF    0x0000
>>   #define SROT_CTRL_OFF        0x0004
>> +#define SROT_MEASURE_PERIOD    0x0008
>> +#define SROT_Sn_CONVERSION    0x0060
>> +#define V2_SHIFT_DEFAULT    0x0003
>> +#define V2_SLOPE_DEFAULT    0x0cd0
>> +#define V2_CZERO_DEFAULT    0x016a
>> +#define ONE_PT_SLOPE        0x0cd0
>> +#define TWO_PT_SHIFTED_GAIN    921600
>> +#define ONE_PT_CZERO_CONST    94
>>   /* ----- TM ------ */
>>   #define TM_INT_EN_OFF            0x0004
>> @@ -59,6 +68,16 @@ static const struct reg_field 
>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>       /* CTRL_OFF */
>>       [TSENS_EN]     = REG_FIELD(SROT_CTRL_OFF,    0,  0),
>>       [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF,    1,  1),
>> +    [SENSOR_EN]    = REG_FIELD(SROT_CTRL_OFF,    3,  18),
>> +    [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF,    21, 21),
>> +
>> +    /* MAIN_MEASURE_PERIOD */
>> +    [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
>> +
>> +    /* Sn Conversion */
>> +    REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
>> +    REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
>> +    REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
>>       /* ----- TM ------ */
>>       /* INTERRUPT ENABLE */
>> @@ -104,6 +123,103 @@ static const struct reg_field 
>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>       [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>>   };
>> +static int tsens_v2_calibration(struct tsens_priv *priv)
>> +{
>> +    struct device *dev = priv->dev;
>> +    u32 mode, base0, base1;
>> +    u32 slope, czero;
>> +    char name[15];
>> +    int i, j, ret;
>> +
>> +    if (priv->num_sensors > MAX_SENSORS)
>> +        return -EINVAL;
>> +
>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
>> +    if (ret == -ENOENT)
>> +        dev_warn(priv->dev, "Calibration data not present in DT\n");
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    dev_dbg(priv->dev, "calibration mode is %d\n", mode);
>> +
>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap 
>> for enabled sensors */
>> +    for (i = 0; i < priv->num_sensors; i++) {
>> +        if (!(priv->sensors_to_en & (0x1 << i)))
>> +            continue;
>> +
>> +        ret = snprintf(name, sizeof(name), "s%d_offset", 
>> priv->sensor[i].hw_id);
>> +        if (ret < 0)
>> +            return ret;
>> +
>> +        ret = nvmem_cell_read_variable_le_u32(priv->dev, name, 
>> &priv->sensor[i].offset);
>> +        if (ret)
>> +            return ret;
>> +
>> +        for (j = SHIFT_0; j <= CZERO_0; j++) {
>> +            int idx = (i * 3) + j;
>> +
>> +            priv->rf[idx] = devm_regmap_field_alloc(dev, priv->srot_map,
>> +                                priv->fields[idx]);
>> +            if (IS_ERR(priv->rf[idx]))
>> +                return PTR_ERR(priv->rf[idx]);
> 
> I think, allocating data structures for 48 regfields, which are written 
> just once, to be an overkill.
> 
>> +        }
>> +    }
>> +
>> +    /* Based on calib mode, program SHIFT, SLOPE and CZERO for 
>> enabled sensors */
>> +    switch (mode) {
>> +    case TWO_PT_CALIB:
>> +        slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
>> +
>> +        for (i = 0; i < priv->num_sensors; i++) {
>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>> +                continue;
>> +
>> +            int idx = i * 3;
>> +
>> +            czero = (base0 + priv->sensor[i].offset - ((base1 - 
>> base0) / 3));
>> +            regmap_field_write(priv->rf[SHIFT_0 + idx], 
>> V2_SHIFT_DEFAULT);
>> +            regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
>> +            regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>> +        }
>> +        fallthrough;
>> +    case ONE_PT_CALIB2:
>> +        for (i = 0; i < priv->num_sensors; i++) {
>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>> +                continue;
>> +
>> +            int idx = i * 3;
>> +
>> +            czero = base0 + priv->sensor[i].offset - ONE_PT_CZERO_CONST;
>> +            regmap_field_write(priv->rf[SHIFT_0 + idx], 
>> V2_SHIFT_DEFAULT);
>> +            regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
>> +            regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>> +        }
>> +        break;
>> +    default:
>> +        dev_dbg(priv->dev, "calibrationless mode\n");
>> +        for (i = 0; i < priv->num_sensors; i++) {
>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>> +                continue;
>> +
>> +            int idx = i * 3;
>> +
>> +            regmap_field_write(priv->rf[SHIFT_0 + idx], 
>> V2_SHIFT_DEFAULT);
>> +            regmap_field_write(priv->rf[SLOPE_0 + idx], 
>> V2_SLOPE_DEFAULT);
>> +            regmap_field_write(priv->rf[CZERO_0 + idx], 
>> V2_CZERO_DEFAULT);
>> +        }
>> +    }
> 
> This code iterates over the sensors field several times. Please consider 
> extracting a function that handles all setup for a single sensor, then 
> calling it in a loop (I should probably do the same for tsens-v0/v1 too).
> 
>> +
>> +    return 0;
>> +}
>> +
>>   static const struct tsens_ops ops_generic_v2 = {
>>       .init        = init_common,
>>       .get_temp    = get_temp_tsens_valid,
>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>> index 98c356acfe98..169690355dad 100644
>> --- a/drivers/thermal/qcom/tsens.c
>> +++ b/drivers/thermal/qcom/tsens.c
>> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
>>       ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
>>       if (ret)
>>           goto err_put_device;
>> -    if (!enabled) {
>> +    if (!enabled && !priv->sensors_to_en) {
>>           dev_err(dev, "%s: device not enabled\n", __func__);
>>           ret = -ENODEV;
>>           goto err_put_device;
>> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
>>           goto err_put_device;
>>       }
>> +    /* Do TSENS initialization if required */
>> +    if (priv->sensors_to_en) {
> 
> Maybe it would be better to explicitly add VER_2_X_NO_RPM and check it 
> here?

After taking a look at the patch 6, can we just add a separate init() 
function which will call init_common() and then initialize these fields?

> 
>> +        priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev, 
>> priv->srot_map,
>> +                                 priv->fields[CODE_OR_TEMP]);
>> +        if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
>> +            ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
>> +            goto err_put_device;
>> +        }
>> +
>> +        priv->rf[MAIN_MEASURE_PERIOD] =
>> +            devm_regmap_field_alloc(dev, priv->srot_map,
>> +                        priv->fields[MAIN_MEASURE_PERIOD]);
>> +        if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
>> +            ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
>> +            goto err_put_device;
>> +        }
>> +
>> +        regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
>> +
>> +        /* Update measure period to 2ms */
>> +        regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
>> +
>> +        /* Enable available sensors */
>> +        regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
>> +
>> +        /* Real temperature format */
>> +        regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
>> +
>> +        regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
>> +
>> +        /* Enable TSENS */
>> +        regmap_field_write(priv->rf[TSENS_EN], 0x1);
>> +    }
>> +
>>       /* This loop might need changes if enum regfield_ids is 
>> reordered */
>>       for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
>>           for (i = 0; i < priv->feat->max_sensors; i++) {
>> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device 
>> *pdev)
>>       priv->dev = dev;
>>       priv->num_sensors = num_sensors;
>> +    priv->sensors_to_en = data->sensors_to_en;
>>       priv->ops = data->ops;
>>       for (i = 0;  i < priv->num_sensors; i++) {
>>           if (data->hw_ids)
>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>> index 2805de1c6827..f8897bc8944e 100644
>> --- a/drivers/thermal/qcom/tsens.h
>> +++ b/drivers/thermal/qcom/tsens.h
>> @@ -168,6 +168,58 @@ enum regfield_ids {
>>       TSENS_SW_RST,
>>       SENSOR_EN,
>>       CODE_OR_TEMP,
>> +    /* MEASURE_PERIOD */
>> +    MAIN_MEASURE_PERIOD,
>> +
>> +    /* Sn_CONVERSION */
>> +    SHIFT_0,
>> +    SLOPE_0,
>> +    CZERO_0,
>> +    SHIFT_1,
>> +    SLOPE_1,
>> +    CZERO_1,
>> +    SHIFT_2,
>> +    SLOPE_2,
>> +    CZERO_2,
>> +    SHIFT_3,
>> +    SLOPE_3,
>> +    CZERO_3,
>> +    SHIFT_4,
>> +    SLOPE_4,
>> +    CZERO_4,
>> +    SHIFT_5,
>> +    SLOPE_5,
>> +    CZERO_5,
>> +    SHIFT_6,
>> +    SLOPE_6,
>> +    CZERO_6,
>> +    SHIFT_7,
>> +    SLOPE_7,
>> +    CZERO_7,
>> +    SHIFT_8,
>> +    SLOPE_8,
>> +    CZERO_8,
>> +    SHIFT_9,
>> +    SLOPE_9,
>> +    CZERO_9,
>> +    SHIFT_10,
>> +    SLOPE_10,
>> +    CZERO_10,
>> +    SHIFT_11,
>> +    SLOPE_11,
>> +    CZERO_11,
>> +    SHIFT_12,
>> +    SLOPE_12,
>> +    CZERO_12,
>> +    SHIFT_13,
>> +    SLOPE_13,
>> +    CZERO_13,
>> +    SHIFT_14,
>> +    SLOPE_14,
>> +    CZERO_14,
>> +    SHIFT_15,
>> +    SLOPE_15,
>> +    CZERO_15,
>>       /* ----- TM ------ */
>>       /* TRDY */
>> @@ -524,6 +576,7 @@ struct tsens_features {
>>   /**
>>    * struct tsens_plat_data - tsens compile-time platform data
>>    * @num_sensors: Number of sensors supported by platform
>> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
>>    * @ops: operations the tsens instance supports
>>    * @hw_ids: Subset of sensors ids supported by platform, if not the 
>> first n
>>    * @feat: features of the IP
>> @@ -531,6 +584,7 @@ struct tsens_features {
>>    */
>>   struct tsens_plat_data {
>>       const u32        num_sensors;
>> +    const u16        sensors_to_en;
> 
> There is already a similar field, hw_ids. Can it be used instead?
> 
>>       const struct tsens_ops    *ops;
>>       unsigned int        *hw_ids;
>>       struct tsens_features    *feat;
>> @@ -551,6 +605,7 @@ struct tsens_context {
>>    * struct tsens_priv - private data for each instance of the tsens IP
>>    * @dev: pointer to struct device
>>    * @num_sensors: number of sensors enabled on this device
>> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
>>    * @tm_map: pointer to TM register address space
>>    * @srot_map: pointer to SROT register address space
>>    * @tm_offset: deal with old device trees that don't address TM and 
>> SROT
>> @@ -569,6 +624,7 @@ struct tsens_context {
>>   struct tsens_priv {
>>       struct device            *dev;
>>       u32                num_sensors;
>> +    u16                sensors_to_en;
>>       struct regmap            *tm_map;
>>       struct regmap            *srot_map;
>>       u32                tm_offset;
> 

-- 
With best wishes
Dmitry

Re: [PATCH 5/6] arm64: dts: qcom: ipq5332: Add thermal zone nodes

[Dmitry Baryshkov]

On 10/07/2023 13:37, Praveenkumar I wrote:
> This patch adds thermal zone nodes for sensors present in
> IPQ5332.
> 
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> ---
>   arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72 +++++++++++++++++++++++++++
>   1 file changed, 72 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index a1e3527178c0..8b276aeca53e 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -527,4 +527,76 @@ timer {
>   			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>   			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>   	};
> +
> +	thermal-zones {
> +		rfa-0-thermal{
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsens 11>;
> +
> +			trips {
> +				rfa-0-critical {
> +					temperature = <125000>;
> +					hysteresis = <1000>;
> +					type = "critical";
> +				};
> +			};
> +		};
> +
> +		rfa-1-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsens 12>;
> +
> +			trips {
> +				rfa-1-critical {
> +					temperature = <125000>;
> +					hysteresis = <1000>;
> +					type = "critical";
> +				};
> +			};
> +		};
> +
> +		misc-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsens 13>;
> +
> +			trips {
> +				misc-critical {
> +					temperature = <125000>;
> +					hysteresis = <1000>;
> +					type = "critical";
> +				};
> +			};
> +		};
> +
> +		cpu-top-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsens 14>;
> +
> +			trips {
> +				cpu-top-critical {
> +					temperature = <125000>;
> +					hysteresis = <1000>;
> +					type = "critical";
> +				};
> +			};

Could you please add a passive cooling devices for the CPU?

> +		};
> +
> +		top-glue-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsens 15>;
> +
> +			trips {
> +				top-glue-critical {
> +					temperature = <125000>;
> +					hysteresis = <1000>;
> +					type = "critical";
> +				};
> +			};
> +		};
> +	};
>   };

-- 
With best wishes
Dmitry

Re: [PATCH 6/6] thermal/drivers/tsens: Add IPQ5332 support

[Dmitry Baryshkov]

On 10/07/2023 13:37, Praveenkumar I wrote:
> IPQ5332 uses tsens v2.3.3 IP and it is having combined interrupt as
> like IPQ8074. But as the SoCs does not have RPM, kernel needs to
> take care of sensor enablement and calibration. Hence introduced
> new ops and data for IPQ5332 and reused the feature_config from
> IPQ8074.
> 
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> ---
>   drivers/thermal/qcom/tsens-v2.c | 13 +++++++++++++
>   drivers/thermal/qcom/tsens.c    |  3 +++
>   drivers/thermal/qcom/tsens.h    |  2 +-
>   3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/qcom/tsens-v2.c b/drivers/thermal/qcom/tsens-v2.c
> index db48b1d95348..8b6e3876fd2c 100644
> --- a/drivers/thermal/qcom/tsens-v2.c
> +++ b/drivers/thermal/qcom/tsens-v2.c
> @@ -237,6 +237,19 @@ struct tsens_plat_data data_ipq8074 = {
>   	.fields	= tsens_v2_regfields,
>   };
>   
> +static const struct tsens_ops ops_ipq5332_v2 = {

Please drop v2. It is unclear if it refers to tsens being v2 or being 
specific to ipq5332 v2.

> +	.init		= init_common,
> +	.get_temp	= get_temp_tsens_valid,
> +	.calibrate	= tsens_v2_calibration,
> +};
> +
> +struct tsens_plat_data data_ipq5332 = {
> +	.sensors_to_en	= 0xF800,

This doesn't seem to match the offsets that you have enabled in the DTSI.

> +	.ops		= &ops_ipq5332_v2,
> +	.feat		= &ipq8074_feat,
> +	.fields		= tsens_v2_regfields,
> +};
> +
>   /* Kept around for backward compatibility with old msm8996.dtsi */
>   struct tsens_plat_data data_8996 = {
>   	.num_sensors	= 13,
> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> index 169690355dad..e8ba2901cda8 100644
> --- a/drivers/thermal/qcom/tsens.c
> +++ b/drivers/thermal/qcom/tsens.c
> @@ -1140,6 +1140,9 @@ static const struct of_device_id tsens_table[] = {
>   	}, {
>   		.compatible = "qcom,ipq8074-tsens",
>   		.data = &data_ipq8074,
> +	}, {
> +		.compatible = "qcom,ipq5332-tsens",
> +		.data = &data_ipq5332,
>   	}, {
>   		.compatible = "qcom,mdm9607-tsens",
>   		.data = &data_9607,
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index f8897bc8944e..36040f9beebc 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -701,6 +701,6 @@ extern struct tsens_plat_data data_8226, data_8909, data_8916, data_8939, data_8
>   extern struct tsens_plat_data data_tsens_v1, data_8976, data_8956;
>   
>   /* TSENS v2 targets */
> -extern struct tsens_plat_data data_8996, data_ipq8074, data_tsens_v2;
> +extern struct tsens_plat_data data_8996, data_ipq8074, data_ipq5332, data_tsens_v2;
>   
>   #endif /* __QCOM_TSENS_H__ */

-- 
With best wishes
Dmitry

Re: [PATCH 5/6] arm64: dts: qcom: ipq5332: Add thermal zone nodes

[Konrad Dybcio]

On 10.07.2023 13:23, Dmitry Baryshkov wrote:
> On 10/07/2023 13:37, Praveenkumar I wrote:
>> This patch adds thermal zone nodes for sensors present in
>> IPQ5332.
>>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72 +++++++++++++++++++++++++++
>>   1 file changed, 72 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> index a1e3527178c0..8b276aeca53e 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> @@ -527,4 +527,76 @@ timer {
>>                    <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>>                    <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>>       };
>> +
>> +    thermal-zones {
>> +        rfa-0-thermal{
thermal {


>> +            polling-delay-passive = <0>;
>> +            polling-delay = <0>;
>> +            thermal-sensors = <&tsens 11>;
>> +
>> +            trips {
Indentation seems off, tab size for kernel code is 8 spaces.

Konrad
>> +                rfa-0-critical {
>> +                    temperature = <125000>;
>> +                    hysteresis = <1000>;
>> +                    type = "critical";
>> +                };
>> +            };
>> +        };
>> +
>> +        rfa-1-thermal {
>> +            polling-delay-passive = <0>;
>> +            polling-delay = <0>;
>> +            thermal-sensors = <&tsens 12>;
>> +
>> +            trips {
>> +                rfa-1-critical {
>> +                    temperature = <125000>;
>> +                    hysteresis = <1000>;
>> +                    type = "critical";
>> +                };
>> +            };
>> +        };
>> +
>> +        misc-thermal {
>> +            polling-delay-passive = <0>;
>> +            polling-delay = <0>;
>> +            thermal-sensors = <&tsens 13>;
>> +
>> +            trips {
>> +                misc-critical {
>> +                    temperature = <125000>;
>> +                    hysteresis = <1000>;
>> +                    type = "critical";
>> +                };
>> +            };
>> +        };
>> +
>> +        cpu-top-thermal {
>> +            polling-delay-passive = <0>;
>> +            polling-delay = <0>;
>> +            thermal-sensors = <&tsens 14>;
>> +
>> +            trips {
>> +                cpu-top-critical {
>> +                    temperature = <125000>;
>> +                    hysteresis = <1000>;
>> +                    type = "critical";
>> +                };
>> +            };
> 
> Could you please add a passive cooling devices for the CPU?
> 
>> +        };
>> +
>> +        top-glue-thermal {
>> +            polling-delay-passive = <0>;
>> +            polling-delay = <0>;
>> +            thermal-sensors = <&tsens 15>;
>> +
>> +            trips {
>> +                top-glue-critical {
>> +                    temperature = <125000>;
>> +                    hysteresis = <1000>;
>> +                    type = "critical";
>> +                };
>> +            };
>> +        };
>> +    };
>>   };
> 

Re: [PATCH 2/6] thermal/drivers/tsens: Add TSENS enable and calibration support for V2

[Praveenkumar I]

On 7/10/2023 4:49 PM, Dmitry Baryshkov wrote:
> On 10/07/2023 13:37, Praveenkumar I wrote:
>> SoCs without RPM have to enable sensors and calibrate from the kernel.
>> Though TSENS IP supports 16 sensors, not all are used. So added
>> sensors_to_en in tsens data help enable the relevant sensors.
>>
>> Added new calibration function for V2 as the tsens.c calib function
>> only supports V1.
>>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> ---
>>   drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
>>   drivers/thermal/qcom/tsens.c    |  37 +++++++++-
>>   drivers/thermal/qcom/tsens.h    |  56 +++++++++++++++
>>   3 files changed, 208 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-v2.c 
>> b/drivers/thermal/qcom/tsens-v2.c
>> index 29a61d2d6ca3..db48b1d95348 100644
>> --- a/drivers/thermal/qcom/tsens-v2.c
>> +++ b/drivers/thermal/qcom/tsens-v2.c
>> @@ -6,11 +6,20 @@
>>     #include <linux/bitops.h>
>>   #include <linux/regmap.h>
>> +#include <linux/nvmem-consumer.h>
>>   #include "tsens.h"
>>     /* ----- SROT ------ */
>>   #define SROT_HW_VER_OFF    0x0000
>>   #define SROT_CTRL_OFF        0x0004
>> +#define SROT_MEASURE_PERIOD    0x0008
>> +#define SROT_Sn_CONVERSION    0x0060
>> +#define V2_SHIFT_DEFAULT    0x0003
>> +#define V2_SLOPE_DEFAULT    0x0cd0
>> +#define V2_CZERO_DEFAULT    0x016a
>> +#define ONE_PT_SLOPE        0x0cd0
>> +#define TWO_PT_SHIFTED_GAIN    921600
>> +#define ONE_PT_CZERO_CONST    94
>>     /* ----- TM ------ */
>>   #define TM_INT_EN_OFF            0x0004
>> @@ -59,6 +68,16 @@ static const struct reg_field 
>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>       /* CTRL_OFF */
>>       [TSENS_EN]     = REG_FIELD(SROT_CTRL_OFF,    0,  0),
>>       [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF,    1,  1),
>> +    [SENSOR_EN]    = REG_FIELD(SROT_CTRL_OFF,    3,  18),
>> +    [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF,    21, 21),
>> +
>> +    /* MAIN_MEASURE_PERIOD */
>> +    [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
>> +
>> +    /* Sn Conversion */
>> +    REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
>> +    REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
>> +    REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
>>         /* ----- TM ------ */
>>       /* INTERRUPT ENABLE */
>> @@ -104,6 +123,103 @@ static const struct reg_field 
>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>       [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>>   };
>>   +static int tsens_v2_calibration(struct tsens_priv *priv)
>> +{
>> +    struct device *dev = priv->dev;
>> +    u32 mode, base0, base1;
>> +    u32 slope, czero;
>> +    char name[15];
>> +    int i, j, ret;
>> +
>> +    if (priv->num_sensors > MAX_SENSORS)
>> +        return -EINVAL;
>> +
>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
>> +    if (ret == -ENOENT)
>> +        dev_warn(priv->dev, "Calibration data not present in DT\n");
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    dev_dbg(priv->dev, "calibration mode is %d\n", mode);
>> +
>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap 
>> for enabled sensors */
>> +    for (i = 0; i < priv->num_sensors; i++) {
>> +        if (!(priv->sensors_to_en & (0x1 << i)))
>> +            continue;
>> +
>> +        ret = snprintf(name, sizeof(name), "s%d_offset", 
>> priv->sensor[i].hw_id);
>> +        if (ret < 0)
>> +            return ret;
>> +
>> +        ret = nvmem_cell_read_variable_le_u32(priv->dev, name, 
>> &priv->sensor[i].offset);
>> +        if (ret)
>> +            return ret;
>> +
>> +        for (j = SHIFT_0; j <= CZERO_0; j++) {
>> +            int idx = (i * 3) + j;
>> +
>> +            priv->rf[idx] = devm_regmap_field_alloc(dev, 
>> priv->srot_map,
>> +                                priv->fields[idx]);
>> +            if (IS_ERR(priv->rf[idx]))
>> +                return PTR_ERR(priv->rf[idx]);
>
> I think, allocating data structures for 48 regfields, which are 
> written just once, to be an overkill.
Can we change it to single field for each sensor. For example, 
CONVERSION_0 instead of SHIFT_0, SLOPE_0 and CZERO_0? This way it will 
be max 16 regfields.
>
>> +        }
>> +    }
>> +
>> +    /* Based on calib mode, program SHIFT, SLOPE and CZERO for 
>> enabled sensors */
>> +    switch (mode) {
>> +    case TWO_PT_CALIB:
>> +        slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
>> +
>> +        for (i = 0; i < priv->num_sensors; i++) {
>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>> +                continue;
>> +
>> +            int idx = i * 3;
>> +
>> +            czero = (base0 + priv->sensor[i].offset - ((base1 - 
>> base0) / 3));
>> +            regmap_field_write(priv->rf[SHIFT_0 + idx], 
>> V2_SHIFT_DEFAULT);
>> +            regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
>> +            regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>> +        }
>> +        fallthrough;
>> +    case ONE_PT_CALIB2:
>> +        for (i = 0; i < priv->num_sensors; i++) {
>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>> +                continue;
>> +
>> +            int idx = i * 3;
>> +
>> +            czero = base0 + priv->sensor[i].offset - 
>> ONE_PT_CZERO_CONST;
>> +            regmap_field_write(priv->rf[SHIFT_0 + idx], 
>> V2_SHIFT_DEFAULT);
>> +            regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
>> +            regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>> +        }
>> +        break;
>> +    default:
>> +        dev_dbg(priv->dev, "calibrationless mode\n");
>> +        for (i = 0; i < priv->num_sensors; i++) {
>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>> +                continue;
>> +
>> +            int idx = i * 3;
>> +
>> +            regmap_field_write(priv->rf[SHIFT_0 + idx], 
>> V2_SHIFT_DEFAULT);
>> +            regmap_field_write(priv->rf[SLOPE_0 + idx], 
>> V2_SLOPE_DEFAULT);
>> +            regmap_field_write(priv->rf[CZERO_0 + idx], 
>> V2_CZERO_DEFAULT);
>> +        }
>> +    }
>
> This code iterates over the sensors field several times. Please 
> consider extracting a function that handles all setup for a single 
> sensor, then calling it in a loop (I should probably do the same for 
> tsens-v0/v1 too).
Sure. After reading the mode0, base0 and base1 from QFPROM, we can call 
a function in a loop to setup the calibration for each sensor.
>
>> +
>> +    return 0;
>> +}
>> +
>>   static const struct tsens_ops ops_generic_v2 = {
>>       .init        = init_common,
>>       .get_temp    = get_temp_tsens_valid,
>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>> index 98c356acfe98..169690355dad 100644
>> --- a/drivers/thermal/qcom/tsens.c
>> +++ b/drivers/thermal/qcom/tsens.c
>> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
>>       ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
>>       if (ret)
>>           goto err_put_device;
>> -    if (!enabled) {
>> +    if (!enabled && !priv->sensors_to_en) {
>>           dev_err(dev, "%s: device not enabled\n", __func__);
>>           ret = -ENODEV;
>>           goto err_put_device;
>> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
>>           goto err_put_device;
>>       }
>>   +    /* Do TSENS initialization if required */
>> +    if (priv->sensors_to_en) {
>
> Maybe it would be better to explicitly add VER_2_X_NO_RPM and check it 
> here?
Sure, will add a separate version macro.
>
>> +        priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev, 
>> priv->srot_map,
>> + priv->fields[CODE_OR_TEMP]);
>> +        if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
>> +            ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
>> +            goto err_put_device;
>> +        }
>> +
>> +        priv->rf[MAIN_MEASURE_PERIOD] =
>> +            devm_regmap_field_alloc(dev, priv->srot_map,
>> +                        priv->fields[MAIN_MEASURE_PERIOD]);
>> +        if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
>> +            ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
>> +            goto err_put_device;
>> +        }
>> +
>> +        regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
>> +
>> +        /* Update measure period to 2ms */
>> +        regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
>> +
>> +        /* Enable available sensors */
>> +        regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
>> +
>> +        /* Real temperature format */
>> +        regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
>> +
>> +        regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
>> +
>> +        /* Enable TSENS */
>> +        regmap_field_write(priv->rf[TSENS_EN], 0x1);
>> +    }
>> +
>>       /* This loop might need changes if enum regfield_ids is 
>> reordered */
>>       for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
>>           for (i = 0; i < priv->feat->max_sensors; i++) {
>> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device 
>> *pdev)
>>         priv->dev = dev;
>>       priv->num_sensors = num_sensors;
>> +    priv->sensors_to_en = data->sensors_to_en;
>>       priv->ops = data->ops;
>>       for (i = 0;  i < priv->num_sensors; i++) {
>>           if (data->hw_ids)
>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>> index 2805de1c6827..f8897bc8944e 100644
>> --- a/drivers/thermal/qcom/tsens.h
>> +++ b/drivers/thermal/qcom/tsens.h
>> @@ -168,6 +168,58 @@ enum regfield_ids {
>>       TSENS_SW_RST,
>>       SENSOR_EN,
>>       CODE_OR_TEMP,
>> +    /* MEASURE_PERIOD */
>> +    MAIN_MEASURE_PERIOD,
>> +
>> +    /* Sn_CONVERSION */
>> +    SHIFT_0,
>> +    SLOPE_0,
>> +    CZERO_0,
>> +    SHIFT_1,
>> +    SLOPE_1,
>> +    CZERO_1,
>> +    SHIFT_2,
>> +    SLOPE_2,
>> +    CZERO_2,
>> +    SHIFT_3,
>> +    SLOPE_3,
>> +    CZERO_3,
>> +    SHIFT_4,
>> +    SLOPE_4,
>> +    CZERO_4,
>> +    SHIFT_5,
>> +    SLOPE_5,
>> +    CZERO_5,
>> +    SHIFT_6,
>> +    SLOPE_6,
>> +    CZERO_6,
>> +    SHIFT_7,
>> +    SLOPE_7,
>> +    CZERO_7,
>> +    SHIFT_8,
>> +    SLOPE_8,
>> +    CZERO_8,
>> +    SHIFT_9,
>> +    SLOPE_9,
>> +    CZERO_9,
>> +    SHIFT_10,
>> +    SLOPE_10,
>> +    CZERO_10,
>> +    SHIFT_11,
>> +    SLOPE_11,
>> +    CZERO_11,
>> +    SHIFT_12,
>> +    SLOPE_12,
>> +    CZERO_12,
>> +    SHIFT_13,
>> +    SLOPE_13,
>> +    CZERO_13,
>> +    SHIFT_14,
>> +    SLOPE_14,
>> +    CZERO_14,
>> +    SHIFT_15,
>> +    SLOPE_15,
>> +    CZERO_15,
>>         /* ----- TM ------ */
>>       /* TRDY */
>> @@ -524,6 +576,7 @@ struct tsens_features {
>>   /**
>>    * struct tsens_plat_data - tsens compile-time platform data
>>    * @num_sensors: Number of sensors supported by platform
>> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
>>    * @ops: operations the tsens instance supports
>>    * @hw_ids: Subset of sensors ids supported by platform, if not the 
>> first n
>>    * @feat: features of the IP
>> @@ -531,6 +584,7 @@ struct tsens_features {
>>    */
>>   struct tsens_plat_data {
>>       const u32        num_sensors;
>> +    const u16        sensors_to_en;
>
> There is already a similar field, hw_ids. Can it be used instead?
Yes, it can be used. I missed to check this hw_ids. Will change the 
num_sensors to 5 and use the hw_ids.
>
>>       const struct tsens_ops    *ops;
>>       unsigned int        *hw_ids;
>>       struct tsens_features    *feat;
>> @@ -551,6 +605,7 @@ struct tsens_context {
>>    * struct tsens_priv - private data for each instance of the tsens IP
>>    * @dev: pointer to struct device
>>    * @num_sensors: number of sensors enabled on this device
>> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
>>    * @tm_map: pointer to TM register address space
>>    * @srot_map: pointer to SROT register address space
>>    * @tm_offset: deal with old device trees that don't address TM and 
>> SROT
>> @@ -569,6 +624,7 @@ struct tsens_context {
>>   struct tsens_priv {
>>       struct device            *dev;
>>       u32                num_sensors;
>> +    u16                sensors_to_en;
>>       struct regmap            *tm_map;
>>       struct regmap            *srot_map;
>>       u32                tm_offset;
>
--
Thanks,
Praveenkumar

Re: [PATCH 2/6] thermal/drivers/tsens: Add TSENS enable and calibration support for V2

[Praveenkumar I]

On 7/10/2023 4:52 PM, Dmitry Baryshkov wrote:
> On 10/07/2023 14:19, Dmitry Baryshkov wrote:
>> On 10/07/2023 13:37, Praveenkumar I wrote:
>>> SoCs without RPM have to enable sensors and calibrate from the kernel.
>>> Though TSENS IP supports 16 sensors, not all are used. So added
>>> sensors_to_en in tsens data help enable the relevant sensors.
>>>
>>> Added new calibration function for V2 as the tsens.c calib function
>>> only supports V1.
>>>
>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>> ---
>>>   drivers/thermal/qcom/tsens-v2.c | 116 
>>> ++++++++++++++++++++++++++++++++
>>>   drivers/thermal/qcom/tsens.c    |  37 +++++++++-
>>>   drivers/thermal/qcom/tsens.h    |  56 +++++++++++++++
>>>   3 files changed, 208 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/thermal/qcom/tsens-v2.c 
>>> b/drivers/thermal/qcom/tsens-v2.c
>>> index 29a61d2d6ca3..db48b1d95348 100644
>>> --- a/drivers/thermal/qcom/tsens-v2.c
>>> +++ b/drivers/thermal/qcom/tsens-v2.c
>>> @@ -6,11 +6,20 @@
>>>   #include <linux/bitops.h>
>>>   #include <linux/regmap.h>
>>> +#include <linux/nvmem-consumer.h>
>>>   #include "tsens.h"
>>>   /* ----- SROT ------ */
>>>   #define SROT_HW_VER_OFF    0x0000
>>>   #define SROT_CTRL_OFF        0x0004
>>> +#define SROT_MEASURE_PERIOD    0x0008
>>> +#define SROT_Sn_CONVERSION    0x0060
>>> +#define V2_SHIFT_DEFAULT    0x0003
>>> +#define V2_SLOPE_DEFAULT    0x0cd0
>>> +#define V2_CZERO_DEFAULT    0x016a
>>> +#define ONE_PT_SLOPE        0x0cd0
>>> +#define TWO_PT_SHIFTED_GAIN    921600
>>> +#define ONE_PT_CZERO_CONST    94
>>>   /* ----- TM ------ */
>>>   #define TM_INT_EN_OFF            0x0004
>>> @@ -59,6 +68,16 @@ static const struct reg_field 
>>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>>       /* CTRL_OFF */
>>>       [TSENS_EN]     = REG_FIELD(SROT_CTRL_OFF,    0,  0),
>>>       [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF,    1,  1),
>>> +    [SENSOR_EN]    = REG_FIELD(SROT_CTRL_OFF,    3,  18),
>>> +    [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF,    21, 21),
>>> +
>>> +    /* MAIN_MEASURE_PERIOD */
>>> +    [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
>>> +
>>> +    /* Sn Conversion */
>>> +    REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
>>> +    REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
>>> +    REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
>>>       /* ----- TM ------ */
>>>       /* INTERRUPT ENABLE */
>>> @@ -104,6 +123,103 @@ static const struct reg_field 
>>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>>       [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>>>   };
>>> +static int tsens_v2_calibration(struct tsens_priv *priv)
>>> +{
>>> +    struct device *dev = priv->dev;
>>> +    u32 mode, base0, base1;
>>> +    u32 slope, czero;
>>> +    char name[15];
>>> +    int i, j, ret;
>>> +
>>> +    if (priv->num_sensors > MAX_SENSORS)
>>> +        return -EINVAL;
>>> +
>>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
>>> +    if (ret == -ENOENT)
>>> +        dev_warn(priv->dev, "Calibration data not present in DT\n");
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    dev_dbg(priv->dev, "calibration mode is %d\n", mode);
>>> +
>>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap 
>>> for enabled sensors */
>>> +    for (i = 0; i < priv->num_sensors; i++) {
>>> +        if (!(priv->sensors_to_en & (0x1 << i)))
>>> +            continue;
>>> +
>>> +        ret = snprintf(name, sizeof(name), "s%d_offset", 
>>> priv->sensor[i].hw_id);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +
>>> +        ret = nvmem_cell_read_variable_le_u32(priv->dev, name, 
>>> &priv->sensor[i].offset);
>>> +        if (ret)
>>> +            return ret;
>>> +
>>> +        for (j = SHIFT_0; j <= CZERO_0; j++) {
>>> +            int idx = (i * 3) + j;
>>> +
>>> +            priv->rf[idx] = devm_regmap_field_alloc(dev, 
>>> priv->srot_map,
>>> +                                priv->fields[idx]);
>>> +            if (IS_ERR(priv->rf[idx]))
>>> +                return PTR_ERR(priv->rf[idx]);
>>
>> I think, allocating data structures for 48 regfields, which are 
>> written just once, to be an overkill.
>>
>>> +        }
>>> +    }
>>> +
>>> +    /* Based on calib mode, program SHIFT, SLOPE and CZERO for 
>>> enabled sensors */
>>> +    switch (mode) {
>>> +    case TWO_PT_CALIB:
>>> +        slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
>>> +
>>> +        for (i = 0; i < priv->num_sensors; i++) {
>>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>>> +                continue;
>>> +
>>> +            int idx = i * 3;
>>> +
>>> +            czero = (base0 + priv->sensor[i].offset - ((base1 - 
>>> base0) / 3));
>>> +            regmap_field_write(priv->rf[SHIFT_0 + idx], 
>>> V2_SHIFT_DEFAULT);
>>> +            regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
>>> +            regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>>> +        }
>>> +        fallthrough;
>>> +    case ONE_PT_CALIB2:
>>> +        for (i = 0; i < priv->num_sensors; i++) {
>>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>>> +                continue;
>>> +
>>> +            int idx = i * 3;
>>> +
>>> +            czero = base0 + priv->sensor[i].offset - 
>>> ONE_PT_CZERO_CONST;
>>> +            regmap_field_write(priv->rf[SHIFT_0 + idx], 
>>> V2_SHIFT_DEFAULT);
>>> +            regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
>>> +            regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>>> +        }
>>> +        break;
>>> +    default:
>>> +        dev_dbg(priv->dev, "calibrationless mode\n");
>>> +        for (i = 0; i < priv->num_sensors; i++) {
>>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>>> +                continue;
>>> +
>>> +            int idx = i * 3;
>>> +
>>> +            regmap_field_write(priv->rf[SHIFT_0 + idx], 
>>> V2_SHIFT_DEFAULT);
>>> +            regmap_field_write(priv->rf[SLOPE_0 + idx], 
>>> V2_SLOPE_DEFAULT);
>>> +            regmap_field_write(priv->rf[CZERO_0 + idx], 
>>> V2_CZERO_DEFAULT);
>>> +        }
>>> +    }
>>
>> This code iterates over the sensors field several times. Please 
>> consider extracting a function that handles all setup for a single 
>> sensor, then calling it in a loop (I should probably do the same for 
>> tsens-v0/v1 too).
>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static const struct tsens_ops ops_generic_v2 = {
>>>       .init        = init_common,
>>>       .get_temp    = get_temp_tsens_valid,
>>> diff --git a/drivers/thermal/qcom/tsens.c 
>>> b/drivers/thermal/qcom/tsens.c
>>> index 98c356acfe98..169690355dad 100644
>>> --- a/drivers/thermal/qcom/tsens.c
>>> +++ b/drivers/thermal/qcom/tsens.c
>>> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
>>>       ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
>>>       if (ret)
>>>           goto err_put_device;
>>> -    if (!enabled) {
>>> +    if (!enabled && !priv->sensors_to_en) {
>>>           dev_err(dev, "%s: device not enabled\n", __func__);
>>>           ret = -ENODEV;
>>>           goto err_put_device;
>>> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
>>>           goto err_put_device;
>>>       }
>>> +    /* Do TSENS initialization if required */
>>> +    if (priv->sensors_to_en) {
>>
>> Maybe it would be better to explicitly add VER_2_X_NO_RPM and check 
>> it here?
>
> After taking a look at the patch 6, can we just add a separate init() 
> function which will call init_common() and then initialize these fields?
Sure, will add separate init() function in tsens-v2.c
>
>>
>>> +        priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev, 
>>> priv->srot_map,
>>> + priv->fields[CODE_OR_TEMP]);
>>> +        if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
>>> +            ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
>>> +            goto err_put_device;
>>> +        }
>>> +
>>> +        priv->rf[MAIN_MEASURE_PERIOD] =
>>> +            devm_regmap_field_alloc(dev, priv->srot_map,
>>> + priv->fields[MAIN_MEASURE_PERIOD]);
>>> +        if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
>>> +            ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
>>> +            goto err_put_device;
>>> +        }
>>> +
>>> +        regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
>>> +
>>> +        /* Update measure period to 2ms */
>>> +        regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
>>> +
>>> +        /* Enable available sensors */
>>> +        regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
>>> +
>>> +        /* Real temperature format */
>>> +        regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
>>> +
>>> +        regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
>>> +
>>> +        /* Enable TSENS */
>>> +        regmap_field_write(priv->rf[TSENS_EN], 0x1);
>>> +    }
>>> +
>>>       /* This loop might need changes if enum regfield_ids is 
>>> reordered */
>>>       for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
>>>           for (i = 0; i < priv->feat->max_sensors; i++) {
>>> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device 
>>> *pdev)
>>>       priv->dev = dev;
>>>       priv->num_sensors = num_sensors;
>>> +    priv->sensors_to_en = data->sensors_to_en;
>>>       priv->ops = data->ops;
>>>       for (i = 0;  i < priv->num_sensors; i++) {
>>>           if (data->hw_ids)
>>> diff --git a/drivers/thermal/qcom/tsens.h 
>>> b/drivers/thermal/qcom/tsens.h
>>> index 2805de1c6827..f8897bc8944e 100644
>>> --- a/drivers/thermal/qcom/tsens.h
>>> +++ b/drivers/thermal/qcom/tsens.h
>>> @@ -168,6 +168,58 @@ enum regfield_ids {
>>>       TSENS_SW_RST,
>>>       SENSOR_EN,
>>>       CODE_OR_TEMP,
>>> +    /* MEASURE_PERIOD */
>>> +    MAIN_MEASURE_PERIOD,
>>> +
>>> +    /* Sn_CONVERSION */
>>> +    SHIFT_0,
>>> +    SLOPE_0,
>>> +    CZERO_0,
>>> +    SHIFT_1,
>>> +    SLOPE_1,
>>> +    CZERO_1,
>>> +    SHIFT_2,
>>> +    SLOPE_2,
>>> +    CZERO_2,
>>> +    SHIFT_3,
>>> +    SLOPE_3,
>>> +    CZERO_3,
>>> +    SHIFT_4,
>>> +    SLOPE_4,
>>> +    CZERO_4,
>>> +    SHIFT_5,
>>> +    SLOPE_5,
>>> +    CZERO_5,
>>> +    SHIFT_6,
>>> +    SLOPE_6,
>>> +    CZERO_6,
>>> +    SHIFT_7,
>>> +    SLOPE_7,
>>> +    CZERO_7,
>>> +    SHIFT_8,
>>> +    SLOPE_8,
>>> +    CZERO_8,
>>> +    SHIFT_9,
>>> +    SLOPE_9,
>>> +    CZERO_9,
>>> +    SHIFT_10,
>>> +    SLOPE_10,
>>> +    CZERO_10,
>>> +    SHIFT_11,
>>> +    SLOPE_11,
>>> +    CZERO_11,
>>> +    SHIFT_12,
>>> +    SLOPE_12,
>>> +    CZERO_12,
>>> +    SHIFT_13,
>>> +    SLOPE_13,
>>> +    CZERO_13,
>>> +    SHIFT_14,
>>> +    SLOPE_14,
>>> +    CZERO_14,
>>> +    SHIFT_15,
>>> +    SLOPE_15,
>>> +    CZERO_15,
>>>       /* ----- TM ------ */
>>>       /* TRDY */
>>> @@ -524,6 +576,7 @@ struct tsens_features {
>>>   /**
>>>    * struct tsens_plat_data - tsens compile-time platform data
>>>    * @num_sensors: Number of sensors supported by platform
>>> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
>>>    * @ops: operations the tsens instance supports
>>>    * @hw_ids: Subset of sensors ids supported by platform, if not 
>>> the first n
>>>    * @feat: features of the IP
>>> @@ -531,6 +584,7 @@ struct tsens_features {
>>>    */
>>>   struct tsens_plat_data {
>>>       const u32        num_sensors;
>>> +    const u16        sensors_to_en;
>>
>> There is already a similar field, hw_ids. Can it be used instead?
>>
>>>       const struct tsens_ops    *ops;
>>>       unsigned int        *hw_ids;
>>>       struct tsens_features    *feat;
>>> @@ -551,6 +605,7 @@ struct tsens_context {
>>>    * struct tsens_priv - private data for each instance of the tsens IP
>>>    * @dev: pointer to struct device
>>>    * @num_sensors: number of sensors enabled on this device
>>> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
>>>    * @tm_map: pointer to TM register address space
>>>    * @srot_map: pointer to SROT register address space
>>>    * @tm_offset: deal with old device trees that don't address TM 
>>> and SROT
>>> @@ -569,6 +624,7 @@ struct tsens_context {
>>>   struct tsens_priv {
>>>       struct device            *dev;
>>>       u32                num_sensors;
>>> +    u16                sensors_to_en;
>>>       struct regmap            *tm_map;
>>>       struct regmap            *srot_map;
>>>       u32                tm_offset;
>>
>
--
Thanks,
Praveenkumar

Re: [PATCH 4/6] arm64: dts: qcom: ipq5332: Add tsens node

[Praveenkumar I]

On 7/10/2023 4:51 PM, Dmitry Baryshkov wrote:
> On 10/07/2023 13:37, Praveenkumar I wrote:
>> IPQ5332 has tsens v2.3.3 peripheral. This patch adds the tsense
>> node with nvmem cells for calibration data.
>>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5332.dtsi | 113 ++++++++++++++++++++++++++
>>   1 file changed, 113 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi 
>> b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> index 8bfc2db44624..a1e3527178c0 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> @@ -150,6 +150,91 @@ qfprom: efuse@a4000 {
>>               reg = <0x000a4000 0x721>;
>>               #address-cells = <1>;
>>               #size-cells = <1>;
>> +
>> +            tsens_mode: mode@3e1 {
>> +                reg = <0x3e1 0x1>;
>> +                bits = <0 3>;
>> +            };
>> +
>> +            tsens_base0: base0@3e1 {
>> +                reg = <0x3e1 0x2>;
>> +                bits = <3 10>;
>> +            };
>> +
>> +            tsens_base1: base1@3e2 {
>> +                reg = <0x3e2 0x2>;
>> +                bits = <5 10>;
>> +            };
>> +
>> +            s0_offset: s0_offset@3e4 {
>> +                reg = <0x3e4 0x1>;
>> +                bits = <0 4>;
>> +            };
>> +
>> +            s3_offset: s3_offset@3e5 {
>> +                reg = <0x3e5 0x1>;
>> +                bits = <4 4>;
>> +            };
>> +
>> +            s4_offset: s4_offset@3e6 {
>> +                reg = <0x3e6 0x1>;
>> +                bits = <0 4>;
>> +            };
>> +
>> +            s5_offset: s5_offset@3e6 {
>> +                reg = <0x3e6 0x1>;
>> +                bits = <4 4>;
>> +            };
>> +
>> +            s6_offset: s6_offset@3e8 {
>> +                reg = <0x3e8 0x1>;
>> +                bits = <0 4>;
>> +            };
>> +
>> +            s7_offset: s7_offset@3e8 {
>> +                reg = <0x3e8 0x1>;
>> +                bits = <4 4>;
>> +            };
>> +
>> +            s8_offset: s8_offset@3a4 {
>> +                reg = <0x3a4 0x1>;
>> +                bits = <0 4>;
>> +            };
>> +
>> +            s9_offset: s9_offset@3a4 {
>> +                reg = <0x3a4 0x1>;
>> +                bits = <4 4>;
>> +            };
>> +
>> +            s10_offset: s10_offset@3a5 {
>> +                reg = <0x3a5 0x1>;
>> +                bits = <0 4>;
>> +            };
>> +
>> +            s11_offset: s11_offset@3a5 {
>> +                reg = <0x3a5 0x1>;
>> +                bits = <4 4>;
>> +            };
>> +
>> +            s12_offset: s12_offset@3a6 {
>> +                reg = <0x3a6 0x1>;
>> +                bits = <0 4>;
>> +            };
>> +
>> +            s13_offset: s13_offset@3a6 {
>> +                reg = <0x3a6 0x1>;
>> +                bits = <4 4>;
>> +            };
>> +
>> +            s14_offset: s14_offset@3ad {
>> +                reg = <0x3ad 0x2>;
>> +                bits = <7 4>;
>> +            };
>> +
>> +            s15_offset: s0_offset@3ae {
>> +                reg = <0x3ae 0x1>;
>> +                bits = <3 4>;
>> +            };
>>           };
>>             rng: rng@e3000 {
>> @@ -159,6 +244,34 @@ rng: rng@e3000 {
>>               clock-names = "core";
>>           };
>>   +        tsens: thermal-sensor@4a9000 {
>> +            compatible = "qcom,ipq5332-tsens";
>> +            reg = <0x4a9000 0x1000>,
>> +                  <0x4a8000 0x1000>;
>> +            nvmem-cells = <&tsens_mode>, <&tsens_base0>,
>> +                    <&tsens_base1>, <&s0_offset>,
>
> Please align vertically.
Sure, will change in next patch.
>
>> + <&s3_offset>, <&s4_offset>,
>> +                    <&s5_offset>, <&s6_offset>,
>> +                    <&s7_offset>, <&s8_offset>,
>> +                    <&s9_offset>, <&s10_offset>,
>> +                    <&s11_offset>, <&s12_offset>,
>> +                    <&s13_offset>, <&s14_offset>,
>> +                    <&s15_offset>;
>> +            nvmem-cell-names = "mode", "base0",
>> +                        "base1", "s0_offset",
>
> And here.
Sure, will change in next patch.
>
>> +                        "s3_offset", "s4_offset",
>> +                        "s5_offset", "s6_offset",
>> +                        "s7_offset", "s8_offset",
>> +                        "s9_offset", "s10_offset",
>> +                        "s11_offset", "s12_offset",
>> +                        "s13_offset", "s14_offset",
>> +                        "s15_offset";
>> +            interrupts = <GIC_SPI 320 IRQ_TYPE_LEVEL_HIGH>;
>> +            interrupt-names = "combined";
>> +            #qcom,sensors = <16>;
>> +            #thermal-sensor-cells = <1>;
>> +        };
>> +
>>           tlmm: pinctrl@1000000 {
>>               compatible = "qcom,ipq5332-tlmm";
>>               reg = <0x01000000 0x300000>;
>

Re: [PATCH 5/6] arm64: dts: qcom: ipq5332: Add thermal zone nodes

[Praveenkumar I]

On 7/10/2023 4:53 PM, Dmitry Baryshkov wrote:
> On 10/07/2023 13:37, Praveenkumar I wrote:
>> This patch adds thermal zone nodes for sensors present in
>> IPQ5332.
>>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72 +++++++++++++++++++++++++++
>>   1 file changed, 72 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi 
>> b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> index a1e3527178c0..8b276aeca53e 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> @@ -527,4 +527,76 @@ timer {
>>                    <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | 
>> IRQ_TYPE_LEVEL_LOW)>,
>>                    <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | 
>> IRQ_TYPE_LEVEL_LOW)>;
>>       };
>> +
>> +    thermal-zones {
>> +        rfa-0-thermal{
>> +            polling-delay-passive = <0>;
>> +            polling-delay = <0>;
>> +            thermal-sensors = <&tsens 11>;
>> +
>> +            trips {
>> +                rfa-0-critical {
>> +                    temperature = <125000>;
>> +                    hysteresis = <1000>;
>> +                    type = "critical";
>> +                };
>> +            };
>> +        };
>> +
>> +        rfa-1-thermal {
>> +            polling-delay-passive = <0>;
>> +            polling-delay = <0>;
>> +            thermal-sensors = <&tsens 12>;
>> +
>> +            trips {
>> +                rfa-1-critical {
>> +                    temperature = <125000>;
>> +                    hysteresis = <1000>;
>> +                    type = "critical";
>> +                };
>> +            };
>> +        };
>> +
>> +        misc-thermal {
>> +            polling-delay-passive = <0>;
>> +            polling-delay = <0>;
>> +            thermal-sensors = <&tsens 13>;
>> +
>> +            trips {
>> +                misc-critical {
>> +                    temperature = <125000>;
>> +                    hysteresis = <1000>;
>> +                    type = "critical";
>> +                };
>> +            };
>> +        };
>> +
>> +        cpu-top-thermal {
>> +            polling-delay-passive = <0>;
>> +            polling-delay = <0>;
>> +            thermal-sensors = <&tsens 14>;
>> +
>> +            trips {
>> +                cpu-top-critical {
>> +                    temperature = <125000>;
>> +                    hysteresis = <1000>;
>> +                    type = "critical";
>> +                };
>> +            };
>
> Could you please add a passive cooling devices for the CPU?
Sure, will add the passive trip.
>
>> +        };
>> +
>> +        top-glue-thermal {
>> +            polling-delay-passive = <0>;
>> +            polling-delay = <0>;
>> +            thermal-sensors = <&tsens 15>;
>> +
>> +            trips {
>> +                top-glue-critical {
>> +                    temperature = <125000>;
>> +                    hysteresis = <1000>;
>> +                    type = "critical";
>> +                };
>> +            };
>> +        };
>> +    };
>>   };
>
--
Thanks,
Praveenkumar

Re: [PATCH 5/6] arm64: dts: qcom: ipq5332: Add thermal zone nodes

[Praveenkumar I]

On 7/10/2023 5:44 PM, Konrad Dybcio wrote:
> On 10.07.2023 13:23, Dmitry Baryshkov wrote:
>> On 10/07/2023 13:37, Praveenkumar I wrote:
>>> This patch adds thermal zone nodes for sensors present in
>>> IPQ5332.
>>>
>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>> ---
>>>    arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72 +++++++++++++++++++++++++++
>>>    1 file changed, 72 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>> index a1e3527178c0..8b276aeca53e 100644
>>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>> @@ -527,4 +527,76 @@ timer {
>>>                     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
>>>                     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
>>>        };
>>> +
>>> +    thermal-zones {
>>> +        rfa-0-thermal{
> thermal {
In all other DTS, name is 'thermal-zones". Hence followed the same.
>
>>> +            polling-delay-passive = <0>;
>>> +            polling-delay = <0>;
>>> +            thermal-sensors = <&tsens 11>;
>>> +
>>> +            trips {
> Indentation seems off, tab size for kernel code is 8 spaces.
Sure, will check the indent and update in next patch.
>
> Konrad
>>> +                rfa-0-critical {
>>> +                    temperature = <125000>;
>>> +                    hysteresis = <1000>;
>>> +                    type = "critical";
>>> +                };
>>> +            };
>>> +        };
>>> +
>>> +        rfa-1-thermal {
>>> +            polling-delay-passive = <0>;
>>> +            polling-delay = <0>;
>>> +            thermal-sensors = <&tsens 12>;
>>> +
>>> +            trips {
>>> +                rfa-1-critical {
>>> +                    temperature = <125000>;
>>> +                    hysteresis = <1000>;
>>> +                    type = "critical";
>>> +                };
>>> +            };
>>> +        };
>>> +
>>> +        misc-thermal {
>>> +            polling-delay-passive = <0>;
>>> +            polling-delay = <0>;
>>> +            thermal-sensors = <&tsens 13>;
>>> +
>>> +            trips {
>>> +                misc-critical {
>>> +                    temperature = <125000>;
>>> +                    hysteresis = <1000>;
>>> +                    type = "critical";
>>> +                };
>>> +            };
>>> +        };
>>> +
>>> +        cpu-top-thermal {
>>> +            polling-delay-passive = <0>;
>>> +            polling-delay = <0>;
>>> +            thermal-sensors = <&tsens 14>;
>>> +
>>> +            trips {
>>> +                cpu-top-critical {
>>> +                    temperature = <125000>;
>>> +                    hysteresis = <1000>;
>>> +                    type = "critical";
>>> +                };
>>> +            };
>> Could you please add a passive cooling devices for the CPU?
>>
>>> +        };
>>> +
>>> +        top-glue-thermal {
>>> +            polling-delay-passive = <0>;
>>> +            polling-delay = <0>;
>>> +            thermal-sensors = <&tsens 15>;
>>> +
>>> +            trips {
>>> +                top-glue-critical {
>>> +                    temperature = <125000>;
>>> +                    hysteresis = <1000>;
>>> +                    type = "critical";
>>> +                };
>>> +            };
>>> +        };
>>> +    };
>>>    };
--
Thanks,
Praveenkumar

Re: [PATCH 6/6] thermal/drivers/tsens: Add IPQ5332 support

[Praveenkumar I]

On 7/10/2023 4:54 PM, Dmitry Baryshkov wrote:
> On 10/07/2023 13:37, Praveenkumar I wrote:
>> IPQ5332 uses tsens v2.3.3 IP and it is having combined interrupt as
>> like IPQ8074. But as the SoCs does not have RPM, kernel needs to
>> take care of sensor enablement and calibration. Hence introduced
>> new ops and data for IPQ5332 and reused the feature_config from
>> IPQ8074.
>>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> ---
>>   drivers/thermal/qcom/tsens-v2.c | 13 +++++++++++++
>>   drivers/thermal/qcom/tsens.c    |  3 +++
>>   drivers/thermal/qcom/tsens.h    |  2 +-
>>   3 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-v2.c 
>> b/drivers/thermal/qcom/tsens-v2.c
>> index db48b1d95348..8b6e3876fd2c 100644
>> --- a/drivers/thermal/qcom/tsens-v2.c
>> +++ b/drivers/thermal/qcom/tsens-v2.c
>> @@ -237,6 +237,19 @@ struct tsens_plat_data data_ipq8074 = {
>>       .fields    = tsens_v2_regfields,
>>   };
>>   +static const struct tsens_ops ops_ipq5332_v2 = {
>
> Please drop v2. It is unclear if it refers to tsens being v2 or being 
> specific to ipq5332 v2.
Sure, will drop v2.
>
>> +    .init        = init_common,
>> +    .get_temp    = get_temp_tsens_valid,
>> +    .calibrate    = tsens_v2_calibration,
>> +};
>> +
>> +struct tsens_plat_data data_ipq5332 = {
>> +    .sensors_to_en    = 0xF800,
>
> This doesn't seem to match the offsets that you have enabled in the DTSI.
In order to overcome the DT binding check failure, added all the 
available QFPROM offsets in the DTSI. Else DT binding check failing on 
"nvmem-cell-names".
>
>> +    .ops        = &ops_ipq5332_v2,
>> +    .feat        = &ipq8074_feat,
>> +    .fields        = tsens_v2_regfields,
>> +};
>> +
>>   /* Kept around for backward compatibility with old msm8996.dtsi */
>>   struct tsens_plat_data data_8996 = {
>>       .num_sensors    = 13,
>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>> index 169690355dad..e8ba2901cda8 100644
>> --- a/drivers/thermal/qcom/tsens.c
>> +++ b/drivers/thermal/qcom/tsens.c
>> @@ -1140,6 +1140,9 @@ static const struct of_device_id tsens_table[] = {
>>       }, {
>>           .compatible = "qcom,ipq8074-tsens",
>>           .data = &data_ipq8074,
>> +    }, {
>> +        .compatible = "qcom,ipq5332-tsens",
>> +        .data = &data_ipq5332,
>>       }, {
>>           .compatible = "qcom,mdm9607-tsens",
>>           .data = &data_9607,
>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>> index f8897bc8944e..36040f9beebc 100644
>> --- a/drivers/thermal/qcom/tsens.h
>> +++ b/drivers/thermal/qcom/tsens.h
>> @@ -701,6 +701,6 @@ extern struct tsens_plat_data data_8226, 
>> data_8909, data_8916, data_8939, data_8
>>   extern struct tsens_plat_data data_tsens_v1, data_8976, data_8956;
>>     /* TSENS v2 targets */
>> -extern struct tsens_plat_data data_8996, data_ipq8074, data_tsens_v2;
>> +extern struct tsens_plat_data data_8996, data_ipq8074, data_ipq5332, 
>> data_tsens_v2;
>>     #endif /* __QCOM_TSENS_H__ */
>
--
Thanks,
Praveenkumar

Re: [PATCH 5/6] arm64: dts: qcom: ipq5332: Add thermal zone nodes

[Praveenkumar I]

On 7/10/2023 7:04 PM, Praveenkumar I wrote:
>
> On 7/10/2023 5:44 PM, Konrad Dybcio wrote:
>> On 10.07.2023 13:23, Dmitry Baryshkov wrote:
>>> On 10/07/2023 13:37, Praveenkumar I wrote:
>>>> This patch adds thermal zone nodes for sensors present in
>>>> IPQ5332.
>>>>
>>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/ipq5332.dtsi | 72 
>>>> +++++++++++++++++++++++++++
>>>>    1 file changed, 72 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi 
>>>> b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>>> index a1e3527178c0..8b276aeca53e 100644
>>>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>>>> @@ -527,4 +527,76 @@ timer {
>>>>                     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | 
>>>> IRQ_TYPE_LEVEL_LOW)>,
>>>>                     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | 
>>>> IRQ_TYPE_LEVEL_LOW)>;
>>>>        };
>>>> +
>>>> +    thermal-zones {
>>>> +        rfa-0-thermal{
>> thermal {
> In all other DTS, name is 'thermal-zones". Hence followed the same.
Sorry, understood now. Will give space after "rfa-0-thermal"
>>
>>>> +            polling-delay-passive = <0>;
>>>> +            polling-delay = <0>;
>>>> +            thermal-sensors = <&tsens 11>;
>>>> +
>>>> +            trips {
>> Indentation seems off, tab size for kernel code is 8 spaces.
> Sure, will check the indent and update in next patch.
>>
>> Konrad
>>>> +                rfa-0-critical {
>>>> +                    temperature = <125000>;
>>>> +                    hysteresis = <1000>;
>>>> +                    type = "critical";
>>>> +                };
>>>> +            };
>>>> +        };
>>>> +
>>>> +        rfa-1-thermal {
>>>> +            polling-delay-passive = <0>;
>>>> +            polling-delay = <0>;
>>>> +            thermal-sensors = <&tsens 12>;
>>>> +
>>>> +            trips {
>>>> +                rfa-1-critical {
>>>> +                    temperature = <125000>;
>>>> +                    hysteresis = <1000>;
>>>> +                    type = "critical";
>>>> +                };
>>>> +            };
>>>> +        };
>>>> +
>>>> +        misc-thermal {
>>>> +            polling-delay-passive = <0>;
>>>> +            polling-delay = <0>;
>>>> +            thermal-sensors = <&tsens 13>;
>>>> +
>>>> +            trips {
>>>> +                misc-critical {
>>>> +                    temperature = <125000>;
>>>> +                    hysteresis = <1000>;
>>>> +                    type = "critical";
>>>> +                };
>>>> +            };
>>>> +        };
>>>> +
>>>> +        cpu-top-thermal {
>>>> +            polling-delay-passive = <0>;
>>>> +            polling-delay = <0>;
>>>> +            thermal-sensors = <&tsens 14>;
>>>> +
>>>> +            trips {
>>>> +                cpu-top-critical {
>>>> +                    temperature = <125000>;
>>>> +                    hysteresis = <1000>;
>>>> +                    type = "critical";
>>>> +                };
>>>> +            };
>>> Could you please add a passive cooling devices for the CPU?
>>>
>>>> +        };
>>>> +
>>>> +        top-glue-thermal {
>>>> +            polling-delay-passive = <0>;
>>>> +            polling-delay = <0>;
>>>> +            thermal-sensors = <&tsens 15>;
>>>> +
>>>> +            trips {
>>>> +                top-glue-critical {
>>>> +                    temperature = <125000>;
>>>> +                    hysteresis = <1000>;
>>>> +                    type = "critical";
>>>> +                };
>>>> +            };
>>>> +        };
>>>> +    };
>>>>    };
> -- 
> Thanks,
> Praveenkumar

Re: [PATCH 2/6] thermal/drivers/tsens: Add TSENS enable and calibration support for V2

[Dmitry Baryshkov]

On Mon, 10 Jul 2023 at 16:22, Praveenkumar I <quic_ipkumar@quicinc.com> wrote:
>
>
> On 7/10/2023 4:49 PM, Dmitry Baryshkov wrote:
> > On 10/07/2023 13:37, Praveenkumar I wrote:
> >> SoCs without RPM have to enable sensors and calibrate from the kernel.
> >> Though TSENS IP supports 16 sensors, not all are used. So added
> >> sensors_to_en in tsens data help enable the relevant sensors.
> >>
> >> Added new calibration function for V2 as the tsens.c calib function
> >> only supports V1.
> >>
> >> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> >> ---
> >>   drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
> >>   drivers/thermal/qcom/tsens.c    |  37 +++++++++-
> >>   drivers/thermal/qcom/tsens.h    |  56 +++++++++++++++
> >>   3 files changed, 208 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/thermal/qcom/tsens-v2.c
> >> b/drivers/thermal/qcom/tsens-v2.c
> >> index 29a61d2d6ca3..db48b1d95348 100644
> >> --- a/drivers/thermal/qcom/tsens-v2.c
> >> +++ b/drivers/thermal/qcom/tsens-v2.c
> >> @@ -6,11 +6,20 @@
> >>     #include <linux/bitops.h>
> >>   #include <linux/regmap.h>
> >> +#include <linux/nvmem-consumer.h>
> >>   #include "tsens.h"
> >>     /* ----- SROT ------ */
> >>   #define SROT_HW_VER_OFF    0x0000
> >>   #define SROT_CTRL_OFF        0x0004
> >> +#define SROT_MEASURE_PERIOD    0x0008
> >> +#define SROT_Sn_CONVERSION    0x0060
> >> +#define V2_SHIFT_DEFAULT    0x0003
> >> +#define V2_SLOPE_DEFAULT    0x0cd0
> >> +#define V2_CZERO_DEFAULT    0x016a
> >> +#define ONE_PT_SLOPE        0x0cd0
> >> +#define TWO_PT_SHIFTED_GAIN    921600
> >> +#define ONE_PT_CZERO_CONST    94
> >>     /* ----- TM ------ */
> >>   #define TM_INT_EN_OFF            0x0004
> >> @@ -59,6 +68,16 @@ static const struct reg_field
> >> tsens_v2_regfields[MAX_REGFIELDS] = {
> >>       /* CTRL_OFF */
> >>       [TSENS_EN]     = REG_FIELD(SROT_CTRL_OFF,    0,  0),
> >>       [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF,    1,  1),
> >> +    [SENSOR_EN]    = REG_FIELD(SROT_CTRL_OFF,    3,  18),
> >> +    [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF,    21, 21),
> >> +
> >> +    /* MAIN_MEASURE_PERIOD */
> >> +    [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
> >> +
> >> +    /* Sn Conversion */
> >> +    REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
> >> +    REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
> >> +    REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
> >>         /* ----- TM ------ */
> >>       /* INTERRUPT ENABLE */
> >> @@ -104,6 +123,103 @@ static const struct reg_field
> >> tsens_v2_regfields[MAX_REGFIELDS] = {
> >>       [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
> >>   };
> >>   +static int tsens_v2_calibration(struct tsens_priv *priv)
> >> +{
> >> +    struct device *dev = priv->dev;
> >> +    u32 mode, base0, base1;
> >> +    u32 slope, czero;
> >> +    char name[15];
> >> +    int i, j, ret;
> >> +
> >> +    if (priv->num_sensors > MAX_SENSORS)
> >> +        return -EINVAL;
> >> +
> >> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
> >> +    if (ret == -ENOENT)
> >> +        dev_warn(priv->dev, "Calibration data not present in DT\n");
> >> +    if (ret < 0)
> >> +        return ret;
> >> +
> >> +    dev_dbg(priv->dev, "calibration mode is %d\n", mode);
> >> +
> >> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
> >> +    if (ret < 0)
> >> +        return ret;
> >> +
> >> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
> >> +    if (ret < 0)
> >> +        return ret;
> >> +
> >> +    /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap
> >> for enabled sensors */
> >> +    for (i = 0; i < priv->num_sensors; i++) {
> >> +        if (!(priv->sensors_to_en & (0x1 << i)))
> >> +            continue;
> >> +
> >> +        ret = snprintf(name, sizeof(name), "s%d_offset",
> >> priv->sensor[i].hw_id);
> >> +        if (ret < 0)
> >> +            return ret;
> >> +
> >> +        ret = nvmem_cell_read_variable_le_u32(priv->dev, name,
> >> &priv->sensor[i].offset);
> >> +        if (ret)
> >> +            return ret;
> >> +
> >> +        for (j = SHIFT_0; j <= CZERO_0; j++) {
> >> +            int idx = (i * 3) + j;
> >> +
> >> +            priv->rf[idx] = devm_regmap_field_alloc(dev,
> >> priv->srot_map,
> >> +                                priv->fields[idx]);
> >> +            if (IS_ERR(priv->rf[idx]))
> >> +                return PTR_ERR(priv->rf[idx]);
> >
> > I think, allocating data structures for 48 regfields, which are
> > written just once, to be an overkill.
> Can we change it to single field for each sensor. For example,
> CONVERSION_0 instead of SHIFT_0, SLOPE_0 and CZERO_0? This way it will
> be max 16 regfields.

If you move writing of the registers to the loop, you won't need
regfields. You can just call regmap_update_bits. The point is that you
don't have to allocate a one-time instance.

> >
> >> +        }
> >> +    }
> >> +
> >> +    /* Based on calib mode, program SHIFT, SLOPE and CZERO for
> >> enabled sensors */
> >> +    switch (mode) {
> >> +    case TWO_PT_CALIB:
> >> +        slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
> >> +
> >> +        for (i = 0; i < priv->num_sensors; i++) {
> >> +            if (!(priv->sensors_to_en & (0x1 << i)))
> >> +                continue;
> >> +
> >> +            int idx = i * 3;
> >> +
> >> +            czero = (base0 + priv->sensor[i].offset - ((base1 -
> >> base0) / 3));
> >> +            regmap_field_write(priv->rf[SHIFT_0 + idx],
> >> V2_SHIFT_DEFAULT);
> >> +            regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
> >> +            regmap_field_write(priv->rf[CZERO_0 + idx], czero);
> >> +        }
> >> +        fallthrough;
> >> +    case ONE_PT_CALIB2:
> >> +        for (i = 0; i < priv->num_sensors; i++) {
> >> +            if (!(priv->sensors_to_en & (0x1 << i)))
> >> +                continue;
> >> +
> >> +            int idx = i * 3;
> >> +
> >> +            czero = base0 + priv->sensor[i].offset -
> >> ONE_PT_CZERO_CONST;
> >> +            regmap_field_write(priv->rf[SHIFT_0 + idx],
> >> V2_SHIFT_DEFAULT);
> >> +            regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
> >> +            regmap_field_write(priv->rf[CZERO_0 + idx], czero);
> >> +        }
> >> +        break;
> >> +    default:
> >> +        dev_dbg(priv->dev, "calibrationless mode\n");
> >> +        for (i = 0; i < priv->num_sensors; i++) {
> >> +            if (!(priv->sensors_to_en & (0x1 << i)))
> >> +                continue;
> >> +
> >> +            int idx = i * 3;
> >> +
> >> +            regmap_field_write(priv->rf[SHIFT_0 + idx],
> >> V2_SHIFT_DEFAULT);
> >> +            regmap_field_write(priv->rf[SLOPE_0 + idx],
> >> V2_SLOPE_DEFAULT);
> >> +            regmap_field_write(priv->rf[CZERO_0 + idx],
> >> V2_CZERO_DEFAULT);
> >> +        }
> >> +    }
> >
> > This code iterates over the sensors field several times. Please
> > consider extracting a function that handles all setup for a single
> > sensor, then calling it in a loop (I should probably do the same for
> > tsens-v0/v1 too).
> Sure. After reading the mode0, base0 and base1 from QFPROM, we can call
> a function in a loop to setup the calibration for each sensor.
> >
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>   static const struct tsens_ops ops_generic_v2 = {
> >>       .init        = init_common,
> >>       .get_temp    = get_temp_tsens_valid,
> >> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> >> index 98c356acfe98..169690355dad 100644
> >> --- a/drivers/thermal/qcom/tsens.c
> >> +++ b/drivers/thermal/qcom/tsens.c
> >> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
> >>       ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
> >>       if (ret)
> >>           goto err_put_device;
> >> -    if (!enabled) {
> >> +    if (!enabled && !priv->sensors_to_en) {
> >>           dev_err(dev, "%s: device not enabled\n", __func__);
> >>           ret = -ENODEV;
> >>           goto err_put_device;
> >> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
> >>           goto err_put_device;
> >>       }
> >>   +    /* Do TSENS initialization if required */
> >> +    if (priv->sensors_to_en) {
> >
> > Maybe it would be better to explicitly add VER_2_X_NO_RPM and check it
> > here?
> Sure, will add a separate version macro.
> >
> >> +        priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev,
> >> priv->srot_map,
> >> + priv->fields[CODE_OR_TEMP]);
> >> +        if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
> >> +            ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
> >> +            goto err_put_device;
> >> +        }
> >> +
> >> +        priv->rf[MAIN_MEASURE_PERIOD] =
> >> +            devm_regmap_field_alloc(dev, priv->srot_map,
> >> +                        priv->fields[MAIN_MEASURE_PERIOD]);
> >> +        if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
> >> +            ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
> >> +            goto err_put_device;
> >> +        }
> >> +
> >> +        regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
> >> +
> >> +        /* Update measure period to 2ms */
> >> +        regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
> >> +
> >> +        /* Enable available sensors */
> >> +        regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
> >> +
> >> +        /* Real temperature format */
> >> +        regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
> >> +
> >> +        regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
> >> +
> >> +        /* Enable TSENS */
> >> +        regmap_field_write(priv->rf[TSENS_EN], 0x1);
> >> +    }
> >> +
> >>       /* This loop might need changes if enum regfield_ids is
> >> reordered */
> >>       for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
> >>           for (i = 0; i < priv->feat->max_sensors; i++) {
> >> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device
> >> *pdev)
> >>         priv->dev = dev;
> >>       priv->num_sensors = num_sensors;
> >> +    priv->sensors_to_en = data->sensors_to_en;
> >>       priv->ops = data->ops;
> >>       for (i = 0;  i < priv->num_sensors; i++) {
> >>           if (data->hw_ids)
> >> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> >> index 2805de1c6827..f8897bc8944e 100644
> >> --- a/drivers/thermal/qcom/tsens.h
> >> +++ b/drivers/thermal/qcom/tsens.h
> >> @@ -168,6 +168,58 @@ enum regfield_ids {
> >>       TSENS_SW_RST,
> >>       SENSOR_EN,
> >>       CODE_OR_TEMP,
> >> +    /* MEASURE_PERIOD */
> >> +    MAIN_MEASURE_PERIOD,
> >> +
> >> +    /* Sn_CONVERSION */
> >> +    SHIFT_0,
> >> +    SLOPE_0,
> >> +    CZERO_0,
> >> +    SHIFT_1,
> >> +    SLOPE_1,
> >> +    CZERO_1,
> >> +    SHIFT_2,
> >> +    SLOPE_2,
> >> +    CZERO_2,
> >> +    SHIFT_3,
> >> +    SLOPE_3,
> >> +    CZERO_3,
> >> +    SHIFT_4,
> >> +    SLOPE_4,
> >> +    CZERO_4,
> >> +    SHIFT_5,
> >> +    SLOPE_5,
> >> +    CZERO_5,
> >> +    SHIFT_6,
> >> +    SLOPE_6,
> >> +    CZERO_6,
> >> +    SHIFT_7,
> >> +    SLOPE_7,
> >> +    CZERO_7,
> >> +    SHIFT_8,
> >> +    SLOPE_8,
> >> +    CZERO_8,
> >> +    SHIFT_9,
> >> +    SLOPE_9,
> >> +    CZERO_9,
> >> +    SHIFT_10,
> >> +    SLOPE_10,
> >> +    CZERO_10,
> >> +    SHIFT_11,
> >> +    SLOPE_11,
> >> +    CZERO_11,
> >> +    SHIFT_12,
> >> +    SLOPE_12,
> >> +    CZERO_12,
> >> +    SHIFT_13,
> >> +    SLOPE_13,
> >> +    CZERO_13,
> >> +    SHIFT_14,
> >> +    SLOPE_14,
> >> +    CZERO_14,
> >> +    SHIFT_15,
> >> +    SLOPE_15,
> >> +    CZERO_15,
> >>         /* ----- TM ------ */
> >>       /* TRDY */
> >> @@ -524,6 +576,7 @@ struct tsens_features {
> >>   /**
> >>    * struct tsens_plat_data - tsens compile-time platform data
> >>    * @num_sensors: Number of sensors supported by platform
> >> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
> >>    * @ops: operations the tsens instance supports
> >>    * @hw_ids: Subset of sensors ids supported by platform, if not the
> >> first n
> >>    * @feat: features of the IP
> >> @@ -531,6 +584,7 @@ struct tsens_features {
> >>    */
> >>   struct tsens_plat_data {
> >>       const u32        num_sensors;
> >> +    const u16        sensors_to_en;
> >
> > There is already a similar field, hw_ids. Can it be used instead?
> Yes, it can be used. I missed to check this hw_ids. Will change the
> num_sensors to 5 and use the hw_ids.
> >
> >>       const struct tsens_ops    *ops;
> >>       unsigned int        *hw_ids;
> >>       struct tsens_features    *feat;
> >> @@ -551,6 +605,7 @@ struct tsens_context {
> >>    * struct tsens_priv - private data for each instance of the tsens IP
> >>    * @dev: pointer to struct device
> >>    * @num_sensors: number of sensors enabled on this device
> >> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
> >>    * @tm_map: pointer to TM register address space
> >>    * @srot_map: pointer to SROT register address space
> >>    * @tm_offset: deal with old device trees that don't address TM and
> >> SROT
> >> @@ -569,6 +624,7 @@ struct tsens_context {
> >>   struct tsens_priv {
> >>       struct device            *dev;
> >>       u32                num_sensors;
> >> +    u16                sensors_to_en;
> >>       struct regmap            *tm_map;
> >>       struct regmap            *srot_map;
> >>       u32                tm_offset;
> >
> --
> Thanks,
> Praveenkumar



-- 
With best wishes
Dmitry

Re: [PATCH 6/6] thermal/drivers/tsens: Add IPQ5332 support

[Dmitry Baryshkov]

On Mon, 10 Jul 2023 at 16:47, Praveenkumar I <quic_ipkumar@quicinc.com> wrote:
>
>
> On 7/10/2023 4:54 PM, Dmitry Baryshkov wrote:
> > On 10/07/2023 13:37, Praveenkumar I wrote:
> >> IPQ5332 uses tsens v2.3.3 IP and it is having combined interrupt as
> >> like IPQ8074. But as the SoCs does not have RPM, kernel needs to
> >> take care of sensor enablement and calibration. Hence introduced
> >> new ops and data for IPQ5332 and reused the feature_config from
> >> IPQ8074.
> >>
> >> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> >> ---
> >>   drivers/thermal/qcom/tsens-v2.c | 13 +++++++++++++
> >>   drivers/thermal/qcom/tsens.c    |  3 +++
> >>   drivers/thermal/qcom/tsens.h    |  2 +-
> >>   3 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/thermal/qcom/tsens-v2.c
> >> b/drivers/thermal/qcom/tsens-v2.c
> >> index db48b1d95348..8b6e3876fd2c 100644
> >> --- a/drivers/thermal/qcom/tsens-v2.c
> >> +++ b/drivers/thermal/qcom/tsens-v2.c
> >> @@ -237,6 +237,19 @@ struct tsens_plat_data data_ipq8074 = {
> >>       .fields    = tsens_v2_regfields,
> >>   };
> >>   +static const struct tsens_ops ops_ipq5332_v2 = {
> >
> > Please drop v2. It is unclear if it refers to tsens being v2 or being
> > specific to ipq5332 v2.
> Sure, will drop v2.
> >
> >> +    .init        = init_common,
> >> +    .get_temp    = get_temp_tsens_valid,
> >> +    .calibrate    = tsens_v2_calibration,
> >> +};
> >> +
> >> +struct tsens_plat_data data_ipq5332 = {
> >> +    .sensors_to_en    = 0xF800,
> >
> > This doesn't seem to match the offsets that you have enabled in the DTSI.
> In order to overcome the DT binding check failure, added all the
> available QFPROM offsets in the DTSI. Else DT binding check failing on
> "nvmem-cell-names".

This is not a way to overcome issues in DT bindings. Please fix DT
bindings instead by using alternatives, enums, etc.

> >
> >> +    .ops        = &ops_ipq5332_v2,
> >> +    .feat        = &ipq8074_feat,
> >> +    .fields        = tsens_v2_regfields,
> >> +};
> >> +
> >>   /* Kept around for backward compatibility with old msm8996.dtsi */
> >>   struct tsens_plat_data data_8996 = {
> >>       .num_sensors    = 13,
> >> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
> >> index 169690355dad..e8ba2901cda8 100644
> >> --- a/drivers/thermal/qcom/tsens.c
> >> +++ b/drivers/thermal/qcom/tsens.c
> >> @@ -1140,6 +1140,9 @@ static const struct of_device_id tsens_table[] = {
> >>       }, {
> >>           .compatible = "qcom,ipq8074-tsens",
> >>           .data = &data_ipq8074,
> >> +    }, {
> >> +        .compatible = "qcom,ipq5332-tsens",
> >> +        .data = &data_ipq5332,
> >>       }, {
> >>           .compatible = "qcom,mdm9607-tsens",
> >>           .data = &data_9607,
> >> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> >> index f8897bc8944e..36040f9beebc 100644
> >> --- a/drivers/thermal/qcom/tsens.h
> >> +++ b/drivers/thermal/qcom/tsens.h
> >> @@ -701,6 +701,6 @@ extern struct tsens_plat_data data_8226,
> >> data_8909, data_8916, data_8939, data_8
> >>   extern struct tsens_plat_data data_tsens_v1, data_8976, data_8956;
> >>     /* TSENS v2 targets */
> >> -extern struct tsens_plat_data data_8996, data_ipq8074, data_tsens_v2;
> >> +extern struct tsens_plat_data data_8996, data_ipq8074, data_ipq5332,
> >> data_tsens_v2;
> >>     #endif /* __QCOM_TSENS_H__ */
> >
> --
> Thanks,
> Praveenkumar



-- 
With best wishes
Dmitry

Re: [PATCH 3/6] dt-bindings: thermal: tsens: Add ipq5332 compatible

[Krzysztof Kozlowski]

On 10/07/2023 12:37, Praveenkumar I wrote:
> IPQ5332 uses TSENS v2.3.3 with combined interrupt. RPM is not
> available in the SoC, hence adding new compatible to have the
> sensor enablement and calibration function.>
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> ---
>  Documentation/devicetree/bindings/thermal/qcom-tsens.yaml | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> index 8b7863c3989e..ee57713f6131 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> @@ -68,8 +68,10 @@ properties:
>            - const: qcom,tsens-v2
>  
>        - description: v2 of TSENS with combined interrupt
> -        enum:
> -          - qcom,ipq8074-tsens
> +        items:

Drop items, you do not have multiple items.

> +          - enum:
> +              - qcom,ipq8074-tsens
> +              - qcom,ipq5332-tsens

Keep the order.
>  
>        - description: v2 of TSENS with combined interrupt
>          items:
> @@ -289,6 +291,7 @@ allOf:
>            contains:
>              enum:
>                - qcom,ipq8074-tsens
> +              - qcom,ipq5332-tsens

And here

>      then:
>        properties:
>          interrupts:
> @@ -304,6 +307,7 @@ allOf:
>            contains:
>              enum:
>                - qcom,ipq8074-tsens
> +              - qcom,ipq5332-tsens

And here.

>                - qcom,tsens-v0_1
>                - qcom,tsens-v1
>                - qcom,tsens-v2

Best regards,
Krzysztof

Re: [PATCH 4/6] arm64: dts: qcom: ipq5332: Add tsens node

[Krzysztof Kozlowski]

On 10/07/2023 12:37, Praveenkumar I wrote:
> IPQ5332 has tsens v2.3.3 peripheral. This patch adds the tsense
> node with nvmem cells for calibration data.
> 
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 113 ++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index 8bfc2db44624..a1e3527178c0 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -150,6 +150,91 @@ qfprom: efuse@a4000 {
>  			reg = <0x000a4000 0x721>;
>  			#address-cells = <1>;
>  			#size-cells = <1>;
> +
> +			tsens_mode: mode@3e1 {
> +				reg = <0x3e1 0x1>;
> +				bits = <0 3>;
> +			};
> +
> +			tsens_base0: base0@3e1 {
> +				reg = <0x3e1 0x2>;
> +				bits = <3 10>;
> +			};
> +
> +			tsens_base1: base1@3e2 {
> +				reg = <0x3e2 0x2>;
> +				bits = <5 10>;
> +			};
> +
> +			s0_offset: s0_offset@3e4 {

Underscores are not allowed in node names.

Best regards,
Krzysztof

Re: [PATCH 1/6] dt-bindings: thermal: tsens: Add nvmem cells for calibration data

[Krzysztof Kozlowski]

On 10/07/2023 12:37, Praveenkumar I wrote:
> Add TSENS V2 calibration nvmem cells for IPQ5332
> 
> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
> ---
>  .../bindings/thermal/qcom-tsens.yaml          | 26 +++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> index 27e9e16e6455..8b7863c3989e 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> @@ -91,7 +91,7 @@ properties:
>      maxItems: 2
>  
>    nvmem-cells:
> -    oneOf:
> +    anyOf:
>        - minItems: 1
>          maxItems: 2
>          description:
> @@ -106,9 +106,13 @@ properties:
>          description: |
>            Reference to nvmem cells for the calibration mode, two calibration
>            bases and two cells per each sensor, main and backup copies, plus use_backup cell
> +      - maxItems: 17
> +        description: |
> +          V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
> +          bases and one cell per each sensor

I think this is already included in one of the previous entries.
Otherwise, are you sure that all new devices will have exactly 17 entries?

>  
>    nvmem-cell-names:
> -    oneOf:
> +    anyOf:
>        - minItems: 1
>          items:
>            - const: calib
> @@ -205,6 +209,24 @@ properties:
>            - const: s9_p2_backup
>            - const: s10_p1_backup
>            - const: s10_p2_backup
> +      - items:
> +          - const: mode
> +          - const: base0
> +          - const: base1
> +          - const: s0_offset
> +          - const: s3_offset
> +          - const: s4_offset
> +          - const: s5_offset
> +          - const: s6_offset
> +          - const: s7_offset
> +          - const: s8_offset
> +          - const: s9_offset
> +          - const: s10_offset
> +          - const: s11_offset
> +          - const: s12_offset
> +          - const: s13_offset
> +          - const: s14_offset
> +          - const: s15_offset

Don't introduce new naming style. Existing uses s[0-9]+, without offset
suffix. Why this should be different?

>  
>    "#qcom,sensors":
>      description:

Best regards,
Krzysztof

Re: [PATCH 2/6] thermal/drivers/tsens: Add TSENS enable and calibration support for V2

[Praveenkumar I]

On 7/10/2023 8:32 PM, Dmitry Baryshkov wrote:
> On Mon, 10 Jul 2023 at 16:22, Praveenkumar I <quic_ipkumar@quicinc.com> wrote:
>>
>> On 7/10/2023 4:49 PM, Dmitry Baryshkov wrote:
>>> On 10/07/2023 13:37, Praveenkumar I wrote:
>>>> SoCs without RPM have to enable sensors and calibrate from the kernel.
>>>> Though TSENS IP supports 16 sensors, not all are used. So added
>>>> sensors_to_en in tsens data help enable the relevant sensors.
>>>>
>>>> Added new calibration function for V2 as the tsens.c calib function
>>>> only supports V1.
>>>>
>>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>>> ---
>>>>    drivers/thermal/qcom/tsens-v2.c | 116 ++++++++++++++++++++++++++++++++
>>>>    drivers/thermal/qcom/tsens.c    |  37 +++++++++-
>>>>    drivers/thermal/qcom/tsens.h    |  56 +++++++++++++++
>>>>    3 files changed, 208 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/thermal/qcom/tsens-v2.c
>>>> b/drivers/thermal/qcom/tsens-v2.c
>>>> index 29a61d2d6ca3..db48b1d95348 100644
>>>> --- a/drivers/thermal/qcom/tsens-v2.c
>>>> +++ b/drivers/thermal/qcom/tsens-v2.c
>>>> @@ -6,11 +6,20 @@
>>>>      #include <linux/bitops.h>
>>>>    #include <linux/regmap.h>
>>>> +#include <linux/nvmem-consumer.h>
>>>>    #include "tsens.h"
>>>>      /* ----- SROT ------ */
>>>>    #define SROT_HW_VER_OFF    0x0000
>>>>    #define SROT_CTRL_OFF        0x0004
>>>> +#define SROT_MEASURE_PERIOD    0x0008
>>>> +#define SROT_Sn_CONVERSION    0x0060
>>>> +#define V2_SHIFT_DEFAULT    0x0003
>>>> +#define V2_SLOPE_DEFAULT    0x0cd0
>>>> +#define V2_CZERO_DEFAULT    0x016a
>>>> +#define ONE_PT_SLOPE        0x0cd0
>>>> +#define TWO_PT_SHIFTED_GAIN    921600
>>>> +#define ONE_PT_CZERO_CONST    94
>>>>      /* ----- TM ------ */
>>>>    #define TM_INT_EN_OFF            0x0004
>>>> @@ -59,6 +68,16 @@ static const struct reg_field
>>>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>>>        /* CTRL_OFF */
>>>>        [TSENS_EN]     = REG_FIELD(SROT_CTRL_OFF,    0,  0),
>>>>        [TSENS_SW_RST] = REG_FIELD(SROT_CTRL_OFF,    1,  1),
>>>> +    [SENSOR_EN]    = REG_FIELD(SROT_CTRL_OFF,    3,  18),
>>>> +    [CODE_OR_TEMP] = REG_FIELD(SROT_CTRL_OFF,    21, 21),
>>>> +
>>>> +    /* MAIN_MEASURE_PERIOD */
>>>> +    [MAIN_MEASURE_PERIOD] = REG_FIELD(SROT_MEASURE_PERIOD, 0, 7),
>>>> +
>>>> +    /* Sn Conversion */
>>>> +    REG_FIELD_FOR_EACH_SENSOR16(SHIFT, SROT_Sn_CONVERSION, 23, 24),
>>>> +    REG_FIELD_FOR_EACH_SENSOR16(SLOPE, SROT_Sn_CONVERSION, 10, 22),
>>>> +    REG_FIELD_FOR_EACH_SENSOR16(CZERO, SROT_Sn_CONVERSION, 0, 9),
>>>>          /* ----- TM ------ */
>>>>        /* INTERRUPT ENABLE */
>>>> @@ -104,6 +123,103 @@ static const struct reg_field
>>>> tsens_v2_regfields[MAX_REGFIELDS] = {
>>>>        [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0),
>>>>    };
>>>>    +static int tsens_v2_calibration(struct tsens_priv *priv)
>>>> +{
>>>> +    struct device *dev = priv->dev;
>>>> +    u32 mode, base0, base1;
>>>> +    u32 slope, czero;
>>>> +    char name[15];
>>>> +    int i, j, ret;
>>>> +
>>>> +    if (priv->num_sensors > MAX_SENSORS)
>>>> +        return -EINVAL;
>>>> +
>>>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "mode", &mode);
>>>> +    if (ret == -ENOENT)
>>>> +        dev_warn(priv->dev, "Calibration data not present in DT\n");
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    dev_dbg(priv->dev, "calibration mode is %d\n", mode);
>>>> +
>>>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "base0", &base0);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    ret = nvmem_cell_read_variable_le_u32(priv->dev, "base1", &base1);
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    /* Read offset values and allocate SHIFT, SLOPE & CZERO regmap
>>>> for enabled sensors */
>>>> +    for (i = 0; i < priv->num_sensors; i++) {
>>>> +        if (!(priv->sensors_to_en & (0x1 << i)))
>>>> +            continue;
>>>> +
>>>> +        ret = snprintf(name, sizeof(name), "s%d_offset",
>>>> priv->sensor[i].hw_id);
>>>> +        if (ret < 0)
>>>> +            return ret;
>>>> +
>>>> +        ret = nvmem_cell_read_variable_le_u32(priv->dev, name,
>>>> &priv->sensor[i].offset);
>>>> +        if (ret)
>>>> +            return ret;
>>>> +
>>>> +        for (j = SHIFT_0; j <= CZERO_0; j++) {
>>>> +            int idx = (i * 3) + j;
>>>> +
>>>> +            priv->rf[idx] = devm_regmap_field_alloc(dev,
>>>> priv->srot_map,
>>>> +                                priv->fields[idx]);
>>>> +            if (IS_ERR(priv->rf[idx]))
>>>> +                return PTR_ERR(priv->rf[idx]);
>>> I think, allocating data structures for 48 regfields, which are
>>> written just once, to be an overkill.
>> Can we change it to single field for each sensor. For example,
>> CONVERSION_0 instead of SHIFT_0, SLOPE_0 and CZERO_0? This way it will
>> be max 16 regfields.
> If you move writing of the registers to the loop, you won't need
> regfields. You can just call regmap_update_bits. The point is that you
> don't have to allocate a one-time instance.
Sure, will update in next patch.
>
>>>> +        }
>>>> +    }
>>>> +
>>>> +    /* Based on calib mode, program SHIFT, SLOPE and CZERO for
>>>> enabled sensors */
>>>> +    switch (mode) {
>>>> +    case TWO_PT_CALIB:
>>>> +        slope = (TWO_PT_SHIFTED_GAIN / (base1 - base0));
>>>> +
>>>> +        for (i = 0; i < priv->num_sensors; i++) {
>>>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>>>> +                continue;
>>>> +
>>>> +            int idx = i * 3;
>>>> +
>>>> +            czero = (base0 + priv->sensor[i].offset - ((base1 -
>>>> base0) / 3));
>>>> +            regmap_field_write(priv->rf[SHIFT_0 + idx],
>>>> V2_SHIFT_DEFAULT);
>>>> +            regmap_field_write(priv->rf[SLOPE_0 + idx], slope);
>>>> +            regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>>>> +        }
>>>> +        fallthrough;
>>>> +    case ONE_PT_CALIB2:
>>>> +        for (i = 0; i < priv->num_sensors; i++) {
>>>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>>>> +                continue;
>>>> +
>>>> +            int idx = i * 3;
>>>> +
>>>> +            czero = base0 + priv->sensor[i].offset -
>>>> ONE_PT_CZERO_CONST;
>>>> +            regmap_field_write(priv->rf[SHIFT_0 + idx],
>>>> V2_SHIFT_DEFAULT);
>>>> +            regmap_field_write(priv->rf[SLOPE_0 + idx], ONE_PT_SLOPE);
>>>> +            regmap_field_write(priv->rf[CZERO_0 + idx], czero);
>>>> +        }
>>>> +        break;
>>>> +    default:
>>>> +        dev_dbg(priv->dev, "calibrationless mode\n");
>>>> +        for (i = 0; i < priv->num_sensors; i++) {
>>>> +            if (!(priv->sensors_to_en & (0x1 << i)))
>>>> +                continue;
>>>> +
>>>> +            int idx = i * 3;
>>>> +
>>>> +            regmap_field_write(priv->rf[SHIFT_0 + idx],
>>>> V2_SHIFT_DEFAULT);
>>>> +            regmap_field_write(priv->rf[SLOPE_0 + idx],
>>>> V2_SLOPE_DEFAULT);
>>>> +            regmap_field_write(priv->rf[CZERO_0 + idx],
>>>> V2_CZERO_DEFAULT);
>>>> +        }
>>>> +    }
>>> This code iterates over the sensors field several times. Please
>>> consider extracting a function that handles all setup for a single
>>> sensor, then calling it in a loop (I should probably do the same for
>>> tsens-v0/v1 too).
>> Sure. After reading the mode0, base0 and base1 from QFPROM, we can call
>> a function in a loop to setup the calibration for each sensor.
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>    static const struct tsens_ops ops_generic_v2 = {
>>>>        .init        = init_common,
>>>>        .get_temp    = get_temp_tsens_valid,
>>>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>>>> index 98c356acfe98..169690355dad 100644
>>>> --- a/drivers/thermal/qcom/tsens.c
>>>> +++ b/drivers/thermal/qcom/tsens.c
>>>> @@ -974,7 +974,7 @@ int __init init_common(struct tsens_priv *priv)
>>>>        ret = regmap_field_read(priv->rf[TSENS_EN], &enabled);
>>>>        if (ret)
>>>>            goto err_put_device;
>>>> -    if (!enabled) {
>>>> +    if (!enabled && !priv->sensors_to_en) {
>>>>            dev_err(dev, "%s: device not enabled\n", __func__);
>>>>            ret = -ENODEV;
>>>>            goto err_put_device;
>>>> @@ -1006,6 +1006,40 @@ int __init init_common(struct tsens_priv *priv)
>>>>            goto err_put_device;
>>>>        }
>>>>    +    /* Do TSENS initialization if required */
>>>> +    if (priv->sensors_to_en) {
>>> Maybe it would be better to explicitly add VER_2_X_NO_RPM and check it
>>> here?
>> Sure, will add a separate version macro.
>>>> +        priv->rf[CODE_OR_TEMP] = devm_regmap_field_alloc(dev,
>>>> priv->srot_map,
>>>> + priv->fields[CODE_OR_TEMP]);
>>>> +        if (IS_ERR(priv->rf[CODE_OR_TEMP])) {
>>>> +            ret = PTR_ERR(priv->rf[CODE_OR_TEMP]);
>>>> +            goto err_put_device;
>>>> +        }
>>>> +
>>>> +        priv->rf[MAIN_MEASURE_PERIOD] =
>>>> +            devm_regmap_field_alloc(dev, priv->srot_map,
>>>> +                        priv->fields[MAIN_MEASURE_PERIOD]);
>>>> +        if (IS_ERR(priv->rf[MAIN_MEASURE_PERIOD])) {
>>>> +            ret = PTR_ERR(priv->rf[MAIN_MEASURE_PERIOD]);
>>>> +            goto err_put_device;
>>>> +        }
>>>> +
>>>> +        regmap_field_write(priv->rf[TSENS_SW_RST], 0x1);
>>>> +
>>>> +        /* Update measure period to 2ms */
>>>> +        regmap_field_write(priv->rf[MAIN_MEASURE_PERIOD], 0x1);
>>>> +
>>>> +        /* Enable available sensors */
>>>> +        regmap_field_write(priv->rf[SENSOR_EN], priv->sensors_to_en);
>>>> +
>>>> +        /* Real temperature format */
>>>> +        regmap_field_write(priv->rf[CODE_OR_TEMP], 0x1);
>>>> +
>>>> +        regmap_field_write(priv->rf[TSENS_SW_RST], 0x0);
>>>> +
>>>> +        /* Enable TSENS */
>>>> +        regmap_field_write(priv->rf[TSENS_EN], 0x1);
>>>> +    }
>>>> +
>>>>        /* This loop might need changes if enum regfield_ids is
>>>> reordered */
>>>>        for (j = LAST_TEMP_0; j <= UP_THRESH_15; j += 16) {
>>>>            for (i = 0; i < priv->feat->max_sensors; i++) {
>>>> @@ -1282,6 +1316,7 @@ static int tsens_probe(struct platform_device
>>>> *pdev)
>>>>          priv->dev = dev;
>>>>        priv->num_sensors = num_sensors;
>>>> +    priv->sensors_to_en = data->sensors_to_en;
>>>>        priv->ops = data->ops;
>>>>        for (i = 0;  i < priv->num_sensors; i++) {
>>>>            if (data->hw_ids)
>>>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>>>> index 2805de1c6827..f8897bc8944e 100644
>>>> --- a/drivers/thermal/qcom/tsens.h
>>>> +++ b/drivers/thermal/qcom/tsens.h
>>>> @@ -168,6 +168,58 @@ enum regfield_ids {
>>>>        TSENS_SW_RST,
>>>>        SENSOR_EN,
>>>>        CODE_OR_TEMP,
>>>> +    /* MEASURE_PERIOD */
>>>> +    MAIN_MEASURE_PERIOD,
>>>> +
>>>> +    /* Sn_CONVERSION */
>>>> +    SHIFT_0,
>>>> +    SLOPE_0,
>>>> +    CZERO_0,
>>>> +    SHIFT_1,
>>>> +    SLOPE_1,
>>>> +    CZERO_1,
>>>> +    SHIFT_2,
>>>> +    SLOPE_2,
>>>> +    CZERO_2,
>>>> +    SHIFT_3,
>>>> +    SLOPE_3,
>>>> +    CZERO_3,
>>>> +    SHIFT_4,
>>>> +    SLOPE_4,
>>>> +    CZERO_4,
>>>> +    SHIFT_5,
>>>> +    SLOPE_5,
>>>> +    CZERO_5,
>>>> +    SHIFT_6,
>>>> +    SLOPE_6,
>>>> +    CZERO_6,
>>>> +    SHIFT_7,
>>>> +    SLOPE_7,
>>>> +    CZERO_7,
>>>> +    SHIFT_8,
>>>> +    SLOPE_8,
>>>> +    CZERO_8,
>>>> +    SHIFT_9,
>>>> +    SLOPE_9,
>>>> +    CZERO_9,
>>>> +    SHIFT_10,
>>>> +    SLOPE_10,
>>>> +    CZERO_10,
>>>> +    SHIFT_11,
>>>> +    SLOPE_11,
>>>> +    CZERO_11,
>>>> +    SHIFT_12,
>>>> +    SLOPE_12,
>>>> +    CZERO_12,
>>>> +    SHIFT_13,
>>>> +    SLOPE_13,
>>>> +    CZERO_13,
>>>> +    SHIFT_14,
>>>> +    SLOPE_14,
>>>> +    CZERO_14,
>>>> +    SHIFT_15,
>>>> +    SLOPE_15,
>>>> +    CZERO_15,
>>>>          /* ----- TM ------ */
>>>>        /* TRDY */
>>>> @@ -524,6 +576,7 @@ struct tsens_features {
>>>>    /**
>>>>     * struct tsens_plat_data - tsens compile-time platform data
>>>>     * @num_sensors: Number of sensors supported by platform
>>>> + * @sensors_to_en: Sensors to be enabled. Each bit represent a sensor
>>>>     * @ops: operations the tsens instance supports
>>>>     * @hw_ids: Subset of sensors ids supported by platform, if not the
>>>> first n
>>>>     * @feat: features of the IP
>>>> @@ -531,6 +584,7 @@ struct tsens_features {
>>>>     */
>>>>    struct tsens_plat_data {
>>>>        const u32        num_sensors;
>>>> +    const u16        sensors_to_en;
>>> There is already a similar field, hw_ids. Can it be used instead?
>> Yes, it can be used. I missed to check this hw_ids. Will change the
>> num_sensors to 5 and use the hw_ids.
>>>>        const struct tsens_ops    *ops;
>>>>        unsigned int        *hw_ids;
>>>>        struct tsens_features    *feat;
>>>> @@ -551,6 +605,7 @@ struct tsens_context {
>>>>     * struct tsens_priv - private data for each instance of the tsens IP
>>>>     * @dev: pointer to struct device
>>>>     * @num_sensors: number of sensors enabled on this device
>>>> + * @sensors_to_en: sensors to be enabled. Each bit represents a sensor
>>>>     * @tm_map: pointer to TM register address space
>>>>     * @srot_map: pointer to SROT register address space
>>>>     * @tm_offset: deal with old device trees that don't address TM and
>>>> SROT
>>>> @@ -569,6 +624,7 @@ struct tsens_context {
>>>>    struct tsens_priv {
>>>>        struct device            *dev;
>>>>        u32                num_sensors;
>>>> +    u16                sensors_to_en;
>>>>        struct regmap            *tm_map;
>>>>        struct regmap            *srot_map;
>>>>        u32                tm_offset;
>> --
>> Thanks,
>> Praveenkumar
>
>

Re: [PATCH 6/6] thermal/drivers/tsens: Add IPQ5332 support

[Praveenkumar I]

On 7/10/2023 8:33 PM, Dmitry Baryshkov wrote:
> On Mon, 10 Jul 2023 at 16:47, Praveenkumar I <quic_ipkumar@quicinc.com> wrote:
>>
>> On 7/10/2023 4:54 PM, Dmitry Baryshkov wrote:
>>> On 10/07/2023 13:37, Praveenkumar I wrote:
>>>> IPQ5332 uses tsens v2.3.3 IP and it is having combined interrupt as
>>>> like IPQ8074. But as the SoCs does not have RPM, kernel needs to
>>>> take care of sensor enablement and calibration. Hence introduced
>>>> new ops and data for IPQ5332 and reused the feature_config from
>>>> IPQ8074.
>>>>
>>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>>> ---
>>>>    drivers/thermal/qcom/tsens-v2.c | 13 +++++++++++++
>>>>    drivers/thermal/qcom/tsens.c    |  3 +++
>>>>    drivers/thermal/qcom/tsens.h    |  2 +-
>>>>    3 files changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/thermal/qcom/tsens-v2.c
>>>> b/drivers/thermal/qcom/tsens-v2.c
>>>> index db48b1d95348..8b6e3876fd2c 100644
>>>> --- a/drivers/thermal/qcom/tsens-v2.c
>>>> +++ b/drivers/thermal/qcom/tsens-v2.c
>>>> @@ -237,6 +237,19 @@ struct tsens_plat_data data_ipq8074 = {
>>>>        .fields    = tsens_v2_regfields,
>>>>    };
>>>>    +static const struct tsens_ops ops_ipq5332_v2 = {
>>> Please drop v2. It is unclear if it refers to tsens being v2 or being
>>> specific to ipq5332 v2.
>> Sure, will drop v2.
>>>> +    .init        = init_common,
>>>> +    .get_temp    = get_temp_tsens_valid,
>>>> +    .calibrate    = tsens_v2_calibration,
>>>> +};
>>>> +
>>>> +struct tsens_plat_data data_ipq5332 = {
>>>> +    .sensors_to_en    = 0xF800,
>>> This doesn't seem to match the offsets that you have enabled in the DTSI.
>> In order to overcome the DT binding check failure, added all the
>> available QFPROM offsets in the DTSI. Else DT binding check failing on
>> "nvmem-cell-names".
> This is not a way to overcome issues in DT bindings. Please fix DT
> bindings instead by using alternatives, enums, etc.
Sure, will fix the DT bindings.
>
>>>> +    .ops        = &ops_ipq5332_v2,
>>>> +    .feat        = &ipq8074_feat,
>>>> +    .fields        = tsens_v2_regfields,
>>>> +};
>>>> +
>>>>    /* Kept around for backward compatibility with old msm8996.dtsi */
>>>>    struct tsens_plat_data data_8996 = {
>>>>        .num_sensors    = 13,
>>>> diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
>>>> index 169690355dad..e8ba2901cda8 100644
>>>> --- a/drivers/thermal/qcom/tsens.c
>>>> +++ b/drivers/thermal/qcom/tsens.c
>>>> @@ -1140,6 +1140,9 @@ static const struct of_device_id tsens_table[] = {
>>>>        }, {
>>>>            .compatible = "qcom,ipq8074-tsens",
>>>>            .data = &data_ipq8074,
>>>> +    }, {
>>>> +        .compatible = "qcom,ipq5332-tsens",
>>>> +        .data = &data_ipq5332,
>>>>        }, {
>>>>            .compatible = "qcom,mdm9607-tsens",
>>>>            .data = &data_9607,
>>>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>>>> index f8897bc8944e..36040f9beebc 100644
>>>> --- a/drivers/thermal/qcom/tsens.h
>>>> +++ b/drivers/thermal/qcom/tsens.h
>>>> @@ -701,6 +701,6 @@ extern struct tsens_plat_data data_8226,
>>>> data_8909, data_8916, data_8939, data_8
>>>>    extern struct tsens_plat_data data_tsens_v1, data_8976, data_8956;
>>>>      /* TSENS v2 targets */
>>>> -extern struct tsens_plat_data data_8996, data_ipq8074, data_tsens_v2;
>>>> +extern struct tsens_plat_data data_8996, data_ipq8074, data_ipq5332,
>>>> data_tsens_v2;
>>>>      #endif /* __QCOM_TSENS_H__ */
>> --
>> Thanks,
>> Praveenkumar
>
>

Re: [PATCH 3/6] dt-bindings: thermal: tsens: Add ipq5332 compatible

[Praveenkumar I]

On 7/11/2023 1:36 AM, Krzysztof Kozlowski wrote:
> On 10/07/2023 12:37, Praveenkumar I wrote:
>> IPQ5332 uses TSENS v2.3.3 with combined interrupt. RPM is not
>> available in the SoC, hence adding new compatible to have the
>> sensor enablement and calibration function.>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> ---
>>   Documentation/devicetree/bindings/thermal/qcom-tsens.yaml | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> index 8b7863c3989e..ee57713f6131 100644
>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> @@ -68,8 +68,10 @@ properties:
>>             - const: qcom,tsens-v2
>>   
>>         - description: v2 of TSENS with combined interrupt
>> -        enum:
>> -          - qcom,ipq8074-tsens
>> +        items:
> Drop items, you do not have multiple items.
Sure, will drop items.
>
>
>> +          - enum:
>> +              - qcom,ipq8074-tsens
>> +              - qcom,ipq5332-tsens
> Keep the order.
>>   
>>         - description: v2 of TSENS with combined interrupt
>>           items:
>> @@ -289,6 +291,7 @@ allOf:
>>             contains:
>>               enum:
>>                 - qcom,ipq8074-tsens
>> +              - qcom,ipq5332-tsens
> And here
>
>>       then:
>>         properties:
>>           interrupts:
>> @@ -304,6 +307,7 @@ allOf:
>>             contains:
>>               enum:
>>                 - qcom,ipq8074-tsens
>> +              - qcom,ipq5332-tsens
> And here.

Sure, will keep the order.

--
Thanks,
Praveenkumar
>
>>                 - qcom,tsens-v0_1
>>                 - qcom,tsens-v1
>>                 - qcom,tsens-v2
> Best regards,
> Krzysztof
>

Re: [PATCH 4/6] arm64: dts: qcom: ipq5332: Add tsens node

[Praveenkumar I]

On 7/11/2023 1:37 AM, Krzysztof Kozlowski wrote:
> On 10/07/2023 12:37, Praveenkumar I wrote:
>> IPQ5332 has tsens v2.3.3 peripheral. This patch adds the tsense
>> node with nvmem cells for calibration data.
>>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5332.dtsi | 113 ++++++++++++++++++++++++++
>>   1 file changed, 113 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> index 8bfc2db44624..a1e3527178c0 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> @@ -150,6 +150,91 @@ qfprom: efuse@a4000 {
>>   			reg = <0x000a4000 0x721>;
>>   			#address-cells = <1>;
>>   			#size-cells = <1>;
>> +
>> +			tsens_mode: mode@3e1 {
>> +				reg = <0x3e1 0x1>;
>> +				bits = <0 3>;
>> +			};
>> +
>> +			tsens_base0: base0@3e1 {
>> +				reg = <0x3e1 0x2>;
>> +				bits = <3 10>;
>> +			};
>> +
>> +			tsens_base1: base1@3e2 {
>> +				reg = <0x3e2 0x2>;
>> +				bits = <5 10>;
>> +			};
>> +
>> +			s0_offset: s0_offset@3e4 {
> Underscores are not allowed in node names.
Sure, will change it to hyphen.

--
Thanks,
Praveenkumar


>
> Best regards,
> Krzysztof
>

Re: [PATCH 1/6] dt-bindings: thermal: tsens: Add nvmem cells for calibration data

[Praveenkumar I]

On 7/11/2023 1:40 AM, Krzysztof Kozlowski wrote:
> On 10/07/2023 12:37, Praveenkumar I wrote:
>> Add TSENS V2 calibration nvmem cells for IPQ5332
>>
>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>> ---
>>   .../bindings/thermal/qcom-tsens.yaml          | 26 +++++++++++++++++--
>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> index 27e9e16e6455..8b7863c3989e 100644
>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>> @@ -91,7 +91,7 @@ properties:
>>       maxItems: 2
>>   
>>     nvmem-cells:
>> -    oneOf:
>> +    anyOf:
>>         - minItems: 1
>>           maxItems: 2
>>           description:
>> @@ -106,9 +106,13 @@ properties:
>>           description: |
>>             Reference to nvmem cells for the calibration mode, two calibration
>>             bases and two cells per each sensor, main and backup copies, plus use_backup cell
>> +      - maxItems: 17
>> +        description: |
>> +          V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
>> +          bases and one cell per each sensor
> I think this is already included in one of the previous entries.
> Otherwise, are you sure that all new devices will have exactly 17 entries?
Previous entries does not support TSENS version 2.X.X QFPROM. TSENS V2 
QFPROM has mode, base0, base1 and s[0-15]+_offset.
Ideally it should be like,
- minItems: 4
- maxItems: 19
But dt binding check fails in oneOf / anyOf condition. So added the 
IPQ5332 properties which is exactly 17.
>
>>   
>>     nvmem-cell-names:
>> -    oneOf:
>> +    anyOf:
>>         - minItems: 1
>>           items:
>>             - const: calib
>> @@ -205,6 +209,24 @@ properties:
>>             - const: s9_p2_backup
>>             - const: s10_p1_backup
>>             - const: s10_p2_backup
>> +      - items:
>> +          - const: mode
>> +          - const: base0
>> +          - const: base1
>> +          - const: s0_offset
>> +          - const: s3_offset
>> +          - const: s4_offset
>> +          - const: s5_offset
>> +          - const: s6_offset
>> +          - const: s7_offset
>> +          - const: s8_offset
>> +          - const: s9_offset
>> +          - const: s10_offset
>> +          - const: s11_offset
>> +          - const: s12_offset
>> +          - const: s13_offset
>> +          - const: s14_offset
>> +          - const: s15_offset
> Don't introduce new naming style. Existing uses s[0-9]+, without offset
> suffix. Why this should be different?
As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2 
QFPROM layout is different from the existing one.
I would like to add mode, base0, base1 and 16 patterns 
'^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf 
condintion.

--
Thanks,
Praveenkumar
>
>>   
>>     "#qcom,sensors":
>>       description:
> Best regards,
> Krzysztof
>

Re: [PATCH 1/6] dt-bindings: thermal: tsens: Add nvmem cells for calibration data

[Krzysztof Kozlowski]

On 11/07/2023 11:39, Praveenkumar I wrote:
> 
> On 7/11/2023 1:40 AM, Krzysztof Kozlowski wrote:
>> On 10/07/2023 12:37, Praveenkumar I wrote:
>>> Add TSENS V2 calibration nvmem cells for IPQ5332
>>>
>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>> ---
>>>   .../bindings/thermal/qcom-tsens.yaml          | 26 +++++++++++++++++--
>>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> index 27e9e16e6455..8b7863c3989e 100644
>>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>> @@ -91,7 +91,7 @@ properties:
>>>       maxItems: 2
>>>   
>>>     nvmem-cells:
>>> -    oneOf:
>>> +    anyOf:
>>>         - minItems: 1
>>>           maxItems: 2
>>>           description:
>>> @@ -106,9 +106,13 @@ properties:
>>>           description: |
>>>             Reference to nvmem cells for the calibration mode, two calibration
>>>             bases and two cells per each sensor, main and backup copies, plus use_backup cell
>>> +      - maxItems: 17
>>> +        description: |
>>> +          V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
>>> +          bases and one cell per each sensor
>> I think this is already included in one of the previous entries.
>> Otherwise, are you sure that all new devices will have exactly 17 entries?
> Previous entries does not support TSENS version 2.X.X QFPROM. TSENS V2 
> QFPROM has mode, base0, base1 and s[0-15]+_offset.
> Ideally it should be like,
> - minItems: 4
> - maxItems: 19

I see it covered:
minItems: 5
maxItems: 35

I think 17 is between 5 and 35.

> But dt binding check fails in oneOf / anyOf condition. So added the 
> IPQ5332 properties which is exactly 17.
>>
>>>   
>>>     nvmem-cell-names:
>>> -    oneOf:
>>> +    anyOf:
>>>         - minItems: 1
>>>           items:
>>>             - const: calib
>>> @@ -205,6 +209,24 @@ properties:
>>>             - const: s9_p2_backup
>>>             - const: s10_p1_backup
>>>             - const: s10_p2_backup
>>> +      - items:
>>> +          - const: mode
>>> +          - const: base0
>>> +          - const: base1
>>> +          - const: s0_offset
>>> +          - const: s3_offset
>>> +          - const: s4_offset
>>> +          - const: s5_offset
>>> +          - const: s6_offset
>>> +          - const: s7_offset
>>> +          - const: s8_offset
>>> +          - const: s9_offset
>>> +          - const: s10_offset
>>> +          - const: s11_offset
>>> +          - const: s12_offset
>>> +          - const: s13_offset
>>> +          - const: s14_offset
>>> +          - const: s15_offset
>> Don't introduce new naming style. Existing uses s[0-9]+, without offset
>> suffix. Why this should be different?
> As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2 
> QFPROM layout is different from the existing one.

I know, I did not write about p1/p2.

> I would like to add mode, base0, base1 and 16 patterns 
> '^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf 
> condintion.

This does not explain why you need different style - this "offset" suffix.

Best regards,
Krzysztof

Re: [PATCH 1/6] dt-bindings: thermal: tsens: Add nvmem cells for calibration data

[Praveenkumar I]

On 7/11/2023 3:22 PM, Krzysztof Kozlowski wrote:
> On 11/07/2023 11:39, Praveenkumar I wrote:
>> On 7/11/2023 1:40 AM, Krzysztof Kozlowski wrote:
>>> On 10/07/2023 12:37, Praveenkumar I wrote:
>>>> Add TSENS V2 calibration nvmem cells for IPQ5332
>>>>
>>>> Signed-off-by: Praveenkumar I <quic_ipkumar@quicinc.com>
>>>> ---
>>>>    .../bindings/thermal/qcom-tsens.yaml          | 26 +++++++++++++++++--
>>>>    1 file changed, 24 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>>> index 27e9e16e6455..8b7863c3989e 100644
>>>> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>>> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
>>>> @@ -91,7 +91,7 @@ properties:
>>>>        maxItems: 2
>>>>    
>>>>      nvmem-cells:
>>>> -    oneOf:
>>>> +    anyOf:
>>>>          - minItems: 1
>>>>            maxItems: 2
>>>>            description:
>>>> @@ -106,9 +106,13 @@ properties:
>>>>            description: |
>>>>              Reference to nvmem cells for the calibration mode, two calibration
>>>>              bases and two cells per each sensor, main and backup copies, plus use_backup cell
>>>> +      - maxItems: 17
>>>> +        description: |
>>>> +          V2 of TSENS, reference to nvmem cells for the calibration mode, two calibration
>>>> +          bases and one cell per each sensor
>>> I think this is already included in one of the previous entries.
>>> Otherwise, are you sure that all new devices will have exactly 17 entries?
>> Previous entries does not support TSENS version 2.X.X QFPROM. TSENS V2
>> QFPROM has mode, base0, base1 and s[0-15]+_offset.
>> Ideally it should be like,
>> - minItems: 4
>> - maxItems: 19
> I see it covered:
> minItems: 5
> maxItems: 35
>
> I think 17 is between 5 and 35.
Okay, will remove the nvmem-cells entry.
>
>> But dt binding check fails in oneOf / anyOf condition. So added the
>> IPQ5332 properties which is exactly 17.
>>>>    
>>>>      nvmem-cell-names:
>>>> -    oneOf:
>>>> +    anyOf:
>>>>          - minItems: 1
>>>>            items:
>>>>              - const: calib
>>>> @@ -205,6 +209,24 @@ properties:
>>>>              - const: s9_p2_backup
>>>>              - const: s10_p1_backup
>>>>              - const: s10_p2_backup
>>>> +      - items:
>>>> +          - const: mode
>>>> +          - const: base0
>>>> +          - const: base1
>>>> +          - const: s0_offset
>>>> +          - const: s3_offset
>>>> +          - const: s4_offset
>>>> +          - const: s5_offset
>>>> +          - const: s6_offset
>>>> +          - const: s7_offset
>>>> +          - const: s8_offset
>>>> +          - const: s9_offset
>>>> +          - const: s10_offset
>>>> +          - const: s11_offset
>>>> +          - const: s12_offset
>>>> +          - const: s13_offset
>>>> +          - const: s14_offset
>>>> +          - const: s15_offset
>>> Don't introduce new naming style. Existing uses s[0-9]+, without offset
>>> suffix. Why this should be different?
>> As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2
>> QFPROM layout is different from the existing one.
> I know, I did not write about p1/p2.
>
>> I would like to add mode, base0, base1 and 16 patterns
>> '^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf
>> condintion.
> This does not explain why you need different style - this "offset" suffix.
In QFPROM, the BIT field names are s0_offset, s3_offset to s15_offset. 
s1_offset and s2_offset not present in IPQ5332.
Hence used the same "offset" suffix here. May I know in this case, which 
nvmem-cells-names you are suggesting to use?

--
Thanks,
Praveenkumar
>
> Best regards,
> Krzysztof
>

Re: [PATCH 1/6] dt-bindings: thermal: tsens: Add nvmem cells for calibration data

[Krzysztof Kozlowski]

On 11/07/2023 16:13, Praveenkumar I wrote:
>>>>>              - const: calib
>>>>> @@ -205,6 +209,24 @@ properties:
>>>>>              - const: s9_p2_backup
>>>>>              - const: s10_p1_backup
>>>>>              - const: s10_p2_backup
>>>>> +      - items:
>>>>> +          - const: mode
>>>>> +          - const: base0
>>>>> +          - const: base1
>>>>> +          - const: s0_offset
>>>>> +          - const: s3_offset
>>>>> +          - const: s4_offset
>>>>> +          - const: s5_offset
>>>>> +          - const: s6_offset
>>>>> +          - const: s7_offset
>>>>> +          - const: s8_offset
>>>>> +          - const: s9_offset
>>>>> +          - const: s10_offset
>>>>> +          - const: s11_offset
>>>>> +          - const: s12_offset
>>>>> +          - const: s13_offset
>>>>> +          - const: s14_offset
>>>>> +          - const: s15_offset
>>>> Don't introduce new naming style. Existing uses s[0-9]+, without offset
>>>> suffix. Why this should be different?
>>> As I mentioned above, s[0-9]+_p1 / s[0-9]+p2 is for TSENS V1. TSENS V2
>>> QFPROM layout is different from the existing one.
>> I know, I did not write about p1/p2.
>>
>>> I would like to add mode, base0, base1 and 16 patterns
>>> '^s[0-15]+_offset$'. But DT binding check is failing in oneOf/ anyOf
>>> condintion.
>> This does not explain why you need different style - this "offset" suffix.
> In QFPROM, the BIT field names are s0_offset, s3_offset to s15_offset. 
> s1_offset and s2_offset not present in IPQ5332.
> Hence used the same "offset" suffix here. May I know in this case, which 
> nvmem-cells-names you are suggesting to use?

"Existing uses s[0-9]+"

so s[0-9]+

Best regards,
Krzysztof