diff mbox

[RFC] pinctrl: sh-pfc: propagate errors on group config

Message ID 1466404822-2540-1-git-send-email-wsa@the-dreams.de
State New
Headers show

Commit Message

Wolfram Sang June 20, 2016, 6:40 a.m. UTC 
From: Wolfram Sang <wsa+renesas@sang-engineering.com>

On group configuration, bail out if setting one of the individual pins
fails. We don't need to roll-back, the pinctrl core will do this for us.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Tested on a Lager and Salvator-X without problems. Needs probably more testing
on various HW to avoid regressions?

 drivers/pinctrl/sh-pfc/pinctrl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Geert Uytterhoeven June 21, 2016, 1:11 p.m. UTC | #1 
Hi Wolfram,

On Mon, Jun 20, 2016 at 8:40 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On group configuration, bail out if setting one of the individual pins
> fails. We don't need to roll-back, the pinctrl core will do this for us.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Tested on a Lager and Salvator-X without problems. Needs probably more testing
> on various HW to avoid regressions?

Your patch causes the display to fail on r8a7740/armadillo.

Disabling the error propagation and adding more debug prints reveals:

+sh_pfc_pinconf_validate: pin 58: 15 not supported
+sh_pfc_pinconf_set:694: config 0: sh_pfc_pinconf_validate() failed
+sh_pfc_pinconf_group_set:781: sh_pfc_pinconf_set() failed -524 for
pin 0 of group lcd0_data24_0
+sh_pfc_pinconf_validate: pin 57: 15 not supported
+sh_pfc_pinconf_set:694: config 0: sh_pfc_pinconf_validate() failed
+sh_pfc_pinconf_group_set:781: sh_pfc_pinconf_set() failed -524 for
pin 1 of group lcd0_data24_0
+sh_pfc_pinconf_validate: pin 56: 15 not supported
+sh_pfc_pinconf_set:694: config 0: sh_pfc_pinconf_validate() failed
+sh_pfc_pinconf_group_set:781: sh_pfc_pinconf_set() failed -524 for
pin 2 of group lcd0_data24_0
 ...

15 = PIN_CONFIG_OUTPUT

No idea why this is set for all pins of the lcd0_data24_0 group (and not for any
other pin or group on any other SoC).

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang June 21, 2016, 8:37 p.m. UTC | #2 
> > On group configuration, bail out if setting one of the individual pins
> > fails. We don't need to roll-back, the pinctrl core will do this for us.
> >
> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > ---
> >
> > Tested on a Lager and Salvator-X without problems. Needs probably more testing
> > on various HW to avoid regressions?
> 
> Your patch causes the display to fail on r8a7740/armadillo.

I would have been surprised if this patch didn't uncover some hidden
issues.

> 15 = PIN_CONFIG_OUTPUT
> 
> No idea why this is set for all pins of the lcd0_data24_0 group (and not for any
> other pin or group on any other SoC).

Me neither. Sadly no bandwith here to dig into it.
Geert Uytterhoeven May 24, 2017, 11:47 a.m. UTC | #3 
Hi Wolfram,

On Tue, Jun 21, 2016 at 3:11 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Jun 20, 2016 at 8:40 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> On group configuration, bail out if setting one of the individual pins
>> fails. We don't need to roll-back, the pinctrl core will do this for us.
>>
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> ---
>>
>> Tested on a Lager and Salvator-X without problems. Needs probably more testing
>> on various HW to avoid regressions?
>
> Your patch causes the display to fail on r8a7740/armadillo.
>
> Disabling the error propagation and adding more debug prints reveals:
>
> +sh_pfc_pinconf_validate: pin 58: 15 not supported
> +sh_pfc_pinconf_set:694: config 0: sh_pfc_pinconf_validate() failed
> +sh_pfc_pinconf_group_set:781: sh_pfc_pinconf_set() failed -524 for
> pin 0 of group lcd0_data24_0
> +sh_pfc_pinconf_validate: pin 57: 15 not supported
> +sh_pfc_pinconf_set:694: config 0: sh_pfc_pinconf_validate() failed
> +sh_pfc_pinconf_group_set:781: sh_pfc_pinconf_set() failed -524 for
> pin 1 of group lcd0_data24_0
> +sh_pfc_pinconf_validate: pin 56: 15 not supported
> +sh_pfc_pinconf_set:694: config 0: sh_pfc_pinconf_validate() failed
> +sh_pfc_pinconf_group_set:781: sh_pfc_pinconf_set() failed -524 for
> pin 2 of group lcd0_data24_0
>  ...
>
> 15 = PIN_CONFIG_OUTPUT
>
> No idea why this is set for all pins of the lcd0_data24_0 group (and not for any
> other pin or group on any other SoC).

This is caused by the "output-high" property for the GPIO hog.
Due to both lcd0 pinmux and GPIO hog being part of the same device node,
"output-high" is also applied to the lcd0 pins, which fails.

RFC fix in "[PATCH/RFC] ARM: dts: armadillo800eva: Split LCD mux and gpio"
(https://patchwork.kernel.org/patch/9745831/)/

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang May 24, 2017, 12:21 p.m. UTC | #4 
Hi Geert,

> This is caused by the "output-high" property for the GPIO hog.
> Due to both lcd0 pinmux and GPIO hog being part of the same device node,
> "output-high" is also applied to the lcd0 pins, which fails.
> 
> RFC fix in "[PATCH/RFC] ARM: dts: armadillo800eva: Split LCD mux and gpio"
> (https://patchwork.kernel.org/patch/9745831/)/

Cool, thanks! So, if this issue could get resolved, there are chances to
revive the original patch?

Regards,

   Wolfram
Geert Uytterhoeven May 24, 2017, 12:27 p.m. UTC | #5 
Hi Wolfram,

On Wed, May 24, 2017 at 2:21 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>> This is caused by the "output-high" property for the GPIO hog.
>> Due to both lcd0 pinmux and GPIO hog being part of the same device node,
>> "output-high" is also applied to the lcd0 pins, which fails.
>>
>> RFC fix in "[PATCH/RFC] ARM: dts: armadillo800eva: Split LCD mux and gpio"
>> (https://patchwork.kernel.org/patch/9745831/)/
>
> Cool, thanks! So, if this issue could get resolved, there are chances to
> revive the original patch?

Yep, that's the plan.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Geert Uytterhoeven July 17, 2017, 8:51 a.m. UTC | #6 
On Mon, Jun 20, 2016 at 8:40 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> On group configuration, bail out if setting one of the individual pins
> fails. We don't need to roll-back, the pinctrl core will do this for us.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Tested on a Lager and Salvator-X without problems. Needs probably more testing
> on various HW to avoid regressions?

Queueing in sh-pfc-for-v4.14, now commit 13132b3f44d36009 ("ARM: dts:
armadillo800eva: Split LCD mux and gpio") is in v4.13-rc1.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pinctrl/sh-pfc/pinctrl.c b/drivers/pinctrl/sh-pfc/pinctrl.c
index d4e65bc7dacd67..c52ca5873974f3 100644
--- a/drivers/pinctrl/sh-pfc/pinctrl.c
+++ b/drivers/pinctrl/sh-pfc/pinctrl.c
@@ -739,13 +739,16 @@  static int sh_pfc_pinconf_group_set(struct pinctrl_dev *pctldev, unsigned group,
 	struct sh_pfc_pinctrl *pmx = pinctrl_dev_get_drvdata(pctldev);
 	const unsigned int *pins;
 	unsigned int num_pins;
-	unsigned int i;
+	unsigned int i, ret;
 
 	pins = pmx->pfc->info->groups[group].pins;
 	num_pins = pmx->pfc->info->groups[group].nr_pins;
 
-	for (i = 0; i < num_pins; ++i)
-		sh_pfc_pinconf_set(pctldev, pins[i], configs, num_configs);
+	for (i = 0; i < num_pins; ++i) {
+		ret = sh_pfc_pinconf_set(pctldev, pins[i], configs, num_configs);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }