| Message ID | 20180518213411.24729-1-daniel@zonque.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: pxa: make controllers without pinctrl work again | expand |
On Friday, May 18, 2018 11:34 PM, Daniel Mack wrote: > Commit a770d946371ec7 ("gpio: pxa: add pin control gpio direction and > request") changed the driver to always call out to the pinctrl generic > functions when a GPIO is requested or its direction is configured. > > This is fine as long as the platform facilitates a pinctrl driver or > CONFIG_PINCTRL is not set. > > On PXA3xx however, we are now using CONFIG_PINCTRL_SINGLE which is not > linked to the gpio subsystem. Please ignore this patch. As Robert taught me off-list, the only missing thing is a pinctrl reference to the pinctrl-single instance in pxa3xx.dtsi. I'll prepare that as a patch instead. Sorry for the noise. Daniel > > To fix this, introduce a small helper function that tells us whether the > platform may expect a pinctrl driver to be present. > > Signed-off-by: Daniel Mack <daniel@zonque.org> > Fixes: a770d946371ec7 > Cc: Robert Jarzmik <robert.jarzmik@free.fr> > Cc: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/gpio/gpio-pxa.c | 35 +++++++++++++++++++++++++++-------- > 1 file changed, 27 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c > index f480fb896963..e217ebccb0c0 100644 > --- a/drivers/gpio/gpio-pxa.c > +++ b/drivers/gpio/gpio-pxa.c > @@ -241,6 +241,17 @@ int pxa_irq_to_gpio(int irq) > return irq_gpio0; > } > > +static bool pxa_gpio_has_pinctrl(void) > +{ > + switch (gpio_type) { > + case PXA3XX_GPIO: > + return false; > + > + default: > + return true; > + } > +} > + > static int pxa_gpio_to_irq(struct gpio_chip *chip, unsigned offset) > { > struct pxa_gpio_chip *pchip = chip_to_pxachip(chip); > @@ -255,9 +266,11 @@ static int pxa_gpio_direction_input(struct gpio_chip *chip, unsigned offset) > unsigned long flags; > int ret; > > - ret = pinctrl_gpio_direction_input(chip->base + offset); > - if (!ret) > - return 0; > + if (pxa_gpio_has_pinctrl()) { > + ret = pinctrl_gpio_direction_input(chip->base + offset); > + if (!ret) > + return 0; > + } > > spin_lock_irqsave(&gpio_lock, flags); > > @@ -282,9 +295,11 @@ 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 ret; > + if (pxa_gpio_has_pinctrl()) { > + ret = pinctrl_gpio_direction_output(chip->base + offset); > + if (ret) > + return ret; > + } > > spin_lock_irqsave(&gpio_lock, flags); > > @@ -348,8 +363,12 @@ static int pxa_init_gpio_chip(struct pxa_gpio_chip *pchip, int ngpio, > pchip->chip.set = pxa_gpio_set; > pchip->chip.to_irq = pxa_gpio_to_irq; > pchip->chip.ngpio = ngpio; > - pchip->chip.request = gpiochip_generic_request; > - pchip->chip.free = gpiochip_generic_free; > + > + if (pxa_gpio_has_pinctrl()) { > + pchip->chip.request = gpiochip_generic_request; > + pchip->chip.free = gpiochip_generic_free; > + } > + > #ifdef CONFIG_OF_GPIO > pchip->chip.of_node = np; > pchip->chip.of_xlate = pxa_gpio_of_xlate; > -- 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
Hi,
I have to get back to this one. The fall-out that made me implement the
patch below in the first place was fixed with the following patch:
e41f76830d (ARM: pxa: dts: add gpio-ranges to gpio controller)
... but now another issue came up.
To recap, the pxa3xx driver uses the pinctrl-single driver since a while
which does not implement a .gpio_set_direction() callback. The pinmux
core will simply return 0 in this case, and the pxa3xx gpio driver hence
thinks the pinctrl driver did its job and returns as well.
This effectively makes pxa_gpio_direction_{input,output} no-ops.
Interestingly, I only noticed that when I tried to use the w1-gpio
driver which makes use of gpiolib's open-drain emulation. All other
use-cases work just fine.
So the question is how this is supposed to work. I see two options:
1) Apply the patch below, so we don't use use the pinctrl driver on PXA3xx.
2) Change pinmux_gpio_direction() and make it return -ENOSYS or
something in case there is no .gpio_set_direction() implementation. I'm
not sure whether that's a good idea though, and what the side-effects
would be.
Any other ideas?
Thanks,
Daniel
On Friday, May 18, 2018 11:34 PM, Daniel Mack wrote:
> Commit a770d946371ec7 ("gpio: pxa: add pin control gpio direction and
> request") changed the driver to always call out to the pinctrl generic
> functions when a GPIO is requested or its direction is configured.
>
> This is fine as long as the platform facilitates a pinctrl driver or
> CONFIG_PINCTRL is not set.
>
> On PXA3xx however, we are now using CONFIG_PINCTRL_SINGLE which is not
> linked to the gpio subsystem.
>
> To fix this, introduce a small helper function that tells us whether the
> platform may expect a pinctrl driver to be present.
>
> Signed-off-by: Daniel Mack <daniel@zonque.org>
> Fixes: a770d946371ec7
> Cc: Robert Jarzmik <robert.jarzmik@free.fr>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> ---
> drivers/gpio/gpio-pxa.c | 35 +++++++++++++++++++++++++++--------
> 1 file changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c
> index f480fb896963..e217ebccb0c0 100644
> --- a/drivers/gpio/gpio-pxa.c
> +++ b/drivers/gpio/gpio-pxa.c
> @@ -241,6 +241,17 @@ int pxa_irq_to_gpio(int irq)
> return irq_gpio0;
> }
>
> +static bool pxa_gpio_has_pinctrl(void)
> +{
> + switch (gpio_type) {
> + case PXA3XX_GPIO:
> + return false;
> +
> + default:
> + return true;
> + }
> +}
> +
> static int pxa_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
> {
> struct pxa_gpio_chip *pchip = chip_to_pxachip(chip);
> @@ -255,9 +266,11 @@ static int pxa_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> unsigned long flags;
> int ret;
>
> - ret = pinctrl_gpio_direction_input(chip->base + offset);
> - if (!ret)
> - return 0;
> + if (pxa_gpio_has_pinctrl()) {
> + ret = pinctrl_gpio_direction_input(chip->base + offset);
> + if (!ret)
> + return 0;
> + }
>
> spin_lock_irqsave(&gpio_lock, flags);
>
> @@ -282,9 +295,11 @@ 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 ret;
> + if (pxa_gpio_has_pinctrl()) {
> + ret = pinctrl_gpio_direction_output(chip->base + offset);
> + if (ret)
> + return ret;
> + }
>
> spin_lock_irqsave(&gpio_lock, flags);
>
> @@ -348,8 +363,12 @@ static int pxa_init_gpio_chip(struct pxa_gpio_chip *pchip, int ngpio,
> pchip->chip.set = pxa_gpio_set;
> pchip->chip.to_irq = pxa_gpio_to_irq;
> pchip->chip.ngpio = ngpio;
> - pchip->chip.request = gpiochip_generic_request;
> - pchip->chip.free = gpiochip_generic_free;
> +
> + if (pxa_gpio_has_pinctrl()) {
> + pchip->chip.request = gpiochip_generic_request;
> + pchip->chip.free = gpiochip_generic_free;
> + }
> +
> #ifdef CONFIG_OF_GPIO
> pchip->chip.of_node = np;
> pchip->chip.of_xlate = pxa_gpio_of_xlate;
>
--
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
Daniel Mack <daniel@zonque.org> writes: > Hi, > > I have to get back to this one. The fall-out that made me implement the patch > below in the first place was fixed with the following patch: > > e41f76830d (ARM: pxa: dts: add gpio-ranges to gpio controller) > > ... but now another issue came up. > > To recap, the pxa3xx driver uses the pinctrl-single driver since a while which > does not implement a .gpio_set_direction() callback. The pinmux core will simply > return 0 in this case, and the pxa3xx gpio driver hence thinks the pinctrl > driver did its job and returns as well. > > This effectively makes pxa_gpio_direction_{input,output} no-ops. Interestingly, > I only noticed that when I tried to use the w1-gpio driver which makes use of > gpiolib's open-drain emulation. All other use-cases work just fine. > > So the question is how this is supposed to work. I see two options: > > 1) Apply the patch below, so we don't use use the pinctrl driver on PXA3xx. > > 2) Change pinmux_gpio_direction() and make it return -ENOSYS or something in > case there is no .gpio_set_direction() implementation. I'm not sure whether > that's a good idea though, and what the side-effects would be. Linus, this question is rather for you : is it normal that whether an pincontroller is available or not, pinctrl_gpio_direction_input() returns 0 (as stated in the function pinmux_gpio_direction()). If that is the case, how a gpio driver can know if the gpio direction will be handled in the pin controller and not take action itself ? As for Daniel, there is another solution, ie remove this from pxa_gpio_direction_input() and pxa_gpio_direction_output(): if (!ret) return 0; Cheers.
On Sun, Jun 24, 2018 at 11:27 PM Daniel Mack <daniel@zonque.org> wrote: > To recap, the pxa3xx driver uses the pinctrl-single driver since a while > which does not implement a .gpio_set_direction() callback. The pinmux > core will simply return 0 in this case, and the pxa3xx gpio driver hence > thinks the pinctrl driver did its job and returns as well. I guess we need to discuss with Tony and Haojian if pinctrl-single should ever implement that or if it is thought as having GPIO and pin control orthogonally. > This effectively makes pxa_gpio_direction_{input,output} no-ops. > Interestingly, I only noticed that when I tried to use the w1-gpio > driver which makes use of gpiolib's open-drain emulation. All other > use-cases work just fine. Hm. Is that because muxing is not getting set up or because of some other electrical configuration not being applied? > So the question is how this is supposed to work. I see two options: > > 1) Apply the patch below, so we don't use use the pinctrl driver on PXA3xx. The patch looks good on its own terms. Normally drivers look to see if the GPIO controller has any gpio-ranges, and if it does, it assumes pin control is in use. > 2) Change pinmux_gpio_direction() and make it return -ENOSYS or > something in case there is no .gpio_set_direction() implementation. I'm > not sure whether that's a good idea though, and what the side-effects > would be. It sounds dangerous. You would have to grep and read through the consumers to present an argument that it is safe. Yours, Linus Walleij -- 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
* Linus Walleij <linus.walleij@linaro.org> [180629 12:52]: > On Sun, Jun 24, 2018 at 11:27 PM Daniel Mack <daniel@zonque.org> wrote: > > > To recap, the pxa3xx driver uses the pinctrl-single driver since a while > > which does not implement a .gpio_set_direction() callback. The pinmux > > core will simply return 0 in this case, and the pxa3xx gpio driver hence > > thinks the pinctrl driver did its job and returns as well. > > I guess we need to discuss with Tony and Haojian if pinctrl-single should > ever implement that or if it is thought as having GPIO and pin > control orthogonally. Sure why not as long as pinctrl-single can decipher the gpio number and then call the GPIO framework. Regards, Tony -- 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 Monday, July 02, 2018 09:28 AM, Tony Lindgren wrote: > * Linus Walleij <linus.walleij@linaro.org> [180629 12:52]: >> On Sun, Jun 24, 2018 at 11:27 PM Daniel Mack <daniel@zonque.org> wrote: >> >>> To recap, the pxa3xx driver uses the pinctrl-single driver since a while >>> which does not implement a .gpio_set_direction() callback. The pinmux >>> core will simply return 0 in this case, and the pxa3xx gpio driver hence >>> thinks the pinctrl driver did its job and returns as well. >> >> I guess we need to discuss with Tony and Haojian if pinctrl-single should >> ever implement that or if it is thought as having GPIO and pin >> control orthogonally. > > Sure why not as long as pinctrl-single can decipher the gpio > number and then call the GPIO framework. Hmm, but the call is coming from the GPIO framework already. I'm not quite following. Anyway. it seems that teaching the pinctrl-single driver needs to learn setting directions at all, so for now, I'd like to get the patch that works around that merged for 4.19. I'll reword the commit log a bit and resend. Thanks, Daniel -- 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 --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c index f480fb896963..e217ebccb0c0 100644 --- a/drivers/gpio/gpio-pxa.c +++ b/drivers/gpio/gpio-pxa.c @@ -241,6 +241,17 @@ int pxa_irq_to_gpio(int irq) return irq_gpio0; } +static bool pxa_gpio_has_pinctrl(void) +{ + switch (gpio_type) { + case PXA3XX_GPIO: + return false; + + default: + return true; + } +} + static int pxa_gpio_to_irq(struct gpio_chip *chip, unsigned offset) { struct pxa_gpio_chip *pchip = chip_to_pxachip(chip); @@ -255,9 +266,11 @@ static int pxa_gpio_direction_input(struct gpio_chip *chip, unsigned offset) unsigned long flags; int ret; - ret = pinctrl_gpio_direction_input(chip->base + offset); - if (!ret) - return 0; + if (pxa_gpio_has_pinctrl()) { + ret = pinctrl_gpio_direction_input(chip->base + offset); + if (!ret) + return 0; + } spin_lock_irqsave(&gpio_lock, flags); @@ -282,9 +295,11 @@ 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 ret; + if (pxa_gpio_has_pinctrl()) { + ret = pinctrl_gpio_direction_output(chip->base + offset); + if (ret) + return ret; + } spin_lock_irqsave(&gpio_lock, flags); @@ -348,8 +363,12 @@ static int pxa_init_gpio_chip(struct pxa_gpio_chip *pchip, int ngpio, pchip->chip.set = pxa_gpio_set; pchip->chip.to_irq = pxa_gpio_to_irq; pchip->chip.ngpio = ngpio; - pchip->chip.request = gpiochip_generic_request; - pchip->chip.free = gpiochip_generic_free; + + if (pxa_gpio_has_pinctrl()) { + pchip->chip.request = gpiochip_generic_request; + pchip->chip.free = gpiochip_generic_free; + } + #ifdef CONFIG_OF_GPIO pchip->chip.of_node = np; pchip->chip.of_xlate = pxa_gpio_of_xlate;
Commit a770d946371ec7 ("gpio: pxa: add pin control gpio direction and request") changed the driver to always call out to the pinctrl generic functions when a GPIO is requested or its direction is configured. This is fine as long as the platform facilitates a pinctrl driver or CONFIG_PINCTRL is not set. On PXA3xx however, we are now using CONFIG_PINCTRL_SINGLE which is not linked to the gpio subsystem. To fix this, introduce a small helper function that tells us whether the platform may expect a pinctrl driver to be present. Signed-off-by: Daniel Mack <daniel@zonque.org> Fixes: a770d946371ec7 Cc: Robert Jarzmik <robert.jarzmik@free.fr> Cc: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpio/gpio-pxa.c | 35 +++++++++++++++++++++++++++-------- 1 file changed, 27 insertions(+), 8 deletions(-)