Hi Niklas,
On Wed, Apr 15, 2020 at 7:23 PM Niklas Söderlund
<niklas.soderlund+renesas@xxxxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
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@xxxxxxxxxxxxxx
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
