gpiolib: Initializing GPIOs using DT property gpio-initval

There is no reason to find out chip and hwnum to use to request a gpio and get another gpio descriptor. We already have the descriptor we want to use so we can directly use it.

Signed-off-by Markus Pargmann <[email protected]>

There is no reason to find out chip and hwnum to use to request a gpio and get another gpio descriptor. We already have the descriptor we want to use so we can directly use it.

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/gpio/gpiolib.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

drivers/gpio/gpiolib.c +3 -10

index 79a0b41ce57b..872fdd3617c1 100644
@@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
              unsigned long lflags, enum gpiod_flags dflags)
 {
-       struct gpio_chip *chip;
-       struct gpio_desc *local_desc;
-       int hwnum;
        int status;
 
-       chip = gpiod_to_chip(desc);
-       hwnum = gpio_chip_hwgpio(desc);
-
-       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
-       if (IS_ERR(local_desc)) {
+       status = __gpiod_request(desc, name);
+       if (status) {
                pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
-                      name, chip->label, hwnum);
-               return PTR_ERR(local_desc);
+                      name, gpiod_to_chip(desc)->label,

Quoted 37 lines · Markus Pargmann

There is no reason to find out chip and hwnum to use to request a gpio and get another gpio descriptor. We already have the descriptor we want to use so we can directly use it.

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/gpio/gpiolib.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

drivers/gpio/gpiolib.c +5 -10

index 79a0b41ce57b..872fdd3617c1 100644
@@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
              unsigned long lflags, enum gpiod_flags dflags)
 {
-       struct gpio_chip *chip;
-       struct gpio_desc *local_desc;
-       int hwnum;
        int status;
 
-       chip = gpiod_to_chip(desc);
-       hwnum = gpio_chip_hwgpio(desc);
-
-       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
-       if (IS_ERR(local_desc)) {
+       status = __gpiod_request(desc, name);
+       if (status) {
                pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
-                      name, chip->label, hwnum);
-               return PTR_ERR(local_desc);
+                      name, gpiod_to_chip(desc)->label,
+                      gpio_chip_hwgpio(desc));
+               return status;
        }
 

drivers/gpio/gpiolib.c EXPORT_SYMBOL_GPL(__gpiod_get_index_optional); · line 2206

Quoted 6 lines

drivers/gpio/gpiolib.c +1 -1

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

Mmm I should have reviewed this patch earlier, but what bothers me a bit is that it breaks the symetry that we had by calling request_own_desc() and free_own_desc() in the failing case (as well as in gpiochip_free_hogs). And in the end you still need to call gpiod_to_chip() so I am not sure what the benefit is.

Sure, the code is less verbose, but at the same time it has become slightly harder to understand. Semantically speaking "request_own_desc()" is exactly the action we want to convey. __gpiod_request() is more ambiguous.

Note that this is not a reject, I just wanted to stress that "less code" is not necessarily the same as "easier to read".

Quoted 33 lines · Markus Pargmann

There is no reason to find out chip and hwnum to use to request a gpio and get another gpio descriptor. We already have the descriptor we want to use so we can directly use it.

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/gpio/gpiolib.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

drivers/gpio/gpiolib.c +3 -10

index 79a0b41ce57b..872fdd3617c1 100644
@@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
              unsigned long lflags, enum gpiod_flags dflags)
 {
-       struct gpio_chip *chip;
-       struct gpio_desc *local_desc;
-       int hwnum;
        int status;
 
-       chip = gpiod_to_chip(desc);
-       hwnum = gpio_chip_hwgpio(desc);
-
-       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
-       if (IS_ERR(local_desc)) {
+       status = __gpiod_request(desc, name);
+       if (status) {
                pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
-                      name, chip->label, hwnum);
-               return PTR_ERR(local_desc);
+                      name, gpiod_to_chip(desc)->label,

drivers/gpio/gpiolib.c EXPORT_SYMBOL_GPL(__gpiod_get_index_optional); · line 2206

Quoted 10 lines

drivers/gpio/gpiolib.c +3 -1

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

Mmm I should have reviewed this patch earlier, but what bothers me a bit is that it breaks the symetry that we had by calling request_own_desc() and free_own_desc() in the failing case (as well as in gpiochip_free_hogs). And in the end you still need to call gpiod_to_chip() so I am not sure what the benefit is.

Sure, the code is less verbose, but at the same time it has become slightly harder to understand. Semantically speaking "request_own_desc()" is exactly the action we want to convey. __gpiod_request() is more ambiguous.

Note that this is not a reject, I just wanted to stress that "less code" is not necessarily the same as "easier to read".

Quoted 56 lines · Alexandre Courbot

Quoted 33 lines · Markus Pargmann

There is no reason to find out chip and hwnum to use to request a gpio and get another gpio descriptor. We already have the descriptor we want to use so we can directly use it.

Signed-off-by Markus Pargmann <[email protected]>

---
 drivers/gpio/gpiolib.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

drivers/gpio/gpiolib.c +3 -10

index 79a0b41ce57b..872fdd3617c1 100644
@@ -2189,25 +2189,20 @@ EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
              unsigned long lflags, enum gpiod_flags dflags)
 {
-       struct gpio_chip *chip;
-       struct gpio_desc *local_desc;
-       int hwnum;
        int status;
 
-       chip = gpiod_to_chip(desc);
-       hwnum = gpio_chip_hwgpio(desc);
-
-       local_desc = gpiochip_request_own_desc(chip, hwnum, name);
-       if (IS_ERR(local_desc)) {
+       status = __gpiod_request(desc, name);
+       if (status) {
                pr_err("requesting hog GPIO %s (chip %s, offset %d) failed\n",
-                      name, chip->label, hwnum);
-               return PTR_ERR(local_desc);
+                      name, gpiod_to_chip(desc)->label,

drivers/gpio/gpiolib.c EXPORT_SYMBOL_GPL(__gpiod_get_index_optional); · line 2206

Quoted 10 lines

drivers/gpio/gpiolib.c +3 -1

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

Mmm I should have reviewed this patch earlier, but what bothers me a bit is that it breaks the symetry that we had by calling request_own_desc() and free_own_desc() in the failing case (as well as in gpiochip_free_hogs). And in the end you still need to call gpiod_to_chip() so I am not sure what the benefit is.

Sure, the code is less verbose, but at the same time it has become slightly harder to understand. Semantically speaking "request_own_desc()" is exactly the action we want to convey. __gpiod_request() is more ambiguous.

Quoted 2 lines

Note that this is not a reject, I just wanted to stress that "less code" is not necessarily the same as "easier to read".

OK I dropped this patch for now.

Markus can you live without this patch for 2/3 and 3/3?

Yours,
Linus Walleij

The gpio name is now stored in the gpio descriptor, so we can simply use that instead of an argument to the function.

Signed-off-by Markus Pargmann <[email protected]>

Quoted 4 lines · Markus Pargmann

The gpio name is now stored in the gpio descriptor, so we can simply use that instead of an argument to the function.

Signed-off-by Markus Pargmann <[email protected]>

Several problems with this patch:

- Does not apply to v4.3-rc1

- Fixed that but noted that it just alters the call to gpiod_hog() in of_gpiochip_scan_hogs(), there is a local const char *name that should be removed too.

- Looking at of_get_gpio_hog() it is unclear to me that .name is set in the gpio_desc as desired. Please check this.

Crucial code looks like this:

    if (name && of_property_read_string(np, "line-name", name))
        *name = np->name;

i.e. is the line-name really propagated to gpiod->name? Are you sure you have tested this with a DTS using line-name and verified that it propagates properly?

Quoted 3 lines

- Fixed that but noted that it just alters the call to gpiod_hog() in of_gpiochip_scan_hogs(), there is a local const char *name that should be removed too.

The local const char *name is temporarily used in of_gpiochip_scan_hogs() to get the name from DT and assign it to the descriptor:

        desc = of_parse_own_gpio(np, &name, &lflags, &dflags);
        ...
        else
                desc->name = name;

Quoted 11 lines · Markus Pargmann

Quoted 3 lines

- Fixed that but noted that it just alters the call to gpiod_hog() in of_gpiochip_scan_hogs(), there is a local const char *name that should be removed too.

The local const char *name is temporarily used in of_gpiochip_scan_hogs() to get the name from DT and assign it to the descriptor:

        desc = of_parse_own_gpio(np, &name, &lflags, &dflags);
        ...
        else
                desc->name = name;

OK the problem is that this series is dependent on the named GPIOs series. I want this series to stand alone, because this series is not as controversial, and I want to merge these initvals first.

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

Signed-off-by Markus Pargmann <[email protected]>

---
 Documentation/devicetree/bindings/gpio/gpio.txt | 29 +++++++------
 drivers/gpio/gpiolib-of.c                       |  9 ++++
 drivers/gpio/gpiolib.c                          | 55 ++++++++++++++++++++-----
 drivers/gpio/gpiolib.h                          |  2 +
 4 files changed, 73 insertions(+), 22 deletions(-)

Documentation/devicetree/bindings/gpio/gpio.txt +10 -5

index 5788d5cf1252..55d58983ba43 100644
@@ -116,29 +116,34 @@ Every GPIO controller node must contain both an empty "gpio-controller"
 property, and a #gpio-cells integer property, which indicates the number of
 cells in a gpio-specifier.
 
-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.
+GPIO hogging is a mechanism providing automatic GPIO request and configuration
+as part of the gpio-controller's driver probe function.
+
+GPIO initialization provides an automatic initialization to known save values.
+Instead of GPIO hogging the GPIO's value and direction can be modified by other
+users after it was initialized.
+
+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).

Documentation/devicetree/bindings/gpio/gpio.txt +1 -3

+ Only one of input, output-low and output-high may 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 at 1400 {

drivers/gpio/gpiolib-of.c +9 -0

index f1fe5123da28..ee00c2c63f57 100644
@@ -234,6 +234,15 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 
 			if (gpiod_hog(desc, lflags, dflags))
 				continue;
+		} else if (of_property_read_bool(np, "gpio-initval")) {
+			if (!dflags) {
+				dev_warn(chip->dev, "GPIO line %d (%s): no initialization state specified, bailing out\n",
+					 desc_to_gpio(desc), np->name);
+				continue;
+			}
+
+			if (gpiod_initialize(desc, lflags, dflags))
+				continue;
 		}
 	}
 }

drivers/gpio/gpiolib.c +45 -10

index f1559fa72c36..d7aa27a92e82 100644
@@ -2178,15 +2178,8 @@ struct gpio_desc *__must_check __gpiod_get_index_optional(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(__gpiod_get_index_optional);
 
-/**
- * gpiod_hog - Hog the specified GPIO desc given the provided flags
- * @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
- */
-int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
-	      enum gpiod_flags dflags)
+static int _gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
+		      enum gpiod_flags dflags)
 {
 	int status;
 	const char *name = desc->name;

@@ -2202,11 +2195,31 @@ int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
 	status = gpiod_configure_flags(desc, name, lflags, dflags);
 	if (status < 0) {
 		pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",
-		       name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
+		       name, gpiod_to_chip(desc)->label,
+		       gpio_chip_hwgpio(desc));
 		gpiochip_free_own_desc(desc);
 		return status;
 	}
 
+	return 0;
+}
+
+/**
+ * gpiod_hog - Hog the specified GPIO desc given the provided flags
+ * @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
+ */
+int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
+	      enum gpiod_flags dflags)
+{
+	int ret;
+
+	ret = _gpiod_initialize(desc, lflags, dflags);
+	if (ret)
+		return ret;
+
 	/* Mark GPIO as hogged so it can be identified and removed later */
 	set_bit(FLAG_IS_HOGGED, &desc->flags);
 

@@ -2236,6 +2249,28 @@ 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 ret;
+
+	ret = _gpiod_initialize(desc, lflags, dflags);
+	if (ret)
+		return ret;
+
+	__gpiod_free(desc);
+	return ret;
+}
+
+/**
  * 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

drivers/gpio/gpiolib.h +2 -0

index 6c2aeff59f86..b6dfa3526e3a 100644
@@ -99,6 +99,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, 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

Signature · 1 line

2.5.0

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

Signed-off-by Markus Pargmann <[email protected]>

Documentation/devicetree/bindings/gpio/gpio.txt +8 -2

-Each GPIO hog definition is represented as a child node of the GPIO controller.
+GPIO hogging is a mechanism providing automatic GPIO request and configuration
+as part of the gpio-controller's driver probe function.
+
+GPIO initialization provides an automatic initialization to known save values.
+Instead of GPIO hogging the GPIO's value and direction can be modified by other
+users after it was initialized.
+
+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

+- gpio-hog:   A property specifying that this child node represent a GPIO hog.
+- gpio-initval: This GPIO should be initialized to the specified configuration.

+ Only one of input, output-low and output-high may 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.

Documentation/devicetree/bindings/gpio/gpio.txt +0 -3

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

drivers/gpio/gpiolib-of.c +0 -0

@@ -234,6 +234,15 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 
                        if (gpiod_hog(desc, lflags, dflags))

Quoted 3 lines · Markus Pargmann

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

Signed-off-by Markus Pargmann <[email protected]>

(...)

Documentation/devicetree/bindings/gpio/gpio.txt Every GPIO controller node must contain both an empty "gpio-controller" · line 120

Quoted 5 lines

Documentation/devicetree/bindings/gpio/gpio.txt +2 -3

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

Insert text like this:

There are two types of GPIO definitions:

- GPIO hogs are ...

- GPIO initializers are ...

This list form is easier to understand.

Quoted 12 lines

Documentation/devicetree/bindings/gpio/gpio.txt +8 -2

-Each GPIO hog definition is represented as a child node of the GPIO controller.
+GPIO hogging is a mechanism providing automatic GPIO request and configuration
+as part of the gpio-controller's driver probe function.
+
+GPIO initialization provides an automatic initialization to known save values.
+Instead of GPIO hogging the GPIO's value and direction can be modified by other
+users after it was initialized.
+
+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

Documentation/devicetree/bindings/gpio/gpio.txt Every GPIO controller node must contain both an empty "gpio-controller" · line 137

Quoted 10 lines

Documentation/devicetree/bindings/gpio/gpio.txt +4 -4

              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.
+ Only one of gpio-hog and gpio-initval may be specified.

This is confusing. Instead write: "The two following options are mutually exclusive. One of them must be specified, but not both."

Quoted 4 lines

+- gpio-hog:   A property specifying that this child node represent a GPIO hog.
+- gpio-initval: This GPIO should be initialized to the specified configuration.

+ Only one of input, output-low and output-high may be specified:

Insert "Of the following arguments, only one..." (etc)

Quoted 11 lines

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

Documentation/devicetree/bindings/gpio/gpio.txt +0 -3

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

(...)

Quoted 5 lines

drivers/gpio/gpiolib-of.c +0 -0

@@ -234,6 +234,15 @@ static void of_gpiochip_scan_gpios(struct gpio_chip *chip)
 
                        if (gpiod_hog(desc, lflags, dflags))

drivers/gpio/gpiolib-of.c static void of_gpiochip_scan_gpios(struct gpio_chip *chip) · line 245

Quoted 10 lines

drivers/gpio/gpiolib-of.c +9 -0

                                continue;
+               } else if (of_property_read_bool(np, "gpio-initval")) {
+                       if (!dflags) {
+                               dev_warn(chip->dev, "GPIO line %d (%s): no initialization state specified, bailing out\n",
+                                        desc_to_gpio(desc), np->name);
+                               continue;
+                       }
+
+                       if (gpiod_initialize(desc, lflags, dflags))
+                               continue;

We usually do not mix implementations and bindings but it's OK with me.

drivers/gpio/gpiolib-of.c static void of_gpiochip_scan_gpios(struct gpio_chip *chip) · line 246

Quoted 1 line

drivers/gpio/gpiolib-of.c +0 -0

                }

You need a terminating else {} - clause to warn if neither of gpio-hog or gpio-initval is specified.

drivers/gpio/gpiolib-of.c static void of_gpiochip_scan_gpios(struct gpio_chip *chip) · line 248

Quoted 5 lines

drivers/gpio/gpiolib-of.c +2 -2

-int gpiod_hog(struct gpio_desc *desc, unsigned long lflags,
-             enum gpiod_flags dflags)
+static int _gpiod_initialize(struct gpio_desc *desc, unsigned long lflags,
+                     enum gpiod_flags dflags)

I don't like _underscore functions. Try to find a name that is descriptive and does not begin with underscore.

What about just gpiod_init()?

drivers/gpio/gpiolib-of.c

Quoted 6 lines

if (status < 0) {
pr_err("setup of hog GPIO %s (chip %s, offset %d) failed\n",

drivers/gpio/gpiolib-of.c +2 -1

-                      name, gpiod_to_chip(desc)->label, gpio_chip_hwgpio(desc));
+                      name, gpiod_to_chip(desc)->label,
+                      gpio_chip_hwgpio(desc));

Looks like a random, unrelated code reshuffling. Don't do this.

Yours,
Linus Walleij

Hello,

(Markus left Pengutronix, so I dropped his non-working address from the recipients and pick up his discussion.)

as far as I see, this topic isn't closed yet. I want something similar, maybe even a bit more than this patch achieves:

Some gpios should get a fixed direction. If such a gpio is configured as output, a value should be specified. This is already working today with gpio-hogs.

Now additionally I want to initialize some gpios but allow them to be grabbed later. IMHO there are the following new cases:

It should be possible to:

a) change the value of a gpio initially configured as output
b) change the direction of a gpio initially configured as output
c) change the direction of a gpio initially configured as input

IMHO the dts should describe which case should be applied to a given gpio.

Is it a valid assumption that a gpio that can change direction is also allowed to change value when configured as output? I assume this in the following discussion, some details need to change if this shouldn't be implied. (I think we need a d) then, not sure how this should look though.)

I'd suggest the following description for these cases:

a)
/*
* initially configured as low output. Consumer can do
* gpio_set_value(..., 1); but not gpio_direction_input(...); */

 	nodename {
		gpio-hog;
		gpios = <...>;
		output-low-init;
	};

b)
/*
* initially configured as low output. Consumer can do
* gpio_set_value(..., 1); and gpio_direction_input(...); */

 	nodename {
		gpio-hog;
		gpios = <...>;
		output-init-low;
	};

c)
/*

	 * initially configured as input. Consumer can do
	 * gpio_direction_output(..., ...) and then set the value
	 * freely.
	 */
	 nodename {
	 	gpio-hog;
		gpios = <...>;
		input-init;
	};

I don't like that "output-low-init" and "output-init-low" are so similar, so if someone suggests a better naming scheme, that would be great.

Quoted 34 lines · On Tue, 7 Feb 2017 12:09:50 +0100 Uwe Kleine-K?nig wrote:

a)
/*
* initially configured as low output. Consumer can do
* gpio_set_value(..., 1); but not gpio_direction_input(...); */

 	nodename {
		gpio-hog;
		gpios = <...>;
		output-low-init;
	};

b)
/*
* initially configured as low output. Consumer can do
* gpio_set_value(..., 1); and gpio_direction_input(...); */

 	nodename {
		gpio-hog;
		gpios = <...>;
		output-init-low;
	};

c)
/*

	 * initially configured as input. Consumer can do
	 * gpio_direction_output(..., ...) and then set the value
	 * freely.
	 */
	 nodename {
	 	gpio-hog;
		gpios = <...>;
		input-init;
	};

Quoted 4 lines

I don't like that "output-low-init" and "output-init-low" are so similar, so if someone suggests a better naming scheme, that would be great.

The position of the 'output' or 'input' within the word could imply whether the direction can be changed lateron or not. E.g.:
output-init-low => an output whose state is initially low, but can be
reconfigured to high lateron.

init-low-output => a gpio that is initially configured as output low,
but can subsequently be reconfigured as input or to a different state

init-input => your case c)

input => a GPIO input that cannot be reconfigured

This would also allow an 'output-low' as a GPIO that is permanently grounded...

as far as I see, this topic isn't closed yet. I want something similar, maybe even a bit more than this patch achieves:

Some gpios should get a fixed direction. If such a gpio is configured as output, a value should be specified. This is already working today with gpio-hogs.

Now additionally I want to initialize some gpios but allow them to be grabbed later. IMHO there are the following new cases:

It should be possible to:

a) change the value of a gpio initially configured as output
b) change the direction of a gpio initially configured as output
c) change the direction of a gpio initially configured as input

IMHO the dts should describe which case should be applied to a given gpio.

Is it a valid assumption that a gpio that can change direction is also allowed to change value when configured as output? I assume this in the following discussion, some details need to change if this shouldn't be implied. (I think we need a d) then, not sure how this should look though.)

I'd suggest the following description for these cases:

a)
/*
* initially configured as low output. Consumer can do
* gpio_set_value(..., 1); but not gpio_direction_input(...); */

 	nodename {
		gpio-hog;
		gpios = <...>;
		output-low-init;
	};

b)
/*
* initially configured as low output. Consumer can do
* gpio_set_value(..., 1); and gpio_direction_input(...); */

 	nodename {
		gpio-hog;
		gpios = <...>;
		output-init-low;
	};

c)
/*

	 * initially configured as input. Consumer can do
	 * gpio_direction_output(..., ...) and then set the value
	 * freely.
	 */
	 nodename {
	 	gpio-hog;
		gpios = <...>;
		input-init;
	};

Quoted 25 lines

I'd suggest the following description for these cases:

a)

      /*
       * initially configured as low output. Consumer can do
       * gpio_set_value(..., 1); but not gpio_direction_input(...);
       */
      nodename {
              gpio-hog;
              gpios = <...>;
              output-low-init;
      };

b)

      /*
       * initially configured as low output. Consumer can do
       * gpio_set_value(..., 1); and gpio_direction_input(...);
       */
      nodename {
              gpio-hog;
              gpios = <...>;
              output-init-low;
      };

c)

Quoted 10 lines

      /*
       * initially configured as input. Consumer can do
       * gpio_direction_output(..., ...) and then set the value
       * freely.
       */
       nodename {
              gpio-hog;
              gpios = <...>;
              input-init;
      };

I thought a bit more about this and the changes that would be necessary to achieve this:

- introduce new flags IS_FIXED_DIR and IS_FIXED_VALUE for struct gpio_desc::flags and error out accordingly in gpiod_set_direction_* and gpiod_set_value respectively. Then we have:

- already existing hogs: IS_FIXED_DIR | IS_FIXED_VALUE
- a) IS_FIXED_DIR
- b)+c) 0
- d) (if desired) IS_FIXED_VALUE

- don't auto-request hogged pins
Currently hogged pins are not available for consumers to be requested. This needs to be changed at least for the three (or four) new cases.

IMHO it is sensible to allow requesting a gpio that is completely hogged and just prevent changing direction and (if it's an output) value.

Quoted 39 lines · Uwe Kleine-K?nig

Quoted 25 lines

I'd suggest the following description for these cases:

a)

      /*
       * initially configured as low output. Consumer can do
       * gpio_set_value(..., 1); but not gpio_direction_input(...);
       */
      nodename {
              gpio-hog;
              gpios = <...>;
              output-low-init;
      };

b)

      /*
       * initially configured as low output. Consumer can do
       * gpio_set_value(..., 1); and gpio_direction_input(...);
       */
      nodename {
              gpio-hog;
              gpios = <...>;
              output-init-low;
      };

c)

Quoted 10 lines

      /*
       * initially configured as input. Consumer can do
       * gpio_direction_output(..., ...) and then set the value
       * freely.
       */
       nodename {
              gpio-hog;
              gpios = <...>;
              input-init;
      };

I thought a bit more about this and the changes that would be necessary to achieve this:

Quoted 8 lines

- introduce new flags IS_FIXED_DIR and IS_FIXED_VALUE for struct gpio_desc::flags and error out accordingly in gpiod_set_direction_* and gpiod_set_value respectively. Then we have:

- already existing hogs: IS_FIXED_DIR | IS_FIXED_VALUE
- a) IS_FIXED_DIR
- b)+c) 0
- d) (if desired) IS_FIXED_VALUE

I don't really understand IS_FIXED_DIR, if that is a property of the hardware then that is something the driver should be handling, i.e. the driver must know if a line is fixed direction (only input or only output). Same for IS_FIXED_VALUE.

Or do you mean it is something else, like these are only software flags for hogs? Then they should be HOG_IS_FIXED_DIR etc.

Quoted 4 lines

- don't auto-request hogged pins
Currently hogged pins are not available for consumers to be requested. This needs to be changed at least for the three (or four) new cases.

But look at the dictionary definition of "hog":
https://en.wiktionary.org/wiki/hog

The usage here is as in "to hog resources" so:

3. A greedy person; one who refuses to share.

And check the verb form:

1. hog (third-person singular simple present hogs, present participle hogging, simple past and past participle hogged)
(transitive) To greedily take more than one's share, to take precedence
at the expense of another or others.
[quotations]Hey! Quit hogging all the blankets.

So of course hogs request and hold the lines. That is what hogging is all about.

Quoted 3 lines

IMHO it is sensible to allow requesting a gpio that is completely hogged and just prevent changing direction and (if it's an output) value.

No it doesn't make sense at all to take something that is hogged because it is completely unintuitive to the very word "hog".

As I have said before: a new property, something like "initial-directions" and "initial-values" need to be created and handled separately for this, without even involving the hogging mechanism.

You need to introduce something in parallel from the hogs that does not request the lines, but just sets up initial values and directions on them and then leave them to be acquired later.

We already keep track of the direction inside the gpio_desc and we have callbacks to read that up from the hardware if supported, and I guess the same goes for initial values.

I just don't see what you need here and why you want to expand the hog concept beyond its intuitive use?

I say define something new in device tree with an intuitive name, that does what you want it to do: just sets up initial direction and optional value (on outputs) using the existing callbacks and then just gets out of the way. No hogging.

[Me]

Quoted 4 lines

Quoted 3 lines

IMHO it is sensible to allow requesting a gpio that is completely hogged and just prevent changing direction and (if it's an output) value.

Quoted 2 lines

No it doesn't make sense at all to take something that is hogged because it is completely unintuitive to the very word "hog".

What I want is that it is possible to restrict the usage of a GPIO more fine-grained. For example only fix the direction to input but allow a driver to still read out it's value. Or fix the direction to output but allow changing the level.

So the semantic of a hogged gpio as
implemented today is a special case of my new use cases. So it seems naturally to extend the already existing concept.

And concerning the meaning of hogging: It still takes some freedom from others, just not everything because you can still request the line. So not being a native English speaker the word still fits for me.

And technically it is sensible to implement the new use cases and today's hogging together and so it is also sensible IMHO to use the same mechanism in the device tree.

So to stick to your suggestion I would have in the end for two lines io12 that must be 0 for ESD reasons and io13 that drives the RST-line of a companion DSP that should be kept low until userspace is ready and then allow being driven high via gpioctl:

        io12 {
                gpio-hog;
                gpio = <4 0>;
                output-low;
        };

        io13 {
                gpio-init;
                gpio = <5 0>;
                output-low;
                fixed-direction;
        };

For me it looks artificial to not use "gpio-hog" for the 2nd specification but if that is the compromise that we can agree on, that's ok for me.

Orthogonal to gpio-hog being used for this or not, I like

        output-low;
        fixed-direction;

better than

        output-low-init;

as the former is more intuitive.

Quoted 6 lines · Uwe Kleine-K?nig

[Me]

Quoted 4 lines

Quoted 3 lines

IMHO it is sensible to allow requesting a gpio that is completely hogged and just prevent changing direction and (if it's an output) value.

Quoted 7 lines

Quoted 2 lines

No it doesn't make sense at all to take something that is hogged because it is completely unintuitive to the very word "hog".

What I want is that it is possible to restrict the usage of a GPIO more fine-grained. For example only fix the direction to input but allow a driver to still read out it's value. Or fix the direction to output but allow changing the level.

So let me understand this right.

For something input-only, return -EINVAL from .direction_output() and vice versa mutatis mutandis for something output-only.

This semantic is set at runtime when using these functions.

Why can't you do this in the specific driver? Why does that have

to be handled by the core? I mean of course we can add a flag to the gpio_desc if you think it's gonna be a common case, but then I want an indication that there is a bunch of drivers that need this done in the core.

Quoted 3 lines

So the semantic of a hogged gpio as
implemented today is a special case of my new use cases. So it seems naturally to extend the already existing concept.

I disagree

and the DT maintainer Rob Herring already said he kind
of dislikes the hogs already, so let's not expand their use beyond what is already being done, which is explicit hogging.

Quoted 3 lines

And concerning the meaning of hogging: It still takes some freedom from others, just not everything because you can still request the line. So not being a native English speaker the word still fits for me.

IMO this breaks Rusty Russell's API Design Manifesto, levels 7 and 6:

7. The obvious use is (probably) the correct one.
6. The name tells you how to use it.

In my case, just no. It is not obvuous and does not tell me how to use it.

Something like gpio-line-initial-directions, gpio-line-initial-values etc does.

Quoted 3 lines

And technically it is sensible to implement the new use cases and today's hogging together and so it is also sensible IMHO to use the same mechanism in the device tree.

I disagree with that, sorry.

Quoted 18 lines

So to stick to your suggestion I would have in the end for two lines io12 that must be 0 for ESD reasons and io13 that drives the RST-line of a companion DSP that should be kept low until userspace is ready and then allow being driven high via gpioctl:

        io12 {
                gpio-hog;
                gpio = <4 0>;
                output-low;
        };

        io13 {
                gpio-init;
                gpio = <5 0>;
                output-low;
                fixed-direction;
        };

Quoted 3 lines

For me it looks artificial to not use "gpio-hog" for the 2nd specification but if that is the compromise that we can agree on, that's ok for me.

This looks OK to me.

I think you will have a problem with Rob on this though, he's not a big fan of these hogging nodes and I'm not sure he's gonna like the gpio-init nodes much.

Now additionally I want to initialize some gpios but allow them to be grabbed later. IMHO there are the following new cases:

It should be possible to:

a) change the value of a gpio initially configured as output
b) change the direction of a gpio initially configured as output
c) change the direction of a gpio initially configured as input

IMHO the dts should describe which case should be applied to a given gpio.

Quoted 11 lines · Uwe Kleine-K?nig

Now additionally I want to initialize some gpios but allow them to be grabbed later. IMHO there are the following new cases:

It should be possible to:

a) change the value of a gpio initially configured as output
b) change the direction of a gpio initially configured as output
c) change the direction of a gpio initially configured as input

IMHO the dts should describe which case should be applied to a given gpio.

Isn't it the job of the board firmware to ensure that the hardware is setup to a reasonably sane state for the board?

You give an example of holding GPIOs low in Linux for "ESD reasons" later in this thread, but if you're only doing that in Linux, what if Linux isn't running yet, and the GPIO has been left floating by the board firmware? Fixing this in Linux is really too late.

Floating GPIOs are also a source of higher current drain - a GPIO sitting mid-rail turns both transistors on for a MOS input, which gives a direct path across the power supply. The quicker that the GPIOs can be initialised, the less wasted power there is. So, that's another argument for board firmware doing the basic GPIO initialisation and not stuffing this into DT and having the kernel do it.

We could then talk about floating GPIOs that activate higher power devices, and the arguments for doing GPIO initialisation as early as possible in board firmware continue to stack up.

IMHO, initial configuration of GPIOs is the job of board firmware, not the kernel. I see no sane reason to push that into the kernel.