[PATCH 0/4] thermal: imx: cleanup and new formula

[PATCH 0/4] thermal: imx: cleanup and new formula

[Uwe Kleine-König]

Hello,

this series updates the imx driver to use a new formula privided by NXP
for better measurements. The first three patches are not strictly
necessary as preparation, but IMHO make the driver easier to understand.
I hope this is not subjective.

Best regards
Uwe

Uwe Kleine-König (4):
  thermal: imx: Use better parameter names than "val"
  thermal: imx: improve comments describing algorithm for temp
    calculation
  thermal: imx: use consistent style to write temperatures
  thermal: imx: update to new formula according to NXP AN5215

 drivers/thermal/imx_thermal.c | 74 +++++++++++++++++++------------------------
 1 file changed, 33 insertions(+), 41 deletions(-)

-- 
2.14.2

[PATCH 1/4] thermal: imx: Use better parameter names than "val"

[Uwe Kleine-König]

The values passed to imx_init_calib() and imx_init_temp_grade() are
read from specific OCOTP values. Use their names (in lower case) as
parameter name instead of "val" to make the code easier to understand.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/thermal/imx_thermal.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index e7d4ffc3de7f..21b8c4c4da3c 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -347,13 +347,13 @@ static struct thermal_zone_device_ops imx_tz_ops = {
 	.set_trip_temp = imx_set_trip_temp,
 };
 
-static int imx_init_calib(struct platform_device *pdev, u32 val)
+static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
 {
 	struct imx_thermal_data *data = platform_get_drvdata(pdev);
 	int t1, n1;
 	u64 temp64;
 
-	if (val == 0 || val == ~0) {
+	if (ocotp_ana1 == 0 || ocotp_ana1 == ~0) {
 		dev_err(&pdev->dev, "invalid sensor calibration data\n");
 		return -EINVAL;
 	}
@@ -364,7 +364,7 @@ static int imx_init_calib(struct platform_device *pdev, u32 val)
 	 * Use universal formula now and only need sensor value @ 25C
 	 * slope = 0.4297157 - (0.0015976 * 25C fuse)
 	 */
-	n1 = val >> 20;
+	n1 = ocotp_ana1 >> 20;
 	t1 = 25; /* t1 always 25C */
 
 	/*
@@ -392,12 +392,12 @@ static int imx_init_calib(struct platform_device *pdev, u32 val)
 	return 0;
 }
 
-static void imx_init_temp_grade(struct platform_device *pdev, u32 val)
+static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)
 {
 	struct imx_thermal_data *data = platform_get_drvdata(pdev);
 
 	/* The maximum die temp is specified by the Temperature Grade */
-	switch ((val >> 6) & 0x3) {
+	switch ((ocotp_mem0 >> 6) & 0x3) {
 	case 0: /* Commercial (0 to 95C) */
 		data->temp_grade = "Commercial";
 		data->temp_max = 95000;
-- 
2.14.2

[PATCH 2/4] thermal: imx: improve comments describing algorithm for temp calculation

[Uwe Kleine-König]

The description of the implemented algorithm is hardly understandable
without having the right application note side-by-side to the code.

Fix this by using shorter and more intuitive variable names, describe
their meaning and transform a single formula instead of first talking about
slope and then about "milli_Tmeas".

There are no code changes.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/thermal/imx_thermal.c | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index 21b8c4c4da3c..c08883dff2cb 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -359,32 +359,28 @@ static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
 	}
 
 	/*
-	 * Sensor data layout:
-	 *   [31:20] - sensor value @ 25C
-	 * Use universal formula now and only need sensor value @ 25C
-	 * slope = 0.4297157 - (0.0015976 * 25C fuse)
+	 * The sensor is calibrated at 25 °C (aka T1) and the value measured
+	 * (aka N1) at this temperature is provided in bits [31:20] in the
+	 * i.MX's OCOTP value ANA1.
+	 * To find the actual temperature T, the following formula has to be used
+	 * when reading value n from the sensor:
+	 *
+	 * T = T1 + (N - N1) / (0.4297157 - 0.0015976 * N1) °C
+	 *   = [T1 - N1 / (0.4297157 - 0.0015976 * N1) °C] + N / (0.4297157 - 0.0015976 * N1) °C
+	 *   = [T1 + N1 / (0.0015976 * N1 - 0.4297157) °C] - N / (0.0015976 * N1 - 0.4297157) °C
+	 *   = c2 - c1 * N
+	 *
+	 * with
+	 *
+	 *   c1 = 1 / (0.0015976 * N1 - 0.4297157) °C
+	 *   c2 = T1 + N1 / (0.0015976 * N1 - 0.4297157) °C
+	 *      = T1 + N1 * C1
 	 */
 	n1 = ocotp_ana1 >> 20;
-	t1 = 25; /* t1 always 25C */
+	t1 = 25; /* °C */
 
-	/*
-	 * Derived from linear interpolation:
-	 * slope = 0.4297157 - (0.0015976 * 25C fuse)
-	 * slope = (FACTOR2 - FACTOR1 * n1) / FACTOR0
-	 * (Nmeas - n1) / (Tmeas - t1) = slope
-	 * We want to reduce this down to the minimum computation necessary
-	 * for each temperature read.  Also, we want Tmeas in millicelsius
-	 * and we don't want to lose precision from integer division. So...
-	 * Tmeas = (Nmeas - n1) / slope + t1
-	 * milli_Tmeas = 1000 * (Nmeas - n1) / slope + 1000 * t1
-	 * milli_Tmeas = -1000 * (n1 - Nmeas) / slope + 1000 * t1
-	 * Let constant c1 = (-1000 / slope)
-	 * milli_Tmeas = (n1 - Nmeas) * c1 + 1000 * t1
-	 * Let constant c2 = n1 *c1 + 1000 * t1
-	 * milli_Tmeas = c2 - Nmeas * c1
-	 */
-	temp64 = FACTOR0;
-	temp64 *= 1000;
+	temp64 = FACTOR0; /* 10^7 for FACTOR1 and FACTOR2 */
+	temp64 *= 1000; /* to get result in °mC */
 	do_div(temp64, FACTOR1 * n1 - FACTOR2);
 	data->c1 = temp64;
 	data->c2 = n1 * data->c1 + 1000 * t1;
-- 
2.14.2

[PATCH 3/4] thermal: imx: use consistent style to write temperatures

[Uwe Kleine-König]

The previous commit already took care to use the right notation for
temperatures. Add correct units to all values representing temperatures in
the right notation for the rest of the file.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/thermal/imx_thermal.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index c08883dff2cb..b24aa6cfebbf 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -394,27 +394,27 @@ static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)
 
 	/* The maximum die temp is specified by the Temperature Grade */
 	switch ((ocotp_mem0 >> 6) & 0x3) {
-	case 0: /* Commercial (0 to 95C) */
+	case 0: /* Commercial (0 to 95 °C) */
 		data->temp_grade = "Commercial";
 		data->temp_max = 95000;
 		break;
-	case 1: /* Extended Commercial (-20 to 105C) */
+	case 1: /* Extended Commercial (-20 °C to 105 °C) */
 		data->temp_grade = "Extended Commercial";
 		data->temp_max = 105000;
 		break;
-	case 2: /* Industrial (-40 to 105C) */
+	case 2: /* Industrial (-40 °C to 105 °C) */
 		data->temp_grade = "Industrial";
 		data->temp_max = 105000;
 		break;
-	case 3: /* Automotive (-40 to 125C) */
+	case 3: /* Automotive (-40 °C to 125 °C) */
 		data->temp_grade = "Automotive";
 		data->temp_max = 125000;
 		break;
 	}
 
 	/*
-	 * Set the critical trip point at 5C under max
-	 * Set the passive trip point at 10C under max (can change via sysfs)
+	 * Set the critical trip point at 5 °C under max
+	 * Set the passive trip point at 10 °C under max (changable via sysfs)
 	 */
 	data->temp_critical = data->temp_max - (1000 * 5);
 	data->temp_passive = data->temp_max - (1000 * 10);
-- 
2.14.2

[PATCH 4/4] thermal: imx: update to new formula according to NXP AN5215

[Uwe Kleine-König]

According to an application note from 03/2017 there is an updated formula to
calculate the temperature that better matches reality. This is implemented here.

While updating move the magic constants from cpp defines which are far above the
explaining formula to constants in the code just under the explaining comment.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/thermal/imx_thermal.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
index b24aa6cfebbf..95116475ac98 100644
--- a/drivers/thermal/imx_thermal.c
+++ b/drivers/thermal/imx_thermal.c
@@ -70,10 +70,6 @@ enum imx_thermal_trip {
 #define IMX_POLLING_DELAY		2000 /* millisecond */
 #define IMX_PASSIVE_DELAY		1000
 
-#define FACTOR0				10000000
-#define FACTOR1				15976
-#define FACTOR2				4297157
-
 #define TEMPMON_IMX6Q			1
 #define TEMPMON_IMX6SX			2
 
@@ -350,7 +346,7 @@ static struct thermal_zone_device_ops imx_tz_ops = {
 static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
 {
 	struct imx_thermal_data *data = platform_get_drvdata(pdev);
-	int t1, n1;
+	int n1;
 	u64 temp64;
 
 	if (ocotp_ana1 == 0 || ocotp_ana1 == ~0) {
@@ -365,25 +361,25 @@ static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
 	 * To find the actual temperature T, the following formula has to be used
 	 * when reading value n from the sensor:
 	 *
-	 * T = T1 + (N - N1) / (0.4297157 - 0.0015976 * N1) °C
-	 *   = [T1 - N1 / (0.4297157 - 0.0015976 * N1) °C] + N / (0.4297157 - 0.0015976 * N1) °C
-	 *   = [T1 + N1 / (0.0015976 * N1 - 0.4297157) °C] - N / (0.0015976 * N1 - 0.4297157) °C
+	 * T = T1 + (N - N1) / (0.4148468 - 0.0015423 * N1) °C + 3.580661 °C
+	 *   = [T1' - N1 / (0.4148468 - 0.0015423 * N1) °C] + N / (0.4148468 - 0.0015423 * N1) °C
+	 *   = [T1' + N1 / (0.0015423 * N1 - 0.4148468) °C] - N / (0.0015423 * N1 - 0.4148468) °C
 	 *   = c2 - c1 * N
 	 *
 	 * with
 	 *
-	 *   c1 = 1 / (0.0015976 * N1 - 0.4297157) °C
-	 *   c2 = T1 + N1 / (0.0015976 * N1 - 0.4297157) °C
-	 *      = T1 + N1 * C1
+	 *  T1' = 28.580661 °C
+	 *   c1 = 1 / (0.0015423 * N1 - 0.4297157) °C
+	 *   c2 = T1' + N1 / (0.0015423 * N1 - 0.4148468) °C
+	 *      = T1' + N1 * c1
 	 */
 	n1 = ocotp_ana1 >> 20;
-	t1 = 25; /* °C */
 
-	temp64 = FACTOR0; /* 10^7 for FACTOR1 and FACTOR2 */
+	temp64 = 10000000; /* use 10^7 as fixed point constant for values in formula */
 	temp64 *= 1000; /* to get result in °mC */
-	do_div(temp64, FACTOR1 * n1 - FACTOR2);
+	do_div(temp64, 15423 * n1 - 4148468);
 	data->c1 = temp64;
-	data->c2 = n1 * data->c1 + 1000 * t1;
+	data->c2 = n1 * data->c1 + 28581;
 
 	return 0;
 }
-- 
2.14.2

Re: [PATCH 1/4] thermal: imx: Use better parameter names than "val"

[Leonard Crestez]

On Tue, 2017-11-21 at 21:02 +0100, Uwe Kleine-König wrote:
> The values passed to imx_init_calib() and imx_init_temp_grade() are
> read from specific OCOTP values. Use their names (in lower case) as
> parameter name instead of "val" to make the code easier to understand.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/thermal/imx_thermal.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index e7d4ffc3de7f..21b8c4c4da3c 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -347,13 +347,13 @@ static struct thermal_zone_device_ops imx_tz_ops = {
>  	.set_trip_temp = imx_set_trip_temp,
>  };
>  
> -static int imx_init_calib(struct platform_device *pdev, u32 val)
> +static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
>  {
>  	struct imx_thermal_data *data = platform_get_drvdata(pdev);
>  	int t1, n1;
>  	u64 temp64;
>  
> -	if (val == 0 || val == ~0) {
> +	if (ocotp_ana1 == 0 || ocotp_ana1 == ~0) {
>  		dev_err(&pdev->dev, "invalid sensor calibration data\n");
>  		return -EINVAL;
>  	}
> @@ -364,7 +364,7 @@ static int imx_init_calib(struct platform_device *pdev, u32 val)
>  	 * Use universal formula now and only need sensor value @ 25C
>  	 * slope = 0.4297157 - (0.0015976 * 25C fuse)
>  	 */
> -	n1 = val >> 20;
> +	n1 = ocotp_ana1 >> 20;
>  	t1 = 25; /* t1 always 25C */
>  
>  	/*
> @@ -392,12 +392,12 @@ static int imx_init_calib(struct platform_device *pdev, u32 val)
>  	return 0;
>  }
>  
> -static void imx_init_temp_grade(struct platform_device *pdev, u32 val)
> +static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)

On imx7 (thermal not currently supported in upstream) the temperature
grade is in OCOTP_TESTER3. So using a more generic parameter name here
would be preferable or this would have to be changed again later.

Unfortunately I can't find a good reference for the imx7 fusemap. But
the IMX7D Reference Manual mentions a speed grade in 0x440 and
something else for 0x480.

--
Regards,
Leonard

Re: [PATCH 2/4] thermal: imx: improve comments describing algorithm for temp calculation

[Leonard Crestez]

On Tue, 2017-11-21 at 21:02 +0100, Uwe Kleine-König wrote:
> The description of the implemented algorithm is hardly understandable
> without having the right application note side-by-side to the code.
> 
> Fix this by using shorter and more intuitive variable names, describe
> their meaning and transform a single formula instead of first talking about
> slope and then about "milli_Tmeas".
> 
> There are no code changes.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Maybe you should reorder so that patches 2 and 4 (which are most
interesting) are right next to each other?

Reviewed-by: Leonard Crestez <leonard.crestez@nxp.com>

Re: [PATCH 3/4] thermal: imx: use consistent style to write temperatures

[Leonard Crestez]

On Tue, 2017-11-21 at 21:02 +0100, Uwe Kleine-König wrote:
> 
> The previous commit already took care to use the right notation for
> temperatures. Add correct units to all values representing temperatures in
> the right notation for the rest of the file.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
I'm not sure adding unicode to source code adds much but OK.

>  	/*
> -	 * Set the critical trip point at 5C under max
> -	 * Set the passive trip point at 10C under max (can change via sysfs)
> +	 * Set the critical trip point at 5 °C under max
> +	 * Set the passive trip point at 10 °C under max (changable via sysfs)

It's spelled "changEable".

Re: [PATCH 4/4] thermal: imx: update to new formula according to NXP AN5215

[Leonard Crestez]

On Tue, 2017-11-21 at 21:02 +0100, Uwe Kleine-König wrote:
> According to an application note from 03/2017 there is an updated formula to
> calculate the temperature that better matches reality. This is implemented here.
> 
> While updating move the magic constants from cpp defines which are far above the
> explaining formula to constants in the code just under the explaining comment.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/thermal/imx_thermal.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> index b24aa6cfebbf..95116475ac98 100644
> --- a/drivers/thermal/imx_thermal.c
> +++ b/drivers/thermal/imx_thermal.c
> @@ -70,10 +70,6 @@ enum imx_thermal_trip {
>  #define IMX_POLLING_DELAY		2000 /* millisecond */
>  #define IMX_PASSIVE_DELAY		1000
>  
> -#define FACTOR0				10000000
> -#define FACTOR1				15976
> -#define FACTOR2				4297157
> -
>  #define TEMPMON_IMX6Q			1
>  #define TEMPMON_IMX6SX			2
>  
> @@ -350,7 +346,7 @@ static struct thermal_zone_device_ops imx_tz_ops = {
>  static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
>  {
>  	struct imx_thermal_data *data = platform_get_drvdata(pdev);
> -	int t1, n1;
> +	int n1;
>  	u64 temp64;
>  
>  	if (ocotp_ana1 == 0 || ocotp_ana1 == ~0) {
> @@ -365,25 +361,25 @@ static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
>  	 * To find the actual temperature T, the following formula has to be used
>  	 * when reading value n from the sensor:
>  	 *
> -	 * T = T1 + (N - N1) / (0.4297157 - 0.0015976 * N1) °C
> -	 *   = [T1 - N1 / (0.4297157 - 0.0015976 * N1) °C] + N / (0.4297157 - 0.0015976 * N1) °C
> -	 *   = [T1 + N1 / (0.0015976 * N1 - 0.4297157) °C] - N / (0.0015976 * N1 - 0.4297157) °C
> +	 * T = T1 + (N - N1) / (0.4148468 - 0.0015423 * N1) °C + 3.580661 °C
> +	 *   = [T1' - N1 / (0.4148468 - 0.0015423 * N1) °C] + N / (0.4148468 - 0.0015423 * N1) °C
> +	 *   = [T1' + N1 / (0.0015423 * N1 - 0.4148468) °C] - N / (0.0015423 * N1 - 0.4148468) °C
>  	 *   = c2 - c1 * N
>  	 *
>  	 * with
>  	 *
> -	 *   c1 = 1 / (0.0015976 * N1 - 0.4297157) °C
> -	 *   c2 = T1 + N1 / (0.0015976 * N1 - 0.4297157) °C
> -	 *      = T1 + N1 * C1
> +	 *  T1' = 28.580661 °C
> +	 *   c1 = 1 / (0.0015423 * N1 - 0.4297157) °C
> +	 *   c2 = T1' + N1 / (0.0015423 * N1 - 0.4148468) °C
> +	 *      = T1' + N1 * c1
>  	 */
>  	n1 = ocotp_ana1 >> 20;
> -	t1 = 25; /* °C */
>  
> -	temp64 = FACTOR0; /* 10^7 for FACTOR1 and FACTOR2 */
> +	temp64 = 10000000; /* use 10^7 as fixed point constant for values in formula */
>  	temp64 *= 1000; /* to get result in °mC */
> -	do_div(temp64, FACTOR1 * n1 - FACTOR2);
> +	do_div(temp64, 15423 * n1 - 4148468);
>  	data->c1 = temp64;
> -	data->c2 = n1 * data->c1 + 1000 * t1;
> +	data->c2 = n1 * data->c1 + 28581;

The freescale tree contains a slightly modified version of this driver
which already includes the new formula. Source here:

http://git.freescale.com/git/cgit.cgi/imx/linux-imx.git/tree/drivers/thermal/imx_thermal.c?h=imx_4.9.11_1.0.0_ga#n540

Your code looks different (and nicer!) but the math seems the same.

Reviewed-by: Leonard Crestez <leonard.crestez@nxp.com>

Re: [PATCH 3/4] thermal: imx: use consistent style to write temperatures

[Uwe Kleine-König]

On Mon, Nov 27, 2017 at 08:40:47PM +0200, Leonard Crestez wrote:
> On Tue, 2017-11-21 at 21:02 +0100, Uwe Kleine-König wrote:
> > 
> > The previous commit already took care to use the right notation for
> > temperatures. Add correct units to all values representing temperatures in
> > the right notation for the rest of the file.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> I'm not sure adding unicode to source code adds much but OK.

Ah, I wasn't aware that ° is not in the ASCII range. AFAIK it doesn't
hurt from the correctness POV for the C compiler. Not sure if all kernel
coders are using utf-8 capable (and correctly configured) editors,
though. But we cannot find out if we don't start using utf-8 :-)

Honestly, I don't care much. If we choose to drop this patch, patch 2
should be changed to not introduce ° either.
 
> > -	 * Set the critical trip point at 5C under max
> > -	 * Set the passive trip point at 10C under max (can change via sysfs)
> > +	 * Set the critical trip point at 5 °C under max
> > +	 * Set the passive trip point at 10 °C under max (changable via sysfs)
> 
> It's spelled "changEable".

I thought I looked that up, but you are right.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/4] thermal: imx: Use better parameter names than "val"

[Uwe Kleine-König]

On Mon, Nov 27, 2017 at 08:39:31PM +0200, Leonard Crestez wrote:
> On Tue, 2017-11-21 at 21:02 +0100, Uwe Kleine-König wrote:
> > The values passed to imx_init_calib() and imx_init_temp_grade() are
> > read from specific OCOTP values. Use their names (in lower case) as
> > parameter name instead of "val" to make the code easier to understand.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/thermal/imx_thermal.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c
> > index e7d4ffc3de7f..21b8c4c4da3c 100644
> > --- a/drivers/thermal/imx_thermal.c
> > +++ b/drivers/thermal/imx_thermal.c
> > @@ -347,13 +347,13 @@ static struct thermal_zone_device_ops imx_tz_ops = {
> >  	.set_trip_temp = imx_set_trip_temp,
> >  };
> >  
> > -static int imx_init_calib(struct platform_device *pdev, u32 val)
> > +static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1)
> >  {
> >  	struct imx_thermal_data *data = platform_get_drvdata(pdev);
> >  	int t1, n1;
> >  	u64 temp64;
> >  
> > -	if (val == 0 || val == ~0) {
> > +	if (ocotp_ana1 == 0 || ocotp_ana1 == ~0) {
> >  		dev_err(&pdev->dev, "invalid sensor calibration data\n");
> >  		return -EINVAL;
> >  	}
> > @@ -364,7 +364,7 @@ static int imx_init_calib(struct platform_device *pdev, u32 val)
> >  	 * Use universal formula now and only need sensor value @ 25C
> >  	 * slope = 0.4297157 - (0.0015976 * 25C fuse)
> >  	 */
> > -	n1 = val >> 20;
> > +	n1 = ocotp_ana1 >> 20;
> >  	t1 = 25; /* t1 always 25C */
> >  
> >  	/*
> > @@ -392,12 +392,12 @@ static int imx_init_calib(struct platform_device *pdev, u32 val)
> >  	return 0;
> >  }
> >  
> > -static void imx_init_temp_grade(struct platform_device *pdev, u32 val)
> > +static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)
> 
> On imx7 (thermal not currently supported in upstream) the temperature
> grade is in OCOTP_TESTER3. So using a more generic parameter name here
> would be preferable or this would have to be changed again later.

Then I suggest to change the semantic and make this:

	imx_init_temp_grade(struct platform_device *pdev, u32 grade)

and let the caller do the bit shift and masking.

Does the same problem exist for ocotp_ana1?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 2/4] thermal: imx: improve comments describing algorithm for temp calculation

[Uwe Kleine-König]

On Mon, Nov 27, 2017 at 08:39:53PM +0200, Leonard Crestez wrote:
> On Tue, 2017-11-21 at 21:02 +0100, Uwe Kleine-König wrote:
> > The description of the implemented algorithm is hardly understandable
> > without having the right application note side-by-side to the code.
> > 
> > Fix this by using shorter and more intuitive variable names, describe
> > their meaning and transform a single formula instead of first talking about
> > slope and then about "milli_Tmeas".
> > 
> > There are no code changes.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Maybe you should reorder so that patches 2 and 4 (which are most
> interesting) are right next to each other?

I don't see added benefit to reshuffle, but as I have to respin the
series, I can do that.

> Reviewed-by: Leonard Crestez <leonard.crestez@nxp.com>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/4] thermal: imx: Use better parameter names than "val"

[Leonard Crestez]

On Mon, 2017-11-27 at 20:50 +0100, Uwe Kleine-König wrote:
> On Mon, Nov 27, 2017 at 08:39:31PM +0200, Leonard Crestez wrote:
> > On Tue, 2017-11-21 at 21:02 +0100, Uwe Kleine-König wrote:
> > > 
> > > The values passed to imx_init_calib() and imx_init_temp_grade() are
> > > read from specific OCOTP values. Use their names (in lower case) as
> > > parameter name instead of "val" to make the code easier to understand.
> > > 

> > > -static void imx_init_temp_grade(struct platform_device *pdev, u32 val)
> > > +static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0)

> > On imx7 (thermal not currently supported in upstream) the temperature
> > grade is in OCOTP_TESTER3. So using a more generic parameter name here
> > would be preferable or this would have to be changed again later.

> Then I suggest to change the semantic and make this:
> 
> 	imx_init_temp_grade(struct platform_device *pdev, u32 grade)
> 
> and let the caller do the bit shift and masking.

That would work. It just so happens that the grade is stored in the
same bits.

It might be better to deal with this when adding imx7 thermal support
instead.

> Does the same problem exist for ocotp_ana1?

No, not that I know of.