[PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api

[PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api

[Peng Fan]

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

[PATCH 2/2] pinctrl: sunxi: sun50i-h5 use platform_irq_count

[Peng Fan]

From: Peng Fan <peng.fan@nxp.com>

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.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/pinctrl/sunxi/pinctrl-sun50i-h5.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sun50i-h5.c b/drivers/pinctrl/sunxi/pinctrl-sun50i-h5.c
index a78d7b922ef4..31d62bbb7f43 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sun50i-h5.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sun50i-h5.c
@@ -19,7 +19,6 @@
 #include <linux/platform_device.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
-#include <linux/of_irq.h>
 #include <linux/pinctrl/pinctrl.h>
 
 #include "pinctrl-sunxi.h"
@@ -549,7 +548,17 @@ static const struct sunxi_pinctrl_desc sun50i_h5_pinctrl_data = {
 
 static int sun50i_h5_pinctrl_probe(struct platform_device *pdev)
 {
-	switch (of_irq_count(pdev->dev.of_node)) {
+	int ret;
+
+	ret = platform_irq_count(pdev);
+	if (ret < 0) {
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Couldn't determine irq count: %pe\n",
+				ERR_PTR(ret));
+		return ret;
+	}
+
+	switch (ret) {
 	case 2:
 		dev_warn(&pdev->dev,
 			 "Your device tree's pinctrl node is broken, which has no IRQ of PG bank routed.\n");
-- 
2.16.4

Re: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api

[Fabio Estevam]

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?

RE: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api

[Peng Fan]

> 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.

Re: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api

[Fabio Estevam]

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.

Re: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api

[Andrew Lunn]

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

Re: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api

[Fabio Estevam]

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 ;-)

Re: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api

[Fabio Estevam]

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 ;-)

Re: [PATCH 2/2] pinctrl: sunxi: sun50i-h5 use platform_irq_count

[Linus Walleij]

On Wed, Dec 18, 2019 at 1:43 PM Peng Fan <peng.fan@nxp.com> wrote:

> From: Peng Fan <peng.fan@nxp.com>
>
> 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.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

This is clearly nicer code, patch applied.
(You only need to reiterate patch 1/2 if you decide to
keep working on that.)

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

RE: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api

[Peng Fan]

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

Re: [PATCH 1/2] pinctrl: mvebu: armada-37xx: use use platform api

[Linus Walleij]

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