linux-kernel - Re: [PATCH v5 3/3] regulator: tps6594-regulator: Add driver for TI TPS6594 regulators

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] [day] [month] [year] [list]

Message-ID: <CAHp75VdTi4y9=XsjjQOBzUhSYf8jzV7mwguruqMZD0jV7VqZkw@mail.gmail.com>
Date:   Wed, 7 Jun 2023 17:52:46 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     jerome Neanne <jneanne@...libre.com>
Cc:     Esteban Blanc <eblanc@...libre.com>, linus.walleij@...aro.org,
        lgirdwood@...il.com, broonie@...nel.org, a.zummo@...ertech.it,
        alexandre.belloni@...tlin.com, linux-kernel@...r.kernel.org,
        linux-gpio@...r.kernel.org, linux-rtc@...r.kernel.org,
        jpanis@...libre.com, aseketeli@...libre.com, u-kumar1@...com
Subject: Re: [PATCH v5 3/3] regulator: tps6594-regulator: Add driver for TI
 TPS6594 regulators

On Wed, Jun 7, 2023 at 2:44 PM jerome Neanne <jneanne@...libre.com> wrote:

...

> >> +    enum {
> >> +            MULTI_BUCK12,
> >> +            MULTI_BUCK123,
> >> +            MULTI_BUCK1234,
> >> +            MULTI_BUCK12_34,
> >
> >> +            MULTI_FIRST = MULTI_BUCK12,
> >> +            MULTI_LAST = MULTI_BUCK12_34,
> >> +            MULTI_NUM = MULTI_LAST - MULTI_FIRST + 1

> >               MULT_NUM
> >
> > will suffice instead of all this.

(1)

> >> +    };
> >
> > But why enum at all? See below.
> Just for the switch case readability.
> I have to iterate across the multiphases array for look up name into
> device tree and evaluate in that order.
>
> This can be reduced to:
>         enum {
>                 MULTI_BUCK12,
>                 MULTI_BUCK123,
>                 MULTI_BUCK1234,
>                 MULTI_BUCK12_34,

>                 MULTI_NUM = MULTI_BUCK12_34 - MULTI_BUCK12 + 1

See (1) above.

>         };

...

> >> +                    continue;
> >> +            delta = strcmp(npname, multiphases[multi]);
> >> +            if (!delta) {
> >> +                    switch (multi) {
> >> +                    case MULTI_BUCK12:
> >
> > This all looks like match_string() reinvention.
> I can go with match_string but this is not significantly changing the game:
>
> index = match_string(multiphases, ARRAY_SIZE(multiphases), npname);
> if (index >= 0) {
>         switch (index) {
>
> No question on all your other feedback. Just wondering if I missed
> something with match_string use. Looks like a good idea indeed but this
> is not drastically changing the code as you seem to expect... Let me
> know if you think I'm doing it in a wrong way.

I guess the entire big for-loop can be optimized, but I haven't looked
at that. At least match_string() would help understanding what you are
trying to do,

In any case it seems Mark applied your version, so the follow ups can be made.

-- 
With Best Regards,
Andy Shevchenko