Alexandre Courbot <gnurou@xxxxxxxxx> wrote on Wed [2014-Dec-17 17:41:51 +0900]: > Looks ok to me. I have a few nits below, but otherwise I am happy with > how it turned out: > > Reviewed-by: Alexandre Courbot <acourbot@xxxxxxxxxx> I'll add this to the next version. > > Thanks for your patience with this! > > On Sat, Dec 13, 2014 at 7:07 AM, Benoit Parrot <bparrot@xxxxxx> wrote: > > Based on Boris Brezillion's work this is a reworked patch > > of his initial GPIO hogging mechanism. > > This patch provides a way to initally configure specific GPIO > > s/initally/initially > > > when the gpio controller is probed. > > GPIO in capitals please. > > > > > The actual DT scanning to collect the GPIO specific data is performed > > as part of the gpiochip_add(). > > d/the > > > > > The purpose of this is to allows specific GPIOs to be configured > > s/allows/allow > > > without any driver specific code. > > This particularly useful because board design are getting > > This is particularly > > > increasingly complex and given SoC pins can now have upward > > of > > s/upward of/up to? well i meant more than 10 I think in some cases we have 13. > > > 10 mux values a lot of connections are now dependent on > > seems like a comma is missing here. > > > external IO muxes to switch various modes and combination. > > not sure what a combination is in that context. > > > > > Specific drivers should not necessarily need to be aware of > > what accounts to a specific board implementation. This board level > > "description" should be best kept as part of the dts file. > > > > Signed-off-by: Benoit Parrot <bparrot@xxxxxx> > > --- > > Changes since v3: > > * Relocated the non-DT "hog" function to gpiolib.c. > > * Rename some of the function to be clearer and remove _ prefixes. > > * Replace the gpiod_request/gpiod_put usage with > > gpiochip_request_own_desc/free_own_desc version instead. > > * Refactor some of the logic to better handle error condition/reporting > > * Renamed the "direction" DT properties to "state". > > > > drivers/gpio/gpiolib-of.c | 119 ++++++++++++++++++++++++++++++++++++++++++ > > drivers/gpio/gpiolib.c | 128 ++++++++++++++++++++++++++++++++++++++-------- > > drivers/gpio/gpiolib.h | 3 ++ > > 3 files changed, 230 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > > index 604dbe6..5422216 100644 > > --- a/drivers/gpio/gpiolib-of.c > > +++ b/drivers/gpio/gpiolib-of.c > > @@ -22,6 +22,7 @@ > > #include <linux/of_gpio.h> > > #include <linux/pinctrl/pinctrl.h> > > #include <linux/slab.h> > > +#include <linux/gpio/machine.h> > > > > #include "gpiolib.h" > > > > @@ -111,6 +112,122 @@ int of_get_named_gpio_flags(struct device_node *np, const char *list_name, > > EXPORT_SYMBOL(of_get_named_gpio_flags); > > > > /** > > + * of_get_gpio_hog() - Get a GPIO hog descriptor, names and flags for GPIO API > > + * @np: device node to get GPIO from > > + * @name: GPIO line name > > + * @flags: a flags pointer to fill in > > + * > > + * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno > > + * value on the error condition. > > + */ > > + > > +static struct gpio_desc *of_get_gpio_hog(struct device_node *np, > > + const char **name, > > + enum gpio_lookup_flags *lflags, > > + enum gpiod_flags *dflags) > > +{ > > + struct device_node *chip_np; > > + enum of_gpio_flags xlate_flags; > > + struct gpio_desc *desc; > > + const char *dir_val; > > + struct gg_data gg_data = { > > + .flags = &xlate_flags, > > + .out_gpio = NULL, > > out_gpio will be set to NULL if you don't specify it, so you don't > need this last line. ok > > > + }; > > + u32 tmp; > > + int i, ret; > > + > > + chip_np = np->parent; > > + if (!chip_np) > > + return ERR_PTR(-EINVAL); > > + > > + xlate_flags = 0; > > + *lflags = 0; > > + *dflags = 0; > > + > > + ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + if (tmp > MAX_PHANDLE_ARGS) > > + return ERR_PTR(-EINVAL); > > + > > + gg_data.gpiospec.args_count = tmp; > > + gg_data.gpiospec.np = chip_np; > > + for (i = 0; i < tmp; i++) { > > + ret = of_property_read_u32_index(np, "gpios", i, > > + &gg_data.gpiospec.args[i]); > > + if (ret) > > + return ERR_PTR(ret); > > + } > > + > > + gpiochip_find(&gg_data, of_gpiochip_find_and_xlate); > > + if (!gg_data.out_gpio) { > > + if (np->parent == np) > > + return ERR_PTR(-ENXIO); > > + else > > + return ERR_PTR(-EPROBE_DEFER); > > + } > > + > > + if (xlate_flags & OF_GPIO_ACTIVE_LOW) > > + *lflags |= GPIO_ACTIVE_LOW; > > + > > + if (of_property_read_string(np, "state", &dir_val)) { > > + pr_warn("%s: GPIO:%d (%s): no hogging state specified, bailing out\n", > > + __func__, desc_to_gpio(gg_data.out_gpio), np->name); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + if (!strcmp(dir_val, "input")) > > + *dflags |= GPIOD_IN; > > + else if (!strcmp(dir_val, "output-low")) > > + *dflags |= GPIOD_OUT_LOW; > > + else if (!strcmp(dir_val, "output-high")) > > + *dflags |= GPIOD_OUT_HIGH; > > + else { > > + pr_warn("%s: GPIO:%d (%s): invalid state '%s' requested, bailing out\n", > > + __func__, desc_to_gpio(gg_data.out_gpio), > > + np->name, dir_val); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + if (name && of_property_read_string(np, "line-name", name)) > > + *name = np->name; > > + > > + desc = gg_data.out_gpio; > > + > > + return desc; > > +} > > + > > +/** > > + * of_gpiochip_scan_hogs - Scan gpio-controller and apply GPIO hog as requested > > + * @chip: gpio chip to act on > > + * > > + * This is only used by of_gpiochip_add to request/set GPIO initial > > + * configuration. > > + */ > > +static void of_gpiochip_scan_hogs(struct gpio_chip *chip) > > +{ > > + struct gpio_desc *desc = NULL; > > + struct device_node *np; > > + const char *name; > > + enum gpio_lookup_flags lflags; > > + enum gpiod_flags dflags; > > + > > + for_each_child_of_node(chip->dev->of_node, np) { As I was backporting this to 3.14 for my own purpose. I encountered a NULL pointer dereference here. In 3.14 chip->dev (which is optional) is NULL at least on my platform. I had to change the above line to: for_each_child_of_node(chip->of_node, np) { Would you recommend the same change here? > > + if (!of_property_read_bool(np, "gpio-hog")) > > + continue; > > + > > + desc = of_get_gpio_hog(np, &name, &lflags, &dflags); > > + if (IS_ERR(desc)) > > + continue; > > + > > + if (gpiod_hog(desc, name, lflags, dflags)) > > + continue; > > + } > > +} > > + > > +/** > > * of_gpio_simple_xlate - translate gpio_spec to the GPIO number and flags > > * @gc: pointer to the gpio_chip structure > > * @np: device node of the GPIO chip > > @@ -302,6 +419,8 @@ void of_gpiochip_add(struct gpio_chip *chip) > > > > of_gpiochip_add_pin_range(chip); > > of_node_get(chip->of_node); > > + > > + of_gpiochip_scan_hogs(chip); > > } > > > > void of_gpiochip_remove(struct gpio_chip *chip) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index 58659db..5527dbb 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -301,6 +301,7 @@ EXPORT_SYMBOL_GPL(gpiochip_add); > > > > /* Forward-declaration */ > > static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip); > > +static void gpiochip_free_hogs(struct gpio_chip *chip); > > > > /** > > * gpiochip_remove() - unregister a gpio_chip > > @@ -319,6 +320,7 @@ void gpiochip_remove(struct gpio_chip *chip) > > > > gpiochip_irqchip_remove(chip); > > gpiochip_remove_pin_ranges(chip); > > + gpiochip_free_hogs(chip); > > of_gpiochip_remove(chip); > > > > for (id = 0; id < chip->ngpio; id++) { > > @@ -849,6 +851,7 @@ static bool __gpiod_free(struct gpio_desc *desc) > > clear_bit(FLAG_REQUESTED, &desc->flags); > > clear_bit(FLAG_OPEN_DRAIN, &desc->flags); > > clear_bit(FLAG_OPEN_SOURCE, &desc->flags); > > + clear_bit(FLAG_IS_HOGGED, &desc->flags); > > ret = true; > > } > > > > @@ -1653,6 +1656,47 @@ struct gpio_desc *__must_check __gpiod_get_optional(struct device *dev, > > } > > EXPORT_SYMBOL_GPL(__gpiod_get_optional); > > > > + > > +/** > > + * __gpiod_set_helper - helper function to configure a given GPIO > > + * @desc: gpio whose value will be assigned > > + * @con_id: function within the GPIO consumer > > + * @lflags: gpio_lookup_flags - returned from of_find_gpio() or > > + * of_get_gpio_hog() > > + * @dflags: gpiod_flags - optional GPIO initialization flags > > + * > > + * Return 0 on success, -ENOENT if no GPIO has been assigned to the > > + * requested function and/or index, or another IS_ERR() code if an error > > + * occurred while trying to acquire the GPIO. > > + */ > > +static int __gpiod_set_helper(struct gpio_desc *desc, const char *con_id, > > + unsigned long lflags, enum gpiod_flags dflags) > > +{ > > + int status; > > + > > + if (lflags & GPIO_ACTIVE_LOW) > > + set_bit(FLAG_ACTIVE_LOW, &desc->flags); > > + if (lflags & GPIO_OPEN_DRAIN) > > + set_bit(FLAG_OPEN_DRAIN, &desc->flags); > > + if (lflags & GPIO_OPEN_SOURCE) > > + set_bit(FLAG_OPEN_SOURCE, &desc->flags); > > + > > + /* No particular flag request, return here... */ > > + if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) { > > + pr_debug("no flags found for %s\n", con_id); > > + return 0; > > + } > > + > > + /* Process flags */ > > + if (dflags & GPIOD_FLAGS_BIT_DIR_OUT) > > + status = gpiod_direction_output(desc, > > + dflags & GPIOD_FLAGS_BIT_DIR_VAL); > > + else > > + status = gpiod_direction_input(desc); > > + > > + return status; > > +} > > + > > /** > > * gpiod_get_index - obtain a GPIO from a multi-index GPIO function > > * @dev: GPIO consumer, can be NULL for system-global GPIOs > > @@ -1702,30 +1746,12 @@ struct gpio_desc *__must_check __gpiod_get_index(struct device *dev, > > } > > > > status = gpiod_request(desc, con_id); > > - > > if (status < 0) > > return ERR_PTR(status); > > > > - if (lookupflags & GPIO_ACTIVE_LOW) > > - set_bit(FLAG_ACTIVE_LOW, &desc->flags); > > - if (lookupflags & GPIO_OPEN_DRAIN) > > - set_bit(FLAG_OPEN_DRAIN, &desc->flags); > > - if (lookupflags & GPIO_OPEN_SOURCE) > > - set_bit(FLAG_OPEN_SOURCE, &desc->flags); > > - > > - /* No particular flag request, return here... */ > > - if (!(flags & GPIOD_FLAGS_BIT_DIR_SET)) > > - return desc; > > - > > - /* Process flags */ > > - if (flags & GPIOD_FLAGS_BIT_DIR_OUT) > > - status = gpiod_direction_output(desc, > > - flags & GPIOD_FLAGS_BIT_DIR_VAL); > > - else > > - status = gpiod_direction_input(desc); > > - > > + status = __gpiod_set_helper(desc, con_id, lookupflags, flags); > > if (status < 0) { > > - dev_dbg(dev, "setup of GPIO %s failed\n", con_id); > > + pr_debug("setup of GPIO %s failed\n", con_id); > > gpiod_put(desc); > > return ERR_PTR(status); > > } > > @@ -1819,6 +1845,68 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev, > > EXPORT_SYMBOL_GPL(__gpiod_get_index_optional); > > > > /** > > + * gpiod_hog - Hog the specified GPIO desc given the provided flags > > + * @desc: gpio whose value will be assigned > > + * @name: gpio line name > > + * @lflags: gpio_lookup_flags - returned from of_find_gpio() or > > + * of_get_gpio_hog() > > + * @dflags: gpiod_flags - optional GPIO initialization flags > > + * > > + */ > > +int gpiod_hog(struct gpio_desc *desc, const char *name, > > + unsigned long lflags, enum gpiod_flags dflags) > > +{ > > + struct gpio_chip *chip; > > + struct gpio_desc *local_desc; > > + int hwnum; > > + int status; > > + > > + chip = gpiod_to_chip(desc); > > + hwnum = gpio_chip_hwgpio(desc); > > + > > + local_desc = gpiochip_request_own_desc(chip, hwnum, name); > > This looks somehow funny because local_desc will be strictly equal to > desc, but there is nothing you can do here. The DT GPIO lookup process > should not return a desc, but a (gpiochip*, hwnum) tuple to begin > with, so let's keep it that way. I made a note to myself to fix this. Yeah it does, I was almost tempted to add a check to make sure local_desc would be identical to desc. :) -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html