Re: [PATCH v1 5/5] pinctrl: intel: Introduce for_each_intel_gpio_group() helper

[Andy Shevchenko]

On Thu, Aug 29, 2024 at 7:53 AM Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> On Wed, Aug 28, 2024 at 09:38:38PM +0300, Andy Shevchenko wrote:
> > Introduce a helper macro for_each_intel_gpio_group().
> > With that in place, update users.
> >
> > It reduces the C code base as well as shrinks the binary:
> >
> >   add/remove: 0/0 grow/shrink: 1/8 up/down: 37/-106 (-69)
> >   Total: Before=15611, After=15542, chg -0.44%

...

> > +#define for_each_intel_gpio_group(pctrl, community, grp)                             \
> > +     for (unsigned int __i = 0;                                                      \
> > +          __i < pctrl->ncommunities && (community = &pctrl->communities[__i]);       \
> > +          __i++)                                                                     \
> > +             for (unsigned int __j = 0;                                              \
> > +                  __j < community->ngpps && (grp = &community->gpps[__j]);           \
> > +                  __j++)                                                             \
> > +                     if (grp->gpio_base == INTEL_GPIO_BASE_NOMAP) {} else
> > +
>
> This looks absolutely grotesque. I hope that you can debug this still
> after couple of months has passed because I cannot ;-)

Yes, I can.

> I wonder if there is a way to make it more readable by adding some sort
> of helpers? Or perhaps we don't need to make the whole thing as macro
> and just provide helpers we can use in the otherwise open-coded callers.

Yes, I can split it into two for-loops. But note, each of them a quite
standard how we define for_each macro with and without conditional,
see the jernel full of them (PCI, GPIOLIB, i915, ...).

...

> > -     for (i = 0; i < pctrl->ncommunities; i++) {
> > -             const struct intel_community *comm = &pctrl->communities[i];
> > -             int j;
> > +     for_each_intel_gpio_group(pctrl, c, gpp) {
> > +             if (offset >= gpp->gpio_base && offset < gpp->gpio_base + gpp->size) {
> > +                     if (community)
> > +                             *community = c;
> > +                     if (padgrp)
> > +                             *padgrp = gpp;
> >
> > -             for (j = 0; j < comm->ngpps; j++) {
> > -                     const struct intel_padgroup *pgrp = &comm->gpps[j];
> > -
> > -                     if (pgrp->gpio_base == INTEL_GPIO_BASE_NOMAP)
> > -                             continue;
> > -
> > -                     if (offset >= pgrp->gpio_base &&
> > -                         offset < pgrp->gpio_base + pgrp->size) {
> > -                             int pin;
> > -
> > -                             pin = pgrp->base + offset - pgrp->gpio_base;
> > -                             if (community)
> > -                                     *community = comm;
> > -                             if (padgrp)
> > -                                     *padgrp = pgrp;
> > -
> > -                             return pin;
> > -                     }
>
> Because I think this open-coded one is still at least readable. Of
> course if there is duplication we should try to get rid of it but not in
> expense of readability IMHO.

The result I think is more readable as it's pretty clear from the
macro name what is iterating over. It also hides unneeded detail, i.e.
iterator variable.

>
> > +                     return gpp->base + offset - gpp->gpio_base;
> >               }
> >       }


-- 
With Best Regards,
Andy Shevchenko