[PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Haojian Zhuang]

gpio-ranges property could binds gpio to pinctrl. But there may be some
gpios without pinctrl operation. So check whether gpio-ranges property
exists in device node first.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 drivers/gpio/gpio-pl061.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index cb5510b..44a0ba1 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -306,8 +306,11 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 
 	spin_lock_init(&chip->lock);
 
-	chip->gc.request = pl061_gpio_request;
-	chip->gc.free = pl061_gpio_free;
+	/* Hook the request()/free() for pinctrl operation */
+	if (of_get_property(dev->of_node, "gpio-ranges", NULL)) {
+		chip->gc.request = pl061_gpio_request;
+		chip->gc.free = pl061_gpio_free;
+	}
 	chip->gc.direction_input = pl061_direction_input;
 	chip->gc.direction_output = pl061_direction_output;
 	chip->gc.get = pl061_get_value;
-- 
1.8.1.2

Re: [PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Linus Walleij]

On Wed, Oct 23, 2013 at 8:36 AM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:

> gpio-ranges property could binds gpio to pinctrl. But there may be some
> gpios without pinctrl operation. So check whether gpio-ranges property
> exists in device node first.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

OK...

> +       /* Hook the request()/free() for pinctrl operation */
> +       if (of_get_property(dev->of_node, "gpio-ranges", NULL)) {
> +               chip->gc.request = pl061_gpio_request;
> +               chip->gc.free = pl061_gpio_free;
> +       }

This is quite hard for those not using pinctrl to understand, maybe someone
wants to use the request/free callbacks for something else in the
future.

Can't you:

- Add a variable bool uses_pinctrl to struct pl061_gpio

- Use the bool check:

if (of_property_read_bool(np, "gpio-ranges"))
  chip->uses_pinctrl = true;

- Alter the code in pl061_gpio_request() to do like this:

if (chip->uses_pinctrl)
   pinctrl_request_gpio()

And the same for pl061_gpio_free().

Yours,
Linus Walleij

[PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Haojian Zhuang]

gpio-ranges property could binds gpio to pinctrl. But there may be some
gpios without pinctrl operation. So check whether gpio-ranges property
exists in device node first.

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 drivers/gpio/gpio-pl061.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 84b49cf..d1813f0 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -52,28 +52,34 @@ struct pl061_gpio {
 
 	void __iomem		*base;
 	struct gpio_chip	gc;
+	int			uses_pinctrl;
 
 #ifdef CONFIG_PM
 	struct pl061_context_save_regs csave_regs;
 #endif
 };
 
-static int pl061_gpio_request(struct gpio_chip *chip, unsigned offset)
+static int pl061_gpio_request(struct gpio_chip *gc, unsigned offset)
 {
 	/*
 	 * Map back to global GPIO space and request muxing, the direction
 	 * parameter does not matter for this controller.
 	 */
-	int gpio = chip->base + offset;
+	struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
+	int gpio = gc->base + offset;
 
-	return pinctrl_request_gpio(gpio);
+	if (chip->uses_pinctrl)
+		return pinctrl_request_gpio(gpio);
+	return 0;
 }
 
-static void pl061_gpio_free(struct gpio_chip *chip, unsigned offset)
+static void pl061_gpio_free(struct gpio_chip *gc, unsigned offset)
 {
-	int gpio = chip->base + offset;
+	struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
+	int gpio = gc->base + offset;
 
-	pinctrl_free_gpio(gpio);
+	if (chip->uses_pinctrl)
+		pinctrl_free_gpio(gpio);
 }
 
 static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
@@ -264,6 +270,9 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 
 	spin_lock_init(&chip->lock);
 
+	/* Hook the request()/free() for pinctrl operation */
+	if (of_property_read_bool(dev->of_node, "gpio-ranges"))
+		chip->uses_pinctrl = true;
 	chip->gc.request = pl061_gpio_request;
 	chip->gc.free = pl061_gpio_free;
 	chip->gc.direction_input = pl061_direction_input;
-- 
1.9.1

Re: [PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Haojian Zhuang]

Hi Xinwei,

Do I miss anything? At here, .request pointer isn't null. It always
points to pl061_gpio_request().

Best Regards
Haojian

On 9 October 2014 18:38, k00278426 <kong.kongxinwei@hisilicon.com> wrote:
> On 2014/10/9 17:42, Haojian Zhuang wrote:
>
> gpio-ranges property could binds gpio to pinctrl. But there may be some
> gpios without pinctrl operation. So check whether gpio-ranges property
> exists in device node first.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  drivers/gpio/gpio-pl061.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 84b49cf..d1813f0 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
> @@ -52,28 +52,34 @@ struct pl061_gpio {
>
>   void __iomem *base;
>   struct gpio_chip gc;
> + int uses_pinctrl;
>
>  #ifdef CONFIG_PM
>   struct pl061_context_save_regs csave_regs;
>  #endif
>  };
>
> -static int pl061_gpio_request(struct gpio_chip *chip, unsigned offset)
> +static int pl061_gpio_request(struct gpio_chip *gc, unsigned offset)
>  {
>   /*
>   * Map back to global GPIO space and request muxing, the direction
>   * parameter does not matter for this controller.
>   */
> - int gpio = chip->base + offset;
> + struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
> + int gpio = gc->base + offset;
>
> - return pinctrl_request_gpio(gpio);
> + if (chip->uses_pinctrl)
> + return pinctrl_request_gpio(gpio);
> + return 0;
>  }
>
>        In the request_gpio process,if the .request point is null,the  return
> of request_gpio process will value 0. the request_gpio process will not
> enter the pintrcl system.
>   so the request_gpio have deal with the "if" branch.
>
>
> -static void pl061_gpio_free(struct gpio_chip *chip, unsigned offset)
> +static void pl061_gpio_free(struct gpio_chip *gc, unsigned offset)
>  {
> - int gpio = chip->base + offset;
> + struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
> + int gpio = gc->base + offset;
>
> - pinctrl_free_gpio(gpio);
> + if (chip->uses_pinctrl)
> + pinctrl_free_gpio(gpio);
>  }
>
>  static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
> @@ -264,6 +270,9 @@ static int pl061_probe(struct amba_device *adev, const
> struct amba_id *id)
>
>   spin_lock_init(&chip->lock);
>
> + /* Hook the request()/free() for pinctrl operation */
> + if (of_property_read_bool(dev->of_node, "gpio-ranges"))
> + chip->uses_pinctrl = true;
>   chip->gc.request = pl061_gpio_request;
>   chip->gc.free = pl061_gpio_free;
>   chip->gc.direction_input = pl061_direction_input;
>
>

Re: [PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[k00278426]

hi jian
  
     In the afternoon,we submit a patch about gpio-ranges item,we slove this issue by setting the .request pointer which is null. before coming true the

pl061_gpio_request function, we slove the issue.you can study it.

Best Regards
xinwei 


On 2014/10/9 18:56, Haojian Zhuang wrote:
> Hi Xinwei,
>
> Do I miss anything? At here, .request pointer isn't null. It always
> points to pl061_gpio_request().
>
> Best Regards
> Haojian
>
> On 9 October 2014 18:38, k00278426 <kong.kongxinwei@hisilicon.com> wrote:
>> On 2014/10/9 17:42, Haojian Zhuang wrote:
>>
>> gpio-ranges property could binds gpio to pinctrl. But there may be some
>> gpios without pinctrl operation. So check whether gpio-ranges property
>> exists in device node first.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>> ---
>>  drivers/gpio/gpio-pl061.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
>> index 84b49cf..d1813f0 100644
>> --- a/drivers/gpio/gpio-pl061.c
>> +++ b/drivers/gpio/gpio-pl061.c
>> @@ -52,28 +52,34 @@ struct pl061_gpio {
>>
>>   void __iomem *base;
>>   struct gpio_chip gc;
>> + int uses_pinctrl;
>>
>>  #ifdef CONFIG_PM
>>   struct pl061_context_save_regs csave_regs;
>>  #endif
>>  };
>>
>> -static int pl061_gpio_request(struct gpio_chip *chip, unsigned offset)
>> +static int pl061_gpio_request(struct gpio_chip *gc, unsigned offset)
>>  {
>>   /*
>>   * Map back to global GPIO space and request muxing, the direction
>>   * parameter does not matter for this controller.
>>   */
>> - int gpio = chip->base + offset;
>> + struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
>> + int gpio = gc->base + offset;
>>
>> - return pinctrl_request_gpio(gpio);
>> + if (chip->uses_pinctrl)
>> + return pinctrl_request_gpio(gpio);
>> + return 0;
>>  }
>>
>>        In the request_gpio process,if the .request point is null,the  return
>> of request_gpio process will value 0. the request_gpio process will not
>> enter the pintrcl system.
>>   so the request_gpio have deal with the "if" branch.
>>
>>
>> -static void pl061_gpio_free(struct gpio_chip *chip, unsigned offset)
>> +static void pl061_gpio_free(struct gpio_chip *gc, unsigned offset)
>>  {
>> - int gpio = chip->base + offset;
>> + struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
>> + int gpio = gc->base + offset;
>>
>> - pinctrl_free_gpio(gpio);
>> + if (chip->uses_pinctrl)
>> + pinctrl_free_gpio(gpio);
>>  }
>>
>>  static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
>> @@ -264,6 +270,9 @@ static int pl061_probe(struct amba_device *adev, const
>> struct amba_id *id)
>>
>>   spin_lock_init(&chip->lock);
>>
>> + /* Hook the request()/free() for pinctrl operation */
>> + if (of_property_read_bool(dev->of_node, "gpio-ranges"))
>> + chip->uses_pinctrl = true;
>>   chip->gc.request = pl061_gpio_request;
>>   chip->gc.free = pl061_gpio_free;
>>   chip->gc.direction_input = pl061_direction_input;
>>
>>
> .
>

Re: [PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Haojian Zhuang]

It's good. But it's similar the one that I sent in last year. You can
check this link in below.

http://marc.info/?l=linux-gpio&m=138300092720593&w=2

My patch is used to fix this issue according to the comments.

Best Regards
Haojian

On 9 October 2014 19:15, k00278426 <kong.kongxinwei@hisilicon.com> wrote:
> hi jian
>
>      In the afternoon,we submit a patch about gpio-ranges item,we slove this issue by setting the .request pointer which is null. before coming true the
>
> pl061_gpio_request function, we slove the issue.you can study it.
>
> Best Regards
> xinwei
>
>
> On 2014/10/9 18:56, Haojian Zhuang wrote:
>> Hi Xinwei,
>>
>> Do I miss anything? At here, .request pointer isn't null. It always
>> points to pl061_gpio_request().
>>
>> Best Regards
>> Haojian
>>
>> On 9 October 2014 18:38, k00278426 <kong.kongxinwei@hisilicon.com> wrote:
>>> On 2014/10/9 17:42, Haojian Zhuang wrote:
>>>
>>> gpio-ranges property could binds gpio to pinctrl. But there may be some
>>> gpios without pinctrl operation. So check whether gpio-ranges property
>>> exists in device node first.
>>>
>>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>>> ---
>>>  drivers/gpio/gpio-pl061.c | 21 +++++++++++++++------
>>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
>>> index 84b49cf..d1813f0 100644
>>> --- a/drivers/gpio/gpio-pl061.c
>>> +++ b/drivers/gpio/gpio-pl061.c
>>> @@ -52,28 +52,34 @@ struct pl061_gpio {
>>>
>>>   void __iomem *base;
>>>   struct gpio_chip gc;
>>> + int uses_pinctrl;
>>>
>>>  #ifdef CONFIG_PM
>>>   struct pl061_context_save_regs csave_regs;
>>>  #endif
>>>  };
>>>
>>> -static int pl061_gpio_request(struct gpio_chip *chip, unsigned offset)
>>> +static int pl061_gpio_request(struct gpio_chip *gc, unsigned offset)
>>>  {
>>>   /*
>>>   * Map back to global GPIO space and request muxing, the direction
>>>   * parameter does not matter for this controller.
>>>   */
>>> - int gpio = chip->base + offset;
>>> + struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
>>> + int gpio = gc->base + offset;
>>>
>>> - return pinctrl_request_gpio(gpio);
>>> + if (chip->uses_pinctrl)
>>> + return pinctrl_request_gpio(gpio);
>>> + return 0;
>>>  }
>>>
>>>        In the request_gpio process,if the .request point is null,the  return
>>> of request_gpio process will value 0. the request_gpio process will not
>>> enter the pintrcl system.
>>>   so the request_gpio have deal with the "if" branch.
>>>
>>>
>>> -static void pl061_gpio_free(struct gpio_chip *chip, unsigned offset)
>>> +static void pl061_gpio_free(struct gpio_chip *gc, unsigned offset)
>>>  {
>>> - int gpio = chip->base + offset;
>>> + struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
>>> + int gpio = gc->base + offset;
>>>
>>> - pinctrl_free_gpio(gpio);
>>> + if (chip->uses_pinctrl)
>>> + pinctrl_free_gpio(gpio);
>>>  }
>>>
>>>  static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
>>> @@ -264,6 +270,9 @@ static int pl061_probe(struct amba_device *adev, const
>>> struct amba_id *id)
>>>
>>>   spin_lock_init(&chip->lock);
>>>
>>> + /* Hook the request()/free() for pinctrl operation */
>>> + if (of_property_read_bool(dev->of_node, "gpio-ranges"))
>>> + chip->uses_pinctrl = true;
>>>   chip->gc.request = pl061_gpio_request;
>>>   chip->gc.free = pl061_gpio_free;
>>>   chip->gc.direction_input = pl061_direction_input;
>>>
>>>
>> .
>>
>
>

Re: [PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Alexandre Courbot]

On Thu, Oct 9, 2014 at 6:42 PM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:
> gpio-ranges property could binds gpio to pinctrl. But there may be some
> gpios without pinctrl operation. So check whether gpio-ranges property
> exists in device node first.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  drivers/gpio/gpio-pl061.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 84b49cf..d1813f0 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
> @@ -52,28 +52,34 @@ struct pl061_gpio {
>
>         void __iomem            *base;
>         struct gpio_chip        gc;
> +       int                     uses_pinctrl;
>
>  #ifdef CONFIG_PM
>         struct pl061_context_save_regs csave_regs;
>  #endif
>  };
>
> -static int pl061_gpio_request(struct gpio_chip *chip, unsigned offset)
> +static int pl061_gpio_request(struct gpio_chip *gc, unsigned offset)
>  {
>         /*
>          * Map back to global GPIO space and request muxing, the direction
>          * parameter does not matter for this controller.
>          */
> -       int gpio = chip->base + offset;
> +       struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
> +       int gpio = gc->base + offset;
>
> -       return pinctrl_request_gpio(gpio);
> +       if (chip->uses_pinctrl)
> +               return pinctrl_request_gpio(gpio);
> +       return 0;
>  }
>
> -static void pl061_gpio_free(struct gpio_chip *chip, unsigned offset)
> +static void pl061_gpio_free(struct gpio_chip *gc, unsigned offset)
>  {
> -       int gpio = chip->base + offset;
> +       struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
> +       int gpio = gc->base + offset;
>
> -       pinctrl_free_gpio(gpio);
> +       if (chip->uses_pinctrl)
> +               pinctrl_free_gpio(gpio);
>  }
>
>  static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
> @@ -264,6 +270,9 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>
>         spin_lock_init(&chip->lock);
>
> +       /* Hook the request()/free() for pinctrl operation */
> +       if (of_property_read_bool(dev->of_node, "gpio-ranges"))
> +               chip->uses_pinctrl = true;

You probably want to document this property in
Documentation/devicetree/bindings/gpio/pl061-gpio.txt.

[PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Yunlei He]

Gpio-ranges property is useful to represent which GPIOs correspond
to which pins on which pin controllers. But there may be some gpios
without pinctrl operation. So check whether gpio-ranges property
exists in device node first.

Signed-off-by: Yunlei He <heyunlei@huawei.com>
Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 Documentation/devicetree/bindings/gpio/pl061-gpio.txt | 2 +-
 drivers/gpio/gpio-pl061.c                             | 7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
index a2c416b..577bcf7 100644
--- a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
@@ -7,4 +7,4 @@ Required properties:
   - bit 0 specifies polarity (0 for normal, 1 for inverted)
 - gpio-controller : Marks the device node as a GPIO controller.
 - interrupts : Interrupt mapping for GPIO IRQ.
-
+- gpio-ranges : Interaction with the PINCTRL subsystem
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 84b49cf..01875d1 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/pm.h>
+#include <linux/of_address.h>
 
 #define GPIODIR 0x400
 #define GPIOIS  0x404
@@ -263,9 +264,11 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 		return PTR_ERR(chip->base);
 
 	spin_lock_init(&chip->lock);
+	if (of_get_property(dev->of_node, "gpio-ranges", NULL)) {
+		chip->gc.request = pl061_gpio_request;
+		chip->gc.free = pl061_gpio_free;
+	}
 
-	chip->gc.request = pl061_gpio_request;
-	chip->gc.free = pl061_gpio_free;
 	chip->gc.direction_input = pl061_direction_input;
 	chip->gc.direction_output = pl061_direction_output;
 	chip->gc.get = pl061_get_value;
-- 
1.9.1

Re: [PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Alexandre Courbot]

On Fri, Nov 21, 2014 at 3:43 PM, Yunlei He <heyunlei@huawei.com> wrote:
> Gpio-ranges property is useful to represent which GPIOs correspond
> to which pins on which pin controllers. But there may be some gpios
> without pinctrl operation. So check whether gpio-ranges property
> exists in device node first.
>
> Signed-off-by: Yunlei He <heyunlei@huawei.com>
> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
> ---
>  Documentation/devicetree/bindings/gpio/pl061-gpio.txt | 2 +-
>  drivers/gpio/gpio-pl061.c                             | 7 +++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
> index a2c416b..577bcf7 100644
> --- a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
> @@ -7,4 +7,4 @@ Required properties:
>    - bit 0 specifies polarity (0 for normal, 1 for inverted)
>  - gpio-controller : Marks the device node as a GPIO controller.
>  - interrupts : Interrupt mapping for GPIO IRQ.
> -
> +- gpio-ranges : Interaction with the PINCTRL subsystem
> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 84b49cf..01875d1 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
> @@ -24,6 +24,7 @@
>  #include <linux/slab.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/pm.h>
> +#include <linux/of_address.h>
>
>  #define GPIODIR 0x400
>  #define GPIOIS  0x404
> @@ -263,9 +264,11 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
>                 return PTR_ERR(chip->base);
>
>         spin_lock_init(&chip->lock);
> +       if (of_get_property(dev->of_node, "gpio-ranges", NULL)) {
> +               chip->gc.request = pl061_gpio_request;
> +               chip->gc.free = pl061_gpio_free;
> +       }

With this property required to set gc.request and gc.free, aren't we
going to break old DTs that do know of this property but still expect
pl061_gpio_request() and pl061_gpio_free() to be called?

It seems that to preserve backward-compatibility, your property should
be used to negate the legacy behavior, not enable it.

Re: [PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Linus Walleij]

On Fri, Nov 21, 2014 at 7:43 AM, Yunlei He <heyunlei@huawei.com> wrote:

> Gpio-ranges property is useful to represent which GPIOs correspond
> to which pins on which pin controllers. But there may be some gpios
> without pinctrl operation. So check whether gpio-ranges property
> exists in device node first.
>
> Signed-off-by: Yunlei He <heyunlei@huawei.com>
> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

(...)

>  Documentation/devicetree/bindings/gpio/pl061-gpio.txt | 2 +-
>  drivers/gpio/gpio-pl061.c                             | 7 +++++--
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
> index a2c416b..577bcf7 100644
> --- a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
> @@ -7,4 +7,4 @@ Required properties:
>    - bit 0 specifies polarity (0 for normal, 1 for inverted)
>  - gpio-controller : Marks the device node as a GPIO controller.
>  - interrupts : Interrupt mapping for GPIO IRQ.
> -
> +- gpio-ranges : Interaction with the PINCTRL subsystem

This can be a separate patch. Please send it as such.

> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index 84b49cf..01875d1 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
> @@ -24,6 +24,7 @@
>  #include <linux/slab.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/pm.h>
> +#include <linux/of_address.h>

What you're checking for is not an address. This would be just
#include <linux/of.h>

>         spin_lock_init(&chip->lock);
> +       if (of_get_property(dev->of_node, "gpio-ranges", NULL)) {
> +               chip->gc.request = pl061_gpio_request;
> +               chip->gc.free = pl061_gpio_free;
> +       }
>
> -       chip->gc.request = pl061_gpio_request;
> -       chip->gc.free = pl061_gpio_free;

NAK.

No this does not work. GPIO ranges doe not *have* to come from
the device tree, it is more common that a GPIO driver adds it by
way of gpiochip_add_pin_range().

Haojian has already solved this problem in the pinctrl core.
Inspect commit 51e13c2475913d45a3ec546dee647538a9341d6a
"pinctrl: check pinctrl ready for gpio range"

The call(s) to pinctrl_request_gpio() from
pl061_gpio_request() should already return silently with 0
AFAICT, Haojian do you agree?

Yours,
Linus Walleij

Re: [PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Haojian Zhuang]

On 28 November 2014 at 20:26, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Nov 21, 2014 at 7:43 AM, Yunlei He <heyunlei@huawei.com> wrote:
>
>> Gpio-ranges property is useful to represent which GPIOs correspond
>> to which pins on which pin controllers. But there may be some gpios
>> without pinctrl operation. So check whether gpio-ranges property
>> exists in device node first.
>>
>> Signed-off-by: Yunlei He <heyunlei@huawei.com>
>> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>
> (...)
>
>>  Documentation/devicetree/bindings/gpio/pl061-gpio.txt | 2 +-
>>  drivers/gpio/gpio-pl061.c                             | 7 +++++--
>>  2 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
>> index a2c416b..577bcf7 100644
>> --- a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
>> +++ b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
>> @@ -7,4 +7,4 @@ Required properties:
>>    - bit 0 specifies polarity (0 for normal, 1 for inverted)
>>  - gpio-controller : Marks the device node as a GPIO controller.
>>  - interrupts : Interrupt mapping for GPIO IRQ.
>> -
>> +- gpio-ranges : Interaction with the PINCTRL subsystem
>
> This can be a separate patch. Please send it as such.
>
>> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
>> index 84b49cf..01875d1 100644
>> --- a/drivers/gpio/gpio-pl061.c
>> +++ b/drivers/gpio/gpio-pl061.c
>> @@ -24,6 +24,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/pinctrl/consumer.h>
>>  #include <linux/pm.h>
>> +#include <linux/of_address.h>
>
> What you're checking for is not an address. This would be just
> #include <linux/of.h>
>
>>         spin_lock_init(&chip->lock);
>> +       if (of_get_property(dev->of_node, "gpio-ranges", NULL)) {
>> +               chip->gc.request = pl061_gpio_request;
>> +               chip->gc.free = pl061_gpio_free;
>> +       }
>>
>> -       chip->gc.request = pl061_gpio_request;
>> -       chip->gc.free = pl061_gpio_free;
>
> NAK.
>
> No this does not work. GPIO ranges doe not *have* to come from
> the device tree, it is more common that a GPIO driver adds it by
> way of gpiochip_add_pin_range().
>
> Haojian has already solved this problem in the pinctrl core.
> Inspect commit 51e13c2475913d45a3ec546dee647538a9341d6a
> "pinctrl: check pinctrl ready for gpio range"
>
> The call(s) to pinctrl_request_gpio() from
> pl061_gpio_request() should already return silently with 0
> AFAICT, Haojian do you agree?
>

It's a bit different.

The commit 51e13c2475 is used to fix this scenario. There're 8 gpio
 pins in GPIO CHIP #19. There're pin muxing on gpio152 -
 gpio155. And there're _no_ pin muxing on gpio156 - gpio159. When
user tries to request gpio159, he'll meet failure since the pinctrl device
can't cover gpio159. But the pinctrl device is already register for
gpio152. In order to distinguish whether the pinctrl device registered,
I added pinctrl_ready_for_gpio_range(gpio).

In another scenario, there's no back-end pinctrl device for GPIO
CHIP #0. So pinctrl_request_gpio() always returns EPROBE_DEFER.
The commit 51e13c2475 can't cover this.

I suggest to write code in below.

-static void pl061_gpio_free(struct gpio_chip *chip, unsigned offset)
+static void pl061_gpio_free(struct gpio_chip *gc, unsigned offset)
 {
-       int gpio = chip->base + offset;
+       struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
+       int gpio = gc->base + offset;

-       pinctrl_free_gpio(gpio);
+       if (chip->uses_pinctrl)
+               pinctrl_free_gpio(gpio);
 }

 static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
@@ -264,6 +270,9 @@ static int pl061_probe(struct amba_device *adev,
const struct amba_id *id)

        spin_lock_init(&chip->lock);

+       /* Hook the request()/free() for pinctrl operation */
+       if (of_property_read_bool(dev->of_node, "gpio-ranges"))
+               chip->uses_pinctrl = true;

Maybe it's more clear.

Best Regards
Haojian

Re: [PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Linus Walleij]

On Fri, Nov 28, 2014 at 6:00 PM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:

> It's a bit different.
>
> The commit 51e13c2475 is used to fix this scenario. There're 8 gpio
>  pins in GPIO CHIP #19. There're pin muxing on gpio152 -
>  gpio155. And there're _no_ pin muxing on gpio156 - gpio159. When
> user tries to request gpio159, he'll meet failure since the pinctrl device
> can't cover gpio159. But the pinctrl device is already register for
> gpio152. In order to distinguish whether the pinctrl device registered,
> I added pinctrl_ready_for_gpio_range(gpio).
>
> In another scenario, there's no back-end pinctrl device for GPIO
> CHIP #0. So pinctrl_request_gpio() always returns EPROBE_DEFER.
> The commit 51e13c2475 can't cover this.
>
> I suggest to write code in below.

This looks much simpler. Anyway: Xinwei whatever you come up
with, include Haojian on To: and get is ACK on the solution.

Yours,
Linus Walleij

Re: [PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[He YunLei]

On 2014/11/29 1:00, Haojian Zhuang wrote:
> On 28 November 2014 at 20:26, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Fri, Nov 21, 2014 at 7:43 AM, Yunlei He <heyunlei@huawei.com> wrote:
>>
>>> Gpio-ranges property is useful to represent which GPIOs correspond
>>> to which pins on which pin controllers. But there may be some gpios
>>> without pinctrl operation. So check whether gpio-ranges property
>>> exists in device node first.
>>>
>>> Signed-off-by: Yunlei He <heyunlei@huawei.com>
>>> Signed-off-by: Xinwei Kong <kong.kongxinwei@hisilicon.com>
>>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>>
>> (...)
>>
>>>   Documentation/devicetree/bindings/gpio/pl061-gpio.txt | 2 +-
>>>   drivers/gpio/gpio-pl061.c                             | 7 +++++--
>>>   2 files changed, 6 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
>>> index a2c416b..577bcf7 100644
>>> --- a/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
>>> +++ b/Documentation/devicetree/bindings/gpio/pl061-gpio.txt
>>> @@ -7,4 +7,4 @@ Required properties:
>>>     - bit 0 specifies polarity (0 for normal, 1 for inverted)
>>>   - gpio-controller : Marks the device node as a GPIO controller.
>>>   - interrupts : Interrupt mapping for GPIO IRQ.
>>> -
>>> +- gpio-ranges : Interaction with the PINCTRL subsystem
>>
>> This can be a separate patch. Please send it as such.
>>
>>> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
>>> index 84b49cf..01875d1 100644
>>> --- a/drivers/gpio/gpio-pl061.c
>>> +++ b/drivers/gpio/gpio-pl061.c
>>> @@ -24,6 +24,7 @@
>>>   #include <linux/slab.h>
>>>   #include <linux/pinctrl/consumer.h>
>>>   #include <linux/pm.h>
>>> +#include <linux/of_address.h>
>>
>> What you're checking for is not an address. This would be just
>> #include <linux/of.h>
>>
>>>          spin_lock_init(&chip->lock);
>>> +       if (of_get_property(dev->of_node, "gpio-ranges", NULL)) {
>>> +               chip->gc.request = pl061_gpio_request;
>>> +               chip->gc.free = pl061_gpio_free;
>>> +       }
>>>
>>> -       chip->gc.request = pl061_gpio_request;
>>> -       chip->gc.free = pl061_gpio_free;
>>
>> NAK.
>>
>> No this does not work. GPIO ranges doe not *have* to come from
>> the device tree, it is more common that a GPIO driver adds it by
>> way of gpiochip_add_pin_range().
>>
>> Haojian has already solved this problem in the pinctrl core.
>> Inspect commit 51e13c2475913d45a3ec546dee647538a9341d6a
>> "pinctrl: check pinctrl ready for gpio range"
>>
>> The call(s) to pinctrl_request_gpio() from
>> pl061_gpio_request() should already return silently with 0
>> AFAICT, Haojian do you agree?
>>
>
> It's a bit different.
>
> The commit 51e13c2475 is used to fix this scenario. There're 8 gpio
>   pins in GPIO CHIP #19. There're pin muxing on gpio152 -
>   gpio155. And there're _no_ pin muxing on gpio156 - gpio159. When
> user tries to request gpio159, he'll meet failure since the pinctrl device
> can't cover gpio159. But the pinctrl device is already register for
> gpio152. In order to distinguish whether the pinctrl device registered,
> I added pinctrl_ready_for_gpio_range(gpio).
>
> In another scenario, there's no back-end pinctrl device for GPIO
> CHIP #0. So pinctrl_request_gpio() always returns EPROBE_DEFER.
> The commit 51e13c2475 can't cover this.
>
> I suggest to write code in below.
>
> -static void pl061_gpio_free(struct gpio_chip *chip, unsigned offset)
> +static void pl061_gpio_free(struct gpio_chip *gc, unsigned offset)
>   {
> -       int gpio = chip->base + offset;
> +       struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc);
> +       int gpio = gc->base + offset;
>
> -       pinctrl_free_gpio(gpio);
> +       if (chip->uses_pinctrl)
> +               pinctrl_free_gpio(gpio);
>   }
>
>   static int pl061_direction_input(struct gpio_chip *gc, unsigned offset)
> @@ -264,6 +270,9 @@ static int pl061_probe(struct amba_device *adev,
> const struct amba_id *id)
>
>          spin_lock_init(&chip->lock);
>
> +       /* Hook the request()/free() for pinctrl operation */
> +       if (of_property_read_bool(dev->of_node, "gpio-ranges"))
> +               chip->uses_pinctrl = true;
>
> Maybe it's more clear.
>
> Best Regards
> Haojian
>
> .
>
Ok, thanks for reviews above. The modification in this path is really obscure. I will update this path
according to the comments and send a new version use the code suggested by Haojian.

Best Regards
Yunlei

Re: [PATCH] gpio: pl061: hook request if gpio-ranges avaiable

[Xinwei Kong]

On 2014/12/1 21:59, Linus Walleij wrote:
> On Fri, Nov 28, 2014 at 6:00 PM, Haojian Zhuang
> <haojian.zhuang@linaro.org> wrote:
>
>> It's a bit different.
>>
>> The commit 51e13c2475 is used to fix this scenario. There're 8 gpio
>>  pins in GPIO CHIP #19. There're pin muxing on gpio152 -
>>  gpio155. And there're _no_ pin muxing on gpio156 - gpio159. When
>> user tries to request gpio159, he'll meet failure since the pinctrl device
>> can't cover gpio159. But the pinctrl device is already register for
>> gpio152. In order to distinguish whether the pinctrl device registered,
>> I added pinctrl_ready_for_gpio_range(gpio).
>>
>> In another scenario, there's no back-end pinctrl device for GPIO
>> CHIP #0. So pinctrl_request_gpio() always returns EPROBE_DEFER.
>> The commit 51e13c2475 can't cover this.
>>
>> I suggest to write code in below.
> This looks much simpler. Anyway: Xinwei whatever you come up
> with, include Haojian on To: and get is ACK on the solution.
>
> Yours,
> Linus Walleij
>
hi Linus:

two patches slove the same problem about GPIO PL061.  You can understand our
patches and merge it in the next kernel version while ensuring that others use
the kernal will not meet with our problem. Because this bug spend  too much
time to debug our board. I will be glad to reduce the kernel bug and help others.

yours
Xinwei