| Message ID | 20180410203028.11412-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] gpiolib: add hogs support for machine code | expand |
On Dienstag, 10. April 2018 22:30:28 CEST Bartosz Golaszewski wrote: > Board files constitute a significant part of the users of the legacy > GPIO framework. In many cases they only export a line and set its > desired value. We could use GPIO hogs for that like we do for DT and > ACPI but there's no support for that in machine code. > > This patch proposes to extend the machine.h API with support for > registering hog tables in board files. > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > --- > @@ -1326,6 +1364,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data, > > acpi_gpiochip_add(chip); > > + machine_gpiochip_add(chip); > + > /* > * By first adding the chardev, and then adding the device, > * we get a device node entry in sysfs under > @@ -3462,6 +3502,33 @@ void gpiod_remove_lookup_table(struct gpiod_lookup_table *table) I think I see the same problem right here in regards to pinctrls and gpiohogs that have with DeviceTree: <https://patchwork.kernel.org/patch/10313767/> The problem is that unlike native gpio-controllers, pinctrls need to have a "pin/gpio range" defined before any gpio-hogs can be added. If this is not the case the generic pinctrl_gpio_reguest() [0] will fail with -EPROBE_DEFER at this point. (see the call chain in the "pinctrl: msm: fix gpio-hog related boot issueslogin register" mail starting from gpiod_hog). And now the crux of the matter is that currently in order for pinctrl drivers to register the range they have to call gpiochip_add_pin_range() [1]. But they only can do it after the gpiochip_add_data_with_key() [2], since this function initializes the pin_ranges list [3]. So what will happen is that you'll get an "gpiochip_machine_hog: unable to hog GPIO line $LABEL $GPIONR -517" error for every single gpio-hog and wonder why :(. Regards, Christian [0] <https://elixir.bootlin.com/linux/v4.16.2/source/drivers/pinctrl/core.c#L743> [1] <https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L2078> [2] <https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L1136> [3] <https://elixir.bootlin.com/linux/latest/source/drivers/gpio/gpiolib.c#L1253> -- 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 Tue, Apr 10, 2018 at 10:30 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > Board files constitute a significant part of the users of the legacy > GPIO framework. In many cases they only export a line and set its > desired value. We could use GPIO hogs for that like we do for DT and > ACPI but there's no support for that in machine code. > > This patch proposes to extend the machine.h API with support for > registering hog tables in board files. > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > --- > v1 -> v2: > - kbuild bot complains about enum gpiod_flags having incomplete type > although it builds fine for me locally: change the type of dflags > to int I like the idea and thinking behind this patch, so patch applied. It's a bit of code to carry, so if it doesn't see any use, I will simply revert it :) But I bet you intend to follow up with some machine patches and then it is immediately worth it. 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 Thu, Apr 12, 2018 at 10:00 PM, Christian Lamparter <chunkeey@gmail.com> wrote: > The problem is that unlike native gpio-controllers, pinctrls need > to have a "pin/gpio range" defined before any gpio-hogs can be added. Indeed. But the primary use case (correct me if I am wrong Bartosz) is to clean up old boardfile code. Old boardfiles belong to equally old boards. They very often do not have pin control, just GPIO, and they very often have custom code that just issue gpio_get(), gpio_* etc to set up hogs. So this will be able to replace all such boilerplate with some hog table for each boardfile and be done with it. I.e. they have only drivers/gpio/gpio-foo.c and no pin control driver in 9 cases out of 10. Cases do exist where they use pin control with board files. Those are rare. But they will have problems. Some machine descriptor tables are used on modern archs and the most prominent is x86 Intel. However the Intel pin control driver is one of those that (IIRC) will actually survive this (i.e. it doesn not have this bug). They are not even using DT, they use ACPI. > So what will happen is that you'll get an > "gpiochip_machine_hog: unable to hog GPIO line $LABEL $GPIONR -517" error > for every single gpio-hog and wonder why :(. Hm maybe we can simply improbe the error messages so people realize they have to go and fix their pin control driver(s)? OK maybe a bit whimsical comment from me here... :/ 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
2018-04-26 14:07 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: > On Tue, Apr 10, 2018 at 10:30 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > >> Board files constitute a significant part of the users of the legacy >> GPIO framework. In many cases they only export a line and set its >> desired value. We could use GPIO hogs for that like we do for DT and >> ACPI but there's no support for that in machine code. >> >> This patch proposes to extend the machine.h API with support for >> registering hog tables in board files. >> >> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> >> --- >> v1 -> v2: >> - kbuild bot complains about enum gpiod_flags having incomplete type >> although it builds fine for me locally: change the type of dflags >> to int > > I like the idea and thinking behind this patch, so patch applied. > > It's a bit of code to carry, so if it doesn't see any use, I will simply > revert it :) > > But I bet you intend to follow up with some machine patches > and then it is immediately worth it. Yes, I'll be submitting patches removing the legacy gpio calls for boards in mach-davinci. Thanks, Bartosz -- 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, Apr 26, 2018 at 6:42 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: > 2018-04-26 14:07 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>: >> On Tue, Apr 10, 2018 at 10:30 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote: >> >>> Board files constitute a significant part of the users of the legacy >>> GPIO framework. In many cases they only export a line and set its >>> desired value. We could use GPIO hogs for that like we do for DT and >>> ACPI but there's no support for that in machine code. >>> >>> This patch proposes to extend the machine.h API with support for >>> registering hog tables in board files. >>> >>> Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> >>> --- >>> v1 -> v2: >>> - kbuild bot complains about enum gpiod_flags having incomplete type >>> although it builds fine for me locally: change the type of dflags >>> to int >> >> I like the idea and thinking behind this patch, so patch applied. >> >> It's a bit of code to carry, so if it doesn't see any use, I will simply >> revert it :) >> >> But I bet you intend to follow up with some machine patches >> and then it is immediately worth it. > > Yes, I'll be submitting patches removing the legacy gpio calls for > boards in mach-davinci. Awesome, thanks! (I guess they should all be converted to device tree though, hope that will happen...) 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
diff --git a/Documentation/driver-api/gpio/board.rst b/Documentation/driver-api/gpio/board.rst index 25d62b2e9fd0..2c112553df84 100644 --- a/Documentation/driver-api/gpio/board.rst +++ b/Documentation/driver-api/gpio/board.rst @@ -177,3 +177,19 @@ mapping and is thus transparent to GPIO consumers. A set of functions such as gpiod_set_value() is available to work with the new descriptor-oriented interface. + +Boards using platform data can also hog GPIO lines by defining GPIO hog tables. + +.. code-block:: c + + struct gpiod_hog gpio_hog_table[] = { + GPIO_HOG("gpio.0", 10, "foo", GPIO_ACTIVE_LOW, GPIOD_OUT_HIGH), + { } + }; + +And the table can be added to the board code as follows:: + + gpiod_add_hogs(gpio_hog_table); + +The line will be hogged as soon as the gpiochip is created or - in case the +chip was created earlier - when the hog table is registered. diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 43aeb07343ec..547adc149b62 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -71,6 +71,9 @@ static DEFINE_MUTEX(gpio_lookup_lock); static LIST_HEAD(gpio_lookup_list); LIST_HEAD(gpio_devices); +static DEFINE_MUTEX(gpio_machine_hogs_mutex); +static LIST_HEAD(gpio_machine_hogs); + static void gpiochip_free_hogs(struct gpio_chip *chip); static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, struct lock_class_key *lock_key, @@ -1171,6 +1174,41 @@ static int gpiochip_setup_dev(struct gpio_device *gdev) return status; } +static void gpiochip_machine_hog(struct gpio_chip *chip, struct gpiod_hog *hog) +{ + struct gpio_desc *desc; + int rv; + + desc = gpiochip_get_desc(chip, hog->chip_hwnum); + if (IS_ERR(desc)) { + pr_err("%s: unable to get GPIO desc: %ld\n", + __func__, PTR_ERR(desc)); + return; + } + + if (desc->flags & FLAG_IS_HOGGED) + return; + + rv = gpiod_hog(desc, hog->line_name, hog->lflags, hog->dflags); + if (rv) + pr_err("%s: unable to hog GPIO line (%s:%u): %d\n", + __func__, chip->label, hog->chip_hwnum, rv); +} + +static void machine_gpiochip_add(struct gpio_chip *chip) +{ + struct gpiod_hog *hog; + + mutex_lock(&gpio_machine_hogs_mutex); + + list_for_each_entry(hog, &gpio_machine_hogs, list) { + if (!strcmp(chip->label, hog->chip_label)) + gpiochip_machine_hog(chip, hog); + } + + mutex_unlock(&gpio_machine_hogs_mutex); +} + static void gpiochip_setup_devs(void) { struct gpio_device *gdev; @@ -1326,6 +1364,8 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data, acpi_gpiochip_add(chip); + machine_gpiochip_add(chip); + /* * By first adding the chardev, and then adding the device, * we get a device node entry in sysfs under @@ -3462,6 +3502,33 @@ void gpiod_remove_lookup_table(struct gpiod_lookup_table *table) } EXPORT_SYMBOL_GPL(gpiod_remove_lookup_table); +/** + * gpiod_add_hogs() - register a set of GPIO hogs from machine code + * @hogs: table of gpio hog entries with a zeroed sentinel at the end + */ +void gpiod_add_hogs(struct gpiod_hog *hogs) +{ + struct gpio_chip *chip; + struct gpiod_hog *hog; + + mutex_lock(&gpio_machine_hogs_mutex); + + for (hog = &hogs[0]; hog->chip_label; hog++) { + list_add_tail(&hog->list, &gpio_machine_hogs); + + /* + * The chip may have been registered earlier, so check if it + * exists and, if so, try to hog the line now. + */ + chip = find_chip_by_name(hog->chip_label); + if (chip) + gpiochip_machine_hog(chip, hog); + } + + mutex_unlock(&gpio_machine_hogs_mutex); +} +EXPORT_SYMBOL_GPL(gpiod_add_hogs); + static struct gpiod_lookup_table *gpiod_find_lookup_table(struct device *dev) { const char *dev_id = dev ? dev_name(dev) : NULL; diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h index b2f2dc638463..daa44eac9241 100644 --- a/include/linux/gpio/machine.h +++ b/include/linux/gpio/machine.h @@ -39,6 +39,23 @@ struct gpiod_lookup_table { struct gpiod_lookup table[]; }; +/** + * struct gpiod_hog - GPIO line hog table + * @chip_label: name of the chip the GPIO belongs to + * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO + * @line_name: consumer name for the hogged line + * @lflags: mask of GPIO lookup flags + * @dflags: GPIO flags used to specify the direction and value + */ +struct gpiod_hog { + struct list_head list; + const char *chip_label; + u16 chip_hwnum; + const char *line_name; + enum gpio_lookup_flags lflags; + int dflags; +}; + /* * Simple definition of a single GPIO under a con_id */ @@ -59,10 +76,23 @@ struct gpiod_lookup_table { .flags = _flags, \ } +/* + * Simple definition of a single GPIO hog in an array. + */ +#define GPIO_HOG(_chip_label, _chip_hwnum, _line_name, _lflags, _dflags) \ +{ \ + .chip_label = _chip_label, \ + .chip_hwnum = _chip_hwnum, \ + .line_name = _line_name, \ + .lflags = _lflags, \ + .dflags = _dflags, \ +} + #ifdef CONFIG_GPIOLIB void gpiod_add_lookup_table(struct gpiod_lookup_table *table); void gpiod_add_lookup_tables(struct gpiod_lookup_table **tables, size_t n); void gpiod_remove_lookup_table(struct gpiod_lookup_table *table); +void gpiod_add_hogs(struct gpiod_hog *hogs); #else static inline void gpiod_add_lookup_table(struct gpiod_lookup_table *table) {} @@ -70,6 +100,7 @@ static inline void gpiod_add_lookup_tables(struct gpiod_lookup_table **tables, size_t n) {} static inline void gpiod_remove_lookup_table(struct gpiod_lookup_table *table) {} +static inline void gpiod_add_hogs(struct gpiod_hog *hogs) {} #endif #endif /* __LINUX_GPIO_MACHINE_H */
Board files constitute a significant part of the users of the legacy GPIO framework. In many cases they only export a line and set its desired value. We could use GPIO hogs for that like we do for DT and ACPI but there's no support for that in machine code. This patch proposes to extend the machine.h API with support for registering hog tables in board files. Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> --- v1 -> v2: - kbuild bot complains about enum gpiod_flags having incomplete type although it builds fine for me locally: change the type of dflags to int Documentation/driver-api/gpio/board.rst | 16 ++++++ drivers/gpio/gpiolib.c | 67 +++++++++++++++++++++++++ include/linux/gpio/machine.h | 31 ++++++++++++ 3 files changed, 114 insertions(+)