| Message ID | 1435754753-31307-2-git-send-email-tomeu.vizoso@collabora.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: > When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if > the pin controller isn't available. > > Otherwise, the GPIO range wouldn't be set at all unless the pin > controller probed always before the GPIO chip. > > With this change, the probe of the GPIO chip will be deferred and will > be retried at a later point, hopefully once the pin controller has been > registered and probed already. This will break cases where the pinctrl driver does not exist, but the DT contains pinctrl bindings. We can have similar problems already with clocks though. However, IMO this problem is a bit different in that pinctrl is more likely entirely optional while clocks are often required. You may do all pin setup in bootloader/firmware on some boards and not others. Of course then why put pinctrl in the DT in that case? They could be present just due to how chip vs. board dts files are structured. We could address this by simply marking the pin controller node disabled. However, ... > @@ -361,7 +361,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip) > > pctldev = of_pinctrl_get(pinspec.np); > if (!pctldev) > - break; > + return -EPROBE_DEFER; But you cannot distinguish that case here. I think of_pinctrl_get needs to set the error code appropriately. Rob -- 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
Hi Tomeu, On Wed, Jul 1, 2015 at 2:45 PM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: > When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if > the pin controller isn't available. > > Otherwise, the GPIO range wouldn't be set at all unless the pin > controller probed always before the GPIO chip. > > With this change, the probe of the GPIO chip will be deferred and will > be retried at a later point, hopefully once the pin controller has been > registered and probed already. > > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> I was a bit afraid this would break the case of gpio controllers that are also pin controllers, i.e. where "gpio-ranges" points to the gpio controller itself[*], but it doesn't. Tested-by: Geert Uytterhoeven <geert+renesas@glider.be> [*] E.g. "[PATCH 2/7] ARM: shmobile: r8a7740 dtsi: Add missing "gpio-ranges" to gpio node" (http://www.spinics.net/lists/linux-sh/msg43077.html) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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
Hi Rob, On Wed, Jul 1, 2015 at 7:36 PM, Rob Herring <robherring2@gmail.com> wrote: > On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: >> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if >> the pin controller isn't available. >> >> Otherwise, the GPIO range wouldn't be set at all unless the pin >> controller probed always before the GPIO chip. >> >> With this change, the probe of the GPIO chip will be deferred and will >> be retried at a later point, hopefully once the pin controller has been >> registered and probed already. > > This will break cases where the pinctrl driver does not exist, but the > DT contains pinctrl bindings. We can have similar problems already > with clocks though. However, IMO this problem is a bit different in > that pinctrl is more likely entirely optional while clocks are often > required. You may do all pin setup in bootloader/firmware on some > boards and not others. Of course then why put pinctrl in the DT in > that case? They could be present just due to how chip vs. board dts > files are structured. Isn't that already the case? If I change the compatible value of a pinctrl node to an invalid value, I get: sh-sci e6c50000.serial: could not find pctldev for node /pfc@e6050000/serial1, deferring probe > We could address this by simply marking the pin controller node > disabled. However, ... Doesn't seem to make any difference. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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 Thu, Jul 2, 2015 at 3:59 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Rob, > > On Wed, Jul 1, 2015 at 7:36 PM, Rob Herring <robherring2@gmail.com> wrote: >> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: >>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if >>> the pin controller isn't available. >>> >>> Otherwise, the GPIO range wouldn't be set at all unless the pin >>> controller probed always before the GPIO chip. >>> >>> With this change, the probe of the GPIO chip will be deferred and will >>> be retried at a later point, hopefully once the pin controller has been >>> registered and probed already. >> >> This will break cases where the pinctrl driver does not exist, but the >> DT contains pinctrl bindings. We can have similar problems already >> with clocks though. However, IMO this problem is a bit different in >> that pinctrl is more likely entirely optional while clocks are often >> required. You may do all pin setup in bootloader/firmware on some >> boards and not others. Of course then why put pinctrl in the DT in >> that case? They could be present just due to how chip vs. board dts >> files are structured. > > Isn't that already the case? > If I change the compatible value of a pinctrl node to an invalid value, I get: > > sh-sci e6c50000.serial: could not find pctldev for node > /pfc@e6050000/serial1, deferring probe I guess so. >> We could address this by simply marking the pin controller node >> disabled. However, ... > > Doesn't seem to make any difference. No doubt. I'm proposing that it should, not that it does already. Of course, the callers will also have to test for -ENODEV and ignore those errors. Rob -- 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 1 July 2015 at 19:36, Rob Herring <robherring2@gmail.com> wrote: > On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: >> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if >> the pin controller isn't available. >> >> Otherwise, the GPIO range wouldn't be set at all unless the pin >> controller probed always before the GPIO chip. >> >> With this change, the probe of the GPIO chip will be deferred and will >> be retried at a later point, hopefully once the pin controller has been >> registered and probed already. > > This will break cases where the pinctrl driver does not exist, but the > DT contains pinctrl bindings. We can have similar problems already > with clocks though. However, IMO this problem is a bit different in > that pinctrl is more likely entirely optional while clocks are often > required. You may do all pin setup in bootloader/firmware on some > boards and not others. Of course then why put pinctrl in the DT in > that case? They could be present just due to how chip vs. board dts > files are structured. I see. My instinct tells me that it would be better if the gpio-ranges property was set in the board dts, but I don't really know what each mach does with its DTSs. > We could address this by simply marking the pin controller node > disabled. However, ... > >> @@ -361,7 +361,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip) >> >> pctldev = of_pinctrl_get(pinspec.np); >> if (!pctldev) >> - break; >> + return -EPROBE_DEFER; > > But you cannot distinguish that case here. I think of_pinctrl_get > needs to set the error code appropriately. Why not? I was thinking of just doing this before we call of_pinctrl_get(): if (!of_device_is_available(pinspec.np)) continue; Thanks, Tomeu > Rob > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- 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 07/10/2015 03:29 AM, Tomeu Vizoso wrote: > On 1 July 2015 at 19:36, Rob Herring <robherring2@gmail.com> wrote: >> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: >>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if >>> the pin controller isn't available. >>> >>> Otherwise, the GPIO range wouldn't be set at all unless the pin >>> controller probed always before the GPIO chip. >>> >>> With this change, the probe of the GPIO chip will be deferred and will >>> be retried at a later point, hopefully once the pin controller has been >>> registered and probed already. >> >> This will break cases where the pinctrl driver does not exist, but the >> DT contains pinctrl bindings. We can have similar problems already >> with clocks though. However, IMO this problem is a bit different in >> that pinctrl is more likely entirely optional while clocks are often >> required. You may do all pin setup in bootloader/firmware on some >> boards and not others. Of course then why put pinctrl in the DT in >> that case? They could be present just due to how chip vs. board dts >> files are structured. > > I see. My instinct tells me that it would be better if the gpio-ranges > property was set in the board dts, but I don't really know what each > mach does with its DTSs. That doesn't make sense; the mapping between GPIO controller pins and pin controller pins is a property of the SoC not the board. -- 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 10 July 2015 at 17:27, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 07/10/2015 03:29 AM, Tomeu Vizoso wrote: >> >> On 1 July 2015 at 19:36, Rob Herring <robherring2@gmail.com> wrote: >>> >>> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> >>> wrote: >>>> >>>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if >>>> the pin controller isn't available. >>>> >>>> Otherwise, the GPIO range wouldn't be set at all unless the pin >>>> controller probed always before the GPIO chip. >>>> >>>> With this change, the probe of the GPIO chip will be deferred and will >>>> be retried at a later point, hopefully once the pin controller has been >>>> registered and probed already. >>> >>> >>> This will break cases where the pinctrl driver does not exist, but the >>> DT contains pinctrl bindings. We can have similar problems already >>> with clocks though. However, IMO this problem is a bit different in >>> that pinctrl is more likely entirely optional while clocks are often >>> required. You may do all pin setup in bootloader/firmware on some >>> boards and not others. Of course then why put pinctrl in the DT in >>> that case? They could be present just due to how chip vs. board dts >>> files are structured. >> >> >> I see. My instinct tells me that it would be better if the gpio-ranges >> property was set in the board dts, but I don't really know what each >> mach does with its DTSs. > > > That doesn't make sense; the mapping between GPIO controller pins and pin > controller pins is a property of the SoC not the board. From what Rob said above, apparently some boards will rely on the pin setup done by the bootloader, and some other boards with the same soc will want to do it in the kernel. So it's not really a difference in the hw itself, but what expectations exist about the firmware on a specific board. Regards, Tomeu -- 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 07/10/2015 10:21 AM, Tomeu Vizoso wrote: > On 10 July 2015 at 17:27, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 07/10/2015 03:29 AM, Tomeu Vizoso wrote: >>> >>> On 1 July 2015 at 19:36, Rob Herring <robherring2@gmail.com> wrote: >>>> >>>> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> >>>> wrote: >>>>> >>>>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if >>>>> the pin controller isn't available. >>>>> >>>>> Otherwise, the GPIO range wouldn't be set at all unless the pin >>>>> controller probed always before the GPIO chip. >>>>> >>>>> With this change, the probe of the GPIO chip will be deferred and will >>>>> be retried at a later point, hopefully once the pin controller has been >>>>> registered and probed already. >>>> >>>> >>>> This will break cases where the pinctrl driver does not exist, but the >>>> DT contains pinctrl bindings. We can have similar problems already >>>> with clocks though. However, IMO this problem is a bit different in >>>> that pinctrl is more likely entirely optional while clocks are often >>>> required. You may do all pin setup in bootloader/firmware on some >>>> boards and not others. Of course then why put pinctrl in the DT in >>>> that case? They could be present just due to how chip vs. board dts >>>> files are structured. >>> >>> >>> I see. My instinct tells me that it would be better if the gpio-ranges >>> property was set in the board dts, but I don't really know what each >>> mach does with its DTSs. >> >> >> That doesn't make sense; the mapping between GPIO controller pins and pin >> controller pins is a property of the SoC not the board. > > From what Rob said above, apparently some boards will rely on the pin > setup done by the bootloader, and some other boards with the same soc > will want to do it in the kernel. So it's not really a difference in > the hw itself, but what expectations exist about the firmware on a > specific board. Sure, but none of that changes the mapping between the GPIO and pin controller pins. -- 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, Jul 10, 2015 at 4:29 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: > On 1 July 2015 at 19:36, Rob Herring <robherring2@gmail.com> wrote: >> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote: >>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if >>> the pin controller isn't available. >>> >>> Otherwise, the GPIO range wouldn't be set at all unless the pin >>> controller probed always before the GPIO chip. >>> >>> With this change, the probe of the GPIO chip will be deferred and will >>> be retried at a later point, hopefully once the pin controller has been >>> registered and probed already. >> >> This will break cases where the pinctrl driver does not exist, but the >> DT contains pinctrl bindings. We can have similar problems already >> with clocks though. However, IMO this problem is a bit different in >> that pinctrl is more likely entirely optional while clocks are often >> required. You may do all pin setup in bootloader/firmware on some >> boards and not others. Of course then why put pinctrl in the DT in >> that case? They could be present just due to how chip vs. board dts >> files are structured. > > I see. My instinct tells me that it would be better if the gpio-ranges > property was set in the board dts, but I don't really know what each > mach does with its DTSs. > >> We could address this by simply marking the pin controller node >> disabled. However, ... >> >>> @@ -361,7 +361,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip) >>> >>> pctldev = of_pinctrl_get(pinspec.np); >>> if (!pctldev) >>> - break; >>> + return -EPROBE_DEFER; >> >> But you cannot distinguish that case here. I think of_pinctrl_get >> needs to set the error code appropriately. > > Why not? I was thinking of just doing this before we call of_pinctrl_get(): > > if (!of_device_is_available(pinspec.np)) > continue; That is exactly what you need, but that should be of_pinctrl_get's responsibility to check, not the caller's. IIRC, this is the only user of of_pinctrl_get, so it should be just as easy to change. Rob -- 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/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index 9a0ec48..4e1f73b 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -338,7 +338,7 @@ void of_mm_gpiochip_remove(struct of_mm_gpio_chip *mm_gc) EXPORT_SYMBOL(of_mm_gpiochip_remove); #ifdef CONFIG_PINCTRL -static void of_gpiochip_add_pin_range(struct gpio_chip *chip) +static int of_gpiochip_add_pin_range(struct gpio_chip *chip) { struct device_node *np = chip->of_node; struct of_phandle_args pinspec; @@ -349,7 +349,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip) struct property *group_names; if (!np) - return; + return 0; group_names = of_find_property(np, group_names_propname, NULL); @@ -361,7 +361,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip) pctldev = of_pinctrl_get(pinspec.np); if (!pctldev) - break; + return -EPROBE_DEFER; if (pinspec.args[2]) { if (group_names) { @@ -381,7 +381,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip) pinspec.args[1], pinspec.args[2]); if (ret) - break; + return ret; } else { /* npins == 0: special range */ if (pinspec.args[1]) { @@ -411,32 +411,41 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip) ret = gpiochip_add_pingroup_range(chip, pctldev, pinspec.args[0], name); if (ret) - break; + return ret; } } + + return 0; } #else -static void of_gpiochip_add_pin_range(struct gpio_chip *chip) {} +static int of_gpiochip_add_pin_range(struct gpio_chip *chip) { return 0; } #endif -void of_gpiochip_add(struct gpio_chip *chip) +int of_gpiochip_add(struct gpio_chip *chip) { + int status; + if ((!chip->of_node) && (chip->dev)) chip->of_node = chip->dev->of_node; if (!chip->of_node) - return; + return 0; if (!chip->of_xlate) { chip->of_gpio_n_cells = 2; chip->of_xlate = of_gpio_simple_xlate; } - of_gpiochip_add_pin_range(chip); + status = of_gpiochip_add_pin_range(chip); + if (status) + return status; + of_node_get(chip->of_node); of_gpiochip_scan_hogs(chip); + + return 0; } void of_gpiochip_remove(struct gpio_chip *chip) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index bf4bd1d..a8cab33 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -287,7 +287,10 @@ int gpiochip_add(struct gpio_chip *chip) INIT_LIST_HEAD(&chip->pin_ranges); #endif - of_gpiochip_add(chip); + status = of_gpiochip_add(chip); + if (status) + goto err_remove_chip; + acpi_gpiochip_add(chip); status = gpiochip_sysfs_register(chip); diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h index 69dbe31..f319182 100644 --- a/include/linux/of_gpio.h +++ b/include/linux/of_gpio.h @@ -54,7 +54,7 @@ extern int of_mm_gpiochip_add(struct device_node *np, struct of_mm_gpio_chip *mm_gc); extern void of_mm_gpiochip_remove(struct of_mm_gpio_chip *mm_gc); -extern void of_gpiochip_add(struct gpio_chip *gc); +extern int of_gpiochip_add(struct gpio_chip *gc); extern void of_gpiochip_remove(struct gpio_chip *gc); extern int of_gpio_simple_xlate(struct gpio_chip *gc, const struct of_phandle_args *gpiospec, @@ -76,7 +76,7 @@ static inline int of_gpio_simple_xlate(struct gpio_chip *gc, return -ENOSYS; } -static inline void of_gpiochip_add(struct gpio_chip *gc) { } +static inline int of_gpiochip_add(struct gpio_chip *gc) { return 0; } static inline void of_gpiochip_remove(struct gpio_chip *gc) { } #endif /* CONFIG_OF_GPIO */
When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if the pin controller isn't available. Otherwise, the GPIO range wouldn't be set at all unless the pin controller probed always before the GPIO chip. With this change, the probe of the GPIO chip will be deferred and will be retried at a later point, hopefully once the pin controller has been registered and probed already. Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> --- drivers/gpio/gpiolib-of.c | 27 ++++++++++++++++++--------- drivers/gpio/gpiolib.c | 5 ++++- include/linux/of_gpio.h | 4 ++-- 3 files changed, 24 insertions(+), 12 deletions(-)