Re: [PATCH repost] thermal: rcar_thermal: update calculation formula for E3

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Niklas,

On Thu, Apr 18, 2019 at 10:12 AM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2019-04-18 09:15:14 +0200, Simon Horman wrote:
> > From: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> >
> > HW manual changes temperature calculation formula for E3:
>
> Is this not also true for V3M and D3?
>
> > - When CTEMP is less than 24
> >    T = CTEMP[5:0] * 5.5 - 72
> > - When CTEMP is equal to/greater than 24
> >    T = CTEMP[5:0] * 5 - 60
> >
> > This was inspired by a patch in the BSP by Van Do <van.do.xw@renesas.com>
> >
> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > Tested-by: Simon Horman <horms+renesas@verge.net.au>
> > Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > ---
> >  drivers/thermal/rcar_thermal.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> > index 97462e9b40d8..11df0cc63bed 100644
> > --- a/drivers/thermal/rcar_thermal.c
> > +++ b/drivers/thermal/rcar_thermal.c
> > @@ -52,6 +52,7 @@ struct rcar_thermal_chip {
> >       unsigned int irq_per_ch : 1;
> >       unsigned int needs_suspend_resume : 1;
> >       unsigned int nirqs;
> > +     unsigned int ctemp_bands;
>
> Would it be possible to rename this to something indicating that this is
> a gen3 thing? Maybe move it to the bit fields above and name it gen3 ?

Is that really a good thing to do? This structure describes features of
the thermal module, and we're already beyond the point where a simple
check  for gen2 or gen3 was sufficient.
Here the feature is having multiple temperature bands.
What if some other Gen3 SoC starts having 3 temperature bands?

> > @@ -263,7 +267,12 @@ static int rcar_thermal_get_current_temp(struct rcar_thermal_priv *priv,
> >               return ret;
> >
> >       mutex_lock(&priv->lock);
> > -     tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> > +     if (priv->chip->ctemp_bands == 1)
> > +             tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> > +     else if (priv->ctemp < 24)
> > +             tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
> > +     else
> > +             tmp = MCELSIUS((priv->ctemp * 5) - 60);
>
> I confirm that the calculations here are correct, but hard to read ;-)
> With the rename about how about.
>
>     if (priv->chip->gen3) {
>         if (priv->ctemp < 24)
>                 tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
>         else
>                 tmp = MCELSIUS((priv->ctemp * 5) - 60);
>     } else {
>             tmp =  MCELSIUS((priv->ctemp * 5) - 65);
>     }

_Iff_ we decide on going for the rename, I'd still write it as:

    if (!priv->chip->gen3)
            tmp =  MCELSIUS((priv->ctemp * 5) - 65);
    else if (priv->ctemp < 24)
            tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
    else
            tmp = MCELSIUS((priv->ctemp * 5) - 60);

Always fold your if/else if/else constructs to minimize the need for indentation
and braces ;-)

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