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@xxxxxxx>
> > ---
> >   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.