| Message ID | 1358494279-16503-9-git-send-email-haojian.zhuang@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 18, 2013 at 8:31 AM, Haojian Zhuang <haojian.zhuang@linaro.org> wrote: > Add the pl061_gpio_request() to request pinctrl. Create the logic > between pl061 gpio driver and pinctrl (pinctrl-single) driver. > > While a gpio pin is requested, it will request pinctrl driver to > set that pin with gpio function mode. So pinctrl driver should > append .gpio_request_enable() in pinmux_ops. > > Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> (...) > +static int pl061_gpio_request(struct gpio_chip *chip, 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; > + > + /* > + * Do NOT check the return value at here. Since sometimes the gpio > + * pin needn't to be configured in pinmux controller. So it's > + * impossible to find the matched gpio range. > + */ > + pinctrl_request_gpio(gpio); Handling of error code? (Maybe I should add a __must_check on this function.) > + return 0; > +} > + > static int pl061_direction_input(struct gpio_chip *gc, unsigned offset) > { > struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc); > @@ -251,6 +269,7 @@ 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.direction_input = pl061_direction_input; > chip->gc.direction_output = pl061_direction_output; > chip->gc.get = pl061_get_value; What happens on a platform that has a PL061 GPIO block but no pinctrl related to it? But still has some other pinctrl driver in the platform .... Right, it'll return -EPROBE_DEFER from pinctrl_request_gpio(). This may happen on for example a combined SPEAr kernel where some platforms have PL061 and others us a pin controller, so both will be enabled. I think, add a field like this to struct pl061_gpio: bool has_pinctrl_backend; The only call that from pl061_gpio_request() if this is set: if (pl061->has_pinctrl_backend) ret = pinctrl_request_gpio(gpio); Then assign it in some clever way. For DT I think the proper way would be so add a cross-binding to the pin controller, like: gpio2: gpio@d8100000 { #gpio-cells = <2>; compatible = "arm,pl061", "arm,primecell"; (...) pinctrl = <&mr_pincontrol>; }; Then just check if you have this pinctrl binding set to figure out if has_pinctrl_backend should be true. Yours, Linus Walleij
On 21 January 2013 22:37, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Fri, Jan 18, 2013 at 8:31 AM, Haojian Zhuang > <haojian.zhuang@linaro.org> wrote: > > > Add the pl061_gpio_request() to request pinctrl. Create the logic > > between pl061 gpio driver and pinctrl (pinctrl-single) driver. > > > > While a gpio pin is requested, it will request pinctrl driver to > > set that pin with gpio function mode. So pinctrl driver should > > append .gpio_request_enable() in pinmux_ops. > > > > Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> > (...) > > +static int pl061_gpio_request(struct gpio_chip *chip, 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; > > + > > + /* > > + * Do NOT check the return value at here. Since sometimes the gpio > > + * pin needn't to be configured in pinmux controller. So it's > > + * impossible to find the matched gpio range. > > + */ > > + pinctrl_request_gpio(gpio); > > Handling of error code? > > (Maybe I should add a __must_check on this function.) > My case is a little special. I don't want to check return value because some gpio pins don't have pinmux registers in Hisilicon SoC. So pinctrl_request_gpio() will always return error for these special pins in Hisilicon SoC. If we must check the return value, maybe we need append a dummy pinctrl driver for those special gpio pins. How do you think about it? Of course, I need to evaluate whether it's possible to implement. > > + return 0; > > +} > > + > > static int pl061_direction_input(struct gpio_chip *gc, unsigned offset) > > { > > struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc); > > @@ -251,6 +269,7 @@ 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.direction_input = pl061_direction_input; > > chip->gc.direction_output = pl061_direction_output; > > chip->gc.get = pl061_get_value; > > What happens on a platform that has a PL061 > GPIO block but no pinctrl related to it? > > But still has some other pinctrl driver in the platform .... > > Right, it'll return -EPROBE_DEFER from pinctrl_request_gpio(). > > This may happen on for example a combined SPEAr > kernel where some platforms have PL061 and others us > a pin controller, so both will be enabled. > > I think, add a field like this to struct pl061_gpio: > > bool has_pinctrl_backend; > > The only call that from pl061_gpio_request() if this is > set: > > if (pl061->has_pinctrl_backend) > ret = pinctrl_request_gpio(gpio); I'm OK to append "has_pinctrl_backend". But if we append a dummy pinctrl driver, is it OK to SPEAr kernel? Do we still need has_pinctrl_backend? > > Then assign it in some clever way. For DT I think the > proper way would be so add a cross-binding to the > pin controller, like: > > gpio2: gpio@d8100000 { > #gpio-cells = <2>; > compatible = "arm,pl061", "arm,primecell"; > (...) > pinctrl = <&mr_pincontrol>; > }; If we could append a dummy pinctrl driver, maybe we needn't to append pinctrl property into gpio node. What's your opinion? > > Then just check if you have this pinctrl binding set > to figure out if has_pinctrl_backend should be true. > > Yours, > Linus Walleij Regards Haojian
On Mon, Jan 21, 2013 at 4:45 PM, Haojian Zhuang <haojian.zhuang@linaro.org> wrote: >> > + pinctrl_request_gpio(gpio); >> >> Handling of error code? >> >> (Maybe I should add a __must_check on this function.) >> > My case is a little special. I don't want to check return value because some > gpio pins don't have pinmux registers in Hisilicon SoC. > So pinctrl_request_gpio() will always return error for these special pins in > Hisilicon SoC. > > If we must check the return value, maybe we need append a dummy pinctrl driver > for those special gpio pins. How do you think about it? Of course, I > need to evaluate > whether it's possible to implement. Hm. A dummy pinctrl back-end is not very elegant. It's better if the GPIO driver (gpio-pl061) keep track of the ranges that are connected to the pinctrl. If the ranges are encoded in the device tree (as I guess you want to do in this case) then the GPIO driver need to check these ranges to see if it uses a pinctrl backend. Magic behind-the-scenes is very hard to understand for people reading this code later. What about this: In gpiolib.c, function gpiochip_add_pin_range(), we save a copy of each range in &chip->pin_ranges. Add a function to gpiolib.c to check if a certain gpio is in the range of the current chip. Then use that: if (gpio_in_pinrange(gpio)) pinctrl_request_gpio(gpio); Yours, Linus Walleij
On 22 January 2013 17:10, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Jan 21, 2013 at 4:45 PM, Haojian Zhuang > <haojian.zhuang@linaro.org> wrote: > >>> > + pinctrl_request_gpio(gpio); >>> >>> Handling of error code? >>> >>> (Maybe I should add a __must_check on this function.) >>> >> My case is a little special. I don't want to check return value because some >> gpio pins don't have pinmux registers in Hisilicon SoC. >> So pinctrl_request_gpio() will always return error for these special pins in >> Hisilicon SoC. >> >> If we must check the return value, maybe we need append a dummy pinctrl driver >> for those special gpio pins. How do you think about it? Of course, I >> need to evaluate >> whether it's possible to implement. > > Hm. A dummy pinctrl back-end is not very elegant. > > It's better if the GPIO driver (gpio-pl061) keep track of the ranges that > are connected to the pinctrl. > > If the ranges are encoded in the device tree (as I guess you want to do > in this case) then the GPIO driver need to check these ranges to see if > it uses a pinctrl backend. Magic behind-the-scenes is very hard to > understand for people reading this code later. > > What about this: > > In gpiolib.c, function gpiochip_add_pin_range(), we save a copy of > each range in &chip->pin_ranges. > > Add a function to gpiolib.c to check if a certain gpio is in the range > of the current chip. > > Then use that: > > if (gpio_in_pinrange(gpio)) > pinctrl_request_gpio(gpio); > > Yours, > Linus Walleij It seems better. I'll update it. Regards Haojian
On Tue, Jan 22, 2013 at 10:55 AM, Haojian Zhuang <haojian.zhuang@linaro.org> wrote: >> Add a function to gpiolib.c to check if a certain gpio is in the range >> of the current chip. >> >> Then use that: >> >> if (gpio_in_pinrange(gpio)) >> pinctrl_request_gpio(gpio); >> >> Yours, >> Linus Walleij > > It seems better. I'll update it. Hm the function above will need to pass the chip as parameter as well, so a bit simplified. Maybe it's better to add a function like gpio_check_pinctrl_backend(chip, gpio); that does the above two statements? Yours, Linus Walleij
diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c index 8336719..8cfdf60 100644 --- a/drivers/gpio/gpio-pl061.c +++ b/drivers/gpio/gpio-pl061.c @@ -23,6 +23,7 @@ #include <linux/amba/bus.h> #include <linux/amba/pl061.h> #include <linux/slab.h> +#include <linux/pinctrl/consumer.h> #include <linux/pm.h> #include <asm/mach/irq.h> @@ -61,6 +62,23 @@ struct pl061_gpio { #endif }; +static int pl061_gpio_request(struct gpio_chip *chip, 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; + + /* + * Do NOT check the return value at here. Since sometimes the gpio + * pin needn't to be configured in pinmux controller. So it's + * impossible to find the matched gpio range. + */ + pinctrl_request_gpio(gpio); + return 0; +} + static int pl061_direction_input(struct gpio_chip *gc, unsigned offset) { struct pl061_gpio *chip = container_of(gc, struct pl061_gpio, gc); @@ -251,6 +269,7 @@ 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.direction_input = pl061_direction_input; chip->gc.direction_output = pl061_direction_output; chip->gc.get = pl061_get_value;
Add the pl061_gpio_request() to request pinctrl. Create the logic between pl061 gpio driver and pinctrl (pinctrl-single) driver. While a gpio pin is requested, it will request pinctrl driver to set that pin with gpio function mode. So pinctrl driver should append .gpio_request_enable() in pinmux_ops. Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> --- drivers/gpio/gpio-pl061.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)