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
