[RFC 0/2] gpiolib: Initial, basic support for shared GPIO lines

[RFC 0/2] gpiolib: Initial, basic support for shared GPIO lines

[Peter Ujfalusi]

Hi,

The initial support can replace all use of GPIOD_FLAGS_BIT_NONEXCLUSIVE if the
shared GPIO is configured to follow pass through 'strategy' for the shared GPIO
pin.

I have only implemented DT support.

With the shared gpio support one can choose between three different strategy for
managing the shared gpio:
refcounted low: Keep the line low as long as there is at least one low
		request is registered
refcounted high: Keep the line high as long as there is at least one high
		request is registered
pass through: all requests are allowed to go through without refcounting.

Few shortcomings as of now:
- can not handle different GPIO_ACTIVE_ on the user side, both the root GPIO
  (which is shared) and clients must have the same GPIO_ACTIVE_ mode.
  We are using common gpio_desc.
  Like with GPIOD_FLAGS_BIT_NONEXCLUSIVE
- refcounting counts _all_ 1/0 requests coming from the users of the shared
  GPIO. This could cause issues if clients are using the gpiod API in unbalanced
  way.
  We would need to have separate tracking for each of the clients and agregate
  the level they are asking for at any moment. Basically a new gpio-chip on top
  of the real gpio pin can solve this.

Regards,
Peter
---
Peter Ujfalusi (2):
  dt-bindings: gpio: Document shared GPIO line usage
  gpiolib: Support for (output only) shared GPIO line

 .../devicetree/bindings/gpio/gpio.txt         |  66 +++++++++
 drivers/gpio/gpiolib-of.c                     |  28 +++-
 drivers/gpio/gpiolib.c                        | 132 ++++++++++++++++--
 drivers/gpio/gpiolib.h                        |  10 ++
 4 files changed, 223 insertions(+), 13 deletions(-)

-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[RFC 1/2] dt-bindings: gpio: Document shared GPIO line usage

[Peter Ujfalusi]

Boards might use the same GPIO line to control several external devices.
Add section to document on how a shared GPIO pin can be described.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 .../devicetree/bindings/gpio/gpio.txt         | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index a8895d339bfe..644e6513607c 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -228,6 +228,72 @@ Example of two SOC GPIO banks defined as gpio-controller nodes:
 		#gpio-cells = <2>;
 	};
 
+On boards one GPIO line might be connected to multiple devices as reset, enable
+or other control pins. In order to make it safer for this usage of a GPIO line
+one can describe the shared GPIO pin.
+
+Each shared GPIO pin definition is represented as a child node of the GPIO
+controller.
+
+Required properties:
+- gpio-shared:	A property specifying that this child node represents a shared
+		GPIO pin.
+- gpios:	Store the GPIO information (id, flags, ...) for each GPIO to
+		affect. Shall contain an integer multiple of 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.
+- output-low:	A property specifying to set the GPIO direction as output with
+		the value low initially.
+- output-high:	A property specifying to set the GPIO direction as output with
+		the value high initially.
+The shared GPIO line management strategy can be selected with either of the
+following properties:
+- refcounted-low: The line must be kept low as long as there is at least one
+		request asking it to be low.
+- refcounted-high: The line must be kept high as long as there is at least one
+		request asking it to be high.
+If neither of the refcounting strategy was selected then the shared GPIO is
+handled as pass through. In this mode all user requests will be forwarded to the
+shared GPIO pin without refcounting.
+
+Optional properties:
+- line-name:  The GPIO label name. If not present the node name is used.
+
+Example of shared GPIO use:
+
+	qe_pio_a: gpio-controller@1400 {
+		compatible = "fsl,qe-pario-bank-a", "fsl,qe-pario-bank";
+		reg = <0x1400 0x18>;
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		line_a {
+			gpio-shared;
+			gpios = <5 0>;
+			output-low;
+			refcounted-high;
+			line-name = "enable-for-devices";
+		};
+	};
+
+	device@0 {
+		compatible = "some,device";
+		enable-gpios = <&qe_pio_a 5 0>;
+	};
+
+	device@1 {
+		compatible = "some,device";
+		enable-gpios = <&qe_pio_a 5 0>;
+	};
+
+	component@0 {
+		compatible = "some,component";
+		select-gpios = <&qe_pio_a 5 0>;
+	};
+
 2.1) gpio- and pin-controller interaction
 -----------------------------------------
 
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[RFC 2/2] gpiolib: Support for (output only) shared GPIO line

[Peter Ujfalusi]

This patch adds basic support for handling shared GPIO lines in the core.
The line must be configured with a child node in DT.
Based on the configuration the core will use different strategy to manage
the shared line:
refcounted low: Keep the line low as long as there is at least one low
		request is registered
refcounted high: Keep the line high as long as there is at least one high
		request is registered
pass through: all requests are allowed to go through without refcounting.

The pass through mode is equivalent to how currently the
GPIOD_FLAGS_BIT_NONEXCLUSIVE is handled.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpio/gpiolib-of.c |  28 ++++++--
 drivers/gpio/gpiolib.c    | 132 +++++++++++++++++++++++++++++++++++---
 drivers/gpio/gpiolib.h    |  10 +++
 3 files changed, 157 insertions(+), 13 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index bd06743a5d7c..fbb628e6d8bc 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -531,6 +531,7 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
  * @lflags:	bitmask of gpio_lookup_flags GPIO_* values - returned from
  *		of_find_gpio() or of_parse_own_gpio()
  * @dflags:	gpiod_flags - optional GPIO initialization flags
+ * @sflags:	Extra flags for the shared GPIO usage
  *
  * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno
  * value on the error condition.
@@ -539,7 +540,8 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
 					   struct gpio_chip *chip,
 					   unsigned int idx, const char **name,
 					   unsigned long *lflags,
-					   enum gpiod_flags *dflags)
+					   enum gpiod_flags *dflags,
+					   unsigned long *sflags)
 {
 	struct device_node *chip_np;
 	enum of_gpio_flags xlate_flags;
@@ -592,6 +594,15 @@ static struct gpio_desc *of_parse_own_gpio(struct device_node *np,
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (sflags) {
+		*sflags = 0;
+
+		if (of_property_read_bool(np, "refcounted-low"))
+			set_bit(FLAG_REFCOUNT_LOW, sflags);
+		else if (of_property_read_bool(np, "refcounted-high"))
+			set_bit(FLAG_REFCOUNT_HIGH, sflags);
+	}
+
 	if (name && of_property_read_string(np, "line-name", name))
 		*name = np->name;
 
@@ -611,22 +622,29 @@ static int of_gpiochip_scan_gpios(struct gpio_chip *chip)
 	struct gpio_desc *desc = NULL;
 	struct device_node *np;
 	const char *name;
-	unsigned long lflags;
+	unsigned long lflags, sflags;
 	enum gpiod_flags dflags;
 	unsigned int i;
 	int ret;
 
 	for_each_available_child_of_node(chip->of_node, np) {
-		if (!of_property_read_bool(np, "gpio-hog"))
+		bool is_hog = of_property_read_bool(np, "gpio-hog");
+		bool is_shared = of_property_read_bool(np, "gpio-shared");
+		if (!is_hog && !is_shared)
 			continue;
 
 		for (i = 0;; i++) {
 			desc = of_parse_own_gpio(np, chip, i, &name, &lflags,
-						 &dflags);
+						&dflags,
+						is_shared ? &sflags : NULL);
 			if (IS_ERR(desc))
 				break;
 
-			ret = gpiod_hog(desc, name, lflags, dflags);
+			if (is_hog)
+				ret = gpiod_hog(desc, name, lflags, dflags);
+			if (is_shared)
+				ret = gpiod_share(desc, name, lflags, dflags,
+						  sflags);
 			if (ret < 0) {
 				of_node_put(np);
 				return ret;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index dba5f08f308c..b01836cd9e58 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -81,7 +81,7 @@ LIST_HEAD(gpio_devices);
 static DEFINE_MUTEX(gpio_machine_hogs_mutex);
 static LIST_HEAD(gpio_machine_hogs);
 
-static void gpiochip_free_hogs(struct gpio_chip *chip);
+static void gpiochip_free_owns(struct gpio_chip *chip);
 static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 				struct lock_class_key *lock_key,
 				struct lock_class_key *request_key);
@@ -1558,7 +1558,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *chip, void *data,
 err_remove_acpi_chip:
 	acpi_gpiochip_remove(chip);
 err_remove_of_chip:
-	gpiochip_free_hogs(chip);
+	gpiochip_free_owns(chip);
 	of_gpiochip_remove(chip);
 err_free_gpiochip_mask:
 	gpiochip_remove_pin_ranges(chip);
@@ -1612,7 +1612,7 @@ void gpiochip_remove(struct gpio_chip *chip)
 
 	/* FIXME: should the legacy sysfs handling be moved to gpio_device? */
 	gpiochip_sysfs_unregister(gdev);
-	gpiochip_free_hogs(chip);
+	gpiochip_free_owns(chip);
 	/* Numb the device, cancelling all outstanding operations */
 	gdev->chip = NULL;
 	gpiochip_irqchip_remove(chip);
@@ -2788,6 +2788,13 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
 		desc_set_label(desc, label ? : "?");
 		ret = 0;
+	} else 	if (test_bit(FLAG_IS_SHARED, &desc->flags)) {
+		desc->shared_users++;
+		pr_info("New user for shared GPIO line %d\n",
+			desc_to_gpio(desc));
+		kfree_const(label);
+		ret = 0;
+		goto done;
 	} else {
 		kfree_const(label);
 		ret = -EBUSY;
@@ -2894,6 +2901,15 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
+	if (test_bit(FLAG_IS_SHARED, &desc->flags) && desc->shared_users) {
+		if (--desc->shared_users) {
+			spin_unlock_irqrestore(&gpio_lock, flags);
+			pr_info("User dropped for shared GPIO line %d\n",
+				desc_to_gpio(desc));
+			return true;
+		}
+	}
+
 	chip = desc->gdev->chip;
 	if (chip && test_bit(FLAG_REQUESTED, &desc->flags)) {
 		if (chip->free) {
@@ -3126,10 +3142,44 @@ int gpiod_direction_input(struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_direction_input);
 
+static int gpiod_get_refcounted_value(struct gpio_desc *desc, int value)
+{
+	value = !!value;
+
+	if (!test_bit(FLAG_IS_SHARED, &desc->flags))
+		return value;
+
+	if (test_bit(FLAG_REFCOUNT_LOW, &desc->flags)) {
+		if (!value)
+			desc->level_refcount++;
+		else if (desc->level_refcount)
+			desc->level_refcount--;
+
+		if (desc->level_refcount)
+			value = 0;
+	} else if (test_bit(FLAG_REFCOUNT_HIGH, &desc->flags)) {
+		if (value)
+			desc->level_refcount++;
+		else if (desc->level_refcount)
+			desc->level_refcount--;
+
+		if (desc->level_refcount)
+			value = 1;
+	}
+
+	pr_debug("Shared %s GPIO line %d: counter: %d: value: %d\n",
+		test_bit(FLAG_REFCOUNT_LOW, &desc->flags) ? "refcounted low" :
+		test_bit(FLAG_REFCOUNT_HIGH, &desc->flags) ? "refcounted high" :
+		"pass through", desc_to_gpio(desc), desc->level_refcount,
+		 value);
+
+	return value;
+}
+
 static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 {
 	struct gpio_chip *gc = desc->gdev->chip;
-	int val = !!value;
+	int val;
 	int ret = 0;
 
 	/*
@@ -3144,6 +3194,8 @@ static int gpiod_direction_output_raw_commit(struct gpio_desc *desc, int value)
 		return -EIO;
 	}
 
+	val = gpiod_get_refcounted_value(desc, value);
+
 	if (gc->direction_output) {
 		ret = gc->direction_output(gc, gpio_chip_hwgpio(desc), val);
 	} else {
@@ -3665,6 +3717,7 @@ static void gpiod_set_raw_value_commit(struct gpio_desc *desc, bool value)
 	struct gpio_chip	*chip;
 
 	chip = desc->gdev->chip;
+	value = gpiod_get_refcounted_value(desc, value);
 	trace_gpio_value(desc_to_gpio(desc), 0, value);
 	chip->set(chip, gpio_chip_hwgpio(desc), value);
 }
@@ -4618,6 +4671,9 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 {
 	int ret;
 
+	if (test_bit(FLAG_IS_SHARED, &desc->flags))
+		goto out;
+
 	if (lflags & GPIO_ACTIVE_LOW)
 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
 
@@ -4659,6 +4715,7 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		return 0;
 	}
 
+out:
 	/* Process flags */
 	if (dflags & GPIOD_FLAGS_BIT_DIR_OUT)
 		ret = gpiod_direction_output(desc,
@@ -4890,16 +4947,72 @@ int gpiod_hog(struct gpio_desc *desc, const char *name,
 }
 
 /**
- * gpiochip_free_hogs - Scan gpio-controller chip and release GPIO hog
+ * gpiod_share - Share the specified GPIO desc given the provided flags
+ * @desc:	gpio whose value will be assigned
+ * @name:	gpio line name
+ * @lflags:	bitmask of gpio_lookup_flags GPIO_* values - returned from
+ *		of_find_gpio() or of_get_gpio_hog()
+ * @dflags:	gpiod_flags - optional GPIO initialization flags
+ * @sflags:	Extra flags for the shared GPIO usage
+ */
+int gpiod_share(struct gpio_desc *desc, const char *name,
+		unsigned long lflags, enum gpiod_flags dflags,
+		unsigned long sflags)
+{
+	struct gpio_chip *chip;
+	struct gpio_desc *local_desc;
+	int hwnum;
+	int ret;
+
+	chip = gpiod_to_chip(desc);
+	hwnum = gpio_chip_hwgpio(desc);
+
+	if (!(dflags & GPIOD_FLAGS_BIT_DIR_OUT)) {
+		pr_err("shared GPIO %s (chip %s, offset %d) must be output\n",
+		       name, chip->label, hwnum);
+		return -EINVAL;
+	}
+
+	local_desc = gpiochip_request_own_desc(chip, hwnum, name,
+					       lflags, dflags);
+	if (IS_ERR(local_desc)) {
+		ret = PTR_ERR(local_desc);
+		pr_err("requesting shared GPIO %s (chip %s, offset %d) failed, %d\n",
+		       name, chip->label, hwnum, ret);
+		return ret;
+	}
+
+	/* Mark GPIO as shared and set refcounting level if not pass through */
+	set_bit(FLAG_IS_SHARED, &desc->flags);
+	if (test_bit(FLAG_REFCOUNT_LOW, &sflags))
+		set_bit(FLAG_REFCOUNT_LOW, &desc->flags);
+	else if (test_bit(FLAG_REFCOUNT_HIGH, &sflags))
+		set_bit(FLAG_REFCOUNT_HIGH, &desc->flags);
+
+	pr_info("GPIO line %d (%s) shared as %s\n",
+		desc_to_gpio(desc), name,
+		test_bit(FLAG_REFCOUNT_LOW, &desc->flags) ? "refcounted low"  :
+		test_bit(FLAG_REFCOUNT_HIGH, &desc->flags) ? "refcounted high" :
+		"pass through");
+
+	return 0;
+}
+
+/**
+ * gpiochip_free_owns - Scan gpio-controller chip and release hogged or shared
+ *			GPIOs
  * @chip:	gpio chip to act on
  */
-static void gpiochip_free_hogs(struct gpio_chip *chip)
+static void gpiochip_free_owns(struct gpio_chip *chip)
 {
 	int id;
 
 	for (id = 0; id < chip->ngpio; id++) {
 		if (test_bit(FLAG_IS_HOGGED, &chip->gpiodev->descs[id].flags))
 			gpiochip_free_own_desc(&chip->gpiodev->descs[id]);
+
+		if (test_bit(FLAG_IS_SHARED, &chip->gpiodev->descs[id].flags))
+			gpiochip_free_own_desc(&chip->gpiodev->descs[id]);
 	}
 }
 
@@ -5115,6 +5228,7 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
 	bool			is_out;
 	bool			is_irq;
 	bool			active_low;
+	bool			shared;
 
 	for (i = 0; i < gdev->ngpio; i++, gpio++, gdesc++) {
 		if (!test_bit(FLAG_REQUESTED, &gdesc->flags)) {
@@ -5129,12 +5243,14 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_device *gdev)
 		is_out = test_bit(FLAG_IS_OUT, &gdesc->flags);
 		is_irq = test_bit(FLAG_USED_AS_IRQ, &gdesc->flags);
 		active_low = test_bit(FLAG_ACTIVE_LOW, &gdesc->flags);
-		seq_printf(s, " gpio-%-3d (%-20.20s|%-20.20s) %s %s %s%s",
+		shared = test_bit(FLAG_IS_SHARED, &gdesc->flags);
+		seq_printf(s, " gpio-%-3d (%-20.20s|%-20.20s) %s %s %s%s%s",
 			gpio, gdesc->name ? gdesc->name : "", gdesc->label,
 			is_out ? "out" : "in ",
 			chip->get ? (chip->get(chip, i) ? "hi" : "lo") : "?  ",
 			is_irq ? "IRQ " : "",
-			active_low ? "ACTIVE LOW" : "");
+			active_low ? "ACTIVE LOW " : "",
+			shared ? "SHARED" : "");
 		seq_printf(s, "\n");
 	}
 }
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index ca9bc1e4803c..0eec0857e3a8 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -111,11 +111,18 @@ struct gpio_desc {
 #define FLAG_PULL_UP    13	/* GPIO has pull up enabled */
 #define FLAG_PULL_DOWN  14	/* GPIO has pull down enabled */
 #define FLAG_BIAS_DISABLE    15	/* GPIO has pull disabled */
+#define FLAG_IS_SHARED 16	/* GPIO is shared */
+#define FLAG_REFCOUNT_LOW 17	/* Shared GPIO is refcounted for raw low */
+#define FLAG_REFCOUNT_HIGH 18	/* Shared GPIO is refcounted for raw high */
 
 	/* Connection label */
 	const char		*label;
 	/* Name of the GPIO */
 	const char		*name;
+	/* Number of users of a shared GPIO */
+	int			shared_users;
+	/* Reference counter for shared GPIO (low or high level) */
+	int			level_refcount;
 };
 
 int gpiod_request(struct gpio_desc *desc, const char *label);
@@ -124,6 +131,9 @@ int gpiod_configure_flags(struct gpio_desc *desc, const char *con_id,
 		unsigned long lflags, enum gpiod_flags dflags);
 int gpiod_hog(struct gpio_desc *desc, const char *name,
 		unsigned long lflags, enum gpiod_flags dflags);
+int gpiod_share(struct gpio_desc *desc, const char *name,
+		unsigned long lflags, enum gpiod_flags dflags,
+		unsigned long sflags);
 
 /*
  * Return the GPIO number of the passed descriptor relative to its chip
-- 
Peter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 0/2] gpiolib: Initial, basic support for shared GPIO lines

[Peter Ujfalusi]

Hi,

On 20/11/2019 15.34, Peter Ujfalusi wrote:
> Hi,
> 
> The initial support can replace all use of GPIOD_FLAGS_BIT_NONEXCLUSIVE if the
> shared GPIO is configured to follow pass through 'strategy' for the shared GPIO
> pin.
> 
> I have only implemented DT support.
> 
> With the shared gpio support one can choose between three different strategy for
> managing the shared gpio:
> refcounted low: Keep the line low as long as there is at least one low
> 		request is registered
> refcounted high: Keep the line high as long as there is at least one high
> 		request is registered
> pass through: all requests are allowed to go through without refcounting.
> 
> Few shortcomings as of now:
> - can not handle different GPIO_ACTIVE_ on the user side, both the root GPIO
>   (which is shared) and clients must have the same GPIO_ACTIVE_ mode.
>   We are using common gpio_desc.
>   Like with GPIOD_FLAGS_BIT_NONEXCLUSIVE
> - refcounting counts _all_ 1/0 requests coming from the users of the shared
>   GPIO. This could cause issues if clients are using the gpiod API in unbalanced
>   way.
>   We would need to have separate tracking for each of the clients and agregate
>   the level they are asking for at any moment. Basically a new gpio-chip on top
>   of the real gpio pin can solve this.

I forgot to add an example for DT.

I have this for two pcm3168a codec, their RST pin is connected to
tca6416's p0 pin.
The codec's RST line is low active, so the gpio-shared is configured to
refcounted high to make sure that a codec is not placed in reset when it
does not want it.

&main_i2c3 {
	#address-cells = <1>;
	#size-cells = <0>;

	audio_exp: gpio@21 {
		compatible = "ti,tca6416";
		reg = <0x21>;
		gpio-controller;
		#gpio-cells = <2>;

		p00 {
			gpio-shared;
			gpios = <0 GPIO_ACTIVE_LOW>;
			output-high;
			refcounted-high;
			line-name = "CODEC RESET";
		};
	};

	pcm3168a_a: audio-codec@47 {
		compatible = "ti,pcm3168a";
		reg = <0x47>;
		#sound-dai-cells = <1>;

		reset-gpios = <&audio_exp 0 GPIO_ACTIVE_LOW>;
	};

	pcm3168a_b: audio-codec@46 {
		compatible = "ti,pcm3168a";
		reg = <0x46>;
		#sound-dai-cells = <1>;

		reset-gpios = <&audio_exp 0 GPIO_ACTIVE_LOW>;
	};
};


> 
> Regards,
> Peter
> ---
> Peter Ujfalusi (2):
>   dt-bindings: gpio: Document shared GPIO line usage
>   gpiolib: Support for (output only) shared GPIO line
> 
>  .../devicetree/bindings/gpio/gpio.txt         |  66 +++++++++
>  drivers/gpio/gpiolib-of.c                     |  28 +++-
>  drivers/gpio/gpiolib.c                        | 132 ++++++++++++++++--
>  drivers/gpio/gpiolib.h                        |  10 ++
>  4 files changed, 223 insertions(+), 13 deletions(-)
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 1/2] dt-bindings: gpio: Document shared GPIO line usage

[Linus Walleij]

On Wed, Nov 20, 2019 at 2:34 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> Boards might use the same GPIO line to control several external devices.
> Add section to document on how a shared GPIO pin can be described.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

As I've stated earlier I think this information is surplus.
If two devices have a phandle to the same GPIO line
then it is by definition shared.

> +               line_a {
> +                       gpio-shared;

So this is unnecessary: if the same line is referenced
by phandle from two places it is shared, simple as that.
It is up to code in the operating system (like Linux) to
detect if they are shared in practice (both consumer
nodes are enabled) and then deal with the outcome.

> +                       gpios = <5 0>;
> +                       output-low;

This is overlapping with the use case to define initial
state values for GPIOs, something that has been
brought up repeatedly and I've collected links for
previous discussions several times.

I guess if need be I have to look them up again.

The DT maintainers don't like the hog syntax so
something else is desired for this.

> +                       refcounted-high;
(snip)
> +The shared GPIO line management strategy can be selected with either of the
> +following properties:
> +- refcounted-low: The line must be kept low as long as there is at least one
> +               request asking it to be low.
> +- refcounted-high: The line must be kept high as long as there is at least one
> +               request asking it to be high.

Is this really needed? Isn't it more appropriate to just define the
semantics such that as soon as some consumer requests the line
high it will be refcounted high, and as soon as it is requested
low by any consumer it will be refcounted low.

> +If neither of the refcounting strategy was selected then the shared GPIO is
> +handled as pass through. In this mode all user requests will be forwarded to the
> +shared GPIO pin without refcounting.

Why should this even be allowed? If we are defining a special semantic
for refcounted GPIOs (even defining a separate API in the Linux
OS, though it is beside the point) why do we have to have a fallback
to the old behaviour at all?

I think you can do this by just detecting multiple phandles to the
same GPIO and implicit refcounting for > 1 consumers.

I.e. no new bindings at all, maybe some patches explaining the
semantic effect of using the same GPIO from two consumer
nodes.

Yours,
Linus Walleij

Re: [RFC 2/2] gpiolib: Support for (output only) shared GPIO line

[Linus Walleij]

On Wed, Nov 20, 2019 at 2:34 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:

> This patch adds basic support for handling shared GPIO lines in the core.
> The line must be configured with a child node in DT.
> Based on the configuration the core will use different strategy to manage
> the shared line:
> refcounted low: Keep the line low as long as there is at least one low
>                 request is registered
> refcounted high: Keep the line high as long as there is at least one high
>                 request is registered
> pass through: all requests are allowed to go through without refcounting.
>
> The pass through mode is equivalent to how currently the
> GPIOD_FLAGS_BIT_NONEXCLUSIVE is handled.
>
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

This is a good start! Some ideas on how I'd like this to develop.

>  drivers/gpio/gpiolib-of.c |  28 ++++++--
>  drivers/gpio/gpiolib.c    | 132 +++++++++++++++++++++++++++++++++++---

Please put this API under its own Kconfig option
and in its own file in
drivers/gpio/gpiolib-refcounted.c
local header in
drivers/gpio/gpiolib-refcounted.h
only built in if the appropriate Kconfig is selected.

Consumer header in
include/linux/gpio/reference-counted.h
And add external driver API to this last file.

> --- a/drivers/gpio/gpiolib-of.c

No commenting on this because as pointed out in the binding
patch I want this done by simply detecting the same GPIO
being referenced by several <&gpio N> phandles.

> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> index ca9bc1e4803c..0eec0857e3a8 100644
> --- a/drivers/gpio/gpiolib.h
> +++ b/drivers/gpio/gpiolib.h
> @@ -111,11 +111,18 @@ struct gpio_desc {
>  #define FLAG_PULL_UP    13     /* GPIO has pull up enabled */
>  #define FLAG_PULL_DOWN  14     /* GPIO has pull down enabled */
>  #define FLAG_BIAS_DISABLE    15        /* GPIO has pull disabled */
> +#define FLAG_IS_SHARED 16      /* GPIO is shared */

This is a good way of flagging that this is a refcounted GPIO
I would call it FLAG_IS_REFERENCE_COUNTED as it is
more precise to what it means.

> +#define FLAG_REFCOUNT_LOW 17   /* Shared GPIO is refcounted for raw low */
> +#define FLAG_REFCOUNT_HIGH 18  /* Shared GPIO is refcounted for raw high */

Do not put this here, keep it in the local refcounted GPIO
struct gpio_refcount_desc.

>         /* Connection label */
>         const char              *label;
>         /* Name of the GPIO */
>         const char              *name;
> +       /* Number of users of a shared GPIO */
> +       int                     shared_users;
> +       /* Reference counter for shared GPIO (low or high level) */
> +       int                     level_refcount;

We can't expand this struct for everyone on the planet like this.

In the local header

drivers/gpio/gpiolib-refcount.h create something like:

struct gpio_refcount_desc {
    struct gpio_desc *gd;
    int shared_users;
    int level_refcount;
};

This should be the opaque cookie returned to consumers of this new
refcounted API.

It defines the reference counted API as separate and optional from
the non-reference counted API, also using its own API.

The only effect on the core
gpiolib will be the single flag in struct gpio_desc; and some
calls into the shared code with stubs if the support for systems
that do not enable the reference counting API.

Yours,
Linus Walleij

Re: [RFC 1/2] dt-bindings: gpio: Document shared GPIO line usage

[Peter Ujfalusi]

On 22/11/2019 14.10, Linus Walleij wrote:
> On Wed, Nov 20, 2019 at 2:34 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> 
>> Boards might use the same GPIO line to control several external devices.
>> Add section to document on how a shared GPIO pin can be described.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> As I've stated earlier I think this information is surplus.
> If two devices have a phandle to the same GPIO line
> then it is by definition shared.

Well, phandle + line number to be precise. There might be a way to grep
the whole DT and figure out if really we have multiple users for the
same line behind of the phandle.

> 
>> +               line_a {
>> +                       gpio-shared;
> 
> So this is unnecessary: if the same line is referenced
> by phandle from two places it is shared, simple as that.

phandle is pointing to the gpio controller, not to the line.

> It is up to code in the operating system (like Linux) to
> detect if they are shared in practice (both consumer
> nodes are enabled) and then deal with the outcome.

Ideally yes.

>> +                       gpios = <5 0>;
>> +                       output-low;
> 
> This is overlapping with the use case to define initial
> state values for GPIOs, something that has been
> brought up repeatedly and I've collected links for
> previous discussions several times.

I don't mind this to go away and the first set would configure the level.
Kept it here so I can reuse the gpio-hog code from gpiolib-of ;)

> I guess if need be I have to look them up again.
> 
> The DT maintainers don't like the hog syntax so
> something else is desired for this.

I see, so the gpio-hog might change?

>> +                       refcounted-high;
> (snip)
>> +The shared GPIO line management strategy can be selected with either of the
>> +following properties:
>> +- refcounted-low: The line must be kept low as long as there is at least one
>> +               request asking it to be low.
>> +- refcounted-high: The line must be kept high as long as there is at least one
>> +               request asking it to be high.
> 
> Is this really needed? Isn't it more appropriate to just define the
> semantics such that as soon as some consumer requests the line
> high it will be refcounted high, and as soon as it is requested
> low by any consumer it will be refcounted low.

Well. How do we decide which level is the one that should be preserved?

How would the core decide what to in a simplest case:
two device, they are the same part.
ENABLE pin which needs to be high to enable the device.
When the driver probes it asks for initial deasserted GPIO as the device
is not in active use.

We are refcouting low?

Then one device is enabled, it asks for assert for the line.

We are now refcounting high?

Then the other device is also enabled, it also asserts the line.

We are still refcounting high?

_one_ of the device is no longer in use, the driver asks for deassert.

We are switching to refcount low and set the line low?
That would break the other device as it still want the line high.

and no, we can not refcount for the asserted state as if the pin is
RESET and active low, then assert will disable the devices.

Now if you throw in a third device to the mix, the whole global
refcounting will be off even more.

Furthermore:
If you have two different devices one with ENABLE (low=reset,
high=enabled) the other with RESET (low=reset, high=enabled) pin and
they are described in DT correctly and the driver is written correctly
for assert/deassert, then this dummy shared implementation will just
fail as we are sharing the same gpio_desc between them, but they want to
use it with different GPIO_ACTIVE_* level.

>> +If neither of the refcounting strategy was selected then the shared GPIO is
>> +handled as pass through. In this mode all user requests will be forwarded to the
>> +shared GPIO pin without refcounting.
> 
> Why should this even be allowed? If we are defining a special semantic
> for refcounted GPIOs (even defining a separate API in the Linux
> OS, though it is beside the point) why do we have to have a fallback
> to the old behaviour at all?

I kept it because there were comments for the gpio-shared driver
indicating that in some cases pass-through shared line might be needed
for some setups.

> I think you can do this by just detecting multiple phandles to the
> same GPIO and implicit refcounting for > 1 consumers.

You mean that at every gpio request we would parse the DT to see if
there is another gpio binding pointing to the same gpio pin?
Or want to do a parse for all gpio pins during boot and mark the pins
which have multiple gpio binding pointing to them?

> I.e. no new bindings at all, maybe some patches explaining the
> semantic effect of using the same GPIO from two consumer
> nodes.

The level that needs to be preserved need to be communicated somehow imho.

If you plan to have separate API for this as I believe you do, then probably

struct gpio_refcount_desc *gpiod_refcounted_get(struct device *dev,
					const char *con_id,
					enum gpiod_flags flags,
					bool i_care_for_asserted_state);

If the device have ENABLE with active high, the driver would:
gpiod_refcounted_get(dev, "enable", GPIOD_OUT_DEASSERTED, true);

if it has RESET with active low, the driver would:
gpiod_refcounted_get(dev, "reset", GPIOD_OUT_ASSERTED, false);

if it is desired that the device would be placed in reset/power-down
when the driver probes.

Then the core would need to figure out what these actually mean at the end.

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 2/2] gpiolib: Support for (output only) shared GPIO line

[Peter Ujfalusi]

On 22/11/2019 14.22, Linus Walleij wrote:
> On Wed, Nov 20, 2019 at 2:34 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> 
>> This patch adds basic support for handling shared GPIO lines in the core.
>> The line must be configured with a child node in DT.
>> Based on the configuration the core will use different strategy to manage
>> the shared line:
>> refcounted low: Keep the line low as long as there is at least one low
>>                 request is registered
>> refcounted high: Keep the line high as long as there is at least one high
>>                 request is registered
>> pass through: all requests are allowed to go through without refcounting.
>>
>> The pass through mode is equivalent to how currently the
>> GPIOD_FLAGS_BIT_NONEXCLUSIVE is handled.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> This is a good start! Some ideas on how I'd like this to develop.

Thanks!

> 
>>  drivers/gpio/gpiolib-of.c |  28 ++++++--
>>  drivers/gpio/gpiolib.c    | 132 +++++++++++++++++++++++++++++++++++---
> 
> Please put this API under its own Kconfig option
> and in its own file in
> drivers/gpio/gpiolib-refcounted.c
> local header in
> drivers/gpio/gpiolib-refcounted.h
> only built in if the appropriate Kconfig is selected.

This patch is not really an API, but extension to the current one so
that clients does not need to be aware of the shared use.

> Consumer header in
> include/linux/gpio/reference-counted.h
> And add external driver API to this last file.

would it be better to have
include/linux/gpio/consumer-refcounted.h

> 
>> --- a/drivers/gpio/gpiolib-of.c
> 
> No commenting on this because as pointed out in the binding
> patch I want this done by simply detecting the same GPIO
> being referenced by several <&gpio N> phandles.

Would you scan all pins for each gpio-chip during boot time or scan only
when a gpio is requested and it is not already requested (so it is shared)?

>> diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
>> index ca9bc1e4803c..0eec0857e3a8 100644
>> --- a/drivers/gpio/gpiolib.h
>> +++ b/drivers/gpio/gpiolib.h
>> @@ -111,11 +111,18 @@ struct gpio_desc {
>>  #define FLAG_PULL_UP    13     /* GPIO has pull up enabled */
>>  #define FLAG_PULL_DOWN  14     /* GPIO has pull down enabled */
>>  #define FLAG_BIAS_DISABLE    15        /* GPIO has pull disabled */
>> +#define FLAG_IS_SHARED 16      /* GPIO is shared */
> 
> This is a good way of flagging that this is a refcounted GPIO
> I would call it FLAG_IS_REFERENCE_COUNTED as it is
> more precise to what it means.

As I said before, I think this refcounting is not going to work nicely
when we have actually shared gpio.

>> +#define FLAG_REFCOUNT_LOW 17   /* Shared GPIO is refcounted for raw low */
>> +#define FLAG_REFCOUNT_HIGH 18  /* Shared GPIO is refcounted for raw high */
> 
> Do not put this here, keep it in the local refcounted GPIO
> struct gpio_refcount_desc.

OK.

>>         /* Connection label */
>>         const char              *label;
>>         /* Name of the GPIO */
>>         const char              *name;
>> +       /* Number of users of a shared GPIO */
>> +       int                     shared_users;
>> +       /* Reference counter for shared GPIO (low or high level) */
>> +       int                     level_refcount;
> 
> We can't expand this struct for everyone on the planet like this.
> 
> In the local header
> 
> drivers/gpio/gpiolib-refcount.h create something like:
> 
> struct gpio_refcount_desc {
>     struct gpio_desc *gd;
>     int shared_users;
>     int level_refcount;
> };

So. If we give this to multiple users then how different GPIO_ACTIVE_*
would be handled? What flag would be used for gd?
How do we know which level that needs to be preserved?

struct gpio_refcount_desc *gpiod_refcounted_get(struct device *dev,
					const char *con_id,
					enum gpiod_flags flags,
					bool i_care_for_asserted_state);

Would take care of that, but we will still have the issue coming from
the global refcounting.

Hrm, actually we might not. If we use the level_refcount to count for
high for example, then we would never decrement below 0 (as I do in this
patch) then things might be balanced.
No, they are not:

We want high refcount for the gpio.
Driver1 probes, asks for gpio and it asks it to be low.
Driver1 sets the gpio to high as it is enabled.
Driver2 probes, asks for the gpio and it asks it to be low.
Device1 also got reset.
Driver2 is not enabling the gpio as it is not needing it
Driver1 bugs out on access to chip.

> This should be the opaque cookie returned to consumers of this new
> refcounted API.
> 
> It defines the reference counted API as separate and optional from
> the non-reference counted API, also using its own API.
> 
> The only effect on the core
> gpiolib will be the single flag in struct gpio_desc; and some
> calls into the shared code with stubs if the support for systems
> that do not enable the reference counting API.

One thing which might work, but it might be a longer shot:
If there is a chance that the driver is used in shared gpio case, the
driver needs to use the new API. I would not call it refcounted, but
perhaps 'managed'?

When the first driver requests the gpio, then we would allocate a
'master' managed descriptor, similar to "struct gpio_refcount_desc" but
with list_head and probably something else as well (lock/mutex, whatever
we need). Get the gpio for it, mark it as shared.
Allocate a 'client' managed struct which we add to the list and give it
back to the driver requesting the gpio line.

Likely the 'master' struct needs to be in a global list as well to be
able to find the correct one when someone else is requesting the same
gpio, which is already managed.
SO we might need to list_heads in there, one for the global 'masters'
list and one for it's 'clients' list.

From here it is kind of simple as when a client driver asks for a level,
we will change the 'client' node's state the the level and then ask the
'master' to check all of it's 'clients' current state and decide if the
real GPIO needs to be changed and to what level.

> 
> Yours,
> Linus Walleij
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 1/2] dt-bindings: gpio: Document shared GPIO line usage

[Linus Walleij]

On Fri, Nov 22, 2019 at 2:36 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 22/11/2019 14.10, Linus Walleij wrote:
> > On Wed, Nov 20, 2019 at 2:34 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> >
> >> Boards might use the same GPIO line to control several external devices.
> >> Add section to document on how a shared GPIO pin can be described.
> >>
> >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> >
> > As I've stated earlier I think this information is surplus.
> > If two devices have a phandle to the same GPIO line
> > then it is by definition shared.
>
> Well, phandle + line number to be precise.

This is what I mean when I say "phandle to the same GPIO line".
Like this:

foo-gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;

If the phandle <&gpio0 5 *>; appear in some other
(non-disabled) node it has > 1 users.

> >> +               line_a {
> >> +                       gpio-shared;
> >
> > So this is unnecessary: if the same line is referenced
> > by phandle from two places it is shared, simple as that.
>
> phandle is pointing to the gpio controller, not to the line.

Cleared up above.

> >> +                       gpios = <5 0>;
> >> +                       output-low;
> >
> > This is overlapping with the use case to define initial
> > state values for GPIOs, something that has been
> > brought up repeatedly and I've collected links for
> > previous discussions several times.
>
> I don't mind this to go away and the first set would configure the level.
> Kept it here so I can reuse the gpio-hog code from gpiolib-of ;)

People have tried to reuse the hog code to set up
initial line levels as well, it failed because they could
not get the DT bindings through the door.

> > I guess if need be I have to look them up again.
> >
> > The DT maintainers don't like the hog syntax so
> > something else is desired for this.
>
> I see, so the gpio-hog might change?

They will not change since they are ABI, but their
use case will not be extended AFAICT.
Not my pick, I liked the hog syntax but we need
consensus.

> > (snip)
> >> +The shared GPIO line management strategy can be selected with either of the
> >> +following properties:
> >> +- refcounted-low: The line must be kept low as long as there is at least one
> >> +               request asking it to be low.
> >> +- refcounted-high: The line must be kept high as long as there is at least one
> >> +               request asking it to be high.
> >
> > Is this really needed? Isn't it more appropriate to just define the
> > semantics such that as soon as some consumer requests the line
> > high it will be refcounted high, and as soon as it is requested
> > low by any consumer it will be refcounted low.
>
> Well. How do we decide which level is the one that should be preserved?

First come first serve.

If there is any conflict amongst the consumers we are
screwed anyway so why try to establish where they should
agree if they don't agree?

> How would the core decide what to in a simplest case:
> two device, they are the same part.
> ENABLE pin which needs to be high to enable the device.
> When the driver probes it asks for initial deasserted GPIO as the device
> is not in active use.

This makes me think it should be a unique driver
with a unique compatible string, as it embodies
use cases.

It is too broad to just define
refcounted-high or refcounted-low, that is hiding the
real use case, so I would go for something like a
resource in the device tree that all other devices that
need it can take.

Like a reset controller, precisely:

reset: reset-controller {
    compatible = "reset-gpio";
    gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
    #reset-cells = <0>;
};

dev0 {
    resets = <&reset>;
};

dev1 {
    resets = <&reset>;
};

The ambition to use refcounted GPIOs to solve this
usecase is probably wrong, I would say try to go for a
GPIO-based reset controller instead.

The fact that some Linux drivers are already using explicit
GPIO's for their reset handling is maybe unfortunate,
they will simply have to grow code to deal with a reset
alternatively to GPIO, like first try to grab a reset
handle and if that doesn't fall back to use a GPIO.

I would say don't try to shoehorn this use case into the
gpio library but instead try to create a reset controller that
takes care of arbitrating the use of a single GPIO line.

Yours,
Linus Walleij

Re: [RFC 1/2] dt-bindings: gpio: Document shared GPIO line usage

[Peter Ujfalusi]

On 28/11/2019 12.06, Linus Walleij wrote:
> On Fri, Nov 22, 2019 at 2:36 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> On 22/11/2019 14.10, Linus Walleij wrote:
>>> On Wed, Nov 20, 2019 at 2:34 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>>>
>>>> Boards might use the same GPIO line to control several external devices.
>>>> Add section to document on how a shared GPIO pin can be described.
>>>>
>>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>>
>>> As I've stated earlier I think this information is surplus.
>>> If two devices have a phandle to the same GPIO line
>>> then it is by definition shared.
>>
>> Well, phandle + line number to be precise.
> 
> This is what I mean when I say "phandle to the same GPIO line".
> Like this:
> 
> foo-gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
> 
> If the phandle <&gpio0 5 *>; appear in some other
> (non-disabled) node it has > 1 users.

I thought so.
Not sure how to look up (and how expensive it is) to find the nodes
which contain any gpio(lib) binding pointing to the given line.

> 
>>>> +               line_a {
>>>> +                       gpio-shared;
>>>
>>> So this is unnecessary: if the same line is referenced
>>> by phandle from two places it is shared, simple as that.
>>
>> phandle is pointing to the gpio controller, not to the line.
> 
> Cleared up above.
> 
>>>> +                       gpios = <5 0>;
>>>> +                       output-low;
>>>
>>> This is overlapping with the use case to define initial
>>> state values for GPIOs, something that has been
>>> brought up repeatedly and I've collected links for
>>> previous discussions several times.
>>
>> I don't mind this to go away and the first set would configure the level.
>> Kept it here so I can reuse the gpio-hog code from gpiolib-of ;)
> 
> People have tried to reuse the hog code to set up
> initial line levels as well, it failed because they could
> not get the DT bindings through the door.

But we are happily using the gpio-hog to control board level muxes to
select functionality...

Initial level is a tricky one, for outputs there is a valid use case for
them for sure. If the GPIO is used to control LCD backlight for example.
You want the backlight to not flicker due to gpio state changes.

It depends on how it is configured when the kernel boots, do we have
users of the given GPIO.

Again, different issue.

>>> I guess if need be I have to look them up again.
>>>
>>> The DT maintainers don't like the hog syntax so
>>> something else is desired for this.
>>
>> I see, so the gpio-hog might change?
> 
> They will not change since they are ABI, but their
> use case will not be extended AFAICT.
> Not my pick, I liked the hog syntax but we need
> consensus.

OK.

>>> (snip)
>>>> +The shared GPIO line management strategy can be selected with either of the
>>>> +following properties:
>>>> +- refcounted-low: The line must be kept low as long as there is at least one
>>>> +               request asking it to be low.
>>>> +- refcounted-high: The line must be kept high as long as there is at least one
>>>> +               request asking it to be high.
>>>
>>> Is this really needed? Isn't it more appropriate to just define the
>>> semantics such that as soon as some consumer requests the line
>>> high it will be refcounted high, and as soon as it is requested
>>> low by any consumer it will be refcounted low.
>>
>> Well. How do we decide which level is the one that should be preserved?
> 
> First come first serve.
> 
> If there is any conflict amongst the consumers we are
> screwed anyway so why try to establish where they should
> agree if they don't agree?

They must agree on the (precious, must be preserved) level _on_ the GPIO
chip side.
It is another matter if one driver will power down it's device at probe,
the other would enable it.
This must not matter, both of them needs the same level to be enabled
and it might not be the level they will request first.

>> How would the core decide what to in a simplest case:
>> two device, they are the same part.
>> ENABLE pin which needs to be high to enable the device.
>> When the driver probes it asks for initial deasserted GPIO as the device
>> is not in active use.
> 
> This makes me think it should be a unique driver
> with a unique compatible string, as it embodies
> use cases.

Like the gpio-shared from the previous RFC ;)

> It is too broad to just define
> refcounted-high or refcounted-low, that is hiding the
> real use case, so I would go for something like a
> resource in the device tree that all other devices that
> need it can take.
> 
> Like a reset controller, precisely:
> 
> reset: reset-controller {
>     compatible = "reset-gpio";
>     gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
>     #reset-cells = <0>;
> };
> 
> dev0 {
>     resets = <&reset>;
> };
> 
> dev1 {
>     resets = <&reset>;
> };
> 
> The ambition to use refcounted GPIOs to solve this
> usecase is probably wrong, I would say try to go for a
> GPIO-based reset controller instead.

I did that. A bit more lines of code than the gpio-shared.
Only works if all clients are converted to reset controller, all must
use reset_control_get_shared()

But my biggest issue was that how would you put a resets/reset-names to
DT for a device where the gpio is used for enabling an output/input pin
and not to place the device or part of the device to reset.

Sure, one can say that something is in 'reset' when it is not enabled,
but do you put the LCD backlight to 'reset' when you turn it off?

Is your DC motor in 'reset' when it is not working?

GPIO stands for General Purpose Input/Output, one of the purpose is to
enable/disable things, reset things, turn on/off things or anything one
could use 3.3V (or more/less).

> The fact that some Linux drivers are already using explicit
> GPIO's for their reset handling is maybe unfortunate,
> they will simply have to grow code to deal with a reset
> alternatively to GPIO, like first try to grab a reset
> handle and if that doesn't fall back to use a GPIO.

Sure, it can be done, but when we hit a case when the reset framework is
not fitting for some devices use of the shared GPIO, then what we will do?

> I would say don't try to shoehorn this use case into the
> gpio library but instead try to create a reset controller that
> takes care of arbitrating the use of a single GPIO line.

It would certainly cover the use case I have.

How would it satisfy the regulator use case? We put the regulators to
'reset' when they are turned off / disabled?

> 
> Yours,
> Linus Walleij
> 

- Péter

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [RFC 1/2] dt-bindings: gpio: Document shared GPIO line usage

[Rob Herring]

On Thu, Nov 28, 2019 at 11:06:35AM +0100, Linus Walleij wrote:
> On Fri, Nov 22, 2019 at 2:36 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> > On 22/11/2019 14.10, Linus Walleij wrote:
> > > On Wed, Nov 20, 2019 at 2:34 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> > >
> > >> Boards might use the same GPIO line to control several external devices.
> > >> Add section to document on how a shared GPIO pin can be described.
> > >>
> > >> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> > >
> > > As I've stated earlier I think this information is surplus.
> > > If two devices have a phandle to the same GPIO line
> > > then it is by definition shared.
> >
> > Well, phandle + line number to be precise.
> 
> This is what I mean when I say "phandle to the same GPIO line".
> Like this:
> 
> foo-gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
> 
> If the phandle <&gpio0 5 *>; appear in some other
> (non-disabled) node it has > 1 users.
> 
> > >> +               line_a {
> > >> +                       gpio-shared;
> > >
> > > So this is unnecessary: if the same line is referenced
> > > by phandle from two places it is shared, simple as that.
> >
> > phandle is pointing to the gpio controller, not to the line.
> 
> Cleared up above.
> 
> > >> +                       gpios = <5 0>;
> > >> +                       output-low;
> > >
> > > This is overlapping with the use case to define initial
> > > state values for GPIOs, something that has been
> > > brought up repeatedly and I've collected links for
> > > previous discussions several times.
> >
> > I don't mind this to go away and the first set would configure the level.
> > Kept it here so I can reuse the gpio-hog code from gpiolib-of ;)
> 
> People have tried to reuse the hog code to set up
> initial line levels as well, it failed because they could
> not get the DT bindings through the door.
> 
> > > I guess if need be I have to look them up again.
> > >
> > > The DT maintainers don't like the hog syntax so
> > > something else is desired for this.
> >
> > I see, so the gpio-hog might change?
> 
> They will not change since they are ABI, but their
> use case will not be extended AFAICT.
> Not my pick, I liked the hog syntax but we need
> consensus.
> 
> > > (snip)
> > >> +The shared GPIO line management strategy can be selected with either of the
> > >> +following properties:
> > >> +- refcounted-low: The line must be kept low as long as there is at least one
> > >> +               request asking it to be low.
> > >> +- refcounted-high: The line must be kept high as long as there is at least one
> > >> +               request asking it to be high.
> > >
> > > Is this really needed? Isn't it more appropriate to just define the
> > > semantics such that as soon as some consumer requests the line
> > > high it will be refcounted high, and as soon as it is requested
> > > low by any consumer it will be refcounted low.
> >
> > Well. How do we decide which level is the one that should be preserved?
> 
> First come first serve.
> 
> If there is any conflict amongst the consumers we are
> screwed anyway so why try to establish where they should
> agree if they don't agree?
> 
> > How would the core decide what to in a simplest case:
> > two device, they are the same part.
> > ENABLE pin which needs to be high to enable the device.
> > When the driver probes it asks for initial deasserted GPIO as the device
> > is not in active use.
> 
> This makes me think it should be a unique driver
> with a unique compatible string, as it embodies
> use cases.
> 
> It is too broad to just define
> refcounted-high or refcounted-low, that is hiding the
> real use case, so I would go for something like a
> resource in the device tree that all other devices that
> need it can take.

I have similar thoughts.

> Like a reset controller, precisely:
> 
> reset: reset-controller {
>     compatible = "reset-gpio";
>     gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
>     #reset-cells = <0>;
> };
> 
> dev0 {
>     resets = <&reset>;
> };
> 
> dev1 {
>     resets = <&reset>;
> };

> 
> The ambition to use refcounted GPIOs to solve this
> usecase is probably wrong, I would say try to go for a
> GPIO-based reset controller instead.

Yes, but I think we can have that AND use the existing binding.

> The fact that some Linux drivers are already using explicit
> GPIO's for their reset handling is maybe unfortunate,
> they will simply have to grow code to deal with a reset
> alternatively to GPIO, like first try to grab a reset
> handle and if that doesn't fall back to use a GPIO.

I think this could just be all handled within the reset subsystem given 
that we've been consistent in using 'reset-gpios' (GPIO regulators are 
similar, but we never had such consistency with GPIO names for 
regulators). We can implement a reset driver for the 'reset-gpios' 
property that deals with the sharing. Drivers to DT nodes doesn't have 
to be 1:1. It's convenient when they are, but that's encoding the OS's 
(current) driver structure into DT.

Rob

Re: [RFC 1/2] dt-bindings: gpio: Document shared GPIO line usage

[Linus Walleij]

On Wed, Dec 4, 2019 at 12:51 AM Rob Herring <robh@kernel.org> wrote:
> On Thu, Nov 28, 2019 at 11:06:35AM +0100, Linus Walleij wrote:

> > The ambition to use refcounted GPIOs to solve this
> > usecase is probably wrong, I would say try to go for a
> > GPIO-based reset controller instead.
>
> Yes, but I think we can have that AND use the existing binding.
>
> > The fact that some Linux drivers are already using explicit
> > GPIO's for their reset handling is maybe unfortunate,
> > they will simply have to grow code to deal with a reset
> > alternatively to GPIO, like first try to grab a reset
> > handle and if that doesn't fall back to use a GPIO.
>
> I think this could just be all handled within the reset subsystem given
> that we've been consistent in using 'reset-gpios' (GPIO regulators are
> similar, but we never had such consistency with GPIO names for
> regulators). We can implement a reset driver for the 'reset-gpios'
> property that deals with the sharing. Drivers to DT nodes doesn't have
> to be 1:1. It's convenient when they are, but that's encoding the OS's
> (current) driver structure into DT.

This seems like a good approach if it can be made to work.
reset-gpios should have the right polarity flags (else drivers
and/or device trees need to be fixed...) so the driver can simply
scan over them and try to build a consensus on how to assert
or deassert a shared reset-gpios line.

It is also a natural placeholder for the connection to device
core that will inevitably need to happen the day the device
hierarchy needs to be torn up/down for a reset on some
random device.

Peter, will you have a go at it?

Yours,
Linus Walleij

Re: [RFC 1/2] dt-bindings: gpio: Document shared GPIO line usage

[Linus Walleij]

On Mon, Dec 2, 2019 at 10:31 PM Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 28/11/2019 12.06, Linus Walleij wrote:

> > The ambition to use refcounted GPIOs to solve this
> > usecase is probably wrong, I would say try to go for a
> > GPIO-based reset controller instead.
>
> I did that. A bit more lines of code than the gpio-shared.
> Only works if all clients are converted to reset controller, all must
> use reset_control_get_shared()

I don't think that's too much to ask, the usecase needs to
be expressed somewhere whether in code or DT properties.

> But my biggest issue was that how would you put a resets/reset-names to
> DT for a device where the gpio is used for enabling an output/input pin
> and not to place the device or part of the device to reset.

Rob suggest using GPIOs but represent them in the Linux kernel
as resets.

This would be a semantic effect of the line being named "reset-gpios"
as Rob pointed out. Name implies usage. We can formalize it
with DT YAML as well, these days, then it is impossible to get it
wrong, as long as the bindings are correct.

When you call the reset subsystem to create the reset handle
it can be instructed to look for a GPIO, possibly shared, and
this way replace the current explicit GPIO handling code
in the driver. It will just look as a reset no matter how many
other device or just this one is using it.

> Sure, one can say that something is in 'reset' when it is not enabled,
> but do you put the LCD backlight to 'reset' when you turn it off?
>
> Is your DC motor in 'reset' when it is not working?
>
> GPIO stands for General Purpose Input/Output, one of the purpose is to
> enable/disable things, reset things, turn on/off things or anything one
> could use 3.3V (or more/less).

Answered by explict interpretation of DT bindings
named "reset-gpios". Those are resets, nothing else.

> > The fact that some Linux drivers are already using explicit
> > GPIO's for their reset handling is maybe unfortunate,
> > they will simply have to grow code to deal with a reset
> > alternatively to GPIO, like first try to grab a reset
> > handle and if that doesn't fall back to use a GPIO.
>
> Sure, it can be done, but when we hit a case when the reset framework is
> not fitting for some devices use of the shared GPIO, then what we will do?

That can be said about literally anything we do in any
framework we design. Rough consensus and running code.
Bad paths will be taken sometimes, hopefully not too much.
We clean up the mess we create and refactor as we go
along, it is always optimistic design.

> How would it satisfy the regulator use case? We put the regulators to
> 'reset' when they are turned off / disabled?

We don't try to fix that now, if it's not broken don't fix it.
Let's try to fix the reset problem instead.

Yours,
Linus Walleij