On Wed, Apr 24, 2019 at 09:10:56AM +0200, Geert Uytterhoeven wrote:
> 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 ;-)
>From my PoV I think the patch is fine in its current form.
Niklas do you feel particularly strongly about changing it?
