LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v1 0/4] Let leds use named gpios
@ 2015-01-07  9:08 Olliver Schinagl
  2015-01-07  9:08 ` [PATCH v1 1/4] gpio:gpiolib: use static const char const * for a suffixes array Olliver Schinagl
                   ` (3 more replies)
  0 siblings, 4 replies; 22+ messages in thread
From: Olliver Schinagl @ 2015-01-07  9:08 UTC (permalink / raw)
  To: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Alexandre Courbot, Dmitry Torokhov, Bryan Wu,
	Richard Purdie
  Cc: Olliver Schinagl, Robin Gong, Rafael J. Wysocki, Aaron Lu,
	Mika Westerberg, Grant Likely, Wolfram Sang, Alexander Shiyan,
	Jingoo Han, devicetree, linux-kernel, linux-gpio, linux-input,
	linux-leds, Olliver Schinagl

From: Olliver Schinagl <o.schinagl@ultimaker.com>

The gpio binding docs ask to use named-gpios wherever possible, however 40b73183 probably forgot that. This patch makes the new devm_get_gpiod_from_child use named gpios.

Olliver Schinagl (4):
  gpio:gpiolib: use static const char const * for a suffixes array
  gpio: add parameter to allow the use named gpios
  leds: Let the binding document example for leds-gpio follow the gpio
    bindings
  leds: no longer use unnamed gpios

 Documentation/devicetree/bindings/leds/leds-gpio.txt | 14 ++++++++------
 drivers/gpio/devres.c                                | 16 +++++++++++++++-
 drivers/gpio/gpiolib.c                               |  2 +-
 drivers/input/keyboard/gpio_keys_polled.c            |  2 +-
 drivers/leds/leds-gpio.c                             |  2 +-
 include/linux/gpio/consumer.h                        |  1 +
 6 files changed, 27 insertions(+), 10 deletions(-)

-- 
2.1.4


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v1 1/4] gpio:gpiolib: use static const char const * for a suffixes array
  2015-01-07  9:08 [PATCH v1 0/4] Let leds use named gpios Olliver Schinagl
@ 2015-01-07  9:08 ` Olliver Schinagl
  2015-01-14 12:35   ` Linus Walleij
  2015-01-07  9:08 ` [PATCH v1 2/4] gpio: add parameter to allow the use named gpios Olliver Schinagl
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 22+ messages in thread
From: Olliver Schinagl @ 2015-01-07  9:08 UTC (permalink / raw)
  To: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Alexandre Courbot, Dmitry Torokhov, Bryan Wu,
	Richard Purdie
  Cc: Olliver Schinagl, Robin Gong, Rafael J. Wysocki, Aaron Lu,
	Mika Westerberg, Grant Likely, Wolfram Sang, Alexander Shiyan,
	Jingoo Han, devicetree, linux-kernel, linux-gpio, linux-input,
	linux-leds

From: Olliver Schinagl <oliver@schinagl.nl>

Checkpatch complains, and probably with good reason that we should use
const char const * for the static constant array that never gets
changed.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/gpio/gpiolib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 487afe6..9d2a371 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1657,7 +1657,7 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 				      unsigned int idx,
 				      enum gpio_lookup_flags *flags)
 {
-	static const char *suffixes[] = { "gpios", "gpio" };
+	static const char const *suffixes[] = { "gpios", "gpio" };
 	char prop_name[32]; /* 32 is max size of property name */
 	enum of_gpio_flags of_flags;
 	struct gpio_desc *desc;
-- 
2.1.4


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v1 2/4] gpio: add parameter to allow the use named gpios
  2015-01-07  9:08 [PATCH v1 0/4] Let leds use named gpios Olliver Schinagl
  2015-01-07  9:08 ` [PATCH v1 1/4] gpio:gpiolib: use static const char const * for a suffixes array Olliver Schinagl
@ 2015-01-07  9:08 ` Olliver Schinagl
  2015-01-08  0:15   ` Dmitry Torokhov
                     ` (2 more replies)
  2015-01-07  9:08 ` [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings Olliver Schinagl
  2015-01-07  9:08 ` [PATCH v1 4/4] leds: no longer use unnamed gpios Olliver Schinagl
  3 siblings, 3 replies; 22+ messages in thread
From: Olliver Schinagl @ 2015-01-07  9:08 UTC (permalink / raw)
  To: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Alexandre Courbot, Dmitry Torokhov, Bryan Wu,
	Richard Purdie
  Cc: Olliver Schinagl, Robin Gong, Rafael J. Wysocki, Aaron Lu,
	Mika Westerberg, Grant Likely, Wolfram Sang, Alexander Shiyan,
	Jingoo Han, devicetree, linux-kernel, linux-gpio, linux-input,
	linux-leds

From: Olliver Schinagl <oliver@schinagl.nl>

The gpio binding document says that new code should always use named
gpios. Patch 40b73183 added support to parse a list of gpios from child
nodes, but does not make it possible to use named gpios. This patch adds
the con_id property and implements it is done in gpiolib.c, where the
old-style of using unnamed gpios still works.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/gpio/devres.c                     | 16 +++++++++++++++-
 drivers/input/keyboard/gpio_keys_polled.c |  2 +-
 drivers/leds/leds-gpio.c                  |  2 +-
 include/linux/gpio/consumer.h             |  1 +
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c
index 13dbd3d..b7fbe1c 100644
--- a/drivers/gpio/devres.c
+++ b/drivers/gpio/devres.c
@@ -111,23 +111,37 @@ EXPORT_SYMBOL(__devm_gpiod_get_index);
 /**
  * devm_get_gpiod_from_child - get a GPIO descriptor from a device's child node
  * @dev:	GPIO consumer
+ * @con_id:	function within the GPIO consumer
  * @child:	firmware node (child of @dev)
  *
  * GPIO descriptors returned from this function are automatically disposed on
  * driver detach.
  */
 struct gpio_desc *devm_get_gpiod_from_child(struct device *dev,
+					    const char *con_id,
 					    struct fwnode_handle *child)
 {
+	static const char const *suffixes[] = { "gpios", "gpio" };
+	char prop_name[32]; /* 32 is max size of property name */
 	struct gpio_desc **dr;
 	struct gpio_desc *desc;
+	unsigned int i;
 
 	dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
 			  GFP_KERNEL);
 	if (!dr)
 		return ERR_PTR(-ENOMEM);
 
-	desc = fwnode_get_named_gpiod(child, "gpios");
+	for (i = 0; i < ARRAY_SIZE(suffixes); i++) {
+		if (con_id)
+			snprintf(prop_name, 32, "%s-%s", con_id, suffixes[i]);
+		else
+			snprintf(prop_name, 32, "%s", suffixes[i]);
+
+		desc = fwnode_get_named_gpiod(child, prop_name);
+		if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER))
+			break;
+	}
 	if (IS_ERR(desc)) {
 		devres_free(dr);
 		return desc;
diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index 90df4df..097d721 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -125,7 +125,7 @@ static struct gpio_keys_platform_data *gpio_keys_polled_get_devtree_pdata(struct
 	device_for_each_child_node(dev, child) {
 		struct gpio_desc *desc;
 
-		desc = devm_get_gpiod_from_child(dev, child);
+		desc = devm_get_gpiod_from_child(dev, NULL, child);
 		if (IS_ERR(desc)) {
 			error = PTR_ERR(desc);
 			if (error != -EPROBE_DEFER)
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 7ea1ea42..8e5af69 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -184,7 +184,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 		struct gpio_led led = {};
 		const char *state = NULL;
 
-		led.gpiod = devm_get_gpiod_from_child(dev, child);
+		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
 		if (IS_ERR(led.gpiod)) {
 			fwnode_handle_put(child);
 			goto err;
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 45afc2d..ed20759 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -110,6 +110,7 @@ struct fwnode_handle;
 struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
 					 const char *propname);
 struct gpio_desc *devm_get_gpiod_from_child(struct device *dev,
+					    const char *con_id,
 					    struct fwnode_handle *child);
 #else /* CONFIG_GPIOLIB */
 
-- 
2.1.4


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings
  2015-01-07  9:08 [PATCH v1 0/4] Let leds use named gpios Olliver Schinagl
  2015-01-07  9:08 ` [PATCH v1 1/4] gpio:gpiolib: use static const char const * for a suffixes array Olliver Schinagl
  2015-01-07  9:08 ` [PATCH v1 2/4] gpio: add parameter to allow the use named gpios Olliver Schinagl
@ 2015-01-07  9:08 ` Olliver Schinagl
  2015-01-07 23:56   ` Dmitry Torokhov
                     ` (2 more replies)
  2015-01-07  9:08 ` [PATCH v1 4/4] leds: no longer use unnamed gpios Olliver Schinagl
  3 siblings, 3 replies; 22+ messages in thread
From: Olliver Schinagl @ 2015-01-07  9:08 UTC (permalink / raw)
  To: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Alexandre Courbot, Dmitry Torokhov, Bryan Wu,
	Richard Purdie
  Cc: Olliver Schinagl, Robin Gong, Rafael J. Wysocki, Aaron Lu,
	Mika Westerberg, Grant Likely, Wolfram Sang, Alexander Shiyan,
	Jingoo Han, devicetree, linux-kernel, linux-gpio, linux-input,
	linux-leds

From: Olliver Schinagl <oliver@schinagl.nl>

In the gpio bindings documents it is requested to use the marco's in
include/dt-bindings/gpio/gpio.h whenever possible. The gpios in the led
drivers don't seem to form an exception, so update the example in the
document bindings.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 Documentation/devicetree/bindings/leds/leds-gpio.txt | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
index f77148f..d544cf1 100644
--- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
+++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
@@ -26,16 +26,18 @@ LED sub-node properties:
 
 Examples:
 
+#include <dt-bindings/gpio/gpio.h>
+
 leds {
 	compatible = "gpio-leds";
 	hdd {
 		label = "IDE Activity";
-		gpios = <&mcu_pio 0 1>; /* Active low */
+		gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>;
 		linux,default-trigger = "ide-disk";
 	};
 
 	fault {
-		gpios = <&mcu_pio 1 0>;
+		gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
 		/* Keep LED on if BIOS detected hardware fault */
 		default-state = "keep";
 	};
@@ -44,11 +46,11 @@ leds {
 run-control {
 	compatible = "gpio-leds";
 	red {
-		gpios = <&mpc8572 6 0>;
+		gpios = <&mpc8572 6 GPIO_ACTIVE_HIHG>;
 		default-state = "off";
 	};
 	green {
-		gpios = <&mpc8572 7 0>;
+		gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>;
 		default-state = "on";
 	};
 };
@@ -57,7 +59,7 @@ leds {
 	compatible = "gpio-leds";
 
 	charger-led {
-		gpios = <&gpio1 2 0>;
+		gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
 		linux,default-trigger = "max8903-charger-charging";
 		retain-state-suspended;
 	};
-- 
2.1.4


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v1 4/4] leds: no longer use unnamed gpios
  2015-01-07  9:08 [PATCH v1 0/4] Let leds use named gpios Olliver Schinagl
                   ` (2 preceding siblings ...)
  2015-01-07  9:08 ` [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings Olliver Schinagl
@ 2015-01-07  9:08 ` Olliver Schinagl
  2015-01-07 23:55   ` Dmitry Torokhov
  3 siblings, 1 reply; 22+ messages in thread
From: Olliver Schinagl @ 2015-01-07  9:08 UTC (permalink / raw)
  To: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Alexandre Courbot, Dmitry Torokhov, Bryan Wu,
	Richard Purdie
  Cc: Olliver Schinagl, Robin Gong, Rafael J. Wysocki, Aaron Lu,
	Mika Westerberg, Grant Likely, Wolfram Sang, Alexander Shiyan,
	Jingoo Han, devicetree, linux-kernel, linux-gpio, linux-input,
	linux-leds

From: Olliver Schinagl <oliver@schinagl.nl>

The gpio document says we should not use unnamed bindings for gpios.
This patch uses the 'led-' prefix to the gpios and updates code and
documents. Because the devm_get_gpiod_from_child() falls back to using
old-style unnamed gpios, we can update the code first, and update
dts files as time allows.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 Documentation/devicetree/bindings/leds/leds-gpio.txt | 12 ++++++------
 drivers/leds/leds-gpio.c                             |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
index d544cf1..2c138cc 100644
--- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
+++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
@@ -7,7 +7,7 @@ Each LED is represented as a sub-node of the gpio-leds device.  Each
 node's name represents the name of the corresponding LED.
 
 LED sub-node properties:
-- gpios :  Should specify the LED's GPIO, see "gpios property" in
+- led-gpios :  Should specify the LED's GPIO, see "gpios property" in
   Documentation/devicetree/bindings/gpio/gpio.txt.  Active low LEDs should be
   indicated using flags in the GPIO specifier.
 - label :  (optional)
@@ -32,12 +32,12 @@ leds {
 	compatible = "gpio-leds";
 	hdd {
 		label = "IDE Activity";
-		gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>;
+		led-gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>;
 		linux,default-trigger = "ide-disk";
 	};
 
 	fault {
-		gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
+		led-gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
 		/* Keep LED on if BIOS detected hardware fault */
 		default-state = "keep";
 	};
@@ -46,11 +46,11 @@ leds {
 run-control {
 	compatible = "gpio-leds";
 	red {
-		gpios = <&mpc8572 6 GPIO_ACTIVE_HIHG>;
+		led-gpios = <&mpc8572 6 GPIO_ACTIVE_HIHG>;
 		default-state = "off";
 	};
 	green {
-		gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>;
+		led-gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>;
 		default-state = "on";
 	};
 };
@@ -59,7 +59,7 @@ leds {
 	compatible = "gpio-leds";
 
 	charger-led {
-		gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
+		led-gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
 		linux,default-trigger = "max8903-charger-charging";
 		retain-state-suspended;
 	};
diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index 8e5af69..5b7bc84 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -184,7 +184,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 		struct gpio_led led = {};
 		const char *state = NULL;
 
-		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
+		led.gpiod = devm_get_gpiod_from_child(dev, "led", child);
 		if (IS_ERR(led.gpiod)) {
 			fwnode_handle_put(child);
 			goto err;
-- 
2.1.4


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 4/4] leds: no longer use unnamed gpios
  2015-01-07  9:08 ` [PATCH v1 4/4] leds: no longer use unnamed gpios Olliver Schinagl
@ 2015-01-07 23:55   ` Dmitry Torokhov
  2015-01-08  8:45     ` Olliver Schinagl
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Torokhov @ 2015-01-07 23:55 UTC (permalink / raw)
  To: Olliver Schinagl
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Alexandre Courbot, Bryan Wu, Richard Purdie,
	Olliver Schinagl, Robin Gong, Rafael J. Wysocki, Aaron Lu,
	Mika Westerberg, Grant Likely, Wolfram Sang, Alexander Shiyan,
	Jingoo Han, devicetree, linux-kernel, linux-gpio, linux-input,
	linux-leds

On Wed, Jan 07, 2015 at 10:08:42AM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
> 
> The gpio document says we should not use unnamed bindings for gpios.
> This patch uses the 'led-' prefix to the gpios and updates code and
> documents. Because the devm_get_gpiod_from_child() falls back to using
> old-style unnamed gpios, we can update the code first, and update
> dts files as time allows.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  Documentation/devicetree/bindings/leds/leds-gpio.txt | 12 ++++++------
>  drivers/leds/leds-gpio.c                             |  2 +-
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
> index d544cf1..2c138cc 100644
> --- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
> @@ -7,7 +7,7 @@ Each LED is represented as a sub-node of the gpio-leds device.  Each
>  node's name represents the name of the corresponding LED.
>  
>  LED sub-node properties:
> -- gpios :  Should specify the LED's GPIO, see "gpios property" in
> +- led-gpios :  Should specify the LED's GPIO, see "gpios property" in
>    Documentation/devicetree/bindings/gpio/gpio.txt.  Active low LEDs should be
>    indicated using flags in the GPIO specifier.
>  - label :  (optional)
> @@ -32,12 +32,12 @@ leds {
>  	compatible = "gpio-leds";
>  	hdd {
>  		label = "IDE Activity";
> -		gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>;
> +		led-gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>;
>  		linux,default-trigger = "ide-disk";
>  	};
>  
>  	fault {
> -		gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
> +		led-gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
>  		/* Keep LED on if BIOS detected hardware fault */
>  		default-state = "keep";
>  	};
> @@ -46,11 +46,11 @@ leds {
>  run-control {
>  	compatible = "gpio-leds";
>  	red {
> -		gpios = <&mpc8572 6 GPIO_ACTIVE_HIHG>;
> +		led-gpios = <&mpc8572 6 GPIO_ACTIVE_HIHG>;
>  		default-state = "off";
>  	};
>  	green {
> -		gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>;
> +		led-gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>;
>  		default-state = "on";
>  	};
>  };
> @@ -59,7 +59,7 @@ leds {
>  	compatible = "gpio-leds";
>  
>  	charger-led {
> -		gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
> +		led-gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
>  		linux,default-trigger = "max8903-charger-charging";
>  		retain-state-suspended;
>  	};
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index 8e5af69..5b7bc84 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -184,7 +184,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
>  		struct gpio_led led = {};
>  		const char *state = NULL;
>  
> -		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
> +		led.gpiod = devm_get_gpiod_from_child(dev, "led", child);

Would not this break existing boards using old bindings? You need to
handle both cases: if you can't located "led-gpios" then you will have to
try just "gpios".

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings
  2015-01-07  9:08 ` [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings Olliver Schinagl
@ 2015-01-07 23:56   ` Dmitry Torokhov
  2015-01-08  1:45   ` Rob Herring
  2015-01-14 12:38   ` Linus Walleij
  2 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2015-01-07 23:56 UTC (permalink / raw)
  To: Olliver Schinagl
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Alexandre Courbot, Bryan Wu, Richard Purdie,
	Olliver Schinagl, Robin Gong, Rafael J. Wysocki, Aaron Lu,
	Mika Westerberg, Grant Likely, Wolfram Sang, Alexander Shiyan,
	Jingoo Han, devicetree, linux-kernel, linux-gpio, linux-input,
	linux-leds

On Wed, Jan 07, 2015 at 10:08:41AM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
> 
> In the gpio bindings documents it is requested to use the marco's in
> include/dt-bindings/gpio/gpio.h whenever possible. The gpios in the led
> drivers don't seem to form an exception, so update the example in the
> document bindings.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  Documentation/devicetree/bindings/leds/leds-gpio.txt | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
> index f77148f..d544cf1 100644
> --- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
> @@ -26,16 +26,18 @@ LED sub-node properties:
>  
>  Examples:
>  
> +#include <dt-bindings/gpio/gpio.h>
> +
>  leds {
>  	compatible = "gpio-leds";
>  	hdd {
>  		label = "IDE Activity";
> -		gpios = <&mcu_pio 0 1>; /* Active low */
> +		gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>;
>  		linux,default-trigger = "ide-disk";
>  	};
>  
>  	fault {
> -		gpios = <&mcu_pio 1 0>;
> +		gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
>  		/* Keep LED on if BIOS detected hardware fault */
>  		default-state = "keep";
>  	};
> @@ -44,11 +46,11 @@ leds {
>  run-control {
>  	compatible = "gpio-leds";
>  	red {
> -		gpios = <&mpc8572 6 0>;
> +		gpios = <&mpc8572 6 GPIO_ACTIVE_HIHG>;

Typo, s/HIHG/HIGH/

>  		default-state = "off";
>  	};
>  	green {
> -		gpios = <&mpc8572 7 0>;
> +		gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>;
>  		default-state = "on";
>  	};
>  };
> @@ -57,7 +59,7 @@ leds {
>  	compatible = "gpio-leds";
>  
>  	charger-led {
> -		gpios = <&gpio1 2 0>;
> +		gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
>  		linux,default-trigger = "max8903-charger-charging";
>  		retain-state-suspended;
>  	};
> -- 
> 2.1.4
> 

-- 
Dmitry

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios
  2015-01-07  9:08 ` [PATCH v1 2/4] gpio: add parameter to allow the use named gpios Olliver Schinagl
@ 2015-01-08  0:15   ` Dmitry Torokhov
  2015-01-14 12:36   ` Linus Walleij
  2015-01-19  4:04   ` Alexandre Courbot
  2 siblings, 0 replies; 22+ messages in thread
From: Dmitry Torokhov @ 2015-01-08  0:15 UTC (permalink / raw)
  To: Olliver Schinagl
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Alexandre Courbot, Bryan Wu, Richard Purdie,
	Olliver Schinagl, Robin Gong, Rafael J. Wysocki, Aaron Lu,
	Mika Westerberg, Grant Likely, Wolfram Sang, Alexander Shiyan,
	Jingoo Han, devicetree, linux-kernel, linux-gpio, linux-input,
	linux-leds

On Wed, Jan 07, 2015 at 10:08:40AM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
> 
> The gpio binding document says that new code should always use named
> gpios. Patch 40b73183 added support to parse a list of gpios from child
> nodes, but does not make it possible to use named gpios. This patch adds
> the con_id property and implements it is done in gpiolib.c, where the
> old-style of using unnamed gpios still works.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  drivers/gpio/devres.c                     | 16 +++++++++++++++-
>  drivers/input/keyboard/gpio_keys_polled.c |  2 +-
>  drivers/leds/leds-gpio.c                  |  2 +-
>  include/linux/gpio/consumer.h             |  1 +
>  4 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c
> index 13dbd3d..b7fbe1c 100644
> --- a/drivers/gpio/devres.c
> +++ b/drivers/gpio/devres.c
> @@ -111,23 +111,37 @@ EXPORT_SYMBOL(__devm_gpiod_get_index);
>  /**
>   * devm_get_gpiod_from_child - get a GPIO descriptor from a device's child node
>   * @dev:	GPIO consumer
> + * @con_id:	function within the GPIO consumer
>   * @child:	firmware node (child of @dev)
>   *
>   * GPIO descriptors returned from this function are automatically disposed on
>   * driver detach.
>   */
>  struct gpio_desc *devm_get_gpiod_from_child(struct device *dev,
> +					    const char *con_id,
>  					    struct fwnode_handle *child)
>  {
> +	static const char const *suffixes[] = { "gpios", "gpio" };
> +	char prop_name[32]; /* 32 is max size of property name */
>  	struct gpio_desc **dr;
>  	struct gpio_desc *desc;
> +	unsigned int i;
>  
>  	dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
>  			  GFP_KERNEL);
>  	if (!dr)
>  		return ERR_PTR(-ENOMEM);
>  
> -	desc = fwnode_get_named_gpiod(child, "gpios");
> +	for (i = 0; i < ARRAY_SIZE(suffixes); i++) {
> +		if (con_id)
> +			snprintf(prop_name, 32, "%s-%s", con_id, suffixes[i]);

sizeof(prop_name) instead of hard-coding 32 would be better.

> +		else
> +			snprintf(prop_name, 32, "%s", suffixes[i]);
> +
> +		desc = fwnode_get_named_gpiod(child, prop_name);
> +		if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER))
> +			break;
> +	}
>  	if (IS_ERR(desc)) {
>  		devres_free(dr);
>  		return desc;
> diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
> index 90df4df..097d721 100644
> --- a/drivers/input/keyboard/gpio_keys_polled.c
> +++ b/drivers/input/keyboard/gpio_keys_polled.c
> @@ -125,7 +125,7 @@ static struct gpio_keys_platform_data *gpio_keys_polled_get_devtree_pdata(struct
>  	device_for_each_child_node(dev, child) {
>  		struct gpio_desc *desc;
>  
> -		desc = devm_get_gpiod_from_child(dev, child);
> +		desc = devm_get_gpiod_from_child(dev, NULL, child);

I guess the number of users is small enough that we can do this API
change in one step if gpio overlords are happy with it.

>  		if (IS_ERR(desc)) {
>  			error = PTR_ERR(desc);
>  			if (error != -EPROBE_DEFER)
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index 7ea1ea42..8e5af69 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -184,7 +184,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
>  		struct gpio_led led = {};
>  		const char *state = NULL;
>  
> -		led.gpiod = devm_get_gpiod_from_child(dev, child);
> +		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
>  		if (IS_ERR(led.gpiod)) {
>  			fwnode_handle_put(child);
>  			goto err;
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index 45afc2d..ed20759 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -110,6 +110,7 @@ struct fwnode_handle;
>  struct gpio_desc *fwnode_get_named_gpiod(struct fwnode_handle *fwnode,
>  					 const char *propname);
>  struct gpio_desc *devm_get_gpiod_from_child(struct device *dev,
> +					    const char *con_id,
>  					    struct fwnode_handle *child);
>  #else /* CONFIG_GPIOLIB */
>  
> -- 
> 2.1.4
> 

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings
  2015-01-07  9:08 ` [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings Olliver Schinagl
  2015-01-07 23:56   ` Dmitry Torokhov
@ 2015-01-08  1:45   ` Rob Herring
  2015-01-14 12:38   ` Linus Walleij
  2 siblings, 0 replies; 22+ messages in thread
From: Rob Herring @ 2015-01-08  1:45 UTC (permalink / raw)
  To: Olliver Schinagl
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Alexandre Courbot, Dmitry Torokhov, Bryan Wu,
	Richard Purdie, Olliver Schinagl, Robin Gong, Rafael J. Wysocki,
	Aaron Lu, Mika Westerberg, Grant Likely, Wolfram Sang,
	Alexander Shiyan, Jingoo Han, devicetree, linux-kernel,
	linux-gpio, linux-input, linux-leds

On Wed, Jan 7, 2015 at 3:08 AM, Olliver Schinagl
<oliver+list@schinagl.nl> wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
>
> In the gpio bindings documents it is requested to use the marco's in
> include/dt-bindings/gpio/gpio.h whenever possible. The gpios in the led
> drivers don't seem to form an exception, so update the example in the
> document bindings.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

The example isn't exactly wrong, so I'd question how much this is
really needed, but:

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  Documentation/devicetree/bindings/leds/leds-gpio.txt | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
> index f77148f..d544cf1 100644
> --- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
> @@ -26,16 +26,18 @@ LED sub-node properties:
>
>  Examples:
>
> +#include <dt-bindings/gpio/gpio.h>
> +
>  leds {
>         compatible = "gpio-leds";
>         hdd {
>                 label = "IDE Activity";
> -               gpios = <&mcu_pio 0 1>; /* Active low */
> +               gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>;
>                 linux,default-trigger = "ide-disk";
>         };
>
>         fault {
> -               gpios = <&mcu_pio 1 0>;
> +               gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
>                 /* Keep LED on if BIOS detected hardware fault */
>                 default-state = "keep";
>         };
> @@ -44,11 +46,11 @@ leds {
>  run-control {
>         compatible = "gpio-leds";
>         red {
> -               gpios = <&mpc8572 6 0>;
> +               gpios = <&mpc8572 6 GPIO_ACTIVE_HIHG>;
>                 default-state = "off";
>         };
>         green {
> -               gpios = <&mpc8572 7 0>;
> +               gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>;
>                 default-state = "on";
>         };
>  };
> @@ -57,7 +59,7 @@ leds {
>         compatible = "gpio-leds";
>
>         charger-led {
> -               gpios = <&gpio1 2 0>;
> +               gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
>                 linux,default-trigger = "max8903-charger-charging";
>                 retain-state-suspended;
>         };
> --
> 2.1.4
>

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 4/4] leds: no longer use unnamed gpios
  2015-01-07 23:55   ` Dmitry Torokhov
@ 2015-01-08  8:45     ` Olliver Schinagl
  2015-01-08 14:40       ` Rob Herring
  0 siblings, 1 reply; 22+ messages in thread
From: Olliver Schinagl @ 2015-01-08  8:45 UTC (permalink / raw)
  To: Dmitry Torokhov, Olliver Schinagl
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Alexandre Courbot, Bryan Wu, Richard Purdie,
	Robin Gong, Rafael J. Wysocki, Aaron Lu, Mika Westerberg,
	Grant Likely, Wolfram Sang, Alexander Shiyan, Jingoo Han,
	devicetree, linux-kernel, linux-gpio, linux-input, linux-leds

Hey Dmitry,

On 08-01-15 00:55, Dmitry Torokhov wrote:
> On Wed, Jan 07, 2015 at 10:08:42AM +0100, Olliver Schinagl wrote:
>> From: Olliver Schinagl <oliver@schinagl.nl>
>>
>> The gpio document says we should not use unnamed bindings for gpios.
>> This patch uses the 'led-' prefix to the gpios and updates code and
>> documents. Because the devm_get_gpiod_from_child() falls back to using
>> old-style unnamed gpios, we can update the code first, and update
>> dts files as time allows.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   Documentation/devicetree/bindings/leds/leds-gpio.txt | 12 ++++++------
>>   drivers/leds/leds-gpio.c                             |  2 +-
>>   2 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
>> index d544cf1..2c138cc 100644
>> --- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
>> +++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
>> @@ -7,7 +7,7 @@ Each LED is represented as a sub-node of the gpio-leds device.  Each
>>   node's name represents the name of the corresponding LED.
>>   
>>   LED sub-node properties:
>> -- gpios :  Should specify the LED's GPIO, see "gpios property" in
>> +- led-gpios :  Should specify the LED's GPIO, see "gpios property" in
>>     Documentation/devicetree/bindings/gpio/gpio.txt.  Active low LEDs should be
>>     indicated using flags in the GPIO specifier.
>>   - label :  (optional)
>> @@ -32,12 +32,12 @@ leds {
>>   	compatible = "gpio-leds";
>>   	hdd {
>>   		label = "IDE Activity";
>> -		gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>;
>> +		led-gpios = <&mcu_pio 0 GPIO_ACTIVE_LOW>;
>>   		linux,default-trigger = "ide-disk";
>>   	};
>>   
>>   	fault {
>> -		gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
>> +		led-gpios = <&mcu_pio 1 GPIO_ACTIVE_HIGH>;
>>   		/* Keep LED on if BIOS detected hardware fault */
>>   		default-state = "keep";
>>   	};
>> @@ -46,11 +46,11 @@ leds {
>>   run-control {
>>   	compatible = "gpio-leds";
>>   	red {
>> -		gpios = <&mpc8572 6 GPIO_ACTIVE_HIHG>;
>> +		led-gpios = <&mpc8572 6 GPIO_ACTIVE_HIHG>;
>>   		default-state = "off";
>>   	};
>>   	green {
>> -		gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>;
>> +		led-gpios = <&mpc8572 7 GPIO_ACTIVE_HIGH>;
>>   		default-state = "on";
>>   	};
>>   };
>> @@ -59,7 +59,7 @@ leds {
>>   	compatible = "gpio-leds";
>>   
>>   	charger-led {
>> -		gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
>> +		led-gpios = <&gpio1 2 GPIO_ACTIVE_HIGH>;
>>   		linux,default-trigger = "max8903-charger-charging";
>>   		retain-state-suspended;
>>   	};
>> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
>> index 8e5af69..5b7bc84 100644
>> --- a/drivers/leds/leds-gpio.c
>> +++ b/drivers/leds/leds-gpio.c
>> @@ -184,7 +184,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
>>   		struct gpio_led led = {};
>>   		const char *state = NULL;
>>   
>> -		led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
>> +		led.gpiod = devm_get_gpiod_from_child(dev, "led", child);
> Would not this break existing boards using old bindings? You need to
> handle both cases: if you can't located "led-gpios" then you will have to
> try just "gpios".
Very true. I was rather even hoping we could update all bindings, I 
don't mind going through the available dts files to fix them ... But 
need to know that that's the proper way to go before doing the work ;)

Olliver
>
> Thanks.
>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 4/4] leds: no longer use unnamed gpios
  2015-01-08  8:45     ` Olliver Schinagl
@ 2015-01-08 14:40       ` Rob Herring
  2015-01-08 22:12         ` Dmitry Torokhov
  0 siblings, 1 reply; 22+ messages in thread
From: Rob Herring @ 2015-01-08 14:40 UTC (permalink / raw)
  To: Olliver Schinagl
  Cc: Dmitry Torokhov, Olliver Schinagl, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Linus Walleij,
	Alexandre Courbot, Bryan Wu, Richard Purdie, Robin Gong,
	Rafael J. Wysocki, Aaron Lu, Mika Westerberg, Grant Likely,
	Wolfram Sang, Alexander Shiyan, Jingoo Han, devicetree,
	linux-kernel, linux-gpio, linux-input, linux-leds

On Thu, Jan 8, 2015 at 2:45 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey Dmitry,
>
>
> On 08-01-15 00:55, Dmitry Torokhov wrote:
>>
>> On Wed, Jan 07, 2015 at 10:08:42AM +0100, Olliver Schinagl wrote:
>>>
>>> From: Olliver Schinagl <oliver@schinagl.nl>
>>>
>>> The gpio document says we should not use unnamed bindings for gpios.
>>> This patch uses the 'led-' prefix to the gpios and updates code and
>>> documents. Because the devm_get_gpiod_from_child() falls back to using
>>> old-style unnamed gpios, we can update the code first, and update
>>> dts files as time allows.

[...]

>>> --- a/drivers/leds/leds-gpio.c
>>> +++ b/drivers/leds/leds-gpio.c
>>> @@ -184,7 +184,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct
>>> platform_device *pdev)
>>>                 struct gpio_led led = {};
>>>                 const char *state = NULL;
>>>   -             led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
>>> +               led.gpiod = devm_get_gpiod_from_child(dev, "led", child);
>>
>> Would not this break existing boards using old bindings? You need to
>> handle both cases: if you can't located "led-gpios" then you will have to
>> try just "gpios".
>
> Very true. I was rather even hoping we could update all bindings, I don't
> mind going through the available dts files to fix them ... But need to know
> that that's the proper way to go before doing the work ;)

That will not work. You cannot make changes that require a new dtb
with a new kernel. This would also break for the other way around
(i.e. a new dtb and old kernel).

You would have to search for both led-gpios and gpios. I'm not sure if
we can do that generically for all GPIOs. If you had a node with both
"blah-gpios" and "gpios", it would break. I would hope there are no
such cases like that. We also now have to consider how ACPI identifies
GPIOs and whether this makes sense.

Rob

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 4/4] leds: no longer use unnamed gpios
  2015-01-08 14:40       ` Rob Herring
@ 2015-01-08 22:12         ` Dmitry Torokhov
  2015-01-14 12:45           ` Linus Walleij
  0 siblings, 1 reply; 22+ messages in thread
From: Dmitry Torokhov @ 2015-01-08 22:12 UTC (permalink / raw)
  To: Rob Herring
  Cc: Olliver Schinagl, Olliver Schinagl, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Linus Walleij,
	Alexandre Courbot, Bryan Wu, Richard Purdie, Robin Gong,
	Rafael J. Wysocki, Aaron Lu, Mika Westerberg, Grant Likely,
	Wolfram Sang, Alexander Shiyan, Jingoo Han, devicetree,
	linux-kernel, linux-gpio, linux-input, linux-leds

On Thu, Jan 08, 2015 at 08:40:20AM -0600, Rob Herring wrote:
> On Thu, Jan 8, 2015 at 2:45 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> > Hey Dmitry,
> >
> >
> > On 08-01-15 00:55, Dmitry Torokhov wrote:
> >>
> >> On Wed, Jan 07, 2015 at 10:08:42AM +0100, Olliver Schinagl wrote:
> >>>
> >>> From: Olliver Schinagl <oliver@schinagl.nl>
> >>>
> >>> The gpio document says we should not use unnamed bindings for gpios.
> >>> This patch uses the 'led-' prefix to the gpios and updates code and
> >>> documents. Because the devm_get_gpiod_from_child() falls back to using
> >>> old-style unnamed gpios, we can update the code first, and update
> >>> dts files as time allows.
> 
> [...]
> 
> >>> --- a/drivers/leds/leds-gpio.c
> >>> +++ b/drivers/leds/leds-gpio.c
> >>> @@ -184,7 +184,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct
> >>> platform_device *pdev)
> >>>                 struct gpio_led led = {};
> >>>                 const char *state = NULL;
> >>>   -             led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
> >>> +               led.gpiod = devm_get_gpiod_from_child(dev, "led", child);
> >>
> >> Would not this break existing boards using old bindings? You need to
> >> handle both cases: if you can't located "led-gpios" then you will have to
> >> try just "gpios".
> >
> > Very true. I was rather even hoping we could update all bindings, I don't
> > mind going through the available dts files to fix them ... But need to know
> > that that's the proper way to go before doing the work ;)
> 
> That will not work. You cannot make changes that require a new dtb
> with a new kernel. This would also break for the other way around
> (i.e. a new dtb and old kernel).
> 
> You would have to search for both led-gpios and gpios. I'm not sure if
> we can do that generically for all GPIOs. If you had a node with both
> "blah-gpios" and "gpios", it would break. I would hope there are no
> such cases like that. We also now have to consider how ACPI identifies
> GPIOs and whether this makes sense.

I think only the driver itself can know about such "legacy" mappings and
make a decision.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 1/4] gpio:gpiolib: use static const char const * for a suffixes array
  2015-01-07  9:08 ` [PATCH v1 1/4] gpio:gpiolib: use static const char const * for a suffixes array Olliver Schinagl
@ 2015-01-14 12:35   ` Linus Walleij
  0 siblings, 0 replies; 22+ messages in thread
From: Linus Walleij @ 2015-01-14 12:35 UTC (permalink / raw)
  To: Olliver Schinagl
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Alexandre Courbot, Dmitry Torokhov, Bryan Wu, Richard Purdie,
	Olliver Schinagl, Robin Gong, Rafael J. Wysocki, Aaron Lu,
	Mika Westerberg, Grant Likely, Wolfram Sang, Alexander Shiyan,
	Jingoo Han, devicetree, linux-kernel, linux-gpio, Linux Input,
	linux-leds

On Wed, Jan 7, 2015 at 10:08 AM, Olliver Schinagl
<oliver+list@schinagl.nl> wrote:

> From: Olliver Schinagl <oliver@schinagl.nl>
>
> Checkpatch complains, and probably with good reason that we should use
> const char const * for the static constant array that never gets
> changed.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Patch applied.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios
  2015-01-07  9:08 ` [PATCH v1 2/4] gpio: add parameter to allow the use named gpios Olliver Schinagl
  2015-01-08  0:15   ` Dmitry Torokhov
@ 2015-01-14 12:36   ` Linus Walleij
  2015-01-19  4:04   ` Alexandre Courbot
  2 siblings, 0 replies; 22+ messages in thread
From: Linus Walleij @ 2015-01-14 12:36 UTC (permalink / raw)
  To: Olliver Schinagl, Alexandre Courbot
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Dmitry Torokhov, Bryan Wu, Richard Purdie, Olliver Schinagl,
	Robin Gong, Rafael J. Wysocki, Aaron Lu, Mika Westerberg,
	Grant Likely, Wolfram Sang, Alexander Shiyan, Jingoo Han,
	devicetree, linux-kernel, linux-gpio, Linux Input, linux-leds

On Wed, Jan 7, 2015 at 10:08 AM, Olliver Schinagl
<oliver+list@schinagl.nl> wrote:

> From: Olliver Schinagl <oliver@schinagl.nl>
>
> The gpio binding document says that new code should always use named
> gpios. Patch 40b73183 added support to parse a list of gpios from child
> nodes, but does not make it possible to use named gpios. This patch adds
> the con_id property and implements it is done in gpiolib.c, where the
> old-style of using unnamed gpios still works.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

Alexandre, can you please take a close look at this
patch.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings
  2015-01-07  9:08 ` [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings Olliver Schinagl
  2015-01-07 23:56   ` Dmitry Torokhov
  2015-01-08  1:45   ` Rob Herring
@ 2015-01-14 12:38   ` Linus Walleij
  2 siblings, 0 replies; 22+ messages in thread
From: Linus Walleij @ 2015-01-14 12:38 UTC (permalink / raw)
  To: Olliver Schinagl
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Alexandre Courbot, Dmitry Torokhov, Bryan Wu, Richard Purdie,
	Olliver Schinagl, Robin Gong, Rafael J. Wysocki, Aaron Lu,
	Mika Westerberg, Grant Likely, Wolfram Sang, Alexander Shiyan,
	Jingoo Han, devicetree, linux-kernel, linux-gpio, Linux Input,
	linux-leds

On Wed, Jan 7, 2015 at 10:08 AM, Olliver Schinagl
<oliver+list@schinagl.nl> wrote:

> From: Olliver Schinagl <oliver@schinagl.nl>
>
> In the gpio bindings documents it is requested to use the marco's in
> include/dt-bindings/gpio/gpio.h whenever possible. The gpios in the led
> drivers don't seem to form an exception, so update the example in the
> document bindings.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>

With the spelling mistake fixed:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 4/4] leds: no longer use unnamed gpios
  2015-01-08 22:12         ` Dmitry Torokhov
@ 2015-01-14 12:45           ` Linus Walleij
  2015-01-14 13:20             ` Olliver Schinagl
  0 siblings, 1 reply; 22+ messages in thread
From: Linus Walleij @ 2015-01-14 12:45 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: Rob Herring, Olliver Schinagl, Olliver Schinagl, Rob Herring,
	Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Alexandre Courbot, Bryan Wu, Richard Purdie, Robin Gong,
	Rafael J. Wysocki, Aaron Lu, Mika Westerberg, Grant Likely,
	Wolfram Sang, Alexander Shiyan, Jingoo Han, devicetree,
	linux-kernel, linux-gpio, linux-input, linux-leds

On Thu, Jan 8, 2015 at 11:12 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Jan 08, 2015 at 08:40:20AM -0600, Rob Herring wrote:
>> On Thu, Jan 8, 2015 at 2:45 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:

>> >>> --- a/drivers/leds/leds-gpio.c
>> >>> +++ b/drivers/leds/leds-gpio.c
>> >>> @@ -184,7 +184,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct
>> >>> platform_device *pdev)
>> >>>                 struct gpio_led led = {};
>> >>>                 const char *state = NULL;
>> >>>   -             led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
>> >>> +               led.gpiod = devm_get_gpiod_from_child(dev, "led", child);
>> >>
>> >> Would not this break existing boards using old bindings? You need to
>> >> handle both cases: if you can't located "led-gpios" then you will have to
>> >> try just "gpios".
>> >
>> > Very true. I was rather even hoping we could update all bindings, I don't
>> > mind going through the available dts files to fix them ... But need to know
>> > that that's the proper way to go before doing the work ;)
>>
>> That will not work. You cannot make changes that require a new dtb
>> with a new kernel. This would also break for the other way around
>> (i.e. a new dtb and old kernel).
>>
>> You would have to search for both led-gpios and gpios. I'm not sure if
>> we can do that generically for all GPIOs. If you had a node with both
>> "blah-gpios" and "gpios", it would break. I would hope there are no
>> such cases like that. We also now have to consider how ACPI identifies
>> GPIOs and whether this makes sense.
>
> I think only the driver itself can know about such "legacy" mappings and
> make a decision.

Yeah leds-gpio.c will need to be patched to check for "led-gpios" first
and then fall back to "gpios" if not found.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 4/4] leds: no longer use unnamed gpios
  2015-01-14 12:45           ` Linus Walleij
@ 2015-01-14 13:20             ` Olliver Schinagl
  2015-01-19  3:43               ` Alexandre Courbot
  0 siblings, 1 reply; 22+ messages in thread
From: Olliver Schinagl @ 2015-01-14 13:20 UTC (permalink / raw)
  To: Linus Walleij, Dmitry Torokhov
  Cc: Rob Herring, Olliver Schinagl, Rob Herring, Pawel Moll,
	Mark Rutland, Ian Campbell, Kumar Gala, Alexandre Courbot,
	Bryan Wu, Richard Purdie, Robin Gong, Rafael J. Wysocki,
	Aaron Lu, Mika Westerberg, Grant Likely, Wolfram Sang,
	Alexander Shiyan, Jingoo Han, devicetree, linux-kernel,
	linux-gpio, linux-input, linux-leds


On 14-01-15 13:45, Linus Walleij wrote:
> On Thu, Jan 8, 2015 at 11:12 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>> On Thu, Jan 08, 2015 at 08:40:20AM -0600, Rob Herring wrote:
>>> On Thu, Jan 8, 2015 at 2:45 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>>>>>> --- a/drivers/leds/leds-gpio.c
>>>>>> +++ b/drivers/leds/leds-gpio.c
>>>>>> @@ -184,7 +184,7 @@ static struct gpio_leds_priv *gpio_leds_create(struct
>>>>>> platform_device *pdev)
>>>>>>                  struct gpio_led led = {};
>>>>>>                  const char *state = NULL;
>>>>>>    -             led.gpiod = devm_get_gpiod_from_child(dev, NULL, child);
>>>>>> +               led.gpiod = devm_get_gpiod_from_child(dev, "led", child);
>>>>> Would not this break existing boards using old bindings? You need to
>>>>> handle both cases: if you can't located "led-gpios" then you will have to
>>>>> try just "gpios".
>>>> Very true. I was rather even hoping we could update all bindings, I don't
>>>> mind going through the available dts files to fix them ... But need to know
>>>> that that's the proper way to go before doing the work ;)
>>> That will not work. You cannot make changes that require a new dtb
>>> with a new kernel. This would also break for the other way around
>>> (i.e. a new dtb and old kernel).
>>>
>>> You would have to search for both led-gpios and gpios. I'm not sure if
>>> we can do that generically for all GPIOs. If you had a node with both
>>> "blah-gpios" and "gpios", it would break. I would hope there are no
>>> such cases like that. We also now have to consider how ACPI identifies
>>> GPIOs and whether this makes sense.
>> I think only the driver itself can know about such "legacy" mappings and
>> make a decision.
> Yeah leds-gpio.c will need to be patched to check for "led-gpios" first
> and then fall back to "gpios" if not found.
yeah I've done the work, just need to double check it and resend a v2.

Linus, I assume you want the already applied patches omitted from v2?

Olliver
>
> Yours,
> Linus Walleij


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 4/4] leds: no longer use unnamed gpios
  2015-01-14 13:20             ` Olliver Schinagl
@ 2015-01-19  3:43               ` Alexandre Courbot
  2015-01-19  4:09                 ` Alexandre Courbot
  0 siblings, 1 reply; 22+ messages in thread
From: Alexandre Courbot @ 2015-01-19  3:43 UTC (permalink / raw)
  To: Olliver Schinagl
  Cc: Linus Walleij, Dmitry Torokhov, Rob Herring, Olliver Schinagl,
	Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Bryan Wu, Richard Purdie, Robin Gong, Rafael J. Wysocki,
	Aaron Lu, Mika Westerberg, Grant Likely, Wolfram Sang,
	Alexander Shiyan, Jingoo Han, devicetree, linux-kernel,
	linux-gpio, linux-input, linux-leds

On Wed, Jan 14, 2015 at 10:20 PM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>
> On 14-01-15 13:45, Linus Walleij wrote:
>>
>> On Thu, Jan 8, 2015 at 11:12 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>>>
>>> On Thu, Jan 08, 2015 at 08:40:20AM -0600, Rob Herring wrote:
>>>>
>>>> On Thu, Jan 8, 2015 at 2:45 AM, Olliver Schinagl <oliver@schinagl.nl>
>>>> wrote:
>>>>>>>
>>>>>>> --- a/drivers/leds/leds-gpio.c
>>>>>>> +++ b/drivers/leds/leds-gpio.c
>>>>>>> @@ -184,7 +184,7 @@ static struct gpio_leds_priv
>>>>>>> *gpio_leds_create(struct
>>>>>>> platform_device *pdev)
>>>>>>>                  struct gpio_led led = {};
>>>>>>>                  const char *state = NULL;
>>>>>>>    -             led.gpiod = devm_get_gpiod_from_child(dev, NULL,
>>>>>>> child);
>>>>>>> +               led.gpiod = devm_get_gpiod_from_child(dev, "led",
>>>>>>> child);
>>>>>>
>>>>>> Would not this break existing boards using old bindings? You need to
>>>>>> handle both cases: if you can't located "led-gpios" then you will have
>>>>>> to
>>>>>> try just "gpios".
>>>>>
>>>>> Very true. I was rather even hoping we could update all bindings, I
>>>>> don't
>>>>> mind going through the available dts files to fix them ... But need to
>>>>> know
>>>>> that that's the proper way to go before doing the work ;)
>>>>
>>>> That will not work. You cannot make changes that require a new dtb
>>>> with a new kernel. This would also break for the other way around
>>>> (i.e. a new dtb and old kernel).
>>>>
>>>> You would have to search for both led-gpios and gpios. I'm not sure if
>>>> we can do that generically for all GPIOs. If you had a node with both
>>>> "blah-gpios" and "gpios", it would break. I would hope there are no
>>>> such cases like that. We also now have to consider how ACPI identifies
>>>> GPIOs and whether this makes sense.
>>>
>>> I think only the driver itself can know about such "legacy" mappings and
>>> make a decision.
>>
>> Yeah leds-gpio.c will need to be patched to check for "led-gpios" first
>> and then fall back to "gpios" if not found.
>
> yeah I've done the work, just need to double check it and resend a v2.
>
> Linus, I assume you want the already applied patches omitted from v2?

Yes, please base new revisions on Linus' devel branch, omitting any
patches that he has already accepted.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios
  2015-01-07  9:08 ` [PATCH v1 2/4] gpio: add parameter to allow the use named gpios Olliver Schinagl
  2015-01-08  0:15   ` Dmitry Torokhov
  2015-01-14 12:36   ` Linus Walleij
@ 2015-01-19  4:04   ` Alexandre Courbot
  2015-01-21 21:44     ` Olliver Schinagl
  2 siblings, 1 reply; 22+ messages in thread
From: Alexandre Courbot @ 2015-01-19  4:04 UTC (permalink / raw)
  To: Olliver Schinagl
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Dmitry Torokhov, Bryan Wu, Richard Purdie,
	Olliver Schinagl, Robin Gong, Rafael J. Wysocki, Aaron Lu,
	Mika Westerberg, Grant Likely, Wolfram Sang, Alexander Shiyan,
	Jingoo Han, devicetree, Linux Kernel Mailing List, linux-gpio,
	Linux Input, linux-leds

On Wed, Jan 7, 2015 at 6:08 PM, Olliver Schinagl
<oliver+list@schinagl.nl> wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
>
> The gpio binding document says that new code should always use named
> gpios. Patch 40b73183 added support to parse a list of gpios from child
> nodes, but does not make it possible to use named gpios. This patch adds
> the con_id property and implements it is done in gpiolib.c, where the
> old-style of using unnamed gpios still works.

This is absolutely correct - thanks for spotting this.

>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  drivers/gpio/devres.c                     | 16 +++++++++++++++-
>  drivers/input/keyboard/gpio_keys_polled.c |  2 +-
>  drivers/leds/leds-gpio.c                  |  2 +-
>  include/linux/gpio/consumer.h             |  1 +
>  4 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c
> index 13dbd3d..b7fbe1c 100644
> --- a/drivers/gpio/devres.c
> +++ b/drivers/gpio/devres.c
> @@ -111,23 +111,37 @@ EXPORT_SYMBOL(__devm_gpiod_get_index);
>  /**
>   * devm_get_gpiod_from_child - get a GPIO descriptor from a device's child node
>   * @dev:       GPIO consumer
> + * @con_id:    function within the GPIO consumer
>   * @child:     firmware node (child of @dev)
>   *
>   * GPIO descriptors returned from this function are automatically disposed on
>   * driver detach.
>   */
>  struct gpio_desc *devm_get_gpiod_from_child(struct device *dev,
> +                                           const char *con_id,
>                                             struct fwnode_handle *child)
>  {
> +       static const char const *suffixes[] = { "gpios", "gpio" };
> +       char prop_name[32]; /* 32 is max size of property name */
>         struct gpio_desc **dr;
>         struct gpio_desc *desc;
> +       unsigned int i;
>
>         dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
>                           GFP_KERNEL);
>         if (!dr)
>                 return ERR_PTR(-ENOMEM);
>
> -       desc = fwnode_get_named_gpiod(child, "gpios");
> +       for (i = 0; i < ARRAY_SIZE(suffixes); i++) {
> +               if (con_id)
> +                       snprintf(prop_name, 32, "%s-%s", con_id, suffixes[i]);
> +               else
> +                       snprintf(prop_name, 32, "%s", suffixes[i]);

Same remark as Dmitry about the hardcoded length of the string, but ...

> +
> +               desc = fwnode_get_named_gpiod(child, prop_name);
> +               if (!IS_ERR(desc) || (PTR_ERR(desc) == -EPROBE_DEFER))
> +                       break;
> +       }

... since it looks like this part has been mostly copy/pasted from
of_find_gpio(), can you add another patch that fixes it there as well?

Also in the case of ACPI this will prove to be an incomplete lookup
since acpi_find_gpio() has an additional fallback if the named lookup
fails.

In that respect, I wonder if it would not be better for
devm_get_gpiod_from_child() to call of_find_gpio() and
acpi_find_gpio() (after making them non-static) followed by
gpiod_request() instead of calling fwnode_get_named_gpiod(). But in
that case it will have to do the OF/ACPI handling by itself.

I'm not really sure about which way is better. I'd appreciate if you
could give a thought to a possible refactoring that would improve the
situation ; otherwise feel free to ignore what I have written above
and to duplicate the property name building code.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 4/4] leds: no longer use unnamed gpios
  2015-01-19  3:43               ` Alexandre Courbot
@ 2015-01-19  4:09                 ` Alexandre Courbot
  0 siblings, 0 replies; 22+ messages in thread
From: Alexandre Courbot @ 2015-01-19  4:09 UTC (permalink / raw)
  To: Olliver Schinagl
  Cc: Linus Walleij, Dmitry Torokhov, Rob Herring, Olliver Schinagl,
	Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Bryan Wu, Richard Purdie, Robin Gong, Rafael J. Wysocki,
	Aaron Lu, Mika Westerberg, Grant Likely, Wolfram Sang,
	Alexander Shiyan, Jingoo Han, devicetree, linux-kernel,
	linux-gpio, linux-input, linux-leds

On Mon, Jan 19, 2015 at 12:43 PM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Wed, Jan 14, 2015 at 10:20 PM, Olliver Schinagl <oliver@schinagl.nl> wrote:
>>
>> On 14-01-15 13:45, Linus Walleij wrote:
>>>
>>> On Thu, Jan 8, 2015 at 11:12 PM, Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
>>>>
>>>> On Thu, Jan 08, 2015 at 08:40:20AM -0600, Rob Herring wrote:
>>>>>
>>>>> On Thu, Jan 8, 2015 at 2:45 AM, Olliver Schinagl <oliver@schinagl.nl>
>>>>> wrote:
>>>>>>>>
>>>>>>>> --- a/drivers/leds/leds-gpio.c
>>>>>>>> +++ b/drivers/leds/leds-gpio.c
>>>>>>>> @@ -184,7 +184,7 @@ static struct gpio_leds_priv
>>>>>>>> *gpio_leds_create(struct
>>>>>>>> platform_device *pdev)
>>>>>>>>                  struct gpio_led led = {};
>>>>>>>>                  const char *state = NULL;
>>>>>>>>    -             led.gpiod = devm_get_gpiod_from_child(dev, NULL,
>>>>>>>> child);
>>>>>>>> +               led.gpiod = devm_get_gpiod_from_child(dev, "led",
>>>>>>>> child);
>>>>>>>
>>>>>>> Would not this break existing boards using old bindings? You need to
>>>>>>> handle both cases: if you can't located "led-gpios" then you will have
>>>>>>> to
>>>>>>> try just "gpios".
>>>>>>
>>>>>> Very true. I was rather even hoping we could update all bindings, I
>>>>>> don't
>>>>>> mind going through the available dts files to fix them ... But need to
>>>>>> know
>>>>>> that that's the proper way to go before doing the work ;)
>>>>>
>>>>> That will not work. You cannot make changes that require a new dtb
>>>>> with a new kernel. This would also break for the other way around
>>>>> (i.e. a new dtb and old kernel).
>>>>>
>>>>> You would have to search for both led-gpios and gpios. I'm not sure if
>>>>> we can do that generically for all GPIOs. If you had a node with both
>>>>> "blah-gpios" and "gpios", it would break. I would hope there are no
>>>>> such cases like that. We also now have to consider how ACPI identifies
>>>>> GPIOs and whether this makes sense.
>>>>
>>>> I think only the driver itself can know about such "legacy" mappings and
>>>> make a decision.
>>>
>>> Yeah leds-gpio.c will need to be patched to check for "led-gpios" first
>>> and then fall back to "gpios" if not found.
>>
>> yeah I've done the work, just need to double check it and resend a v2.
>>
>> Linus, I assume you want the already applied patches omitted from v2?
>
> Yes, please base new revisions on Linus' devel branch, omitting any
> patches that he has already accepted.

Actually on top of breaking backward compatibility, I think the case
of LED GPIOs is one of thoses where it makes sense to not have a name
(the GPIO usage is obvious from the DT hierarchy, and there cannot be
another one). So I don't feel like this change is really needed (other
patches in this series are still useful though).

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios
  2015-01-19  4:04   ` Alexandre Courbot
@ 2015-01-21 21:44     ` Olliver Schinagl
  2015-01-23  9:16       ` Alexandre Courbot
  0 siblings, 1 reply; 22+ messages in thread
From: Olliver Schinagl @ 2015-01-21 21:44 UTC (permalink / raw)
  To: Alexandre Courbot, Olliver Schinagl
  Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
	Linus Walleij, Dmitry Torokhov, Bryan Wu, Richard Purdie,
	Robin Gong, Rafael J. Wysocki, Aaron Lu, Mika Westerberg,
	Grant Likely, Wolfram Sang, Alexander Shiyan, Jingoo Han,
	devicetree, Linux Kernel Mailing List, linux-gpio, Linux Input,
	linux-leds

Hey Alexandre,

On 01/19/2015 05:04 AM, Alexandre Courbot wrote:
> On Wed, Jan 7, 2015 at 6:08 PM, Olliver Schinagl
> <oliver+list@schinagl.nl> wrote:
>> From: Olliver Schinagl <oliver@schinagl.nl>
>>
>> The gpio binding document says that new code should always use named
>> gpios. Patch 40b73183 added support to parse a list of gpios from child
>> nodes, but does not make it possible to use named gpios. This patch adds
>> the con_id property and implements it is done in gpiolib.c, where the
>> old-style of using unnamed gpios still works.
> This is absolutely correct - thanks for spotting this.
>
> <snip>
> ... since it looks like this part has been mostly copy/pasted from
> of_find_gpio(), can you add another patch that fixes it there as well?
Yeah, since it has the same functionality, i copy pasted it. Wasn't sure 
if it was worth to macro it or anything. I've sent a v2 with that patch 
added to the mix :)
>
> Also in the case of ACPI this will prove to be an incomplete lookup
> since acpi_find_gpio() has an additional fallback if the named lookup
> fails.
I'm not very familiar (or at all) how ACPI falls into all of this, I'm 
just starting to get a hang of the DT, but since this is how the dts is 
being parsed, where is the relation here? Or did I misunderstand?
>
> In that respect, I wonder if it would not be better for
> devm_get_gpiod_from_child() to call of_find_gpio() and
> acpi_find_gpio() (after making them non-static) followed by
> gpiod_request() instead of calling fwnode_get_named_gpiod(). But in
> that case it will have to do the OF/ACPI handling by itself.
>
> I'm not really sure about which way is better. I'd appreciate if you
> could give a thought to a possible refactoring that would improve the
> situation ; otherwise feel free to ignore what I have written above
> and to duplicate the property name building code.
I'm afraid I'm a little too inexperienced to follow exactly what you say ;)

Olliver


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios
  2015-01-21 21:44     ` Olliver Schinagl
@ 2015-01-23  9:16       ` Alexandre Courbot
  0 siblings, 0 replies; 22+ messages in thread
From: Alexandre Courbot @ 2015-01-23  9:16 UTC (permalink / raw)
  To: Olliver Schinagl
  Cc: Olliver Schinagl, Rob Herring, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, Linus Walleij, Dmitry Torokhov,
	Bryan Wu, Richard Purdie, Robin Gong, Rafael J. Wysocki,
	Aaron Lu, Mika Westerberg, Grant Likely, Wolfram Sang,
	Alexander Shiyan, Jingoo Han, devicetree,
	Linux Kernel Mailing List, linux-gpio, Linux Input, linux-leds

On Thu, Jan 22, 2015 at 6:44 AM, Olliver Schinagl <oliver@schinagl.nl> wrote:
> Hey Alexandre,
>
> On 01/19/2015 05:04 AM, Alexandre Courbot wrote:
>>
>> On Wed, Jan 7, 2015 at 6:08 PM, Olliver Schinagl
>> <oliver+list@schinagl.nl> wrote:
>>>
>>> From: Olliver Schinagl <oliver@schinagl.nl>
>>>
>>> The gpio binding document says that new code should always use named
>>> gpios. Patch 40b73183 added support to parse a list of gpios from child
>>> nodes, but does not make it possible to use named gpios. This patch adds
>>> the con_id property and implements it is done in gpiolib.c, where the
>>> old-style of using unnamed gpios still works.
>>
>> This is absolutely correct - thanks for spotting this.
>>
>> <snip>
>> ... since it looks like this part has been mostly copy/pasted from
>> of_find_gpio(), can you add another patch that fixes it there as well?
>
> Yeah, since it has the same functionality, i copy pasted it. Wasn't sure if
> it was worth to macro it or anything. I've sent a v2 with that patch added
> to the mix :)
>>
>>
>> Also in the case of ACPI this will prove to be an incomplete lookup
>> since acpi_find_gpio() has an additional fallback if the named lookup
>> fails.
>
> I'm not very familiar (or at all) how ACPI falls into all of this, I'm just
> starting to get a hang of the DT, but since this is how the dts is being
> parsed, where is the relation here? Or did I misunderstand?

GPIO mappings can be provided by DT or by ACPI. In the latter case
there is a fallback method if the name does not match (see
acpi_find_gpio()). fwnode_get_named_gpiod() does not check it though,
so maybe we can just ignore that.

>> In that respect, I wonder if it would not be better for
>> devm_get_gpiod_from_child() to call of_find_gpio() and
>> acpi_find_gpio() (after making them non-static) followed by
>> gpiod_request() instead of calling fwnode_get_named_gpiod(). But in
>> that case it will have to do the OF/ACPI handling by itself.
>>
>> I'm not really sure about which way is better. I'd appreciate if you
>> could give a thought to a possible refactoring that would improve the
>> situation ; otherwise feel free to ignore what I have written above
>> and to duplicate the property name building code.
>
> I'm afraid I'm a little too inexperienced to follow exactly what you say ;)

Don't worry about it then, this patch is already an improvement. GPIO
mappings lookup from firmware has become kind of messy, and I'm just
trying to enroll people to clean it instead of doing it myself. ;)

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2015-01-23  9:16 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-07  9:08 [PATCH v1 0/4] Let leds use named gpios Olliver Schinagl
2015-01-07  9:08 ` [PATCH v1 1/4] gpio:gpiolib: use static const char const * for a suffixes array Olliver Schinagl
2015-01-14 12:35   ` Linus Walleij
2015-01-07  9:08 ` [PATCH v1 2/4] gpio: add parameter to allow the use named gpios Olliver Schinagl
2015-01-08  0:15   ` Dmitry Torokhov
2015-01-14 12:36   ` Linus Walleij
2015-01-19  4:04   ` Alexandre Courbot
2015-01-21 21:44     ` Olliver Schinagl
2015-01-23  9:16       ` Alexandre Courbot
2015-01-07  9:08 ` [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings Olliver Schinagl
2015-01-07 23:56   ` Dmitry Torokhov
2015-01-08  1:45   ` Rob Herring
2015-01-14 12:38   ` Linus Walleij
2015-01-07  9:08 ` [PATCH v1 4/4] leds: no longer use unnamed gpios Olliver Schinagl
2015-01-07 23:55   ` Dmitry Torokhov
2015-01-08  8:45     ` Olliver Schinagl
2015-01-08 14:40       ` Rob Herring
2015-01-08 22:12         ` Dmitry Torokhov
2015-01-14 12:45           ` Linus Walleij
2015-01-14 13:20             ` Olliver Schinagl
2015-01-19  3:43               ` Alexandre Courbot
2015-01-19  4:09                 ` Alexandre Courbot

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).