Alexandre,
Thanks for the feedback.
Alexandre Courbot <gnurou@xxxxxxxxx> wrote on Wed [2014-Oct-29 16:09:59 +0900]:
> Sorry for the delay in reviewing. Adding Jiri and Pantelis who might
> want to extend over this patch and thus will likely have interesting
> comments to make.
>
> On Wed, Oct 22, 2014 at 5:09 AM, Benoit Parrot <bparrot@xxxxxx> wrote:
> > Based on Boris Brezillion 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 probe.
>
> Typo nit: s/probe/probed
Will fix.
>
> >
> > 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 usueful because board design are getting
>
> s/suseful/useful
>
> > increassingly complex and given SoC pins can now have upward
>
> s/increassingly/increasingly
Will fix.
>
> > 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>
> > ---
> > Documentation/devicetree/bindings/gpio/gpio.txt | 33 +++++++++
> > drivers/gpio/gpiolib-of.c | 99 +++++++++++++++++++++++++
> > drivers/gpio/gpiolib.c | 81 ++++++++++++++++++++
> > include/linux/of_gpio.h | 11 +++
> > 4 files changed, 224 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> > index 3fb8f53..a9700d8 100644
> > --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> > +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
>
> Note: changes to DT bindings documentation should generally come as a
> separate patch.
Ok.
>
> > @@ -103,6 +103,26 @@ Every GPIO controller node must contain both an empty "gpio-controller"
> > property, and a #gpio-cells integer property, which indicates the number of
> > cells in a gpio-specifier.
> >
> > +It might contain GPIO hog definitions. GPIO hogging is a mechanism providing
> > +automatic GPIO request and configuration as part of the gpio-controller's
> > +driver probe function.
> > +
> > +Each GPIO hog definition is represented as a child node of the GPIO controller.
> > +Required properties:
> > +- gpios: store the gpio information (id, flags, ...). Shall contain the
> > + number of cells specified in its parent node (GPIO controller node).
>
> If would be nice to be able to specify several GPIO references to that
> they can be all set to the same configuration in one row instead of
> requiring one node each.
>
> > +- input: a property specifying to set the GPIO direction as input.
> > +- output-high: a property specifying to set the GPIO direction to output with
> > + the value high.
> > +- output-low: a property specifying to set the GPIO direction to output with
> > + the value low.
>
> Wouldn't a "direction" property taking one of the values "input",
> "output-low" or "output-high" be easier to parse and generally
> clearer?
I guess here you mean something like this:
...
line_y: line_y {
gpios = <15 0>;
direction = <output-low>;
};
Not sure about easier to parse, but it would be clearer.
>
> > +
> > +Optional properties:
> > +- line-name: the GPIO label name. If not present the node name is used.
>
> I guess we also could use this for naming the GPIO during its export
> if we decide to allow this in a near-future patch.
>
Right.
> > +
> > +A GPIO controller can request GPIO hogs using the "gpio-hogs" property, which
> > +encodes a list of requested GPIO hogs.
> > +
> > Example of two SOC GPIO banks defined as gpio-controller nodes:
> >
> > qe_pio_a: gpio-controller@1400 {
> > @@ -110,6 +130,19 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
> > reg = <0x1400 0x18>;
> > gpio-controller;
> > #gpio-cells = <2>;
> > + gpio-hogs = <&line_b>;
> > +
> > + /* line_a hog is defined but not enabled in this example*/
> > + line_a: line_a {
> > + gpios = <5 0>;
> > + input;
> > + };
> > +
> > + line_b: line_b {
> > + gpios = <6 0>;
> > + output-low;
> > + line-name = "foo-bar-gpio";
> > + };
>
> I think it might be good to group the hog nodes such as to allow
> complex configurations to be set in one go, à la pinmux:
See below.
>
> gpio-controller;
> #gpio-cells = <2>;
> + gpio-hogs = <&line_b>;
> +
> + line_a: line_a {
> + line_a {
> + gpios = <5 0>;
> + input;
> + };
> + line_c {
> + gpios = <7 0>;
> + inputl
> + };
> + };
> +
> + line_b: line_b {
> + line_b {
> + gpios = <6 0>;
> + output-low;
> + line-name = "foo-bar-gpio";
> + };
> + };
>
> Here if you want to enable the first configuration you don't need to
> specify all the lines one by one, you just need to select the right
> group.
>
> I wonder if such usage of child nodes could not interfere with GPIO
> drivers (existing or to be) that need to use child nodes for other
> purposes. Right now there is no way to distinguish a hog node from a
> node that serves another purpose, and that might become a problem in
> the future. Not sure how that should be addressed though - maybe by
> enforcing a "-hog" suffix for the group nodes, e.g. line_a-hog?
> Comments from people more experienced with DT would be nice.
I did consider a "pinmux" flavored format (not sure how hard to parse it would be).
It would allow grouping if nothing else.
/* Line syntax: line_name <gpio# flags> direction-value [export] */
gpio-hogs = <&group_y>;
group_y: group_y {
gpio-hogs-group = <
line_x <15 0> output-low
line_y <16 0> output-high export
line_z <17 0> input
>;
};
>
> > };
> >
> > qe_pio_e: gpio-controller@1460 {
> > diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> > index 604dbe6..7308dfc 100644
> > --- a/drivers/gpio/gpiolib-of.c
> > +++ b/drivers/gpio/gpiolib-of.c
> > @@ -111,6 +111,105 @@ 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
> > + * @index: index of the GPIO
> > + * @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.
> > + */
> > +struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
> > + const char **name, unsigned long *flags)
> > +{
> > + struct device_node *cfg_np, *chip_np;
> > + enum of_gpio_flags xlate_flags;
> > + unsigned long req_flags = 0;
> > + struct gpio_desc *desc;
> > + struct gg_data gg_data = {
> > + .flags = &xlate_flags,
> > + .out_gpio = NULL,
> > + };
> > + u32 tmp;
> > + int i;
> > + int ret;
> > +
> > + cfg_np = of_parse_phandle(np, "gpio-hogs", index);
> > + if (!cfg_np)
> > + return ERR_PTR(-EINVAL);
> > +
> > + chip_np = cfg_np->parent;
> > + if (!chip_np) {
> > + desc = ERR_PTR(-EINVAL);
> > + goto out;
> > + }
> > +
> > + ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
> > + if (ret) {
> > + desc = ERR_PTR(ret);
> > + goto out;
> > + }
> > +
> > + 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(cfg_np, "gpios",
> > + &gg_data.gpiospec.args[i]);
> > + if (ret) {
> > + desc = ERR_PTR(ret);
> > + goto out;
> > + }
> > + }
> > +
> > + gpiochip_find(&gg_data, of_gpiochip_find_and_xlate);
> > + if (!gg_data.out_gpio) {
> > + if (cfg_np->parent == np)
> > + desc = ERR_PTR(-ENXIO);
> > + else
> > + desc = ERR_PTR(-EPROBE_DEFER);
> > +
> > + goto out;
> > + }
> > +
> > + if (xlate_flags & OF_GPIO_ACTIVE_LOW)
> > + req_flags |= GPIOF_ACTIVE_LOW;
> > +
> > + if (of_property_read_bool(cfg_np, "input")) {
> > + req_flags |= GPIOF_DIR_IN;
> > + } else if (of_property_read_bool(cfg_np, "output-high")) {
> > + req_flags |= GPIOF_INIT_HIGH;
> > + } else if (!of_property_read_bool(cfg_np, "output-low")) {
> > + desc = ERR_PTR(-EINVAL);
> > + goto out;
> > + }
>
> That's why I would prefer a "direction" property instead of these 3
> ones: because if you have the following node:
>
> line_foo {
> gpios = <1 GPIO_ACTIVE_HIGH>;
> input;
> output-high;
> };
>
> Then this code will not trigger an error and will just configure the
> GPIO as input.
Understood.
>
> > +
> > + if (of_property_read_bool(cfg_np, "open-drain-line"))
> > + req_flags |= GPIOF_OPEN_DRAIN;
> > +
> > + if (of_property_read_bool(cfg_np, "open-source-line"))
> > + req_flags |= GPIOF_OPEN_DRAIN;
> > +
> > + if (name && of_property_read_string(cfg_np, "line-name", name))
> > + *name = cfg_np->name;
> > +
> > + if (flags)
> > + *flags = req_flags;
> > +
> > + desc = gg_data.out_gpio;
> > +
> > +out:
> > + of_node_put(cfg_np);
> > +
> > + return desc;
> > +}
> > +
> > +/**
> > * 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
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index e8e98ca..b6f5bdb 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -55,6 +55,9 @@ static DEFINE_MUTEX(gpio_lookup_lock);
> > static LIST_HEAD(gpio_lookup_list);
> > LIST_HEAD(gpio_chips);
> >
> > +struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
> > + unsigned int idx);
> > +
> > static inline void desc_set_label(struct gpio_desc *d, const char *label)
> > {
> > d->label = label;
> > @@ -226,6 +229,8 @@ int gpiochip_add(struct gpio_chip *chip)
> > int status = 0;
> > unsigned id;
> > int base = chip->base;
> > + struct gpio_desc *desc;
> > + int i;
> >
> > if ((!gpio_is_valid(base) || !gpio_is_valid(base + chip->ngpio - 1))
> > && base >= 0) {
> > @@ -275,6 +280,16 @@ int gpiochip_add(struct gpio_chip *chip)
> > of_gpiochip_add(chip);
> > acpi_gpiochip_add(chip);
> >
> > + /*
> > + * Instantiate GPIO hogs
> > + * There shouldn't be mores hogs then gpio available on this chip
>
> s/then/than
Will fix.
>
> > + */
> > + for (i = 0; i < chip->ngpio; i++) {
> > + desc = gpiod_get_hog_index(chip->dev, i);
> > + if (IS_ERR(desc))
> > + break;
> > + }
>
> chip->ngpio? Isn't there a better way to know the number of phandles
> in your gpio-hogs property?
Maybe there is. I'll look into it.
>
> Also you may have an error returned either because you reached the end
> of the list, or because the hog configuration itself failed. In the
> latter case don't you want to continue to go through the list?
Understood.
>
> Finally your desc variable should be declared within this loop since
> it is not used elsewhere.
>
Understood.
> > +
> > if (status)
> > goto fail;
> >
> > @@ -1742,6 +1757,72 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
> > EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
> >
> > /**
> > + * gpiod_get_hog_index - obtain a GPIO from a multi-index GPIO hog
> > + * @dev: GPIO consumer
> > + * @idx: index of the GPIO to obtain
> > + *
> > + * This should only be used by the gpiochip_add to request/set GPIO initial
> > + * configuration.
> > + *
> > + * Return a valid GPIO descriptor, or an IS_ERR() condition in case of error.
> > + */
> > +struct gpio_desc *__must_check gpiod_get_hog_index(struct device *dev,
> > + unsigned int idx)
> > +{
> > + struct gpio_desc *desc = NULL;
> > + int err;
> > + unsigned long flags;
> > + const char *name;
> > +
> > + /* Using device tree? */
> > + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
> > + desc = of_get_gpio_hog(dev->of_node, idx, &name, &flags);
> > +
> > + if (!desc)
> > + return ERR_PTR(-ENOTSUPP);
> > + else if (IS_ERR(desc))
> > + return desc;
> > +
> > + dev_dbg(dev, "gpio-hog: GPIO:%d (%s) as %s%s\n",
> > + desc_to_gpio(desc), name, (flags&GPIOF_DIR_IN)?"input":"output",
> > + (flags&GPIOF_DIR_IN)?"":(flags&GPIOF_INIT_HIGH)?"/high":"/low");
> > +
>
> ...
I guess this means to remove the dev_dbg code?
>
> > + err = gpiod_request(desc, name);
> > + if (err)
> > + return ERR_PTR(err);
> > +
> > + if (flags & GPIOF_OPEN_DRAIN)
> > + set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> > +
> > + if (flags & GPIOF_OPEN_SOURCE)
> > + set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> > +
> > + if (flags & GPIOF_ACTIVE_LOW)
> > + set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > +
> > + if (flags & GPIOF_DIR_IN)
> > + err = gpiod_direction_input(desc);
> > + else
> > + err = gpiod_direction_output_raw(desc,
> > + (flags & GPIOF_INIT_HIGH) ? 1 : 0);
> > +
>
> This part of the code is very similar to what is found in
> __gpiod_get_index. Could you maybe try to extract the common code into
> its own function and call it from both sites instead of duplicating
> code?
Agreed.
Originally I was making use of gpio_request_one, but then I noticed it was move to the "_legacy" and would probably dissapear.
>
> > + if (err)
> > + goto free_gpio;
> > +
> > + if (flags & GPIOF_EXPORT) {
> > + err = gpiod_export(desc, flags & GPIOF_EXPORT_CHANGEABLE);
> > + if (err)
> > + goto free_gpio;
>
> Right now export is not possible so I don't think you need that block.
Unless the export feature is requested along with this pacth.
>
> > + }
> > +
> > + return desc;
> > +
> > +free_gpio:
> > + gpiod_free(desc);
> > + return ERR_PTR(err);
> > +}
> > +
> > +/**
> > * gpiod_put - dispose of a GPIO descriptor
> > * @desc: GPIO descriptor to dispose of
> > *
> > diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> > index 38fc050..52d154c 100644
> > --- a/include/linux/of_gpio.h
> > +++ b/include/linux/of_gpio.h
> > @@ -59,6 +59,9 @@ extern int of_gpio_simple_xlate(struct gpio_chip *gc,
> > const struct of_phandle_args *gpiospec,
> > u32 *flags);
> >
> > +extern struct gpio_desc *of_get_gpio_hog(struct device_node *np, int index,
> > + const char **name,
> > + unsigned long *flags);
>
> This function is gpiolib-private, therefore it should be declared in
> drivers/gpio/gpiolib.h alongside with the declaration of
> of_get_named_gpiod_flags.
Ah yes would be much better.
>
> If I understood the latest discussions correctly we might want to add
> an export (and named export) feature on top of this patch. Linus, am I
> correct to understand that you are not opposed to both features? In
> this case, we might want to go ahead with the merging of one of the
> named GPIOs patches, so that Jiri and Pantelis can implement export
> based on this patch.
--
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
