diff mbox series

[v3,3/6] drivers/thermal/exynos: improve sanitize_temp_error

Message ID 20240807084829.1037303-4-m.majewski2@samsung.com (mailing list archive)
State New, archived
Headers show
Series Add initial Exynos850 support to the thermal driver | expand

Commit Message

Mateusz Majewski Aug. 7, 2024, 8:48 a.m. UTC 
There are two minor issues regarding this function.

One is that it attempts to calculate the second calibration value even
if 1-point trimming is being used; in this case, the calculated value is
probably not useful and is never used anyway. Changing this also
requires a minor reordering in Exynos5433 initialization function, so
that we know which type of trimming is used before we call
sanitize_temp_error.

The second issue is that the function is not very consistent when it
comes to the use of Exynos7-specific parameters. This seems to not be an
issue in practice, in part because some of these issues are related to
the mentioned calculation of the second calibration value. However,
fixing this makes the code a bit less confusing, and will be required
for Exynos850 which has 9-bit temperature values and uses 2-point
trimming.

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
v1 -> v2: reworked to change shift instead of only mask and to also fix
  the 2-point trimming issue.

 drivers/thermal/samsung/exynos_tmu.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

Comments

Daniel Lezcano Sept. 2, 2024, 12:50 p.m. UTC | #1 
On 07/08/2024 10:48, Mateusz Majewski wrote:
> There are two minor issues regarding this function.
> 
> One is that it attempts to calculate the second calibration value even
> if 1-point trimming is being used; in this case, the calculated value is
> probably not useful and is never used anyway. Changing this also
> requires a minor reordering in Exynos5433 initialization function, so
> that we know which type of trimming is used before we call
> sanitize_temp_error.
> 
> The second issue is that the function is not very consistent when it
> comes to the use of Exynos7-specific parameters. This seems to not be an
> issue in practice, in part because some of these issues are related to
> the mentioned calculation of the second calibration value. However,
> fixing this makes the code a bit less confusing, and will be required
> for Exynos850 which has 9-bit temperature values and uses 2-point
> trimming.

May I suggest to convert this function to a specific soc ops to be put 
in the struct exynos_tmu_data ?

> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---
> v1 -> v2: reworked to change shift instead of only mask and to also fix
>    the 2-point trimming issue.
> 
>   drivers/thermal/samsung/exynos_tmu.c | 23 ++++++++++++++---------
>   1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index b68e9755c933..087a09628e23 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -111,6 +111,7 @@
>   #define EXYNOS7_TMU_REG_EMUL_CON		0x160
>   
>   #define EXYNOS7_TMU_TEMP_MASK			0x1ff
> +#define EXYNOS7_TMU_TEMP_SHIFT			9
>   #define EXYNOS7_PD_DET_EN_SHIFT			23
>   #define EXYNOS7_TMU_INTEN_RISE0_SHIFT		0
>   #define EXYNOS7_EMUL_DATA_SHIFT			7
> @@ -234,20 +235,23 @@ static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
>   	u16 tmu_temp_mask =
>   		(data->soc == SOC_ARCH_EXYNOS7) ? EXYNOS7_TMU_TEMP_MASK
>   						: EXYNOS_TMU_TEMP_MASK;
> +	int tmu_85_shift =
> +		(data->soc == SOC_ARCH_EXYNOS7) ? EXYNOS7_TMU_TEMP_SHIFT
> +						: EXYNOS_TRIMINFO_85_SHIFT;
>   
>   	data->temp_error1 = trim_info & tmu_temp_mask;
> -	data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
> -				EXYNOS_TMU_TEMP_MASK);
> -
>   	if (!data->temp_error1 ||
>   	    (data->min_efuse_value > data->temp_error1) ||
>   	    (data->temp_error1 > data->max_efuse_value))
> -		data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK;
> +		data->temp_error1 = data->efuse_value & tmu_temp_mask;
>   
> -	if (!data->temp_error2)
> -		data->temp_error2 =
> -			(data->efuse_value >> EXYNOS_TRIMINFO_85_SHIFT) &
> -			EXYNOS_TMU_TEMP_MASK;
> +	if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
> +		data->temp_error2 = (trim_info >> tmu_85_shift) & tmu_temp_mask;
> +		if (!data->temp_error2)
> +			data->temp_error2 =
> +				(data->efuse_value >> tmu_85_shift) &
> +				tmu_temp_mask;
> +	}
>   }
>   
>   static int exynos_tmu_initialize(struct platform_device *pdev)
> @@ -510,7 +514,6 @@ static void exynos5433_tmu_initialize(struct platform_device *pdev)
>   	int sensor_id, cal_type;
>   
>   	trim_info = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
> -	sanitize_temp_error(data, trim_info);
>   
>   	/* Read the temperature sensor id */
>   	sensor_id = (trim_info & EXYNOS5433_TRIMINFO_SENSOR_ID_MASK)
> @@ -532,6 +535,8 @@ static void exynos5433_tmu_initialize(struct platform_device *pdev)
>   		break;
>   	}
>   
> +	sanitize_temp_error(data, trim_info);
> +
>   	dev_info(&pdev->dev, "Calibration type is %d-point calibration\n",
>   			cal_type ?  2 : 1);
>   }
Mateusz Majewski Sept. 3, 2024, 7:19 a.m. UTC | #2 
Hello :)

> May I suggest to convert this function to a specific soc ops to be put
> in the struct exynos_tmu_data ?

Like this?

static void exynos4210_sanitize_temp_error(struct exynos_tmu_data *data,
					   u32 trim_info)
{
	data->temp_error1 = trim_info & EXYNOS_TMU_TEMP_MASK;
	if (!data->temp_error1 ||
	    (data->min_efuse_value > data->temp_error1) ||
	    (data->temp_error1 > data->max_efuse_value))
		data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK;
	WARN_ON_ONCE(data->cal_type == TYPE_TWO_POINT_TRIMMING);
}

static void exynos5433_sanitize_temp_error(struct exynos_tmu_data *data,
					   u32 trim_info)
{
	data->temp_error1 = trim_info & EXYNOS_TMU_TEMP_MASK;
	if (!data->temp_error1 ||
	    (data->min_efuse_value > data->temp_error1) ||
	    (data->temp_error1 > data->max_efuse_value))
		data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK;

	if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
		data->temp_error2 = (trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
				    EXYNOS_TMU_TEMP_MASK;
		if (!data->temp_error2)
			data->temp_error2 = (data->efuse_value >>
					     EXYNOS_TRIMINFO_85_SHIFT) &
					    EXYNOS_TMU_TEMP_MASK;
	}
}

static void exynos7_sanitize_temp_error(struct exynos_tmu_data *data,
					u32 trim_info)
{
	data->temp_error1 = trim_info & EXYNOS7_TMU_TEMP_MASK;
	if (!data->temp_error1 ||
	    (data->min_efuse_value > data->temp_error1) ||
	    (data->temp_error1 > data->max_efuse_value))
		data->temp_error1 = data->efuse_value & EXYNOS7_TMU_TEMP_MASK;
	WARN_ON_ONCE(data->cal_type == TYPE_TWO_POINT_TRIMMING);
}

static void exynos850_sanitize_temp_error(struct exynos_tmu_data *data,
					   u32 trim_info)
{
	data->temp_error1 = trim_info & EXYNOS7_TMU_TEMP_MASK;
	if (!data->temp_error1 ||
	    (data->min_efuse_value > data->temp_error1) ||
	    (data->temp_error1 > data->max_efuse_value))
		data->temp_error1 = data->efuse_value & EXYNOS7_TMU_TEMP_MASK;

	if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
		data->temp_error2 = (trim_info >> EXYNOS7_TMU_TEMP_SHIFT) &
				    EXYNOS7_TMU_TEMP_MASK;
		if (!data->temp_error2)
			data->temp_error2 = (data->efuse_value >>
					     EXYNOS7_TMU_TEMP_SHIFT) &
					    EXYNOS_TMU_TEMP_MASK;
	}
}

Or maybe we could put tmu_temp_mask and tmu_85_shift in data instead,
and have one function like this:

static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
{
	data->temp_error1 = trim_info & data->tmu_temp_mask;
	if (!data->temp_error1 || (data->min_efuse_value > data->temp_error1) ||
	    (data->temp_error1 > data->max_efuse_value))
		data->temp_error1 = data->efuse_value & data->tmu_temp_mask;

	if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
		data->temp_error2 = (trim_info >> data->tmu_85_shift) &
				    data->tmu_temp_mask;
		if (!data->temp_error2)
			data->temp_error2 =
				(data->efuse_value >> data->tmu_85_shift) &
				data->tmu_temp_mask;
	}
}

Thank you,
Mateusz
Daniel Lezcano Sept. 6, 2024, 10:04 a.m. UTC | #3 
On 03/09/2024 09:19, Mateusz Majewski wrote:
> Hello :)
> 
>> May I suggest to convert this function to a specific soc ops to be put
>> in the struct exynos_tmu_data ?
> 
> Like this?
> 
> static void exynos4210_sanitize_temp_error(struct exynos_tmu_data *data,
> 					   u32 trim_info)
> {
> 	data->temp_error1 = trim_info & EXYNOS_TMU_TEMP_MASK;
> 	if (!data->temp_error1 ||
> 	    (data->min_efuse_value > data->temp_error1) ||
> 	    (data->temp_error1 > data->max_efuse_value))
> 		data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK;
> 	WARN_ON_ONCE(data->cal_type == TYPE_TWO_POINT_TRIMMING);
> }
> 
> static void exynos5433_sanitize_temp_error(struct exynos_tmu_data *data,
> 					   u32 trim_info)
> {
> 	data->temp_error1 = trim_info & EXYNOS_TMU_TEMP_MASK;
> 	if (!data->temp_error1 ||
> 	    (data->min_efuse_value > data->temp_error1) ||
> 	    (data->temp_error1 > data->max_efuse_value))
> 		data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK;
> 
> 	if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
> 		data->temp_error2 = (trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
> 				    EXYNOS_TMU_TEMP_MASK;
> 		if (!data->temp_error2)
> 			data->temp_error2 = (data->efuse_value >>
> 					     EXYNOS_TRIMINFO_85_SHIFT) &
> 					    EXYNOS_TMU_TEMP_MASK;
> 	}
> }
> 
> static void exynos7_sanitize_temp_error(struct exynos_tmu_data *data,
> 					u32 trim_info)
> {
> 	data->temp_error1 = trim_info & EXYNOS7_TMU_TEMP_MASK;
> 	if (!data->temp_error1 ||
> 	    (data->min_efuse_value > data->temp_error1) ||
> 	    (data->temp_error1 > data->max_efuse_value))
> 		data->temp_error1 = data->efuse_value & EXYNOS7_TMU_TEMP_MASK;
> 	WARN_ON_ONCE(data->cal_type == TYPE_TWO_POINT_TRIMMING);
> }
> 
> static void exynos850_sanitize_temp_error(struct exynos_tmu_data *data,
> 					   u32 trim_info)
> {
> 	data->temp_error1 = trim_info & EXYNOS7_TMU_TEMP_MASK;
> 	if (!data->temp_error1 ||
> 	    (data->min_efuse_value > data->temp_error1) ||
> 	    (data->temp_error1 > data->max_efuse_value))
> 		data->temp_error1 = data->efuse_value & EXYNOS7_TMU_TEMP_MASK;
> 
> 	if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
> 		data->temp_error2 = (trim_info >> EXYNOS7_TMU_TEMP_SHIFT) &
> 				    EXYNOS7_TMU_TEMP_MASK;
> 		if (!data->temp_error2)
> 			data->temp_error2 = (data->efuse_value >>
> 					     EXYNOS7_TMU_TEMP_SHIFT) &
> 					    EXYNOS_TMU_TEMP_MASK;
> 	}
> }

Yes, something like that but may be with more code factoring, like a 
common routine to pass the temp_mask and the specific chunk of code.

> Or maybe we could put tmu_temp_mask and tmu_85_shift in data instead,
> and have one function like this:

Either way

It would be nice if the code can be commented to explain how the sensor 
works in order to understand what means "sanitize the temp error"

> static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
> {
> 	data->temp_error1 = trim_info & data->tmu_temp_mask;
> 	if (!data->temp_error1 || (data->min_efuse_value > data->temp_error1) ||
> 	    (data->temp_error1 > data->max_efuse_value))
> 		data->temp_error1 = data->efuse_value & data->tmu_temp_mask;
> 
> 	if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
> 		data->temp_error2 = (trim_info >> data->tmu_85_shift) &
> 				    data->tmu_temp_mask;
> 		if (!data->temp_error2)
> 			data->temp_error2 =
> 				(data->efuse_value >> data->tmu_85_shift) &
> 				data->tmu_temp_mask;
> 	}
> }
> 
> Thank you,
> Mateusz
diff mbox series

Patch

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index b68e9755c933..087a09628e23 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -111,6 +111,7 @@ 
 #define EXYNOS7_TMU_REG_EMUL_CON		0x160
 
 #define EXYNOS7_TMU_TEMP_MASK			0x1ff
+#define EXYNOS7_TMU_TEMP_SHIFT			9
 #define EXYNOS7_PD_DET_EN_SHIFT			23
 #define EXYNOS7_TMU_INTEN_RISE0_SHIFT		0
 #define EXYNOS7_EMUL_DATA_SHIFT			7
@@ -234,20 +235,23 @@  static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
 	u16 tmu_temp_mask =
 		(data->soc == SOC_ARCH_EXYNOS7) ? EXYNOS7_TMU_TEMP_MASK
 						: EXYNOS_TMU_TEMP_MASK;
+	int tmu_85_shift =
+		(data->soc == SOC_ARCH_EXYNOS7) ? EXYNOS7_TMU_TEMP_SHIFT
+						: EXYNOS_TRIMINFO_85_SHIFT;
 
 	data->temp_error1 = trim_info & tmu_temp_mask;
-	data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
-				EXYNOS_TMU_TEMP_MASK);
-
 	if (!data->temp_error1 ||
 	    (data->min_efuse_value > data->temp_error1) ||
 	    (data->temp_error1 > data->max_efuse_value))
-		data->temp_error1 = data->efuse_value & EXYNOS_TMU_TEMP_MASK;
+		data->temp_error1 = data->efuse_value & tmu_temp_mask;
 
-	if (!data->temp_error2)
-		data->temp_error2 =
-			(data->efuse_value >> EXYNOS_TRIMINFO_85_SHIFT) &
-			EXYNOS_TMU_TEMP_MASK;
+	if (data->cal_type == TYPE_TWO_POINT_TRIMMING) {
+		data->temp_error2 = (trim_info >> tmu_85_shift) & tmu_temp_mask;
+		if (!data->temp_error2)
+			data->temp_error2 =
+				(data->efuse_value >> tmu_85_shift) &
+				tmu_temp_mask;
+	}
 }
 
 static int exynos_tmu_initialize(struct platform_device *pdev)
@@ -510,7 +514,6 @@  static void exynos5433_tmu_initialize(struct platform_device *pdev)
 	int sensor_id, cal_type;
 
 	trim_info = readl(data->base + EXYNOS_TMU_REG_TRIMINFO);
-	sanitize_temp_error(data, trim_info);
 
 	/* Read the temperature sensor id */
 	sensor_id = (trim_info & EXYNOS5433_TRIMINFO_SENSOR_ID_MASK)
@@ -532,6 +535,8 @@  static void exynos5433_tmu_initialize(struct platform_device *pdev)
 		break;
 	}
 
+	sanitize_temp_error(data, trim_info);
+
 	dev_info(&pdev->dev, "Calibration type is %d-point calibration\n",
 			cal_type ?  2 : 1);
 }