| Message ID | 1360602659-4774-5-git-send-email-haojian.zhuang@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 11, 2013 at 6:10 PM, Haojian Zhuang <haojian.zhuang@linaro.org> wrote: > /** > + * pinctrl_overlapped_gpio_range() - check if the GPIO chip of a certain GPIO > + * pin is overlapped with gpio range. > + * @gpio: gpio pin to check taken from the global GPIO pin space > + * > + * This function is complement of pinctrl_match_gpio_range(). If the return > + * value of pinctrl_match_gpio_range() is NULL, this function could be used > + * to check whether pinctrl device is ready or not. Maybe some GPIO pins > + * don't have back-end pinctrl interface. > + * If the return value is true, it means that pinctrl device is ready & the > + * certain GPIO pin doesn't have back-end pinctrl device. If the return value > + * is false, it means that pinctrl device may not be ready. > + */ > +static bool pinctrl_overlapped_gpio_range(unsigned gpio) The name of this function confuses me, can we name it something like: pinctrl_gpio_is_in_some_chip_but_no_range() Which is actually what you test, right? > +{ > + struct pinctrl_dev *pctldev; > + struct pinctrl_gpio_range *range = NULL; > + struct gpio_chip *chip = gpio_to_chip(gpio); Handle if chip happens to become NULL here. > + /* Loop over the pin controllers */ > + list_for_each_entry(pctldev, &pinctrldev_list, node) { > + /* Loop over the ranges */ > + list_for_each_entry(range, &pctldev->gpio_ranges, node) { > + /* Check if any gpio range overlapped with gpio chip */ > + if (range->base + range->npins - 1 < chip->base || > + range->base > chip->base + chip->ngpio - 1) > + continue; > + return true; > + } > + } > + return false; > +} > + > +/** > * pinctrl_get_device_gpio_range() - find device for GPIO range > * @gpio: the pin to locate the pin controller for > * @outdev: the pin control device if found > @@ -459,6 +493,8 @@ int pinctrl_request_gpio(unsigned gpio) > > ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range); > if (ret) { > + if (pinctrl_overlapped_gpio_range(gpio)) > + ret = 0; If you change the name of the function like above this call can be understood, but maybe also add a comment above this call explaining the situation. Yours, Linus Walleij
On 14 February 2013 23:23, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Feb 11, 2013 at 6:10 PM, Haojian Zhuang > <haojian.zhuang@linaro.org> wrote: > >> /** >> + * pinctrl_overlapped_gpio_range() - check if the GPIO chip of a certain GPIO >> + * pin is overlapped with gpio range. >> + * @gpio: gpio pin to check taken from the global GPIO pin space >> + * >> + * This function is complement of pinctrl_match_gpio_range(). If the return >> + * value of pinctrl_match_gpio_range() is NULL, this function could be used >> + * to check whether pinctrl device is ready or not. Maybe some GPIO pins >> + * don't have back-end pinctrl interface. >> + * If the return value is true, it means that pinctrl device is ready & the >> + * certain GPIO pin doesn't have back-end pinctrl device. If the return value >> + * is false, it means that pinctrl device may not be ready. >> + */ >> +static bool pinctrl_overlapped_gpio_range(unsigned gpio) > > The name of this function confuses me, can we name it something > like: > > pinctrl_gpio_is_in_some_chip_but_no_range() > > Which is actually what you test, right? > How about pinctrl_match_gpio_range_exclude_some_pins()? >> +{ >> + struct pinctrl_dev *pctldev; >> + struct pinctrl_gpio_range *range = NULL; >> + struct gpio_chip *chip = gpio_to_chip(gpio); > > Handle if chip happens to become NULL here. > >> + /* Loop over the pin controllers */ >> + list_for_each_entry(pctldev, &pinctrldev_list, node) { >> + /* Loop over the ranges */ >> + list_for_each_entry(range, &pctldev->gpio_ranges, node) { >> + /* Check if any gpio range overlapped with gpio chip */ >> + if (range->base + range->npins - 1 < chip->base || >> + range->base > chip->base + chip->ngpio - 1) >> + continue; >> + return true; >> + } >> + } >> + return false; >> +} >> + >> +/** >> * pinctrl_get_device_gpio_range() - find device for GPIO range >> * @gpio: the pin to locate the pin controller for >> * @outdev: the pin control device if found >> @@ -459,6 +493,8 @@ int pinctrl_request_gpio(unsigned gpio) >> >> ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range); >> if (ret) { >> + if (pinctrl_overlapped_gpio_range(gpio)) >> + ret = 0; > > If you change the name of the function like above this call can be understood, > but maybe also add a comment above this call explaining the situation. > > Yours, > Linus Walleij
On Thu, Feb 14, 2013 at 6:01 PM, Haojian Zhuang <haojian.zhuang@linaro.org> wrote: > On 14 February 2013 23:23, Linus Walleij <linus.walleij@linaro.org> wrote: >> On Mon, Feb 11, 2013 at 6:10 PM, Haojian Zhuang >> <haojian.zhuang@linaro.org> wrote: >> >>> /** >>> + * pinctrl_overlapped_gpio_range() - check if the GPIO chip of a certain GPIO >>> + * pin is overlapped with gpio range. >>> + * @gpio: gpio pin to check taken from the global GPIO pin space >>> + * >>> + * This function is complement of pinctrl_match_gpio_range(). If the return >>> + * value of pinctrl_match_gpio_range() is NULL, this function could be used >>> + * to check whether pinctrl device is ready or not. Maybe some GPIO pins >>> + * don't have back-end pinctrl interface. >>> + * If the return value is true, it means that pinctrl device is ready & the >>> + * certain GPIO pin doesn't have back-end pinctrl device. If the return value >>> + * is false, it means that pinctrl device may not be ready. >>> + */ >>> +static bool pinctrl_overlapped_gpio_range(unsigned gpio) >> >> The name of this function confuses me, can we name it something >> like: >> >> pinctrl_gpio_is_in_some_chip_but_no_range() >> >> Which is actually what you test, right? >> > How about pinctrl_match_gpio_range_exclude_some_pins()? Actually I get ever more confused by this patch, I just don't understand it. :-( Something will need to be expanded on here or I will be unable to maintaine the code, sorry for being a slow learner... By the way: Nowaday the recommended practice is to register a GPIO chip, then add ranges using gpiochip_add_pin_range() in the non-DT case. Doesn't that create a race window when the above code will not behave as expected, e.g. between registration of the gpio chip, the pin controller and the ranges? I guess the code assumes that *all* initialization is complete? Yours, Linus Walleij
On 15 February 2013 17:06, Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Feb 14, 2013 at 6:01 PM, Haojian Zhuang > <haojian.zhuang@linaro.org> wrote: >> On 14 February 2013 23:23, Linus Walleij <linus.walleij@linaro.org> wrote: >>> On Mon, Feb 11, 2013 at 6:10 PM, Haojian Zhuang >>> <haojian.zhuang@linaro.org> wrote: >>> >>>> /** >>>> + * pinctrl_overlapped_gpio_range() - check if the GPIO chip of a certain GPIO >>>> + * pin is overlapped with gpio range. >>>> + * @gpio: gpio pin to check taken from the global GPIO pin space >>>> + * >>>> + * This function is complement of pinctrl_match_gpio_range(). If the return >>>> + * value of pinctrl_match_gpio_range() is NULL, this function could be used >>>> + * to check whether pinctrl device is ready or not. Maybe some GPIO pins >>>> + * don't have back-end pinctrl interface. >>>> + * If the return value is true, it means that pinctrl device is ready & the >>>> + * certain GPIO pin doesn't have back-end pinctrl device. If the return value >>>> + * is false, it means that pinctrl device may not be ready. >>>> + */ >>>> +static bool pinctrl_overlapped_gpio_range(unsigned gpio) >>> >>> The name of this function confuses me, can we name it something >>> like: >>> >>> pinctrl_gpio_is_in_some_chip_but_no_range() >>> >>> Which is actually what you test, right? >>> >> How about pinctrl_match_gpio_range_exclude_some_pins()? > > Actually I get ever more confused by this patch, I just don't understand > it. :-( > > Something will need to be expanded on here or I will be unable to > maintaine the code, sorry for being a slow learner... > > By the way: > Nowaday the recommended practice is to register a GPIO chip, > then add ranges using gpiochip_add_pin_range() in the non-DT > case. Doesn't that create a race window when the above code will > not behave as expected, e.g. between registration of the gpio > chip, the pin controller and the ranges? I guess the code assumes > that *all* initialization is complete? > > Yours, > Linus Walleij The implementation is a little tricky at here. Let me explain the case. We have a back-end pinctrl-single device & pl061 gpio devices. So we need to define gpio range in DT. But there's no related pinmux registers on some special pins, like gpio159. Although we initialized pinctrl-single driver, we still get failure from pinctrl_match_gpio_range() because it doesn't exist in DT. I implement it to double check for the failure case. If match_gpio_range() return failure, we need to check whether the pinctrl device isn't ready or the special pins like gpio159 don't exist in gpio range. So if the pinctrl device isn't ready, it return EPROBE_DEFERED. If the special pins doesn't exist in gpio range, it's not the error case & return 0 (sucessful). Regards Haojian
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c index b0a8e9a..140d6cb 100644 --- a/drivers/pinctrl/core.c +++ b/drivers/pinctrl/core.c @@ -27,6 +27,7 @@ #include <linux/pinctrl/consumer.h> #include <linux/pinctrl/pinctrl.h> #include <linux/pinctrl/machine.h> +#include <asm-generic/gpio.h> #include "core.h" #include "devicetree.h" #include "pinmux.h" @@ -293,6 +294,39 @@ pinctrl_match_gpio_range(struct pinctrl_dev *pctldev, unsigned gpio) } /** + * pinctrl_overlapped_gpio_range() - check if the GPIO chip of a certain GPIO + * pin is overlapped with gpio range. + * @gpio: gpio pin to check taken from the global GPIO pin space + * + * This function is complement of pinctrl_match_gpio_range(). If the return + * value of pinctrl_match_gpio_range() is NULL, this function could be used + * to check whether pinctrl device is ready or not. Maybe some GPIO pins + * don't have back-end pinctrl interface. + * If the return value is true, it means that pinctrl device is ready & the + * certain GPIO pin doesn't have back-end pinctrl device. If the return value + * is false, it means that pinctrl device may not be ready. + */ +static bool pinctrl_overlapped_gpio_range(unsigned gpio) +{ + struct pinctrl_dev *pctldev; + struct pinctrl_gpio_range *range = NULL; + struct gpio_chip *chip = gpio_to_chip(gpio); + + /* Loop over the pin controllers */ + list_for_each_entry(pctldev, &pinctrldev_list, node) { + /* Loop over the ranges */ + list_for_each_entry(range, &pctldev->gpio_ranges, node) { + /* Check if any gpio range overlapped with gpio chip */ + if (range->base + range->npins - 1 < chip->base || + range->base > chip->base + chip->ngpio - 1) + continue; + return true; + } + } + return false; +} + +/** * pinctrl_get_device_gpio_range() - find device for GPIO range * @gpio: the pin to locate the pin controller for * @outdev: the pin control device if found @@ -459,6 +493,8 @@ int pinctrl_request_gpio(unsigned gpio) ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range); if (ret) { + if (pinctrl_overlapped_gpio_range(gpio)) + ret = 0; mutex_unlock(&pinctrl_mutex); return ret; }
pinctrl_get_device_gpio_range() only checks whether a certain GPIO pin is in gpio range. But maybe some GPIO pins don't have back-end pinctrl interface, it means that these pins are always configured as GPIO function. Append pinctrl_overlapped_gpio_range() that is used to check whether the pins of GPIO chip are overlapped with pins in the GPIO range. This function will be called after pinctrl_get_device_gpio_range() fails. If overlapped GPIO pins are found, it means that pinctrl device is already launched and a certain GPIO pin don't have back-end pinctrl interface. Then pinctrl_request_gpio() shouldn't return -EPROBE_DEFER in this case. Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org> --- drivers/pinctrl/core.c | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)