| Message ID | 1576672860-14420-1-git-send-email-peng.fan@nxp.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 06e26b75f5e613b400116fdb7ff6206a681ab271 |
| Headers | show |
| Series | [1/2] pinctrl: mvebu: armada-37xx: use use platform api | expand |
On Wed, Dec 18, 2019 at 9:44 AM Peng Fan <peng.fan@nxp.com> wrote: > - nr_irq_parent = of_irq_count(np); > + nr_irq_parent = platform_irq_count(pdev); > + if (nr_irq_parent < 0) { > + if (nr_irq_parent != -EPROBE_DEFER) > + dev_err(dev, "Couldn't determine irq count: %pe\n", > + ERR_PTR(nr_irq_parent)); Why do you return ERR_PTR(nr_irq_parent) instead of simply nr_irq_parent?
> Subject: Re: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api > > On Wed, Dec 18, 2019 at 9:44 AM Peng Fan <peng.fan@nxp.com> wrote: > > > - nr_irq_parent = of_irq_count(np); > > + nr_irq_parent = platform_irq_count(pdev); > > + if (nr_irq_parent < 0) { > > + if (nr_irq_parent != -EPROBE_DEFER) > > + dev_err(dev, "Couldn't determine irq > count: %pe\n", > > + ERR_PTR(nr_irq_parent)); > > Why do you return ERR_PTR(nr_irq_parent) instead of simply nr_irq_parent? Please see: https://lkml.org/lkml/2019/12/4/64 and the patch in tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/ commit/?id=cfdca14c44a79b9c9c491235a39b9fc1e520820b Thanks, Peng.
Hi Peng, On Wed, Dec 18, 2019 at 9:53 AM Peng Fan <peng.fan@nxp.com> wrote: > Please see: > https://lkml.org/lkml/2019/12/4/64 Still think it makes things more complicated than simply returning the error code directly.
On Wed, Dec 18, 2019 at 09:57:57AM -0300, Fabio Estevam wrote: > Hi Peng, > > On Wed, Dec 18, 2019 at 9:53 AM Peng Fan <peng.fan@nxp.com> wrote: > > > Please see: > > https://lkml.org/lkml/2019/12/4/64 + dev_err(dev, "Couldn't determine irq count: %pe\n", + ERR_PTR(nr_irq_parent)); > Still think it makes things more complicated than simply returning the > error code directly. Hi Fabio Look closer. This is not about returning an error, it is about printing an error. I think the API could better. A %ie formatter would make a lot of sense, so avoiding the ERR_PTR(). Andrew
Hi Andrew, On Wed, Dec 18, 2019 at 12:00 PM Andrew Lunn <andrew@lunn.ch> wrote: > Hi Fabio > > Look closer. This is not about returning an error, it is about > printing an error. > > I think the API could better. A %ie formatter would make a lot of > sense, so avoiding the ERR_PTR(). Yes, I think that returning the error like: dev_err(dev, "Couldn't determine irq count: %d\n", nr_irq_parent); would make the code cleaner. Maybe just a matter of taste though ;-)
On Wed, Dec 18, 2019 at 12:09 PM Fabio Estevam <festevam@gmail.com> wrote: > Yes, I think that returning the error like: s/returning/printing > dev_err(dev, "Couldn't determine irq count: %d\n", nr_irq_parent); > > would make the code cleaner. > > Maybe just a matter of taste though ;-)
Hello Fabio, On Wed, Dec 18, 2019 at 12:09:42PM -0300, Fabio Estevam wrote: > On Wed, Dec 18, 2019 at 12:00 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > Hi Fabio > > > > Look closer. This is not about returning an error, it is about > > printing an error. > > > > I think the API could better. A %ie formatter would make a lot of > > sense, so avoiding the ERR_PTR(). > > Yes, I think that returning the error like: > > dev_err(dev, "Couldn't determine irq count: %d\n", nr_irq_parent); > > would make the code cleaner. Are you aware of the semantic difference between dev_err(..., "Couldn't determine irq count: %d\n", nr_irq_parent); and dev_err(..., "Couldn't determine irq count: %pe\n", ERR_PTR(nr_irq_parent)); ? The first yields: Couldn't determine irq count: -5 while the latter yields Couldn't determine irq count: -EIO which is more expressive. I agree that having a format for printing an integer error code would be useful. I have this on my todo-list but having some %pe with ERR_PTR conversion would help me arguing my case. So I would like the patch to go in with ERR_PTR even though v2 was sent using %d today. Best regards Uwe
Hi Linus, > Subject: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api Would you pick this patch up? Per Uwe, this v1 patch use %pe is better that v2 use %d. Thanks, Peng. > > From: Peng Fan <peng.fan@nxp.com> > > platform_irq_count() and platform_get_irq() 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. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/pinctrl/mvebu/pinctrl-armada-37xx.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c > b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c > index aa9dcde0f069..cc66a6429a06 100644 > --- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c > +++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c > @@ -15,7 +15,6 @@ > #include <linux/of.h> > #include <linux/of_address.h> > #include <linux/of_device.h> > -#include <linux/of_irq.h> > #include <linux/pinctrl/pinconf-generic.h> #include > <linux/pinctrl/pinconf.h> #include <linux/pinctrl/pinctrl.h> @@ -739,7 > +738,14 @@ static int armada_37xx_irqchip_register(struct platform_device > *pdev, > return ret; > } > > - nr_irq_parent = of_irq_count(np); > + nr_irq_parent = platform_irq_count(pdev); > + if (nr_irq_parent < 0) { > + if (nr_irq_parent != -EPROBE_DEFER) > + dev_err(dev, "Couldn't determine irq count: %pe\n", > + ERR_PTR(nr_irq_parent)); > + return nr_irq_parent; > + } > + > spin_lock_init(&info->irq_lock); > > if (!nr_irq_parent) { > @@ -776,7 +782,7 @@ static int armada_37xx_irqchip_register(struct > platform_device *pdev, > if (!girq->parents) > return -ENOMEM; > for (i = 0; i < nr_irq_parent; i++) { > - int irq = irq_of_parse_and_map(np, i); > + int irq = platform_get_irq(pdev, i); > > if (irq < 0) > continue; > -- > 2.16.4
On Thu, Jan 16, 2020 at 9:28 AM Peng Fan <peng.fan@nxp.com> wrote: > Hi Linus, > > > Subject: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api > > Would you pick this patch up? > > Per Uwe, this v1 patch use %pe is better that v2 use %d. OK patch applied to my devel branch for v5.6. Yours, Linus Walleij
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c index aa9dcde0f069..cc66a6429a06 100644 --- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c +++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c @@ -15,7 +15,6 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_device.h> -#include <linux/of_irq.h> #include <linux/pinctrl/pinconf-generic.h> #include <linux/pinctrl/pinconf.h> #include <linux/pinctrl/pinctrl.h> @@ -739,7 +738,14 @@ static int armada_37xx_irqchip_register(struct platform_device *pdev, return ret; } - nr_irq_parent = of_irq_count(np); + nr_irq_parent = platform_irq_count(pdev); + if (nr_irq_parent < 0) { + if (nr_irq_parent != -EPROBE_DEFER) + dev_err(dev, "Couldn't determine irq count: %pe\n", + ERR_PTR(nr_irq_parent)); + return nr_irq_parent; + } + spin_lock_init(&info->irq_lock); if (!nr_irq_parent) { @@ -776,7 +782,7 @@ static int armada_37xx_irqchip_register(struct platform_device *pdev, if (!girq->parents) return -ENOMEM; for (i = 0; i < nr_irq_parent; i++) { - int irq = irq_of_parse_and_map(np, i); + int irq = platform_get_irq(pdev, i); if (irq < 0) continue;