[PATCH] thermal/drivers/qcom: Code refactoring

[PATCH] thermal/drivers/qcom: Code refactoring

[Carlos Bilbao]

Some functions in tsens-8960.c can directly return ret instead of doing an
extra check. In function calibrate_8960(), a second check for IS_ERR(data)
can also be avoided in some cases. A constant could be used to represent
the maximum number of sensors (11). Finally, function code_to_degc() can be
simplified, avoiding using an extra variable.

Include these small refactoring changes.

Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
---
 drivers/thermal/qcom/tsens-8960.c   | 25 +++++++++----------------
 drivers/thermal/qcom/tsens-common.c | 18 ++++++++----------
 drivers/thermal/qcom/tsens-v0_1.c   |  6 +++---
 drivers/thermal/qcom/tsens-v1.c     |  2 +-
 drivers/thermal/qcom/tsens.h        |  1 +
 5 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
index 8d9b721dadb6..576bca871655 100644
--- a/drivers/thermal/qcom/tsens-8960.c
+++ b/drivers/thermal/qcom/tsens-8960.c
@@ -76,10 +76,8 @@ static int suspend_8960(struct tsens_priv *priv)
 		mask = SLP_CLK_ENA_8660 | EN;
 
 	ret = regmap_update_bits(map, CNTL_ADDR, mask, 0);
-	if (ret)
-		return ret;
 
-	return 0;
+	return ret;
 }
 
 static int resume_8960(struct tsens_priv *priv)
@@ -106,10 +104,8 @@ static int resume_8960(struct tsens_priv *priv)
 		return ret;
 
 	ret = regmap_write(map, CNTL_ADDR, priv->ctx.control);
-	if (ret)
-		return ret;
 
-	return 0;
+	return ret;
 }
 
 static int enable_8960(struct tsens_priv *priv, int id)
@@ -132,10 +128,8 @@ static int enable_8960(struct tsens_priv *priv, int id)
 		reg |= mask | SLP_CLK_ENA_8660 | EN;
 
 	ret = regmap_write(priv->tm_map, CNTL_ADDR, reg);
-	if (ret)
-		return ret;
 
-	return 0;
+	return ret;
 }
 
 static void disable_8960(struct tsens_priv *priv)
@@ -206,10 +200,8 @@ static int init_8960(struct tsens_priv *priv)
 
 	reg_cntl |= EN;
 	ret = regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl);
-	if (ret)
-		return ret;
 
-	return 0;
+	return ret;
 }
 
 static int calibrate_8960(struct tsens_priv *priv)
@@ -221,10 +213,11 @@ static int calibrate_8960(struct tsens_priv *priv)
 	struct tsens_sensor *s = priv->sensor;
 
 	data = qfprom_read(priv->dev, "calib");
-	if (IS_ERR(data))
+	if (IS_ERR(data)) {
 		data = qfprom_read(priv->dev, "calib_backup");
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+		if (IS_ERR(data))
+			return PTR_ERR(data);
+	}
 
 	for (i = 0; i < num_read; i++, s++)
 		s->offset = data[i];
@@ -278,6 +271,6 @@ static const struct tsens_ops ops_8960 = {
 };
 
 const struct tsens_plat_data data_8960 = {
-	.num_sensors	= 11,
+	.num_sensors	= MAX_NUM_SENSORS,
 	.ops		= &ops_8960,
 };
diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
index 528df8801254..fe5f4459e1cc 100644
--- a/drivers/thermal/qcom/tsens-common.c
+++ b/drivers/thermal/qcom/tsens-common.c
@@ -66,19 +66,17 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,
 
 static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
 {
-	int degc, num, den;
+	int degc, den;
 
-	num = (adc_code * SLOPE_FACTOR) - s->offset;
+	degc = (adc_code * SLOPE_FACTOR) - s->offset;
 	den = s->slope;
 
-	if (num > 0)
-		degc = num + (den / 2);
-	else if (num < 0)
-		degc = num - (den / 2);
-	else
-		degc = num;
-
-	degc /= den;
+	if (degc != 0) {
+		if (degc > 0)
+			degc = (degc + (den / 2)) / den;
+		else
+			degc = (degc - (den / 2)) / den;
+	}
 
 	return degc;
 }
diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c
index 6f26fadf4c27..42e897526345 100644
--- a/drivers/thermal/qcom/tsens-v0_1.c
+++ b/drivers/thermal/qcom/tsens-v0_1.c
@@ -188,7 +188,7 @@ static int calibrate_8916(struct tsens_priv *priv)
 static int calibrate_8974(struct tsens_priv *priv)
 {
 	int base1 = 0, base2 = 0, i;
-	u32 p1[11], p2[11];
+	u32 p1[MAX_NUM_SENSORS], p2[MAX_NUM_SENSORS];
 	int mode = 0;
 	u32 *calib, *bkp;
 	u32 calib_redun_sel;
@@ -324,7 +324,7 @@ static const struct tsens_features tsens_v0_1_feat = {
 	.crit_int	= 0,
 	.adc		= 1,
 	.srot_split	= 1,
-	.max_sensors	= 11,
+	.max_sensors	= MAX_NUM_SENSORS,
 };
 
 static const struct reg_field tsens_v0_1_regfields[MAX_REGFIELDS] = {
@@ -374,7 +374,7 @@ static const struct tsens_ops ops_8974 = {
 };
 
 const struct tsens_plat_data data_8974 = {
-	.num_sensors	= 11,
+	.num_sensors	= MAX_NUM_SENSORS,
 	.ops		= &ops_8974,
 	.feat		= &tsens_v0_1_feat,
 	.fields	= tsens_v0_1_regfields,
diff --git a/drivers/thermal/qcom/tsens-v1.c b/drivers/thermal/qcom/tsens-v1.c
index 10b595d4f619..98acc9b64555 100644
--- a/drivers/thermal/qcom/tsens-v1.c
+++ b/drivers/thermal/qcom/tsens-v1.c
@@ -149,7 +149,7 @@ static const struct tsens_features tsens_v1_feat = {
 	.crit_int	= 0,
 	.adc		= 1,
 	.srot_split	= 1,
-	.max_sensors	= 11,
+	.max_sensors	= MAX_NUM_SENSORS,
 };
 
 static const struct reg_field tsens_v1_regfields[MAX_REGFIELDS] = {
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index 2fd94997245b..d2d78c7e20c8 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -6,6 +6,7 @@
 #ifndef __QCOM_TSENS_H__
 #define __QCOM_TSENS_H__
 
+#define MAX_NUM_SENSORS		11
 #define ONE_PT_CALIB		0x1
 #define ONE_PT_CALIB2		0x2
 #define TWO_PT_CALIB		0x3
-- 
2.31.1

Re: [PATCH] thermal/drivers/qcom: Code refactoring

[Konrad Dybcio]

On 12.07.2022 19:31, Carlos Bilbao wrote:
> Some functions in tsens-8960.c can directly return ret instead of doing an
> extra check. In function calibrate_8960(), a second check for IS_ERR(data)
> can also be avoided in some cases. A constant could be used to represent
> the maximum number of sensors (11). Finally, function code_to_degc() can be
> simplified, avoiding using an extra variable.
> 
> Include these small refactoring changes.
> 
> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
> ---
>  drivers/thermal/qcom/tsens-8960.c   | 25 +++++++++----------------
>  drivers/thermal/qcom/tsens-common.c | 18 ++++++++----------
>  drivers/thermal/qcom/tsens-v0_1.c   |  6 +++---
>  drivers/thermal/qcom/tsens-v1.c     |  2 +-
>  drivers/thermal/qcom/tsens.h        |  1 +
>  5 files changed, 22 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
> index 8d9b721dadb6..576bca871655 100644
> --- a/drivers/thermal/qcom/tsens-8960.c
> +++ b/drivers/thermal/qcom/tsens-8960.c
> @@ -76,10 +76,8 @@ static int suspend_8960(struct tsens_priv *priv)
>  		mask = SLP_CLK_ENA_8660 | EN;
>  
>  	ret = regmap_update_bits(map, CNTL_ADDR, mask, 0);
> -	if (ret)
> -		return ret;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int resume_8960(struct tsens_priv *priv)
> @@ -106,10 +104,8 @@ static int resume_8960(struct tsens_priv *priv)
>  		return ret;
>  
>  	ret = regmap_write(map, CNTL_ADDR, priv->ctx.control);
> -	if (ret)
> -		return ret;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int enable_8960(struct tsens_priv *priv, int id)
> @@ -132,10 +128,8 @@ static int enable_8960(struct tsens_priv *priv, int id)
>  		reg |= mask | SLP_CLK_ENA_8660 | EN;
>  
>  	ret = regmap_write(priv->tm_map, CNTL_ADDR, reg);
> -	if (ret)
> -		return ret;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void disable_8960(struct tsens_priv *priv)
> @@ -206,10 +200,8 @@ static int init_8960(struct tsens_priv *priv)
>  
>  	reg_cntl |= EN;
>  	ret = regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl);
> -	if (ret)
> -		return ret;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int calibrate_8960(struct tsens_priv *priv)
> @@ -221,10 +213,11 @@ static int calibrate_8960(struct tsens_priv *priv)
>  	struct tsens_sensor *s = priv->sensor;
>  
>  	data = qfprom_read(priv->dev, "calib");
> -	if (IS_ERR(data))
> +	if (IS_ERR(data)) {
>  		data = qfprom_read(priv->dev, "calib_backup");
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +		if (IS_ERR(data))
> +			return PTR_ERR(data);
> +	}
>  
>  	for (i = 0; i < num_read; i++, s++)
>  		s->offset = data[i];
> @@ -278,6 +271,6 @@ static const struct tsens_ops ops_8960 = {
>  };
>  
>  const struct tsens_plat_data data_8960 = {
> -	.num_sensors	= 11,
> +	.num_sensors	= MAX_NUM_SENSORS,
>  	.ops		= &ops_8960,
>  };
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index 528df8801254..fe5f4459e1cc 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -66,19 +66,17 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,
>  
>  static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
>  {
> -	int degc, num, den;
> +	int degc, den;
>  
> -	num = (adc_code * SLOPE_FACTOR) - s->offset;
> +	degc = (adc_code * SLOPE_FACTOR) - s->offset;
>  	den = s->slope;
>  
> -	if (num > 0)
> -		degc = num + (den / 2);
> -	else if (num < 0)
> -		degc = num - (den / 2);
> -	else
> -		degc = num;
> -
> -	degc /= den;
> +	if (degc != 0) {
> +		if (degc > 0)
> +			degc = (degc + (den / 2)) / den;
> +		else
> +			degc = (degc - (den / 2)) / den;
> +	}
>  
>  	return degc;
>  }
> diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c
> index 6f26fadf4c27..42e897526345 100644
> --- a/drivers/thermal/qcom/tsens-v0_1.c
> +++ b/drivers/thermal/qcom/tsens-v0_1.c
> @@ -188,7 +188,7 @@ static int calibrate_8916(struct tsens_priv *priv)
>  static int calibrate_8974(struct tsens_priv *priv)
>  {
>  	int base1 = 0, base2 = 0, i;
> -	u32 p1[11], p2[11];
> +	u32 p1[MAX_NUM_SENSORS], p2[MAX_NUM_SENSORS];
>  	int mode = 0;
>  	u32 *calib, *bkp;
>  	u32 calib_redun_sel;
> @@ -324,7 +324,7 @@ static const struct tsens_features tsens_v0_1_feat = {
>  	.crit_int	= 0,
>  	.adc		= 1,
>  	.srot_split	= 1,
> -	.max_sensors	= 11,
> +	.max_sensors	= MAX_NUM_SENSORS,
>  };
>  
>  static const struct reg_field tsens_v0_1_regfields[MAX_REGFIELDS] = {
> @@ -374,7 +374,7 @@ static const struct tsens_ops ops_8974 = {
>  };
>  
>  const struct tsens_plat_data data_8974 = {
> -	.num_sensors	= 11,
> +	.num_sensors	= MAX_NUM_SENSORS,
>  	.ops		= &ops_8974,
>  	.feat		= &tsens_v0_1_feat,
>  	.fields	= tsens_v0_1_regfields,
> diff --git a/drivers/thermal/qcom/tsens-v1.c b/drivers/thermal/qcom/tsens-v1.c
> index 10b595d4f619..98acc9b64555 100644
> --- a/drivers/thermal/qcom/tsens-v1.c
> +++ b/drivers/thermal/qcom/tsens-v1.c
> @@ -149,7 +149,7 @@ static const struct tsens_features tsens_v1_feat = {
>  	.crit_int	= 0,
>  	.adc		= 1,
>  	.srot_split	= 1,
> -	.max_sensors	= 11,
> +	.max_sensors	= MAX_NUM_SENSORS,
>  };
>  
>  static const struct reg_field tsens_v1_regfields[MAX_REGFIELDS] = {
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index 2fd94997245b..d2d78c7e20c8 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -6,6 +6,7 @@
>  #ifndef __QCOM_TSENS_H__
>  #define __QCOM_TSENS_H__
>  
> +#define MAX_NUM_SENSORS		11
Hi, this number matching up for v0.1 and v1 is purely coincidental
and will change when more platforms are added. Please keep them
separate to avoid confusion in the future.

Konrad
>  #define ONE_PT_CALIB		0x1
>  #define ONE_PT_CALIB2		0x2
>  #define TWO_PT_CALIB		0x3
> 

Re: [PATCH] thermal/drivers/qcom: Code refactoring

[Bjorn Andersson]

On Tue 12 Jul 12:31 CDT 2022, Carlos Bilbao wrote:

> Some functions in tsens-8960.c can directly return ret instead of doing an
> extra check. In function calibrate_8960(), a second check for IS_ERR(data)
> can also be avoided in some cases. A constant could be used to represent
> the maximum number of sensors (11). Finally, function code_to_degc() can be
> simplified, avoiding using an extra variable.
> 

Thanks for the patch Carlos. These are rather small fixes, but it would
still be nice to keep them separate, so that in the even of there being
some unforseen regression it would be easy to track down and fix the
relevant patch.

> Include these small refactoring changes.
> 
> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
> ---
>  drivers/thermal/qcom/tsens-8960.c   | 25 +++++++++----------------
>  drivers/thermal/qcom/tsens-common.c | 18 ++++++++----------
>  drivers/thermal/qcom/tsens-v0_1.c   |  6 +++---
>  drivers/thermal/qcom/tsens-v1.c     |  2 +-
>  drivers/thermal/qcom/tsens.h        |  1 +
>  5 files changed, 22 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
> index 8d9b721dadb6..576bca871655 100644
> --- a/drivers/thermal/qcom/tsens-8960.c
> +++ b/drivers/thermal/qcom/tsens-8960.c
> @@ -76,10 +76,8 @@ static int suspend_8960(struct tsens_priv *priv)
>  		mask = SLP_CLK_ENA_8660 | EN;
>  
>  	ret = regmap_update_bits(map, CNTL_ADDR, mask, 0);

Why not just do:

	return regmap_writen(...);

> -	if (ret)
> -		return ret;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int resume_8960(struct tsens_priv *priv)
> @@ -106,10 +104,8 @@ static int resume_8960(struct tsens_priv *priv)
>  		return ret;
>  
>  	ret = regmap_write(map, CNTL_ADDR, priv->ctx.control);
> -	if (ret)
> -		return ret;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int enable_8960(struct tsens_priv *priv, int id)
> @@ -132,10 +128,8 @@ static int enable_8960(struct tsens_priv *priv, int id)
>  		reg |= mask | SLP_CLK_ENA_8660 | EN;
>  
>  	ret = regmap_write(priv->tm_map, CNTL_ADDR, reg);
> -	if (ret)
> -		return ret;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void disable_8960(struct tsens_priv *priv)
> @@ -206,10 +200,8 @@ static int init_8960(struct tsens_priv *priv)
>  
>  	reg_cntl |= EN;
>  	ret = regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl);
> -	if (ret)
> -		return ret;
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int calibrate_8960(struct tsens_priv *priv)
> @@ -221,10 +213,11 @@ static int calibrate_8960(struct tsens_priv *priv)
>  	struct tsens_sensor *s = priv->sensor;
>  
>  	data = qfprom_read(priv->dev, "calib");
> -	if (IS_ERR(data))
> +	if (IS_ERR(data)) {
>  		data = qfprom_read(priv->dev, "calib_backup");
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +		if (IS_ERR(data))
> +			return PTR_ERR(data);
> +	}
>  
>  	for (i = 0; i < num_read; i++, s++)
>  		s->offset = data[i];
> @@ -278,6 +271,6 @@ static const struct tsens_ops ops_8960 = {
>  };
>  
>  const struct tsens_plat_data data_8960 = {
> -	.num_sensors	= 11,
> +	.num_sensors	= MAX_NUM_SENSORS,
>  	.ops		= &ops_8960,
>  };
> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
> index 528df8801254..fe5f4459e1cc 100644
> --- a/drivers/thermal/qcom/tsens-common.c
> +++ b/drivers/thermal/qcom/tsens-common.c
> @@ -66,19 +66,17 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,
>  
>  static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
>  {
> -	int degc, num, den;
> +	int degc, den;
>  
> -	num = (adc_code * SLOPE_FACTOR) - s->offset;
> +	degc = (adc_code * SLOPE_FACTOR) - s->offset;

At this point the variable name is misleading, it's not until you have
reassigned degc below that it's value represent the temperature.

>  	den = s->slope;
>  
> -	if (num > 0)
> -		degc = num + (den / 2);
> -	else if (num < 0)
> -		degc = num - (den / 2);
> -	else
> -		degc = num;

So the main part of this change is to rework the else case, how about
just starting with:

	if (!num)
		return 0;

> -
> -	degc /= den;
> +	if (degc != 0) {
> +		if (degc > 0)
> +			degc = (degc + (den / 2)) / den;
> +		else
> +			degc = (degc - (den / 2)) / den;
> +	}
>  
>  	return degc;
>  }
> diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c
> index 6f26fadf4c27..42e897526345 100644
> --- a/drivers/thermal/qcom/tsens-v0_1.c
> +++ b/drivers/thermal/qcom/tsens-v0_1.c
> @@ -188,7 +188,7 @@ static int calibrate_8916(struct tsens_priv *priv)
>  static int calibrate_8974(struct tsens_priv *priv)
>  {
>  	int base1 = 0, base2 = 0, i;
> -	u32 p1[11], p2[11];
> +	u32 p1[MAX_NUM_SENSORS], p2[MAX_NUM_SENSORS];
>  	int mode = 0;
>  	u32 *calib, *bkp;
>  	u32 calib_redun_sel;
> @@ -324,7 +324,7 @@ static const struct tsens_features tsens_v0_1_feat = {
>  	.crit_int	= 0,
>  	.adc		= 1,
>  	.srot_split	= 1,
> -	.max_sensors	= 11,
> +	.max_sensors	= MAX_NUM_SENSORS,
>  };
>  
>  static const struct reg_field tsens_v0_1_regfields[MAX_REGFIELDS] = {
> @@ -374,7 +374,7 @@ static const struct tsens_ops ops_8974 = {
>  };
>  
>  const struct tsens_plat_data data_8974 = {
> -	.num_sensors	= 11,
> +	.num_sensors	= MAX_NUM_SENSORS,
>  	.ops		= &ops_8974,
>  	.feat		= &tsens_v0_1_feat,
>  	.fields	= tsens_v0_1_regfields,
> diff --git a/drivers/thermal/qcom/tsens-v1.c b/drivers/thermal/qcom/tsens-v1.c
> index 10b595d4f619..98acc9b64555 100644
> --- a/drivers/thermal/qcom/tsens-v1.c
> +++ b/drivers/thermal/qcom/tsens-v1.c
> @@ -149,7 +149,7 @@ static const struct tsens_features tsens_v1_feat = {
>  	.crit_int	= 0,
>  	.adc		= 1,
>  	.srot_split	= 1,
> -	.max_sensors	= 11,
> +	.max_sensors	= MAX_NUM_SENSORS,
>  };
>  
>  static const struct reg_field tsens_v1_regfields[MAX_REGFIELDS] = {
> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> index 2fd94997245b..d2d78c7e20c8 100644
> --- a/drivers/thermal/qcom/tsens.h
> +++ b/drivers/thermal/qcom/tsens.h
> @@ -6,6 +6,7 @@
>  #ifndef __QCOM_TSENS_H__
>  #define __QCOM_TSENS_H__
>  
> +#define MAX_NUM_SENSORS		11

This only seems to apply for the three cases you have listed here, e.g.
tsens-v2 (which also includes tsens.h) has max_sensors = 16.

Regards,
Bjorn

>  #define ONE_PT_CALIB		0x1
>  #define ONE_PT_CALIB2		0x2
>  #define TWO_PT_CALIB		0x3
> -- 
> 2.31.1
> 

Re: [PATCH] thermal/drivers/qcom: Code refactoring

[Carlos Bilbao]

On 7/12/22 23:30, Bjorn Andersson wrote:

> On Tue 12 Jul 12:31 CDT 2022, Carlos Bilbao wrote:
>
>> Some functions in tsens-8960.c can directly return ret instead of doing an
>> extra check. In function calibrate_8960(), a second check for IS_ERR(data)
>> can also be avoided in some cases. A constant could be used to represent
>> the maximum number of sensors (11). Finally, function code_to_degc() can be
>> simplified, avoiding using an extra variable.
>>
> Thanks for the patch Carlos. These are rather small fixes, but it would
> still be nice to keep them separate, so that in the even of there being
> some unforseen regression it would be easy to track down and fix the
> relevant patch.
Thanks for your comments Bjorn and Konrad. From what you say, I believe it
would be good to have this as a small refactoring patchset that includes:

1/4 Simplify return values.
2/4 Simplify function code_to_degc().
3/4 Simplify function calibrate_8960().
4/4 Create array MAX_NUM_SENSORS[] for maximum number of sensors per
version (v0.1, v1, v2), that can be further populated when future versions
appear.
>> Include these small refactoring changes.
>>
>> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com>
>> ---
>>   drivers/thermal/qcom/tsens-8960.c   | 25 +++++++++----------------
>>   drivers/thermal/qcom/tsens-common.c | 18 ++++++++----------
>>   drivers/thermal/qcom/tsens-v0_1.c   |  6 +++---
>>   drivers/thermal/qcom/tsens-v1.c     |  2 +-
>>   drivers/thermal/qcom/tsens.h        |  1 +
>>   5 files changed, 22 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/thermal/qcom/tsens-8960.c b/drivers/thermal/qcom/tsens-8960.c
>> index 8d9b721dadb6..576bca871655 100644
>> --- a/drivers/thermal/qcom/tsens-8960.c
>> +++ b/drivers/thermal/qcom/tsens-8960.c
>> @@ -76,10 +76,8 @@ static int suspend_8960(struct tsens_priv *priv)
>>   		mask = SLP_CLK_ENA_8660 | EN;
>>   
>>   	ret = regmap_update_bits(map, CNTL_ADDR, mask, 0);
> Why not just do:
>
> 	return regmap_writen(...);

Yep, should be part of patch 1/4.

>> -	if (ret)
>> -		return ret;
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   static int resume_8960(struct tsens_priv *priv)
>> @@ -106,10 +104,8 @@ static int resume_8960(struct tsens_priv *priv)
>>   		return ret;
>>   
>>   	ret = regmap_write(map, CNTL_ADDR, priv->ctx.control);
>> -	if (ret)
>> -		return ret;
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   static int enable_8960(struct tsens_priv *priv, int id)
>> @@ -132,10 +128,8 @@ static int enable_8960(struct tsens_priv *priv, int id)
>>   		reg |= mask | SLP_CLK_ENA_8660 | EN;
>>   
>>   	ret = regmap_write(priv->tm_map, CNTL_ADDR, reg);
>> -	if (ret)
>> -		return ret;
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   static void disable_8960(struct tsens_priv *priv)
>> @@ -206,10 +200,8 @@ static int init_8960(struct tsens_priv *priv)
>>   
>>   	reg_cntl |= EN;
>>   	ret = regmap_write(priv->tm_map, CNTL_ADDR, reg_cntl);
>> -	if (ret)
>> -		return ret;
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   static int calibrate_8960(struct tsens_priv *priv)
>> @@ -221,10 +213,11 @@ static int calibrate_8960(struct tsens_priv *priv)
>>   	struct tsens_sensor *s = priv->sensor;
>>   
>>   	data = qfprom_read(priv->dev, "calib");
>> -	if (IS_ERR(data))
>> +	if (IS_ERR(data)) {
>>   		data = qfprom_read(priv->dev, "calib_backup");
>> -	if (IS_ERR(data))
>> -		return PTR_ERR(data);
>> +		if (IS_ERR(data))
>> +			return PTR_ERR(data);
>> +	}
>>   
>>   	for (i = 0; i < num_read; i++, s++)
>>   		s->offset = data[i];
>> @@ -278,6 +271,6 @@ static const struct tsens_ops ops_8960 = {
>>   };
>>   
>>   const struct tsens_plat_data data_8960 = {
>> -	.num_sensors	= 11,
>> +	.num_sensors	= MAX_NUM_SENSORS,
>>   	.ops		= &ops_8960,
>>   };
>> diff --git a/drivers/thermal/qcom/tsens-common.c b/drivers/thermal/qcom/tsens-common.c
>> index 528df8801254..fe5f4459e1cc 100644
>> --- a/drivers/thermal/qcom/tsens-common.c
>> +++ b/drivers/thermal/qcom/tsens-common.c
>> @@ -66,19 +66,17 @@ void compute_intercept_slope(struct tsens_priv *priv, u32 *p1,
>>   
>>   static inline int code_to_degc(u32 adc_code, const struct tsens_sensor *s)
>>   {
>> -	int degc, num, den;
>> +	int degc, den;
>>   
>> -	num = (adc_code * SLOPE_FACTOR) - s->offset;
>> +	degc = (adc_code * SLOPE_FACTOR) - s->offset;
> At this point the variable name is misleading, it's not until you have
> reassigned degc below that it's value represent the temperature.
>
Sure, will rename.
>>   	den = s->slope;
>>   
>> -	if (num > 0)
>> -		degc = num + (den / 2);
>> -	else if (num < 0)
>> -		degc = num - (den / 2);
>> -	else
>> -		degc = num;
> So the main part of this change is to rework the else case, how about
> just starting with:
>
> 	if (!num)
> 		return 0;
Makes sense.
>
>> -
>> -	degc /= den;
>> +	if (degc != 0) {
>> +		if (degc > 0)
>> +			degc = (degc + (den / 2)) / den;
>> +		else
>> +			degc = (degc - (den / 2)) / den;
>> +	}
>>   
>>   	return degc;
>>   }
>> diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c
>> index 6f26fadf4c27..42e897526345 100644
>> --- a/drivers/thermal/qcom/tsens-v0_1.c
>> +++ b/drivers/thermal/qcom/tsens-v0_1.c
>> @@ -188,7 +188,7 @@ static int calibrate_8916(struct tsens_priv *priv)
>>   static int calibrate_8974(struct tsens_priv *priv)
>>   {
>>   	int base1 = 0, base2 = 0, i;
>> -	u32 p1[11], p2[11];
>> +	u32 p1[MAX_NUM_SENSORS], p2[MAX_NUM_SENSORS];
>>   	int mode = 0;
>>   	u32 *calib, *bkp;
>>   	u32 calib_redun_sel;
>> @@ -324,7 +324,7 @@ static const struct tsens_features tsens_v0_1_feat = {
>>   	.crit_int	= 0,
>>   	.adc		= 1,
>>   	.srot_split	= 1,
>> -	.max_sensors	= 11,
>> +	.max_sensors	= MAX_NUM_SENSORS,
>>   };
>>   
>>   static const struct reg_field tsens_v0_1_regfields[MAX_REGFIELDS] = {
>> @@ -374,7 +374,7 @@ static const struct tsens_ops ops_8974 = {
>>   };
>>   
>>   const struct tsens_plat_data data_8974 = {
>> -	.num_sensors	= 11,
>> +	.num_sensors	= MAX_NUM_SENSORS,
>>   	.ops		= &ops_8974,
>>   	.feat		= &tsens_v0_1_feat,
>>   	.fields	= tsens_v0_1_regfields,
>> diff --git a/drivers/thermal/qcom/tsens-v1.c b/drivers/thermal/qcom/tsens-v1.c
>> index 10b595d4f619..98acc9b64555 100644
>> --- a/drivers/thermal/qcom/tsens-v1.c
>> +++ b/drivers/thermal/qcom/tsens-v1.c
>> @@ -149,7 +149,7 @@ static const struct tsens_features tsens_v1_feat = {
>>   	.crit_int	= 0,
>>   	.adc		= 1,
>>   	.srot_split	= 1,
>> -	.max_sensors	= 11,
>> +	.max_sensors	= MAX_NUM_SENSORS,
>>   };
>>   
>>   static const struct reg_field tsens_v1_regfields[MAX_REGFIELDS] = {
>> diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
>> index 2fd94997245b..d2d78c7e20c8 100644
>> --- a/drivers/thermal/qcom/tsens.h
>> +++ b/drivers/thermal/qcom/tsens.h
>> @@ -6,6 +6,7 @@
>>   #ifndef __QCOM_TSENS_H__
>>   #define __QCOM_TSENS_H__
>>   
>> +#define MAX_NUM_SENSORS		11
> This only seems to apply for the three cases you have listed here, e.g.
> tsens-v2 (which also includes tsens.h) has max_sensors = 16.

This should be an array with an enum for the versions.

> Regards,
> Bjorn
>
>>   #define ONE_PT_CALIB		0x1
>>   #define ONE_PT_CALIB2		0x2
>>   #define TWO_PT_CALIB		0x3
>> -- 
>> 2.31.1
>>

Thanks,

Carlos.

Re: [PATCH] thermal/drivers/qcom: Code refactoring

[Bjorn Andersson]

On Wed 13 Jul 09:10 CDT 2022, Carlos Bilbao wrote:

> On 7/12/22 23:30, Bjorn Andersson wrote:
> 
> > On Tue 12 Jul 12:31 CDT 2022, Carlos Bilbao wrote:
[..]
> > > diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
> > > index 2fd94997245b..d2d78c7e20c8 100644
> > > --- a/drivers/thermal/qcom/tsens.h
> > > +++ b/drivers/thermal/qcom/tsens.h
> > > @@ -6,6 +6,7 @@
> > >   #ifndef __QCOM_TSENS_H__
> > >   #define __QCOM_TSENS_H__
> > > +#define MAX_NUM_SENSORS		11
> > This only seems to apply for the three cases you have listed here, e.g.
> > tsens-v2 (which also includes tsens.h) has max_sensors = 16.
> 
> This should be an array with an enum for the versions.
> 

As the numbers are used within each c-file, some defines at the top of
these would probably be a good idea.

Regards,
Bjorn