Hi Niklas,
On Thu, Apr 18, 2019 at 10:12 AM Niklas Söderlund
<niklas.soderlund@xxxxxxxxxxxx> wrote:
> On 2019-04-18 09:15:14 +0200, Simon Horman wrote:
> > From: Yoshihiro Kaneko <ykaneko0929@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> >
> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@xxxxxxxxx>
> > Tested-by: Simon Horman <horms+renesas@xxxxxxxxxxxx>
> > Acked-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Simon Horman <horms+renesas@xxxxxxxxxxxx>
> > ---
> > 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@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
