[02/11] gpiolib: introduce devm_fwnode_gpiod_get_index()
diff mbox series

Message ID 20190911075215.78047-3-dmitry.torokhov@gmail.com
State New
Headers show
Series
  • Add support for software nodes to gpiolib
Related show

Commit Message

Dmitry Torokhov Sept. 11, 2019, 7:52 a.m. UTC
devm_fwnode_get_index_gpiod_from_child() is too long, besides the fwnode
in question does not have to be a child of device node. Let's rename it
to devm_fwnode_gpiod_get_index() and keep the old name for compatibility
for now.

Also let's add a devm_fwnode_gpiod_get() wrapper as majority of the
callers need a single GPIO.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

 drivers/gpio/gpiolib-devres.c | 19 ++++++++--------
 include/linux/gpio/consumer.h | 41 ++++++++++++++++++++++++++---------
 2 files changed, 40 insertions(+), 20 deletions(-)

Comments

Andy Shevchenko Sept. 11, 2019, 5:01 p.m. UTC | #1
On Wed, Sep 11, 2019 at 12:52:06AM -0700, Dmitry Torokhov wrote:
> devm_fwnode_get_index_gpiod_from_child() is too long, besides the fwnode
> in question does not have to be a child of device node. Let's rename it
> to devm_fwnode_gpiod_get_index() and keep the old name for compatibility
> for now.
> 
> Also let's add a devm_fwnode_gpiod_get() wrapper as majority of the
> callers need a single GPIO.

> +	return devm_fwnode_gpiod_get_index(dev, fwnode, con_id, 0,
> +					   flags, label);

At least one parameter can fit previous line, but taking into consideration
that moving second one makes it 81 character long, I would do it completely on
one line. I don't remember Linus' preferences.

> +}

> +	return devm_fwnode_gpiod_get_index(dev, child, con_id, index,
> +					   flags, label);

Ditto.
Linus Walleij Sept. 12, 2019, 9:35 a.m. UTC | #2
On Wed, Sep 11, 2019 at 6:01 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, Sep 11, 2019 at 12:52:06AM -0700, Dmitry Torokhov wrote:
> > devm_fwnode_get_index_gpiod_from_child() is too long, besides the fwnode
> > in question does not have to be a child of device node. Let's rename it
> > to devm_fwnode_gpiod_get_index() and keep the old name for compatibility
> > for now.
> >
> > Also let's add a devm_fwnode_gpiod_get() wrapper as majority of the
> > callers need a single GPIO.
>
> > +     return devm_fwnode_gpiod_get_index(dev, fwnode, con_id, 0,
> > +                                        flags, label);
>
> At least one parameter can fit previous line, but taking into consideration
> that moving second one makes it 81 character long, I would do it completely on
> one line. I don't remember Linus' preferences.

I don't really have one, I don't mind 80+ chars, I don't mind breaking lines
to avoid it.

Yours,
Linus Walleij
Linus Walleij Sept. 12, 2019, 9:48 a.m. UTC | #3
On Wed, Sep 11, 2019 at 8:52 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> devm_fwnode_get_index_gpiod_from_child() is too long, besides the fwnode
> in question does not have to be a child of device node. Let's rename it
> to devm_fwnode_gpiod_get_index() and keep the old name for compatibility
> for now.
>
> Also let's add a devm_fwnode_gpiod_get() wrapper as majority of the
> callers need a single GPIO.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

The patch is good because this is in line with Rusty Russells API
manifesto:

7. The obvious use is (probably) the correct one.
6. The name tells you how to use it.

It doesn't apply to my "devel" branch as of now:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=devel

If you rebase this and the next patch and send them separately I
am willing to apply them already for v5.4 to easy your refactoring
work during the v5.5 cycle here, provided we try to fix up the old users
ASAP and delete the compatibility fallbacks in the near future.

Yours,
Linus Walleij
Dmitry Torokhov Sept. 13, 2019, 6:14 p.m. UTC | #4
On Thu, Sep 12, 2019 at 10:48:13AM +0100, Linus Walleij wrote:
> On Wed, Sep 11, 2019 at 8:52 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> 
> > devm_fwnode_get_index_gpiod_from_child() is too long, besides the fwnode
> > in question does not have to be a child of device node. Let's rename it
> > to devm_fwnode_gpiod_get_index() and keep the old name for compatibility
> > for now.
> >
> > Also let's add a devm_fwnode_gpiod_get() wrapper as majority of the
> > callers need a single GPIO.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> The patch is good because this is in line with Rusty Russells API
> manifesto:
> 
> 7. The obvious use is (probably) the correct one.
> 6. The name tells you how to use it.
> 
> It doesn't apply to my "devel" branch as of now:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=devel
> 
> If you rebase this and the next patch and send them separately I
> am willing to apply them already for v5.4 to easy your refactoring
> work during the v5.5 cycle here, provided we try to fix up the old users
> ASAP and delete the compatibility fallbacks in the near future.

Done.

Thanks.

Patch
diff mbox series

diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index 0acc2cc6e868..190ce7c43b4e 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -185,12 +185,11 @@  struct gpio_desc *devm_gpiod_get_from_of_node(struct device *dev,
 EXPORT_SYMBOL(devm_gpiod_get_from_of_node);
 
 /**
- * devm_fwnode_get_index_gpiod_from_child - get a GPIO descriptor from a
- *					    device's child node
+ * devm_fwnode_gpiod_get_index - get a GPIO descriptor from a given node
  * @dev:	GPIO consumer
+ * @fwnode:	firmware node containing GPIO reference
  * @con_id:	function within the GPIO consumer
  * @index:	index of the GPIO to obtain in the consumer
- * @child:	firmware node (child of @dev)
  * @flags:	GPIO initialization flags
  * @label:	label to attach to the requested GPIO
  *
@@ -200,11 +199,11 @@  EXPORT_SYMBOL(devm_gpiod_get_from_of_node);
  * On successful request the GPIO pin is configured in accordance with
  * provided @flags.
  */
-struct gpio_desc *devm_fwnode_get_index_gpiod_from_child(struct device *dev,
-						const char *con_id, int index,
-						struct fwnode_handle *child,
-						enum gpiod_flags flags,
-						const char *label)
+struct gpio_desc *devm_fwnode_gpiod_get_index(struct device *dev,
+					      struct fwnode_handle *fwnode,
+					      const char *con_id, int index,
+					      enum gpiod_flags flags,
+					      const char *label)
 {
 	char prop_name[32]; /* 32 is max size of property name */
 	struct gpio_desc **dr;
@@ -224,7 +223,7 @@  struct gpio_desc *devm_fwnode_get_index_gpiod_from_child(struct device *dev,
 			snprintf(prop_name, sizeof(prop_name), "%s",
 					    gpio_suffixes[i]);
 
-		desc = fwnode_get_named_gpiod(child, prop_name, index, flags,
+		desc = fwnode_get_named_gpiod(fwnode, prop_name, index, flags,
 					      label);
 		if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT))
 			break;
@@ -239,7 +238,7 @@  struct gpio_desc *devm_fwnode_get_index_gpiod_from_child(struct device *dev,
 
 	return desc;
 }
-EXPORT_SYMBOL(devm_fwnode_get_index_gpiod_from_child);
+EXPORT_SYMBOL(devm_fwnode_gpiod_get_index);
 
 /**
  * devm_gpiod_get_index_optional - Resource-managed gpiod_get_index_optional()
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index b70af921c614..dc0ddcd30515 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -176,11 +176,11 @@  struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 					 const char *propname, int index,
 					 enum gpiod_flags dflags,
 					 const char *label);
-struct gpio_desc *devm_fwnode_get_index_gpiod_from_child(struct device *dev,
-						const char *con_id, int index,
-						struct fwnode_handle *child,
-						enum gpiod_flags flags,
-						const char *label);
+struct gpio_desc *devm_fwnode_gpiod_get_index(struct device *dev,
+					      struct fwnode_handle *child,
+					      const char *con_id, int index,
+					      enum gpiod_flags flags,
+					      const char *label);
 
 #else /* CONFIG_GPIOLIB */
 
@@ -531,6 +531,29 @@  struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 	return ERR_PTR(-ENOSYS);
 }
 
+static inline
+struct gpio_desc *devm_fwnode_gpiod_get_index(struct device *dev,
+					      struct fwnode_handle *fwnode,
+					      const char *con_id, int index,
+					      enum gpiod_flags flags,
+					      const char *label)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+#endif /* CONFIG_GPIOLIB */
+
+static inline
+struct gpio_desc *devm_fwnode_gpiod_get(struct device *dev,
+					struct fwnode_handle *fwnode,
+					const char *con_id,
+					enum gpiod_flags flags,
+					const char *label)
+{
+	return devm_fwnode_gpiod_get_index(dev, fwnode, con_id, 0,
+					   flags, label);
+}
+
 static inline
 struct gpio_desc *devm_fwnode_get_index_gpiod_from_child(struct device *dev,
 						const char *con_id, int index,
@@ -538,11 +561,10 @@  struct gpio_desc *devm_fwnode_get_index_gpiod_from_child(struct device *dev,
 						enum gpiod_flags flags,
 						const char *label)
 {
-	return ERR_PTR(-ENOSYS);
+	return devm_fwnode_gpiod_get_index(dev, child, con_id, index,
+					   flags, label);
 }
 
-#endif /* CONFIG_GPIOLIB */
-
 static inline
 struct gpio_desc *devm_fwnode_get_gpiod_from_child(struct device *dev,
 						   const char *con_id,
@@ -550,8 +572,7 @@  struct gpio_desc *devm_fwnode_get_gpiod_from_child(struct device *dev,
 						   enum gpiod_flags flags,
 						   const char *label)
 {
-	return devm_fwnode_get_index_gpiod_from_child(dev, con_id, 0, child,
-						      flags, label);
+	return devm_fwnode_gpiod_get_index(dev, child, con_id, 0, flags, label);
 }
 
 #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_OF_GPIO)