[PATCH v4 8/8] thermal: exynos: use set_trips

[Lukasz Luba]

On 11/2/23 10:35, Mateusz Majewski wrote:
> Hi,
> 
>>> +        th &= ~(0xff << 0);
>>> +        th |= temp_to_code(data, temp) << 0;
>>   
>> This 2-line pattern repeats a few times. It looks like a nice cadidate
>> for an inline function which can abstract that. Something like:
>>   
>> val = update_temp_value(data, temp, threshold, LOW_TEMP_SHIFT)
>>   
>> Assisted with the macros {LOW|HIGH|CRIT}_TEMP_SHIFT, the code
>> would look less convoluted IMO.
>> (The old code with the multiplication for the shift value wasn't
>> cleaner nor faster).
> 
> What would you think about something like this?
> 
> static void exynos_tmu_update_temp(struct exynos_tmu_data *data, int reg_off,
>                                     int bit_off, u8 temp)
> {
>          u32 th;
> 
>          th = readl(data->base + reg_off);
>          th &= ~(0xff << bit_off);
>          th |= temp_to_code(data, temp) << bit_off;
>          writel(th, data->base + reg_off);
> }
> 
> And then, it would be used like this:
> 
> static void exynos4412_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          exynos_tmu_update_temp(data, EXYNOS_THD_TEMP_RISE, 24, temp);
>          exynos_tmu_update_bit(data, EXYNOS_TMU_REG_CONTROL,
>                                EXYNOS_TMU_THERM_TRIP_EN_SHIFT, true);
> }

Yes, this looks good.

> 
> Granted it's not as clear as if we had some macro like CRIT_TEMP_SHIFT, but
> we would need more than one variant anyway, as Exynos 5433 uses different
> values of reg_off, and the new function looks short and inviting IMHO.

Fair enough.

> 
>>> -static void exynos7_tmu_set_trip_temp(struct exynos_tmu_data *data,
>>> -                                      int trip, u8 temp)
>>> +static void exynos7_tmu_update_temp(struct exynos_tmu_data *data, u8 temp,
>>> +                                    int idx, bool rise)
>>>     {
>>>             unsigned int reg_off, bit_off;
>>>             u32 th;
>>> +        void __iomem *reg;
>>>     
>>> -        reg_off = ((7 - trip) / 2) * 4;
>>> -        bit_off = ((8 - trip) % 2);
>>> +        reg_off = ((7 - idx) / 2) * 4;
>>   
>> Why can't we just have a set of defined register macros and pick one
>> in some small function?
>> A lot of operations here, also some assumption.
>>   
>>> +        bit_off = ((8 - idx) % 2);
>>   
>> So this can only be 0 or 1 and than it's used for the shift
>> multiplication. Also I don't know the history of older code and
>> if it was missed after some cleaning, but 'idx % 2' gives
>> equal values but w/o subtraction.
>>   
>> BTW, the code assumes the 'idx' values are under control somewhere else.
>> Is that because the DT make sure in the schema that the range cannot be
>> too big?
>> What are the possible values for 'idx'?
> 
> In the old code, the values of trip (which is the same thing, I will
> change the name back from idx) were limited by the value of data->ntrip,
> which was always 8 (value is per SoC). In the new code, there are only three
> variants:
> 
> static void exynos7_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          exynos7_tmu_update_temp(data, temp, 0, false);
> }
> 
> static void exynos7_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          exynos7_tmu_update_temp(data, temp, 1, true);
> }
> 
> static void exynos7_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          /*
>           * Like Exynos 4210, Exynos 7 does not seem to support critical temperature
>           * handling in hardware. Again, we still set a separate interrupt for it.
>           */
>          exynos7_tmu_update_temp(data, temp, 7, true);
> }
> 
> To be fair, considering the values are constant like this, I should probably
> just do the calculations myself and then in code just call exynos_tmu_update_temp
> (from above) and exynos_tmu_update_bit, like on all other SoCs. I guess I were
> a bit too scared to touch Exynos 7 code...

Yes, anything that can be pre-calculated with nice comment, would be
more desired. I would suggest to not be afraid about touching that
Exynos 7 code.

> 
>>> -        if (on) {
>>> -                for (i = 0; i < data->ntrip; i++) {
>>> -                        if (thermal_zone_get_trip(tz, i, &trip))
>>> -                                continue;
>>> -
>>> -                        interrupt_en |=
>>> -                                (1 << (EXYNOS_TMU_INTEN_RISE0_SHIFT + i * 4));
>>> -                }
>>> -
>>> -                if (data->soc != SOC_ARCH_EXYNOS4210)
>>> -                        interrupt_en |=
>>> -                                interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT;
>>> -
>>> +        if (on)
>>>                     con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
>>> -        } else {
>>> +        else
>>>                     con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT);
>>   
>> Please also consider the BIT() helper here and above...
> 
> Will do, but should I do this in a separate patch in these cases? I don't touch
> the con lines otherwise, and this patch is already humongous.

That would definitely deserve an extra patch to address it. Please add
to the patch set.

Regards,
Lukasz