Hello,
On 2019-04-26 11:55:11 +0200, Simon Horman wrote:
> 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?
Good point, after reviewing your comment I now agree with the original
patch.
> >
> > > > @@ -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?
No I do not feel strongly about this and would be fine with the patch in
it's current form. With and update commit message to mention V3M and D3
feel free to add
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
--
Regards,
Niklas Söderlund
