Alexandre Courbot <gnurou@xxxxxxxxx> wrote on Wed [2014-Dec-10 20:19:51 +0900]: > On Fri, Dec 5, 2014 at 6:02 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 > > when the gpio controller is probed. > > > > The actual DT scanning to collect the GPIO specific data is performed > > as part of the gpiochip_add(). > > > > The purpose of this is to allows specific GPIOs to be configured > > without any driver specific code. > > This particularly useful because board design are getting > > increasingly complex and given SoC pins can now have upward > > of 10 mux values a lot of connections are now dependent on > > external IO muxes to switch various modes and combination. > > > > 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 v2: > > * Refactor the gpio-hog mechanism to split the DT related action > > from the actual "hogging" operation. > > * This allows non-DT providers to implement hogs as well. > > * Added FLAG_IS_HOGGED to mark hogged gpio and make gpiochip removal > > able to release hogged gpio. > > * Similarly to the hogging, the cleanup is performed as part of > > of_gpiochip_remove > > * Refactor the gpio-hog mechanism as private functions meant to > > be to invoked from of_gpiochip_add(). > > > > Changes since v1: > > * Refactor the gpio-hog mechanism as private functions meant to > > be to invoked from of_gpiochip_add(). > > > > drivers/gpio/gpiolib-of.c | 127 ++++++++++++++++++++++++++++++++++++++++++ > > drivers/gpio/gpiolib.c | 118 ++++++++++++++++++++++++++++++--------- > > drivers/gpio/gpiolib.h | 1 + > > include/linux/gpio/consumer.h | 9 +++ > > 4 files changed, 229 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c > > index 604dbe6..e13134d 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,128 @@ 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, > > + }; > > + 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, "direction", &dir_val)) { > > + 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? > > We should probably return an error if the property is not specified - > is there a point in hogging a GPIO without a direction? E.g: > > if (of_property_read_string(np, "direction", &dir_val)) > return ERR_PTR(-EINVAL); > > if (!strcmp(... > > to use the nice pattern that errors (and not normal behavior) are the exception. Bah, I was going for compartmentalization. It make sense if you don't think about it ..... :) > > > + > > + if (name && of_property_read_string(np, "line-name", name)) > > + *name = np->name; > > + > > + desc = gg_data.out_gpio; > > + > > + return desc; > > +} > > + > > +/** > > + * _gpiochip_hog - 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 _gpiochip_hog(struct gpio_chip *chip) > > Rename to of_gpio_scan_hogs() maybe? Given that it is meant for gpiochip_add, how about _gpiochip_scan_hogs()? > > > +{ > > + 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) { > > + if (!of_property_read_bool(np, "gpio-hog")) > > + continue; > > + > > + desc = of_get_gpio_hog(np, &name, &lflags, &dflags); > > + if (IS_ERR(desc)) > > + continue; > > + > > + __gpiod_hog(desc, name, lflags, dflags); > > You are not propagating any error returned by __gpiod_hog here. _gpiochip_hog is a void function given that __gpiod_hog() is the last call of that loop there is nothing to propagate. You would still want to scan all of the child node regardless of errors, no? > > > + } > > +} > > + > > +/** > > + * _gpiochip_unhog - Scan gpio-controller and apply GPIO hog as requested > > + * @chip: gpio chip to act on > > + * > > + * This is only used by of_gpiochip_remove to free hogged gpios > > + * > > + */ > > +static void _gpiochip_unhog(struct gpio_chip *chip) > > +{ > > + int id; > > + > > + for (id = 0; id < chip->ngpio; id++) { > > + if (test_bit(FLAG_IS_HOGGED, &chip->desc[id].flags)) > > + gpiod_put(&chip->desc[id]); > > + } > > +} > > This function is not DT-specific. It should be included in gpiolib.c > and called from there before of_gpiochip_remove(). Agreed, any name request while I am at it or tis this fine as is? > > > + > > +/** > > * 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,10 +425,14 @@ void of_gpiochip_add(struct gpio_chip *chip) > > > > of_gpiochip_add_pin_range(chip); > > of_node_get(chip->of_node); > > + > > + _gpiochip_hog(chip); > > } > > > > void of_gpiochip_remove(struct gpio_chip *chip) > > { > > gpiochip_remove_pin_ranges(chip); > > of_node_put(chip->of_node); > > + > > + _gpiochip_unhog(chip); > > } > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > > index e8e98ca..4ef6eb8 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -849,6 +849,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; > > } > > > > @@ -1631,6 +1632,58 @@ struct gpio_desc *__must_check __gpiod_get_optional(struct device *dev, > > } > > EXPORT_SYMBOL_GPL(__gpiod_get_optional); > > > > + > > +/** > > + * __gpiod_get_helper - helper function to request and configure a given GPIO > > + * @desc: gpio whose value will be assigned > > + * @con_id: unction 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_get_helper(struct gpio_desc *desc, const char *con_id, > > + unsigned long lflags, enum gpiod_flags dflags) > > +{ > > + int status; > > + > > + status = gpiod_request(desc, con_id); > > As I mentioned in the previous revision, this will prevent the module > from being unloaded with hogged GPIOs. You need to use > gpiochip_request_own_desc() here and gpiochip_free_own_desc() instead > of gpiod_put() to free hogged GPIOs. Therefore the call to > gpiod_request/gpiochip_request_own_gpio should be taken out of this > (very nice otherwise!) helper. I can split the functionality out but I do not understand why in this case using gpiod_request would prevent module from being unloaded? Isn't gpiochip_remove() part of a gpio module unload sequence? Because then the _gpiochip_unhog() would release these descriptors. Am I missing something? Also would using gpiochip_request_own_desc() basically allow the very same hogged GPIO to be requested later on by a consumer. I thought we did not want that to be possible? > > > + > > + if (status < 0) > > + return 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); > > + > > + if (status < 0) { > > + pr_debug("setup of GPIO %s failed\n", con_id); > > + gpiod_put(desc); > > + return status; > > + } > > + > > + return 0; > > +} > > + > > + > > /** > > * gpiod_get_index - obtain a GPIO from a multi-index GPIO function > > * @dev: GPIO consumer, can be NULL for system-global GPIOs > > @@ -1679,35 +1732,10 @@ struct gpio_desc *__must_check __gpiod_get_index(struct device *dev, > > return desc; > > } > > > > - status = gpiod_request(desc, con_id); > > - > > + status = __gpiod_get_helper(desc, con_id, lookupflags, flags); > > 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); > > - > > - if (status < 0) { > > - dev_dbg(dev, "setup of GPIO %s failed\n", con_id); > > - gpiod_put(desc); > > - return ERR_PTR(status); > > - } > > - > > return desc; > > } > > EXPORT_SYMBOL_GPL(__gpiod_get_index); > > @@ -1742,6 +1770,44 @@ 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) > > +{ > > + int status; > > + > > + /* No particular flag request, not really hogging then... */ > > + if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) { > > + pr_warn("%s: GPIO:%d (%s): no hogging direction specified, bailing out\n", > > + __func__, desc_to_gpio(desc), name); > > + return -EINVAL; > > + } > > Better to catch that error earlier as I suggested IMHO. Agreed. > > > + > > + status = __gpiod_get_helper(desc, name, lflags, dflags); > > + if (status < 0) > > + return status; > > + > > + /* Mark GPIO as hogged so it can be identified and removed later */ > > + set_bit(FLAG_IS_HOGGED, &desc->flags); > > + > > + pr_debug("%s: GPIO:%d (%s) as %s%s\n", __func__, > > + desc_to_gpio(desc), name, > > + (dflags&GPIOD_FLAGS_BIT_DIR_OUT) ? "output" : "input", > > + (dflags&GPIOD_FLAGS_BIT_DIR_OUT) ? > > + (dflags&GPIOD_FLAGS_BIT_DIR_VAL) ? "/high" : "/low":""); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(__gpiod_hog); > > No export needed for this gpiolib-private function. Agreed. > > > + > > +/** > > * gpiod_put - dispose of a GPIO descriptor > > * @desc: GPIO descriptor to dispose of > > * > > diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h > > index 9db2b6a..e3c8a1f 100644 > > --- a/drivers/gpio/gpiolib.h > > +++ b/drivers/gpio/gpiolib.h > > @@ -76,6 +76,7 @@ struct gpio_desc { > > #define FLAG_OPEN_DRAIN 7 /* Gpio is open drain type */ > > #define FLAG_OPEN_SOURCE 8 /* Gpio is open source type */ > > #define FLAG_USED_AS_IRQ 9 /* GPIO is connected to an IRQ */ > > +#define FLAG_IS_HOGGED 10 /* GPIO is hogged */ > > > > #define ID_SHIFT 16 /* add new flags before this one */ > > > > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h > > index 12f146f..626f93d 100644 > > --- a/include/linux/gpio/consumer.h > > +++ b/include/linux/gpio/consumer.h > > @@ -64,6 +64,8 @@ struct gpio_desc *__must_check __devm_gpiod_get_optional(struct device *dev, > > struct gpio_desc *__must_check > > __devm_gpiod_get_index_optional(struct device *dev, const char *con_id, > > unsigned int index, enum gpiod_flags flags); > > +int __gpiod_hog(struct gpio_desc *desc, const char *name, > > + unsigned long lflags, enum gpiod_flags dflags); > > Errr nope. __gpiod_hog is a gpiolib-private function, so it should be > in drivers/gpio/gpiolib.h. Agreed. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html