| Message ID | 20190418071514.13027-1-horms+renesas@verge.net.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Eduardo Valentin |
| Headers | show |
| Series | [repost] thermal: rcar_thermal: update calculation formula for E3 | expand |
Hi Kaneko-san, 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 ? > }; > > static const struct rcar_thermal_chip rcar_thermal = { > @@ -60,6 +61,7 @@ static const struct rcar_thermal_chip rcar_thermal = { > .irq_per_ch = 0, > .needs_suspend_resume = 0, > .nirqs = 1, > + .ctemp_bands = 1, > }; > > static const struct rcar_thermal_chip rcar_gen2_thermal = { > @@ -68,6 +70,7 @@ static const struct rcar_thermal_chip rcar_gen2_thermal = { > .irq_per_ch = 0, > .needs_suspend_resume = 0, > .nirqs = 1, > + .ctemp_bands = 1, > }; > > static const struct rcar_thermal_chip rcar_gen3_thermal = { > @@ -80,6 +83,7 @@ static const struct rcar_thermal_chip rcar_gen3_thermal = { > * interrupts to detect a temperature change, rise or fall. > */ > .nirqs = 2, > + .ctemp_bands = 2, > }; > > struct rcar_thermal_priv { > @@ -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); } > mutex_unlock(&priv->lock); > > if ((tmp < MCELSIUS(-45)) || (tmp > MCELSIUS(125))) { > -- > 2.11.0 >
On 18.04.2019 10:15, Simon Horman wrote: > From: Yoshihiro Kaneko <ykaneko0929@gmail.com> > > HW manual changes temperature calculation formula for E3: > - 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 [...] > @@ -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); ^ stray space? > + else if (priv->ctemp < 24) > + tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10); > + else > + tmp = MCELSIUS((priv->ctemp * 5) - 60); > mutex_unlock(&priv->lock); > > if ((tmp < MCELSIUS(-45)) || (tmp > MCELSIUS(125))) { MBR, Sergei
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
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@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 ;-) From my PoV I think the patch is fine in its current form. Niklas do you feel particularly strongly about changing it?
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@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? 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@ragnatech.se>
On Wed, May 08, 2019 at 12:14:46AM +0200, Niklas Söderlund wrote: ... > > 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@ragnatech.se> Thanks Niklas, I have gone ahead and posted [PATCH v2] thermal: rcar_thermal: update calculation formula for R-Car Gen3 SoCs
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; }; static const struct rcar_thermal_chip rcar_thermal = { @@ -60,6 +61,7 @@ static const struct rcar_thermal_chip rcar_thermal = { .irq_per_ch = 0, .needs_suspend_resume = 0, .nirqs = 1, + .ctemp_bands = 1, }; static const struct rcar_thermal_chip rcar_gen2_thermal = { @@ -68,6 +70,7 @@ static const struct rcar_thermal_chip rcar_gen2_thermal = { .irq_per_ch = 0, .needs_suspend_resume = 0, .nirqs = 1, + .ctemp_bands = 1, }; static const struct rcar_thermal_chip rcar_gen3_thermal = { @@ -80,6 +83,7 @@ static const struct rcar_thermal_chip rcar_gen3_thermal = { * interrupts to detect a temperature change, rise or fall. */ .nirqs = 2, + .ctemp_bands = 2, }; struct rcar_thermal_priv { @@ -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); mutex_unlock(&priv->lock); if ((tmp < MCELSIUS(-45)) || (tmp > MCELSIUS(125))) {