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

Andy Shevchenko
2024-08-29T05:34:46Z

On Thu, Aug 29, 2024 at 7:53 AM Mika Westerberg
<mika.westerberg@linux.intel.com> 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