Message-ID: <CAAVeFu+VZiuzobB7McSAUehKUHJjBv1o8ZpYjSHJAKjr3o5wTA@mail.gmail.com>
Date:	Fri, 28 Nov 2014 16:30:01 +0900
From:	Alexandre Courbot <gnurou@...il.com>
To:	Benoit Parrot <bparrot@...com>,
	Pantelis Antoniou <panto@...oniou-consulting.com>,
	Jiri Prchal <jiri.prchal@...ignal.cz>,
	Maxime Ripard <maxime.ripard@...e-electrons.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>
Subject: Re: [Patch v2 1/2] gpio: add GPIO hogging mechanism

On Fri, Nov 21, 2014 at 8:54 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 v1:
>  * Refactor the gpio-hog mechanism as private functions meant to
>    be to invoked from of_gpiochip_add().
>
>  drivers/gpio/gpiolib-of.c | 188 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 188 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 604dbe6..3caed76 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,184 @@ 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;
> +       struct gg_data gg_data = {
> +               .flags = &xlate_flags,
> +               .out_gpio = NULL,
> +       };
> +       u32 tmp;
> +       int i, in, outlo, outhi;
> +       int ret;
> +
> +       if (!np)
> +               return ERR_PTR(-EINVAL);

This function is being called from a perfectly mastered context in
this file, so maybe we can avoid this check.

> +
> +       chip_np = np->parent;
> +       if (!chip_np) {
> +               desc = ERR_PTR(-EINVAL);
> +               goto out;
> +       }
> +
> +       if (!lflags || !dflags) {
> +               desc = ERR_PTR(-EINVAL);
> +               goto out;
> +       }

Same for this one.

> +
> +       *lflags = 0;
> +       *dflags = 0;
> +       in = 0;
> +       outlo = 0;
> +       outhi = 0;
> +
> +       ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
> +       if (ret) {
> +               desc = ERR_PTR(ret);
> +               goto out;

Please use "return ERR_PTR(ret);" directly, since you do absolutely no
cleanup in out:. Same remark everywhere it applies.

> +       }
> +
> +       if (tmp > MAX_PHANDLE_ARGS) {
> +               desc = ERR_PTR(-EINVAL);
> +               goto out;
> +       }
> +
> +       gg_data.gpiospec.args_count = tmp;
> +       gg_data.gpiospec.np = chip_np;
> +       for (i = 0; i < tmp; i++) {
> +               ret = of_property_read_u32(np, "gpios",
> +                                          &gg_data.gpiospec.args[i]);
> +               if (ret) {
> +                       desc = ERR_PTR(ret);
> +                       goto out;
> +               }
> +       }
> +
> +       gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);

This seems to work but only supports one GPIO per hog node. It would
be nice to be able to specify several GPIOs to which the same settings
need to be applied.

> +       if (!gg_data.out_gpio) {
> +               if (np->parent == np)
> +                       desc = ERR_PTR(-ENXIO);
> +               else
> +                       desc = ERR_PTR(-EPROBE_DEFER);
> +               goto out;
> +       }
> +
> +       if (xlate_flags & OF_GPIO_ACTIVE_LOW)
> +               *lflags |= GPIOF_ACTIVE_LOW;
> +
> +       if (of_property_read_bool(np, "input")) {
> +               *dflags |= GPIOD_IN;
> +               in = 1;
> +       }
> +       if (of_property_read_bool(np, "output-low")) {
> +               *dflags |= GPIOD_OUT_LOW;
> +               outlo = 1;
> +       }
> +       if (of_property_read_bool(np, "output-high")) {
> +               *dflags |= GPIOD_OUT_HIGH;
> +               outhi = 1;
> +       }

I thought we agreed that this should be a "direction =
input|output-low|output-high" property?

> +       if ((in + outlo + outhi) > 1) {
> +               pr_warn("%s: GPIO %s: more than one direction/value selected, assuming: %s.\n",
> +                        __func__, np->name,
> +                        (outhi)?"output-high":(outlo)?"output-low":"input");
> +       }
> +
> +       if (of_property_read_bool(np, "open-drain-line"))
> +               *lflags |= GPIO_OPEN_DRAIN;
> +       if (of_property_read_bool(np, "open-source-line"))
> +               *lflags |= GPIO_OPEN_SOURCE;

These properties are not documented in your DT bindings.

> +       if (name && of_property_read_string(np, "line-name", name))
> +               *name = np->name;
> +
> +       desc = gg_data.out_gpio;
> +
> +out:
> +       return desc;
> +}
> +
> +
> +/**
> + * do_gpio_hog - Given node is a GPIO hog configuration, handle it
> + * @np:                device node to get GPIO from
> + *
> + * This is only used by of_gpiochip_add to request/set GPIO initial
> + * configuration.
> + */
> +static int do_gpio_hog(struct device_node *np)
> +{
> +       struct gpio_desc *desc = NULL;
> +       int err;
> +       const char *name;
> +       enum gpio_lookup_flags lflags;
> +       enum gpiod_flags dflags;
> +
> +       desc = of_get_gpio_hog(np, &name, &lflags, &dflags);
> +       if (!desc)
> +               return -ENOTSUPP;
> +       else if (IS_ERR(desc))
> +               return PTR_ERR(desc);
> +
> +       err = gpiod_request(desc, name);

Using this function means that a GPIO chip module cannot be unloaded
if it uses GPIO hogs. Is it the intended behavior? If not, please use
gpiochip_request_own_desc() instead, and make sure to call
gpiochip_free_own_desc() for each hog when the driver is unloaded.

> +       if (err < 0)
> +               return err;
> +
> +       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, not really hogging then... */
> +       if (!(dflags & GPIOD_FLAGS_BIT_DIR_SET)) {
> +               pr_warn("%s: GPIO %s: no hogging direction specified, bailing out\n",
> +                        __func__, name);
> +               err = -EINVAL;
> +               goto free_gpio;
> +       }
> +
> +       /* Process flags */
> +       if (dflags & GPIOD_FLAGS_BIT_DIR_OUT)
> +               err = gpiod_direction_output(desc,
> +                                            dflags & GPIOD_FLAGS_BIT_DIR_VAL);
> +       else
> +               err = gpiod_direction_input(desc);
> +
> +       if (err < 0) {
> +               pr_warn("%s: GPIO %s setting direction/value failed\n",
> +                        __func__, name);
> +               goto free_gpio;
> +       }

I would suggest to factorize this code that is similar to the one
found in __gpiod_get_index(). Do all the DT parsing in a function that
just returns a descriptor and the

> +
> +       pr_debug("%s: gpio-hog: 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;
> +
> +free_gpio:
> +       gpiod_put(desc);
> +       return err;
> +}

I think most of this function should be moved into gpiolib.c. Add a
function there that takes the descriptor, lflags and dflags and that
sets the GPIO up, possibly adding it to a list of hogs so we can unset
the hogs when unloading the module. That way we have a hogging
mechanism that is not dependent on device tree and can be used by
other GPIO binders, such as ACPI.

There is still some work needed but it looks much better than the
first version. Please make sure to also keep Maxime, Pantelis and Jiri
in the loop since they expressed interest in your first version.
--
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