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%
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/intel/pinctrl-intel.c | 89 +++++++++------------------
1 file changed, 29 insertions(+), 60 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index ae30969b2dee..143174abda32 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -931,6 +931,15 @@ static const struct pinctrl_desc intel_pinctrl_desc = {
.owner = THIS_MODULE,
};
+#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
+
/**
* intel_gpio_to_pin() - Translate from GPIO offset to pin number
* @pctrl: Pinctrl structure
@@ -949,30 +958,17 @@ static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned int offset,
const struct intel_community **community,
const struct intel_padgroup **padgrp)
{
- int i;
+ const struct intel_community *c;
+ const struct intel_padgroup *gpp;
- 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;
- }
+ return gpp->base + offset - gpp->gpio_base;
}
}
@@ -1348,36 +1344,17 @@ static int intel_gpio_irq_init_hw(struct gpio_chip *gc)
return 0;
}
-static int intel_gpio_add_community_ranges(struct intel_pinctrl *pctrl,
- const struct intel_community *community)
-{
- int ret = 0, i;
-
- for (i = 0; i < community->ngpps; i++) {
- const struct intel_padgroup *gpp = &community->gpps[i];
-
- if (gpp->gpio_base == INTEL_GPIO_BASE_NOMAP)
- continue;
-
- ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
- gpp->gpio_base, gpp->base,
- gpp->size);
- if (ret)
- return ret;
- }
-
- return ret;
-}
-
static int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
{
struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
- int ret, i;
+ struct intel_community *community;
+ const struct intel_padgroup *gpp;
+ int ret;
- for (i = 0; i < pctrl->ncommunities; i++) {
- struct intel_community *community = &pctrl->communities[i];
-
- ret = intel_gpio_add_community_ranges(pctrl, community);
+ for_each_intel_gpio_group(pctrl, community, gpp) {
+ ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
+ gpp->gpio_base, gpp->base,
+ gpp->size);
if (ret) {
dev_err(pctrl->dev, "failed to add GPIO pin range\n");
return ret;
@@ -1390,20 +1367,12 @@ static int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
static unsigned int intel_gpio_ngpio(const struct intel_pinctrl *pctrl)
{
const struct intel_community *community;
+ const struct intel_padgroup *gpp;
unsigned int ngpio = 0;
- int i, j;
- for (i = 0; i < pctrl->ncommunities; i++) {
- community = &pctrl->communities[i];
- for (j = 0; j < community->ngpps; j++) {
- const struct intel_padgroup *gpp = &community->gpps[j];
-
- if (gpp->gpio_base == INTEL_GPIO_BASE_NOMAP)
- continue;
-
- if (gpp->gpio_base + gpp->size > ngpio)
- ngpio = gpp->gpio_base + gpp->size;
- }
+ for_each_intel_gpio_group(pctrl, community, gpp) {
+ if (gpp->gpio_base + gpp->size > ngpio)
+ ngpio = gpp->gpio_base + gpp->size;
}
return ngpio;
--
2.43.0.rc1.1336.g36b5255a03acOn 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%
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/pinctrl/intel/pinctrl-intel.c | 89 +++++++++------------------
> 1 file changed, 29 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index ae30969b2dee..143174abda32 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -931,6 +931,15 @@ static const struct pinctrl_desc intel_pinctrl_desc = {
> .owner = THIS_MODULE,
> };
>
> +#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 ;-)
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.
> /**
> * intel_gpio_to_pin() - Translate from GPIO offset to pin number
> * @pctrl: Pinctrl structure
> @@ -949,30 +958,17 @@ static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned int offset,
> const struct intel_community **community,
> const struct intel_padgroup **padgrp)
> {
> - int i;
> + const struct intel_community *c;
> + const struct intel_padgroup *gpp;
>
> - 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.
> + return gpp->base + offset - gpp->gpio_base;
> }
> }
>
> @@ -1348,36 +1344,17 @@ static int intel_gpio_irq_init_hw(struct gpio_chip *gc)
> return 0;
> }
>
> -static int intel_gpio_add_community_ranges(struct intel_pinctrl *pctrl,
> - const struct intel_community *community)
> -{
> - int ret = 0, i;
> -
> - for (i = 0; i < community->ngpps; i++) {
> - const struct intel_padgroup *gpp = &community->gpps[i];
> -
> - if (gpp->gpio_base == INTEL_GPIO_BASE_NOMAP)
> - continue;
> -
> - ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
> - gpp->gpio_base, gpp->base,
> - gpp->size);
> - if (ret)
> - return ret;
> - }
> -
> - return ret;
> -}
> -
> static int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
> {
> struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
> - int ret, i;
> + struct intel_community *community;
> + const struct intel_padgroup *gpp;
> + int ret;
>
> - for (i = 0; i < pctrl->ncommunities; i++) {
> - struct intel_community *community = &pctrl->communities[i];
> -
> - ret = intel_gpio_add_community_ranges(pctrl, community);
> + for_each_intel_gpio_group(pctrl, community, gpp) {
> + ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
> + gpp->gpio_base, gpp->base,
> + gpp->size);
> if (ret) {
> dev_err(pctrl->dev, "failed to add GPIO pin range\n");
> return ret;
> @@ -1390,20 +1367,12 @@ static int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
> static unsigned int intel_gpio_ngpio(const struct intel_pinctrl *pctrl)
> {
> const struct intel_community *community;
> + const struct intel_padgroup *gpp;
> unsigned int ngpio = 0;
> - int i, j;
>
> - for (i = 0; i < pctrl->ncommunities; i++) {
> - community = &pctrl->communities[i];
> - for (j = 0; j < community->ngpps; j++) {
> - const struct intel_padgroup *gpp = &community->gpps[j];
> -
> - if (gpp->gpio_base == INTEL_GPIO_BASE_NOMAP)
> - continue;
> -
> - if (gpp->gpio_base + gpp->size > ngpio)
> - ngpio = gpp->gpio_base + gpp->size;
> - }
> + for_each_intel_gpio_group(pctrl, community, gpp) {
> + if (gpp->gpio_base + gpp->size > ngpio)
> + ngpio = gpp->gpio_base + gpp->size;
> }
>
> return ngpio;
> --
> 2.43.0.rc1.1336.g36b5255a03ac
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
© 2016 - 2025 Red Hat, Inc.