Hi Geert-san,
Thank you for your comments!
> From: Geert Uytterhoeven, Sent: Monday, June 8, 2020 8:38 PM
<snip>
> > --- a/drivers/thermal/rcar_gen3_thermal.c
> > +++ b/drivers/thermal/rcar_gen3_thermal.c
> > @@ -167,7 +167,7 @@ static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
> > {
> > struct rcar_gen3_thermal_tsc *tsc = devdata;
> > int mcelsius, val;
> > - u32 reg;
> > + long reg;
>
> "long" is 64-bit, so "int" should be sufficient.
Oops.
> > /* Read register and convert to mili Celsius */
> > reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
>
> However, rcar_gen3_thermal_read() does return u32, so keeping u32 for
> reg looks more logical to me.
>
> Successive lines are:
>
> if (reg <= thcode[tsc->id][1])
> val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b1,
> tsc->coef.a1);
> else
> val = FIXPT_DIV(FIXPT_INT(reg) - tsc->coef.b2,
> tsc->coef.a2);
>
> Perhaps it's safer to add an cast to FIXPT_INT(), so it always returns
> int, and/or add casts to FIXPT_DIV(), so only signed values are passed
> to DIV_ROUND_CLOSEST?
> That would prevent the issue from reappearing later.
There is not related to the issue but, thcode[] is also int.
So, if we use casts, we will see a lot of casts like below:
---
static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
{
struct rcar_gen3_thermal_tsc *tsc = devdata;
int mcelsius, val;
u32 reg;
/* Read register and convert to mili Celsius */
reg = rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK;
if ((int)reg <= thcode[tsc->id][1])
val = (int)FIXPT_DIV((int)FIXPT_INT(reg) - tsc->coef.b1,
tsc->coef.a1);
else
val = (int)FIXPT_DIV((int)FIXPT_INT(reg) - tsc->coef.b2,
tsc->coef.a2);
---
I'm thinking the name "reg" is not good because it should be the same type as
rcar_gen3_thermal_read(). But, if we use other name like "int ctemp;", we can use
it like below. What do you think?
---
static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
{
struct rcar_gen3_thermal_tsc *tsc = devdata;
int mcelsius, val;
int ctemp;
/* Read register and convert to mili Celsius */
ctemp = int(rcar_gen3_thermal_read(tsc, REG_GEN3_TEMP) & CTEMP_MASK);
if (ctemp <= thcode[tsc->id][1])
val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b1,
tsc->coef.a1);
else
val = FIXPT_DIV(FIXPT_INT(ctemp) - tsc->coef.b2,
tsc->coef.a2);
---
> BTW, rcar_gen3_thermal_mcelsius_to_temp() returns a value to store in a
> register, hence I think it should return u32 instead of int.
I think so.
Best regards,
Yoshihiro Shimoda
> 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
