Date:	Wed, 10 Dec 2014 17:48:17 -0600
From:	Benoit Parrot <bparrot@...com>
To:	Alexandre Courbot <gnurou@...il.com>
CC:	Linus Walleij <linus.walleij@...aro.org>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Jiri Prchal <jiri.prchal@...ignal.cz>,
	Pantelis Antoniou <panto@...oniou-consulting.com>
Subject: Re: [Patch v3 1/2] gpio: add GPIO hogging mechanism

Alexandre Courbot <gnurou@...il.com> wrote on Wed [2014-Dec-10 20:19:51 +0900]:
> On Fri, Dec 5, 2014 at 6:02 AM, Benoit Parrot <bparrot@...com> 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@...com>
> > ---
> > 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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists