| Message ID | 87oa9zr7g3.fsf@belgarion.home |
|---|---|
| State | New |
| Headers | show |
On 03/27/2016 01:29 PM, Robert Jarzmik wrote: > Guenter Roeck <guenter@roeck-us.net> writes: > >> Hi, > Hi Guenter, > >> when trying pxa_defconfig with various pxa270 and pxa255 qemu targets, I >> noticed that the gpio pin direction is no longer set. Bisect points to commit >> a770d946371e ("gpio: pxa: add pin control gpio direction and request"). > This is a bug, you're perfectly right. > >> As it turns out, pxa_defconfig does not configure PINCTRL. As a result, >> pinctrl stub functions are used. > Not having PINCTRL for pxa platforms is also right : only pxa device-tree > platforms will have pinctrl in a first stage, the legacy ones will lives for > several cycles without pinctrl. > >> Those all return 0 if PINCTRL is not configured. This causes the pxa gpio >> driver to wrongly assume that pinctrl configured the gpio pin direction, and >> does nothing. > And as you explain, this is the root case. > >> Looking into gpio-mvebu.c, its use of the pinctrl functions is completely >> different. It aborts on error, not on success, from the pinctrl >> functions. >> Given that, I have no idea how to resolve the problem. Having the stub >> functions return an error might cause the mvebu driver (and maybe others) to >> fail if there is no pinctrl driver, so that does not seem to be an option. > You're right, it's the inverted logic in pxa_gpio_direction_output() which is at > fault, ie. the commit you pointed out. > >> If the idea is to mandate pinctrl for PXA architectures, it should probably be >> enabled for those architectures. Unless I am missing something, PXA >> architectures to not select PINCTRL, which suggests that the problem may >> affect a wide range of systems. Please have a look. > As I said above, selecting PINCTRL is not yet an option. But fixing gpio-pxa is > a necessity for sure. It is beyond me how this can pass through my > non-regression Jenkins ... oh wait, I found : > - pxa machine files still use the old "mfp" stuff > - in this mfp (arch/arm/mach-pxa/mfp-pxa[23]xxx.c", GPDR is directly accessed > > But no more poor excuses, would you try the patch at the end of this mail, while > I'm doing the same on my Jenkins, on both device-tree and legacy platform-data > builds, to see if we can fix this ? Your patch fixes the problem. Feel free to add Tested-by: Guenter Roeck <linux@roeck-us.net> when you submit it for real. Thanks, Guenter -- 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
On 03/27/2016 01:29 PM, Robert Jarzmik wrote: > Guenter Roeck <guenter@roeck-us.net> writes: > >> Hi, > Hi Guenter, > >> when trying pxa_defconfig with various pxa270 and pxa255 qemu targets, I >> noticed that the gpio pin direction is no longer set. Bisect points to commit >> a770d946371e ("gpio: pxa: add pin control gpio direction and request"). > This is a bug, you're perfectly right. > >> As it turns out, pxa_defconfig does not configure PINCTRL. As a result, >> pinctrl stub functions are used. > Not having PINCTRL for pxa platforms is also right : only pxa device-tree > platforms will have pinctrl in a first stage, the legacy ones will lives for > several cycles without pinctrl. > >> Those all return 0 if PINCTRL is not configured. This causes the pxa gpio >> driver to wrongly assume that pinctrl configured the gpio pin direction, and >> does nothing. > And as you explain, this is the root case. > >> Looking into gpio-mvebu.c, its use of the pinctrl functions is completely >> different. It aborts on error, not on success, from the pinctrl >> functions. >> Given that, I have no idea how to resolve the problem. Having the stub >> functions return an error might cause the mvebu driver (and maybe others) to >> fail if there is no pinctrl driver, so that does not seem to be an option. > You're right, it's the inverted logic in pxa_gpio_direction_output() which is at > fault, ie. the commit you pointed out. > >> If the idea is to mandate pinctrl for PXA architectures, it should probably be >> enabled for those architectures. Unless I am missing something, PXA >> architectures to not select PINCTRL, which suggests that the problem may >> affect a wide range of systems. Please have a look. > As I said above, selecting PINCTRL is not yet an option. But fixing gpio-pxa is > a necessity for sure. It is beyond me how this can pass through my > non-regression Jenkins ... oh wait, I found : > - pxa machine files still use the old "mfp" stuff > - in this mfp (arch/arm/mach-pxa/mfp-pxa[23]xxx.c", GPDR is directly accessed > > But no more poor excuses, would you try the patch at the end of this mail, while > I'm doing the same on my Jenkins, on both device-tree and legacy platform-data > builds, to see if we can fix this ? > > Cheers. Any updates ? I still see the problem in mainline. Guenter -- 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
Guenter Roeck <guenter@roeck-us.net> writes: >> But no more poor excuses, would you try the patch at the end of this mail, while >> I'm doing the same on my Jenkins, on both device-tree and legacy platform-data >> builds, to see if we can fix this ? >> >> Cheers. > Any updates ? I still see the problem in mainline. Please be patient, it's scheduled for this kernel version, see here : - Linus fixes tree - and more specifically here : https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/commit/?h=fixes&id=c32f4fd558e1b278addc9514578dd98e20218e00 Cheers.
diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c index b2b7b78664b8..76ac906b4d78 100644 --- a/drivers/gpio/gpio-pxa.c +++ b/drivers/gpio/gpio-pxa.c @@ -283,8 +283,8 @@ static int pxa_gpio_direction_output(struct gpio_chip *chip, writel_relaxed(mask, base + (value ? GPSR_OFFSET : GPCR_OFFSET)); ret = pinctrl_gpio_direction_output(chip->base + offset); - if (!ret) - return 0; + if (ret) + return ret; spin_lock_irqsave(&gpio_lock, flags);