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

[Andy Shevchenko]

On Wed, Jun 7, 2023 at 2:44 PM jerome Neanne <jneanne@xxxxxxxxxxxx> 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