[PATCH] pinctrl: sh-pfc: Convert to platform_get_*()

[PATCH] pinctrl: sh-pfc: Convert to platform_get_*()

[Geert Uytterhoeven]

If the pin function controller (which can be a GPIO controller) is
instantiated before the interrupt controllers, due to the ordering in
the DTS, the irq domains for the interrupt controllers referenced by its
"interrupts-extended" property cannot be found yet:

    irq: no irq domain found for /interrupt-controller@e61c0000 !

As the sh-pfc driver accesses the platform device's resources directly,
it cannot find the (optional) IRQ resources, and thinks no interrupts
are available. This may lead to failures later, when GPIOs are used as
interupts:

    gpio-keys keyboard: Unable to claim irq 0; error -22
    gpio-keys: probe of keyboard failed with error -22

To fix this, add support for deferred probing to sh-pfc, by converting
the driver from direct platform device resource access to using the
platform_get_resource() and platform_get_irq() helpers.

Note that while this fixes the root cause worked around by commit
e4ba0a9bddff3ba5 ("ARM: shmobile: r8a73a4: Move pfc node to work around
probe ordering bug"), I strongly recommend against reverting the
workaround now, as this would lead to lots of probe deferrals in drivers
relying on pinctrl. This may be reconsidered once the DT code starts
taking into account phandle dependencies during device instantation.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
This patch is against next-20150625

"[PATCH] [RFC] OF: probe order dependency aware of_platform_populate"
(https://www.marc.info/?l=devicetree&m=141873189825553&w=1) is a first
step, but it doesn't postpone the instantiation of the pfc.

Tested:
  - r8a73a4/ape6evm (with pfc before/after irqc in DT),
  - sh73a0/kzm9g (with pfc before/after intc-irqpin in DT).

Regression-tested:
  - r8a7791/koelsch (pfc doesn't have interrupts),
  - r8a7740/armadillo (pfc after intc-irqpin in DT),
  - r8a7740/armadillo-legacy (gpio-keys wired to pfc),
  - sh73a0/kzm9g-legacy (gpio-keys not wired to pfc).

Compile-tested:
  - sh/se7724_defconfig.
---
 drivers/pinctrl/sh-pfc/core.c | 46 ++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
index 865d235612c5200a..9796238959047508 100644
--- a/drivers/pinctrl/sh-pfc/core.c
+++ b/drivers/pinctrl/sh-pfc/core.c
@@ -29,24 +29,25 @@
 static int sh_pfc_map_resources(struct sh_pfc *pfc,
 				struct platform_device *pdev)
 {
-	unsigned int num_windows = 0;
-	unsigned int num_irqs = 0;
+	unsigned int num_windows, num_irqs;
 	struct sh_pfc_window *windows;
 	unsigned int *irqs = NULL;
 	struct resource *res;
 	unsigned int i;
+	int irq;
 
 	/* Count the MEM and IRQ resources. */
-	for (i = 0; i < pdev->num_resources; ++i) {
-		switch (resource_type(&pdev->resource[i])) {
-		case IORESOURCE_MEM:
-			num_windows++;
+	for (num_windows = 0;; num_windows++) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, num_windows);
+		if (!res)
 			break;
-
-		case IORESOURCE_IRQ:
-			num_irqs++;
+	}
+	for (num_irqs = 0;; num_irqs++) {
+		irq = platform_get_irq(pdev, num_irqs);
+		if (irq == -EPROBE_DEFER)
+			return irq;
+		if (irq < 0)
 			break;
-		}
 	}
 
 	if (num_windows == 0)
@@ -72,22 +73,17 @@ static int sh_pfc_map_resources(struct sh_pfc *pfc,
 	}
 
 	/* Fill them. */
-	for (i = 0, res = pdev->resource; i < pdev->num_resources; i++, res++) {
-		switch (resource_type(res)) {
-		case IORESOURCE_MEM:
-			windows->phys = res->start;
-			windows->size = resource_size(res);
-			windows->virt = devm_ioremap_resource(pfc->dev, res);
-			if (IS_ERR(windows->virt))
-				return -ENOMEM;
-			windows++;
-			break;
-
-		case IORESOURCE_IRQ:
-			*irqs++ = res->start;
-			break;
-		}
+	for (i = 0; i < num_windows; i++) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		windows->phys = res->start;
+		windows->size = resource_size(res);
+		windows->virt = devm_ioremap_resource(pfc->dev, res);
+		if (IS_ERR(windows->virt))
+			return -ENOMEM;
+		windows++;
 	}
+	for (i = 0; i < num_irqs; i++)
+		*irqs++ = platform_get_irq(pdev, i);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH] pinctrl: sh-pfc: Convert to platform_get_*()

[Linus Walleij]

On Thu, Jun 25, 2015 at 11:39 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

> If the pin function controller (which can be a GPIO controller) is
> instantiated before the interrupt controllers, due to the ordering in
> the DTS, the irq domains for the interrupt controllers referenced by its
> "interrupts-extended" property cannot be found yet:
>
>     irq: no irq domain found for /interrupt-controller@e61c0000 !
>
> As the sh-pfc driver accesses the platform device's resources directly,
> it cannot find the (optional) IRQ resources, and thinks no interrupts
> are available. This may lead to failures later, when GPIOs are used as
> interupts:
>
>     gpio-keys keyboard: Unable to claim irq 0; error -22
>     gpio-keys: probe of keyboard failed with error -22
>
> To fix this, add support for deferred probing to sh-pfc, by converting
> the driver from direct platform device resource access to using the
> platform_get_resource() and platform_get_irq() helpers.
>
> Note that while this fixes the root cause worked around by commit
> e4ba0a9bddff3ba5 ("ARM: shmobile: r8a73a4: Move pfc node to work around
> probe ordering bug"), I strongly recommend against reverting the
> workaround now, as this would lead to lots of probe deferrals in drivers
> relying on pinctrl. This may be reconsidered once the DT code starts
> taking into account phandle dependencies during device instantation.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> This patch is against next-20150625
>
> "[PATCH] [RFC] OF: probe order dependency aware of_platform_populate"
> (https://www.marc.info/?l=devicetree&m=141873189825553&w=1) is a first
> step, but it doesn't postpone the instantiation of the pfc.
>
> Tested:
>   - r8a73a4/ape6evm (with pfc before/after irqc in DT),
>   - sh73a0/kzm9g (with pfc before/after intc-irqpin in DT).
>
> Regression-tested:
>   - r8a7791/koelsch (pfc doesn't have interrupts),
>   - r8a7740/armadillo (pfc after intc-irqpin in DT),
>   - r8a7740/armadillo-legacy (gpio-keys wired to pfc),
>   - sh73a0/kzm9g-legacy (gpio-keys not wired to pfc).
>
> Compile-tested:
>   - sh/se7724_defconfig.

Waiting for Laurent's ACK on this.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: sh-pfc: Convert to platform_get_*()

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Thursday 25 June 2015 11:39:53 Geert Uytterhoeven wrote:
> If the pin function controller (which can be a GPIO controller) is
> instantiated before the interrupt controllers, due to the ordering in
> the DTS, the irq domains for the interrupt controllers referenced by its
> "interrupts-extended" property cannot be found yet:
> 
>     irq: no irq domain found for /interrupt-controller@e61c0000 !
> 
> As the sh-pfc driver accesses the platform device's resources directly,
> it cannot find the (optional) IRQ resources, and thinks no interrupts
> are available. This may lead to failures later, when GPIOs are used as
> interupts:
> 
>     gpio-keys keyboard: Unable to claim irq 0; error -22
>     gpio-keys: probe of keyboard failed with error -22
> 
> To fix this, add support for deferred probing to sh-pfc, by converting
> the driver from direct platform device resource access to using the
> platform_get_resource() and platform_get_irq() helpers.
> 
> Note that while this fixes the root cause worked around by commit
> e4ba0a9bddff3ba5 ("ARM: shmobile: r8a73a4: Move pfc node to work around
> probe ordering bug"), I strongly recommend against reverting the
> workaround now, as this would lead to lots of probe deferrals in drivers
> relying on pinctrl. This may be reconsidered once the DT code starts
> taking into account phandle dependencies during device instantation.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> This patch is against next-20150625
> 
> "[PATCH] [RFC] OF: probe order dependency aware of_platform_populate"
> (https://www.marc.info/?l=devicetree&m=141873189825553&w=1) is a first
> step, but it doesn't postpone the instantiation of the pfc.
> 
> Tested:
>   - r8a73a4/ape6evm (with pfc before/after irqc in DT),
>   - sh73a0/kzm9g (with pfc before/after intc-irqpin in DT).
> 
> Regression-tested:
>   - r8a7791/koelsch (pfc doesn't have interrupts),
>   - r8a7740/armadillo (pfc after intc-irqpin in DT),
>   - r8a7740/armadillo-legacy (gpio-keys wired to pfc),
>   - sh73a0/kzm9g-legacy (gpio-keys not wired to pfc).
> 
> Compile-tested:
>   - sh/se7724_defconfig.
> ---
>  drivers/pinctrl/sh-pfc/core.c | 46 ++++++++++++++++++----------------------
>  1 file changed, 21 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
> index 865d235612c5200a..9796238959047508 100644
> --- a/drivers/pinctrl/sh-pfc/core.c
> +++ b/drivers/pinctrl/sh-pfc/core.c
> @@ -29,24 +29,25 @@
>  static int sh_pfc_map_resources(struct sh_pfc *pfc,
>  				struct platform_device *pdev)
>  {
> -	unsigned int num_windows = 0;
> -	unsigned int num_irqs = 0;
> +	unsigned int num_windows, num_irqs;
>  	struct sh_pfc_window *windows;
>  	unsigned int *irqs = NULL;
>  	struct resource *res;
>  	unsigned int i;
> +	int irq;
> 
>  	/* Count the MEM and IRQ resources. */
> -	for (i = 0; i < pdev->num_resources; ++i) {
> -		switch (resource_type(&pdev->resource[i])) {
> -		case IORESOURCE_MEM:
> -			num_windows++;
> +	for (num_windows = 0;; num_windows++) {

Just a bit of nit-picking, I'd add a space between the two ; (same for the 
next loop).

> +		res = platform_get_resource(pdev, IORESOURCE_MEM, num_windows);
> +		if (!res)
>  			break;
> -
> -		case IORESOURCE_IRQ:
> -			num_irqs++;
> +	}

And a blank line here.

The rest looks fine to me.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	for (num_irqs = 0;; num_irqs++) {
> +		irq = platform_get_irq(pdev, num_irqs);
> +		if (irq == -EPROBE_DEFER)
> +			return irq;
> +		if (irq < 0)
>  			break;
> -		}
>  	}
> 
>  	if (num_windows == 0)
> @@ -72,22 +73,17 @@ static int sh_pfc_map_resources(struct sh_pfc *pfc,
>  	}
> 
>  	/* Fill them. */
> -	for (i = 0, res = pdev->resource; i < pdev->num_resources; i++, res++) {
> -		switch (resource_type(res)) {
> -		case IORESOURCE_MEM:
> -			windows->phys = res->start;
> -			windows->size = resource_size(res);
> -			windows->virt = devm_ioremap_resource(pfc->dev, res);
> -			if (IS_ERR(windows->virt))
> -				return -ENOMEM;
> -			windows++;
> -			break;
> -
> -		case IORESOURCE_IRQ:
> -			*irqs++ = res->start;
> -			break;
> -		}
> +	for (i = 0; i < num_windows; i++) {
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> +		windows->phys = res->start;
> +		windows->size = resource_size(res);
> +		windows->virt = devm_ioremap_resource(pfc->dev, res);
> +		if (IS_ERR(windows->virt))
> +			return -ENOMEM;
> +		windows++;
>  	}
> +	for (i = 0; i < num_irqs; i++)
> +		*irqs++ = platform_get_irq(pdev, i);
> 
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH] pinctrl: sh-pfc: Convert to platform_get_*()

[Geert Uytterhoeven]

Hi Laurent,

On Thu, Jul 16, 2015 at 10:41 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>>  drivers/pinctrl/sh-pfc/core.c | 46 ++++++++++++++++++----------------------
>>  1 file changed, 21 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c
>> index 865d235612c5200a..9796238959047508 100644
>> --- a/drivers/pinctrl/sh-pfc/core.c
>> +++ b/drivers/pinctrl/sh-pfc/core.c
>> @@ -29,24 +29,25 @@
>>  static int sh_pfc_map_resources(struct sh_pfc *pfc,
>>                               struct platform_device *pdev)
>>  {
>> -     unsigned int num_windows = 0;
>> -     unsigned int num_irqs = 0;
>> +     unsigned int num_windows, num_irqs;
>>       struct sh_pfc_window *windows;
>>       unsigned int *irqs = NULL;
>>       struct resource *res;
>>       unsigned int i;
>> +     int irq;
>>
>>       /* Count the MEM and IRQ resources. */
>> -     for (i = 0; i < pdev->num_resources; ++i) {
>> -             switch (resource_type(&pdev->resource[i])) {
>> -             case IORESOURCE_MEM:
>> -                     num_windows++;
>> +     for (num_windows = 0;; num_windows++) {
>
> Just a bit of nit-picking, I'd add a space between the two ; (same for the
> next loop).

I had done my research ;-)

$ git grep 'for.*;;' | wc -l
1410
$ git grep 'for.*; ;' | wc -l
190
$

>> +             res = platform_get_resource(pdev, IORESOURCE_MEM, num_windows);
>> +             if (!res)
>>                       break;
>> -
>> -             case IORESOURCE_IRQ:
>> -                     num_irqs++;
>> +     }
>
> And a blank line here.

I didn't add one, to make clear the block falls under the "Count the MEM
and IRQ resources. " comment, too.

Let's leave this to Linus to decide...

> The rest looks fine to me.
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] pinctrl: sh-pfc: Convert to platform_get_*()

[Linus Walleij]

On Thu, Jun 25, 2015 at 11:39 AM, Geert Uytterhoeven
<geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org> wrote:

> If the pin function controller (which can be a GPIO controller) is
> instantiated before the interrupt controllers, due to the ordering in
> the DTS, the irq domains for the interrupt controllers referenced by its
> "interrupts-extended" property cannot be found yet:
>
>     irq: no irq domain found for /interrupt-controller@e61c0000 !
>
> As the sh-pfc driver accesses the platform device's resources directly,
> it cannot find the (optional) IRQ resources, and thinks no interrupts
> are available. This may lead to failures later, when GPIOs are used as
> interupts:
>
>     gpio-keys keyboard: Unable to claim irq 0; error -22
>     gpio-keys: probe of keyboard failed with error -22
>
> To fix this, add support for deferred probing to sh-pfc, by converting
> the driver from direct platform device resource access to using the
> platform_get_resource() and platform_get_irq() helpers.
>
> Note that while this fixes the root cause worked around by commit
> e4ba0a9bddff3ba5 ("ARM: shmobile: r8a73a4: Move pfc node to work around
> probe ordering bug"), I strongly recommend against reverting the
> workaround now, as this would lead to lots of probe deferrals in drivers
> relying on pinctrl. This may be reconsidered once the DT code starts
> taking into account phandle dependencies during device instantation.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>

Patch applied with Laurent's ACK.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html