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