| Message ID | 1420621722-7428-3-git-send-email-oliver+list@schinagl.nl |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 07, 2015 at 10:08:40AM +0100, Olliver Schinagl wrote: > From: Olliver Schinagl <oliver@schinagl.nl> > > The gpio binding document says that new code should always use named > gpios. Patch 40b73183 added support to parse a list of gpios from child > nodes, but does not make it possible to use named gpios. This patch adds > the con_id property and implements it is done in gpiolib.c, where the > old-style of using unnamed gpios still works. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > --- > drivers/gpio/devres.c | 16 +++++++++++++++- > drivers/input/keyboard/gpio_keys_polled.c | 2 +- > drivers/leds/leds-gpio.c | 2 +- > include/linux/gpio/consumer.h | 1 + > 4 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c > index 13dbd3d..b7fbe1c 100644 > --- a/drivers/gpio/devres.c > +++ b/drivers/gpio/devres.c > @@ -111,23 +111,37 @@ EXPORT_SYMBOL(__devm_gpiod_get_index); > /** > * devm_get_gpiod_from_child - get a GPIO descriptor from a device's child node > * @dev: GPIO consumer > + * @con_id: function within the GPIO consumer > * @child: firmware node (child of @dev) > * > * GPIO descriptors returned from this function are automatically disposed on > * driver detach. > */ > struct gpio_desc *devm_get_gpiod_from_child(struct device *dev, > + const char *con_id, > struct fwnode_handle *child) > { > + static const char const *suffixes[] = { "gpios", "gpio" }; > + char prop_name[32]; /* 32 is max size of property name */ > struct gpio_desc **dr; > struct gpio_desc *desc; > + unsigned int i; > > dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *), > GFP_KERNEL); > if (!dr) > return ERR_PTR(-ENOMEM); > > - desc = fwnode_get_named_gpiod(child, "gpios"); > + for (i = 0; i < ARRAY_SIZE(suffixes); i++) { > + if (con_id) > + snprintf(prop_name, 32, "%s-%s", con_id, suffixes[i]); sizeof(prop_name) instead of hard-coding 32 would be better. > + else > + snprintf(prop_name, 32, "%s", suffixes[i]); > + > + desc = fwnode_get_named_gpiod(child, prop_name); > + if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER)) > + break; > + } > if (IS_ERR(desc)) { > devres_free(dr); > return desc; > diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c > index 90df4df..097d721 100644 > --- a/drivers/input/keyboard/gpio_keys_polled.c > +++ b/drivers/input/keyboard/gpio_keys_polled.c > @@ -125,7 +125,7 @@ static struct gpio_keys_platform_data *gpio_keys_polled_get_devtree_pdata(struct > device_for_each_child_node(dev, child) { > struct gpio_desc *desc; > > - desc = devm_get_gpiod_from_child(dev, child); > + desc = devm_get_gpiod_from_child(dev, NULL, child); I guess the number of users is small enough that we can do this API change in one step if gpio overlords are happy with it. > if (IS_ERR(desc)) { > error = PTR_ERR(desc); > if (error != -EPROBE_DEFER) > diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c > index 7ea1ea42..8e5af69 100644 > --- a/drivers/leds/leds-gpio.c > +++ b/drivers/leds/leds-gpio.c > @@ -184,7 +184,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev) > struct gpio_led led = {}; > const char *state = NULL; > > - led.gpiod = devm_get_gpiod_from_child(dev, child); > + led.gpiod = devm_get_gpiod_from_child(dev, NULL, child); > if (IS_ERR(led.gpiod)) { > fwnode_handle_put(child); > goto err; > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h > index 45afc2d..ed20759 100644 > --- a/include/linux/gpio/consumer.h > +++ b/include/linux/gpio/consumer.h > @@ -110,6 +110,7 @@ struct fwnode_handle; > struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode, > const char *propname); > struct gpio_desc *devm_get_gpiod_from_child(struct device *dev, > + const char *con_id, > struct fwnode_handle *child); > #else /* CONFIG_GPIOLIB */ > > -- > 2.1.4 > Thanks.
On Wed, Jan 7, 2015 at 10:08 AM, Olliver Schinagl <oliver+list@schinagl.nl> wrote: > From: Olliver Schinagl <oliver@schinagl.nl> > > The gpio binding document says that new code should always use named > gpios. Patch 40b73183 added support to parse a list of gpios from child > nodes, but does not make it possible to use named gpios. This patch adds > the con_id property and implements it is done in gpiolib.c, where the > old-style of using unnamed gpios still works. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> Alexandre, can you please take a close look at this patch. 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, Jan 7, 2015 at 6:08 PM, Olliver Schinagl <oliver+list@schinagl.nl> wrote: > From: Olliver Schinagl <oliver@schinagl.nl> > > The gpio binding document says that new code should always use named > gpios. Patch 40b73183 added support to parse a list of gpios from child > nodes, but does not make it possible to use named gpios. This patch adds > the con_id property and implements it is done in gpiolib.c, where the > old-style of using unnamed gpios still works. This is absolutely correct - thanks for spotting this. > > Signed-off-by: Olliver Schinagl <oliver@schinagl.nl> > --- > drivers/gpio/devres.c | 16 +++++++++++++++- > drivers/input/keyboard/gpio_keys_polled.c | 2 +- > drivers/leds/leds-gpio.c | 2 +- > include/linux/gpio/consumer.h | 1 + > 4 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c > index 13dbd3d..b7fbe1c 100644 > --- a/drivers/gpio/devres.c > +++ b/drivers/gpio/devres.c > @@ -111,23 +111,37 @@ EXPORT_SYMBOL(__devm_gpiod_get_index); > /** > * devm_get_gpiod_from_child - get a GPIO descriptor from a device's child node > * @dev: GPIO consumer > + * @con_id: function within the GPIO consumer > * @child: firmware node (child of @dev) > * > * GPIO descriptors returned from this function are automatically disposed on > * driver detach. > */ > struct gpio_desc *devm_get_gpiod_from_child(struct device *dev, > + const char *con_id, > struct fwnode_handle *child) > { > + static const char const *suffixes[] = { "gpios", "gpio" }; > + char prop_name[32]; /* 32 is max size of property name */ > struct gpio_desc **dr; > struct gpio_desc *desc; > + unsigned int i; > > dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *), > GFP_KERNEL); > if (!dr) > return ERR_PTR(-ENOMEM); > > - desc = fwnode_get_named_gpiod(child, "gpios"); > + for (i = 0; i < ARRAY_SIZE(suffixes); i++) { > + if (con_id) > + snprintf(prop_name, 32, "%s-%s", con_id, suffixes[i]); > + else > + snprintf(prop_name, 32, "%s", suffixes[i]); Same remark as Dmitry about the hardcoded length of the string, but ... > + > + desc = fwnode_get_named_gpiod(child, prop_name); > + if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER)) > + break; > + } ... since it looks like this part has been mostly copy/pasted from of_find_gpio(), can you add another patch that fixes it there as well? Also in the case of ACPI this will prove to be an incomplete lookup since acpi_find_gpio() has an additional fallback if the named lookup fails. In that respect, I wonder if it would not be better for devm_get_gpiod_from_child() to call of_find_gpio() and acpi_find_gpio() (after making them non-static) followed by gpiod_request() instead of calling fwnode_get_named_gpiod(). But in that case it will have to do the OF/ACPI handling by itself. I'm not really sure about which way is better. I'd appreciate if you could give a thought to a possible refactoring that would improve the situation ; otherwise feel free to ignore what I have written above and to duplicate the property name building code. -- 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
Hey Alexandre, On 01/19/2015 05:04 AM, Alexandre Courbot wrote: > On Wed, Jan 7, 2015 at 6:08 PM, Olliver Schinagl > <oliver+list@schinagl.nl> wrote: >> From: Olliver Schinagl <oliver@schinagl.nl> >> >> The gpio binding document says that new code should always use named >> gpios. Patch 40b73183 added support to parse a list of gpios from child >> nodes, but does not make it possible to use named gpios. This patch adds >> the con_id property and implements it is done in gpiolib.c, where the >> old-style of using unnamed gpios still works. > This is absolutely correct - thanks for spotting this. > > <snip> > ... since it looks like this part has been mostly copy/pasted from > of_find_gpio(), can you add another patch that fixes it there as well? Yeah, since it has the same functionality, i copy pasted it. Wasn't sure if it was worth to macro it or anything. I've sent a v2 with that patch added to the mix :) > > Also in the case of ACPI this will prove to be an incomplete lookup > since acpi_find_gpio() has an additional fallback if the named lookup > fails. I'm not very familiar (or at all) how ACPI falls into all of this, I'm just starting to get a hang of the DT, but since this is how the dts is being parsed, where is the relation here? Or did I misunderstand? > > In that respect, I wonder if it would not be better for > devm_get_gpiod_from_child() to call of_find_gpio() and > acpi_find_gpio() (after making them non-static) followed by > gpiod_request() instead of calling fwnode_get_named_gpiod(). But in > that case it will have to do the OF/ACPI handling by itself. > > I'm not really sure about which way is better. I'd appreciate if you > could give a thought to a possible refactoring that would improve the > situation ; otherwise feel free to ignore what I have written above > and to duplicate the property name building code. I'm afraid I'm a little too inexperienced to follow exactly what you say ;) Olliver -- 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, Jan 22, 2015 at 6:44 AM, Olliver Schinagl <oliver@schinagl.nl> wrote: > Hey Alexandre, > > On 01/19/2015 05:04 AM, Alexandre Courbot wrote: >> >> On Wed, Jan 7, 2015 at 6:08 PM, Olliver Schinagl >> <oliver+list@schinagl.nl> wrote: >>> >>> From: Olliver Schinagl <oliver@schinagl.nl> >>> >>> The gpio binding document says that new code should always use named >>> gpios. Patch 40b73183 added support to parse a list of gpios from child >>> nodes, but does not make it possible to use named gpios. This patch adds >>> the con_id property and implements it is done in gpiolib.c, where the >>> old-style of using unnamed gpios still works. >> >> This is absolutely correct - thanks for spotting this. >> >> <snip> >> ... since it looks like this part has been mostly copy/pasted from >> of_find_gpio(), can you add another patch that fixes it there as well? > > Yeah, since it has the same functionality, i copy pasted it. Wasn't sure if > it was worth to macro it or anything. I've sent a v2 with that patch added > to the mix :) >> >> >> Also in the case of ACPI this will prove to be an incomplete lookup >> since acpi_find_gpio() has an additional fallback if the named lookup >> fails. > > I'm not very familiar (or at all) how ACPI falls into all of this, I'm just > starting to get a hang of the DT, but since this is how the dts is being > parsed, where is the relation here? Or did I misunderstand? GPIO mappings can be provided by DT or by ACPI. In the latter case there is a fallback method if the name does not match (see acpi_find_gpio()). fwnode_get_named_gpiod() does not check it though, so maybe we can just ignore that. >> In that respect, I wonder if it would not be better for >> devm_get_gpiod_from_child() to call of_find_gpio() and >> acpi_find_gpio() (after making them non-static) followed by >> gpiod_request() instead of calling fwnode_get_named_gpiod(). But in >> that case it will have to do the OF/ACPI handling by itself. >> >> I'm not really sure about which way is better. I'd appreciate if you >> could give a thought to a possible refactoring that would improve the >> situation ; otherwise feel free to ignore what I have written above >> and to duplicate the property name building code. > > I'm afraid I'm a little too inexperienced to follow exactly what you say ;) Don't worry about it then, this patch is already an improvement. GPIO mappings lookup from firmware has become kind of messy, and I'm just trying to enroll people to clean it instead of doing it myself. ;) -- 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
diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c index 13dbd3d..b7fbe1c 100644 --- a/drivers/gpio/devres.c +++ b/drivers/gpio/devres.c @@ -111,23 +111,37 @@ EXPORT_SYMBOL(__devm_gpiod_get_index); /** * devm_get_gpiod_from_child - get a GPIO descriptor from a device's child node * @dev: GPIO consumer + * @con_id: function within the GPIO consumer * @child: firmware node (child of @dev) * * GPIO descriptors returned from this function are automatically disposed on * driver detach. */ struct gpio_desc *devm_get_gpiod_from_child(struct device *dev, + const char *con_id, struct fwnode_handle *child) { + static const char const *suffixes[] = { "gpios", "gpio" }; + char prop_name[32]; /* 32 is max size of property name */ struct gpio_desc **dr; struct gpio_desc *desc; + unsigned int i; dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *), GFP_KERNEL); if (!dr) return ERR_PTR(-ENOMEM); - desc = fwnode_get_named_gpiod(child, "gpios"); + for (i = 0; i < ARRAY_SIZE(suffixes); i++) { + if (con_id) + snprintf(prop_name, 32, "%s-%s", con_id, suffixes[i]); + else + snprintf(prop_name, 32, "%s", suffixes[i]); + + desc = fwnode_get_named_gpiod(child, prop_name); + if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER)) + break; + } if (IS_ERR(desc)) { devres_free(dr); return desc; diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c index 90df4df..097d721 100644 --- a/drivers/input/keyboard/gpio_keys_polled.c +++ b/drivers/input/keyboard/gpio_keys_polled.c @@ -125,7 +125,7 @@ static struct gpio_keys_platform_data *gpio_keys_polled_get_devtree_pdata(struct device_for_each_child_node(dev, child) { struct gpio_desc *desc; - desc = devm_get_gpiod_from_child(dev, child); + desc = devm_get_gpiod_from_child(dev, NULL, child); if (IS_ERR(desc)) { error = PTR_ERR(desc); if (error != -EPROBE_DEFER) diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c index 7ea1ea42..8e5af69 100644 --- a/drivers/leds/leds-gpio.c +++ b/drivers/leds/leds-gpio.c @@ -184,7 +184,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev) struct gpio_led led = {}; const char *state = NULL; - led.gpiod = devm_get_gpiod_from_child(dev, child); + led.gpiod = devm_get_gpiod_from_child(dev, NULL, child); if (IS_ERR(led.gpiod)) { fwnode_handle_put(child); goto err; diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index 45afc2d..ed20759 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -110,6 +110,7 @@ struct fwnode_handle; struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode, const char *propname); struct gpio_desc *devm_get_gpiod_from_child(struct device *dev, + const char *con_id, struct fwnode_handle *child); #else /* CONFIG_GPIOLIB */