[PATCH v1 0/4] Let leds use named gpios

[PATCH v1 0/4] Let leds use named gpios

[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

[PATCH v1 1/4] gpio:gpiolib: use static const char const * for a suffixes array

[Olliver Schinagl]

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

[PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Olliver Schinagl]

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

[PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings

[Olliver Schinagl]

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

[PATCH v1 4/4] leds: no longer use unnamed gpios

[Olliver Schinagl]

From: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>

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-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 4/4] leds: no longer use unnamed gpios

[Olliver Schinagl]

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

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Dmitry Torokhov]

On Wed, Jan 07, 2015 at 10:08:42AM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl <oliver-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
> 
> 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-dxLnbx3+1qmEVqv0pETR8A@public.gmane.org>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Dmitry Torokhov]

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

Re: [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings

[Dmitry Torokhov]

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

Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Dmitry Torokhov]

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

Re: [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings

[Rob Herring]

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
>

Re: [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings

[Rob Herring]

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
>

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Olliver Schinagl]

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

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Rob Herring]

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

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Rob Herring]

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

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Dmitry Torokhov]

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

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Dmitry Torokhov]

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

Re: [PATCH v1 1/4] gpio:gpiolib: use static const char const * for a suffixes array

[Linus Walleij]

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

Re: [PATCH v1 1/4] gpio:gpiolib: use static const char const * for a suffixes array

[Linus Walleij]

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

Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Linus Walleij]

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

Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Linus Walleij]

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

Re: [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings

[Linus Walleij]

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

Re: [PATCH v1 3/4] leds: Let the binding document example for leds-gpio follow the gpio bindings

[Linus Walleij]

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

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Linus Walleij]

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

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Linus Walleij]

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

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Olliver Schinagl]

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

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Olliver Schinagl]

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

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Alexandre Courbot]

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.

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Alexandre Courbot]

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.

Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Alexandre Courbot]

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.

Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Alexandre Courbot]

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.

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Alexandre Courbot]

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

Re: [PATCH v1 4/4] leds: no longer use unnamed gpios

[Alexandre Courbot]

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

Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Olliver Schinagl]

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

Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Olliver Schinagl]

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

Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Alexandre Courbot]

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

Re: [PATCH v1 2/4] gpio: add parameter to allow the use named gpios

[Alexandre Courbot]

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