| Message ID | 1575352925-17271-2-git-send-email-peng.fan@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpio: mvebu: use platform_irq_count | expand |
On Tue, Dec 03, 2019 at 06:04:27AM +0000, Peng Fan wrote: > From: Peng Fan <peng.fan@nxp.com> > > Use platform_irq_count to replace of_irq_count > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > > V1: > Code inspection, not tested > > drivers/gpio/gpio-bcm-kona.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c > index 4122683eb1f9..c50721980a7c 100644 > --- a/drivers/gpio/gpio-bcm-kona.c > +++ b/drivers/gpio/gpio-bcm-kona.c > @@ -19,7 +19,6 @@ > #include <linux/io.h> > #include <linux/gpio/driver.h> > #include <linux/of_device.h> > -#include <linux/of_irq.h> > #include <linux/init.h> > #include <linux/irqdomain.h> > #include <linux/irqchip/chained_irq.h> > @@ -586,7 +585,7 @@ static int bcm_kona_gpio_probe(struct platform_device *pdev) > > kona_gpio->gpio_chip = template_chip; > chip = &kona_gpio->gpio_chip; > - kona_gpio->num_bank = of_irq_count(dev->of_node); > + kona_gpio->num_bank = platform_irq_count(pdev); of_irq_count returns 0 or a positive int while platform_irq_count might return a negative error code. This needs handling. Also I wonder why platform_irq_count() is better than of_irq_count() which would be good to describe in the commit log. Best regards Uwe
> Subject: Re: [PATCH 2/2] gpio: bcm-kona: use platform_irq_count > > On Tue, Dec 03, 2019 at 06:04:27AM +0000, Peng Fan wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > Use platform_irq_count to replace of_irq_count > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > > > V1: > > Code inspection, not tested > > > > drivers/gpio/gpio-bcm-kona.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/gpio/gpio-bcm-kona.c > > b/drivers/gpio/gpio-bcm-kona.c index 4122683eb1f9..c50721980a7c > 100644 > > --- a/drivers/gpio/gpio-bcm-kona.c > > +++ b/drivers/gpio/gpio-bcm-kona.c > > @@ -19,7 +19,6 @@ > > #include <linux/io.h> > > #include <linux/gpio/driver.h> > > #include <linux/of_device.h> > > -#include <linux/of_irq.h> > > #include <linux/init.h> > > #include <linux/irqdomain.h> > > #include <linux/irqchip/chained_irq.h> @@ -586,7 +585,7 @@ static int > > bcm_kona_gpio_probe(struct platform_device *pdev) > > > > kona_gpio->gpio_chip = template_chip; > > chip = &kona_gpio->gpio_chip; > > - kona_gpio->num_bank = of_irq_count(dev->of_node); > > + kona_gpio->num_bank = platform_irq_count(pdev); > > of_irq_count returns 0 or a positive int while platform_irq_count might return > a negative error code. This needs handling. Also I wonder why > platform_irq_count() is better than of_irq_count() which would be good to > describe in the commit log. Inspired from https://lkml.org/lkml/2015/11/18/466 From Rob: " So I started looking at why you are using of_irq_count which drivers shouldn't need to. In patch 5 you use it to allocate memory to store the irq names, then use them here... " Is this ok? Thanks, Peng. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww. > pengutronix.de%2F&data=02%7C01%7Cpeng.fan%40nxp.com%7C36e86 > 59eca384830eee508d777be21cc%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C637109530809227993&sdata=y95EHGlmw12IVVlWoiUiS > cQzO7GIBjQmnAfYrtd7P6k%3D&reserved=0 |
On Tue, Dec 03, 2019 at 07:00:40AM +0000, Peng Fan wrote: > > Subject: Re: [PATCH 2/2] gpio: bcm-kona: use platform_irq_count > > > > On Tue, Dec 03, 2019 at 06:04:27AM +0000, Peng Fan wrote: > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > Use platform_irq_count to replace of_irq_count > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > --- > > > > > > V1: > > > Code inspection, not tested > > > > > > drivers/gpio/gpio-bcm-kona.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpio/gpio-bcm-kona.c > > > b/drivers/gpio/gpio-bcm-kona.c index 4122683eb1f9..c50721980a7c > > 100644 > > > --- a/drivers/gpio/gpio-bcm-kona.c > > > +++ b/drivers/gpio/gpio-bcm-kona.c > > > @@ -19,7 +19,6 @@ > > > #include <linux/io.h> > > > #include <linux/gpio/driver.h> > > > #include <linux/of_device.h> > > > -#include <linux/of_irq.h> > > > #include <linux/init.h> > > > #include <linux/irqdomain.h> > > > #include <linux/irqchip/chained_irq.h> @@ -586,7 +585,7 @@ static int > > > bcm_kona_gpio_probe(struct platform_device *pdev) > > > > > > kona_gpio->gpio_chip = template_chip; > > > chip = &kona_gpio->gpio_chip; > > > - kona_gpio->num_bank = of_irq_count(dev->of_node); > > > + kona_gpio->num_bank = platform_irq_count(pdev); > > > > of_irq_count returns 0 or a positive int while platform_irq_count might return > > a negative error code. This needs handling. Also I wonder why > > platform_irq_count() is better than of_irq_count() which would be good to > > describe in the commit log. > > Inspired from https://lkml.org/lkml/2015/11/18/466 > From Rob: > " > So I started looking at why you are using of_irq_count which drivers > shouldn't need to. In patch 5 you use it to allocate memory to store > the irq names, then use them here... > " > > Is this ok? I would say something like: platform_irq_count() is the more generic way (independent of device trees) to determine the count of available interrupts. So use this instead. As platform_irq_count() might return an error code (which of_irq_count doesn't) some additional handling is necessary. Best regards Uwe
> Subject: Re: [PATCH 2/2] gpio: bcm-kona: use platform_irq_count > > On Tue, Dec 03, 2019 at 07:00:40AM +0000, Peng Fan wrote: > > > Subject: Re: [PATCH 2/2] gpio: bcm-kona: use platform_irq_count > > > > > > On Tue, Dec 03, 2019 at 06:04:27AM +0000, Peng Fan wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > Use platform_irq_count to replace of_irq_count > > > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > --- > > > > > > > > V1: > > > > Code inspection, not tested > > > > > > > > drivers/gpio/gpio-bcm-kona.c | 3 +-- > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpio/gpio-bcm-kona.c > > > > b/drivers/gpio/gpio-bcm-kona.c index 4122683eb1f9..c50721980a7c > > > 100644 > > > > --- a/drivers/gpio/gpio-bcm-kona.c > > > > +++ b/drivers/gpio/gpio-bcm-kona.c > > > > @@ -19,7 +19,6 @@ > > > > #include <linux/io.h> > > > > #include <linux/gpio/driver.h> > > > > #include <linux/of_device.h> > > > > -#include <linux/of_irq.h> > > > > #include <linux/init.h> > > > > #include <linux/irqdomain.h> > > > > #include <linux/irqchip/chained_irq.h> @@ -586,7 +585,7 @@ static > > > > int bcm_kona_gpio_probe(struct platform_device *pdev) > > > > > > > > kona_gpio->gpio_chip = template_chip; > > > > chip = &kona_gpio->gpio_chip; > > > > - kona_gpio->num_bank = of_irq_count(dev->of_node); > > > > + kona_gpio->num_bank = platform_irq_count(pdev); > > > > > > of_irq_count returns 0 or a positive int while platform_irq_count > > > might return a negative error code. This needs handling. Also I > > > wonder why > > > platform_irq_count() is better than of_irq_count() which would be > > > good to describe in the commit log. > > > > Inspired from > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml > > .org%2Flkml%2F2015%2F11%2F18%2F466&data=02%7C01%7Cpeng.f > an%40nxp.c > > > om%7C46364bf12479463df7d308d777c09b39%7C686ea1d3bc2b4c6fa92cd9 > 9c5c3016 > > > 35%7C0%7C0%7C637109541440912984&sdata=awTvwwmo4692Hx7IJ > H%2BllEVJH7 > > ngINufoMH8UsosU%2BA%3D&reserved=0 > > From Rob: > > " > > So I started looking at why you are using of_irq_count which drivers > > shouldn't need to. In patch 5 you use it to allocate memory to store > > the irq names, then use them here... > > " > > > > Is this ok? > > I would say something like: > > platform_irq_count() is the more generic way (independent of > device trees) to determine the count of available interrupts. So > use this instead. > > As platform_irq_count() might return an error code (which > of_irq_count doesn't) some additional handling is necessary. Thanks, how about this change? @@ -586,11 +585,15 @@ static int bcm_kona_gpio_probe(struct platform_device *pdev) kona_gpio->gpio_chip = template_chip; chip = &kona_gpio->gpio_chip; - kona_gpio->num_bank = of_irq_count(dev->of_node); - if (kona_gpio->num_bank == 0) { + ret = platform_irq_count(pdev); + if (!ret) { dev_err(dev, "Couldn't determine # GPIO banks\n"); return -ENOENT; + } else if (ret < 0) { + return ret; } + kona_gpio->num_bank = ret; + if (kona_gpio->num_bank > GPIO_MAX_BANK_NUM) { Thanks, Peng. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww. > pengutronix.de%2F&data=02%7C01%7Cpeng.fan%40nxp.com%7C46364 > bf12479463df7d308d777c09b39%7C686ea1d3bc2b4c6fa92cd99c5c301635% > 7C0%7C0%7C637109541440922981&sdata=8zAFDgy56%2BjS4I4dUvB6 > y37AMHBu1PDdzESa6c2jUlc%3D&reserved=0 |
On Tue, Dec 03, 2019 at 07:23:14AM +0000, Peng Fan wrote: > > Subject: Re: [PATCH 2/2] gpio: bcm-kona: use platform_irq_count > > > > On Tue, Dec 03, 2019 at 07:00:40AM +0000, Peng Fan wrote: > > > > Subject: Re: [PATCH 2/2] gpio: bcm-kona: use platform_irq_count > > > > > > > > On Tue, Dec 03, 2019 at 06:04:27AM +0000, Peng Fan wrote: > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > > > Use platform_irq_count to replace of_irq_count > > > > > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > > --- > > > > > > > > > > V1: > > > > > Code inspection, not tested > > > > > > > > > > drivers/gpio/gpio-bcm-kona.c | 3 +-- > > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpio/gpio-bcm-kona.c > > > > > b/drivers/gpio/gpio-bcm-kona.c index 4122683eb1f9..c50721980a7c > > > > 100644 > > > > > --- a/drivers/gpio/gpio-bcm-kona.c > > > > > +++ b/drivers/gpio/gpio-bcm-kona.c > > > > > @@ -19,7 +19,6 @@ > > > > > #include <linux/io.h> > > > > > #include <linux/gpio/driver.h> > > > > > #include <linux/of_device.h> > > > > > -#include <linux/of_irq.h> > > > > > #include <linux/init.h> > > > > > #include <linux/irqdomain.h> > > > > > #include <linux/irqchip/chained_irq.h> @@ -586,7 +585,7 @@ static > > > > > int bcm_kona_gpio_probe(struct platform_device *pdev) > > > > > > > > > > kona_gpio->gpio_chip = template_chip; > > > > > chip = &kona_gpio->gpio_chip; > > > > > - kona_gpio->num_bank = of_irq_count(dev->of_node); > > > > > + kona_gpio->num_bank = platform_irq_count(pdev); > > > > > > > > of_irq_count returns 0 or a positive int while platform_irq_count > > > > might return a negative error code. This needs handling. Also I > > > > wonder why > > > > platform_irq_count() is better than of_irq_count() which would be > > > > good to describe in the commit log. > > > > > > Inspired from > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml > > > .org%2Flkml%2F2015%2F11%2F18%2F466&data=02%7C01%7Cpeng.f > > an%40nxp.c > > > > > om%7C46364bf12479463df7d308d777c09b39%7C686ea1d3bc2b4c6fa92cd9 > > 9c5c3016 > > > > > 35%7C0%7C0%7C637109541440912984&sdata=awTvwwmo4692Hx7IJ > > H%2BllEVJH7 > > > ngINufoMH8UsosU%2BA%3D&reserved=0 > > > From Rob: > > > " > > > So I started looking at why you are using of_irq_count which drivers > > > shouldn't need to. In patch 5 you use it to allocate memory to store > > > the irq names, then use them here... > > > " > > > > > > Is this ok? > > > > I would say something like: > > > > platform_irq_count() is the more generic way (independent of > > device trees) to determine the count of available interrupts. So > > use this instead. > > > > As platform_irq_count() might return an error code (which > > of_irq_count doesn't) some additional handling is necessary. > > Thanks, how about this change? > @@ -586,11 +585,15 @@ static int bcm_kona_gpio_probe(struct platform_device *pdev) > > kona_gpio->gpio_chip = template_chip; > chip = &kona_gpio->gpio_chip; > - kona_gpio->num_bank = of_irq_count(dev->of_node); > - if (kona_gpio->num_bank == 0) { > + ret = platform_irq_count(pdev); > + if (!ret) { > dev_err(dev, "Couldn't determine # GPIO banks\n"); > return -ENOENT; > + } else if (ret < 0) { > + return ret; This is inconsitent. In the ret==0 case you emit an error message, in the ret < 0 case you don't. I think the sensible approach would be to do: if (ret == 0) { dev_err(dev, "Couldn't determine # GPIO banks\n"); return -ENOENT; } else if (ret < 0) { if (ret != -EPROBE_DEFER) dev_err(dev, "Failed to determine count of GPIO banks (%pe)\n", ERR_PTR(ret)); return ret; } Best regards Uwe
> Subject: Re: [PATCH 2/2] gpio: bcm-kona: use platform_irq_count > > On Tue, Dec 03, 2019 at 07:23:14AM +0000, Peng Fan wrote: > > > Subject: Re: [PATCH 2/2] gpio: bcm-kona: use platform_irq_count > > > > > > On Tue, Dec 03, 2019 at 07:00:40AM +0000, Peng Fan wrote: > > > > > Subject: Re: [PATCH 2/2] gpio: bcm-kona: use platform_irq_count > > > > > > > > > > On Tue, Dec 03, 2019 at 06:04:27AM +0000, Peng Fan wrote: > > > > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > > > > > > > Use platform_irq_count to replace of_irq_count > > > > > > > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > > > > > --- > > > > > > > > > > > > V1: > > > > > > Code inspection, not tested > > > > > > > > > > > > drivers/gpio/gpio-bcm-kona.c | 3 +-- > > > > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpio/gpio-bcm-kona.c > > > > > > b/drivers/gpio/gpio-bcm-kona.c index > > > > > > 4122683eb1f9..c50721980a7c > > > > > 100644 > > > > > > --- a/drivers/gpio/gpio-bcm-kona.c > > > > > > +++ b/drivers/gpio/gpio-bcm-kona.c > > > > > > @@ -19,7 +19,6 @@ > > > > > > #include <linux/io.h> > > > > > > #include <linux/gpio/driver.h> #include <linux/of_device.h> > > > > > > -#include <linux/of_irq.h> #include <linux/init.h> #include > > > > > > <linux/irqdomain.h> #include <linux/irqchip/chained_irq.h> @@ > > > > > > -586,7 +585,7 @@ static int bcm_kona_gpio_probe(struct > > > > > > platform_device *pdev) > > > > > > > > > > > > kona_gpio->gpio_chip = template_chip; > > > > > > chip = &kona_gpio->gpio_chip; > > > > > > - kona_gpio->num_bank = of_irq_count(dev->of_node); > > > > > > + kona_gpio->num_bank = platform_irq_count(pdev); > > > > > > > > > > of_irq_count returns 0 or a positive int while > > > > > platform_irq_count might return a negative error code. This > > > > > needs handling. Also I wonder why > > > > > platform_irq_count() is better than of_irq_count() which would > > > > > be good to describe in the commit log. > > > > > > > > Inspired from > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > > > lkml .org%2Flkml%2F2015%2F11%2F18%2F466&data=02%7C01%7Cpen > g.f > > > an%40nxp.c > > > > > > > > om%7C46364bf12479463df7d308d777c09b39%7C686ea1d3bc2b4c6fa92cd9 > > > 9c5c3016 > > > > > > > > 35%7C0%7C0%7C637109541440912984&sdata=awTvwwmo4692Hx7IJ > > > H%2BllEVJH7 > > > > ngINufoMH8UsosU%2BA%3D&reserved=0 > > > > From Rob: > > > > " > > > > So I started looking at why you are using of_irq_count which > > > > drivers shouldn't need to. In patch 5 you use it to allocate > > > > memory to store the irq names, then use them here... > > > > " > > > > > > > > Is this ok? > > > > > > I would say something like: > > > > > > platform_irq_count() is the more generic way (independent of > > > device trees) to determine the count of available interrupts. So > > > use this instead. > > > > > > As platform_irq_count() might return an error code (which > > > of_irq_count doesn't) some additional handling is necessary. > > > > Thanks, how about this change? > > @@ -586,11 +585,15 @@ static int bcm_kona_gpio_probe(struct > > platform_device *pdev) > > > > kona_gpio->gpio_chip = template_chip; > > chip = &kona_gpio->gpio_chip; > > - kona_gpio->num_bank = of_irq_count(dev->of_node); > > - if (kona_gpio->num_bank == 0) { > > + ret = platform_irq_count(pdev); > > + if (!ret) { > > dev_err(dev, "Couldn't determine # GPIO banks\n"); > > return -ENOENT; > > + } else if (ret < 0) { > > + return ret; > > This is inconsitent. In the ret==0 case you emit an error message, in the ret < > 0 case you don't. I think the sensible approach would be to > do: > > if (ret == 0) { !ret should be fine, I think checkpatch might trigger warning using ret == 0. > dev_err(dev, "Couldn't determine # GPIO banks\n"); > return -ENOENT; > } else if (ret < 0) { > if (ret != -EPROBE_DEFER) > dev_err(dev, "Failed to determine count of GPIO banks > (%pe)\n", ERR_PTR(ret)); > return ret; > } ok, will use this for v2. Thanks, Peng. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww. > pengutronix.de%2F&data=02%7C01%7Cpeng.fan%40nxp.com%7Cab060 > 7335df3485a98e908d777c3d594%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C637109555298587766&sdata=9NTBkwRTIuFBF9y97jqyr > b4Vym8FpvOTZYBrqJqEERk%3D&reserved=0 |
wt., 3 gru 2019 o 07:57 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> napisał(a): > > On Tue, Dec 03, 2019 at 06:04:27AM +0000, Peng Fan wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > Use platform_irq_count to replace of_irq_count > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > > > V1: > > Code inspection, not tested > > > > drivers/gpio/gpio-bcm-kona.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c > > index 4122683eb1f9..c50721980a7c 100644 > > --- a/drivers/gpio/gpio-bcm-kona.c > > +++ b/drivers/gpio/gpio-bcm-kona.c > > @@ -19,7 +19,6 @@ > > #include <linux/io.h> > > #include <linux/gpio/driver.h> > > #include <linux/of_device.h> > > -#include <linux/of_irq.h> > > #include <linux/init.h> > > #include <linux/irqdomain.h> > > #include <linux/irqchip/chained_irq.h> > > @@ -586,7 +585,7 @@ static int bcm_kona_gpio_probe(struct platform_device *pdev) > > > > kona_gpio->gpio_chip = template_chip; > > chip = &kona_gpio->gpio_chip; > > - kona_gpio->num_bank = of_irq_count(dev->of_node); > > + kona_gpio->num_bank = platform_irq_count(pdev); > > of_irq_count returns 0 or a positive int while platform_irq_count might > return a negative error code. This needs handling. Also I wonder why > platform_irq_count() is better than of_irq_count() which would be good > to describe in the commit log. > In general I like drivers to use generic APIs whenever possible. There are a lot of drivers that use some random of_ routines just because the developer didn't know any better and it turns out we now have generic device properties and a lot of platform device helpers. Consolidation is always good. Waiting for v2. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
diff --git a/drivers/gpio/gpio-bcm-kona.c b/drivers/gpio/gpio-bcm-kona.c index 4122683eb1f9..c50721980a7c 100644 --- a/drivers/gpio/gpio-bcm-kona.c +++ b/drivers/gpio/gpio-bcm-kona.c @@ -19,7 +19,6 @@ #include <linux/io.h> #include <linux/gpio/driver.h> #include <linux/of_device.h> -#include <linux/of_irq.h> #include <linux/init.h> #include <linux/irqdomain.h> #include <linux/irqchip/chained_irq.h> @@ -586,7 +585,7 @@ static int bcm_kona_gpio_probe(struct platform_device *pdev) kona_gpio->gpio_chip = template_chip; chip = &kona_gpio->gpio_chip; - kona_gpio->num_bank = of_irq_count(dev->of_node); + kona_gpio->num_bank = platform_irq_count(pdev); if (kona_gpio->num_bank == 0) { dev_err(dev, "Couldn't determine # GPIO banks\n"); return -ENOENT;