Re: [PATCH 2/3] thermal: rcar_thermal: Do not store ctemp in rcar_thermal_priv

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
Cc: Linux PM list <linux-pm@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Subject: Re: [PATCH 2/3] thermal: rcar_thermal: Do not store ctemp in rcar_thermal_priv
Date: Wed, 11 Mar 2020 13:40:46 +0100	[thread overview]
Message-ID: <CAMuHMdU8VUf9deXmpSD=P9TQEL0s0xspHW2BdTtS=o-ZsFzGOw@mail.gmail.com> (raw)
In-Reply-To: <20200310170029.1648996-3-niklas.soderlund+renesas@ragnatech.se>

Hi Niklas,

On Tue, Mar 10, 2020 at 6:02 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> There is no need to cache the ctemp value in the private data structure
> as it's always prefetched before it's used. Remove it from the structure
> and have rcar_thermal_update_temp return the value instead of storing
> it.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Thanks for your patch!

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

One suggestion below...

> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -201,7 +200,6 @@ 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 ret = -EINVAL;

It might make sense to change ctemp to int, and preinitialize it to
-EINVAL...

>
>         mutex_lock(&priv->lock);
>
> @@ -247,32 +245,28 @@ static int rcar_thermal_update_temp(struct rcar_thermal_priv *priv)
>                                                    ((ctemp - 1) << 0)));
>         }
>
> -       dev_dbg(dev, "thermal%d  %d -> %d\n", priv->id, priv->ctemp, ctemp);
> -
> -       priv->ctemp = ctemp;
> -       ret = 0;
>  err_out_unlock:
>         mutex_unlock(&priv->lock);
> -       return ret;
> +
> +       return ctemp ? ctemp : -EINVAL;

... so you can just return ctemp here.

>  }

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