Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230522163115.2592883-1-eblanc@baylibre.com> <20230522163115.2592883-4-eblanc@baylibre.com>
 <ZG0VHnEByyMW9i4a@surfacebook> <613601c2-cc98-526b-a9e3-2ad2abc68e1d@baylibre.com>
In-Reply-To: <613601c2-cc98-526b-a9e3-2ad2abc68e1d@baylibre.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 7 Jun 2023 17:52:46 +0300
Message-ID: <CAHp75VdTi4y9=XsjjQOBzUhSYf8jzV7mwguruqMZD0jV7VqZkw@mail.gmail.com>
Subject: Re: [PATCH v5 3/3] regulator: tps6594-regulator: Add driver for TI
 TPS6594 regulators
To:     jerome Neanne <jneanne@baylibre.com>
Cc:     Esteban Blanc <eblanc@baylibre.com>, linus.walleij@linaro.org,
        lgirdwood@gmail.com, broonie@kernel.org, a.zummo@towertech.it,
        alexandre.belloni@bootlin.com, linux-kernel@vger.kernel.org,
        linux-gpio@vger.kernel.org, linux-rtc@vger.kernel.org,
        jpanis@baylibre.com, aseketeli@baylibre.com, u-kumar1@ti.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jun 7, 2023 at 2:44=E2=80=AFPM jerome Neanne <jneanne@baylibre.com>=
 wrote:

...

> >> +    enum {
> >> +            MULTI_BUCK12,
> >> +            MULTI_BUCK123,
> >> +            MULTI_BUCK1234,
> >> +            MULTI_BUCK12_34,
> >
> >> +            MULTI_FIRST =3D MULTI_BUCK12,
> >> +            MULTI_LAST =3D MULTI_BUCK12_34,
> >> +            MULTI_NUM =3D 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 =3D MULTI_BUCK12_34 - MULTI_BUCK12 + 1

See (1) above.

>         };

...

> >> +                    continue;
> >> +            delta =3D 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 gam=
e:
>
> index =3D match_string(multiphases, ARRAY_SIZE(multiphases), npname);
> if (index >=3D 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 ma=
de.

--=20
With Best Regards,
Andy Shevchenko