[PATCH] thermal: rcar_thermal: Clean up rcar_thermal_update_temp()

[PATCH] thermal: rcar_thermal: Clean up rcar_thermal_update_temp()

[Niklas Söderlund]

Moving the ctemp variable out of the private data structure made it
possible to clean up rcar_thermal_update_temp(). Initialize the local
ctemp to the error code to return if the reading fails and just return
it at the end of the function.

It's OK to change the datatype of old, new and ctemp to int as all
values are AND with CTEMP (0x3f) before being stored. While at it
change the datatype of the loop variable 'i' to to unsigned int.

Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/thermal/rcar_thermal.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index e0c1f2409035e2bb..3c0c86b0664ec57f 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -198,8 +198,8 @@ static void _rcar_thermal_bset(struct rcar_thermal_priv *priv, u32 reg,
 static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
 {
 	struct device *dev = rcar_priv_to_dev(priv);
-	int i;
-	u32 ctemp, old, new;
+	int old, new, ctemp = -EINVAL;
+	unsigned int i;
 
 	mutex_lock(&priv->lock);
 
@@ -209,7 +209,6 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
 	 */
 	rcar_thermal_bset(priv, THSCR, CPCTL, CPCTL);
 
-	ctemp = 0;
 	old = ~0;
 	for (i = 0; i < 128; i++) {
 		/*
@@ -227,7 +226,7 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
 		old = new;
 	}
 
-	if (!ctemp) {
+	if (ctemp == -EINVAL) {
 		dev_err(dev, "thermal sensor was broken\n");
 		goto err_out_unlock;
 	}
@@ -248,7 +247,7 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
 err_out_unlock:
 	mutex_unlock(&priv->lock);
 
-	return ctemp ? ctemp : -EINVAL;
+	return ctemp;
 }
 
 static int rcar_thermal_get_current_temp(struct rcar_thermal_priv *priv,
-- 
2.26.0

Re: [PATCH] thermal: rcar_thermal: Clean up rcar_thermal_update_temp()

[Geert Uytterhoeven]

Hi Niklas,

On Wed, Apr 15, 2020 at 7:23 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Moving the ctemp variable out of the private data structure made it
> possible to clean up rcar_thermal_update_temp(). Initialize the local
> ctemp to the error code to return if the reading fails and just return
> it at the end of the function.
>
> It's OK to change the datatype of old, new and ctemp to int as all
> values are AND with CTEMP (0x3f) before being stored. While at it

ANDed

> change the datatype of the loop variable 'i' to to unsigned int.
>
> Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

One small suggestion below.

> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -198,8 +198,8 @@ static void _rcar_thermal_bset(struct rcar_thermal_priv *priv, u32 reg,
>  static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
>  {
>         struct device *dev = rcar_priv_to_dev(priv);
> -       int i;
> -       u32 ctemp, old, new;
> +       int old, new, ctemp = -EINVAL;
> +       unsigned int i;
>
>         mutex_lock(&priv->lock);
>
> @@ -209,7 +209,6 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
>          */
>         rcar_thermal_bset(priv, THSCR, CPCTL, CPCTL);
>
> -       ctemp = 0;
>         old = ~0;
>         for (i = 0; i < 128; i++) {
>                 /*
> @@ -227,7 +226,7 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
>                 old = new;
>         }
>
> -       if (!ctemp) {
> +       if (ctemp == -EINVAL) {

I think "if (ctemp < 0)" is safer, as it doesn't rely on the actual error code.

>                 dev_err(dev, "thermal sensor was broken\n");
>                 goto err_out_unlock;
>         }
> @@ -248,7 +247,7 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
>  err_out_unlock:
>         mutex_unlock(&priv->lock);
>
> -       return ctemp ? ctemp : -EINVAL;
> +       return ctemp;
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds