[PATCH v3 0/2] gpiolib: Initializing GPIOs using DT property gpio-initval

[PATCH v3 0/2] gpiolib: Initializing GPIOs using DT property gpio-initval

[Markus Pargmann]

Hi,

I completely forgot this series. Here is an update.

This series adds a gpio-initval property to the devicetree. It provides a way
to initialize GPIOs to a defined value.

In v3 I splitted the patch into dt-bindings and the gpiolib patch. This series
is now based on v4.4-rc1. gpiod_initialize() does not use common code with
gpiod_hog anymore.

Best Regards,

Markus


Markus Pargmann (2):
  dt-bindings: GPIO: Add gpio-initval
  gpiolib: Add GPIO initialization

 Documentation/devicetree/bindings/gpio/gpio.txt | 34 ++++++++++++++++---------
 drivers/gpio/gpiolib-of.c                       | 13 ++++++----
 drivers/gpio/gpiolib.c                          | 24 +++++++++++++++++
 drivers/gpio/gpiolib.h                          |  2 ++
 4 files changed, 56 insertions(+), 17 deletions(-)

-- 
2.6.2

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

[PATCH v3 1/2] dt-bindings: GPIO: Add gpio-initval

[Markus Pargmann]

Add a binding for GPIO initialization.

Signed-off-by: Markus Pargmann <mpa-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 Documentation/devicetree/bindings/gpio/gpio.txt | 34 ++++++++++++++++---------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index 069cdf6f9dac..d11abfa13add 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -155,29 +155,39 @@ gpio-controller@00000000 {
 	ngpios = <18>;
 }
 
-The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
-providing automatic GPIO request and configuration as part of the
-gpio-controller's driver probe function.
+The GPIO chip may contain GPIO definitions. These define properties for single
+GPIOs of this controller.
 
-Each GPIO hog definition is represented as a child node of the GPIO controller.
+There are two types of GPIO definitions:
+
+- GPIO hogging is a mechanism providing automatic GPIO request and
+  configuration as part of the gpio-controller driver's probe function. The
+  GPIO is held until the gpio-controller is removed.
+- GPIO initialization provides an automatic initialization to known save
+  values. The GPIO can be used normally afterwards.
+
+Each GPIO 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).
-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
-configuration.
+
+Optional properties:
+- line-name:  The GPIO label name. If not present the node name is used.
+
+ The following two options are mutually exclusive. One of them should be
+ specified, but not both:
+- gpio-hog:   A property specifying that this child node represent a GPIO hog.
+- gpio-initval: This GPIO should be initialized to the specified configuration.
+
+ The following three options are mutually exclusive. They change the setting of
+ the GPIO pin. One of them should be specified:
 - input:      A property specifying to set the GPIO direction as input.
 - output-low  A property specifying to set the GPIO direction as output with
 	      the value low.
 - output-high A property specifying to set the GPIO direction as output with
 	      the value high.
 
-Optional properties:
-- line-name:  The GPIO label name. If not present the node name is used.
-
 Example of two SOC GPIO banks defined as gpio-controller nodes:
 
 	qe_pio_a: gpio-controller@1400 {
-- 
2.6.2

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

Re: [PATCH v3 1/2] dt-bindings: GPIO: Add gpio-initval

[Rob Herring]

On Tue, Nov 17, 2015 at 11:07:57AM +0100, Markus Pargmann wrote:
> Add a binding for GPIO initialization.

Some comments, but they are really more on the gpio hog. I must have 
missed them.
 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  Documentation/devicetree/bindings/gpio/gpio.txt | 34 ++++++++++++++++---------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
> index 069cdf6f9dac..d11abfa13add 100644
> --- a/Documentation/devicetree/bindings/gpio/gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/gpio.txt
> @@ -155,29 +155,39 @@ gpio-controller@00000000 {
>  	ngpios = <18>;
>  }
>  
> -The GPIO chip may contain GPIO hog definitions. GPIO hogging is a mechanism
> -providing automatic GPIO request and configuration as part of the
> -gpio-controller's driver probe function.
> +The GPIO chip may contain GPIO definitions. These define properties for single
> +GPIOs of this controller.
>  
> -Each GPIO hog definition is represented as a child node of the GPIO controller.
> +There are two types of GPIO definitions:
> +
> +- GPIO hogging is a mechanism providing automatic GPIO request and
> +  configuration as part of the gpio-controller driver's probe function. The
> +  GPIO is held until the gpio-controller is removed.

I can't say I like the term hogs, but that's just cosmetic.

> +- GPIO initialization provides an automatic initialization to known save
> +  values. The GPIO can be used normally afterwards.
> +
> +Each GPIO 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).
> -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
> -configuration.
> +
> +Optional properties:
> +- line-name:  The GPIO label name. If not present the node name is used.

We already have "label" for this purpose.

> +
> + The following two options are mutually exclusive. One of them should be
> + specified, but not both:
> +- gpio-hog:   A property specifying that this child node represent a GPIO hog.
> +- gpio-initval: This GPIO should be initialized to the specified configuration.
> +
> + The following three options are mutually exclusive. They change the setting of
> + the GPIO pin. One of them should be specified:
>  - input:      A property specifying to set the GPIO direction as input.
>  - output-low  A property specifying to set the GPIO direction as output with
>  	      the value low.
>  - output-high A property specifying to set the GPIO direction as output with
>  	      the value high.

If they are mutually exclusive, then it should just be one property with 
values. That is much more simple to validate.

But none of this is really even needed. We can do all this with existing 
bindings. Most gpio controllers use 2 cells and the 2nd cell was 
reserved for something like this. Simply adding a -gpios property in the 
gpio controller node would do the job:

init-gpios = <&self n (GPIO_ACTIVE_HIGH | GPIO_OUTPUT)>;
hog-gpios = <&self m GPIO_INPUT>;

Perhaps we don't want to use GPIO_ACTIVE_x here, but we have 31 more 
free bits to use.

Rob

Re: [PATCH v3 1/2] dt-bindings: GPIO: Add gpio-initval

[Linus Walleij]

On Tue, Nov 17, 2015 at 9:35 PM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Nov 17, 2015 at 11:07:57AM +0100, Markus Pargmann wrote:
>> Add a binding for GPIO initialization.
>
> Some comments, but they are really more on the gpio hog. I must have
> missed them.

They are already merged, and in use :(

>> +Optional properties:
>> +- line-name:  The GPIO label name. If not present the node name is used.
>
> We already have "label" for this purpose.

I would rather say we already have another instance of "line-name"
for this purpose. See
Documentation/devicetree/bindings/gpio/gpio.txt

"label" was not chosen because it's a Linuxism (that is what the internal
API is using) and not to the point.

I like the clear "line" specifier over "pin" etc since GPIOs can be
chip-internal and what not.

>> + The following two options are mutually exclusive. One of them should be
>> + specified, but not both:
>> +- gpio-hog:   A property specifying that this child node represent a GPIO hog.
>> +- gpio-initval: This GPIO should be initialized to the specified configuration.
>> +
>> + The following three options are mutually exclusive. They change the setting of
>> + the GPIO pin. One of them should be specified:
>>  - input:      A property specifying to set the GPIO direction as input.
>>  - output-low  A property specifying to set the GPIO direction as output with
>>             the value low.
>>  - output-high A property specifying to set the GPIO direction as output with
>>             the value high.
>
> If they are mutually exclusive, then it should just be one property with
> values. That is much more simple to validate.
>
> But none of this is really even needed. We can do all this with existing
> bindings. Most gpio controllers use 2 cells and the 2nd cell was
> reserved for something like this. Simply adding a -gpios property in the
> gpio controller node would do the job:
>
> init-gpios = <&self n (GPIO_ACTIVE_HIGH | GPIO_OUTPUT)>;
> hog-gpios = <&self m GPIO_INPUT>;
>
> Perhaps we don't want to use GPIO_ACTIVE_x here, but we have 31 more
> free bits to use.

I prefer if the flags to be used for characteristics of the line rather
than driving it in any way. Anyway the input/output-low/output-high is
already merged, and in use for hogs.

We looked at using -gpios in a self-referential manner, but that
made it impossible to name the lines, and that is a desired
property. Hogged lines need names so we can figure out who is
using them during debug etc. It also makes the DT more
human-readable.

Yours,
Linus Walleij

[PATCH v3 2/2] gpiolib: Add GPIO initialization

[Markus Pargmann]

This functions adds a way to initialize a GPIO without hogging it.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/gpio/gpiolib-of.c | 13 ++++++++-----
 drivers/gpio/gpiolib.c    | 24 ++++++++++++++++++++++++
 drivers/gpio/gpiolib.h    |  2 ++
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 5fe34a9df3e6..48cdc22eabf2 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -211,15 +211,18 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 	enum gpiod_flags dflags;
 
 	for_each_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))
+		if (IS_ERR(desc) || !dflags)
 			continue;
 
-		if (gpiod_hog(desc, name, lflags, dflags))
-			continue;
+		if (of_property_read_bool(np, "gpio-hog"))
+			gpiod_hog(desc, name, lflags, dflags);
+		else if (of_property_read_bool(np, "gpio-initval"))
+			gpiod_initialize(desc, lflags, dflags);
+		else
+			dev_dbg(chip->dev, "GPIO line %d (%s): neither gpio-hog nor gpio-initval found.\n",
+				desc_to_gpio(desc), np->name);
 	}
 }
 
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a18f00fc1bb8..3b35f2fafeb2 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2329,6 +2329,30 @@ static void gpiochip_free_hogs(struct gpio_chip *chip)
 }
 
 /**
+ * gpiod_initialize - Initialize a GPIO with a given value
+ * @desc:	gpio whose value will be assigned
+ * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
+ *		of_get_gpio_hog()
+ * @dflags:	gpiod_flags - optional GPIO initialization flags
+ *
+ * This is used to initialize GPIOs that were defined in DT
+ */
+int gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
+		     enum gpiod_flags dflags)
+{
+	int status;
+
+	status = gpiod_configure_flags(desc, NULL, dflags);
+	if (status < 0) {
+		pr_err("initial setup of GPIO (chip %s, offset %d) failed\n",
+		       gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
+		return status;
+	}
+
+	return 0;
+}
+
+/**
  * gpiod_get_array - obtain multiple GPIOs from a multi-index GPIO function
  * @dev:	GPIO consumer, can be NULL for system-global GPIOs
  * @con_id:	function within the GPIO consumer
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 98ab08c0aa2d..4abf53a5e651 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -107,6 +107,8 @@ int gpiod_request(struct gpio_desc *desc, const char *label);
 void gpiod_free(struct gpio_desc *desc);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 		unsigned long lflags, enum gpiod_flags dflags);
+int gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
+		     enum gpiod_flags dflags);
 
 /*
  * Return the GPIO number of the passed descriptor relative to its chip
-- 
2.6.2

Re: [PATCH v3 2/2] gpiolib: Add GPIO initialization

[Markus Pargmann]

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

Hi,

On Tuesday 17 November 2015 11:07:58 Markus Pargmann wrote:
[...]
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a18f00fc1bb8..3b35f2fafeb2 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2329,6 +2329,30 @@ static void gpiochip_free_hogs(struct gpio_chip *chip)
>  }
>  
>  /**
> + * gpiod_initialize - Initialize a GPIO with a given value
> + * @desc:	gpio whose value will be assigned
> + * @lflags:	gpio_lookup_flags - returned from of_find_gpio() or
> + *		of_get_gpio_hog()
> + * @dflags:	gpiod_flags - optional GPIO initialization flags
> + *
> + * This is used to initialize GPIOs that were defined in DT
> + */
> +int gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
> +		     enum gpiod_flags dflags)
> +{
> +	int status;
> +

Sorry, this is missing a gpiod_parse_flags(). Will fix this and resend tomorrow.

Best Regards,

Markus

> +	status = gpiod_configure_flags(desc, NULL, dflags);
> +	if (status < 0) {
> +		pr_err("initial setup of GPIO (chip %s, offset %d) failed\n",
> +		       gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
> +		return status;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
>   * gpiod_get_array - obtain multiple GPIOs from a multi-index GPIO function
>   * @dev:	GPIO consumer, can be NULL for system-global GPIOs
>   * @con_id:	function within the GPIO consumer
> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index 98ab08c0aa2d..4abf53a5e651 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -107,6 +107,8 @@ int gpiod_request(struct gpio_desc *desc, const char *label);
>  void gpiod_free(struct gpio_desc *desc);
>  int gpiod_hog(struct gpio_desc *desc, const char *name,
>  		unsigned long lflags, enum gpiod_flags dflags);
> +int gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
> +		     enum gpiod_flags dflags);
>  
>  /*
>   * Return the GPIO number of the passed descriptor relative to its chip
> 

-- 
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: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 2/2] gpiolib: Add GPIO initialization

[Linus Walleij]

On Tue, Nov 17, 2015 at 11:21 AM, Markus Pargmann <mpa@pengutronix.de> wrote:

>> +int gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
>> +                  enum gpiod_flags dflags)
>> +{
>> +     int status;
>> +
>
> Sorry, this is missing a gpiod_parse_flags(). Will fix this and resend tomorrow.

I haven't seen a new version, am I looking in the wrong places?

Yours,
Linus Walleij