| Message ID | 1485790909-2915-2-git-send-email-boris.brezillon@free-electrons.com |
|---|---|
| State | New |
| Headers | show |
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); >
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.
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 -- 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, 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.
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. > -- 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, 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.
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). -- 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, 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 ;)
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? -- 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, 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 -- 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 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 -- 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, 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 -- 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 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.
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.
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 ;-) -- 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 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 -- 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, 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 -- 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 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);
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(-)