* [PATCH 0/2] gpio: Add the devm_fwnode_get_index_gpiod_from_child() helper @ 2017-01-30 15:41 Boris Brezillon 2017-01-30 15:41 ` [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon 2017-01-30 15:41 ` [PATCH 2/2] gpio: Add the devm_fwnode_get_index_gpiod_from_child() helper Boris Brezillon 0 siblings, 2 replies; 20+ messages in thread From: Boris Brezillon @ 2017-01-30 15:41 UTC (permalink / raw) To: Linus Walleij, Alexandre Courbot, linux-gpio Cc: Dmitry Torokhov, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King, Boris Brezillon Hello Linus, I send these 2 patches separately from the Atmel NAND driver rework since these are self-contained and do not have any external dependencies. Feel free to apply them in your tree if you are happy with this version and once other subsystem maintainers have acked patch 1. Thanks, Boris --- Changes since v1: - Rename the devm_get_gpiod_from_child() helper into devm_fwnode_get_gpiod_from_child() - Define as a static inline wrapper around devm_fwnode_get_gpiod_from_child() Boris Brezillon (2): gpio: Rename devm_get_gpiod_from_child() gpio: Add the devm_fwnode_get_index_gpiod_from_child() helper drivers/gpio/devres.c | 14 ++++++++------ drivers/gpio/gpiolib.c | 9 +++++---- drivers/input/keyboard/gpio_keys.c | 3 ++- drivers/input/keyboard/gpio_keys_polled.c | 5 +++-- drivers/leds/leds-gpio.c | 2 +- drivers/video/fbdev/amba-clcd-nomadik.c | 8 ++++---- include/linux/gpio/consumer.h | 23 ++++++++++++++++------- 7 files changed, 39 insertions(+), 25 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-30 15:41 [PATCH 0/2] gpio: Add the devm_fwnode_get_index_gpiod_from_child() helper Boris Brezillon @ 2017-01-30 15:41 ` Boris Brezillon 2017-01-30 19:57 ` Jacek Anaszewski ` (3 more replies) 2017-01-30 15:41 ` [PATCH 2/2] gpio: Add the devm_fwnode_get_index_gpiod_from_child() helper Boris Brezillon 1 sibling, 4 replies; 20+ messages in thread From: Boris Brezillon @ 2017-01-30 15:41 UTC (permalink / raw) To: Linus Walleij, Alexandre Courbot, linux-gpio Cc: Dmitry Torokhov, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King, Boris Brezillon Rename devm_get_gpiod_from_child() into devm_fwnode_get_gpiod_from_child() to reflect the fact that this function is operating on a fwnode object. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/gpio/devres.c | 11 ++++++----- drivers/input/keyboard/gpio_keys.c | 3 ++- drivers/input/keyboard/gpio_keys_polled.c | 5 +++-- drivers/leds/leds-gpio.c | 2 +- drivers/video/fbdev/amba-clcd-nomadik.c | 8 ++++---- include/linux/gpio/consumer.h | 8 ++++---- 6 files changed, 20 insertions(+), 17 deletions(-) diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c index b760cbbb41d8..dfbbd92d21b6 100644 --- a/drivers/gpio/devres.c +++ b/drivers/gpio/devres.c @@ -123,7 +123,8 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev, EXPORT_SYMBOL(devm_gpiod_get_index); /** - * devm_get_gpiod_from_child - get a GPIO descriptor from a device's child node + * devm_fwnode_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) @@ -131,9 +132,9 @@ EXPORT_SYMBOL(devm_gpiod_get_index); * 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) +struct gpio_desc *devm_fwnode_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 */ @@ -168,7 +169,7 @@ struct gpio_desc *devm_get_gpiod_from_child(struct device *dev, return desc; } -EXPORT_SYMBOL(devm_get_gpiod_from_child); +EXPORT_SYMBOL(devm_fwnode_get_gpiod_from_child); /** * devm_gpiod_get_index_optional - Resource-managed gpiod_get_index_optional() diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 582462d0af75..ef6813c1f759 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -481,7 +481,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev, spin_lock_init(&bdata->lock); if (child) { - bdata->gpiod = devm_get_gpiod_from_child(dev, NULL, child); + bdata->gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL, + child); if (IS_ERR(bdata->gpiod)) { error = PTR_ERR(bdata->gpiod); if (error == -ENOENT) { diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c index bed4f2086158..c0c9f2133ecd 100644 --- a/drivers/input/keyboard/gpio_keys_polled.c +++ b/drivers/input/keyboard/gpio_keys_polled.c @@ -303,8 +303,9 @@ static int gpio_keys_polled_probe(struct platform_device *pdev) return -EINVAL; } - bdata->gpiod = devm_get_gpiod_from_child(dev, NULL, - child); + bdata->gpiod = devm_fwnode_get_gpiod_from_child(dev, + NULL, + child); if (IS_ERR(bdata->gpiod)) { error = PTR_ERR(bdata->gpiod); if (error != -EPROBE_DEFER) diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c index d400dcaf4d29..c0ef838fc993 100644 --- a/drivers/leds/leds-gpio.c +++ b/drivers/leds/leds-gpio.c @@ -174,7 +174,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev) const char *state = NULL; struct device_node *np = to_of_node(child); - led.gpiod = devm_get_gpiod_from_child(dev, NULL, child); + led.gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL, child); if (IS_ERR(led.gpiod)) { fwnode_handle_put(child); return ERR_CAST(led.gpiod); diff --git a/drivers/video/fbdev/amba-clcd-nomadik.c b/drivers/video/fbdev/amba-clcd-nomadik.c index 0c06fcaaa6e8..a4c58c650f8c 100644 --- a/drivers/video/fbdev/amba-clcd-nomadik.c +++ b/drivers/video/fbdev/amba-clcd-nomadik.c @@ -184,7 +184,7 @@ static void tpg110_init(struct device *dev, struct device_node *np, { dev_info(dev, "TPG110 display init\n"); - grestb = devm_get_gpiod_from_child(dev, "grestb", &np->fwnode); + grestb = devm_fwnode_get_gpiod_from_child(dev, "grestb", &np->fwnode); if (IS_ERR(grestb)) { dev_err(dev, "no GRESTB GPIO\n"); return; @@ -192,19 +192,19 @@ static void tpg110_init(struct device *dev, struct device_node *np, /* This asserts the GRESTB signal, putting the display into reset */ gpiod_direction_output(grestb, 1); - scen = devm_get_gpiod_from_child(dev, "scen", &np->fwnode); + scen = devm_fwnode_get_gpiod_from_child(dev, "scen", &np->fwnode); if (IS_ERR(scen)) { dev_err(dev, "no SCEN GPIO\n"); return; } gpiod_direction_output(scen, 0); - scl = devm_get_gpiod_from_child(dev, "scl", &np->fwnode); + scl = devm_fwnode_get_gpiod_from_child(dev, "scl", &np->fwnode); if (IS_ERR(scl)) { dev_err(dev, "no SCL GPIO\n"); return; } gpiod_direction_output(scl, 0); - sda = devm_get_gpiod_from_child(dev, "sda", &np->fwnode); + sda = devm_fwnode_get_gpiod_from_child(dev, "sda", &np->fwnode); if (IS_ERR(sda)) { dev_err(dev, "no SDA GPIO\n"); return; diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index fb0fde686cb1..2ce4bc164735 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -136,9 +136,9 @@ 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); +struct gpio_desc *devm_fwnode_get_gpiod_from_child(struct device *dev, + const char *con_id, + struct fwnode_handle *child); #else /* CONFIG_GPIOLIB */ static inline int gpiod_count(struct device *dev, const char *con_id) @@ -417,7 +417,7 @@ static inline struct gpio_desc *fwnode_get_named_gpiod( return ERR_PTR(-ENOSYS); } -static inline struct gpio_desc *devm_get_gpiod_from_child( +static inline struct gpio_desc *devm_fwnode_get_gpiod_from_child( struct device *dev, const char *con_id, struct fwnode_handle *child) { return ERR_PTR(-ENOSYS); -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-30 15:41 ` [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon @ 2017-01-30 19:57 ` Jacek Anaszewski 2017-01-31 1:06 ` Dmitry Torokhov ` (2 subsequent siblings) 3 siblings, 0 replies; 20+ messages in thread From: Jacek Anaszewski @ 2017-01-30 19:57 UTC (permalink / raw) To: Boris Brezillon, Linus Walleij, Alexandre Courbot, linux-gpio Cc: Dmitry Torokhov, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On 01/30/2017 04:41 PM, Boris Brezillon wrote: > Rename devm_get_gpiod_from_child() into > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > function is operating on a fwnode object. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/gpio/devres.c | 11 ++++++----- > drivers/input/keyboard/gpio_keys.c | 3 ++- > drivers/input/keyboard/gpio_keys_polled.c | 5 +++-- > drivers/leds/leds-gpio.c | 2 +- > drivers/video/fbdev/amba-clcd-nomadik.c | 8 ++++---- > include/linux/gpio/consumer.h | 8 ++++---- > 6 files changed, 20 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c > index b760cbbb41d8..dfbbd92d21b6 100644 > --- a/drivers/gpio/devres.c > +++ b/drivers/gpio/devres.c > @@ -123,7 +123,8 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev, > EXPORT_SYMBOL(devm_gpiod_get_index); > > /** > - * devm_get_gpiod_from_child - get a GPIO descriptor from a device's child node > + * devm_fwnode_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) > @@ -131,9 +132,9 @@ EXPORT_SYMBOL(devm_gpiod_get_index); > * 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) > +struct gpio_desc *devm_fwnode_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 */ > @@ -168,7 +169,7 @@ struct gpio_desc *devm_get_gpiod_from_child(struct device *dev, > > return desc; > } > -EXPORT_SYMBOL(devm_get_gpiod_from_child); > +EXPORT_SYMBOL(devm_fwnode_get_gpiod_from_child); > > /** > * devm_gpiod_get_index_optional - Resource-managed gpiod_get_index_optional() > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c > index 582462d0af75..ef6813c1f759 100644 > --- a/drivers/input/keyboard/gpio_keys.c > +++ b/drivers/input/keyboard/gpio_keys.c > @@ -481,7 +481,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev, > spin_lock_init(&bdata->lock); > > if (child) { > - bdata->gpiod = devm_get_gpiod_from_child(dev, NULL, child); > + bdata->gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL, > + child); > if (IS_ERR(bdata->gpiod)) { > error = PTR_ERR(bdata->gpiod); > if (error == -ENOENT) { > diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c > index bed4f2086158..c0c9f2133ecd 100644 > --- a/drivers/input/keyboard/gpio_keys_polled.c > +++ b/drivers/input/keyboard/gpio_keys_polled.c > @@ -303,8 +303,9 @@ static int gpio_keys_polled_probe(struct platform_device *pdev) > return -EINVAL; > } > > - bdata->gpiod = devm_get_gpiod_from_child(dev, NULL, > - child); > + bdata->gpiod = devm_fwnode_get_gpiod_from_child(dev, > + NULL, > + child); > if (IS_ERR(bdata->gpiod)) { > error = PTR_ERR(bdata->gpiod); > if (error != -EPROBE_DEFER) > diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c > index d400dcaf4d29..c0ef838fc993 100644 > --- a/drivers/leds/leds-gpio.c > +++ b/drivers/leds/leds-gpio.c > @@ -174,7 +174,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev) > const char *state = NULL; > struct device_node *np = to_of_node(child); > > - led.gpiod = devm_get_gpiod_from_child(dev, NULL, child); > + led.gpiod = devm_fwnode_get_gpiod_from_child(dev, NULL, child); > if (IS_ERR(led.gpiod)) { > fwnode_handle_put(child); > return ERR_CAST(led.gpiod); For leds-gpio: Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > diff --git a/drivers/video/fbdev/amba-clcd-nomadik.c b/drivers/video/fbdev/amba-clcd-nomadik.c > index 0c06fcaaa6e8..a4c58c650f8c 100644 > --- a/drivers/video/fbdev/amba-clcd-nomadik.c > +++ b/drivers/video/fbdev/amba-clcd-nomadik.c > @@ -184,7 +184,7 @@ static void tpg110_init(struct device *dev, struct device_node *np, > { > dev_info(dev, "TPG110 display init\n"); > > - grestb = devm_get_gpiod_from_child(dev, "grestb", &np->fwnode); > + grestb = devm_fwnode_get_gpiod_from_child(dev, "grestb", &np->fwnode); > if (IS_ERR(grestb)) { > dev_err(dev, "no GRESTB GPIO\n"); > return; > @@ -192,19 +192,19 @@ static void tpg110_init(struct device *dev, struct device_node *np, > /* This asserts the GRESTB signal, putting the display into reset */ > gpiod_direction_output(grestb, 1); > > - scen = devm_get_gpiod_from_child(dev, "scen", &np->fwnode); > + scen = devm_fwnode_get_gpiod_from_child(dev, "scen", &np->fwnode); > if (IS_ERR(scen)) { > dev_err(dev, "no SCEN GPIO\n"); > return; > } > gpiod_direction_output(scen, 0); > - scl = devm_get_gpiod_from_child(dev, "scl", &np->fwnode); > + scl = devm_fwnode_get_gpiod_from_child(dev, "scl", &np->fwnode); > if (IS_ERR(scl)) { > dev_err(dev, "no SCL GPIO\n"); > return; > } > gpiod_direction_output(scl, 0); > - sda = devm_get_gpiod_from_child(dev, "sda", &np->fwnode); > + sda = devm_fwnode_get_gpiod_from_child(dev, "sda", &np->fwnode); > if (IS_ERR(sda)) { > dev_err(dev, "no SDA GPIO\n"); > return; > diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h > index fb0fde686cb1..2ce4bc164735 100644 > --- a/include/linux/gpio/consumer.h > +++ b/include/linux/gpio/consumer.h > @@ -136,9 +136,9 @@ 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); > +struct gpio_desc *devm_fwnode_get_gpiod_from_child(struct device *dev, > + const char *con_id, > + struct fwnode_handle *child); > #else /* CONFIG_GPIOLIB */ > > static inline int gpiod_count(struct device *dev, const char *con_id) > @@ -417,7 +417,7 @@ static inline struct gpio_desc *fwnode_get_named_gpiod( > return ERR_PTR(-ENOSYS); > } > > -static inline struct gpio_desc *devm_get_gpiod_from_child( > +static inline struct gpio_desc *devm_fwnode_get_gpiod_from_child( > struct device *dev, const char *con_id, struct fwnode_handle *child) > { > return ERR_PTR(-ENOSYS); > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-30 15:41 ` [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon 2017-01-30 19:57 ` Jacek Anaszewski @ 2017-01-31 1:06 ` Dmitry Torokhov 2017-01-31 8:04 ` Boris Brezillon 2017-02-01 17:17 ` Dmitry Torokhov 2017-02-02 10:53 ` Linus Walleij 3 siblings, 1 reply; 20+ messages in thread From: Dmitry Torokhov @ 2017-01-31 1:06 UTC (permalink / raw) To: Boris Brezillon Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote: > Rename devm_get_gpiod_from_child() into > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > function is operating on a fwnode object. I believe this is completely pointless rename. Are you planning on adding devm_of_get_gpiod_from_child()? Or devm_acpt_get_gpiod_from_child()? (I sure hope not). Also, on what object? Does it take fwnode as first argument? Or maybe we should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we know types of all arguments? Please, no. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-31 1:06 ` Dmitry Torokhov @ 2017-01-31 8:04 ` Boris Brezillon 2017-01-31 8:44 ` Dmitry Torokhov 0 siblings, 1 reply; 20+ messages in thread From: Boris Brezillon @ 2017-01-31 8:04 UTC (permalink / raw) To: Dmitry Torokhov Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Mon, 30 Jan 2017 17:06:07 -0800 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote: > > Rename devm_get_gpiod_from_child() into > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > > function is operating on a fwnode object. > > I believe this is completely pointless rename. Are you planning on > adding devm_of_get_gpiod_from_child()? Or > devm_acpt_get_gpiod_from_child()? (I sure hope not). Of course not. > > Also, on what object? Does it take fwnode as first argument? Or maybe we > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we > know types of all arguments? Linus suggested to rename this function [1]. I personally don't care much about the name, though I agree with Linus that names should be consistent and descriptive. Moreover, he's the maintainer, and I tend to follow maintainers suggestion when I contribute to a specific subsystem. IIUC, you're concerned about the length of this function name. If I had to drop something it would be the _from_child() suffix, because the function is not even checking that the child parameter is actually a direct child (or a descendant) of device->fwnode. Also, if we want to be consistent with the rest of the GPIO API, we could rename it devm_gpiod_get_from_fwnode() (with the function in added in patch 2 renamed into devm_gpiod_get_from_fwnode()). Linus, what do you think? One last thing, I don't want to start a discussion where we're bikeshedding on a function name instead of focusing on the functionality, so if it turns into this kind of discussion I'll probably implement devm_fwnode_get_gpiod_from_child() directly in the atmel NAND driver and wait for an agreement before switching to the official version. Regards, Boris [1]https://www.spinics.net/lists/arm-kernel/msg558986.html ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-31 8:04 ` Boris Brezillon @ 2017-01-31 8:44 ` Dmitry Torokhov 2017-01-31 9:07 ` Boris Brezillon 0 siblings, 1 reply; 20+ messages in thread From: Dmitry Torokhov @ 2017-01-31 8:44 UTC (permalink / raw) To: Boris Brezillon Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote: > On Mon, 30 Jan 2017 17:06:07 -0800 > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote: > > > Rename devm_get_gpiod_from_child() into > > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > > > function is operating on a fwnode object. > > > > I believe this is completely pointless rename. Are you planning on > > adding devm_of_get_gpiod_from_child()? Or > > devm_acpt_get_gpiod_from_child()? (I sure hope not). > > Of course not. > > > > > Also, on what object? Does it take fwnode as first argument? Or maybe we > > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we > > know types of all arguments? > > Linus suggested to rename this function [1]. I personally don't care > much about the name, though I agree with Linus that names should be > consistent and descriptive. Moreover, he's the maintainer, and I tend > to follow maintainers suggestion when I contribute to a specific > subsystem. OK, I did not know that that was Linus' request, my objection still stands. > > IIUC, you're concerned about the length of this function name. If I had > to drop something it would be the _from_child() suffix, because the > function is not even checking that the child parameter is actually a > direct child (or a descendant) of device->fwnode. OK, that sounds better. Actually, we already have fwnode_get_named_gpiod(), unfortunately it does not do suffixes permutations. There are also no users, except devm_get_gpiod_from_child(). So I would: - rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod() - made new fwnode_get_named_gpiod() that did suffix permutation and called __fwnode_get_named_gpiod() (or pulled its implementation inline) - renamed devm_get_gpiod_from_child() -> devm_fwnode_get_named_gpiod(dev, fwnode, con_id) and called fwnode_get_named_gpiod(). This would indeed match the pattern with other fwnode/property handling APIs. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-31 8:44 ` Dmitry Torokhov @ 2017-01-31 9:07 ` Boris Brezillon 2017-01-31 9:11 ` Dmitry Torokhov 0 siblings, 1 reply; 20+ messages in thread From: Boris Brezillon @ 2017-01-31 9:07 UTC (permalink / raw) To: Dmitry Torokhov Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Tue, 31 Jan 2017 00:44:47 -0800 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote: > > On Mon, 30 Jan 2017 17:06:07 -0800 > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote: > > > > Rename devm_get_gpiod_from_child() into > > > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > > > > function is operating on a fwnode object. > > > > > > I believe this is completely pointless rename. Are you planning on > > > adding devm_of_get_gpiod_from_child()? Or > > > devm_acpt_get_gpiod_from_child()? (I sure hope not). > > > > Of course not. > > > > > > > > Also, on what object? Does it take fwnode as first argument? Or maybe we > > > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we > > > know types of all arguments? > > > > Linus suggested to rename this function [1]. I personally don't care > > much about the name, though I agree with Linus that names should be > > consistent and descriptive. Moreover, he's the maintainer, and I tend > > to follow maintainers suggestion when I contribute to a specific > > subsystem. > > OK, I did not know that that was Linus' request, my objection still > stands. > > > > > IIUC, you're concerned about the length of this function name. If I had > > to drop something it would be the _from_child() suffix, because the > > function is not even checking that the child parameter is actually a > > direct child (or a descendant) of device->fwnode. > > OK, that sounds better. Actually, we already have > fwnode_get_named_gpiod(), unfortunately it does not do suffixes > permutations. There are also no users, except > devm_get_gpiod_from_child(). So I would: > > - rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod() > - made new fwnode_get_named_gpiod() that did suffix permutation and > called __fwnode_get_named_gpiod() (or pulled its implementation > inline) Sorry but I don't follow you. Why do you need __fwnode_get_named_gpiod(), and what is the suffix permutation you're mentioning here? > - renamed devm_get_gpiod_from_child() -> > devm_fwnode_get_named_gpiod(dev, fwnode, con_id) > and called fwnode_get_named_gpiod(). Okay. I'm fine with this name, let's see what Linus says. > > This would indeed match the pattern with other fwnode/property handling > APIs. > > Thanks. > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-31 9:07 ` Boris Brezillon @ 2017-01-31 9:11 ` Dmitry Torokhov 2017-01-31 9:24 ` Boris Brezillon 0 siblings, 1 reply; 20+ messages in thread From: Dmitry Torokhov @ 2017-01-31 9:11 UTC (permalink / raw) To: Boris Brezillon Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Tue, Jan 31, 2017 at 10:07:21AM +0100, Boris Brezillon wrote: > On Tue, 31 Jan 2017 00:44:47 -0800 > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote: > > > On Mon, 30 Jan 2017 17:06:07 -0800 > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > > > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote: > > > > > Rename devm_get_gpiod_from_child() into > > > > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > > > > > function is operating on a fwnode object. > > > > > > > > I believe this is completely pointless rename. Are you planning on > > > > adding devm_of_get_gpiod_from_child()? Or > > > > devm_acpt_get_gpiod_from_child()? (I sure hope not). > > > > > > Of course not. > > > > > > > > > > > Also, on what object? Does it take fwnode as first argument? Or maybe we > > > > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we > > > > know types of all arguments? > > > > > > Linus suggested to rename this function [1]. I personally don't care > > > much about the name, though I agree with Linus that names should be > > > consistent and descriptive. Moreover, he's the maintainer, and I tend > > > to follow maintainers suggestion when I contribute to a specific > > > subsystem. > > > > OK, I did not know that that was Linus' request, my objection still > > stands. > > > > > > > > IIUC, you're concerned about the length of this function name. If I had > > > to drop something it would be the _from_child() suffix, because the > > > function is not even checking that the child parameter is actually a > > > direct child (or a descendant) of device->fwnode. > > > > OK, that sounds better. Actually, we already have > > fwnode_get_named_gpiod(), unfortunately it does not do suffixes > > permutations. There are also no users, except > > devm_get_gpiod_from_child(). So I would: > > > > - rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod() > > - made new fwnode_get_named_gpiod() that did suffix permutation and > > called __fwnode_get_named_gpiod() (or pulled its implementation > > inline) > > Sorry but I don't follow you. Why do you need > __fwnode_get_named_gpiod(), You do not need it, it will just reduce size of the patch if you use it. I'd be perfectly fine not with having it and have everything in fwnode_get_named_gpiod(). > and what is the suffix permutation you're > mentioning here? devm_get_gpiod_from_child() tries to apply "-gpio" and "-gpios" suffixes to the supplied GPIO ID while current fwnode_get_named_gpiod() takes property name literally. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-31 9:11 ` Dmitry Torokhov @ 2017-01-31 9:24 ` Boris Brezillon 2017-01-31 18:39 ` Dmitry Torokhov 0 siblings, 1 reply; 20+ messages in thread From: Boris Brezillon @ 2017-01-31 9:24 UTC (permalink / raw) To: Dmitry Torokhov Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Tue, 31 Jan 2017 01:11:55 -0800 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Tue, Jan 31, 2017 at 10:07:21AM +0100, Boris Brezillon wrote: > > On Tue, 31 Jan 2017 00:44:47 -0800 > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote: > > > > On Mon, 30 Jan 2017 17:06:07 -0800 > > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote: > > > > > > Rename devm_get_gpiod_from_child() into > > > > > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > > > > > > function is operating on a fwnode object. > > > > > > > > > > I believe this is completely pointless rename. Are you planning on > > > > > adding devm_of_get_gpiod_from_child()? Or > > > > > devm_acpt_get_gpiod_from_child()? (I sure hope not). > > > > > > > > Of course not. > > > > > > > > > > > > > > Also, on what object? Does it take fwnode as first argument? Or maybe we > > > > > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we > > > > > know types of all arguments? > > > > > > > > Linus suggested to rename this function [1]. I personally don't care > > > > much about the name, though I agree with Linus that names should be > > > > consistent and descriptive. Moreover, he's the maintainer, and I tend > > > > to follow maintainers suggestion when I contribute to a specific > > > > subsystem. > > > > > > OK, I did not know that that was Linus' request, my objection still > > > stands. > > > > > > > > > > > IIUC, you're concerned about the length of this function name. If I had > > > > to drop something it would be the _from_child() suffix, because the > > > > function is not even checking that the child parameter is actually a > > > > direct child (or a descendant) of device->fwnode. > > > > > > OK, that sounds better. Actually, we already have > > > fwnode_get_named_gpiod(), unfortunately it does not do suffixes > > > permutations. There are also no users, except > > > devm_get_gpiod_from_child(). So I would: > > > > > > - rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod() > > > - made new fwnode_get_named_gpiod() that did suffix permutation and > > > called __fwnode_get_named_gpiod() (or pulled its implementation > > > inline) > > > > Sorry but I don't follow you. Why do you need > > __fwnode_get_named_gpiod(), > > You do not need it, it will just reduce size of the patch if you use > it. I'd be perfectly fine not with having it and have everything in > fwnode_get_named_gpiod(). Okay. > > > and what is the suffix permutation you're > > mentioning here? > > devm_get_gpiod_from_child() tries to apply "-gpio" and "-gpios" suffixes > to the supplied GPIO ID while current fwnode_get_named_gpiod() takes > property name literally. fwnode_get_named_gpiod() just mimics what of_get_named_gpiod_flags(), acpi_node_get_gpiod(), of_find_gpio() and acpi_find_gpio() do. It would be weird/inconsistent to have the con_id suffixing logic moved in the fwnode_get_named_gpiod() (if that's what you're suggesting, but I'm not sure it is). ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-31 9:24 ` Boris Brezillon @ 2017-01-31 18:39 ` Dmitry Torokhov 2017-01-31 19:42 ` Boris Brezillon 0 siblings, 1 reply; 20+ messages in thread From: Dmitry Torokhov @ 2017-01-31 18:39 UTC (permalink / raw) To: Boris Brezillon Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Tue, Jan 31, 2017 at 10:24:24AM +0100, Boris Brezillon wrote: > On Tue, 31 Jan 2017 01:11:55 -0800 > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > On Tue, Jan 31, 2017 at 10:07:21AM +0100, Boris Brezillon wrote: > > > On Tue, 31 Jan 2017 00:44:47 -0800 > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > > > On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote: > > > > > On Mon, 30 Jan 2017 17:06:07 -0800 > > > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > > > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote: > > > > > > > Rename devm_get_gpiod_from_child() into > > > > > > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > > > > > > > function is operating on a fwnode object. > > > > > > > > > > > > I believe this is completely pointless rename. Are you planning on > > > > > > adding devm_of_get_gpiod_from_child()? Or > > > > > > devm_acpt_get_gpiod_from_child()? (I sure hope not). > > > > > > > > > > Of course not. > > > > > > > > > > > > > > > > > Also, on what object? Does it take fwnode as first argument? Or maybe we > > > > > > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we > > > > > > know types of all arguments? > > > > > > > > > > Linus suggested to rename this function [1]. I personally don't care > > > > > much about the name, though I agree with Linus that names should be > > > > > consistent and descriptive. Moreover, he's the maintainer, and I tend > > > > > to follow maintainers suggestion when I contribute to a specific > > > > > subsystem. > > > > > > > > OK, I did not know that that was Linus' request, my objection still > > > > stands. > > > > > > > > > > > > > > IIUC, you're concerned about the length of this function name. If I had > > > > > to drop something it would be the _from_child() suffix, because the > > > > > function is not even checking that the child parameter is actually a > > > > > direct child (or a descendant) of device->fwnode. > > > > > > > > OK, that sounds better. Actually, we already have > > > > fwnode_get_named_gpiod(), unfortunately it does not do suffixes > > > > permutations. There are also no users, except > > > > devm_get_gpiod_from_child(). So I would: > > > > > > > > - rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod() > > > > - made new fwnode_get_named_gpiod() that did suffix permutation and > > > > called __fwnode_get_named_gpiod() (or pulled its implementation > > > > inline) > > > > > > Sorry but I don't follow you. Why do you need > > > __fwnode_get_named_gpiod(), > > > > You do not need it, it will just reduce size of the patch if you use > > it. I'd be perfectly fine not with having it and have everything in > > fwnode_get_named_gpiod(). > > Okay. > > > > > > and what is the suffix permutation you're > > > mentioning here? > > > > devm_get_gpiod_from_child() tries to apply "-gpio" and "-gpios" suffixes > > to the supplied GPIO ID while current fwnode_get_named_gpiod() takes > > property name literally. > > fwnode_get_named_gpiod() just mimics what of_get_named_gpiod_flags(), > acpi_node_get_gpiod(), of_find_gpio() and acpi_find_gpio() do. It would > be weird/inconsistent to have the con_id suffixing logic moved in the > fwnode_get_named_gpiod() (if that's what you're suggesting, but I'm not > sure it is). Hmm, yeah, I agree, that would be weird. Then let's leave devm_get_gpiod_from_child() as is ;) -- Dmitry ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-31 18:39 ` Dmitry Torokhov @ 2017-01-31 19:42 ` Boris Brezillon 2017-02-01 13:05 ` Linus Walleij 0 siblings, 1 reply; 20+ messages in thread From: Boris Brezillon @ 2017-01-31 19:42 UTC (permalink / raw) To: Dmitry Torokhov, Linus Walleij Cc: Alexandre Courbot, linux-gpio, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Tue, 31 Jan 2017 10:39:36 -0800 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Tue, Jan 31, 2017 at 10:24:24AM +0100, Boris Brezillon wrote: > > On Tue, 31 Jan 2017 01:11:55 -0800 > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > On Tue, Jan 31, 2017 at 10:07:21AM +0100, Boris Brezillon wrote: > > > > On Tue, 31 Jan 2017 00:44:47 -0800 > > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > On Tue, Jan 31, 2017 at 09:04:32AM +0100, Boris Brezillon wrote: > > > > > > On Mon, 30 Jan 2017 17:06:07 -0800 > > > > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > > > > > > > > > > > > On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote: > > > > > > > > Rename devm_get_gpiod_from_child() into > > > > > > > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > > > > > > > > function is operating on a fwnode object. > > > > > > > > > > > > > > I believe this is completely pointless rename. Are you planning on > > > > > > > adding devm_of_get_gpiod_from_child()? Or > > > > > > > devm_acpt_get_gpiod_from_child()? (I sure hope not). > > > > > > > > > > > > Of course not. > > > > > > > > > > > > > > > > > > > > Also, on what object? Does it take fwnode as first argument? Or maybe we > > > > > > > should call it devm_dev_const_charp_fwnode_get_gpiod_from_child() so we > > > > > > > know types of all arguments? > > > > > > > > > > > > Linus suggested to rename this function [1]. I personally don't care > > > > > > much about the name, though I agree with Linus that names should be > > > > > > consistent and descriptive. Moreover, he's the maintainer, and I tend > > > > > > to follow maintainers suggestion when I contribute to a specific > > > > > > subsystem. > > > > > > > > > > OK, I did not know that that was Linus' request, my objection still > > > > > stands. > > > > > > > > > > > > > > > > > IIUC, you're concerned about the length of this function name. If I had > > > > > > to drop something it would be the _from_child() suffix, because the > > > > > > function is not even checking that the child parameter is actually a > > > > > > direct child (or a descendant) of device->fwnode. > > > > > > > > > > OK, that sounds better. Actually, we already have > > > > > fwnode_get_named_gpiod(), unfortunately it does not do suffixes > > > > > permutations. There are also no users, except > > > > > devm_get_gpiod_from_child(). So I would: > > > > > > > > > > - rename fwnode_get_named_gpiod() -> static __fwnode_get_named_gpiod() > > > > > - made new fwnode_get_named_gpiod() that did suffix permutation and > > > > > called __fwnode_get_named_gpiod() (or pulled its implementation > > > > > inline) > > > > > > > > Sorry but I don't follow you. Why do you need > > > > __fwnode_get_named_gpiod(), > > > > > > You do not need it, it will just reduce size of the patch if you use > > > it. I'd be perfectly fine not with having it and have everything in > > > fwnode_get_named_gpiod(). > > > > Okay. > > > > > > > > > and what is the suffix permutation you're > > > > mentioning here? > > > > > > devm_get_gpiod_from_child() tries to apply "-gpio" and "-gpios" suffixes > > > to the supplied GPIO ID while current fwnode_get_named_gpiod() takes > > > property name literally. > > > > fwnode_get_named_gpiod() just mimics what of_get_named_gpiod_flags(), > > acpi_node_get_gpiod(), of_find_gpio() and acpi_find_gpio() do. It would > > be weird/inconsistent to have the con_id suffixing logic moved in the > > fwnode_get_named_gpiod() (if that's what you're suggesting, but I'm not > > sure it is). > > Hmm, yeah, I agree, that would be weird. Then let's leave > devm_get_gpiod_from_child() as is ;) Changing the internal implementation has never been the goal of this patch. As explained in the commit log, I'm just renaming the function to make it consistent with other fwnode functions (as suggested by Linus). What's happening here is exactly the kind of discussion I wanted to avoid, and the reason I decided to not change the devm_get_gpiod_from_child() prototype/name in the first place. Linus, is this something you really care about? If that's the case, can you step in? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-31 19:42 ` Boris Brezillon @ 2017-02-01 13:05 ` Linus Walleij 2017-02-01 13:22 ` Boris Brezillon 0 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2017-02-01 13:05 UTC (permalink / raw) To: Boris Brezillon Cc: Dmitry Torokhov, Alexandre Courbot, linux-gpio, Linux Input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Tue, Jan 31, 2017 at 8:42 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Tue, 31 Jan 2017 10:39:36 -0800 > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: >> Hmm, yeah, I agree, that would be weird. Then let's leave >> devm_get_gpiod_from_child() as is ;) > > Changing the internal implementation has never been the goal of this > patch. As explained in the commit log, I'm just renaming the function > to make it consistent with other fwnode functions (as suggested by > Linus). > What's happening here is exactly the kind of discussion I wanted to > avoid, and the reason I decided to not change the > devm_get_gpiod_from_child() prototype/name in the first place. > > Linus, is this something you really care about? If that's the case, can > you step in? I can only throw up my hands... The way I percieved it, a new function was added, but I guess it could be that the diffstat was so convoluted in the other patch (by the way that diff sometimes give very confusing stuff unless you use the right fuzz) so I misunderstood some other renaming as introducing a new function. Please drop the patch if it is controversial. The name of the function *is* confusing though but maybe it's not the biggest problem in the world. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-02-01 13:05 ` Linus Walleij @ 2017-02-01 13:22 ` Boris Brezillon 2017-02-01 14:51 ` Linus Walleij 0 siblings, 1 reply; 20+ messages in thread From: Boris Brezillon @ 2017-02-01 13:22 UTC (permalink / raw) To: Linus Walleij Cc: Dmitry Torokhov, Alexandre Courbot, linux-gpio, Linux Input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King Hi Linus, On Wed, 1 Feb 2017 14:05:43 +0100 Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Jan 31, 2017 at 8:42 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Tue, 31 Jan 2017 10:39:36 -0800 > > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > >> Hmm, yeah, I agree, that would be weird. Then let's leave > >> devm_get_gpiod_from_child() as is ;) > > > > Changing the internal implementation has never been the goal of this > > patch. As explained in the commit log, I'm just renaming the function > > to make it consistent with other fwnode functions (as suggested by > > Linus). > > What's happening here is exactly the kind of discussion I wanted to > > avoid, and the reason I decided to not change the > > devm_get_gpiod_from_child() prototype/name in the first place. > > > > Linus, is this something you really care about? If that's the case, can > > you step in? > > I can only throw up my hands... Sorry for forcing your hand like this, but this is the kind of discussion I'm not comfortable with (when I need to argue on something I'm not completely convinced of, or I don't have opinion on). > The way I percieved it, a new function > was added, but I guess it could be that the diffstat was so > convoluted in the other patch (by the way that diff sometimes give > very confusing stuff unless you use the right fuzz) so I misunderstood > some other renaming as introducing a new function. Indeed, a new function is added (see patch 2), and this new function is taking an additional 'index' parameter. If that's a problem, I can also change the prototype of devm_get_gpiod_from_child() and patch all existing users of this function, but I fear we'll end up with pretty much the same discussion :-/. > > Please drop the patch if it is controversial. > > The name of the function *is* confusing though but maybe it's not > the biggest problem in the world. I can still name the new function as you suggested (devm_fwnode_get_index_gpiod_from_child()), and keep the existing one unchanged if you want. Just let me know what you prefer. Thanks, Boris ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-02-01 13:22 ` Boris Brezillon @ 2017-02-01 14:51 ` Linus Walleij 2017-02-01 17:18 ` Dmitry Torokhov 2017-02-02 10:07 ` Mika Westerberg 0 siblings, 2 replies; 20+ messages in thread From: Linus Walleij @ 2017-02-01 14:51 UTC (permalink / raw) To: Boris Brezillon, Mika Westerberg, Alexandre Courbot Cc: Dmitry Torokhov, linux-gpio, Linux Input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Wed, Feb 1, 2017 at 2:22 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > On Wed, 1 Feb 2017 14:05:43 +0100 > Linus Walleij <linus.walleij@linaro.org> wrote: >> > Linus, is this something you really care about? If that's the case, can >> > you step in? >> >> I can only throw up my hands... > > Sorry for forcing your hand like this, but this is the kind of > discussion I'm not comfortable with (when I need to argue on something > I'm not completely convinced of, or I don't have opinion on). Sorry, I'm just too stressed by all patches. I now read back on the context below. >> The way I percieved it, a new function >> was added, but I guess it could be that the diffstat was so >> convoluted in the other patch (by the way that diff sometimes give >> very confusing stuff unless you use the right fuzz) so I misunderstood >> some other renaming as introducing a new function. > > Indeed, a new function is added (see patch 2), and this new function is > taking an additional 'index' parameter. If that's a problem, I can also > change the prototype of devm_get_gpiod_from_child() and patch all > existing users of this function, but I fear we'll end up with pretty > much the same discussion :-/. Yeah. >> Please drop the patch if it is controversial. >> >> The name of the function *is* confusing though but maybe it's not >> the biggest problem in the world. > > I can still name the new function as you suggested > (devm_fwnode_get_index_gpiod_from_child()), and keep the existing one > unchanged if you want. But that is just insane. Then it is just better to apply this and the other patch making the situation manageable. This is a good time to do it too since I'm anyways patching around in all the consumers this merge window. Dmitry: is this such a big deal to you? commit 40b7318319281b1bdec804f6435f26cadd329c13 "gpio: Support for unified device properties interface" by Mika Westerberg introduced fwnode_get_named_gpiod() devm_get_gpiod_from_child() Both are taking a fwnode as argument and the naming is as inconsistent as it can be. Some more churn should be expected as a side effect of naming this function wrong in the first place. The fwnode API change was on fast-forward and mistakes were made, also by me, mea culpa. When I write kernel code, I usually intuitively look for a function doing what I want, this naming is unintuitive, and it has confused me so it will confuse others. Can I please apply these two patches? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-02-01 14:51 ` Linus Walleij @ 2017-02-01 17:18 ` Dmitry Torokhov 2017-02-02 10:07 ` Mika Westerberg 1 sibling, 0 replies; 20+ messages in thread From: Dmitry Torokhov @ 2017-02-01 17:18 UTC (permalink / raw) To: Linus Walleij Cc: Boris Brezillon, Mika Westerberg, Alexandre Courbot, linux-gpio, Linux Input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Wed, Feb 01, 2017 at 03:51:06PM +0100, Linus Walleij wrote: > On Wed, Feb 1, 2017 at 2:22 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > On Wed, 1 Feb 2017 14:05:43 +0100 > > Linus Walleij <linus.walleij@linaro.org> wrote: > > >> > Linus, is this something you really care about? If that's the case, can > >> > you step in? > >> > >> I can only throw up my hands... > > > > Sorry for forcing your hand like this, but this is the kind of > > discussion I'm not comfortable with (when I need to argue on something > > I'm not completely convinced of, or I don't have opinion on). > > Sorry, I'm just too stressed by all patches. I now read back on the > context below. > > >> The way I percieved it, a new function > >> was added, but I guess it could be that the diffstat was so > >> convoluted in the other patch (by the way that diff sometimes give > >> very confusing stuff unless you use the right fuzz) so I misunderstood > >> some other renaming as introducing a new function. > > > > Indeed, a new function is added (see patch 2), and this new function is > > taking an additional 'index' parameter. If that's a problem, I can also > > change the prototype of devm_get_gpiod_from_child() and patch all > > existing users of this function, but I fear we'll end up with pretty > > much the same discussion :-/. > > Yeah. > > >> Please drop the patch if it is controversial. > >> > >> The name of the function *is* confusing though but maybe it's not > >> the biggest problem in the world. > > > > I can still name the new function as you suggested > > (devm_fwnode_get_index_gpiod_from_child()), and keep the existing one > > unchanged if you want. > > But that is just insane. Then it is just better to apply this and the > other patch making the situation manageable. > > This is a good time to do it too since I'm anyways patching around > in all the consumers this merge window. > > Dmitry: is this such a big deal to you? No, not really. But sometimes it is soooo hard to pass on some bikeshedding opportunity ;) > > commit 40b7318319281b1bdec804f6435f26cadd329c13 > "gpio: Support for unified device properties interface" > > by Mika Westerberg introduced > > fwnode_get_named_gpiod() > devm_get_gpiod_from_child() > > Both are taking a fwnode as argument and the naming is as > inconsistent as it can be. > > Some more churn should be expected as a side > effect of naming this function wrong in the first place. > The fwnode API change was on fast-forward and mistakes > were made, also by me, mea culpa. > > When I write kernel code, I usually intuitively look for a function doing > what I want, this naming is unintuitive, and it has confused me so > it will confuse others. > > Can I please apply these two patches? You have my ack for the input bits. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-02-01 14:51 ` Linus Walleij 2017-02-01 17:18 ` Dmitry Torokhov @ 2017-02-02 10:07 ` Mika Westerberg 1 sibling, 0 replies; 20+ messages in thread From: Mika Westerberg @ 2017-02-02 10:07 UTC (permalink / raw) To: Linus Walleij Cc: Boris Brezillon, Alexandre Courbot, Dmitry Torokhov, linux-gpio, Linux Input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Wed, Feb 01, 2017 at 03:51:06PM +0100, Linus Walleij wrote: > fwnode_get_named_gpiod() > devm_get_gpiod_from_child() > > Both are taking a fwnode as argument and the naming is as > inconsistent as it can be. > > Some more churn should be expected as a side > effect of naming this function wrong in the first place. > The fwnode API change was on fast-forward and mistakes > were made, also by me, mea culpa. The name fwnode_get_named_gpiod() tries to follow of_get_named_gpio() so that we can easily convert a driver to use fwnodes instead. The other function is named so because we look child fwnodes under a device. For that, yes certainly we could have invented a better name ;-) ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-30 15:41 ` [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon 2017-01-30 19:57 ` Jacek Anaszewski 2017-01-31 1:06 ` Dmitry Torokhov @ 2017-02-01 17:17 ` Dmitry Torokhov 2017-02-02 10:53 ` Linus Walleij 3 siblings, 0 replies; 20+ messages in thread From: Dmitry Torokhov @ 2017-02-01 17:17 UTC (permalink / raw) To: Boris Brezillon Cc: Linus Walleij, Alexandre Courbot, linux-gpio, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Mon, Jan 30, 2017 at 04:41:48PM +0100, Boris Brezillon wrote: > Rename devm_get_gpiod_from_child() into > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > function is operating on a fwnode object. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/gpio/devres.c | 11 ++++++----- > drivers/input/keyboard/gpio_keys.c | 3 ++- > drivers/input/keyboard/gpio_keys_polled.c | 5 +++-- Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> for input bits. -- Dmitry ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-01-30 15:41 ` [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon ` (2 preceding siblings ...) 2017-02-01 17:17 ` Dmitry Torokhov @ 2017-02-02 10:53 ` Linus Walleij 2017-02-02 11:53 ` Boris Brezillon 3 siblings, 1 reply; 20+ messages in thread From: Linus Walleij @ 2017-02-02 10:53 UTC (permalink / raw) To: Boris Brezillon Cc: Alexandre Courbot, linux-gpio, Dmitry Torokhov, Linux Input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Mon, Jan 30, 2017 at 4:41 PM, Boris Brezillon <boris.brezillon@free-electrons.com> wrote: > Rename devm_get_gpiod_from_child() into > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > function is operating on a fwnode object. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> All right! So we settled we're gonna merge this and the other patch and we have the necessary ACKs. This is unfortunately not applying because I have a bunch of other patches touching the same files (that's why I think it's a good opportunity to rename it now, as mentioned IIRC). Could you rebase this on the following branch: https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/log/?h=ib-gpiod-flags Then I will apply it on top of that immutable branch and pull it into the devel branch of gpio so that we get it in and also have an immutable branch with all changes if some other subsystem maintainer needs it because of clashes. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() 2017-02-02 10:53 ` Linus Walleij @ 2017-02-02 11:53 ` Boris Brezillon 0 siblings, 0 replies; 20+ messages in thread From: Boris Brezillon @ 2017-02-02 11:53 UTC (permalink / raw) To: Linus Walleij Cc: Alexandre Courbot, linux-gpio, Dmitry Torokhov, Linux Input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King On Thu, 2 Feb 2017 11:53:17 +0100 Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Jan 30, 2017 at 4:41 PM, Boris Brezillon > <boris.brezillon@free-electrons.com> wrote: > > > Rename devm_get_gpiod_from_child() into > > devm_fwnode_get_gpiod_from_child() to reflect the fact that this > > function is operating on a fwnode object. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > All right! So we settled we're gonna merge this and the other patch and > we have the necessary ACKs. > > This is unfortunately not applying because I have a bunch of other patches > touching the same files (that's why I think it's a good opportunity to > rename it now, as mentioned IIRC). > > Could you rebase this on the following branch: > https://git.kernel.org/cgit/linux/kernel/git/linusw/linux-gpio.git/log/?h=ib-gpiod-flags While rebasing I noticed that you forgot to add the label parameter to the kernel doc header in commit b2987d7438e0 ("gpio: Pass GPIO label down to gpiod_request"). > > Then I will apply it on top of that immutable branch and pull it into the > devel branch of gpio so that we get it in and also have an immutable > branch with all changes if some other subsystem maintainer needs it > because of clashes. > > Yours, > Linus Walleij ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/2] gpio: Add the devm_fwnode_get_index_gpiod_from_child() helper 2017-01-30 15:41 [PATCH 0/2] gpio: Add the devm_fwnode_get_index_gpiod_from_child() helper Boris Brezillon 2017-01-30 15:41 ` [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon @ 2017-01-30 15:41 ` Boris Brezillon 1 sibling, 0 replies; 20+ messages in thread From: Boris Brezillon @ 2017-01-30 15:41 UTC (permalink / raw) To: Linus Walleij, Alexandre Courbot, linux-gpio Cc: Dmitry Torokhov, linux-input, Bryan Wu, Richard Purdie, Jacek Anaszewski, linux-leds, Tomi Valkeinen, linux-fbdev, linux-kernel, Russell King, Boris Brezillon devm_fwnode_get_gpiod_from_child() currently allows GPIO users to request a GPIO that is defined in a child fwnode instead of directly in the device fwnode. Extend this API by adding the devm_fwnode_get_index_gpiod_from_child() helper which does the same except you can also specify an index in case the 'xx-gpios' property describe several GPIOs. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/gpio/devres.c | 15 ++++++++------- drivers/gpio/gpiolib.c | 9 +++++---- include/linux/gpio/consumer.h | 21 +++++++++++++++------ 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c index dfbbd92d21b6..be5f947570db 100644 --- a/drivers/gpio/devres.c +++ b/drivers/gpio/devres.c @@ -123,18 +123,19 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev, EXPORT_SYMBOL(devm_gpiod_get_index); /** - * devm_fwnode_get_gpiod_from_child - get a GPIO descriptor from a device's - * child node + * devm_fwnode_get_index_gpiod_from_child - get a GPIO descriptor from a + * device's child node * @dev: GPIO consumer * @con_id: function within the GPIO consumer + * @index: index of the GPIO to obtain in the consumer * @child: firmware node (child of @dev) * * GPIO descriptors returned from this function are automatically disposed on * driver detach. */ -struct gpio_desc *devm_fwnode_get_gpiod_from_child(struct device *dev, - const char *con_id, - struct fwnode_handle *child) +struct gpio_desc *devm_fwnode_get_index_gpiod_from_child(struct device *dev, + const char *con_id, int index, + struct fwnode_handle *child) { static const char * const suffixes[] = { "gpios", "gpio" }; char prop_name[32]; /* 32 is max size of property name */ @@ -155,7 +156,7 @@ struct gpio_desc *devm_fwnode_get_gpiod_from_child(struct device *dev, snprintf(prop_name, sizeof(prop_name), "%s", suffixes[i]); - desc = fwnode_get_named_gpiod(child, prop_name); + desc = fwnode_get_named_gpiod(child, prop_name, index); if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT)) break; } @@ -169,7 +170,7 @@ struct gpio_desc *devm_fwnode_get_gpiod_from_child(struct device *dev, return desc; } -EXPORT_SYMBOL(devm_fwnode_get_gpiod_from_child); +EXPORT_SYMBOL(devm_fwnode_get_index_gpiod_from_child); /** * devm_gpiod_get_index_optional - Resource-managed gpiod_get_index_optional() diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 86bf3b84ada5..9b7a7734e742 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -3309,6 +3309,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_index); * fwnode_get_named_gpiod - obtain a GPIO from firmware node * @fwnode: handle of the firmware node * @propname: name of the firmware property representing the GPIO + * @index: index of the GPIO to obtain in the consumer * * This function can be used for drivers that get their configuration * from firmware. @@ -3320,7 +3321,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_index); * In case of error an ERR_PTR() is returned. */ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode, - const char *propname) + const char *propname, int index) { struct gpio_desc *desc = ERR_PTR(-ENODEV); bool active_low = false; @@ -3333,8 +3334,8 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode, if (is_of_node(fwnode)) { enum of_gpio_flags flags; - desc = of_get_named_gpiod_flags(to_of_node(fwnode), propname, 0, - &flags); + desc = of_get_named_gpiod_flags(to_of_node(fwnode), propname, + index, &flags); if (!IS_ERR(desc)) { active_low = flags & OF_GPIO_ACTIVE_LOW; single_ended = flags & OF_GPIO_SINGLE_ENDED; @@ -3342,7 +3343,7 @@ struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode, } else if (is_acpi_node(fwnode)) { struct acpi_gpio_info info; - desc = acpi_node_get_gpiod(fwnode, propname, 0, &info); + desc = acpi_node_get_gpiod(fwnode, propname, index, &info); if (!IS_ERR(desc)) active_low = info.polarity == GPIO_ACTIVE_LOW; } diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index 2ce4bc164735..5d1125198fd7 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -135,10 +135,11 @@ int desc_to_gpio(const struct gpio_desc *desc); struct fwnode_handle; struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode, - const char *propname); -struct gpio_desc *devm_fwnode_get_gpiod_from_child(struct device *dev, - const char *con_id, + const char *propname, int index); +struct gpio_desc *devm_fwnode_get_index_gpiod_from_child(struct device *dev, + const char *con_id, int index, struct fwnode_handle *child); + #else /* CONFIG_GPIOLIB */ static inline int gpiod_count(struct device *dev, const char *con_id) @@ -412,19 +413,27 @@ static inline int desc_to_gpio(const struct gpio_desc *desc) struct fwnode_handle; static inline struct gpio_desc *fwnode_get_named_gpiod( - struct fwnode_handle *fwnode, const char *propname) + struct fwnode_handle *fwnode, const char *propname, int index) { return ERR_PTR(-ENOSYS); } -static inline struct gpio_desc *devm_fwnode_get_gpiod_from_child( - struct device *dev, const char *con_id, struct fwnode_handle *child) +static inline struct gpio_desc *devm_fwnode_get_index_gpiod_from_child( + struct device *dev, const char *con_id, int index, + struct fwnode_handle *child) { return ERR_PTR(-ENOSYS); } #endif /* CONFIG_GPIOLIB */ +static inline struct gpio_desc * +devm_fwnode_get_gpiod_from_child(struct device *dev, const char *con_id, + struct fwnode_handle *child) +{ + return devm_fwnode_get_index_gpiod_from_child(dev, con_id, 0, child); +} + #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_GPIO_SYSFS) int gpiod_export(struct gpio_desc *desc, bool direction_may_change); -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2017-02-02 11:53 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-30 15:41 [PATCH 0/2] gpio: Add the devm_fwnode_get_index_gpiod_from_child() helper Boris Brezillon 2017-01-30 15:41 ` [PATCH 1/2] gpio: Rename devm_get_gpiod_from_child() Boris Brezillon 2017-01-30 19:57 ` Jacek Anaszewski 2017-01-31 1:06 ` Dmitry Torokhov 2017-01-31 8:04 ` Boris Brezillon 2017-01-31 8:44 ` Dmitry Torokhov 2017-01-31 9:07 ` Boris Brezillon 2017-01-31 9:11 ` Dmitry Torokhov 2017-01-31 9:24 ` Boris Brezillon 2017-01-31 18:39 ` Dmitry Torokhov 2017-01-31 19:42 ` Boris Brezillon 2017-02-01 13:05 ` Linus Walleij 2017-02-01 13:22 ` Boris Brezillon 2017-02-01 14:51 ` Linus Walleij 2017-02-01 17:18 ` Dmitry Torokhov 2017-02-02 10:07 ` Mika Westerberg 2017-02-01 17:17 ` Dmitry Torokhov 2017-02-02 10:53 ` Linus Walleij 2017-02-02 11:53 ` Boris Brezillon 2017-01-30 15:41 ` [PATCH 2/2] gpio: Add the devm_fwnode_get_index_gpiod_from_child() helper Boris Brezillon
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).