[PATCH 0/9] Mediatek thermal sensor driver support for MT8186 and MT8188

[PATCH 0/9] Mediatek thermal sensor driver support for MT8186 and MT8188

[Nicolas Pitre]

This is a bunch of patches to support the MT8186 and MT8188 thermal
sensor configurations. Several changes are needed to cope with oddities
these SOCs implement.

All values (calibration data offsets, etc.) were lifted and adapted from
the vendor driver source code.

Feedback appreciated.

diffstat:

 .../thermal/mediatek,lvts-thermal.yaml        |   6 +
 arch/arm64/boot/dts/mediatek/mt8186.dtsi      | 256 ++++++++++++
 drivers/thermal/mediatek/lvts_thermal.c       | 375 ++++++++++++++----
 .../thermal/mediatek,lvts-thermal.h           |  26 ++
 4 files changed, 585 insertions(+), 78 deletions(-)

[PATCH 1/9] thermal/drivers/mediatek/lvts_thermal: retrieve all calibration bytes

[Nicolas Pitre]

From: Nicolas Pitre <npitre@baylibre.com>

Calibration values are 24-bit wide. Those values so far appear to span
only 16 bits but let's not push our luck.

Found while looking at the original Mediatek driver code.

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
---
 drivers/thermal/mediatek/lvts_thermal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index 98d9c80bd4..8aa6a8675b 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -679,7 +679,7 @@ static int lvts_calibration_init(struct device *dev, struct lvts_ctrl *lvts_ctrl
 
 	for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++)
 		memcpy(&lvts_ctrl->calibration[i],
-		       efuse_calibration + lvts_ctrl_data->cal_offset[i], 2);
+		       efuse_calibration + lvts_ctrl_data->cal_offset[i], 3);
 
 	return 0;
 }
-- 
2.42.0

[PATCH 2/9] thermal/drivers/mediatek/lvts_thermal: move comment

[Nicolas Pitre]

From: Nicolas Pitre <npitre@baylibre.com>

Move efuse data interpretation inside lvts_golden_temp_init() alongside
the actual code retrieving wanted value.

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
---
 drivers/thermal/mediatek/lvts_thermal.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index 8aa6a8675b..73ca2be0f5 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -732,11 +732,15 @@ static int lvts_calibration_read(struct device *dev, struct lvts_domain *lvts_td
 	return 0;
 }
 
-static int lvts_golden_temp_init(struct device *dev, u32 *value, int temp_offset)
+static int lvts_golden_temp_init(struct device *dev, u8 *calib, int temp_offset)
 {
 	u32 gt;
 
-	gt = (*value) >> 24;
+	/*
+	 * The golden temp information is contained in the 4th byte (index = 3)
+	 * of efuse data.
+	 */
+	gt = calib[3];
 
 	if (gt && gt < LVTS_GOLDEN_TEMP_MAX)
 		golden_temp = gt;
@@ -760,11 +764,7 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
 	if (ret)
 		return ret;
 
-	/*
-	 * The golden temp information is contained in the first chunk
-	 * of efuse data.
-	 */
-	ret = lvts_golden_temp_init(dev, (u32 *)lvts_td->calib, lvts_data->temp_offset);
+	ret = lvts_golden_temp_init(dev, lvts_td->calib, lvts_data->temp_offset);
 	if (ret)
 		return ret;
 
-- 
2.42.0

[PATCH 3/9] thermal/drivers/mediatek/lvts_thermal: use offsets for every calibration byte

[Nicolas Pitre]

From: Nicolas Pitre <npitre@baylibre.com>

Current code assumes calibration values are always stored contiguously
in host endian order. A future patch will prove this wrong.

Let's specify the offset for each calibration byte instead.

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
---
 drivers/thermal/mediatek/lvts_thermal.c | 165 ++++++++++++++----------
 1 file changed, 99 insertions(+), 66 deletions(-)

diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index 73ca2be0f5..2c346ea7c6 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -102,11 +102,11 @@ static int golden_temp_offset;
 
 struct lvts_sensor_data {
 	int dt_id;
+	u8 cal_offsets[3];
 };
 
 struct lvts_ctrl_data {
 	struct lvts_sensor_data lvts_sensor[LVTS_SENSOR_MAX];
-	int cal_offset[LVTS_SENSOR_MAX];
 	int hw_tshut_temp;
 	int num_lvts_sensor;
 	int offset;
@@ -668,8 +668,9 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl,
  * <-----ap--tc#3-----> <-----sensor#7-----> <-----sensor#8----->
  *  0x40 | 0x41 | 0x42 | 0x43 | 0x44 | 0x45 | 0x46 | 0x47 | 0x48
  *
- * The data description gives the offset of the calibration data in
- * this bytes stream for each sensor.
+ * Note: In some cases, values don't strictly follow a little endian ordering.
+ * The data description gives byte offsets constituting each calibration value
+ * for each sensor.
  */
 static int lvts_calibration_init(struct device *dev, struct lvts_ctrl *lvts_ctrl,
 					const struct lvts_ctrl_data *lvts_ctrl_data,
@@ -677,9 +678,15 @@ static int lvts_calibration_init(struct device *dev, struct lvts_ctrl *lvts_ctrl
 {
 	int i;
 
-	for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++)
-		memcpy(&lvts_ctrl->calibration[i],
-		       efuse_calibration + lvts_ctrl_data->cal_offset[i], 3);
+	for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) {
+		const struct lvts_sensor_data *sensor =
+					&lvts_ctrl_data->lvts_sensor[i];
+
+		lvts_ctrl->calibration[i] =
+			(efuse_calibration[sensor->cal_offsets[0]] << 0) +
+			(efuse_calibration[sensor->cal_offsets[1]] << 8) +
+			(efuse_calibration[sensor->cal_offsets[2]] << 16);
+	}
 
 	return 0;
 }
@@ -1300,24 +1307,30 @@ static void lvts_remove(struct platform_device *pdev)
 
 static const struct lvts_ctrl_data mt7988_lvts_ap_data_ctrl[] = {
 	{
-		.cal_offset = { 0x00, 0x04, 0x08, 0x0c },
 		.lvts_sensor = {
-			{ .dt_id = MT7988_CPU_0 },
-			{ .dt_id = MT7988_CPU_1 },
-			{ .dt_id = MT7988_ETH2P5G_0 },
-			{ .dt_id = MT7988_ETH2P5G_1 }
+			{ .dt_id = MT7988_CPU_0,
+			  .cal_offsets = { 0x00, 0x01, 0x02 } },
+			{ .dt_id = MT7988_CPU_1,
+			  .cal_offsets = { 0x04, 0x05, 0x06 } },
+			{ .dt_id = MT7988_ETH2P5G_0,
+			  .cal_offsets = { 0x08, 0x09, 0x0a } },
+			{ .dt_id = MT7988_ETH2P5G_1,
+			  .cal_offsets = { 0x0c, 0x0d, 0x0e } }
 		},
 		.num_lvts_sensor = 4,
 		.offset = 0x0,
 		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
 	},
 	{
-		.cal_offset = { 0x14, 0x18, 0x1c, 0x20 },
 		.lvts_sensor = {
-			{ .dt_id = MT7988_TOPS_0},
-			{ .dt_id = MT7988_TOPS_1},
-			{ .dt_id = MT7988_ETHWARP_0},
-			{ .dt_id = MT7988_ETHWARP_1}
+			{ .dt_id = MT7988_TOPS_0,
+			   .cal_offsets = { 0x14, 0x15, 0x16 } },
+			{ .dt_id = MT7988_TOPS_1,
+			   .cal_offsets = { 0x18, 0x19, 0x1a } },
+			{ .dt_id = MT7988_ETHWARP_0,
+			   .cal_offsets = { 0x1c, 0x1d, 0x1e } },
+			{ .dt_id = MT7988_ETHWARP_1,
+			   .cal_offsets = { 0x20, 0x21, 0x22 } }
 		},
 		.num_lvts_sensor = 4,
 		.offset = 0x100,
@@ -1359,10 +1372,11 @@ static int lvts_resume(struct device *dev)
 
 static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = {
 	{
-		.cal_offset = { 0x04, 0x08 },
 		.lvts_sensor = {
-			{ .dt_id = MT8192_MCU_BIG_CPU0 },
-			{ .dt_id = MT8192_MCU_BIG_CPU1 }
+			{ .dt_id = MT8192_MCU_BIG_CPU0,
+			  .cal_offsets = { 0x04, 0x05, 0x06 } },
+			{ .dt_id = MT8192_MCU_BIG_CPU1,
+			  .cal_offsets = { 0x08, 0x09, 0x0a } }
 		},
 		.num_lvts_sensor = 2,
 		.offset = 0x0,
@@ -1370,10 +1384,11 @@ static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = {
 		.mode = LVTS_MSR_FILTERED_MODE,
 	},
 	{
-		.cal_offset = { 0x0c, 0x10 },
 		.lvts_sensor = {
-			{ .dt_id = MT8192_MCU_BIG_CPU2 },
-			{ .dt_id = MT8192_MCU_BIG_CPU3 }
+			{ .dt_id = MT8192_MCU_BIG_CPU2,
+			  .cal_offsets = { 0x0c, 0x0d, 0x0e } },
+			{ .dt_id = MT8192_MCU_BIG_CPU3,
+			  .cal_offsets = { 0x10, 0x11, 0x12 } }
 		},
 		.num_lvts_sensor = 2,
 		.offset = 0x100,
@@ -1381,12 +1396,15 @@ static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = {
 		.mode = LVTS_MSR_FILTERED_MODE,
 	},
 	{
-		.cal_offset = { 0x14, 0x18, 0x1c, 0x20 },
 		.lvts_sensor = {
-			{ .dt_id = MT8192_MCU_LITTLE_CPU0 },
-			{ .dt_id = MT8192_MCU_LITTLE_CPU1 },
-			{ .dt_id = MT8192_MCU_LITTLE_CPU2 },
-			{ .dt_id = MT8192_MCU_LITTLE_CPU3 }
+			{ .dt_id = MT8192_MCU_LITTLE_CPU0,
+			  .cal_offsets = { 0x14, 0x15, 0x16 } },
+			{ .dt_id = MT8192_MCU_LITTLE_CPU1,
+			  .cal_offsets = { 0x18, 0x19, 0x1a } },
+			{ .dt_id = MT8192_MCU_LITTLE_CPU2,
+			  .cal_offsets = { 0x1c, 0x1d, 0x1e } },
+			{ .dt_id = MT8192_MCU_LITTLE_CPU3,
+			  .cal_offsets = { 0x20, 0x21, 0x22 } }
 		},
 		.num_lvts_sensor = 4,
 		.offset = 0x200,
@@ -1396,42 +1414,47 @@ static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = {
 };
 
 static const struct lvts_ctrl_data mt8192_lvts_ap_data_ctrl[] = {
-		{
-		.cal_offset = { 0x24, 0x28 },
+	{
 		.lvts_sensor = {
-			{ .dt_id = MT8192_AP_VPU0 },
-			{ .dt_id = MT8192_AP_VPU1 }
+			{ .dt_id = MT8192_AP_VPU0,
+			  .cal_offsets = { 0x24, 0x25, 0x26 } },
+			{ .dt_id = MT8192_AP_VPU1,
+			  .cal_offsets = { 0x28, 0x29, 0x2a } }
 		},
 		.num_lvts_sensor = 2,
 		.offset = 0x0,
 		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192,
 	},
 	{
-		.cal_offset = { 0x2c, 0x30 },
 		.lvts_sensor = {
-			{ .dt_id = MT8192_AP_GPU0 },
-			{ .dt_id = MT8192_AP_GPU1 }
+			{ .dt_id = MT8192_AP_GPU0,
+			  .cal_offsets = { 0x2c, 0x2d, 0x2e } },
+			{ .dt_id = MT8192_AP_GPU1,
+			  .cal_offsets = { 0x30, 0x31, 0x32 } }
 		},
 		.num_lvts_sensor = 2,
 		.offset = 0x100,
 		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192,
 	},
 	{
-		.cal_offset = { 0x34, 0x38 },
 		.lvts_sensor = {
-			{ .dt_id = MT8192_AP_INFRA },
-			{ .dt_id = MT8192_AP_CAM },
+			{ .dt_id = MT8192_AP_INFRA,
+			  .cal_offsets = { 0x34, 0x35, 0x36 } },
+			{ .dt_id = MT8192_AP_CAM,
+			  .cal_offsets = { 0x38, 0x39, 0x3a } },
 		},
 		.num_lvts_sensor = 2,
 		.offset = 0x200,
 		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8192,
 	},
 	{
-		.cal_offset = { 0x3c, 0x40, 0x44 },
 		.lvts_sensor = {
-			{ .dt_id = MT8192_AP_MD0 },
-			{ .dt_id = MT8192_AP_MD1 },
-			{ .dt_id = MT8192_AP_MD2 }
+			{ .dt_id = MT8192_AP_MD0,
+			  .cal_offsets = { 0x3c, 0x3d, 0x3e } },
+			{ .dt_id = MT8192_AP_MD1,
+			  .cal_offsets = { 0x40, 0x41, 0x42 } },
+			{ .dt_id = MT8192_AP_MD2,
+			  .cal_offsets = { 0x44, 0x45, 0x46 } }
 		},
 		.num_lvts_sensor = 3,
 		.offset = 0x300,
@@ -1441,32 +1464,37 @@ static const struct lvts_ctrl_data mt8192_lvts_ap_data_ctrl[] = {
 
 static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = {
 	{
-		.cal_offset = { 0x04, 0x07 },
 		.lvts_sensor = {
-			{ .dt_id = MT8195_MCU_BIG_CPU0 },
-			{ .dt_id = MT8195_MCU_BIG_CPU1 }
+			{ .dt_id = MT8195_MCU_BIG_CPU0,
+			  .cal_offsets = { 0x04, 0x05, 0x06 } },
+			{ .dt_id = MT8195_MCU_BIG_CPU1,
+			  .cal_offsets = { 0x07, 0x08, 0x09 } }
 		},
 		.num_lvts_sensor = 2,
 		.offset = 0x0,
 		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195,
 	},
 	{
-		.cal_offset = { 0x0d, 0x10 },
 		.lvts_sensor = {
-			{ .dt_id = MT8195_MCU_BIG_CPU2 },
-			{ .dt_id = MT8195_MCU_BIG_CPU3 }
+			{ .dt_id = MT8195_MCU_BIG_CPU2,
+			  .cal_offsets = { 0x0d, 0x0e, 0x0f } },
+			{ .dt_id = MT8195_MCU_BIG_CPU3,
+			  .cal_offsets = { 0x10, 0x11, 0x12 } }
 		},
 		.num_lvts_sensor = 2,
 		.offset = 0x100,
 		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195,
 	},
 	{
-		.cal_offset = { 0x16, 0x19, 0x1c, 0x1f },
 		.lvts_sensor = {
-			{ .dt_id = MT8195_MCU_LITTLE_CPU0 },
-			{ .dt_id = MT8195_MCU_LITTLE_CPU1 },
-			{ .dt_id = MT8195_MCU_LITTLE_CPU2 },
-			{ .dt_id = MT8195_MCU_LITTLE_CPU3 }
+			{ .dt_id = MT8195_MCU_LITTLE_CPU0,
+			  .cal_offsets = { 0x16, 0x17, 0x18 } },
+			{ .dt_id = MT8195_MCU_LITTLE_CPU1,
+			  .cal_offsets = { 0x19, 0x1a, 0x1b } },
+			{ .dt_id = MT8195_MCU_LITTLE_CPU2,
+			  .cal_offsets = { 0x1c, 0x1d, 0x1e } },
+			{ .dt_id = MT8195_MCU_LITTLE_CPU3,
+			  .cal_offsets = { 0x1f, 0x20, 0x21 } }
 		},
 		.num_lvts_sensor = 4,
 		.offset = 0x200,
@@ -1475,42 +1503,47 @@ static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = {
 };
 
 static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = {
-		{
-		.cal_offset = { 0x25, 0x28 },
+	{
 		.lvts_sensor = {
-			{ .dt_id = MT8195_AP_VPU0 },
-			{ .dt_id = MT8195_AP_VPU1 }
+			{ .dt_id = MT8195_AP_VPU0,
+			  .cal_offsets = { 0x25, 0x26, 0x27 } },
+			{ .dt_id = MT8195_AP_VPU1,
+			  .cal_offsets = { 0x28, 0x29, 0x2a } }
 		},
 		.num_lvts_sensor = 2,
 		.offset = 0x0,
 		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195,
 	},
 	{
-		.cal_offset = { 0x2e, 0x31 },
 		.lvts_sensor = {
-			{ .dt_id = MT8195_AP_GPU0 },
-			{ .dt_id = MT8195_AP_GPU1 }
+			{ .dt_id = MT8195_AP_GPU0,
+			  .cal_offsets = { 0x2e, 0x2f, 0x30 } },
+			{ .dt_id = MT8195_AP_GPU1,
+			  .cal_offsets = { 0x31, 0x32, 0x33 } }
 		},
 		.num_lvts_sensor = 2,
 		.offset = 0x100,
 		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195,
 	},
 	{
-		.cal_offset = { 0x37, 0x3a, 0x3d },
 		.lvts_sensor = {
-			{ .dt_id = MT8195_AP_VDEC },
-			{ .dt_id = MT8195_AP_IMG },
-			{ .dt_id = MT8195_AP_INFRA },
+			{ .dt_id = MT8195_AP_VDEC,
+			  .cal_offsets = { 0x37, 0x38, 0x39 } },
+			{ .dt_id = MT8195_AP_IMG,
+			  .cal_offsets = { 0x3a, 0x3b, 0x3c } },
+			{ .dt_id = MT8195_AP_INFRA,
+			  .cal_offsets = { 0x3d, 0x3e, 0x3f } }
 		},
 		.num_lvts_sensor = 3,
 		.offset = 0x200,
 		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195,
 	},
 	{
-		.cal_offset = { 0x43, 0x46 },
 		.lvts_sensor = {
-			{ .dt_id = MT8195_AP_CAM0 },
-			{ .dt_id = MT8195_AP_CAM1 }
+			{ .dt_id = MT8195_AP_CAM0,
+			  .cal_offsets = { 0x43, 0x44, 0x45 } },
+			{ .dt_id = MT8195_AP_CAM1,
+			  .cal_offsets = { 0x46, 0x47, 0x48 } }
 		},
 		.num_lvts_sensor = 2,
 		.offset = 0x300,
-- 
2.42.0

[PATCH 4/9] thermal/drivers/mediatek/lvts_thermal: guard against efuse data buffer overflow

[Nicolas Pitre]

From: Nicolas Pitre <npitre@baylibre.com>

We don't want to silently fetch garbage past the actual buffer.

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
---
 drivers/thermal/mediatek/lvts_thermal.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index 2c346ea7c6..ed1888fb24 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -674,7 +674,8 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl,
  */
 static int lvts_calibration_init(struct device *dev, struct lvts_ctrl *lvts_ctrl,
 					const struct lvts_ctrl_data *lvts_ctrl_data,
-					u8 *efuse_calibration)
+					u8 *efuse_calibration,
+					size_t calib_len)
 {
 	int i;
 
@@ -682,6 +683,11 @@ static int lvts_calibration_init(struct device *dev, struct lvts_ctrl *lvts_ctrl
 		const struct lvts_sensor_data *sensor =
 					&lvts_ctrl_data->lvts_sensor[i];
 
+		if (sensor->cal_offsets[0] >= calib_len ||
+		    sensor->cal_offsets[1] >= calib_len ||
+		    sensor->cal_offsets[2] >= calib_len)
+			return -EINVAL;
+
 		lvts_ctrl->calibration[i] =
 			(efuse_calibration[sensor->cal_offsets[0]] << 0) +
 			(efuse_calibration[sensor->cal_offsets[1]] << 8) +
@@ -791,7 +797,8 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
 
 		ret = lvts_calibration_init(dev, &lvts_ctrl[i],
 					    &lvts_data->lvts_ctrl[i],
-					    lvts_td->calib);
+					    lvts_td->calib,
+					    lvts_td->calib_len);
 		if (ret)
 			return ret;
 
-- 
2.42.0

[PATCH 5/9] thermal/drivers/mediatek/lvts_thermal: add MT8186 support

[Nicolas Pitre]

From: Nicolas Pitre <npitre@baylibre.com>

Various values extracted from the vendor's kernel driver.

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
---
 .../thermal/mediatek,lvts-thermal.yaml        |  2 +
 arch/arm64/boot/dts/mediatek/mt8186.dtsi      | 20 ++++++
 drivers/thermal/mediatek/lvts_thermal.c       | 67 +++++++++++++++++++
 .../thermal/mediatek,lvts-thermal.h           | 10 +++
 4 files changed, 99 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
index e6665af52e..4173bae530 100644
--- a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
+++ b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
@@ -19,6 +19,7 @@ properties:
   compatible:
     enum:
       - mediatek,mt7988-lvts-ap
+      - mediatek,mt8186-lvts
       - mediatek,mt8192-lvts-ap
       - mediatek,mt8192-lvts-mcu
       - mediatek,mt8195-lvts-ap
@@ -75,6 +76,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - mediatek,mt8186-lvts
               - mediatek,mt8195-lvts-ap
               - mediatek,mt8195-lvts-mcu
     then:
diff --git a/arch/arm64/boot/dts/mediatek/mt8186.dtsi b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
index df0c04f2ba..8fc563dce6 100644
--- a/arch/arm64/boot/dts/mediatek/mt8186.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
@@ -1355,6 +1355,18 @@ spi0: spi@1100a000 {
 			status = "disabled";
 		};
 
+		lvts: lvts@1100b000 {
+			compatible = "mediatek,mt8186-lvts";
+			#thermal-sensor-cells = <1>;
+			reg = <0 0x1100b000 0 0x1000>;
+			interrupts = <GIC_SPI 99 IRQ_TYPE_LEVEL_HIGH 0>;
+			clocks = <&infracfg_ao CLK_INFRA_AO_THERM>;
+			clock-names = "lvts_clk";
+			resets = <&infracfg_ao MT8186_INFRA_THERMAL_CTRL_RST>;
+			nvmem-cells = <&lvts_e_data1 &lvts_e_data2>;
+			nvmem-cell-names = "e_data1","e_data2";
+		};
+
 		pwm0: pwm@1100e000 {
 			compatible = "mediatek,mt8186-disp-pwm", "mediatek,mt8183-disp-pwm";
 			reg = <0 0x1100e000 0 0x1000>;
@@ -1668,6 +1680,14 @@ efuse: efuse@11cb0000 {
 			#address-cells = <1>;
 			#size-cells = <1>;
 
+			lvts_e_data1: data1 {
+				reg = <0x1cc 0x14>;
+			};
+
+			lvts_e_data2: data1-1 {
+				reg = <0x2f8 0x14>;
+			};
+
 			gpu_speedbin: gpu-speedbin@59c {
 				reg = <0x59c 0x4>;
 				bits = <0 3>;
diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index ed1888fb24..e923d22c17 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -80,6 +80,8 @@
 #define LVTS_SENSOR_MAX				4
 #define LVTS_GOLDEN_TEMP_MAX		62
 #define LVTS_GOLDEN_TEMP_DEFAULT	50
+#define LVTS_COEFF_A_MT8186			-204650
+#define LVTS_COEFF_B_MT8186			204650
 #define LVTS_COEFF_A_MT8195			-250460
 #define LVTS_COEFF_B_MT8195			250460
 #define LVTS_COEFF_A_MT7988			-204650
@@ -92,6 +94,7 @@
 #define LVTS_MSR_READ_WAIT_US		(LVTS_MSR_READ_TIMEOUT_US / 2)
 
 #define LVTS_HW_SHUTDOWN_MT7988		105000
+#define LVTS_HW_SHUTDOWN_MT8186		105000
 #define LVTS_HW_SHUTDOWN_MT8192		105000
 #define LVTS_HW_SHUTDOWN_MT8195		105000
 
@@ -1377,6 +1380,62 @@ static int lvts_resume(struct device *dev)
 	return 0;
 }
 
+/*
+ * The MT8186 calibration data is stored as packed 3-byte little-endian
+ * values using a weird layout that makes sense only when viewed as a 32-bit
+ * hexadecimal word dump. Let's suppose SxBy where x = sensor number and
+ * y = byte number where the LSB is y=0. We then have:
+ *
+ *   [S0B2-S0B1-S0B0-S1B2] [S1B1-S1B0-S2B2-S2B1] [S2B0-S3B2-S3B1-S3B0]
+ *
+ * However, when considering a byte stream, those appear as follows:
+ *
+ *   [S1B2] [S0B0[ [S0B1] [S0B2] [S2B1] [S2B2] [S1B0] [S1B1] [S3B0] [S3B1] [S3B2] [S2B0]
+ *
+ * Hence the rather confusing offsets provided below.
+ */
+static const struct lvts_ctrl_data mt8186_lvts_data_ctrl[] = {
+	{
+		.lvts_sensor = {
+			{ .dt_id = MT8186_TS1_0,
+			  .cal_offsets = { 5, 6, 7 } },
+			{ .dt_id = MT8186_TS1_1,
+			  .cal_offsets = { 10, 11, 4 } },
+			{ .dt_id = MT8186_TS1_2,
+			  .cal_offsets = { 15, 8, 9 } },
+			{ .dt_id = MT8186_TS1_3,
+			  .cal_offsets = { 12, 13, 14 } }
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x0,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8186,
+	},
+	{
+		.lvts_sensor = {
+			{ .dt_id = MT8186_TS2_0,
+			  .cal_offsets = { 22, 23, 16 } },
+			{ .dt_id = MT8186_TS2_1,
+			  .cal_offsets = { 27, 20, 21 } }
+		},
+		.num_lvts_sensor = 2,
+		.offset = 0x100,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8186,
+	},
+	{
+		.lvts_sensor = {
+			{ .dt_id = MT8186_TS3_0,
+			  .cal_offsets = { 29, 30, 31 } },
+			{ .dt_id = MT8186_TS3_1,
+			  .cal_offsets = { 34, 35, 28 } },
+			{ .dt_id = MT8186_TS3_2,
+			  .cal_offsets = { 39, 32, 33 } }
+		},
+		.num_lvts_sensor = 3,
+		.offset = 0x200,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8186,
+	}
+};
+
 static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = {
 	{
 		.lvts_sensor = {
@@ -1565,6 +1624,13 @@ static const struct lvts_data mt7988_lvts_ap_data = {
 	.temp_offset	= LVTS_COEFF_B_MT7988,
 };
 
+static const struct lvts_data mt8186_lvts_data = {
+	.lvts_ctrl	= mt8186_lvts_data_ctrl,
+	.num_lvts_ctrl	= ARRAY_SIZE(mt8186_lvts_data_ctrl),
+	.temp_factor	= LVTS_COEFF_A_MT8186,
+	.temp_offset	= LVTS_COEFF_B_MT8186,
+};
+
 static const struct lvts_data mt8192_lvts_mcu_data = {
 	.lvts_ctrl	= mt8192_lvts_mcu_data_ctrl,
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8192_lvts_mcu_data_ctrl),
@@ -1591,6 +1657,7 @@ static const struct lvts_data mt8195_lvts_ap_data = {
 
 static const struct of_device_id lvts_of_match[] = {
 	{ .compatible = "mediatek,mt7988-lvts-ap", .data = &mt7988_lvts_ap_data },
+	{ .compatible = "mediatek,mt8186-lvts", .data = &mt8186_lvts_data },
 	{ .compatible = "mediatek,mt8192-lvts-mcu", .data = &mt8192_lvts_mcu_data },
 	{ .compatible = "mediatek,mt8192-lvts-ap", .data = &mt8192_lvts_ap_data },
 	{ .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt8195_lvts_mcu_data },
diff --git a/include/dt-bindings/thermal/mediatek,lvts-thermal.h b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
index 997e2f5512..3197ca6087 100644
--- a/include/dt-bindings/thermal/mediatek,lvts-thermal.h
+++ b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
@@ -16,6 +16,16 @@
 #define MT7988_ETHWARP_0	6
 #define MT7988_ETHWARP_1	7
 
+#define MT8186_TS1_0		0
+#define MT8186_TS1_1		1
+#define MT8186_TS1_2		2
+#define MT8186_TS1_3		3
+#define MT8186_TS2_0		4
+#define MT8186_TS2_1		5
+#define MT8186_TS3_0		6
+#define MT8186_TS3_1		7
+#define MT8186_TS3_2		8
+
 #define MT8195_MCU_BIG_CPU0     0
 #define MT8195_MCU_BIG_CPU1     1
 #define MT8195_MCU_BIG_CPU2     2
-- 
2.42.0

[PATCH 6/9] arm64: dts: mediatek: mt8186: add default thermal zones

[Nicolas Pitre]

From: Nicolas Pitre <npitre@baylibre.com>

Inspired by the vendor kernel but adapted to the upstream thermal
driver version.

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
---
 arch/arm64/boot/dts/mediatek/mt8186.dtsi | 236 +++++++++++++++++++++++
 1 file changed, 236 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8186.dtsi b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
index 8fc563dce6..91b902a9f0 100644
--- a/arch/arm64/boot/dts/mediatek/mt8186.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
@@ -13,6 +13,8 @@
 #include <dt-bindings/power/mt8186-power.h>
 #include <dt-bindings/phy/phy.h>
 #include <dt-bindings/reset/mt8186-resets.h>
+#include <dt-bindings/thermal/thermal.h>
+#include <dt-bindings/thermal/mediatek,lvts-thermal.h>
 
 / {
 	compatible = "mediatek,mt8186";
@@ -2115,4 +2117,238 @@ larb19: smi@1c10f000 {
 			power-domains = <&spm MT8186_POWER_DOMAIN_IPE>;
 		};
 	};
+
+	thermal_zones: thermal-zones {
+		cpu_zone0-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <250>;
+			thermal-sensors = <&lvts MT8186_TS1_0>;
+
+			trips {
+				cpu_zone0_alert: trip-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cpu_zone0_crit: trip-crit {
+					temperature = <100000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map0 {
+					trip = <&cpu_zone0_alert>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+
+		cpu_zone1-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <250>;
+			thermal-sensors = <&lvts MT8186_TS1_1>;
+
+			trips {
+				cpu_zone1_alert: trip-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cpu_zone1_crit: trip-crit {
+					temperature = <100000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map0 {
+					trip = <&cpu_zone1_alert>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+
+		cpu_zone2-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <250>;
+			thermal-sensors = <&lvts MT8186_TS1_2>;
+
+			trips {
+				cpu_zone2_alert: trip-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cpu_zone2_crit: trip-crit {
+					temperature = <100000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map0 {
+					trip = <&cpu_zone2_alert>;
+					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+
+		cam-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <250>;
+			thermal-sensors = <&lvts MT8186_TS1_3>;
+
+			trips {
+				cam_alert: trip-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cam_crit: trip-crit {
+					temperature = <100000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};
+
+		nna-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <250>;
+			thermal-sensors = <&lvts MT8186_TS3_0>;
+
+			trips {
+				nna_alert: trip-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				nna_crit: trip-crit {
+					temperature = <100000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};
+
+		adsp-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <250>;
+			thermal-sensors = <&lvts MT8186_TS3_1>;
+
+			trips {
+				adsp_alert: trip-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				adsp_crit: trip-crit {
+					temperature = <100000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};
+
+		mfg-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <250>;
+			thermal-sensors = <&lvts MT8186_TS3_2>;
+
+			trips {
+				mfg_alert: trip-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				mfg_crit: trip-crit {
+					temperature = <100000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+		};
+
+		cpu_big0-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <250>;
+			thermal-sensors = <&lvts MT8186_TS2_0>;
+
+			trips {
+				cpu_big0_alert: trip-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cpu_big0_crit: trip-crit {
+					temperature = <100000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map0 {
+					trip = <&cpu_big0_alert>;
+					cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+
+		cpu_big1-thermal {
+			polling-delay = <1000>;
+			polling-delay-passive = <250>;
+			thermal-sensors = <&lvts MT8186_TS2_1>;
+
+			trips {
+				cpu_big1_alert: trip-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				cpu_big1_crit: trip-crit {
+					temperature = <100000>;
+					hysteresis = <2000>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map0 {
+					trip = <&cpu_big1_alert>;
+					cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+							 <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+	};
 };
-- 
2.42.0

[PATCH 7/9] thermal/drivers/mediatek/lvts_thermal: provision for gt variable location

[Nicolas Pitre]

From: Nicolas Pitre <npitre@baylibre.com>

The golden temperature calibration value in nvram is not always the
3rd byte. A future commit will prove this assumption wrong.

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
---
 drivers/thermal/mediatek/lvts_thermal.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index e923d22c17..b20b70fd36 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -121,6 +121,7 @@ struct lvts_data {
 	int num_lvts_ctrl;
 	int temp_factor;
 	int temp_offset;
+	int gt_calib_bit_offset;
 };
 
 struct lvts_sensor {
@@ -748,20 +749,21 @@ static int lvts_calibration_read(struct device *dev, struct lvts_domain *lvts_td
 	return 0;
 }
 
-static int lvts_golden_temp_init(struct device *dev, u8 *calib, int temp_offset)
+static int lvts_golden_temp_init(struct device *dev, u8 *calib,
+				 const struct lvts_data *lvts_data)
 {
 	u32 gt;
 
 	/*
-	 * The golden temp information is contained in the 4th byte (index = 3)
-	 * of efuse data.
+	 * The golden temp information is contained in the first 32-bit
+	 * word  of efuse data at a specific bit offset.
 	 */
-	gt = calib[3];
+	gt = (((u32 *)calib)[0] >> lvts_data->gt_calib_bit_offset) & 0xff;
 
 	if (gt && gt < LVTS_GOLDEN_TEMP_MAX)
 		golden_temp = gt;
 
-	golden_temp_offset = golden_temp * 500 + temp_offset;
+	golden_temp_offset = golden_temp * 500 + lvts_data->temp_offset;
 
 	return 0;
 }
@@ -780,7 +782,7 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
 	if (ret)
 		return ret;
 
-	ret = lvts_golden_temp_init(dev, lvts_td->calib, lvts_data->temp_offset);
+	ret = lvts_golden_temp_init(dev, lvts_td->calib, lvts_data);
 	if (ret)
 		return ret;
 
@@ -1622,6 +1624,7 @@ static const struct lvts_data mt7988_lvts_ap_data = {
 	.num_lvts_ctrl	= ARRAY_SIZE(mt7988_lvts_ap_data_ctrl),
 	.temp_factor	= LVTS_COEFF_A_MT7988,
 	.temp_offset	= LVTS_COEFF_B_MT7988,
+	.gt_calib_bit_offset = 24,
 };
 
 static const struct lvts_data mt8186_lvts_data = {
@@ -1629,16 +1632,19 @@ static const struct lvts_data mt8186_lvts_data = {
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8186_lvts_data_ctrl),
 	.temp_factor	= LVTS_COEFF_A_MT8186,
 	.temp_offset	= LVTS_COEFF_B_MT8186,
+	.gt_calib_bit_offset = 24,
 };
 
 static const struct lvts_data mt8192_lvts_mcu_data = {
 	.lvts_ctrl	= mt8192_lvts_mcu_data_ctrl,
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8192_lvts_mcu_data_ctrl),
+	.gt_calib_bit_offset = 24,
 };
 
 static const struct lvts_data mt8192_lvts_ap_data = {
 	.lvts_ctrl	= mt8192_lvts_ap_data_ctrl,
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8192_lvts_ap_data_ctrl),
+	.gt_calib_bit_offset = 24,
 };
 
 static const struct lvts_data mt8195_lvts_mcu_data = {
@@ -1646,6 +1652,7 @@ static const struct lvts_data mt8195_lvts_mcu_data = {
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8195_lvts_mcu_data_ctrl),
 	.temp_factor	= LVTS_COEFF_A_MT8195,
 	.temp_offset	= LVTS_COEFF_B_MT8195,
+	.gt_calib_bit_offset = 24,
 };
 
 static const struct lvts_data mt8195_lvts_ap_data = {
@@ -1653,6 +1660,7 @@ static const struct lvts_data mt8195_lvts_ap_data = {
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8195_lvts_ap_data_ctrl),
 	.temp_factor	= LVTS_COEFF_A_MT8195,
 	.temp_offset	= LVTS_COEFF_B_MT8195,
+	.gt_calib_bit_offset = 24,
 };
 
 static const struct of_device_id lvts_of_match[] = {
-- 
2.42.0

[PATCH 8/9] thermal/drivers/mediatek/lvts_thermal: allow early empty sensor slots

[Nicolas Pitre]

From: Nicolas Pitre <npitre@baylibre.com>

Some systems don't always populate sensor controller slots starting
at slot 0.

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
---
 drivers/thermal/mediatek/lvts_thermal.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index b20b70fd36..473ef91ea3 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -112,6 +112,7 @@ struct lvts_ctrl_data {
 	struct lvts_sensor_data lvts_sensor[LVTS_SENSOR_MAX];
 	int hw_tshut_temp;
 	int num_lvts_sensor;
+	int skipped_sensors;
 	int offset;
 	int mode;
 };
@@ -555,6 +556,7 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl,
 					const struct lvts_ctrl_data *lvts_ctrl_data)
 {
 	struct lvts_sensor *lvts_sensor = lvts_ctrl->sensors;
+
 	void __iomem *msr_regs[] = {
 		LVTS_MSR0(lvts_ctrl->base),
 		LVTS_MSR1(lvts_ctrl->base),
@@ -569,7 +571,7 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl,
 		LVTS_IMMD3(lvts_ctrl->base)
 	};
 
-	int i;
+	int i, skip;
 
 	for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) {
 
@@ -604,8 +606,9 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl,
 		/*
 		 * Each sensor has its own register address to read from.
 		 */
+		skip = lvts_ctrl_data->skipped_sensors;
 		lvts_sensor[i].msr = lvts_ctrl_data->mode == LVTS_MSR_IMMEDIATE_MODE ?
-			imm_regs[i] : msr_regs[i];
+			imm_regs[i + skip] : msr_regs[i + skip];
 
 		lvts_sensor[i].low_thresh = INT_MIN;
 		lvts_sensor[i].high_thresh = INT_MIN;
-- 
2.42.0

[PATCH 9/9] thermal/drivers/mediatek/lvts_thermal: add MT8188 support

[Nicolas Pitre]

From: Nicolas Pitre <npitre@baylibre.com>

Various values extracted from the vendor's kernel driver.

Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
---
 .../thermal/mediatek,lvts-thermal.yaml        |   4 +
 drivers/thermal/mediatek/lvts_thermal.c       | 101 ++++++++++++++++++
 .../thermal/mediatek,lvts-thermal.h           |  16 +++
 3 files changed, 121 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
index 4173bae530..331cf4e662 100644
--- a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
+++ b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
@@ -20,6 +20,8 @@ properties:
     enum:
       - mediatek,mt7988-lvts-ap
       - mediatek,mt8186-lvts
+      - mediatek,mt8188-lvts-ap
+      - mediatek,mt8188-lvts-mcu
       - mediatek,mt8192-lvts-ap
       - mediatek,mt8192-lvts-mcu
       - mediatek,mt8195-lvts-ap
@@ -61,6 +63,8 @@ allOf:
         compatible:
           contains:
             enum:
+              - mediatek,mt8188-lvts-ap
+              - mediatek,mt8188-lvts-mcu
               - mediatek,mt8192-lvts-ap
               - mediatek,mt8192-lvts-mcu
     then:
diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index 473ef91ea3..8942b50d84 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -1441,6 +1441,89 @@ static const struct lvts_ctrl_data mt8186_lvts_data_ctrl[] = {
 	}
 };
 
+static const struct lvts_ctrl_data mt8188_lvts_mcu_data_ctrl[] = {
+	{
+		.lvts_sensor = {
+			{ .dt_id = MT8188_MCU_TS1_0,
+			  .cal_offsets = { 22, 23, 24 } },
+			{ .dt_id = MT8188_MCU_TS1_1,
+			  .cal_offsets = { 25, 26, 27 } },
+			{ .dt_id = MT8188_MCU_TS1_2,
+			  .cal_offsets = { 28, 29, 30 } },
+			{ .dt_id = MT8188_MCU_TS1_3,
+			  .cal_offsets = { 31, 32, 33 } },
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x0,
+		.hw_tshut_temp = 117000,
+		.mode = LVTS_MSR_FILTERED_MODE,
+	},
+	{
+		.lvts_sensor = {
+			{ .dt_id = MT8188_MCU_TS2_0,
+			  .cal_offsets = { 34, 35, 36 } },
+			{ .dt_id = MT8188_MCU_TS2_1,
+			  .cal_offsets = { 37, 38, 39 } },
+		},
+		.num_lvts_sensor = 2,
+		.offset = 0x100,
+		.hw_tshut_temp = 117000,
+		.mode = LVTS_MSR_FILTERED_MODE,
+	}
+};
+
+static const struct lvts_ctrl_data mt8188_lvts_ap_data_ctrl[] = {
+	{
+		.lvts_sensor = {
+			{ .dt_id = MT8188_AP_TS3_1,
+			  .cal_offsets = { 40, 41, 42 } },
+		},
+		.num_lvts_sensor = 1,
+		.skipped_sensors = 1,
+		.offset = 0x0,
+		.hw_tshut_temp = 117000,
+		.mode = LVTS_MSR_FILTERED_MODE,
+	},
+	{
+		.lvts_sensor = {
+			{ .dt_id = MT8188_AP_TS4_0,
+			  .cal_offsets = { 43, 44, 45 } },
+			{ .dt_id = MT8188_AP_TS4_1,
+			  .cal_offsets = { 46, 47, 48 } },
+			{ .dt_id = MT8188_AP_TS4_2,
+			  .cal_offsets = { 49, 50, 51 } },
+		},
+		.num_lvts_sensor = 3,
+		.offset = 0x100,
+		.hw_tshut_temp = 117000,
+		.mode = LVTS_MSR_FILTERED_MODE,
+	},
+	{
+		.lvts_sensor = {
+			{ .dt_id = MT8188_AP_TS5_0,
+			  .cal_offsets = { 52, 53, 54 } },
+			{ .dt_id = MT8188_AP_TS5_1,
+			  .cal_offsets = { 55, 56, 57 } },
+		},
+		.num_lvts_sensor = 2,
+		.offset = 0x200,
+		.hw_tshut_temp = 117000,
+		.mode = LVTS_MSR_FILTERED_MODE,
+	},
+	{
+		.lvts_sensor = {
+			{ .dt_id = MT8188_AP_TS6_0,
+			  .cal_offsets = { 58, 59, 60 } },
+			{ .dt_id = MT8188_AP_TS6_1,
+			  .cal_offsets = { 61, 62, 63 } },
+		},
+		.num_lvts_sensor = 2,
+		.offset = 0x300,
+		.hw_tshut_temp = 117000,
+		.mode = LVTS_MSR_FILTERED_MODE,
+	}
+};
+
 static const struct lvts_ctrl_data mt8192_lvts_mcu_data_ctrl[] = {
 	{
 		.lvts_sensor = {
@@ -1638,6 +1721,22 @@ static const struct lvts_data mt8186_lvts_data = {
 	.gt_calib_bit_offset = 24,
 };
 
+static const struct lvts_data mt8188_lvts_mcu_data = {
+	.lvts_ctrl	= mt8188_lvts_mcu_data_ctrl,
+	.num_lvts_ctrl	= ARRAY_SIZE(mt8188_lvts_mcu_data_ctrl),
+	.temp_factor	= -250460,
+	.temp_offset	= 250460,
+	.gt_calib_bit_offset = 20,
+};
+
+static const struct lvts_data mt8188_lvts_ap_data = {
+	.lvts_ctrl	= mt8188_lvts_ap_data_ctrl,
+	.num_lvts_ctrl	= ARRAY_SIZE(mt8188_lvts_ap_data_ctrl),
+	.temp_factor	= -250460,
+	.temp_offset	= 250460,
+	.gt_calib_bit_offset = 20,
+};
+
 static const struct lvts_data mt8192_lvts_mcu_data = {
 	.lvts_ctrl	= mt8192_lvts_mcu_data_ctrl,
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8192_lvts_mcu_data_ctrl),
@@ -1669,6 +1768,8 @@ static const struct lvts_data mt8195_lvts_ap_data = {
 static const struct of_device_id lvts_of_match[] = {
 	{ .compatible = "mediatek,mt7988-lvts-ap", .data = &mt7988_lvts_ap_data },
 	{ .compatible = "mediatek,mt8186-lvts", .data = &mt8186_lvts_data },
+	{ .compatible = "mediatek,mt8188-lvts-mcu", .data = &mt8188_lvts_mcu_data },
+	{ .compatible = "mediatek,mt8188-lvts-ap", .data = &mt8188_lvts_ap_data },
 	{ .compatible = "mediatek,mt8192-lvts-mcu", .data = &mt8192_lvts_mcu_data },
 	{ .compatible = "mediatek,mt8192-lvts-ap", .data = &mt8192_lvts_ap_data },
 	{ .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt8195_lvts_mcu_data },
diff --git a/include/dt-bindings/thermal/mediatek,lvts-thermal.h b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
index 3197ca6087..04fa9d7821 100644
--- a/include/dt-bindings/thermal/mediatek,lvts-thermal.h
+++ b/include/dt-bindings/thermal/mediatek,lvts-thermal.h
@@ -26,6 +26,22 @@
 #define MT8186_TS3_1		7
 #define MT8186_TS3_2		8
 
+#define MT8188_MCU_TS1_0	0
+#define MT8188_MCU_TS1_1	1
+#define MT8188_MCU_TS1_2	2
+#define MT8188_MCU_TS1_3	3
+#define MT8188_MCU_TS2_0	4
+#define MT8188_MCU_TS2_1	5
+
+#define MT8188_AP_TS3_1		0
+#define MT8188_AP_TS4_0		1
+#define MT8188_AP_TS4_1		2
+#define MT8188_AP_TS4_2		3
+#define MT8188_AP_TS5_0		4
+#define MT8188_AP_TS5_1		5
+#define MT8188_AP_TS6_0		6
+#define MT8188_AP_TS6_1		7
+
 #define MT8195_MCU_BIG_CPU0     0
 #define MT8195_MCU_BIG_CPU1     1
 #define MT8195_MCU_BIG_CPU2     2
-- 
2.42.0

Re: [PATCH 6/9] arm64: dts: mediatek: mt8186: add default thermal zones

[Krzysztof Kozlowski]

On 11/01/2024 23:30, Nicolas Pitre wrote:
> From: Nicolas Pitre <npitre@baylibre.com>
> 
> Inspired by the vendor kernel but adapted to the upstream thermal
> driver version.

DTS should not go via PM tree, so you need to Cc Mediatek SoC
maintainers. b4 would do this automatically. Other way is using
get_maintainers.pl.


> 
> Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
> ---
>  arch/arm64/boot/dts/mediatek/mt8186.dtsi | 236 +++++++++++++++++++++++
>  1 file changed, 236 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8186.dtsi b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
> index 8fc563dce6..91b902a9f0 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8186.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
> @@ -13,6 +13,8 @@
>  #include <dt-bindings/power/mt8186-power.h>
>  #include <dt-bindings/phy/phy.h>
>  #include <dt-bindings/reset/mt8186-resets.h>
> +#include <dt-bindings/thermal/thermal.h>
> +#include <dt-bindings/thermal/mediatek,lvts-thermal.h>
>  
>  / {
>  	compatible = "mediatek,mt8186";
> @@ -2115,4 +2117,238 @@ larb19: smi@1c10f000 {
>  			power-domains = <&spm MT8186_POWER_DOMAIN_IPE>;
>  		};
>  	};
> +
> +	thermal_zones: thermal-zones {
> +		cpu_zone0-thermal {

No underscores in node names. Could one CPU have multiple names? If not,
then it is just "cpu0-thermal".

Best regards,
Krzysztof

Re: [PATCH 6/9] arm64: dts: mediatek: mt8186: add default thermal zones

[Nicolas Pitre]

On Fri, 12 Jan 2024, Krzysztof Kozlowski wrote:

> On 11/01/2024 23:30, Nicolas Pitre wrote:
> > From: Nicolas Pitre <npitre@baylibre.com>
> > 
> > Inspired by the vendor kernel but adapted to the upstream thermal
> > driver version.
> 
> DTS should not go via PM tree, so you need to Cc Mediatek SoC
> maintainers. b4 would do this automatically. Other way is using
> get_maintainers.pl.

Sure. But I'd prefer to have reviews making sure those patches are sane 
enough before spamming more people with this.


Nicolas

Re: [PATCH 6/9] arm64: dts: mediatek: mt8186: add default thermal zones

[Nicolas Pitre]

On Fri, 12 Jan 2024, Krzysztof Kozlowski wrote:

> On 11/01/2024 23:30, Nicolas Pitre wrote:
> 
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8186.dtsi b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
> > index 8fc563dce6..91b902a9f0 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8186.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
> > @@ -2115,4 +2117,238 @@ larb19: smi@1c10f000 {
> >  			power-domains = <&spm MT8186_POWER_DOMAIN_IPE>;
> >  		};
> >  	};
> > +
> > +	thermal_zones: thermal-zones {
> > +		cpu_zone0-thermal {
> 
> No underscores in node names. Could one CPU have multiple names? If not,
> then it is just "cpu0-thermal".

Well... I'm not completely clear about this given the available info, 
but this thermal zone would not be matching a single CPU but a few of 
them, hence several "zones of CPUs".


Nicolas

Re: [PATCH 6/9] arm64: dts: mediatek: mt8186: add default thermal zones

[Krzysztof Kozlowski]

On 15/01/2024 18:46, Nicolas Pitre wrote:
>>> @@ -2115,4 +2117,238 @@ larb19: smi@1c10f000 {
>>>  			power-domains = <&spm MT8186_POWER_DOMAIN_IPE>;
>>>  		};
>>>  	};
>>> +
>>> +	thermal_zones: thermal-zones {
>>> +		cpu_zone0-thermal {
>>
>> No underscores in node names. Could one CPU have multiple names? If not,
>> then it is just "cpu0-thermal".
> 
> Well... I'm not completely clear about this given the available info, 
> but this thermal zone would not be matching a single CPU but a few of 
> them, hence several "zones of CPUs".

OK, then just: cpu-zone0-thermal

Best regards,
Krzysztof

Re: [PATCH 8/9] thermal/drivers/mediatek/lvts_thermal: allow early empty sensor slots

[Daniel Lezcano]

Hi Nico,

On 11/01/2024 23:30, Nicolas Pitre wrote:
> From: Nicolas Pitre <npitre@baylibre.com>
> 
> Some systems don't always populate sensor controller slots starting
> at slot 0.
> 
> Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
> ---
>   drivers/thermal/mediatek/lvts_thermal.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> index b20b70fd36..473ef91ea3 100644
> --- a/drivers/thermal/mediatek/lvts_thermal.c
> +++ b/drivers/thermal/mediatek/lvts_thermal.c
> @@ -112,6 +112,7 @@ struct lvts_ctrl_data {
>   	struct lvts_sensor_data lvts_sensor[LVTS_SENSOR_MAX];
>   	int hw_tshut_temp;
>   	int num_lvts_sensor;
> +	int skipped_sensors;
>   	int offset;
>   	int mode;
>   };
> @@ -555,6 +556,7 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl,
>   					const struct lvts_ctrl_data *lvts_ctrl_data)
>   {
>   	struct lvts_sensor *lvts_sensor = lvts_ctrl->sensors;
> +
>   	void __iomem *msr_regs[] = {
>   		LVTS_MSR0(lvts_ctrl->base),
>   		LVTS_MSR1(lvts_ctrl->base),
> @@ -569,7 +571,7 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl,
>   		LVTS_IMMD3(lvts_ctrl->base)
>   	};
>   
> -	int i;
> +	int i, skip;
>   
>   	for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) {
>   
> @@ -604,8 +606,9 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl,
>   		/*
>   		 * Each sensor has its own register address to read from.
>   		 */
> +		skip = lvts_ctrl_data->skipped_sensors;
>   		lvts_sensor[i].msr = lvts_ctrl_data->mode == LVTS_MSR_IMMEDIATE_MODE ?
> -			imm_regs[i] : msr_regs[i];
> +			imm_regs[i + skip] : msr_regs[i + skip];

Overall the series look ok but this changes is hard to understand.

Could you propose a different approach to have the resulting code easier 
to understand ?


>   		lvts_sensor[i].low_thresh = INT_MIN;
>   		lvts_sensor[i].high_thresh = INT_MIN;

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 8/9] thermal/drivers/mediatek/lvts_thermal: allow early empty sensor slots

[Nicolas Pitre]

On Fri, 19 Jan 2024, Daniel Lezcano wrote:

> 
> Hi Nico,
> 
> On 11/01/2024 23:30, Nicolas Pitre wrote:
> > From: Nicolas Pitre <npitre@baylibre.com>
> > 
> > Some systems don't always populate sensor controller slots starting
> > at slot 0.
> > 
> > Signed-off-by: Nicolas Pitre <npitre@baylibre.com>
> > ---
> >   drivers/thermal/mediatek/lvts_thermal.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/thermal/mediatek/lvts_thermal.c
> > b/drivers/thermal/mediatek/lvts_thermal.c
> > index b20b70fd36..473ef91ea3 100644
> > --- a/drivers/thermal/mediatek/lvts_thermal.c
> > +++ b/drivers/thermal/mediatek/lvts_thermal.c
> > @@ -112,6 +112,7 @@ struct lvts_ctrl_data {
> >    struct lvts_sensor_data lvts_sensor[LVTS_SENSOR_MAX];
> >    int hw_tshut_temp;
> >    int num_lvts_sensor;
> > +	int skipped_sensors;
> >    int offset;
> >    int mode;
> >   };
> > @@ -555,6 +556,7 @@ static int lvts_sensor_init(struct device *dev, struct
> > @@ lvts_ctrl *lvts_ctrl,
> >   					const struct lvts_ctrl_data
> >   *lvts_ctrl_data)
> >   {
> >   	struct lvts_sensor *lvts_sensor = lvts_ctrl->sensors;
> > +
> >    void __iomem *msr_regs[] = {
> >     LVTS_MSR0(lvts_ctrl->base),
> >     LVTS_MSR1(lvts_ctrl->base),
> > @@ -569,7 +571,7 @@ static int lvts_sensor_init(struct device *dev, struct
> > @@ lvts_ctrl *lvts_ctrl,
> >    	LVTS_IMMD3(lvts_ctrl->base)
> >    };
> >   -	int i;
> > +	int i, skip;
> >   
> >    for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) {
> >   
> > @@ -604,8 +606,9 @@ static int lvts_sensor_init(struct device *dev, struct
> > @@ lvts_ctrl *lvts_ctrl,
> >     /*
> >      * Each sensor has its own register address to read from.
> >      */
> > +		skip = lvts_ctrl_data->skipped_sensors;
> >   		lvts_sensor[i].msr = lvts_ctrl_data->mode ==
> > LVTS_MSR_IMMEDIATE_MODE ?
> > -			imm_regs[i] : msr_regs[i];
> > +			imm_regs[i + skip] : msr_regs[i + skip];
> 
> Overall the series look ok but this changes is hard to understand.
> 
> Could you propose a different approach to have the resulting code easier to
> understand ?

I'm not sure how I could make it simpler. Maybe a comment is in order 
though?

The sensor controller has 4 slots. Those slots are accessible either 
through imm_regs[<slot_number>] oe msr_regs[<slot_number>].  If, say, 
slot 0 is unpopulated then sensor 0 (i = 0) needs to address slot 1 (i = 
0, skip = 1), sensor 1 is in slot 2 (i = 1, skip = 1), etc. Does this 
make sense?


Nicolas

Re: [PATCH 6/9] arm64: dts: mediatek: mt8186: add default thermal zones

[Daniel Lezcano]

On 15/01/2024 18:52, Krzysztof Kozlowski wrote:
> On 15/01/2024 18:46, Nicolas Pitre wrote:
>>>> @@ -2115,4 +2117,238 @@ larb19: smi@1c10f000 {
>>>>   			power-domains = <&spm MT8186_POWER_DOMAIN_IPE>;
>>>>   		};
>>>>   	};
>>>> +
>>>> +	thermal_zones: thermal-zones {
>>>> +		cpu_zone0-thermal {
>>>
>>> No underscores in node names. Could one CPU have multiple names? If not,
>>> then it is just "cpu0-thermal".
>>
>> Well... I'm not completely clear about this given the available info,
>> but this thermal zone would not be matching a single CPU but a few of
>> them, hence several "zones of CPUs".
> 
> OK, then just: cpu-zone0-thermal

Given the node name is used as the thermal zone name. The 'zone' is 
duplicate in the namespace.

/sys/class/thermal/thermal_zone0/type = cpu-zone0-thermal

Furthermore, if other thermal zones are registered before, that could be:

/sys/class/thermal/thermal_zone4/type = cpu-zone0-thermal

Kind of inconsistent :/

If it is a group of CPUS, they are probably belonging to the same 
performance domain, may be we can use the name 'cluster' here:

	cluster0-thermal



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 8/9] thermal/drivers/mediatek/lvts_thermal: allow early empty sensor slots

[Daniel Lezcano]

Hi Nico,

On 19/01/2024 17:53, Nicolas Pitre wrote:

[ ... ]

>>> +		skip = lvts_ctrl_data->skipped_sensors;
>>>    		lvts_sensor[i].msr = lvts_ctrl_data->mode ==
>>> LVTS_MSR_IMMEDIATE_MODE ?
>>> -			imm_regs[i] : msr_regs[i];
>>> +			imm_regs[i + skip] : msr_regs[i + skip];
>>
>> Overall the series look ok but this changes is hard to understand.
>>
>> Could you propose a different approach to have the resulting code easier to
>> understand ?
> 
> I'm not sure how I could make it simpler. Maybe a comment is in order
> though?
> 
> The sensor controller has 4 slots. Those slots are accessible either
> through imm_regs[<slot_number>] oe msr_regs[<slot_number>].  If, say,
> slot 0 is unpopulated then sensor 0 (i = 0) needs to address slot 1 (i =
> 0, skip = 1), sensor 1 is in slot 2 (i = 1, skip = 1), etc. Does this
> make sense?

Why not keep the sensor id = slot id and declare the ones which are 
disabled with a mask?



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 8/9] thermal/drivers/mediatek/lvts_thermal: allow early empty sensor slots

[Nicolas Pitre]

On Mon, 22 Jan 2024, Daniel Lezcano wrote:

> 
> Hi Nico,
> 
> On 19/01/2024 17:53, Nicolas Pitre wrote:
> 
> [ ... ]
> 
> >>> +		skip = lvts_ctrl_data->skipped_sensors;
> >>>    		lvts_sensor[i].msr = lvts_ctrl_data->mode ==
> >>> LVTS_MSR_IMMEDIATE_MODE ?
> >>> -			imm_regs[i] : msr_regs[i];
> >>> +			imm_regs[i + skip] : msr_regs[i + skip];
> >>
> >> Overall the series look ok but this changes is hard to understand.
> >>
> >> Could you propose a different approach to have the resulting code easier to
> >> understand ?
> > 
> > I'm not sure how I could make it simpler. Maybe a comment is in order
> > though?
> > 
> > The sensor controller has 4 slots. Those slots are accessible either
> > through imm_regs[<slot_number>] oe msr_regs[<slot_number>].  If, say,
> > slot 0 is unpopulated then sensor 0 (i = 0) needs to address slot 1 (i =
> > 0, skip = 1), sensor 1 is in slot 2 (i = 1, skip = 1), etc. Does this
> > make sense?
> 
> Why not keep the sensor id = slot id and declare the ones which are disabled
> with a mask?

Then what do you do with the empty sensor 0? Do we want to present dead 
sensor IDs to users?


Nicolas

Re: [PATCH 8/9] thermal/drivers/mediatek/lvts_thermal: allow early empty sensor slots

[Daniel Lezcano]

On 22/01/2024 16:23, Nicolas Pitre wrote:
> On Mon, 22 Jan 2024, Daniel Lezcano wrote:
> 
>>
>> Hi Nico,
>>
>> On 19/01/2024 17:53, Nicolas Pitre wrote:
>>
>> [ ... ]
>>
>>>>> +		skip = lvts_ctrl_data->skipped_sensors;
>>>>>     		lvts_sensor[i].msr = lvts_ctrl_data->mode ==
>>>>> LVTS_MSR_IMMEDIATE_MODE ?
>>>>> -			imm_regs[i] : msr_regs[i];
>>>>> +			imm_regs[i + skip] : msr_regs[i + skip];
>>>>
>>>> Overall the series look ok but this changes is hard to understand.
>>>>
>>>> Could you propose a different approach to have the resulting code easier to
>>>> understand ?
>>>
>>> I'm not sure how I could make it simpler. Maybe a comment is in order
>>> though?
>>>
>>> The sensor controller has 4 slots. Those slots are accessible either
>>> through imm_regs[<slot_number>] oe msr_regs[<slot_number>].  If, say,
>>> slot 0 is unpopulated then sensor 0 (i = 0) needs to address slot 1 (i =
>>> 0, skip = 1), sensor 1 is in slot 2 (i = 1, skip = 1), etc. Does this
>>> make sense?
>>
>> Why not keep the sensor id = slot id and declare the ones which are disabled
>> with a mask?
> 
> Then what do you do with the empty sensor 0? Do we want to present dead
> sensor IDs to users?

You can skip it somehow in lvts_ctrl_start(), right?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 8/9] thermal/drivers/mediatek/lvts_thermal: allow early empty sensor slots

[Nicolas Pitre]

On Mon, 22 Jan 2024, Daniel Lezcano wrote:

> On 22/01/2024 16:23, Nicolas Pitre wrote:
> > On Mon, 22 Jan 2024, Daniel Lezcano wrote:
> > 
> >>
> >> Hi Nico,
> >>
> >> On 19/01/2024 17:53, Nicolas Pitre wrote:
> >>
> >> [ ... ]
> >>
> >>>>> +		skip = lvts_ctrl_data->skipped_sensors;
> >>>>>     		lvts_sensor[i].msr = lvts_ctrl_data->mode ==
> >>>>> LVTS_MSR_IMMEDIATE_MODE ?
> >>>>> -			imm_regs[i] : msr_regs[i];
> >>>>> +			imm_regs[i + skip] : msr_regs[i + skip];
> >>>>
> >>>> Overall the series look ok but this changes is hard to understand.
> >>>>
> >>>> Could you propose a different approach to have the resulting code easier
> >>>> to
> >>>> understand ?
> >>>
> >>> I'm not sure how I could make it simpler. Maybe a comment is in order
> >>> though?
> >>>
> >>> The sensor controller has 4 slots. Those slots are accessible either
> >>> through imm_regs[<slot_number>] oe msr_regs[<slot_number>].  If, say,
> >>> slot 0 is unpopulated then sensor 0 (i = 0) needs to address slot 1 (i =
> >>> 0, skip = 1), sensor 1 is in slot 2 (i = 1, skip = 1), etc. Does this
> >>> make sense?
> >>
> >> Why not keep the sensor id = slot id and declare the ones which are
> >> disabled
> >> with a mask?
> > 
> > Then what do you do with the empty sensor 0? Do we want to present dead
> > sensor IDs to users?
> 
> You can skip it somehow in lvts_ctrl_start(), right?

I see what you mean. Indeed that would be better.


Nicolas