Re: [PATCH 5/6] drivers/thermal/exynos: add initial Exynos 850 support

[Mateusz Majewski <m.majewski2@samsung.com>]

> It feels like there is an error for Exynos7 case there. Take a look at
> this commit:
> 
>     aef27b658b43 ("thermal: exynos: use sanitize_temp_error() in
> exynos7_tmu_initialize()")
> 
> I think that commit just forgets to update the shift value for Exynos7
> properly. This code:
> 
>     data->temp_error1 = trim_info & tmu_temp_mask;
>     data->temp_error2 = ((trim_info >> EXYNOS_TRIMINFO_85_SHIFT) &
>                 EXYNOS_TMU_TEMP_MASK);
> 
> in case of Exynos7 becomes:
> 
>     data->temp_error1 = trim_info & 0x1ff;    // mask = 9 bits
>     data->temp_error2 = (trim_info >> 8) & 0xff;
> 
> it contradicts itself, because it takes 9 rightmost bits for error1,
> and then uses 1 of those bits for error2 too. It's obvious that if 9
> bits are already used for error1, then for error2 it has to be shifted
> by 9 bits, not 8.
> 
> That's why I think your patch 2/6 is legit and useful on its own, and
> it's actually a good catch on your part! But the shift value has to be
> fixed as well (for Exynos7). It's not ideal you don't have the
> hardware to test it, but it just screams *bug* to me :) Also, maybe we
> can ask someone who has Exynos7 hardware to test it for us?

I thought about it for a bit and finally realized that Exynos7 only
supports one point trimming. That is why in that commit, the original
exynos7_tmu_initialize did not do anything with temp_error2. So
temp_error2 will never be used in Exynos7. The real "fix" I guess is to
only calculate temp_error2 if two point trimming is used, which is
possible with a very small reordering of exynos7_tmu_initialize. But
then the shift value will never be reachable in Exynos7 anyway. What do
you think about this? I feel like it's worth it to change the shift
value just because the code is a bit confusing anyway.