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);
}
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.
> > -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...
> > - 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.
Thank you :)
Mateusz
