linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] gpio: of: Add error handling and support for multiple gpio in gpio-hog
@ 2016-03-08 12:02 Laxman Dewangan
  2016-03-08 12:02 ` [PATCH 1/5] gpio: of: Scan available child node for gpio-hog Laxman Dewangan
                   ` (4 more replies)
  0 siblings, 5 replies; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-08 12:02 UTC (permalink / raw)
  To: linus.walleij, robh+dt
  Cc: pawel.moll, mark.rutland, devicetree, linux-kernel, linux-gpio,
	treding, swarren, Laxman Dewangan

This series enhance the error print by adding error number in print message,
handle the error if gpio_hogd() fails and returns error to caller, and
add the support of multiple GPIOs to be passed from property "gpios" under
gpio hogd node.

Laxman Dewangan (5):
  gpio: of: Scan available child node for gpio-hog
  gpio: gpiolib: Print error number if gpio hog failed
  gpio: of: Return error if gpio hog configuration failed
  gpio: of: Add support to have multiple gpios in gpio-hog
  gpio: DT: Rephrase property "gpios" of hog node to support multiple
    gpios

 Documentation/devicetree/bindings/gpio/gpio.txt |  6 +-
 drivers/gpio/gpiolib-of.c                       | 73 +++++++++++++++++++------
 drivers/gpio/gpiolib.c                          |  9 +--
 3 files changed, 63 insertions(+), 25 deletions(-)

-- 
2.1.4

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

* [PATCH 1/5] gpio: of: Scan available child node for gpio-hog
  2016-03-08 12:02 [PATCH 0/5] gpio: of: Add error handling and support for multiple gpio in gpio-hog Laxman Dewangan
@ 2016-03-08 12:02 ` Laxman Dewangan
  2016-03-08 14:18   ` Thierry Reding
  2016-03-08 12:02 ` [PATCH 2/5] gpio: gpiolib: Print error number if gpio hog failed Laxman Dewangan
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-08 12:02 UTC (permalink / raw)
  To: linus.walleij, robh+dt
  Cc: pawel.moll, mark.rutland, devicetree, linux-kernel, linux-gpio,
	treding, swarren, Laxman Dewangan

Look for child node which are available when iterating for
gpio hog node for request/set GPIO initial configuration
during OF gpio chip registration.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/gpio/gpiolib-of.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 42a4bb7..f2ba1a4 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -210,7 +210,7 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 	enum gpio_lookup_flags lflags;
 	enum gpiod_flags dflags;
 
-	for_each_child_of_node(chip->of_node, np) {
+	for_each_available_child_of_node(chip->of_node, np) {
 		if (!of_property_read_bool(np, "gpio-hog"))
 			continue;
 
-- 
2.1.4

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

* [PATCH 2/5] gpio: gpiolib: Print error number if gpio hog failed
  2016-03-08 12:02 [PATCH 0/5] gpio: of: Add error handling and support for multiple gpio in gpio-hog Laxman Dewangan
  2016-03-08 12:02 ` [PATCH 1/5] gpio: of: Scan available child node for gpio-hog Laxman Dewangan
@ 2016-03-08 12:02 ` Laxman Dewangan
  2016-03-08 12:27   ` Vladimir Zapolskiy
  2016-03-08 14:22   ` Thierry Reding
  2016-03-08 12:02 ` [PATCH 3/5] gpio: of: Return error if gpio hog configuration failed Laxman Dewangan
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-08 12:02 UTC (permalink / raw)
  To: linus.walleij, robh+dt
  Cc: pawel.moll, mark.rutland, devicetree, linux-kernel, linux-gpio,
	treding, swarren, Laxman Dewangan

Print the error number of GPIO hog failed during
its configurations. This helps in identifying the
failure without instrumenting the code.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/gpio/gpiolib.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bc788b9..7575ebb 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2621,15 +2621,16 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 
 	local_desc = gpiochip_request_own_desc(chip, hwnum, name);
 	if (IS_ERR(local_desc)) {
-		pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
-		       name, chip->label, hwnum);
+		status = PTR_ERR(local_desc);
+		pr_err("requesting hog GPIO %s, chip %s, offset %d failed %d\n",
+		       name, chip->label, hwnum, status);
 		return PTR_ERR(local_desc);
 	}
 
 	status = gpiod_configure_flags(desc, name, dflags);
 	if (status < 0) {
-		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
-		       name, chip->label, hwnum);
+		pr_err("setup of hog GPIO %s chip %s, offset %d failed %d\n",
+		       name, chip->label, hwnum, status);
 		gpiochip_free_own_desc(desc);
 		return status;
 	}
-- 
2.1.4

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

* [PATCH 3/5] gpio: of: Return error if gpio hog configuration failed
  2016-03-08 12:02 [PATCH 0/5] gpio: of: Add error handling and support for multiple gpio in gpio-hog Laxman Dewangan
  2016-03-08 12:02 ` [PATCH 1/5] gpio: of: Scan available child node for gpio-hog Laxman Dewangan
  2016-03-08 12:02 ` [PATCH 2/5] gpio: gpiolib: Print error number if gpio hog failed Laxman Dewangan
@ 2016-03-08 12:02 ` Laxman Dewangan
  2016-03-08 14:23   ` Thierry Reding
  2016-03-09 17:11   ` Stephen Warren
  2016-03-08 12:02 ` [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog Laxman Dewangan
  2016-03-08 12:02 ` [PATCH 5/5] gpio: DT: Rephrase property "gpios" of hog node to support multiple gpios Laxman Dewangan
  4 siblings, 2 replies; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-08 12:02 UTC (permalink / raw)
  To: linus.walleij, robh+dt
  Cc: pawel.moll, mark.rutland, devicetree, linux-kernel, linux-gpio,
	treding, swarren, Laxman Dewangan, Benoit Parrot,
	Alexandre Courbot

If GPIO hog configuration failed while adding OF based
gpiochip() then return the error instead of ignoring it.

This helps of properly handling the gpio driver dependency.

When adding the gpio hog nodes for NVIDIA's Tegra210 platforms,
the gpio_hogd() fails with EPROBE_DEFER because pinctrl is not
ready at this time and gpio_request() for Tegra GPIO driver
returns error. The error was not causing the Tegra GPIO driver
to fail as the error was getting ignored.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Cc: Benoit Parrot <bparrot@ti.com>
Cc: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib-of.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index f2ba1a4..d81dbd8 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -201,14 +201,16 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
  *
  * This is only used by of_gpiochip_add to request/set GPIO initial
  * configuration.
+ * It retures error if it fails otherwise 0 on success.
  */
-static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
+static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
 {
 	struct gpio_desc *desc = NULL;
 	struct device_node *np;
 	const char *name;
 	enum gpio_lookup_flags lflags;
 	enum gpiod_flags dflags;
+	int ret;
 
 	for_each_available_child_of_node(chip->of_node, np) {
 		if (!of_property_read_bool(np, "gpio-hog"))
@@ -218,9 +220,12 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 		if (IS_ERR(desc))
 			continue;
 
-		if (gpiod_hog(desc, name, lflags, dflags))
-			continue;
+		ret = gpiod_hog(desc, name, lflags, dflags);
+		if (ret < 0)
+			return ret;
 	}
+
+	return 0;
 }
 
 /**
@@ -442,9 +447,7 @@ int of_gpiochip_add(struct gpio_chip *chip)
 
 	of_node_get(chip->of_node);
 
-	of_gpiochip_scan_gpios(chip);
-
-	return 0;
+	return of_gpiochip_scan_gpios(chip);
 }
 
 void of_gpiochip_remove(struct gpio_chip *chip)
-- 
2.1.4

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

* [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog
  2016-03-08 12:02 [PATCH 0/5] gpio: of: Add error handling and support for multiple gpio in gpio-hog Laxman Dewangan
                   ` (2 preceding siblings ...)
  2016-03-08 12:02 ` [PATCH 3/5] gpio: of: Return error if gpio hog configuration failed Laxman Dewangan
@ 2016-03-08 12:02 ` Laxman Dewangan
  2016-03-09  6:28   ` Markus Pargmann
  2016-03-08 12:02 ` [PATCH 5/5] gpio: DT: Rephrase property "gpios" of hog node to support multiple gpios Laxman Dewangan
  4 siblings, 1 reply; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-08 12:02 UTC (permalink / raw)
  To: linus.walleij, robh+dt
  Cc: pawel.moll, mark.rutland, devicetree, linux-kernel, linux-gpio,
	treding, swarren, Laxman Dewangan, Benoit Parrot,
	Alexandre Courbot

The child node for gpio hogs under gpio controller's node
provide the mechanism to automatic GPIO request and
configuration as part of the gpio-controller's driver
probe function.

Currently, property "gpio" takes one gpios for such
configuration. Add support to have multiple GPIOs in
this property so that multiple GPIOs of gpio-controller
can be configured by this mechanism with one child node.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Cc: Benoit Parrot <bparrot@ti.com>
Cc: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib-of.c | 64 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index d81dbd8..0e4e8fd 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -118,6 +118,21 @@ int of_get_named_gpio_flags(struct device_node *np, const char *list_name,
 }
 EXPORT_SYMBOL(of_get_named_gpio_flags);
 
+static int of_gpio_get_gpio_cells_size(struct device_node *chip_np)
+{
+	u32 ncells;
+	int ret;
+
+	ret = of_property_read_u32(chip_np, "#gpio-cells", &ncells);
+	if (ret)
+		return ret;
+
+	if (ncells > MAX_PHANDLE_ARGS)
+		return -EINVAL;
+
+	return ncells;
+}
+
 /**
  * of_parse_own_gpio() - Get a GPIO hog descriptor, names and flags for GPIO API
  * @np:		device node to get GPIO from
@@ -131,6 +146,7 @@ EXPORT_SYMBOL(of_get_named_gpio_flags);
  */
 static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
 					   const char **name,
+					   int gpio_index,
 					   enum gpio_lookup_flags *lflags,
 					   enum gpiod_flags *dflags)
 {
@@ -139,8 +155,8 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
 	struct gg_data gg_data = {
 		.flags = &xlate_flags,
 	};
-	u32 tmp;
-	int i, ret;
+	int ncells;
+	int i, start_index, ret;
 
 	chip_np = np->parent;
 	if (!chip_np)
@@ -150,17 +166,16 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
 	*lflags = 0;
 	*dflags = 0;
 
-	ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
-	if (ret)
-		return ERR_PTR(ret);
+	ncells = of_gpio_get_gpio_cells_size(chip_np);
+	if (ncells < 0)
+		return ERR_PTR(ncells);
 
-	if (tmp > MAX_PHANDLE_ARGS)
-		return ERR_PTR(-EINVAL);
+	start_index = ncells * gpio_index;
 
-	gg_data.gpiospec.args_count = tmp;
+	gg_data.gpiospec.args_count = ncells;
 	gg_data.gpiospec.np = chip_np;
-	for (i = 0; i < tmp; i++) {
-		ret = of_property_read_u32_index(np, "gpios", i,
+	for (i = 0; i < ncells; i++) {
+		ret = of_property_read_u32_index(np, "gpios", start_index + i,
 					   &gg_data.gpiospec.args[i]);
 		if (ret)
 			return ERR_PTR(ret);
@@ -211,18 +226,37 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
 	enum gpio_lookup_flags lflags;
 	enum gpiod_flags dflags;
 	int ret;
+	int i, ncells, ngpios;
+
+	ncells = of_gpio_get_gpio_cells_size(chip->of_node);
+	if (ncells < 0)
+		return 0;
 
 	for_each_available_child_of_node(chip->of_node, np) {
 		if (!of_property_read_bool(np, "gpio-hog"))
 			continue;
 
-		desc = of_parse_own_gpio(np, &name, &lflags, &dflags);
-		if (IS_ERR(desc))
+		ngpios = of_property_count_u32_elems(np, "gpios");
+		if (ngpios < 0)
+			continue;
+
+		if (ngpios % ncells) {
+			dev_warn(chip->parent,
+				"GPIOs entries are not proper in gpios\n");
 			continue;
+		}
+
+		ngpios /= ncells;
+		for (i = 0; i < ngpios; i++) {
+			desc = of_parse_own_gpio(np, &name, i,
+						 &lflags, &dflags);
+			if (IS_ERR(desc))
+				continue;
 
-		ret = gpiod_hog(desc, name, lflags, dflags);
-		if (ret < 0)
-			return ret;
+			ret = gpiod_hog(desc, name, lflags, dflags);
+			if (ret < 0)
+				return ret;
+		}
 	}
 
 	return 0;
-- 
2.1.4

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

* [PATCH 5/5] gpio: DT: Rephrase property "gpios" of hog node to support multiple gpios
  2016-03-08 12:02 [PATCH 0/5] gpio: of: Add error handling and support for multiple gpio in gpio-hog Laxman Dewangan
                   ` (3 preceding siblings ...)
  2016-03-08 12:02 ` [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog Laxman Dewangan
@ 2016-03-08 12:02 ` Laxman Dewangan
  2016-03-09 17:18   ` Stephen Warren
  4 siblings, 1 reply; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-08 12:02 UTC (permalink / raw)
  To: linus.walleij, robh+dt
  Cc: pawel.moll, mark.rutland, devicetree, linux-kernel, linux-gpio,
	treding, swarren, Laxman Dewangan

The property "gpios" of GPIO hog node support the multiple GPIO entries.
Rephrase the details of this property for this new support.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 Documentation/devicetree/bindings/gpio/gpio.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index 069cdf6..ea5d7df 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -162,9 +162,9 @@ gpio-controller's driver probe function.
 Each GPIO hog definition is represented as a child node of the GPIO controller.
 Required properties:
 - gpio-hog:   A property specifying that this child node represent a GPIO hog.
-- gpios:      Store the GPIO information (id, flags, ...). Shall contain the
-	      number of cells specified in its parent node (GPIO controller
-	      node).
+- gpios:      Store the GPIO information (id, flags, ...). Multiple GPIOs are
+	      possible to list here. Shall contain the number of cells
+	      specified in its parent node (GPIO controller node) per GPIOs.
 Only one of the following properties scanned in the order shown below.
 This means that when multiple properties are present they will be searched
 in the order presented below and the first match is taken as the intended
-- 
2.1.4

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

* Re: [PATCH 2/5] gpio: gpiolib: Print error number if gpio hog failed
  2016-03-08 12:02 ` [PATCH 2/5] gpio: gpiolib: Print error number if gpio hog failed Laxman Dewangan
@ 2016-03-08 12:27   ` Vladimir Zapolskiy
  2016-03-08 14:22   ` Thierry Reding
  1 sibling, 0 replies; 24+ messages in thread
From: Vladimir Zapolskiy @ 2016-03-08 12:27 UTC (permalink / raw)
  To: Laxman Dewangan, linus.walleij, robh+dt
  Cc: pawel.moll, mark.rutland, devicetree, linux-kernel, linux-gpio,
	treding, swarren

On 08.03.2016 14:02, Laxman Dewangan wrote:
> Print the error number of GPIO hog failed during
> its configurations. This helps in identifying the
> failure without instrumenting the code.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/gpio/gpiolib.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index bc788b9..7575ebb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2621,15 +2621,16 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
>  
>  	local_desc = gpiochip_request_own_desc(chip, hwnum, name);
>  	if (IS_ERR(local_desc)) {
> -		pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
> -		       name, chip->label, hwnum);
> +		status = PTR_ERR(local_desc);
> +		pr_err("requesting hog GPIO %s, chip %s, offset %d failed %d\n",
> +		       name, chip->label, hwnum, status);
>  		return PTR_ERR(local_desc);

You can do "return status;" now.

>  	}
>  
>  	status = gpiod_configure_flags(desc, name, dflags);
>  	if (status < 0) {
> -		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
> -		       name, chip->label, hwnum);
> +		pr_err("setup of hog GPIO %s chip %s, offset %d failed %d\n",
> +		       name, chip->label, hwnum, status);
>  		gpiochip_free_own_desc(desc);
>  		return status;
>  	}
> 

--
With best wishes,
Vladimir

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

* Re: [PATCH 1/5] gpio: of: Scan available child node for gpio-hog
  2016-03-08 12:02 ` [PATCH 1/5] gpio: of: Scan available child node for gpio-hog Laxman Dewangan
@ 2016-03-08 14:18   ` Thierry Reding
  0 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2016-03-08 14:18 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: linus.walleij, robh+dt, pawel.moll, mark.rutland, devicetree,
	linux-kernel, linux-gpio, swarren

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

On Tue, Mar 08, 2016 at 05:32:04PM +0530, Laxman Dewangan wrote:
> Look for child node which are available when iterating for
> gpio hog node for request/set GPIO initial configuration
> during OF gpio chip registration.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/gpio/gpiolib-of.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 42a4bb7..f2ba1a4 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -210,7 +210,7 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
>  	enum gpio_lookup_flags lflags;
>  	enum gpiod_flags dflags;
>  
> -	for_each_child_of_node(chip->of_node, np) {
> +	for_each_available_child_of_node(chip->of_node, np) {
>  		if (!of_property_read_bool(np, "gpio-hog"))
>  			continue;

Makes sense:

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

* Re: [PATCH 2/5] gpio: gpiolib: Print error number if gpio hog failed
  2016-03-08 12:02 ` [PATCH 2/5] gpio: gpiolib: Print error number if gpio hog failed Laxman Dewangan
  2016-03-08 12:27   ` Vladimir Zapolskiy
@ 2016-03-08 14:22   ` Thierry Reding
  2016-03-08 15:32     ` Laxman Dewangan
  1 sibling, 1 reply; 24+ messages in thread
From: Thierry Reding @ 2016-03-08 14:22 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: linus.walleij, robh+dt, pawel.moll, mark.rutland, devicetree,
	linux-kernel, linux-gpio, swarren

[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]

On Tue, Mar 08, 2016 at 05:32:05PM +0530, Laxman Dewangan wrote:
> Print the error number of GPIO hog failed during
> its configurations. This helps in identifying the
> failure without instrumenting the code.

Please use up all 72 characters per line at your disposal. Excessively
short lines are hard to read.

> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/gpio/gpiolib.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index bc788b9..7575ebb 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2621,15 +2621,16 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
>  
>  	local_desc = gpiochip_request_own_desc(chip, hwnum, name);
>  	if (IS_ERR(local_desc)) {
> -		pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
> -		       name, chip->label, hwnum);
> +		status = PTR_ERR(local_desc);
> +		pr_err("requesting hog GPIO %s, chip %s, offset %d failed %d\n",
> +		       name, chip->label, hwnum, status);

I find this type of format hard to read. I prefer a semi-colon to
separate the message from the failure reason (i.e. error code).

Besides that I don't understand why you're dropping the parentheses
around the "chip %s, offset %d", I found that easier on the eye.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

* Re: [PATCH 3/5] gpio: of: Return error if gpio hog configuration failed
  2016-03-08 12:02 ` [PATCH 3/5] gpio: of: Return error if gpio hog configuration failed Laxman Dewangan
@ 2016-03-08 14:23   ` Thierry Reding
  2016-03-09 17:11   ` Stephen Warren
  1 sibling, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2016-03-08 14:23 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: linus.walleij, robh+dt, pawel.moll, mark.rutland, devicetree,
	linux-kernel, linux-gpio, swarren, Benoit Parrot,
	Alexandre Courbot

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

On Tue, Mar 08, 2016 at 05:32:06PM +0530, Laxman Dewangan wrote:
> If GPIO hog configuration failed while adding OF based
> gpiochip() then return the error instead of ignoring it.
> 
> This helps of properly handling the gpio driver dependency.
> 
> When adding the gpio hog nodes for NVIDIA's Tegra210 platforms,
> the gpio_hogd() fails with EPROBE_DEFER because pinctrl is not
> ready at this time and gpio_request() for Tegra GPIO driver
> returns error. The error was not causing the Tegra GPIO driver
> to fail as the error was getting ignored.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> Cc: Benoit Parrot <bparrot@ti.com>
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpio/gpiolib-of.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)

Reviewed-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

* Re: [PATCH 2/5] gpio: gpiolib: Print error number if gpio hog failed
  2016-03-08 14:22   ` Thierry Reding
@ 2016-03-08 15:32     ` Laxman Dewangan
  2016-03-09 17:07       ` Stephen Warren
  0 siblings, 1 reply; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-08 15:32 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linus.walleij, robh+dt, pawel.moll, mark.rutland, devicetree,
	linux-kernel, linux-gpio, swarren


On Tuesday 08 March 2016 07:52 PM, Thierry Reding wrote:
> On Tue, Mar 08, 2016 at 05:32:05PM +0530, Laxman Dewangan wrote:
>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>> ---
>>   drivers/gpio/gpiolib.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index bc788b9..7575ebb 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -2621,15 +2621,16 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
>>   
>>   	local_desc = gpiochip_request_own_desc(chip, hwnum, name);
>>   	if (IS_ERR(local_desc)) {
>> -		pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
>> -		       name, chip->label, hwnum);
>> +		status = PTR_ERR(local_desc);
>> +		pr_err("requesting hog GPIO %s, chip %s, offset %d failed %d\n",
>> +		       name, chip->label, hwnum, status);
> I find this type of format hard to read. I prefer a semi-colon to
> separate the message from the failure reason (i.e. error code).
>
> Besides that I don't understand why you're dropping the parentheses
> around the "chip %s, offset %d", I found that easier on the eye.
>
>


I did to accommodate the  3 extra character ( %d) for string format on 
that line as it was already near to 80 column.
Just did not want to split in multiple lines.

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

* Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog
  2016-03-08 12:02 ` [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog Laxman Dewangan
@ 2016-03-09  6:28   ` Markus Pargmann
  2016-03-09 13:20     ` Laxman Dewangan
  0 siblings, 1 reply; 24+ messages in thread
From: Markus Pargmann @ 2016-03-09  6:28 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: linus.walleij, robh+dt, pawel.moll, mark.rutland, devicetree,
	linux-kernel, linux-gpio, treding, swarren, Benoit Parrot,
	Alexandre Courbot

[-- Attachment #1: Type: text/plain, Size: 4874 bytes --]

Hi,

On Tue, Mar 08, 2016 at 05:32:07PM +0530, Laxman Dewangan wrote:
> The child node for gpio hogs under gpio controller's node
> provide the mechanism to automatic GPIO request and
> configuration as part of the gpio-controller's driver
> probe function.
> 
> Currently, property "gpio" takes one gpios for such
> configuration. Add support to have multiple GPIOs in
> this property so that multiple GPIOs of gpio-controller
> can be configured by this mechanism with one child node.

So if I read this correctly you want to have multiple GPIOs with the
same line name? Why don't you use multiple child nodes with individual
line names?

Best Regards,

Markus

> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> Cc: Benoit Parrot <bparrot@ti.com>
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpio/gpiolib-of.c | 64 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 49 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index d81dbd8..0e4e8fd 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -118,6 +118,21 @@ int of_get_named_gpio_flags(struct device_node *np, const char *list_name,
>  }
>  EXPORT_SYMBOL(of_get_named_gpio_flags);
>  
> +static int of_gpio_get_gpio_cells_size(struct device_node *chip_np)
> +{
> +	u32 ncells;
> +	int ret;
> +
> +	ret = of_property_read_u32(chip_np, "#gpio-cells", &ncells);
> +	if (ret)
> +		return ret;
> +
> +	if (ncells > MAX_PHANDLE_ARGS)
> +		return -EINVAL;
> +
> +	return ncells;
> +}
> +
>  /**
>   * of_parse_own_gpio() - Get a GPIO hog descriptor, names and flags for GPIO API
>   * @np:		device node to get GPIO from
> @@ -131,6 +146,7 @@ EXPORT_SYMBOL(of_get_named_gpio_flags);
>   */
>  static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
>  					   const char **name,
> +					   int gpio_index,
>  					   enum gpio_lookup_flags *lflags,
>  					   enum gpiod_flags *dflags)
>  {
> @@ -139,8 +155,8 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
>  	struct gg_data gg_data = {
>  		.flags = &xlate_flags,
>  	};
> -	u32 tmp;
> -	int i, ret;
> +	int ncells;
> +	int i, start_index, ret;
>  
>  	chip_np = np->parent;
>  	if (!chip_np)
> @@ -150,17 +166,16 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
>  	*lflags = 0;
>  	*dflags = 0;
>  
> -	ret = of_property_read_u32(chip_np, "#gpio-cells", &tmp);
> -	if (ret)
> -		return ERR_PTR(ret);
> +	ncells = of_gpio_get_gpio_cells_size(chip_np);
> +	if (ncells < 0)
> +		return ERR_PTR(ncells);
>  
> -	if (tmp > MAX_PHANDLE_ARGS)
> -		return ERR_PTR(-EINVAL);
> +	start_index = ncells * gpio_index;
>  
> -	gg_data.gpiospec.args_count = tmp;
> +	gg_data.gpiospec.args_count = ncells;
>  	gg_data.gpiospec.np = chip_np;
> -	for (i = 0; i < tmp; i++) {
> -		ret = of_property_read_u32_index(np, "gpios", i,
> +	for (i = 0; i < ncells; i++) {
> +		ret = of_property_read_u32_index(np, "gpios", start_index + i,
>  					   &gg_data.gpiospec.args[i]);
>  		if (ret)
>  			return ERR_PTR(ret);
> @@ -211,18 +226,37 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
>  	enum gpio_lookup_flags lflags;
>  	enum gpiod_flags dflags;
>  	int ret;
> +	int i, ncells, ngpios;
> +
> +	ncells = of_gpio_get_gpio_cells_size(chip->of_node);
> +	if (ncells < 0)
> +		return 0;
>  
>  	for_each_available_child_of_node(chip->of_node, np) {
>  		if (!of_property_read_bool(np, "gpio-hog"))
>  			continue;
>  
> -		desc = of_parse_own_gpio(np, &name, &lflags, &dflags);
> -		if (IS_ERR(desc))
> +		ngpios = of_property_count_u32_elems(np, "gpios");
> +		if (ngpios < 0)
> +			continue;
> +
> +		if (ngpios % ncells) {
> +			dev_warn(chip->parent,
> +				"GPIOs entries are not proper in gpios\n");
>  			continue;
> +		}
> +
> +		ngpios /= ncells;
> +		for (i = 0; i < ngpios; i++) {
> +			desc = of_parse_own_gpio(np, &name, i,
> +						 &lflags, &dflags);
> +			if (IS_ERR(desc))
> +				continue;
>  
> -		ret = gpiod_hog(desc, name, lflags, dflags);
> -		if (ret < 0)
> -			return ret;
> +			ret = gpiod_hog(desc, name, lflags, dflags);
> +			if (ret < 0)
> +				return ret;
> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

* Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog
  2016-03-09  6:28   ` Markus Pargmann
@ 2016-03-09 13:20     ` Laxman Dewangan
  2016-03-09 17:17       ` Stephen Warren
  0 siblings, 1 reply; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-09 13:20 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: linus.walleij, robh+dt, pawel.moll, mark.rutland, devicetree,
	linux-kernel, linux-gpio, treding, swarren, Benoit Parrot,
	Alexandre Courbot


On Wednesday 09 March 2016 11:58 AM, Markus Pargmann wrote:
> * PGP Signed by an unknown key
>
> Hi,
>
> On Tue, Mar 08, 2016 at 05:32:07PM +0530, Laxman Dewangan wrote:
>> The child node for gpio hogs under gpio controller's node
>> provide the mechanism to automatic GPIO request and
>> configuration as part of the gpio-controller's driver
>> probe function.
>>
>> Currently, property "gpio" takes one gpios for such
>> configuration. Add support to have multiple GPIOs in
>> this property so that multiple GPIOs of gpio-controller
>> can be configured by this mechanism with one child node.
> So if I read this correctly you want to have multiple GPIOs with the
> same line name? Why don't you use multiple child nodes with individual
> line names?
>
There is cases on which particular functional configuration needs sets 
of GPIO to set. On this case, making sub node for each GPIOs creates 
lots of sub-nodes and  add complexity on readability, usability and 
maintainability.
Example: for my board, I wanted to set GPIO H2 to input and H0 and H1 to 
be output high.
Instead of three nodes, I can have two here:
        gpio@0,6000d000 {
                wlan_input {
                        gpio-hog;
                        gpios = <TEGRA_GPIO(H, 2) 0>;
                        input;
                };

                wlan_output {
                        gpio-hog;
                        gpios = <TEGRA_GPIO(H, 0) 0 TEGRA_GPIO(H, 1) 0>;
                        output-high;
                };
        };

So here I am grouping the multiple output GPIO together.

This looks much similar if we have many GPIOs for one type of 
configurations.

Even it looks better if we have something:
        gpio@0,6000d000 {
                wlan_control {
                        gpio-hog;
                        gpios-input = <TEGRA_GPIO(H, 2) 0>;
                        gpios-output-high = <TEGRA_GPIO(H, 0) 0 
TEGRA_GPIO(H, 1) 0>;
                };
        };

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

* Re: [PATCH 2/5] gpio: gpiolib: Print error number if gpio hog failed
  2016-03-08 15:32     ` Laxman Dewangan
@ 2016-03-09 17:07       ` Stephen Warren
  2016-03-10  6:58         ` Laxman Dewangan
  0 siblings, 1 reply; 24+ messages in thread
From: Stephen Warren @ 2016-03-09 17:07 UTC (permalink / raw)
  To: Laxman Dewangan, Thierry Reding
  Cc: linus.walleij, robh+dt, pawel.moll, mark.rutland, devicetree,
	linux-kernel, linux-gpio

On 03/08/2016 08:32 AM, Laxman Dewangan wrote:
>
> On Tuesday 08 March 2016 07:52 PM, Thierry Reding wrote:
>> On Tue, Mar 08, 2016 at 05:32:05PM +0530, Laxman Dewangan wrote:
>>
>>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>>> ---
>>>   drivers/gpio/gpiolib.c | 9 +++++----
>>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> index bc788b9..7575ebb 100644
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -2621,15 +2621,16 @@ int gpiod_hog(struct gpio_desc *desc, const
>>> char *name,
>>>       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
>>>       if (IS_ERR(local_desc)) {
>>> -        pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
>>> -               name, chip->label, hwnum);
>>> +        status = PTR_ERR(local_desc);
>>> +        pr_err("requesting hog GPIO %s, chip %s, offset %d failed
>>> %d\n",
>>> +               name, chip->label, hwnum, status);
>> I find this type of format hard to read. I prefer a semi-colon to
>> separate the message from the failure reason (i.e. error code).
>>
>> Besides that I don't understand why you're dropping the parentheses
>> around the "chip %s, offset %d", I found that easier on the eye.
>
> I did to accommodate the  3 extra character ( %d) for string format on
> that line as it was already near to 80 column.
> Just did not want to split in multiple lines.

Note that strings shouldn't be split across lines since it makes it 
harder to grep for them. This is one case where the 80-column limit 
isn't strict, within reason.

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

* Re: [PATCH 3/5] gpio: of: Return error if gpio hog configuration failed
  2016-03-08 12:02 ` [PATCH 3/5] gpio: of: Return error if gpio hog configuration failed Laxman Dewangan
  2016-03-08 14:23   ` Thierry Reding
@ 2016-03-09 17:11   ` Stephen Warren
  2016-03-10  7:02     ` Laxman Dewangan
  1 sibling, 1 reply; 24+ messages in thread
From: Stephen Warren @ 2016-03-09 17:11 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: linus.walleij, robh+dt, pawel.moll, mark.rutland, devicetree,
	linux-kernel, linux-gpio, treding, Benoit Parrot,
	Alexandre Courbot

On 03/08/2016 05:02 AM, Laxman Dewangan wrote:
> If GPIO hog configuration failed while adding OF based
> gpiochip() then return the error instead of ignoring it.
>
> This helps of properly handling the gpio driver dependency.
>
> When adding the gpio hog nodes for NVIDIA's Tegra210 platforms,
> the gpio_hogd() fails with EPROBE_DEFER because pinctrl is not
> ready at this time and gpio_request() for Tegra GPIO driver
> returns error. The error was not causing the Tegra GPIO driver
> to fail as the error was getting ignored.

> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c

> @@ -218,9 +220,12 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
>   		if (IS_ERR(desc))
>   			continue;
>
> -		if (gpiod_hog(desc, name, lflags, dflags))
> -			continue;
> +		ret = gpiod_hog(desc, name, lflags, dflags);
> +		if (ret < 0)
> +			return ret;
>   	}
> +
> +	return 0;
>   }

If there are multiple child nodes (which the code above is looping 
over), and the hog for entries 0, 1, 2 succeed and the hog for entry 3 
fails, don't you need to go back and unhog for nodes 0..2 so that the 
next time this function is called, those hogs won't already be in place 
thus preventing them from being hogged the second time around? Or does 
hogging not take ownership of the resource and thus prevent it from 
being acquired again?

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

* Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog
  2016-03-09 13:20     ` Laxman Dewangan
@ 2016-03-09 17:17       ` Stephen Warren
  2016-03-10  7:07         ` Laxman Dewangan
  0 siblings, 1 reply; 24+ messages in thread
From: Stephen Warren @ 2016-03-09 17:17 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: Markus Pargmann, linus.walleij, robh+dt, pawel.moll,
	mark.rutland, devicetree, linux-kernel, linux-gpio, treding,
	Benoit Parrot, Alexandre Courbot

On 03/09/2016 06:20 AM, Laxman Dewangan wrote:
>
> On Wednesday 09 March 2016 11:58 AM, Markus Pargmann wrote:
>> * PGP Signed by an unknown key
>>
>> Hi,
>>
>> On Tue, Mar 08, 2016 at 05:32:07PM +0530, Laxman Dewangan wrote:
>>> The child node for gpio hogs under gpio controller's node
>>> provide the mechanism to automatic GPIO request and
>>> configuration as part of the gpio-controller's driver
>>> probe function.
>>>
>>> Currently, property "gpio" takes one gpios for such
>>> configuration. Add support to have multiple GPIOs in
>>> this property so that multiple GPIOs of gpio-controller
>>> can be configured by this mechanism with one child node.
>> So if I read this correctly you want to have multiple GPIOs with the
>> same line name? Why don't you use multiple child nodes with individual
>> line names?
>>
> There is cases on which particular functional configuration needs sets
> of GPIO to set. On this case, making sub node for each GPIOs creates
> lots of sub-nodes and  add complexity on readability, usability and
> maintainability.
> Example: for my board, I wanted to set GPIO H2 to input and H0 and H1 to
> be output high.
> Instead of three nodes, I can have two here:
>         gpio@0,6000d000 {
>                 wlan_input {
>                         gpio-hog;
>                         gpios = <TEGRA_GPIO(H, 2) 0>;
>                         input;
>                 };
>
>                 wlan_output {
>                         gpio-hog;
>                         gpios = <TEGRA_GPIO(H, 0) 0 TEGRA_GPIO(H, 1) 0>;
>                         output-high;
>                 };
>         };
 >
> So here I am grouping the multiple output GPIO together.
>
> This looks much similar if we have many GPIOs for one type of
> configurations.
>
> Even it looks better if we have something:
>         gpio@0,6000d000 {
>                 wlan_control {
>                         gpio-hog;
>                         gpios-input = <TEGRA_GPIO(H, 2) 0>;
>                         gpios-output-high = <TEGRA_GPIO(H, 0) 0
> TEGRA_GPIO(H, 1) 0>;
>                 };
>         };

The problem with that is the description used when acquiring the GPIO is 
just "wlan_input", "wlan_output", or "wlan_control". There's nothing to 
indicate what those individual pins do (perhaps one is a reset signal, 
one is a regulator enable, etc.?) By requiring separate nodes for each 
GPIO, then the node name can provide a meaningful semantic 
name/description for each GPIO, which provides much more information.

If the approach in this patch is acceptable though, I think you want to 
update the description of "gpios" (in the GPIO hog definition section) 
in Documentation/devicetree/bindings/gpio/gpio.txt to mention that 
multiple GPIO entries are legal. Right now it says that property much 
contain exactly #gpio-cells, not a multiple of #gpio-cells.

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

* Re: [PATCH 5/5] gpio: DT: Rephrase property "gpios" of hog node to support multiple gpios
  2016-03-08 12:02 ` [PATCH 5/5] gpio: DT: Rephrase property "gpios" of hog node to support multiple gpios Laxman Dewangan
@ 2016-03-09 17:18   ` Stephen Warren
  0 siblings, 0 replies; 24+ messages in thread
From: Stephen Warren @ 2016-03-09 17:18 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: linus.walleij, robh+dt, pawel.moll, mark.rutland, devicetree,
	linux-kernel, linux-gpio, treding

On 03/08/2016 05:02 AM, Laxman Dewangan wrote:
> The property "gpios" of GPIO hog node support the multiple GPIO entries.
> Rephrase the details of this property for this new support.

Oh, I see the document is updated here. I think either patch 4 and 5 
should be swapped so the docs are updated first, or this should be 
squashed into patch 4.

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

* Re: [PATCH 2/5] gpio: gpiolib: Print error number if gpio hog failed
  2016-03-09 17:07       ` Stephen Warren
@ 2016-03-10  6:58         ` Laxman Dewangan
  0 siblings, 0 replies; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-10  6:58 UTC (permalink / raw)
  To: Stephen Warren, Thierry Reding
  Cc: linus.walleij, robh+dt, pawel.moll, mark.rutland, devicetree,
	linux-kernel, linux-gpio


On Wednesday 09 March 2016 10:37 PM, Stephen Warren wrote:
> On 03/08/2016 08:32 AM, Laxman Dewangan wrote:
>>
>> On Tuesday 08 March 2016 07:52 PM, Thierry Reding wrote:
>>> On Tue, Mar 08, 2016 at 05:32:05PM +0530, Laxman Dewangan wrote:
>>>
>>>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>>>> ---
>>>>   drivers/gpio/gpiolib.c | 9 +++++----
>>>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>>> index bc788b9..7575ebb 100644
>>>> --- a/drivers/gpio/gpiolib.c
>>>> +++ b/drivers/gpio/gpiolib.c
>>>> @@ -2621,15 +2621,16 @@ int gpiod_hog(struct gpio_desc *desc, const
>>>> char *name,
>>>>       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
>>>>       if (IS_ERR(local_desc)) {
>>>> -        pr_err("requesting hog GPIO %s (chip %s, offset %d) 
>>>> failed\n",
>>>> -               name, chip->label, hwnum);
>>>> +        status = PTR_ERR(local_desc);
>>>> +        pr_err("requesting hog GPIO %s, chip %s, offset %d failed
>>>> %d\n",
>>>> +               name, chip->label, hwnum, status);
>>> I find this type of format hard to read. I prefer a semi-colon to
>>> separate the message from the failure reason (i.e. error code).
>>>
>>> Besides that I don't understand why you're dropping the parentheses
>>> around the "chip %s, offset %d", I found that easier on the eye.
>>
>> I did to accommodate the  3 extra character ( %d) for string format on
>> that line as it was already near to 80 column.
>> Just did not want to split in multiple lines.
>
> Note that strings shouldn't be split across lines since it makes it 
> harder to grep for them. This is one case where the 80-column limit 
> isn't strict, within reason.

OK, so not change the existing string, just add new  format.

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

* Re: [PATCH 3/5] gpio: of: Return error if gpio hog configuration failed
  2016-03-09 17:11   ` Stephen Warren
@ 2016-03-10  7:02     ` Laxman Dewangan
  0 siblings, 0 replies; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-10  7:02 UTC (permalink / raw)
  To: Stephen Warren
  Cc: linus.walleij, robh+dt, pawel.moll, mark.rutland, devicetree,
	linux-kernel, linux-gpio, treding, Benoit Parrot,
	Alexandre Courbot


On Wednesday 09 March 2016 10:41 PM, Stephen Warren wrote:
> On 03/08/2016 05:02 AM, Laxman Dewangan wrote:
>> If GPIO hog configuration failed while adding OF based
>> gpiochip() then return the error instead of ignoring it.
>>
>> This helps of properly handling the gpio driver dependency.
>>
>> When adding the gpio hog nodes for NVIDIA's Tegra210 platforms,
>> the gpio_hogd() fails with EPROBE_DEFER because pinctrl is not
>> ready at this time and gpio_request() for Tegra GPIO driver
>> returns error. The error was not causing the Tegra GPIO driver
>> to fail as the error was getting ignored.
>
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>
>> @@ -218,9 +220,12 @@ static void of_gpiochip_scan_gpios(struct 
>> gpio_chip *chip)
>>           if (IS_ERR(desc))
>>               continue;
>>
>> -        if (gpiod_hog(desc, name, lflags, dflags))
>> -            continue;
>> +        ret = gpiod_hog(desc, name, lflags, dflags);
>> +        if (ret < 0)
>> +            return ret;
>>       }
>> +
>> +    return 0;
>>   }
>
> If there are multiple child nodes (which the code above is looping 
> over), and the hog for entries 0, 1, 2 succeed and the hog for entry 3 
> fails, don't you need to go back and unhog for nodes 0..2 so that the 
> next time this function is called, those hogs won't already be in 
> place thus preventing them from being hogged the second time around? 
> Or does hogging not take ownership of the resource and thus prevent it 
> from being acquired again?

The gpiolib take care per the error handling:

         status = of_gpiochip_add(chip);
         if (status)
                 goto err_remove_chip;

:::
err_remove_chip:
         acpi_gpiochip_remove(chip);
         gpiochip_free_hogs(chip);
         of_gpiochip_remove(chip);

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

* Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog
  2016-03-09 17:17       ` Stephen Warren
@ 2016-03-10  7:07         ` Laxman Dewangan
  2016-03-10 11:16           ` Markus Pargmann
  0 siblings, 1 reply; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-10  7:07 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Markus Pargmann, linus.walleij, robh+dt, pawel.moll,
	mark.rutland, devicetree, linux-kernel, linux-gpio, treding,
	Benoit Parrot, Alexandre Courbot


On Wednesday 09 March 2016 10:47 PM, Stephen Warren wrote:
> On 03/09/2016 06:20 AM, Laxman Dewangan wrote:
>>
>> On Wednesday 09 March 2016 11:58 AM, Markus Pargmann wrote:
>>> * PGP Signed by an unknown key
>>>
>>> Hi,
>>>
>>> On Tue, Mar 08, 2016 at 05:32:07PM +0530, Laxman Dewangan wrote:
>>>> The child node for gpio hogs under gpio controller's node
>>>> provide the mechanism to automatic GPIO request and
>>>> configuration as part of the gpio-controller's driver
>>>> probe function.
>>>>
>>>> Currently, property "gpio" takes one gpios for such
>>>> configuration. Add support to have multiple GPIOs in
>>>> this property so that multiple GPIOs of gpio-controller
>>>> can be configured by this mechanism with one child node.
>>> So if I read this correctly you want to have multiple GPIOs with the
>>> same line name? Why don't you use multiple child nodes with individual
>>> line names?
>>>
>> There is cases on which particular functional configuration needs sets
>> of GPIO to set. On this case, making sub node for each GPIOs creates
>> lots of sub-nodes and  add complexity on readability, usability and
>> maintainability.
>> Example: for my board, I wanted to set GPIO H2 to input and H0 and H1 to
>> be output high.
>> Instead of three nodes, I can have two here:
>>         gpio@0,6000d000 {
>>                 wlan_input {
>>                         gpio-hog;
>>                         gpios = <TEGRA_GPIO(H, 2) 0>;
>>                         input;
>>                 };
>>
>>                 wlan_output {
>>                         gpio-hog;
>>                         gpios = <TEGRA_GPIO(H, 0) 0 TEGRA_GPIO(H, 1) 0>;
>>                         output-high;
>>                 };
>>         };
> >
>> So here I am grouping the multiple output GPIO together.
>>
>> This looks much similar if we have many GPIOs for one type of
>> configurations.
>>
>> Even it looks better if we have something:
>>         gpio@0,6000d000 {
>>                 wlan_control {
>>                         gpio-hog;
>>                         gpios-input = <TEGRA_GPIO(H, 2) 0>;
>>                         gpios-output-high = <TEGRA_GPIO(H, 0) 0
>> TEGRA_GPIO(H, 1) 0>;
>>                 };
>>         };
>
> The problem with that is the description used when acquiring the GPIO 
> is just "wlan_input", "wlan_output", or "wlan_control". There's 
> nothing to indicate what those individual pins do (perhaps one is a 
> reset signal, one is a regulator enable, etc.?) By requiring separate 
> nodes for each GPIO, then the node name can provide a meaningful 
> semantic name/description for each GPIO, which provides much more 
> information.
>

On this case, we have already property "line-name" and passed the name 
of the gpio via this property.
The property names is "line-name" which is good for one string. We can 
support other property "line-names" with multiple string per GPIO index.

line-names = "wlan-reset", "wlan-enable";


> If the approach in this patch is acceptable though, I think you want 
> to update the description of "gpios" (in the GPIO hog definition 
> section) in Documentation/devicetree/bindings/gpio/gpio.txt to mention 
> that multiple GPIO entries are legal. Right now it says that property 
> much contain exactly #gpio-cells, not a multiple of #gpio-cells.

I have 5th patch for this and will rearrange series as you suggested on 
5th patch.

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

* Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog
  2016-03-10  7:07         ` Laxman Dewangan
@ 2016-03-10 11:16           ` Markus Pargmann
  2016-03-10 11:53             ` Laxman Dewangan
  0 siblings, 1 reply; 24+ messages in thread
From: Markus Pargmann @ 2016-03-10 11:16 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: Stephen Warren, linus.walleij, robh+dt, pawel.moll, mark.rutland,
	devicetree, linux-kernel, linux-gpio, treding, Benoit Parrot,
	Alexandre Courbot

On Thursday 10 March 2016 12:37:32 Laxman Dewangan wrote:
> 
> On Wednesday 09 March 2016 10:47 PM, Stephen Warren wrote:
> > On 03/09/2016 06:20 AM, Laxman Dewangan wrote:
> >>
> >> On Wednesday 09 March 2016 11:58 AM, Markus Pargmann wrote:
> >>> * PGP Signed by an unknown key
> >>>
> >>> Hi,
> >>>
> >>> On Tue, Mar 08, 2016 at 05:32:07PM +0530, Laxman Dewangan wrote:
> >>>> The child node for gpio hogs under gpio controller's node
> >>>> provide the mechanism to automatic GPIO request and
> >>>> configuration as part of the gpio-controller's driver
> >>>> probe function.
> >>>>
> >>>> Currently, property "gpio" takes one gpios for such
> >>>> configuration. Add support to have multiple GPIOs in
> >>>> this property so that multiple GPIOs of gpio-controller
> >>>> can be configured by this mechanism with one child node.
> >>> So if I read this correctly you want to have multiple GPIOs with the
> >>> same line name? Why don't you use multiple child nodes with individual
> >>> line names?
> >>>
> >> There is cases on which particular functional configuration needs sets
> >> of GPIO to set. On this case, making sub node for each GPIOs creates
> >> lots of sub-nodes and  add complexity on readability, usability and
> >> maintainability.
> >> Example: for my board, I wanted to set GPIO H2 to input and H0 and H1 to
> >> be output high.
> >> Instead of three nodes, I can have two here:
> >>         gpio@0,6000d000 {
> >>                 wlan_input {
> >>                         gpio-hog;
> >>                         gpios = <TEGRA_GPIO(H, 2) 0>;
> >>                         input;
> >>                 };
> >>
> >>                 wlan_output {
> >>                         gpio-hog;
> >>                         gpios = <TEGRA_GPIO(H, 0) 0 TEGRA_GPIO(H, 1) 0>;
> >>                         output-high;
> >>                 };
> >>         };
> > >
> >> So here I am grouping the multiple output GPIO together.
> >>
> >> This looks much similar if we have many GPIOs for one type of
> >> configurations.
> >>
> >> Even it looks better if we have something:
> >>         gpio@0,6000d000 {
> >>                 wlan_control {
> >>                         gpio-hog;
> >>                         gpios-input = <TEGRA_GPIO(H, 2) 0>;
> >>                         gpios-output-high = <TEGRA_GPIO(H, 0) 0
> >> TEGRA_GPIO(H, 1) 0>;
> >>                 };
> >>         };
> >
> > The problem with that is the description used when acquiring the GPIO 
> > is just "wlan_input", "wlan_output", or "wlan_control". There's 
> > nothing to indicate what those individual pins do (perhaps one is a 
> > reset signal, one is a regulator enable, etc.?) By requiring separate 
> > nodes for each GPIO, then the node name can provide a meaningful 
> > semantic name/description for each GPIO, which provides much more 
> > information.
> >
> 
> On this case, we have already property "line-name" and passed the name 
> of the gpio via this property.
> The property names is "line-name" which is good for one string. We can 
> support other property "line-names" with multiple string per GPIO index.
> 
> line-names = "wlan-reset", "wlan-enable";

There is currently a discussion about the future bindings for subnodes in GPIO
controller nodes. Please have a look at these two mail threads:

	"Device tree binding documentation for gpio-switch"
	"gpio: of: Add support to have multiple gpios in gpio-hog"

Best Regards,

Markus

> 
> 
> > If the approach in this patch is acceptable though, I think you want 
> > to update the description of "gpios" (in the GPIO hog definition 
> > section) in Documentation/devicetree/bindings/gpio/gpio.txt to mention 
> > that multiple GPIO entries are legal. Right now it says that property 
> > much contain exactly #gpio-cells, not a multiple of #gpio-cells.
> 
> I have 5th patch for this and will rearrange series as you suggested on 
> 5th patch.
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

* Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog
  2016-03-10 11:16           ` Markus Pargmann
@ 2016-03-10 11:53             ` Laxman Dewangan
  2016-03-17 15:46               ` Rob Herring
  0 siblings, 1 reply; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-10 11:53 UTC (permalink / raw)
  To: Markus Pargmann
  Cc: Stephen Warren, linus.walleij, robh+dt, pawel.moll, mark.rutland,
	devicetree, linux-kernel, linux-gpio, treding, Benoit Parrot,
	Alexandre Courbot


On Thursday 10 March 2016 04:46 PM, Markus Pargmann wrote:
> On Thursday 10 March 2016 12:37:32 Laxman Dewangan wrote:
>> On Wednesday 09 March 2016 10:47 PM, Stephen Warren wrote:
>>> On 03/09/2016 06:20 AM, Laxman Dewangan wrote:
>>> The problem with that is the description used when acquiring the GPIO
>>> is just "wlan_input", "wlan_output", or "wlan_control". There's
>>> nothing to indicate what those individual pins do (perhaps one is a
>>> reset signal, one is a regulator enable, etc.?) By requiring separate
>>> nodes for each GPIO, then the node name can provide a meaningful
>>> semantic name/description for each GPIO, which provides much more
>>> information.
>>>
>> On this case, we have already property "line-name" and passed the name
>> of the gpio via this property.
>> The property names is "line-name" which is good for one string. We can
>> support other property "line-names" with multiple string per GPIO index.
>>
>> line-names = "wlan-reset", "wlan-enable";
> There is currently a discussion about the future bindings for subnodes in GPIO
> controller nodes. Please have a look at these two mail threads:
>
> 	"Device tree binding documentation for gpio-switch"
> 	"gpio: of: Add support to have multiple gpios in gpio-hog"

Second one is this patch only. Is it by intention?

The binding details about the gpio-switch and names are given by 
property "lable". I think property "label" is standard way of going 
forward i.e. I post similar patch for gpio-keys device name from DT 
after got review comment.

So here,  we can have the gpio names  under property "label" or "labels".


Or am I missing anything?

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

* Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog
  2016-03-10 11:53             ` Laxman Dewangan
@ 2016-03-17 15:46               ` Rob Herring
  2016-03-17 17:44                 ` Laxman Dewangan
  0 siblings, 1 reply; 24+ messages in thread
From: Rob Herring @ 2016-03-17 15:46 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: Markus Pargmann, Stephen Warren, linus.walleij, pawel.moll,
	mark.rutland, devicetree, linux-kernel, linux-gpio, treding,
	Benoit Parrot, Alexandre Courbot

On Thu, Mar 10, 2016 at 05:23:55PM +0530, Laxman Dewangan wrote:
> 
> On Thursday 10 March 2016 04:46 PM, Markus Pargmann wrote:
> >On Thursday 10 March 2016 12:37:32 Laxman Dewangan wrote:
> >>On Wednesday 09 March 2016 10:47 PM, Stephen Warren wrote:
> >>>On 03/09/2016 06:20 AM, Laxman Dewangan wrote:
> >>>The problem with that is the description used when acquiring the GPIO
> >>>is just "wlan_input", "wlan_output", or "wlan_control". There's
> >>>nothing to indicate what those individual pins do (perhaps one is a
> >>>reset signal, one is a regulator enable, etc.?) By requiring separate
> >>>nodes for each GPIO, then the node name can provide a meaningful
> >>>semantic name/description for each GPIO, which provides much more
> >>>information.

I agree.

> >>On this case, we have already property "line-name" and passed the name
> >>of the gpio via this property.
> >>The property names is "line-name" which is good for one string. We can
> >>support other property "line-names" with multiple string per GPIO index.
> >>
> >>line-names = "wlan-reset", "wlan-enable";

Then what happens when someone wants to selectively disable gpio hogs?

status = "okay", "disabled", "okay";

While I often push things to fewer nodes and more compact descriptions, 
I don't think that is the right direction in this case.

> >There is currently a discussion about the future bindings for subnodes in GPIO
> >controller nodes. Please have a look at these two mail threads:
> >
> >	"Device tree binding documentation for gpio-switch"
> >	"gpio: of: Add support to have multiple gpios in gpio-hog"
> 
> Second one is this patch only. Is it by intention?
> 
> The binding details about the gpio-switch and names are given by property
> "lable". I think property "label" is standard way of going forward i.e. I
> post similar patch for gpio-keys device name from DT after got review
> comment.
> 
> So here,  we can have the gpio names  under property "label" or "labels".

label is standard. labels you just made up.

> Or am I missing anything?

The point is the more one off changes I see that are all inter-related, 
the less willing I am to accept any that don't consider all the cases. 
The inter-relationship here is how do we describe gpio lines that don't 
otherwise have a connection to another node and how to deal with them if 
that changes.

Rob

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

* Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog
  2016-03-17 15:46               ` Rob Herring
@ 2016-03-17 17:44                 ` Laxman Dewangan
  0 siblings, 0 replies; 24+ messages in thread
From: Laxman Dewangan @ 2016-03-17 17:44 UTC (permalink / raw)
  To: Rob Herring
  Cc: Markus Pargmann, Stephen Warren, linus.walleij, pawel.moll,
	mark.rutland, devicetree, linux-kernel, linux-gpio, treding,
	Benoit Parrot, Alexandre Courbot


On Thursday 17 March 2016 09:16 PM, Rob Herring wrote:
> On Thu, Mar 10, 2016 at 05:23:55PM +0530, Laxman Dewangan wrote:
>>>> On this case, we have already property "line-name" and passed the name
>>>> of the gpio via this property.
>>>> The property names is "line-name" which is good for one string. We can
>>>> support other property "line-names" with multiple string per GPIO index.
>>>>
>>>> line-names = "wlan-reset", "wlan-enable";
> Then what happens when someone wants to selectively disable gpio hogs?
>
> status = "okay", "disabled", "okay";
>
> While I often push things to fewer nodes and more compact descriptions,
> I don't think that is the right direction in this case.

I dont think we need to support the individual gpio to be enable/disable 
by status.
We need to support the status property at node level only. if individual 
gpio need to be enabled/disabled by status then it need to break in 
different hog nodes.

This is same as like we have in pincontrol where we can provide the list 
of pin names for some configuration under same node.


>>> There is currently a discussion about the future bindings for subnodes in GPIO
>>> controller nodes. Please have a look at these two mail threads:
>>>
>>> 	"Device tree binding documentation for gpio-switch"
>>> 	"gpio: of: Add support to have multiple gpios in gpio-hog"
>> Second one is this patch only. Is it by intention?
>>
>> The binding details about the gpio-switch and names are given by property
>> "lable". I think property "label" is standard way of going forward i.e. I
>> post similar patch for gpio-keys device name from DT after got review
>> comment.
>>
>> So here,  we can have the gpio names  under property "label" or "labels".
> label is standard. labels you just made up.

Yaah, lables for plural only. Otherwise no issue with "label".

>
>> Or am I missing anything?
> The point is the more one off changes I see that are all inter-related,
> the less willing I am to accept any that don't consider all the cases.
> The inter-relationship here is how do we describe gpio lines that don't
> otherwise have a connection to another node and how to deal with them if
> that changes.

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

end of thread, other threads:[~2016-03-17 17:58 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-08 12:02 [PATCH 0/5] gpio: of: Add error handling and support for multiple gpio in gpio-hog Laxman Dewangan
2016-03-08 12:02 ` [PATCH 1/5] gpio: of: Scan available child node for gpio-hog Laxman Dewangan
2016-03-08 14:18   ` Thierry Reding
2016-03-08 12:02 ` [PATCH 2/5] gpio: gpiolib: Print error number if gpio hog failed Laxman Dewangan
2016-03-08 12:27   ` Vladimir Zapolskiy
2016-03-08 14:22   ` Thierry Reding
2016-03-08 15:32     ` Laxman Dewangan
2016-03-09 17:07       ` Stephen Warren
2016-03-10  6:58         ` Laxman Dewangan
2016-03-08 12:02 ` [PATCH 3/5] gpio: of: Return error if gpio hog configuration failed Laxman Dewangan
2016-03-08 14:23   ` Thierry Reding
2016-03-09 17:11   ` Stephen Warren
2016-03-10  7:02     ` Laxman Dewangan
2016-03-08 12:02 ` [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog Laxman Dewangan
2016-03-09  6:28   ` Markus Pargmann
2016-03-09 13:20     ` Laxman Dewangan
2016-03-09 17:17       ` Stephen Warren
2016-03-10  7:07         ` Laxman Dewangan
2016-03-10 11:16           ` Markus Pargmann
2016-03-10 11:53             ` Laxman Dewangan
2016-03-17 15:46               ` Rob Herring
2016-03-17 17:44                 ` Laxman Dewangan
2016-03-08 12:02 ` [PATCH 5/5] gpio: DT: Rephrase property "gpios" of hog node to support multiple gpios Laxman Dewangan
2016-03-09 17:18   ` Stephen Warren

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