Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1352946 > unrolled thread

[PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog Laxman Dewangan <ldewangan@nvidia.com> - 2016-03-08 13:20 +0100
    Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in  gpio-hog Markus Pargmann <mpa@pengutronix.de> - 2016-03-09 07:30 +0100
      Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog Laxman Dewangan <ldewangan@nvidia.com> - 2016-03-09 14:40 +0100
        Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in  gpio-hog Stephen Warren <swarren@wwwdotorg.org> - 2016-03-09 18:20 +0100
          Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog Laxman Dewangan <ldewangan@nvidia.com> - 2016-03-10 08:30 +0100
            Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog Markus Pargmann <mpa@pengutronix.de> - 2016-03-10 12:20 +0100
              Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog Laxman Dewangan <ldewangan@nvidia.com> - 2016-03-10 13:10 +0100
                Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in  gpio-hog Rob Herring <robh@kernel.org> - 2016-03-17 16:50 +0100
                  Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog Laxman Dewangan <ldewangan@nvidia.com> - 2016-03-17 19:00 +0100

#1352946 — [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog

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

[toc] | [next] | [standalone]

#1353859 — Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog

[Multipart message — attachments visible in raw view] — view raw

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 |

[toc] | [prev] | [next] | [standalone]

#1354120

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>;
                };
        };

[toc] | [prev] | [next] | [standalone]

#1354291 — Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog

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.

[toc] | [prev] | [next] | [standalone]

#1354898

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.

[toc] | [prev] | [next] | [standalone]

#1355036

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 |

[toc] | [prev] | [next] | [standalone]

#1355058

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?

[toc] | [prev] | [next] | [standalone]

#1359972 — Re: [PATCH 4/5] gpio: of: Add support to have multiple gpios in gpio-hog

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

[toc] | [prev] | [next] | [standalone]

#1360091

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web