| Message ID | 1416552238-9908-1-git-send-email-heyunlei@huawei.com |
|---|---|
| State | Rejected |
| Headers | show |
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. -- 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 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 -- 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 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 -- 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 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 -- 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 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 -- 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 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 -- 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/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;