| Message ID | 1413922198-29373-1-git-send-email-bparrot@ti.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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@ti.com> 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 > > 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 > 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@ti.com> > --- > 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. > @@ -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? > + > +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. > + > +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: 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. > }; > > 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. > + > + 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 > + */ > + 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? 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? Finally your desc variable should be declared within this loop since it is not used elsewhere. > + > 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"); > + ... > + 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? > + 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. > + } > + > + 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. 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 linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Benoit, > On Oct 21, 2014, at 23:09 , Benoit Parrot <bparrot@ti.com> 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. 2) There is no way for having hogs inserted dynamically as far as I can tell, and no way to remove a hog either. 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. > Signed-off-by: Benoit Parrot <bparrot@ti.com> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <bparrot@ti.com> 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
Alexandre, Thanks for the feedback. Alexandre Courbot <gnurou@gmail.com> 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@ti.com> 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@ti.com> > > --- > > 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 linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Pantelis, Thanks for the feedback. Pantelis Antoniou <pantelis.antoniou@gmail.com> wrote on Wed [2014-Oct-29 10:53:44 +0200]: > Hi Benoit, > > > On Oct 21, 2014, at 23:09 , Benoit Parrot <bparrot@ti.com> 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 <bparrot@ti.com> > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Maxime Ripard <maxime.ripard@free-electrons.com> 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 <bparrot@ti.com> > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Benoit, > On Oct 29, 2014, at 18:34 , Benoit Parrot <bparrot@ti.com> wrote: > > Pantelis, > > Thanks for the feedback. > > Pantelis Antoniou <pantelis.antoniou@gmail.com> wrote on Wed [2014-Oct-29 10:53:44 +0200]: >> Hi Benoit, >> >>> On Oct 21, 2014, at 23:09 , Benoit Parrot <bparrot@ti.com> 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 <bparrot@ti.com> >>> --- >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 11:41:22AM -0500, Benoit Parrot wrote: > Maxime Ripard <maxime.ripard@free-electrons.com> 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 <bparrot@ti.com> > > > > 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.
Pantelis Antoniou <pantelis.antoniou@gmail.com> wrote on Wed [2014-Oct-29 18:42:48 +0200]: > Hi Benoit, > > > On Oct 29, 2014, at 18:34 , Benoit Parrot <bparrot@ti.com> wrote: > > > > Pantelis, > > > > Thanks for the feedback. > > > > Pantelis Antoniou <pantelis.antoniou@gmail.com> wrote on Wed [2014-Oct-29 10:53:44 +0200]: > >> Hi Benoit, > >> > >>> On Oct 21, 2014, at 23:09 , Benoit Parrot <bparrot@ti.com> 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 <bparrot@ti.com> > >>> --- > >>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Maxime Ripard <maxime.ripard@free-electrons.com> 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 <maxime.ripard@free-electrons.com> 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 <bparrot@ti.com> > > > > > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 30, 2014 at 1:21 AM, Benoit Parrot <bparrot@ti.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 30, 2014 at 1:42 AM, Pantelis Antoniou <pantelis.antoniou@gmail.com> wrote: > Hi Benoit, > >> On Oct 29, 2014, at 18:34 , Benoit Parrot <bparrot@ti.com> wrote: >> >> Pantelis, >> >> Thanks for the feedback. >> >> Pantelis Antoniou <pantelis.antoniou@gmail.com> wrote on Wed [2014-Oct-29 10:53:44 +0200]: >>> Hi Benoit, >>> >>>> On Oct 21, 2014, at 23:09 , Benoit Parrot <bparrot@ti.com> 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? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 06:09:12PM -0500, Benoit Parrot wrote: > Maxime Ripard <maxime.ripard@free-electrons.com> 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 <maxime.ripard@free-electrons.com> 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 <bparrot@ti.com> > > > > > > > > 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
On Wed, Oct 29, 2014 at 9:53 AM, Pantelis Antoniou <pantelis.antoniou@gmail.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 21, 2014 at 10:09 PM, Benoit Parrot <bparrot@ti.com> 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 <bparrot@ti.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <linus.walleij@linaro.org> wrote on Mon [2014-Nov-03 10:59:53 +0100]: > On Tue, Oct 21, 2014 at 10:09 PM, Benoit Parrot <bparrot@ti.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 4, 2014 at 1:38 AM, Benoit Parrot <bparrot@ti.com> wrote: Sorry for slow replies... > Linus Walleij <linus.walleij@linaro.org> wrote on Mon [2014-Nov-03 10:59:53 +0100]: >> On Tue, Oct 21, 2014 at 10:09 PM, Benoit Parrot <bparrot@ti.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 8:09 AM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Wed, Oct 22, 2014 at 5:09 AM, Benoit Parrot <bparrot@ti.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 14, 2014 at 10:19:47AM +0100, Linus Walleij wrote: > On Wed, Oct 29, 2014 at 8:09 AM, Alexandre Courbot <gnurou@gmail.com> wrote: > > On Wed, Oct 22, 2014 at 5:09 AM, Benoit Parrot <bparrot@ti.com> 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? o/ Our pinctrl driver is also our GPIO driver, so they both share the same node. Our pinctrl definitions are there. Maxime
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. + +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"; + }; }; 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; + } + + 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 + */ + for (i = 0; i < chip->ngpio; i++) { + desc = gpiod_get_hog_index(chip->dev, i); + if (IS_ERR(desc)) + break; + } + 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"); + + 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); +} + +/** * 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); #else /* CONFIG_OF_GPIO */ /* Drivers may not strictly depend on the GPIO support, so let them link. */ @@ -78,6 +81,14 @@ static inline int of_gpio_simple_xlate(struct gpio_chip *gc, static inline void of_gpiochip_add(struct gpio_chip *gc) { } static inline void of_gpiochip_remove(struct gpio_chip *gc) { } +static inline struct gpio_desc *of_get_gpio_hog(struct device_node *np, + int index, + const char **name, + unsigned long *flags) +{ + return ERR_PTR(-ENOSYS); +} + #endif /* CONFIG_OF_GPIO */ /**
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 <bparrot@ti.com> --- 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(+)