gpio: pxa: make controllers without pinctrl work again
diff mbox series

Message ID 20180518213411.24729-1-daniel@zonque.org
State New
Headers show
Series
  • gpio: pxa: make controllers without pinctrl work again
Related show

Commit Message

Daniel Mack May 18, 2018, 9:34 p.m. UTC
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(-)

Comments

Daniel Mack May 19, 2018, 8:32 p.m. UTC | #1
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
Daniel Mack June 24, 2018, 9:27 p.m. UTC | #2
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
Robert Jarzmik June 26, 2018, 7:22 p.m. UTC | #3
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.
Linus Walleij June 29, 2018, 12:49 p.m. UTC | #4
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
Tony Lindgren July 2, 2018, 7:28 a.m. UTC | #5
* 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
Daniel Mack July 13, 2018, 4:15 p.m. UTC | #6
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

Patch
diff mbox series

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;