Message-ID: <15e97e28-0008-cda4-176d-a3feb9ad4e8a@schinagl.nl>
Date:   Sat, 23 Feb 2019 21:37:01 +0100
From:   Olliver Schinagl <oliver@...inagl.nl>
To:     Axel Lin <axel.lin@...ics.com>
Cc:     Mark Brown <broonie@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
        Priit Laes <plaes@...es.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS
 defines

On 23-02-2019 13:54, Axel Lin wrote:
>> I will not disagree that it may be extra work to look up the define
>> (especially if there is no tool tip or split view in the editor) but
>> reading the whole lot of code, with only the magic values, you still
>> have to look up the meaning of each magic value, have to guess which one
>> has the same meaning etc.
>>
>> Further more, I do believe far more people reading will find an define
>> to be more descriptive to read. Whoever needs to actually go in and
>> fix/change things.
> I disagree.
> The reason I sent this patch is because these defines reduce readability.
> I do care about readability, but in this case these defines make
> readability worsen.
Well this really is up to personal preference isn't it? As personally
find it much nicer to read without the magics :) If I actually have to
modify or go into the actual meaning, then yes, I will have to dig into
it a little deeper. But the overal code to a passer by, is still in my
opinion much more readable.
>
> At the context of REGULATOR_LINEAR_RANGE, each fields has well known meaning.
> When I look at the table with values (I don't care if it's hex or decimal),
> it tells everything I need to know.
> I can easily check if any linear ranger cover other ranges.
> I can easily check if any gap between linar ranges, (probably due to
> reserved bits).
> I can easily count the number of entries in each range.
> I can easily calculate the min/max voltage of each range and double
> check with datasheet.
> i.e. If there are something wrong, it's eash to detect it.

In any case, you seem like a smart person that reads and writes hex and
bits often enough. This is not true for everyone. I can just as easily
reverse your arguments of course, for example, 'each field has a well
known meaning' ... to whom? People that use these things daily, sure.
People who just need to double check something or modify something, not
so much. They have to look up the MACRO, the struct its in, compare it
to others, so as you can see, what is natural for you, is not true for
everyone. :)

Also, the general consensus is still to avoid magic values, and to stay
consistent with the rest and not make expceptions, it makes sense to
have defines instead of magic values.

>
> When you change the values to DEFINES, I have to check the value of
> each define *one-by-one*.
> There is no benefit in this case.
>
> I don't mean adding DEFINES is wrong. Just in this case it does not
> help and actually has downside.
> I only remove AXP20X_xxx_START/END/STEPS defines, not all defines.
>
> BTW, just show you an example (from drivers/regulator/88pm8607.c)
> I don't think change all below values to DEFINES help in readability.
> static const unsigned int BUCK1_table[] = {
>          725000,  750000,  775000,  800000,  825000,  850000,  875000,  900000,
>          925000,  950000,  975000, 1000000, 1025000, 1050000, 1075000, 1100000,
>         1125000, 1150000, 1175000, 1200000, 1225000, 1250000, 1275000, 1300000,
>         1325000, 1350000, 1375000, 1400000, 1425000, 1450000, 1475000, 1500000,
>               0,   25000,   50000,   75000,  100000,  125000,  150000,  175000,
>          200000,  225000,  250000,  275000,  300000,  325000,  350000,  375000,
>          400000,  425000,  450000,  475000,  500000,  525000,  550000,  575000,
>          600000,  625000,  650000,  675000,  700000,  725000,  750000,  775000,
> };

Personally, I think this is a horrible table :p sure, I can guess that
these are voltages (based on the fact that it's a regulator table and I
am a little familiar here), but without knowing the context, I see a
bunch of voltages, from 0,725 to 1,5 appearantly in equal steps, but the
first question I ask, is the step always .25? I can't see, i'd have to
go over each value and compare them all. Quite cumbersome ;)

And then, there is a nother row, which starts after the 1.5 but at 0 and
goes to 7.75. Are these two the same regulator? Why the overlap?

The only way to save this, would be with 2 linear range define/macro's,
with a start voltage, and end voltage and steps; right? So yes, macro's
and defines would help immensly here :)

>
>
> Regards,
> Axel

Powered by blists - more mailing lists