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

[Mateusz Majewski]

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