Re: [PATCHv5 2/5] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hi Geert,

Thanks for your feedback.

On 2016-12-13 09:19:53 +0100, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Mon, Dec 12, 2016 at 3:18 PM, Niklas Söderlund
> <niklas.soderlund@ragnatech.se> wrote:
> > +/*
> > + * Linear approximation for temperature
> > + *
> > + * [reg] = [temp] * a + b => [temp] = ([reg] - b) / a
> > + *
> > + * The constants a and b are calculated using two triplets of int values PTAT
> > + * and THCODE. PTAT and THCODE can either be read from hardware or use hard
> > + * coded values from driver. The formula to calculate a and b are taken from
> > + * BSP and sparsely documented and understood.
> > + *
> > + * Examining the linear formula and the formula used to calculate constants a
> > + * and b while knowing that the span for PTAT and THCODE values are between
> > + * 0x000 and 0xfff the largest integer possible is 0xfff * 0xfff == 0xffe001.
> > + * Integer also needs to be signed so that leaves 7 bits for decimal
> > + * fixed point scaling, which amounts to a decimal scaling factor of 100.
> > + */
> > +
> > +#define SCALE_FACTOR 100
> 
> What about using 128 instead?
> Fixed point is much easier with shifts
> (the compiler will turn multiplications in shifts where appropriate).

I tried using binary scaled instead of decimal scaled fixed point but 
noticed that the diff I got compared to the original formula which used 
decimal scaled (factor 1000) where larger so I picked decimal scaling.  

Maybe with feedback from Morimoto-san or Khiem we can switch to binary 
scaled number as it should over all increase the accuracy of the 
calculations, but then maybe some of the other hard coded constant also 
should be updated?

> 
> > +#define SCALE_INT(_x) ((_x) * SCALE_FACTOR)
> > +#define SCALE_MUL(_a, _b) (((_a)*(_b)) / SCALE_FACTOR)
> > +#define SCALE_DIV(_a, _b) (((_a)*SCALE_FACTOR)/(_b))
> 
> DIV_ROUND_CLOSEST()

Don't DIV_ROUND_CLOSEST() require the divisor to be a positive integer?  

  tj_2 = SCALE_DIV(SCALE_MUL(SCALE_INT(ptat[1] - ptat[2]), SCALE_INT(137)),
                   SCALE_INT(ptat[0] - ptat[2])) - SCALE_INT(41);

In this case if ptat[0] < (ptat[2] + 41) the divisor is negative so I 
think DIV_ROUND_CLOSEST() can't be used, or am I misunderstanding?

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> 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

-- 
Regards,
Niklas Söderlund