[RFC v1 0/3] add LVTS support for mt7988

[RFC v1 0/3] add LVTS support for mt7988

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

This series makes allows soc specific temperature coefficient
and adds support for mt7988 which has a different one.

Frank Wunderlich (3):
  dt-bindings: thermal: mediatek: add mt7988 compatible
  thermal/drivers/mediatek/lvts_thermal: make coeff configurable
  thermal/drivers/mediatek/lvts_thermal: add mt7988 support

 .../thermal/mediatek,lvts-thermal.yaml        |   1 +
 drivers/thermal/mediatek/lvts_thermal.c       | 129 ++++++++++++++++--
 2 files changed, 115 insertions(+), 15 deletions(-)

-- 
2.34.1

[RFC v1 1/3] dt-bindings: thermal: mediatek: add mt7988 compatible

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

Add compatible string for mt7988.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 .../devicetree/bindings/thermal/mediatek,lvts-thermal.yaml       | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
index fe9ae4c425c0..49effe561963 100644
--- a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
+++ b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
@@ -18,6 +18,7 @@ description: |
 properties:
   compatible:
     enum:
+      - mediatek,mt7988-lvts
       - mediatek,mt8192-lvts-ap
       - mediatek,mt8192-lvts-mcu
       - mediatek,mt8195-lvts-ap
-- 
2.34.1

[RFC v1 2/3] thermal/drivers/mediatek/lvts_thermal: make coeff configurable

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

The upcoming mt7988 has different temperature coefficients so we
cannot use constants in the functions lvts_golden_temp_init,
lvts_golden_temp_init and lvts_raw_to_temp anymore.

Add a field in the lvts_ctrl pointing to the lvts_data which now
contains the soc-specific temperature coefficents.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 drivers/thermal/mediatek/lvts_thermal.c | 56 ++++++++++++++++++-------
 1 file changed, 41 insertions(+), 15 deletions(-)

diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index effd9b00a424..c1004b4da3b6 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -80,8 +80,8 @@
 #define LVTS_SENSOR_MAX				4
 #define LVTS_GOLDEN_TEMP_MAX		62
 #define LVTS_GOLDEN_TEMP_DEFAULT	50
-#define LVTS_COEFF_A				-250460
-#define LVTS_COEFF_B				250460
+#define LVTS_COEFF_A_MT8195			-250460
+#define LVTS_COEFF_B_MT8195			250460
 
 #define LVTS_MSR_IMMEDIATE_MODE		0
 #define LVTS_MSR_FILTERED_MODE		1
@@ -94,7 +94,7 @@
 #define LVTS_MINIMUM_THRESHOLD		20000
 
 static int golden_temp = LVTS_GOLDEN_TEMP_DEFAULT;
-static int coeff_b = LVTS_COEFF_B;
+static int coeff_b;
 
 struct lvts_sensor_data {
 	int dt_id;
@@ -109,9 +109,15 @@ struct lvts_ctrl_data {
 	int mode;
 };
 
+struct formula_coeff {
+	int a;
+	int b;
+};
+
 struct lvts_data {
 	const struct lvts_ctrl_data *lvts_ctrl;
 	int num_lvts_ctrl;
+	struct formula_coeff coeff;
 };
 
 struct lvts_sensor {
@@ -126,6 +132,7 @@ struct lvts_sensor {
 
 struct lvts_ctrl {
 	struct lvts_sensor sensors[LVTS_SENSOR_MAX];
+	const struct lvts_data *lvts_data;
 	u32 calibration[LVTS_SENSOR_MAX];
 	u32 hw_tshut_raw_temp;
 	int num_lvts_sensor;
@@ -247,21 +254,21 @@ static void lvts_debugfs_exit(struct lvts_domain *lvts_td) { }
 
 #endif
 
-static int lvts_raw_to_temp(u32 raw_temp)
+static int lvts_raw_to_temp(u32 raw_temp, struct formula_coeff coeff)
 {
 	int temperature;
 
-	temperature = ((s64)(raw_temp & 0xFFFF) * LVTS_COEFF_A) >> 14;
+	temperature = ((s64)(raw_temp & 0xFFFF) * coeff.a) >> 14;
 	temperature += coeff_b;
 
 	return temperature;
 }
 
-static u32 lvts_temp_to_raw(int temperature)
+static u32 lvts_temp_to_raw(int temperature, struct formula_coeff coeff)
 {
 	u32 raw_temp = ((s64)(coeff_b - temperature)) << 14;
 
-	raw_temp = div_s64(raw_temp, -LVTS_COEFF_A);
+	raw_temp = div_s64(raw_temp, -coeff.a);
 
 	return raw_temp;
 }
@@ -269,6 +276,9 @@ static u32 lvts_temp_to_raw(int temperature)
 static int lvts_get_temp(struct thermal_zone_device *tz, int *temp)
 {
 	struct lvts_sensor *lvts_sensor = thermal_zone_device_priv(tz);
+	struct lvts_ctrl *lvts_ctrl = container_of(lvts_sensor, struct lvts_ctrl,
+						   sensors[lvts_sensor->id]);
+	const struct lvts_data *lvts_data = lvts_ctrl->lvts_data;
 	void __iomem *msr = lvts_sensor->msr;
 	u32 value;
 	int rc;
@@ -301,7 +311,7 @@ static int lvts_get_temp(struct thermal_zone_device *tz, int *temp)
 	if (rc)
 		return -EAGAIN;
 
-	*temp = lvts_raw_to_temp(value & 0xFFFF);
+	*temp = lvts_raw_to_temp(value & 0xFFFF, lvts_data->coeff);
 
 	return 0;
 }
@@ -348,10 +358,13 @@ static bool lvts_should_update_thresh(struct lvts_ctrl *lvts_ctrl, int high)
 static int lvts_set_trips(struct thermal_zone_device *tz, int low, int high)
 {
 	struct lvts_sensor *lvts_sensor = thermal_zone_device_priv(tz);
-	struct lvts_ctrl *lvts_ctrl = container_of(lvts_sensor, struct lvts_ctrl, sensors[lvts_sensor->id]);
+	struct lvts_ctrl *lvts_ctrl = container_of(lvts_sensor, struct lvts_ctrl,
+						   sensors[lvts_sensor->id]);
+	const struct lvts_data *lvts_data = lvts_ctrl->lvts_data;
 	void __iomem *base = lvts_sensor->base;
-	u32 raw_low = lvts_temp_to_raw(low != -INT_MAX ? low : LVTS_MINIMUM_THRESHOLD);
-	u32 raw_high = lvts_temp_to_raw(high);
+	u32 raw_low = lvts_temp_to_raw(low != -INT_MAX ? low : LVTS_MINIMUM_THRESHOLD,
+				       lvts_data->coeff);
+	u32 raw_high = lvts_temp_to_raw(high, lvts_data->coeff);
 	bool should_update_thresh;
 
 	lvts_sensor->low_thresh = low;
@@ -692,7 +705,7 @@ 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)
+static int lvts_golden_temp_init(struct device *dev, u32 *value, struct formula_coeff coeff)
 {
 	u32 gt;
 
@@ -701,7 +714,7 @@ static int lvts_golden_temp_init(struct device *dev, u32 *value)
 	if (gt && gt < LVTS_GOLDEN_TEMP_MAX)
 		golden_temp = gt;
 
-	coeff_b = golden_temp * 500 + LVTS_COEFF_B;
+	coeff_b = golden_temp * 500 + coeff.b;
 
 	return 0;
 }
@@ -724,7 +737,7 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
 	 * The golden temp information is contained in the first chunk
 	 * of efuse data.
 	 */
-	ret = lvts_golden_temp_init(dev, (u32 *)lvts_td->calib);
+	ret = lvts_golden_temp_init(dev, (u32 *)lvts_td->calib, lvts_data->coeff);
 	if (ret)
 		return ret;
 
@@ -735,6 +748,7 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
 	for (i = 0; i < lvts_data->num_lvts_ctrl; i++) {
 
 		lvts_ctrl[i].base = lvts_td->base + lvts_data->lvts_ctrl[i].offset;
+		lvts_ctrl[i].lvts_data = lvts_data;
 
 		ret = lvts_sensor_init(dev, &lvts_ctrl[i],
 				       &lvts_data->lvts_ctrl[i]);
@@ -758,7 +772,7 @@ static int lvts_ctrl_init(struct device *dev, struct lvts_domain *lvts_td,
 		 * after initializing the calibration.
 		 */
 		lvts_ctrl[i].hw_tshut_raw_temp =
-			lvts_temp_to_raw(lvts_data->lvts_ctrl[i].hw_tshut_temp);
+			lvts_temp_to_raw(lvts_data->lvts_ctrl[i].hw_tshut_temp, lvts_data->coeff);
 
 		lvts_ctrl[i].low_thresh = INT_MIN;
 		lvts_ctrl[i].high_thresh = INT_MIN;
@@ -1223,6 +1237,8 @@ static int lvts_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
+	coeff_b = lvts_data->coeff.b;
+
 	ret = lvts_domain_init(dev, lvts_td, lvts_data);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to initialize the lvts domain\n");
@@ -1338,11 +1354,21 @@ static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = {
 static const struct lvts_data mt8195_lvts_mcu_data = {
 	.lvts_ctrl	= mt8195_lvts_mcu_data_ctrl,
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8195_lvts_mcu_data_ctrl),
+
+	.coeff = {
+		.a = LVTS_COEFF_A_MT8195,
+		.b = LVTS_COEFF_B_MT8195,
+	},
 };
 
 static const struct lvts_data mt8195_lvts_ap_data = {
 	.lvts_ctrl	= mt8195_lvts_ap_data_ctrl,
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8195_lvts_ap_data_ctrl),
+
+	.coeff = {
+		.a = LVTS_COEFF_A_MT8195,
+		.b = LVTS_COEFF_B_MT8195,
+	},
 };
 
 static const struct of_device_id lvts_of_match[] = {
-- 
2.34.1

[RFC v1 3/3] thermal/drivers/mediatek/lvts_thermal: add mt7988 support

[Frank Wunderlich]

From: Frank Wunderlich <frank-w@public-files.de>

Add Support for mediatek fologic 880/MT7988.

Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
---
 drivers/thermal/mediatek/lvts_thermal.c | 73 +++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
index c1004b4da3b6..48b257a3c80e 100644
--- a/drivers/thermal/mediatek/lvts_thermal.c
+++ b/drivers/thermal/mediatek/lvts_thermal.c
@@ -82,6 +82,8 @@
 #define LVTS_GOLDEN_TEMP_DEFAULT	50
 #define LVTS_COEFF_A_MT8195			-250460
 #define LVTS_COEFF_B_MT8195			250460
+#define LVTS_COEFF_A_MT7988			-204650
+#define LVTS_COEFF_B_MT7988			204650
 
 #define LVTS_MSR_IMMEDIATE_MODE		0
 #define LVTS_MSR_FILTERED_MODE		1
@@ -1272,6 +1274,67 @@ static int lvts_remove(struct platform_device *pdev)
 	return 0;
 }
 
+/*
+ * LVTS MT7988
+ */
+#define LVTS_HW_SHUTDOWN_MT7988	117000
+//enum mt7988_lvts_domain { MT7988_AP_DOMAIN, MT7988_NUM_DOMAIN };
+
+enum mt7988_lvts_sensor_enum {
+	MT7988_TS3_0,
+	MT7988_TS3_1,
+	MT7988_TS3_2,
+	MT7988_TS3_3,
+	MT7988_TS4_0,
+	MT7988_TS4_1,
+	MT7988_TS4_2,
+	MT7988_TS4_3,
+	MT7988_NUM_TS
+};
+
+//Efuse base : 0x11f50000
+//lvts offset in efuse : 0x918 (set in efuse dts node as calibration data)
+//offsets:
+//918 = LVTS_3_0_COUNT_R
+//91C = LVTS_3_1_COUNT_R
+//920 = LVTS_3_2_COUNT_R
+//924 = LVTS_3_3_COUNT_R
+//928 = LVTS_3_COUNT_RC
+
+//92C = LVTS_4_0_COUNT_R
+//930 = LVTS_4_1_COUNT_R
+//934 = LVTS_4_2_COUNT_R
+//938 = LVTS_4_3_COUNT_R
+//93C = LVTS_4_COUNT_RC
+
+static const struct lvts_ctrl_data mt7988_lvts_data_ctrl[] = {
+	{
+		.cal_offset = { 0x00, 0x04, 0x08, 0x0c }, //918,91C,920,924
+		.lvts_sensor = {
+			{ .dt_id = MT7988_TS3_0 },
+			{ .dt_id = MT7988_TS3_1 },
+			{ .dt_id = MT7988_TS3_2 },
+			{ .dt_id = MT7988_TS3_3 }
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x0,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
+	},
+	{
+		.cal_offset = { 0x14, 0x18, 0x1c, 0x20 }, //92C,930,934,938
+		.lvts_sensor = {
+			{ .dt_id = MT7988_TS4_0},
+			{ .dt_id = MT7988_TS4_1},
+			{ .dt_id = MT7988_TS4_2},
+			{ .dt_id = MT7988_TS4_3}
+		},
+		.num_lvts_sensor = 4,
+		.offset = 0x100,
+		.hw_tshut_temp = LVTS_HW_SHUTDOWN_MT7988,
+	}
+};
+
+//MT8195
 static const struct lvts_ctrl_data mt8195_lvts_mcu_data_ctrl[] = {
 	{
 		.cal_offset = { 0x04, 0x07 },
@@ -1351,6 +1414,15 @@ static const struct lvts_ctrl_data mt8195_lvts_ap_data_ctrl[] = {
 	}
 };
 
+static const struct lvts_data mt7988_lvts_data = {
+	.lvts_ctrl	= mt7988_lvts_data_ctrl,
+	.num_lvts_ctrl	= ARRAY_SIZE(mt7988_lvts_data_ctrl),
+	.coeff = {
+		.a = LVTS_COEFF_A_MT7988,
+		.b = LVTS_COEFF_B_MT7988,
+	},
+};
+
 static const struct lvts_data mt8195_lvts_mcu_data = {
 	.lvts_ctrl	= mt8195_lvts_mcu_data_ctrl,
 	.num_lvts_ctrl	= ARRAY_SIZE(mt8195_lvts_mcu_data_ctrl),
@@ -1372,6 +1444,7 @@ static const struct lvts_data mt8195_lvts_ap_data = {
 };
 
 static const struct of_device_id lvts_of_match[] = {
+	{ .compatible = "mediatek,mt7988-lvts", .data = &mt7988_lvts_data },
 	{ .compatible = "mediatek,mt8195-lvts-mcu", .data = &mt8195_lvts_mcu_data },
 	{ .compatible = "mediatek,mt8195-lvts-ap", .data = &mt8195_lvts_ap_data },
 	{},
-- 
2.34.1

Re: [RFC v1 1/3] dt-bindings: thermal: mediatek: add mt7988 compatible

[Rob Herring]

On Mon, 11 Sep 2023 20:33:52 +0200, Frank Wunderlich wrote:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add compatible string for mt7988.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>  .../devicetree/bindings/thermal/mediatek,lvts-thermal.yaml       | 1 +
>  1 file changed, 1 insertion(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [RFC v1 1/3] dt-bindings: thermal: mediatek: add mt7988 compatible

[AngeloGioacchino Del Regno]

Il 11/09/23 20:33, Frank Wunderlich ha scritto:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add compatible string for mt7988.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>   .../devicetree/bindings/thermal/mediatek,lvts-thermal.yaml       | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
> index fe9ae4c425c0..49effe561963 100644
> --- a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
> +++ b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
> @@ -18,6 +18,7 @@ description: |
>   properties:
>     compatible:
>       enum:
> +      - mediatek,mt7988-lvts

Are you sure that MT7988 has only one LVTS controller, and that it is global?

>         - mediatek,mt8192-lvts-ap
>         - mediatek,mt8192-lvts-mcu
>         - mediatek,mt8195-lvts-ap

Re: [RFC v1 2/3] thermal/drivers/mediatek/lvts_thermal: make coeff configurable

[AngeloGioacchino Del Regno]

Il 11/09/23 20:33, Frank Wunderlich ha scritto:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> The upcoming mt7988 has different temperature coefficients so we
> cannot use constants in the functions lvts_golden_temp_init,
> lvts_golden_temp_init and lvts_raw_to_temp anymore.
> 
> Add a field in the lvts_ctrl pointing to the lvts_data which now
> contains the soc-specific temperature coefficents.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>   drivers/thermal/mediatek/lvts_thermal.c | 56 ++++++++++++++++++-------
>   1 file changed, 41 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> index effd9b00a424..c1004b4da3b6 100644
> --- a/drivers/thermal/mediatek/lvts_thermal.c
> +++ b/drivers/thermal/mediatek/lvts_thermal.c
> @@ -80,8 +80,8 @@
>   #define LVTS_SENSOR_MAX				4
>   #define LVTS_GOLDEN_TEMP_MAX		62
>   #define LVTS_GOLDEN_TEMP_DEFAULT	50
> -#define LVTS_COEFF_A				-250460
> -#define LVTS_COEFF_B				250460
> +#define LVTS_COEFF_A_MT8195			-250460
> +#define LVTS_COEFF_B_MT8195			250460
>   
>   #define LVTS_MSR_IMMEDIATE_MODE		0
>   #define LVTS_MSR_FILTERED_MODE		1
> @@ -94,7 +94,7 @@
>   #define LVTS_MINIMUM_THRESHOLD		20000
>   
>   static int golden_temp = LVTS_GOLDEN_TEMP_DEFAULT;
> -static int coeff_b = LVTS_COEFF_B;
> +static int coeff_b;

This could be renamed to `golden_temp_offset`

>   
>   struct lvts_sensor_data {
>   	int dt_id;
> @@ -109,9 +109,15 @@ struct lvts_ctrl_data {
>   	int mode;
>   };
>   
> +struct formula_coeff {
> +	int a;
> +	int b;
> +};
> +
>   struct lvts_data {
>   	const struct lvts_ctrl_data *lvts_ctrl;
>   	int num_lvts_ctrl;
> +	struct formula_coeff coeff;

You can just add the coefficients here directly... and while at it you can
also make it self explanatory, because the "A" coefficient is a factor while
the "B" coefficient is just an offset.

	int temp_factor; <--- coeff_a
	int temp_offset; <--- coeff_b

>   };
>   
>   struct lvts_sensor {
> @@ -126,6 +132,7 @@ struct lvts_sensor {
>   
>   struct lvts_ctrl {
>   	struct lvts_sensor sensors[LVTS_SENSOR_MAX];
> +	const struct lvts_data *lvts_data;
>   	u32 calibration[LVTS_SENSOR_MAX];
>   	u32 hw_tshut_raw_temp;
>   	int num_lvts_sensor;
> @@ -247,21 +254,21 @@ static void lvts_debugfs_exit(struct lvts_domain *lvts_td) { }
>   
>   #endif
>   
> -static int lvts_raw_to_temp(u32 raw_temp)
> +static int lvts_raw_to_temp(u32 raw_temp, struct formula_coeff coeff)
>   {
>   	int temperature;
>   
> -	temperature = ((s64)(raw_temp & 0xFFFF) * LVTS_COEFF_A) >> 14;
> +	temperature = ((s64)(raw_temp & 0xFFFF) * coeff.a) >> 14;

so that this also becomes more readable:

static int lvts_raw_to_temp(u32 raw_temp, int temp_factor)
{
	int temperature;

	temperature = ((s64)(raw_temp & 0xFFFF) * temp_factor) >> 14;
	temperature += golden_temp_offset;

	return temperature;
}

where temp_factor is lvts_data.temp_factor, and golden_temp_offset is a
rename of `static int coeff_b`.

Cheers,
Angelo

Re: [RFC v1 3/3] thermal/drivers/mediatek/lvts_thermal: add mt7988 support

[AngeloGioacchino Del Regno]

Il 11/09/23 20:33, Frank Wunderlich ha scritto:
> From: Frank Wunderlich <frank-w@public-files.de>
> 
> Add Support for mediatek fologic 880/MT7988.
> 
> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> ---
>   drivers/thermal/mediatek/lvts_thermal.c | 73 +++++++++++++++++++++++++
>   1 file changed, 73 insertions(+)
> 
> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> index c1004b4da3b6..48b257a3c80e 100644
> --- a/drivers/thermal/mediatek/lvts_thermal.c
> +++ b/drivers/thermal/mediatek/lvts_thermal.c
> @@ -82,6 +82,8 @@
>   #define LVTS_GOLDEN_TEMP_DEFAULT	50
>   #define LVTS_COEFF_A_MT8195			-250460
>   #define LVTS_COEFF_B_MT8195			250460
> +#define LVTS_COEFF_A_MT7988			-204650
> +#define LVTS_COEFF_B_MT7988			204650
>   
>   #define LVTS_MSR_IMMEDIATE_MODE		0
>   #define LVTS_MSR_FILTERED_MODE		1
> @@ -1272,6 +1274,67 @@ static int lvts_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +/*
> + * LVTS MT7988
> + */
> +#define LVTS_HW_SHUTDOWN_MT7988	117000

Are you sure that this chip's Tj is >117°C ?!

Looks a bit high... if it is exactly 117°C, I would suggest cutting earlier,
either at 110 (safe side) or 115: after all, this is a life-saver feature and
the chip is actually never meant to *constantly* work at 110°C (as it would
degrade fast and say goodbye earlier than "planned").

> +//enum mt7988_lvts_domain { MT7988_AP_DOMAIN, MT7988_NUM_DOMAIN };
> +
> +enum mt7988_lvts_sensor_enum {
> +	MT7988_TS3_0,
> +	MT7988_TS3_1,
> +	MT7988_TS3_2,
> +	MT7988_TS3_3,
> +	MT7988_TS4_0,
> +	MT7988_TS4_1,
> +	MT7988_TS4_2,
> +	MT7988_TS4_3,
> +	MT7988_NUM_TS
> +};

This enumeration should be definitions in bindings (mediatek,lvts-thermal.h).

Besides, the LVTS is about internal temperatures, so those TS3_x and 4_x can
be renamed like what was done for MT8192 and MT8195: this is because you will
never see TS3_2 being CPU2 on a board and CPU4 on another, being those - again -
internal to the SoC, hence unchangeable.

Another reason is that you'll anyway have to refer to those sensors in the
devicetree to configure thermal trips and such, so... :-)

Regards,
Angelo

Re: [RFC v1 3/3] thermal/drivers/mediatek/lvts_thermal: add mt7988 support

[Frank Wunderlich]

Am 13. September 2023 10:16:51 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
Hi angelo,

thanks for first look

>Il 11/09/23 20:33, Frank Wunderlich ha scritto:
>> From: Frank Wunderlich <frank-w@public-files.de>
>> 
>> Add Support for mediatek fologic 880/MT7988.
>> 
>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
>> ---
>>   drivers/thermal/mediatek/lvts_thermal.c | 73 +++++++++++++++++++++++++
>>   1 file changed, 73 insertions(+)
>> 
>> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
>> index c1004b4da3b6..48b257a3c80e 100644
>> --- a/drivers/thermal/mediatek/lvts_thermal.c
>> +++ b/drivers/thermal/mediatek/lvts_thermal.c
>> @@ -82,6 +82,8 @@
>>   #define LVTS_GOLDEN_TEMP_DEFAULT	50
>>   #define LVTS_COEFF_A_MT8195			-250460
>>   #define LVTS_COEFF_B_MT8195			250460
>> +#define LVTS_COEFF_A_MT7988			-204650
>> +#define LVTS_COEFF_B_MT7988			204650
>>     #define LVTS_MSR_IMMEDIATE_MODE		0
>>   #define LVTS_MSR_FILTERED_MODE		1
>> @@ -1272,6 +1274,67 @@ static int lvts_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   +/*
>> + * LVTS MT7988
>> + */
>> +#define LVTS_HW_SHUTDOWN_MT7988	117000
>
>Are you sure that this chip's Tj is >117°C ?!
>
>Looks a bit high... if it is exactly 117°C, I would suggest cutting earlier,
>either at 110 (safe side) or 115: after all, this is a life-saver feature and
>the chip is actually never meant to *constantly* work at 110°C (as it would
>degrade fast and say goodbye earlier than "planned").

I took values from SDK

https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/refs/heads/master/target/linux/mediatek/files-5.4/drivers/thermal/mediatek/soc_temp_lvts.c#1483

>> +//enum mt7988_lvts_domain { MT7988_AP_DOMAIN, MT7988_NUM_DOMAIN };
>> +
>> +enum mt7988_lvts_sensor_enum {
>> +	MT7988_TS3_0,
>> +	MT7988_TS3_1,
>> +	MT7988_TS3_2,
>> +	MT7988_TS3_3,
>> +	MT7988_TS4_0,
>> +	MT7988_TS4_1,
>> +	MT7988_TS4_2,
>> +	MT7988_TS4_3,
>> +	MT7988_NUM_TS
>> +};

>This enumeration should be definitions in bindings (mediatek,lvts-thermal.h).
>
>Besides, the LVTS is about internal temperatures, so those TS3_x and 4_x can
>be renamed like what was done for MT8192 and MT8195: this is because you will
>never see TS3_2 being CPU2 on a board and CPU4 on another, being those - again -
>internal to the SoC, hence unchangeable.

Right these sensors are internally only and i took naming from sdk to avoid confusion. And i have not more information about these internal sensors (special meaning),but their values are packed together to get the resulting (average) temperature.

>Another reason is that you'll anyway have to refer to those sensors in the
>devicetree to configure thermal trips and such, so... :-)

In device tree it will look like this:

https://github.com/frank-w/BPI-Router-Linux/blob/6.5-lvts/arch/arm64/boot/dts/mediatek/mt7988a.dtsi#L771

Daniel has also defined thermal trips there,but these are untested atm. I only verified temperature itself i get from sysfs as far as i can (start at ~40°C and reaching ~70 while running).

>Regards,
>Angelo

regards Frank

Re: [RFC v1 1/3] dt-bindings: thermal: mediatek: add mt7988 compatible

[Frank Wunderlich]

Am 13. September 2023 09:49:08 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
>Il 11/09/23 20:33, Frank Wunderlich ha scritto:
>> From: Frank Wunderlich <frank-w@public-files.de>
>> 
>> Add compatible string for mt7988.
>> 
>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
>> ---
>>   .../devicetree/bindings/thermal/mediatek,lvts-thermal.yaml       | 1 +
>>   1 file changed, 1 insertion(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
>> index fe9ae4c425c0..49effe561963 100644
>> --- a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
>> +++ b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
>> @@ -18,6 +18,7 @@ description: |
>>   properties:
>>     compatible:
>>       enum:
>> +      - mediatek,mt7988-lvts
>
>Are you sure that MT7988 has only one LVTS controller, and that it is global?

Based on the information i have it is only 1 lvts device (dts node) with 2 internal controllers. Do i need to define it in different way?

>>         - mediatek,mt8192-lvts-ap
>>         - mediatek,mt8192-lvts-mcu
>>         - mediatek,mt8195-lvts-ap
>


regards Frank

Re: [RFC v1 1/3] dt-bindings: thermal: mediatek: add mt7988 compatible

[AngeloGioacchino Del Regno]

Il 13/09/23 12:57, Frank Wunderlich ha scritto:
> Am 13. September 2023 09:49:08 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
>> Il 11/09/23 20:33, Frank Wunderlich ha scritto:
>>> From: Frank Wunderlich <frank-w@public-files.de>
>>>
>>> Add compatible string for mt7988.
>>>
>>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
>>> ---
>>>    .../devicetree/bindings/thermal/mediatek,lvts-thermal.yaml       | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
>>> index fe9ae4c425c0..49effe561963 100644
>>> --- a/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
>>> +++ b/Documentation/devicetree/bindings/thermal/mediatek,lvts-thermal.yaml
>>> @@ -18,6 +18,7 @@ description: |
>>>    properties:
>>>      compatible:
>>>        enum:
>>> +      - mediatek,mt7988-lvts
>>
>> Are you sure that MT7988 has only one LVTS controller, and that it is global?
> 
> Based on the information i have it is only 1 lvts device (dts node) with 2 internal controllers. Do i need to define it in different way?
> 

In the MediaTek BSP, I can see that the controller at 0x1100a000 is referenced
to as `MT7988_AP_DOMAIN`... this means that this controller effectively is the
LVTS-AP one.

This means that the compatible here should be "mediatek,mt7988-lvts-ap" :-)

Regards,
Angelo

>>>          - mediatek,mt8192-lvts-ap
>>>          - mediatek,mt8192-lvts-mcu
>>>          - mediatek,mt8195-lvts-ap
>>
> 
> 
> regards Frank

Re: [RFC v1 3/3] thermal/drivers/mediatek/lvts_thermal: add mt7988 support

[AngeloGioacchino Del Regno]

Il 13/09/23 12:52, Frank Wunderlich ha scritto:
> Am 13. September 2023 10:16:51 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
> Hi angelo,
> 
> thanks for first look
> 
>> Il 11/09/23 20:33, Frank Wunderlich ha scritto:
>>> From: Frank Wunderlich <frank-w@public-files.de>
>>>
>>> Add Support for mediatek fologic 880/MT7988.
>>>
>>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
>>> ---
>>>    drivers/thermal/mediatek/lvts_thermal.c | 73 +++++++++++++++++++++++++
>>>    1 file changed, 73 insertions(+)
>>>
>>> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
>>> index c1004b4da3b6..48b257a3c80e 100644
>>> --- a/drivers/thermal/mediatek/lvts_thermal.c
>>> +++ b/drivers/thermal/mediatek/lvts_thermal.c
>>> @@ -82,6 +82,8 @@
>>>    #define LVTS_GOLDEN_TEMP_DEFAULT	50
>>>    #define LVTS_COEFF_A_MT8195			-250460
>>>    #define LVTS_COEFF_B_MT8195			250460
>>> +#define LVTS_COEFF_A_MT7988			-204650
>>> +#define LVTS_COEFF_B_MT7988			204650
>>>      #define LVTS_MSR_IMMEDIATE_MODE		0
>>>    #define LVTS_MSR_FILTERED_MODE		1
>>> @@ -1272,6 +1274,67 @@ static int lvts_remove(struct platform_device *pdev)
>>>    	return 0;
>>>    }
>>>    +/*
>>> + * LVTS MT7988
>>> + */
>>> +#define LVTS_HW_SHUTDOWN_MT7988	117000
>>
>> Are you sure that this chip's Tj is >117°C ?!
>>
>> Looks a bit high... if it is exactly 117°C, I would suggest cutting earlier,
>> either at 110 (safe side) or 115: after all, this is a life-saver feature and
>> the chip is actually never meant to *constantly* work at 110°C (as it would
>> degrade fast and say goodbye earlier than "planned").
> 
> I took values from SDK
> 
> https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/refs/heads/master/target/linux/mediatek/files-5.4/drivers/thermal/mediatek/soc_temp_lvts.c#1483
> 

That kernel also defines 117°C for MT8195, which leaves me a bit dubious.

For safety, I would recommend using the same 105°C hw shutdown temperature
for 7988 as well: after all if you think about it, reaching that kind of
temperature means that there's a real problem (fan stopped working and/or
heatsink detached) that needs attention.

This is because you'll have trip points in devicetree that should throttle
the CPU in case it reaches at least a dangerously high temperature (read:
a temperature outside the recommended continuous operation range), bringing
the temperatures down because of the throttling action; I would imagine
throttling the CPU a bit down at 80°C, then a bit more at 90°C - but then,
if the temps won't drop like that, there's clearly a HW-related issue that
must be addressed (like the fan/heatsink scenario that I just described).

Though, take this as an advice; I'll respect whichever decision you'll take.

>>> +//enum mt7988_lvts_domain { MT7988_AP_DOMAIN, MT7988_NUM_DOMAIN };
>>> +
>>> +enum mt7988_lvts_sensor_enum {
>>> +	MT7988_TS3_0,
>>> +	MT7988_TS3_1,
>>> +	MT7988_TS3_2,
>>> +	MT7988_TS3_3,
>>> +	MT7988_TS4_0,
>>> +	MT7988_TS4_1,
>>> +	MT7988_TS4_2,
>>> +	MT7988_TS4_3,
>>> +	MT7988_NUM_TS
>>> +};
> 
>> This enumeration should be definitions in bindings (mediatek,lvts-thermal.h).
>>
>> Besides, the LVTS is about internal temperatures, so those TS3_x and 4_x can
>> be renamed like what was done for MT8192 and MT8195: this is because you will
>> never see TS3_2 being CPU2 on a board and CPU4 on another, being those - again -
>> internal to the SoC, hence unchangeable.
> 
> Right these sensors are internally only and i took naming from sdk to avoid confusion. And i have not more information about these internal sensors (special meaning),but their values are packed together to get the resulting (average) temperature.
> 
>> Another reason is that you'll anyway have to refer to those sensors in the
>> devicetree to configure thermal trips and such, so... :-)
> 
> In device tree it will look like this:
> 
> https://github.com/frank-w/BPI-Router-Linux/blob/6.5-lvts/arch/arm64/boot/dts/mediatek/mt7988a.dtsi#L771
> 
> Daniel has also defined thermal trips there,but these are untested atm. I only verified temperature itself i get from sysfs as far as i can (start at ~40°C and reaching ~70 while running).
> 

Check how it's done in mt8192.dtsi and mt8195.dtsi: we're using the definitions
from the bindings for thermal zones.
At least for consistency (apart from other even more valid considerations), that's
how it should be done: please do it like that.

Besides, I think that you can definitely guess what `TS` is CPU related: checking
the driver and devicetree from the downstream kernel that you provided, what I
understand is:

1. Your naming TS3,4 corresponds to TS2,3 downstream (so it's wrong?)
2. Downstream TS2 is related to the CPU cores, so it should be
    - TS2_0 - CPU0
    - TS2_1 - CPU1
    - TS2_2 - CPU2
    - TS2_3 - CPU3

The other set of thermal sensors seem to be completely unused, so we cannot guess
just by looking at the code. Since you have the hardware, you may be able to take
a position about what they are by checking what sensor heats up for each "action"
(could be ethernet controller or infra or whatever else); if in doubt, just leave
them out, but add a comment saying that there are more sensors and say where, so
that if anyone finds what they're for, it'll be easy to add them.

In any case, adding thermal sensors that you don't know about is meaningless, as
the sense of all this is:
  - Monitoring temperatures of the hardware
  - Taking action to prevent temperature overrun
but if you don't know what you're reading, you can't interpret temperatures for
something unknown, hence you can't as well take action to prevent overruns, as
you won't know what's the maximum temperature for "unknown thing" :-)

Regards,
Angelo

Aw: Re: [RFC v1 3/3] thermal/drivers/mediatek/lvts_thermal: add mt7988 support

[Frank Wunderlich]

Hi,
> Gesendet: Mittwoch, 13. September 2023 um 13:43 Uhr
> Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>
> An: frank-w@public-files.de, "Frank Wunderlich" <linux@fw-web.de>, linux-mediatek@lists.infradead.org
> Il 13/09/23 12:52, Frank Wunderlich ha scritto:
> > Am 13. September 2023 10:16:51 MESZ schrieb AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>:
> > Hi angelo,
> > 
> > thanks for first look
> > 
> >> Il 11/09/23 20:33, Frank Wunderlich ha scritto:
> >>> From: Frank Wunderlich <frank-w@public-files.de>
> >>>
> >>> Add Support for mediatek fologic 880/MT7988.
> >>>
> >>> Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> >>> ---
> >>>    drivers/thermal/mediatek/lvts_thermal.c | 73 +++++++++++++++++++++++++
> >>>    1 file changed, 73 insertions(+)
> >>>
> >>> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> >>> index c1004b4da3b6..48b257a3c80e 100644
> >>> --- a/drivers/thermal/mediatek/lvts_thermal.c
> >>> +++ b/drivers/thermal/mediatek/lvts_thermal.c
> >>> @@ -82,6 +82,8 @@
> >>>    #define LVTS_GOLDEN_TEMP_DEFAULT	50
> >>>    #define LVTS_COEFF_A_MT8195			-250460
> >>>    #define LVTS_COEFF_B_MT8195			250460
> >>> +#define LVTS_COEFF_A_MT7988			-204650
> >>> +#define LVTS_COEFF_B_MT7988			204650
> >>>      #define LVTS_MSR_IMMEDIATE_MODE		0
> >>>    #define LVTS_MSR_FILTERED_MODE		1
> >>> @@ -1272,6 +1274,67 @@ static int lvts_remove(struct platform_device *pdev)
> >>>    	return 0;
> >>>    }
> >>>    +/*
> >>> + * LVTS MT7988
> >>> + */
> >>> +#define LVTS_HW_SHUTDOWN_MT7988	117000
> >>
> >> Are you sure that this chip's Tj is >117°C ?!
> >>
> >> Looks a bit high... if it is exactly 117°C, I would suggest cutting earlier,
> >> either at 110 (safe side) or 115: after all, this is a life-saver feature and
> >> the chip is actually never meant to *constantly* work at 110°C (as it would
> >> degrade fast and say goodbye earlier than "planned").
> > 
> > I took values from SDK
> > 
> > https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/refs/heads/master/target/linux/mediatek/files-5.4/drivers/thermal/mediatek/soc_temp_lvts.c#1483
> > 
> 
> That kernel also defines 117°C for MT8195, which leaves me a bit dubious.
> 
> For safety, I would recommend using the same 105°C hw shutdown temperature
> for 7988 as well: after all if you think about it, reaching that kind of
> temperature means that there's a real problem (fan stopped working and/or
> heatsink detached) that needs attention.

ack, will change to same value and put the define on top, abve the mt8195

> This is because you'll have trip points in devicetree that should throttle
> the CPU in case it reaches at least a dangerously high temperature (read:
> a temperature outside the recommended continuous operation range), bringing
> the temperatures down because of the throttling action; I would imagine
> throttling the CPU a bit down at 80°C, then a bit more at 90°C - but then,
> if the temps won't drop like that, there's clearly a HW-related issue that
> must be addressed (like the fan/heatsink scenario that I just described).
> 
> Though, take this as an advice; I'll respect whichever decision you'll take.
> 
> >>> +//enum mt7988_lvts_domain { MT7988_AP_DOMAIN, MT7988_NUM_DOMAIN };
> >>> +
> >>> +enum mt7988_lvts_sensor_enum {
> >>> +	MT7988_TS3_0,
> >>> +	MT7988_TS3_1,
> >>> +	MT7988_TS3_2,
> >>> +	MT7988_TS3_3,
> >>> +	MT7988_TS4_0,
> >>> +	MT7988_TS4_1,
> >>> +	MT7988_TS4_2,
> >>> +	MT7988_TS4_3,
> >>> +	MT7988_NUM_TS
> >>> +};
> > 
> >> This enumeration should be definitions in bindings (mediatek,lvts-thermal.h).
> >>
> >> Besides, the LVTS is about internal temperatures, so those TS3_x and 4_x can
> >> be renamed like what was done for MT8192 and MT8195: this is because you will
> >> never see TS3_2 being CPU2 on a board and CPU4 on another, being those - again -
> >> internal to the SoC, hence unchangeable.
> > 
> > Right these sensors are internally only and i took naming from sdk to avoid confusion. And i have not more information about these internal sensors (special meaning),but their values are packed together to get the resulting (average) temperature.
> > 
> >> Another reason is that you'll anyway have to refer to those sensors in the
> >> devicetree to configure thermal trips and such, so... :-)
> > 
> > In device tree it will look like this:
> > 
> > https://github.com/frank-w/BPI-Router-Linux/blob/6.5-lvts/arch/arm64/boot/dts/mediatek/mt7988a.dtsi#L771
> > 
> > Daniel has also defined thermal trips there,but these are untested atm. I only verified temperature itself i get from sysfs as far as i can (start at ~40°C and reaching ~70 while running).
> > 
> 
> Check how it's done in mt8192.dtsi and mt8195.dtsi: we're using the definitions
> from the bindings for thermal zones.
> At least for consistency (apart from other even more valid considerations), that's
> how it should be done: please do it like that.
> 
> Besides, I think that you can definitely guess what `TS` is CPU related: checking
> the driver and devicetree from the downstream kernel that you provided, what I
> understand is:
> 
> 1. Your naming TS3,4 corresponds to TS2,3 downstream (so it's wrong?)
> 2. Downstream TS2 is related to the CPU cores, so it should be
>     - TS2_0 - CPU0
>     - TS2_1 - CPU1
>     - TS2_2 - CPU2
>     - TS2_3 - CPU3

i took the names from efuse names (which are listed as comment above), had 2/3 before.
need to ask mtk here. same for the second controller and if it is really a "lvts-ap".

> The other set of thermal sensors seem to be completely unused, so we cannot guess
> just by looking at the code. Since you have the hardware, you may be able to take
> a position about what they are by checking what sensor heats up for each "action"
> (could be ethernet controller or infra or whatever else); if in doubt, just leave
> them out, but add a comment saying that there are more sensors and say where, so
> that if anyone finds what they're for, it'll be easy to add them.
> 
> In any case, adding thermal sensors that you don't know about is meaningless, as
> the sense of all this is:
>   - Monitoring temperatures of the hardware
>   - Taking action to prevent temperature overrun
> but if you don't know what you're reading, you can't interpret temperatures for
> something unknown, hence you can't as well take action to prevent overruns, as
> you won't know what's the maximum temperature for "unknown thing" :-)

yes i have only the downstream kernel and some additional information like interupt
number and the efuse offsets for calibration. And for the 8 sensors i get this information:

"8 sensors are distributed across the whole soc in order to get the correct average temperature.
chip designer didn’t disclose more detailed info to us."

so i thought all 8 sensors are taken into account with both controllers to calculate 1
resulting temperature. In downstream i saw also only 1 controller in dts and i have
only 1 interrupt.

https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/refs/heads/master/target/linux/mediatek/files-5.4/arch/arm64/boot/dts/mediatek/mt7988.dtsi#579

and the compatible for both (mt7988 and mt8195) are without "ap" there, so i took the same.

Where have you seen the mt7988-AP ?

i have only seen 1 sensor which should be the SOC itself and this chip has metal surface
which makes it hard to get real temperature (infrared thermometer gives wrong temp).

> Regards,
> Angelo

Aw: Re: [RFC v1 2/3] thermal/drivers/mediatek/lvts_thermal: make coeff configurable

[Frank Wunderlich]

Hi

> Gesendet: Mittwoch, 13. September 2023 um 10:03 Uhr
> Von: "AngeloGioacchino Del Regno" <angelogioacchino.delregno@collabora.com>
> Il 11/09/23 20:33, Frank Wunderlich ha scritto:
> > From: Frank Wunderlich <frank-w@public-files.de>
> >
> > The upcoming mt7988 has different temperature coefficients so we
> > cannot use constants in the functions lvts_golden_temp_init,
> > lvts_golden_temp_init and lvts_raw_to_temp anymore.
> >
> > Add a field in the lvts_ctrl pointing to the lvts_data which now
> > contains the soc-specific temperature coefficents.
> >
> > Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> > ---
> >   drivers/thermal/mediatek/lvts_thermal.c | 56 ++++++++++++++++++-------
> >   1 file changed, 41 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> > index effd9b00a424..c1004b4da3b6 100644
> > --- a/drivers/thermal/mediatek/lvts_thermal.c
> > +++ b/drivers/thermal/mediatek/lvts_thermal.c
> > @@ -80,8 +80,8 @@
> >   #define LVTS_SENSOR_MAX				4
> >   #define LVTS_GOLDEN_TEMP_MAX		62
> >   #define LVTS_GOLDEN_TEMP_DEFAULT	50
> > -#define LVTS_COEFF_A				-250460
> > -#define LVTS_COEFF_B				250460
> > +#define LVTS_COEFF_A_MT8195			-250460
> > +#define LVTS_COEFF_B_MT8195			250460
> >
> >   #define LVTS_MSR_IMMEDIATE_MODE		0
> >   #define LVTS_MSR_FILTERED_MODE		1
> > @@ -94,7 +94,7 @@
> >   #define LVTS_MINIMUM_THRESHOLD		20000
> >
> >   static int golden_temp = LVTS_GOLDEN_TEMP_DEFAULT;
> > -static int coeff_b = LVTS_COEFF_B;
> > +static int coeff_b;
>
> This could be renamed to `golden_temp_offset`
>
> >
> >   struct lvts_sensor_data {
> >   	int dt_id;
> > @@ -109,9 +109,15 @@ struct lvts_ctrl_data {
> >   	int mode;
> >   };
> >
> > +struct formula_coeff {
> > +	int a;
> > +	int b;
> > +};
> > +
> >   struct lvts_data {
> >   	const struct lvts_ctrl_data *lvts_ctrl;
> >   	int num_lvts_ctrl;
> > +	struct formula_coeff coeff;
>
> You can just add the coefficients here directly... and while at it you can
> also make it self explanatory, because the "A" coefficient is a factor while
> the "B" coefficient is just an offset.
>
> 	int temp_factor; <--- coeff_a
> 	int temp_offset; <--- coeff_b

makes sense...imho very good idea.
originally i took the naming based on the Constants which maybe should be changed too?

> >   };
> >
> >   struct lvts_sensor {
> > @@ -126,6 +132,7 @@ struct lvts_sensor {
> >
> >   struct lvts_ctrl {
> >   	struct lvts_sensor sensors[LVTS_SENSOR_MAX];
> > +	const struct lvts_data *lvts_data;
> >   	u32 calibration[LVTS_SENSOR_MAX];
> >   	u32 hw_tshut_raw_temp;
> >   	int num_lvts_sensor;
> > @@ -247,21 +254,21 @@ static void lvts_debugfs_exit(struct lvts_domain *lvts_td) { }
> >
> >   #endif
> >
> > -static int lvts_raw_to_temp(u32 raw_temp)
> > +static int lvts_raw_to_temp(u32 raw_temp, struct formula_coeff coeff)
> >   {
> >   	int temperature;
> >
> > -	temperature = ((s64)(raw_temp & 0xFFFF) * LVTS_COEFF_A) >> 14;
> > +	temperature = ((s64)(raw_temp & 0xFFFF) * coeff.a) >> 14;
>
> so that this also becomes more readable:
>
> static int lvts_raw_to_temp(u32 raw_temp, int temp_factor)
> {
> 	int temperature;
>
> 	temperature = ((s64)(raw_temp & 0xFFFF) * temp_factor) >> 14;
> 	temperature += golden_temp_offset;
>
> 	return temperature;
> }
>
> where temp_factor is lvts_data.temp_factor, and golden_temp_offset is a
> rename of `static int coeff_b`.

right and passing an int (instead of struct) is easier and more readable too.

> Cheers,
> Angelo
>
>