linux-kernel - Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d52c2711-71f3-9e72-f01b-e761f4a92a07@schinagl.nl>
Date:   Sat, 23 Feb 2019 08:55:05 +0100
From:   Olliver Schinagl <oliver@...inagl.nl>
To:     Mark Brown <broonie@...nel.org>, Axel Lin <axel.lin@...ics.com>
Cc:     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 21-02-2019 10:42, Mark Brown wrote:
> On Thu, Feb 21, 2019 at 08:22:53AM +0800, Axel Lin wrote:
>> Olliver Schinagl <oliver@...inagl.nl> 於 2019年2月21日 週四 上午6:57寫道：
>>> On February 20, 2019 5:50:13 PM GMT+01:00, Axel Lin <axel.lin@...ics.com> wrote:
>>>> The AXP20X_xxx_START/END/STEPS defines make the code hard to read and
>>>> very hard to check the linear range settings because it needs to check
>>>> the defines one-by-one.
>>>> The original code without the defines is very good in readability
>>>> as the meaning of each field of REGULATOR_LINEAR_RANGE is clear.
>>>> So I suggest to get rid of AXP20X_xxx_START/END/STEPS defines.
>>> Are you suggesting that magic values and hex numbers are more readable?
>> For example:
>> static const struct regulator_linear_range axp803_dcdc234_ranges[] = {
>>        REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
>>        REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x4b, 20000),
>> };
>> Above looks very clear to me as it describes the linear ranges:
>> 1st linear range: min_uV is 500000, from selector 0 ~ 0x46, the uV_step is 10000
>> 2nd linear range: min_uV is 1220000, from selector 0x47 ~ 0x4b, the
>> uV_step is 20000
>> And it's easy to check the min_sel and max_sel in each linear range.
> Frankly I tend to agree with Axel here - the defines are used in exactly
> one place so their main impact is that you can't read the ranges
> directly in the array but have to jump around to read through the
> defines.

The number of times the defines are used, are of course no reason for it
to be a bad thing. Defines should certainly not be thought of as a tool
to only reduce duplicity of course. Also being consistent where others
do have multiple invocations makes sense.

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.

Also, how many steps are in the first or second regulator ranges? which
bits are these exactly? Etc etc, you still have to use your mind to
figure things out.

Finally, while some people live and breath hexadecimals and bitmasks,
others just see magic values. So having something readable there, may
make it harder for the one that needs to work on it (which hopefully is
almost never, as these things are not very dynamic/commonly changing)
but passers by see readable text and understand what is happening at a
glance. Which is why we where taught not to use magic values in the
first place :)