LinuxLists.cc - [RFC Patch] gpio: add GPIO hogging mechanism

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

Hi,

On Tue, Oct 21, 2014 at 03:09:58PM -0500, Benoit Parrot 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.
>
> 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
> increassingly 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 <[email protected]>

I've been thinking about this for quite some time, it's good to see
some progress on that :)

However, I have a slightly different use case for it: the Allwinner
SoCs have a vdd pin coming in for every gpio bank. Nothing out of the
ordinary so far, except that some of the boards are using a
GPIO-controlled regulator to feed another bank vdd. That obviously
causes a chicken-egg issue, since for the gpio-regulator driver to
probe, it needs to gpio driver, and for the gpio driver to probe, it
needs the regulator driver.

I was thinking of solving this by enforcing gpio hogs, in order to
have the gpio driver loading, grabing its gpios setting them to the
right value to enable the current to flow in, and then let the
regulator driver probe later on.

I don't think it's possible with your current code, would it be
something worth considering, or would someone have a better solution?

Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (1.93 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-10-29 16:21:14

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

Alexandre,

Thanks for the feedback.

Alexandre Courbot <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > ---
> > 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.

2014-10-29 16:34:49

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

Pantelis,

Thanks for the feedback.

Pantelis Antoniou <[email protected]> wrote on Wed [2014-Oct-29 10:53:44 +0200]:
> Hi Benoit,
>
> > On Oct 21, 2014, at 23:09 , Benoit Parrot <[email protected]> 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.
> >
> > 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
> > increassingly 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.
> >
>
> This look like it’s going to the right direction. I have a few general
> comments at first.
>
> 1) It relies on dubious DT binding of having sub-nodes of the
> gpio device implicitly defining hogs.

I think in this instance the nodes are explicitly defining hogs.
Please clarify. What would you like to see here?
>
> 2) There is no way for having hogs inserted dynamically as far as I can tell, and
> no way to remove a hog either.

The original patch was allowing that but, Linus's review comment suggested this feature be
part of the gpio-controller's gpiochip_add() hook only.

>
> 3) I’m not very fond of having this being part of the gpio controller. This
> configuration conceptually has little to do with the gpio controller per se,
> it is more of a board specific thing. Why not come up with a gpio-hog driver that
> references GPIOs? That way with a single gpio-hog driver instance you could setup
> all the GPIO-hogging configuration for all GPIOs on the board, even one that
> lie on different GPIO controllers.

Again this follows Linus's review comment.
I agree that it prevent a centralize spot where all hog would be defined but it has the advantages of not relying on PROBE_DEFER.
I mean since all "gpio-hogs" would be defined in a single board dts file it would not be that hard to figure out the big picture anyways.

>
>
> > Signed-off-by: Benoit Parrot <[email protected]>
> > ---
> > 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(+)
> >
>
> Regards
>
> — Pantelis
>

Regards,
Benoit

2014-10-29 16:41:30

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

Maxime Ripard <[email protected]> wrote on Wed [2014-Oct-29 11:45:59 +0100]:
> Hi,
>
> On Tue, Oct 21, 2014 at 03:09:58PM -0500, Benoit Parrot 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.
> >
> > 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
> > increassingly 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 <[email protected]>
>
> I've been thinking about this for quite some time, it's good to see
> some progress on that :)
>
> However, I have a slightly different use case for it: the Allwinner
> SoCs have a vdd pin coming in for every gpio bank. Nothing out of the
> ordinary so far, except that some of the boards are using a
> GPIO-controlled regulator to feed another bank vdd. That obviously
> causes a chicken-egg issue, since for the gpio-regulator driver to
> probe, it needs to gpio driver, and for the gpio driver to probe, it
> needs the regulator driver.

Unless the gpio controlling the vdd pin is from the same bank your trying to power up
I do not see the issue here.

In this patch the gpio-hogs are setup as part of the gpio-controller probe function.

>
> I was thinking of solving this by enforcing gpio hogs, in order to
> have the gpio driver loading, grabing its gpios setting them to the
> right value to enable the current to flow in, and then let the
> regulator driver probe later on.
>
> I don't think it's possible with your current code, would it be
> something worth considering, or would someone have a better solution?

Unless I am missing something, this should work.

>
> Maxime
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Regards,
Benoit

2014-10-29 16:50:14

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

On Wed, Oct 29, 2014 at 11:41:22AM -0500, Benoit Parrot wrote:
> Maxime Ripard <[email protected]> wrote on Wed [2014-Oct-29 11:45:59 +0100]:
> > Hi,
> >
> > On Tue, Oct 21, 2014 at 03:09:58PM -0500, Benoit Parrot 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.
> > >
> > > 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
> > > increassingly 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 <[email protected]>
> >
> > I've been thinking about this for quite some time, it's good to see
> > some progress on that :)
> >
> > However, I have a slightly different use case for it: the Allwinner
> > SoCs have a vdd pin coming in for every gpio bank. Nothing out of the
> > ordinary so far, except that some of the boards are using a
> > GPIO-controlled regulator to feed another bank vdd. That obviously
> > causes a chicken-egg issue, since for the gpio-regulator driver to
> > probe, it needs to gpio driver, and for the gpio driver to probe, it
> > needs the regulator driver.
>
> Unless the gpio controlling the vdd pin is from the same bank your
> trying to power up I do not see the issue here.

Not the same bank, but the same driver.

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (2.01 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-10-29 16:50:28

by Pantelis Antoniou

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

Hi Benoit,

> On Oct 29, 2014, at 18:34 , Benoit Parrot <[email protected]> wrote:
>
> Pantelis,
>
> Thanks for the feedback.
>
> Pantelis Antoniou <[email protected]> wrote on Wed [2014-Oct-29 10:53:44 +0200]:
>> Hi Benoit,
>>
>>> On Oct 21, 2014, at 23:09 , Benoit Parrot <[email protected]> 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.
>>>
>>> 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
>>> increassingly 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.
>>>
>>
>> This look like it’s going to the right direction. I have a few general
>> comments at first.
>>
>> 1) It relies on dubious DT binding of having sub-nodes of the
>> gpio device implicitly defining hogs.
>
> I think in this instance the nodes are explicitly defining hogs.
> Please clarify. What would you like to see here?
>>

Any subnodes are implicitly taken as hog definitions. This is not right because
gpio controllers might have subnodes that they use for another purpose.

>> 2) There is no way for having hogs inserted dynamically as far as I can tell, and
>> no way to remove a hog either.
>
> The original patch was allowing that but, Linus's review comment suggested this feature be
> part of the gpio-controller's gpiochip_add() hook only.
>

If it’s not possible to remove a hog, then it’s no good for my use case in which
the gpios get exported and then removed.

>>
>> 3) I’m not very fond of having this being part of the gpio controller. This
>> configuration conceptually has little to do with the gpio controller per se,
>> it is more of a board specific thing. Why not come up with a gpio-hog driver that
>> references GPIOs? That way with a single gpio-hog driver instance you could setup
>> all the GPIO-hogging configuration for all GPIOs on the board, even one that
>> lie on different GPIO controllers.
>
> Again this follows Linus's review comment.
> I agree that it prevent a centralize spot where all hog would be defined but it has the advantages of not relying on PROBE_DEFER.
> I mean since all "gpio-hogs" would be defined in a single board dts file it would not be that hard to figure out the big picture anyways.
>

There won’t be a single board dts file if you’re using things like overlays.

>>
>>
>>> Signed-off-by: Benoit Parrot <[email protected]>
>>> ---
>>> 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(+)
>>>
>>
>> Regards
>>
>> — Pantelis
>>
>
> Regards,
> Benoit

Regards

— Pantelis

2014-10-29 19:36:55

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

Pantelis Antoniou <[email protected]> wrote on Wed [2014-Oct-29 18:42:48 +0200]:
> Hi Benoit,
>
> > On Oct 29, 2014, at 18:34 , Benoit Parrot <[email protected]> wrote:
> >
> > Pantelis,
> >
> > Thanks for the feedback.
> >
> > Pantelis Antoniou <[email protected]> wrote on Wed [2014-Oct-29 10:53:44 +0200]:
> >> Hi Benoit,
> >>
> >>> On Oct 21, 2014, at 23:09 , Benoit Parrot <[email protected]> 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.
> >>>
> >>> 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
> >>> increassingly 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.
> >>>
> >>
> >> This look like it’s going to the right direction. I have a few general
> >> comments at first.
> >>
> >> 1) It relies on dubious DT binding of having sub-nodes of the
> >> gpio device implicitly defining hogs.
> >
> > I think in this instance the nodes are explicitly defining hogs.
> > Please clarify. What would you like to see here?
> >>
>
> Any subnodes are implicitly taken as hog definitions. This is not right because
> gpio controllers might have subnodes that they use for another purpose.

I think I see what you mean.
Even though "gpio-hog = <&phandle>" guarantee that only those sub-node
are going to be parsed, if a particular gpio-controller has sub-node for
some other reason then the "hog" related sub-node might be in the way.

Do you know of such an example currently in mainline I can take a look at?

>
> >> 2) There is no way for having hogs inserted dynamically as far as I can tell, and
> >> no way to remove a hog either.
> >
> > The original patch was allowing that but, Linus's review comment suggested this feature be
> > part of the gpio-controller's gpiochip_add() hook only.
> >
>
> If it’s not possible to remove a hog, then it’s no good for my use case in which
> the gpios get exported and then removed.

I guess if we add the export feature then that could be possible.
But if I am not mistaken the hog concept was to allow gpio setting
for gpio not belonging to any particular drivers.
If you have gpio which needs to be released then would they fall in
the "own" by a driver category and which case use the exixting method?
>
> >>
> >> 3) I’m not very fond of having this being part of the gpio controller. This
> >> configuration conceptually has little to do with the gpio controller per se,
> >> it is more of a board specific thing. Why not come up with a gpio-hog driver that
> >> references GPIOs? That way with a single gpio-hog driver instance you could setup
> >> all the GPIO-hogging configuration for all GPIOs on the board, even one that
> >> lie on different GPIO controllers.
> >
> > Again this follows Linus's review comment.
> > I agree that it prevent a centralize spot where all hog would be defined but it has the advantages of not relying on PROBE_DEFER.
> > I mean since all "gpio-hogs" would be defined in a single board dts file it would not be that hard to figure out the big picture anyways.
> >
>
> There won’t be a single board dts file if you’re using things like overlays.
I see, I had not consider that.

A generic board level gpio driver would be a different option I guess.

>
> >>
> >>
> >>> Signed-off-by: Benoit Parrot <[email protected]>
> >>> ---
> >>> 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(+)
> >>>
> >>
> >> Regards
> >>
> >> — Pantelis
> >>
> >
> > Regards,
> > Benoit
>
> Regards
>
> — Pantelis
>

Regards,
Benoit

2014-10-29 23:09:17

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

Maxime Ripard <[email protected]> wrote on Wed [2014-Oct-29 17:47:46 +0100]:
> On Wed, Oct 29, 2014 at 11:41:22AM -0500, Benoit Parrot wrote:
> > Maxime Ripard <[email protected]> wrote on Wed [2014-Oct-29 11:45:59 +0100]:
> > > Hi,
> > >
> > > On Tue, Oct 21, 2014 at 03:09:58PM -0500, Benoit Parrot 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.
> > > >
> > > > 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
> > > > increassingly 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 <[email protected]>
> > >
> > > I've been thinking about this for quite some time, it's good to see
> > > some progress on that :)
> > >
> > > However, I have a slightly different use case for it: the Allwinner
> > > SoCs have a vdd pin coming in for every gpio bank. Nothing out of the
> > > ordinary so far, except that some of the boards are using a
> > > GPIO-controlled regulator to feed another bank vdd. That obviously
> > > causes a chicken-egg issue, since for the gpio-regulator driver to
> > > probe, it needs to gpio driver, and for the gpio driver to probe, it
> > > needs the regulator driver.
> >
> > Unless the gpio controlling the vdd pin is from the same bank your
> > trying to power up I do not see the issue here.
>
> Not the same bank, but the same driver.

How are you currently working around this issue?

>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

2014-10-30 00:29:25

by Alexandre Courbot

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

On Thu, Oct 30, 2014 at 1:21 AM, Benoit Parrot <[email protected]> wrote:
>> > +
>> > 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?

No, it was just to delimitate the code which I suggested to factorize
into its own function below. :) This dev_dbg is fine IMHO.

2014-10-30 00:32:06

by Alexandre Courbot

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

On Thu, Oct 30, 2014 at 1:42 AM, Pantelis Antoniou
<[email protected]> wrote:
> Hi Benoit,
>
>> On Oct 29, 2014, at 18:34 , Benoit Parrot <[email protected]> wrote:
>>
>> Pantelis,
>>
>> Thanks for the feedback.
>>
>> Pantelis Antoniou <[email protected]> wrote on Wed [2014-Oct-29 10:53:44 +0200]:
>>> Hi Benoit,
>>>
>>>> On Oct 21, 2014, at 23:09 , Benoit Parrot <[email protected]> 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.
>>>>
>>>> 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
>>>> increassingly 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.
>>>>
>>>
>>> This look like it’s going to the right direction. I have a few general
>>> comments at first.
>>>
>>> 1) It relies on dubious DT binding of having sub-nodes of the
>>> gpio device implicitly defining hogs.
>>
>> I think in this instance the nodes are explicitly defining hogs.
>> Please clarify. What would you like to see here?
>>>
>
> Any subnodes are implicitly taken as hog definitions. This is not right because
> gpio controllers might have subnodes that they use for another purpose.
>
>>> 2) There is no way for having hogs inserted dynamically as far as I can tell, and
>>> no way to remove a hog either.
>>
>> The original patch was allowing that but, Linus's review comment suggested this feature be
>> part of the gpio-controller's gpiochip_add() hook only.
>>
>
> If it’s not possible to remove a hog, then it’s no good for my use case in which
> the gpios get exported and then removed.

Why would you want to remove a GPIO hog at runtime, and what is the
point of having it set in the DT in that case?

2014-10-30 17:20:08

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

On Wed, Oct 29, 2014 at 06:09:12PM -0500, Benoit Parrot wrote:
> Maxime Ripard <[email protected]> wrote on Wed [2014-Oct-29 17:47:46 +0100]:
> > On Wed, Oct 29, 2014 at 11:41:22AM -0500, Benoit Parrot wrote:
> > > Maxime Ripard <[email protected]> wrote on Wed [2014-Oct-29 11:45:59 +0100]:
> > > > Hi,
> > > >
> > > > On Tue, Oct 21, 2014 at 03:09:58PM -0500, Benoit Parrot 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.
> > > > >
> > > > > 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
> > > > > increassingly 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 <[email protected]>
> > > >
> > > > I've been thinking about this for quite some time, it's good to see
> > > > some progress on that :)
> > > >
> > > > However, I have a slightly different use case for it: the Allwinner
> > > > SoCs have a vdd pin coming in for every gpio bank. Nothing out of the
> > > > ordinary so far, except that some of the boards are using a
> > > > GPIO-controlled regulator to feed another bank vdd. That obviously
> > > > causes a chicken-egg issue, since for the gpio-regulator driver to
> > > > probe, it needs to gpio driver, and for the gpio driver to probe, it
> > > > needs the regulator driver.
> > >
> > > Unless the gpio controlling the vdd pin is from the same bank your
> > > trying to power up I do not see the issue here.
> >
> > Not the same bank, but the same driver.
>
> How are you currently working around this issue?

For now, we are not, which is exactly why I'm interested in such a
mechanism :)

What I was thinking about for this case would be to "fake" the fact
that the GPIO is even there. The regulator driver would probe, claim
the GPIO, without actually doing anything more than just storing the
value to set, which would be set in the hardware only when the GPIO
driver probes.

I'm not very happy about it though, because that would mean that
drivers that require more than just a value assignment (for example
set high, wait, set low, for a reset for example) wouldn't even know
that what they expect didn't happen.

Maybe that could work by passing some kind of flag to gpio_request to
trigger that mecanism, I don't know.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (3.09 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-11-03 09:43:52

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

On Wed, Oct 29, 2014 at 9:53 AM, Pantelis Antoniou
<[email protected]> wrote:

> 3) I’m not very fond of having this being part of the gpio controller. This
> configuration conceptually has little to do with the gpio controller per se,
> it is more of a board specific thing. Why not come up with a gpio-hog driver that
> references GPIOs? That way with a single gpio-hog driver instance you could setup
> all the GPIO-hogging configuration for all GPIOs on the board, even one that
> lie on different GPIO controllers.

You have a point. But it's vital that we set up hogs at the same time as the
gpio_chip is probed, not later, or there may be a race as to who
gets the GPIO line, and that should be avoided at any cost.

Yours,
Linus Walleij

2014-11-03 09:59:57

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

On Tue, Oct 21, 2014 at 10:09 PM, Benoit Parrot <[email protected]> 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.
>
> 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
> increassingly 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 <[email protected]>

Thanks for working on this.

> 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
> @@ -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.

The GPIO chip may contain... you cannot start a sentence with "It"

> +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).
> +- 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.
> +
> +Optional properties:
> +- line-name: the GPIO label name. If not present the node name is used.
> +
> +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 don't see the point of having unused hogs and enabling them using
phandles.

Just let the core walk over all children nodes of a GPIO controller
and hog them. Put in a bool property saying it's a hog.

+ line_b: line_b {
+ gpio-hog;
+ gpios = <6 0>;
+ output-low;
+ line-name = "foo-bar-gpio";
+ };

I don't quite see the point with input hogs that noone can use
but whatever.

I am thinking that maybe the line name should be compulsory
so as to improbe readability. I mean there is always a reason
why you're hogging a pin and the name should say it.

> /**
> + * 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);

So no phandles please.

> + 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;
> + }
> +
> + 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);

Alter the body of of_gpiochip_add() instead of adding more code here
in the core gpiolib. You need not patch a single line of this code.

> acpi_gpiochip_add(chip);
>
> + /*
> + * Instantiate GPIO hogs
> + * There shouldn't be mores hogs then gpio available on this chip
> + */
> + for (i = 0; i < chip->ngpio; i++) {
> + desc = gpiod_get_hog_index(chip->dev, i);
> + if (IS_ERR(desc))
> + break;
> + }

Ick that is not elegant.

Instead use

struct device_node *child;

for_each_child_of_node(chip->dev.of_node, np) {
if (!of_property_read_bool(np, "gpio-hog"))
continue;
/* Do the hogging */
}

> @@ -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");
> +
> + 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);
> +
> + if (err)
> + goto free_gpio;
> +
> + if (flags & GPIOF_EXPORT) {
> + err = gpiod_export(desc, flags & GPIOF_EXPORT_CHANGEABLE);
> + if (err)
> + goto free_gpio;
> + }
> +
> + return desc;
> +
> +free_gpio:
> + gpiod_free(desc);
> + return ERR_PTR(err);
> +}

I don't see the point of the device tree code calling into the core to
set up the hog if currently only device tree can do this.

Keep all the code in gpiolib-of.c and make it a static call for now.

If ACPI needs the same we can refactor it later.

> +
> +/**
> * 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);

No, if this is a gpiolib-internal call it should not be in the public
header at all.

Move to drivers/gpio/gpiolib.h, stubs and all.

Yours,
Linus Walleij

2014-11-04 00:38:33

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

Linus,

Thanks for the feedback.

To summarize the hog feature should be local to gpiolib-of.c, correct?

I also also need some clarifications, see below.

Linus Walleij <[email protected]> wrote on Mon [2014-Nov-03 10:59:53 +0100]:
> On Tue, Oct 21, 2014 at 10:09 PM, Benoit Parrot <[email protected]> wrote:
>
> > 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 don't see the point of having unused hogs and enabling them using
> phandles.
>
> Just let the core walk over all children nodes of a GPIO controller
> and hog them. Put in a bool property saying it's a hog.
>
> + line_b: line_b {
> + gpio-hog;
> + gpios = <6 0>;
> + output-low;
> + line-name = "foo-bar-gpio";
> + };
>
> I don't quite see the point with input hogs that noone can use
> but whatever.
>
> I am thinking that maybe the line name should be compulsory
> so as to improbe readability. I mean there is always a reason
> why you're hogging a pin and the name should say it.

Ok, so as an alternative I had presented something like this in my reply
to Alexandre Courbot's review comments:

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
>;
};

Now based on your comment would something like this work?

qe_pio_a: gpio-controller@1400 {
reg = <0x1400 0x18>;
gpio-controller;
#gpio-cells = <2>;

/* Line syntax: line_name <gpio# flags> direction-value [export] */
gpio-hogs: {
gpio-hogs-group = <
foo-bar-gpio <15 0> output-low
bar-foo-gpio <16 0> output-high export
>;
};
};

This would group all hogs for one controller under a single child node.
Again I am not sure how feasible or easy to implement the DT parsing would be.

I guess for completeness if you could also comment on my reply to Alexandre from Oct 29th,
that would be great, before I head in the wrong directions.

Regards,
Benoit

2014-11-14 09:16:26

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

On Tue, Nov 4, 2014 at 1:38 AM, Benoit Parrot <[email protected]> wrote:

Sorry for slow replies...

> Linus Walleij <[email protected]> wrote on Mon [2014-Nov-03 10:59:53 +0100]:
>> On Tue, Oct 21, 2014 at 10:09 PM, Benoit Parrot <[email protected]> wrote:
>>
>> > 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 don't see the point of having unused hogs and enabling them using
>> phandles.
>>
>> Just let the core walk over all children nodes of a GPIO controller
>> and hog them. Put in a bool property saying it's a hog.
>>
>> + line_b: line_b {
>> + gpio-hog;
>> + gpios = <6 0>;
>> + output-low;
>> + line-name = "foo-bar-gpio";
>> + };
>>
>> I don't quite see the point with input hogs that noone can use
>> but whatever.
>>
>> I am thinking that maybe the line name should be compulsory
>> so as to improbe readability. I mean there is always a reason
>> why you're hogging a pin and the name should say it.
>
> Ok, so as an alternative I had presented something like this in my reply
> to Alexandre Courbot's review comments:
>
> 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
> >;
> };
>
> Now based on your comment would something like this work?
>
> qe_pio_a: gpio-controller@1400 {
> reg = <0x1400 0x18>;
> gpio-controller;
> #gpio-cells = <2>;
>
> /* Line syntax: line_name <gpio# flags> direction-value [export] */
> gpio-hogs: {
> gpio-hogs-group = <
> foo-bar-gpio <15 0> output-low
> bar-foo-gpio <16 0> output-high export
> >;
> };
> };

I *DON'T* want to mix up the exporting interface with the hogging
mechanism. These have to be two different things and different
patches.

But it looks strange and a bit convoluted. I don't see the point
of the grouping concept. There are ages old mails where I suggest
a very flat mechanism like this:

qe_pio_a: gpio-controller@1400 {
reg = <0x1400 0x18>;
gpio-controller;
#gpio-cells = <2>;
gpio-hogs-output-high = <15 0>, <12 0>;
gpio-hogs-output-low = <16 0>;
};

I understand that if you want to give names to the pins that
is maybe a bit terse, then I suggest these named nodes:

qe_pio_a: gpio-controller@1400 {
reg = <0x1400 0x18>;
gpio-controller;
#gpio-cells = <2>;
{
gpio-hog-output-high = <15 0>;
line-name = "foo";
};
{
gpio-hog-output-high = <12 0>;
line-name = "bar";
};
{
gpio-hog-output-low = <16 0>;
line-name = "baz";
};
};

This is pretty straight-forward to parse from the device
tree by just walking over the children of a GPIO controller
node and looking for the hog keywords and optional
line names.

This mechanism can later add some per-pin export
keyword too, if that is desired. But that is a separate
discussion.

Still no need for groups or phandles or stuff like that...

It's a terser version of what I suggested in the last
reply from me:

+ line_b: line_b {
+ gpio-hog;
+ gpios = <6 0>;
+ output-low;
+ line-name = "foo-bar-gpio";
+ };

Just that I combine gpio-hog, gpios and output-low
into one property.

Any version works, I just don't get this grouping and
phandle business, if such complexity is needed it has
to be motivated.

> This would group all hogs for one controller under a single child node.

Why is that a desireable feature?

I will try to find the other mail thread...

Yours,
Linus Walleij

2014-11-14 09:19:51

[permalink] [raw]

Subject: Re: [RFC Patch] gpio: add GPIO hogging mechanism

On Wed, Oct 29, 2014 at 8:09 AM, Alexandre Courbot <[email protected]> wrote:
> On Wed, Oct 22, 2014 at 5:09 AM, Benoit Parrot <[email protected]> wrote:

> + line_b: line_b {
> + line_b {
> + gpios = <6 0>;
> + output-low;
> + line-name = "foo-bar-gpio";
> + };
> + };
>
(...)
>
> 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.

Yes, so I have suggested a hog-something; keyword in there.

As long as the children don't have any compatible-strings we can
decide pretty much how they should be handled internally.

Are there custom drivers with child nodes inside the main chip
today?

Yours,
Linus Walleij

2014-11-14 10:25:08