[RFC PATCH v2 0/3] gpio: Add gpio-delay support

[RFC PATCH v2 0/3] gpio: Add gpio-delay support

[Alexander Stein]

Hello everyone,

thanks for the feedback I've received. This is the reworked RFC for
adressing a platform specific ramp-up/ramp-down delay on GPIO outputs.
Now the delays are neither specified as gpio-controller nor
consumer-specific properties.

v2 is a different approach than v1 in that it adds a new driver which will
simply forward setting the GPIO output of specified GPIOs in OF node.
The ramp-up/ramp-down delay can now be actually defined on consumer side,
see Patch 1 or 3 for examples.

Thanks a lot to the existing gpio-latch driver where I could learn/copy
from a lot for creating this driver!

Patch 1 is the new binding. I welcome improvements for the description,
if needed.

Patch 2 is the new driver. I'm open for a better name, if the current one
is too ambiguous.

Patch 3 is what I am actually using for testing. It is actually based
on a not-yet-commited patch, but the diff should be enough for
demonstration.

Alexander Stein (3):
  dt-bindings: gpio: Add gpio-delay binding document
  gpio: Add gpio delay driver
  [DNI] arm64: dts: mba8mx: Use gpio-delay for LVDS bridge

 .../devicetree/bindings/gpio/gpio-delay.yaml  |  75 ++++++++
 arch/arm64/boot/dts/freescale/mba8mx.dtsi     |  14 +-
 drivers/gpio/Kconfig                          |   8 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-delay.c                     | 164 ++++++++++++++++++
 5 files changed, 261 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-delay.yaml
 create mode 100644 drivers/gpio/gpio-delay.c

-- 
2.34.1

[RFC PATCH v2 1/3] dt-bindings: gpio: Add gpio-delay binding document

[Alexander Stein]

This adds bindings for a GPIO enable/disable delay driver.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 .../devicetree/bindings/gpio/gpio-delay.yaml  | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-delay.yaml

diff --git a/Documentation/devicetree/bindings/gpio/gpio-delay.yaml b/Documentation/devicetree/bindings/gpio/gpio-delay.yaml
new file mode 100644
index 000000000000..20871356e9b5
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-delay.yaml
@@ -0,0 +1,75 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-delay.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO delay controller
+
+maintainers:
+  - Alexander Stein <linux@ew.tq-group.com>
+
+description: |
+  This binding describes an electrical setup where setting an GPIO output
+  is delayed by some external setup, e.g. RC curcuit.
+
+  +----------+                    +-----------+
+  |          |             VCC_B  |           |
+  |          |              |     |           |
+  |          | VCC_A        _     |           |
+  |  GPIO    |             | | R  |  Consumer |
+  |controller|        ___  |_|    |           |
+  |          |       |   |  |     |           |
+  |      [IOx|-------|   |--+-----|-----+     |
+  |          |       |___|  |     |   input   |
+  |          |              |     |           |
+  +----------+             --- C  +-----------+
+                           ---
+                            |
+                            -
+                           GND
+
+  If the input on the consumer is controlled by an open-drain signal
+  attached to an RC curcuit the ramp-up delay is not under control
+  of the GPIO controller.
+
+properties:
+  compatible:
+    const: gpio-delay
+
+  "#gpio-cells":
+    description: |
+      Specifies the pin, ramp-up and ramp-down delays. The
+      delays are specified in microseconds.
+    const: 3
+
+  input-gpios:
+    description: Array of GPIOs which output signal change is delayed
+
+  gpio-controller: true
+
+  gpio-line-names: true
+
+required:
+  - compatible
+  - "#gpio-cells"
+  - gpio-controller
+  - input-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    enable_delay: enable-delay {
+        compatible = "gpio-delay";
+        #gpio-cells = <3>;
+        gpio-controller;
+        input-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>,
+                      <&gpio3 1 GPIO_ACTIVE_HIGH>;
+    };
+
+    consumer {
+        enable-gpios = <&enable_delay 0 130000 30000>;
+    };
-- 
2.34.1

[RFC PATCH v2 2/3] gpio: Add gpio delay driver

[Alexander Stein]

This driver implements a GPIO enable/disable delay. It supports a list
of GPIO outputs, which ramp-up/ramp-down delay can be specified at
consumer location.
The main purpose is to address external, passive delays upon line
voltage changes.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/gpio/Kconfig      |   8 ++
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/gpio-delay.c | 164 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 drivers/gpio/gpio-delay.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ec7cfd4f52b1..8f1dcbe3fd00 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1684,6 +1684,14 @@ config GPIO_AGGREGATOR
 	      industrial control context, to be operated from userspace using
 	      the GPIO chardev interface.
 
+config GPIO_DELAY
+	tristate "GPIO delay"
+	help
+	  Say yes here to enable the GPIO delay, which provides a way to
+	  configure platform specific delays for GPIO ramp-up or ramp-down
+	  delays. This can serve the following purposes:
+	    - Open-drain output using an RC filter
+
 config GPIO_LATCH
 	tristate "GPIO latch driver"
 	help
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 010587025fc8..56b6cbe35248 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_GPIO_DA9052)		+= gpio-da9052.o
 obj-$(CONFIG_GPIO_DA9055)		+= gpio-da9055.o
 obj-$(CONFIG_GPIO_DAVINCI)		+= gpio-davinci.o
 obj-$(CONFIG_GPIO_DLN2)			+= gpio-dln2.o
+obj-$(CONFIG_GPIO_DELAY)		+= gpio-delay.o
 obj-$(CONFIG_GPIO_DWAPB)		+= gpio-dwapb.o
 obj-$(CONFIG_GPIO_EIC_SPRD)		+= gpio-eic-sprd.o
 obj-$(CONFIG_GPIO_EM)			+= gpio-em.o
diff --git a/drivers/gpio/gpio-delay.c b/drivers/gpio/gpio-delay.c
new file mode 100644
index 000000000000..a91d2ea9a8bc
--- /dev/null
+++ b/drivers/gpio/gpio-delay.c
@@ -0,0 +1,164 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2022 TQ-Systems GmbH
+ * Author: Alexander Stein <linux@ew.tq-group.com>
+ */
+
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+
+#include "gpiolib.h"
+
+struct gpio_delay_timing {
+	unsigned long ramp_up_delay_us;
+	unsigned long ramp_down_delay_us;
+};
+
+struct gpio_delay_priv {
+	struct gpio_chip gc;
+	struct gpio_descs *input_gpio;
+	struct gpio_delay_timing *delay_timings;
+};
+
+static int gpio_delay_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static void gpio_delay_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	struct gpio_delay_priv *priv = gpiochip_get_data(gc);
+	struct gpio_desc *gpio_desc = priv->input_gpio->desc[offset];
+	const struct gpio_delay_timing *delay_timings;
+	bool ramp_up;
+
+	gpiod_set_value(gpio_desc, val);
+
+	delay_timings = &priv->delay_timings[offset];
+	ramp_up = (!gpiod_is_active_low(gpio_desc) && val) ||
+		  (gpiod_is_active_low(gpio_desc) && !val);
+
+	if (ramp_up && delay_timings->ramp_up_delay_us)
+		udelay(delay_timings->ramp_up_delay_us);
+	if (!ramp_up && delay_timings->ramp_down_delay_us)
+		udelay(delay_timings->ramp_down_delay_us);
+}
+
+static void gpio_delay_set_can_sleep(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	struct gpio_delay_priv *priv = gpiochip_get_data(gc);
+	struct gpio_desc *gpio_desc = priv->input_gpio->desc[offset];
+	const struct gpio_delay_timing *delay_timings;
+	bool ramp_up;
+
+	gpiod_set_value_cansleep(gpio_desc, val);
+
+	delay_timings = &priv->delay_timings[offset];
+	ramp_up = (!gpiod_is_active_low(gpio_desc) && val) ||
+		  (gpiod_is_active_low(gpio_desc) && !val);
+
+	if (ramp_up && delay_timings->ramp_up_delay_us)
+		fsleep(delay_timings->ramp_up_delay_us);
+	if (!ramp_up && delay_timings->ramp_down_delay_us)
+		fsleep(delay_timings->ramp_down_delay_us);
+}
+
+static int gpio_delay_of_xlate(struct gpio_chip *gc,
+			       const struct of_phandle_args *gpiospec,
+			       u32 *flags)
+{
+	struct gpio_delay_priv *priv = gpiochip_get_data(gc);
+	struct gpio_delay_timing *timings;
+	u32 line;
+
+	if (gpiospec->args_count != gc->of_gpio_n_cells)
+		return -EINVAL;
+
+	line = gpiospec->args[0];
+	if (line >= gc->ngpio)
+		return -EINVAL;
+
+	timings = &priv->delay_timings[line];
+	timings->ramp_up_delay_us = gpiospec->args[1];
+	timings->ramp_down_delay_us = gpiospec->args[2];
+
+	return line;
+}
+
+static bool gpio_delay_can_sleep(const struct gpio_delay_priv *priv)
+{
+	int i;
+
+	for (i = 0; i < priv->input_gpio->ndescs; i++)
+		if (gpiod_cansleep(priv->input_gpio->desc[i]))
+			return true;
+
+	return false;
+}
+
+static int gpio_delay_probe(struct platform_device *pdev)
+{
+	struct gpio_delay_priv *priv;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->input_gpio = devm_gpiod_get_array(&pdev->dev, "input", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->input_gpio))
+		return dev_err_probe(&pdev->dev, PTR_ERR(priv->input_gpio),
+				     "Failed to get input-gpios");
+
+	priv->delay_timings = devm_kcalloc(&pdev->dev,
+					   priv->input_gpio->ndescs,
+					   sizeof(*priv->delay_timings),
+					   GFP_KERNEL);
+	if (!priv->delay_timings)
+		return -ENOMEM;
+
+	if (gpio_delay_can_sleep(priv)) {
+		priv->gc.can_sleep = true;
+		priv->gc.set = gpio_delay_set_can_sleep;
+	} else {
+		priv->gc.can_sleep = false;
+		priv->gc.set = gpio_delay_set;
+	}
+
+	priv->gc.get_direction = gpio_delay_get_direction;
+	priv->gc.of_xlate = gpio_delay_of_xlate;
+	priv->gc.of_gpio_n_cells = 3;
+	priv->gc.ngpio = priv->input_gpio->ndescs;
+	priv->gc.owner = THIS_MODULE;
+	priv->gc.base = -1;
+	priv->gc.parent = &pdev->dev;
+
+	platform_set_drvdata(pdev, priv);
+
+	return devm_gpiochip_add_data(&pdev->dev, &priv->gc, priv);
+}
+
+static const struct of_device_id gpio_delay_ids[] = {
+	{
+		.compatible = "gpio-delay",
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, gpio_delay_ids);
+
+static struct platform_driver gpio_delay_driver = {
+	.driver	= {
+		.name		= "gpio-delay",
+		.of_match_table	= gpio_delay_ids,
+	},
+	.probe	= gpio_delay_probe,
+};
+module_platform_driver(gpio_delay_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Alexander Stein <alexander.stein@ew.tq-group.com>");
+MODULE_DESCRIPTION("GPIO delay driver");
-- 
2.34.1

[RFC PATCH v2 3/3] [DNI] arm64: dts: mba8mx: Use gpio-delay for LVDS bridge

[Alexander Stein]

Add a gpio-delay for LVDS_BRIDGE_EN_1V8 which ramp-up time is defined
by the external RC filter.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 arch/arm64/boot/dts/freescale/mba8mx.dtsi | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/mba8mx.dtsi b/arch/arm64/boot/dts/freescale/mba8mx.dtsi
index b0662dfa6194..a1ceadfa5b3d 100644
--- a/arch/arm64/boot/dts/freescale/mba8mx.dtsi
+++ b/arch/arm64/boot/dts/freescale/mba8mx.dtsi
@@ -75,6 +75,14 @@ led2: led2 {
 		};
 	};
 
+	gpio_delays: gpio-delays {
+		compatible = "gpio-delay";
+		#gpio-cells = <3>;
+		gpio-controller;
+		input-gpios = <&expander0 6 GPIO_ACTIVE_HIGH>;
+		gpio-line-names = "LVDS_BRIDGE_EN_1V8";
+	};
+
 	panel0: panel_lvds0 {
 		/*
 		 * Display is not fixed, so compatible has to be added from
@@ -191,6 +199,10 @@ expander0: gpio@23 {
 		interrupts = <9 IRQ_TYPE_EDGE_FALLING>;
 		interrupt-controller;
 		#interrupt-cells = <2>;
+		gpio-line-names = "", "", "", "",
+				  "", "", "LVDS_BRIDGE_EN_3V3", "",
+				  "", "", "", "",
+				  "", "", "", "";
 
 		sd-mux-oe-hog {
 			gpio-hog;
@@ -272,7 +284,7 @@ &i2c3 {
 	dsi_lvds_bridge: bridge@2d {
 		compatible = "ti,sn65dsi83";
 		reg = <0x2d>;
-		enable-gpios = <&expander0 6 GPIO_ACTIVE_HIGH>;
+		enable-gpios = <&gpio_delays 0 130000 0>;
 		vcc-supply = <&reg_sn65dsi83_1v8>;
 		status = "disabled";
 
-- 
2.34.1

Re: [RFC PATCH v2 1/3] dt-bindings: gpio: Add gpio-delay binding document

[Krzysztof Kozlowski]

On 14/12/2022 10:53, Alexander Stein wrote:
> This adds bindings for a GPIO enable/disable delay driver.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  .../devicetree/bindings/gpio/gpio-delay.yaml  | 75 +++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-delay.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-delay.yaml b/Documentation/devicetree/bindings/gpio/gpio-delay.yaml
> new file mode 100644
> index 000000000000..20871356e9b5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-delay.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-delay.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO delay controller
> +
> +maintainers:
> +  - Alexander Stein <linux@ew.tq-group.com>
> +
> +description: |
> +  This binding describes an electrical setup where setting an GPIO output
> +  is delayed by some external setup, e.g. RC curcuit.
> +
> +  +----------+                    +-----------+
> +  |          |             VCC_B  |           |
> +  |          |              |     |           |
> +  |          | VCC_A        _     |           |
> +  |  GPIO    |             | | R  |  Consumer |
> +  |controller|        ___  |_|    |           |
> +  |          |       |   |  |     |           |
> +  |      [IOx|-------|   |--+-----|-----+     |
> +  |          |       |___|  |     |   input   |
> +  |          |              |     |           |
> +  +----------+             --- C  +-----------+
> +                           ---
> +                            |
> +                            -
> +                           GND
> +
> +  If the input on the consumer is controlled by an open-drain signal

If IOx is open-drain, what is the VCC_A on the diagram? I think it
wasn't present in original Laurent's diagram.

> +  attached to an RC curcuit the ramp-up delay is not under control
> +  of the GPIO controller.
> +
> +properties:
> +  compatible:
> +    const: gpio-delay
> +
> +  "#gpio-cells":
> +    description: |
> +      Specifies the pin, ramp-up and ramp-down delays. The
> +      delays are specified in microseconds.
> +    const: 3
> +
> +  input-gpios:
> +    description: Array of GPIOs which output signal change is delayed

maxItems: 32 or some other reasonable value

> +
> +  gpio-controller: true
> +
> +  gpio-line-names: true

and then the same maxItems.

> +
> +required:
> +  - compatible
> +  - "#gpio-cells"
> +  - gpio-controller
> +  - input-gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    enable_delay: enable-delay {
> +        compatible = "gpio-delay";

I am not sure whether the naming is the most accurate - it represents
desired behavior (so the delay in rising signal), not actual hardware
(RC filter), but maybe that's a bit more generic.

Anyway look fine for me.

> +        #gpio-cells = <3>;
> +        gpio-controller;
> +        input-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>,
> +                      <&gpio3 1 GPIO_ACTIVE_HIGH>;
> +    };
> +
> +    consumer {
> +        enable-gpios = <&enable_delay 0 130000 30000>;
> +    };

Best regards,
Krzysztof

Re: [RFC PATCH v2 1/3] dt-bindings: gpio: Add gpio-delay binding document

[Alexander Stein]

Hi Krzysztof,

Am Donnerstag, 15. Dezember 2022, 10:11:47 CET schrieb Krzysztof Kozlowski:
> On 14/12/2022 10:53, Alexander Stein wrote:
> > This adds bindings for a GPIO enable/disable delay driver.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > 
> >  .../devicetree/bindings/gpio/gpio-delay.yaml  | 75 +++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-delay.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-delay.yaml
> > b/Documentation/devicetree/bindings/gpio/gpio-delay.yaml new file mode
> > 100644
> > index 000000000000..20871356e9b5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-delay.yaml
> > @@ -0,0 +1,75 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/gpio-delay.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: GPIO delay controller
> > +
> > +maintainers:
> > +  - Alexander Stein <linux@ew.tq-group.com>
> > +
> > +description: |
> > +  This binding describes an electrical setup where setting an GPIO output
> > +  is delayed by some external setup, e.g. RC curcuit.
> > +
> > +  +----------+                    +-----------+
> > +  |          |             VCC_B  |           |
> > +  |          |              |     |           |
> > +  |          | VCC_A        _     |           |
> > +  |  GPIO    |             | | R  |  Consumer |
> > +  |controller|        ___  |_|    |           |
> > +  |          |       |   |  |     |           |
> > +  |      [IOx|-------|   |--+-----|-----+     |
> > +  |          |       |___|  |     |   input   |
> > +  |          |              |     |           |
> > +  +----------+             --- C  +-----------+
> > +                           ---
> > +                            |
> > +                            -
> > +                           GND
> > +
> > +  If the input on the consumer is controlled by an open-drain signal
> 
> If IOx is open-drain, what is the VCC_A on the diagram? I think it
> wasn't present in original Laurent's diagram.

I have to admit my artistic skills are lacking :( I wanted to highlight that 
the actual GPIO output IOx is not (necessarily) the open-drain. This is 
somewhat important, because this can not be solved by just reconfiguring the 
GPIO to push-pull.

Instead there is a buffer (small box in the middle) which (in this case) 
converts from VCC_A on the left side connected to SoC to VCC_B on the right 
connected to consumer using an open-drain.
So this delay is induced passively by external circuits the SoC can not do 
anything about.

Best regards,
Alexander

> > +  attached to an RC curcuit the ramp-up delay is not under control
> > +  of the GPIO controller.
> > +
> > +properties:
> > +  compatible:
> > +    const: gpio-delay
> > +
> > +  "#gpio-cells":
> > +    description: |
> > +      Specifies the pin, ramp-up and ramp-down delays. The
> > +      delays are specified in microseconds.
> > +    const: 3
> > +
> > +  input-gpios:
> > +    description: Array of GPIOs which output signal change is delayed
> 
> maxItems: 32 or some other reasonable value

Okay. Apparently there is no limit within gpiolib, so I was not limiting the 
amount unnecessarily.

> > +
> > +  gpio-controller: true
> > +
> > +  gpio-line-names: true
> 
> and then the same maxItems.

Sure, will adjust as well.

> > +
> > +required:
> > +  - compatible
> > +  - "#gpio-cells"
> > +  - gpio-controller
> > +  - input-gpios
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    enable_delay: enable-delay {
> > +        compatible = "gpio-delay";
> 
> I am not sure whether the naming is the most accurate - it represents
> desired behavior (so the delay in rising signal), not actual hardware
> (RC filter), but maybe that's a bit more generic.
> 
> Anyway look fine for me.

IMHO delay fits pretty well, because it's the behaviour. I'm no hardware 
developer, but I assume that there are more possibilities than just RC filter 
which might require this delay.

Best regards,
Alexander

> > +        #gpio-cells = <3>;
> > +        gpio-controller;
> > +        input-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>,
> > +                      <&gpio3 1 GPIO_ACTIVE_HIGH>;
> > +    };
> > +
> > +    consumer {
> > +        enable-gpios = <&enable_delay 0 130000 30000>;
> > +    };
> 
> Best regards,
> Krzysztof

Re: [RFC PATCH v2 1/3] dt-bindings: gpio: Add gpio-delay binding document

[Linus Walleij]

On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:

> This adds bindings for a GPIO enable/disable delay driver.
>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

> +  input-gpios:
> +    description: Array of GPIOs which output signal change is delayed

I would just call this "gpios", it's not like we're ever going to add
any more specified GPIOs to this hardware, ever.

Yours,
Linus Walleij

Re: [RFC PATCH v2 0/3] gpio: Add gpio-delay support

[Linus Walleij]

On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:

> thanks for the feedback I've received. This is the reworked RFC for
> adressing a platform specific ramp-up/ramp-down delay on GPIO outputs.
> Now the delays are neither specified as gpio-controller nor
> consumer-specific properties.
>
> v2 is a different approach than v1 in that it adds a new driver which will
> simply forward setting the GPIO output of specified GPIOs in OF node.
> The ramp-up/ramp-down delay can now be actually defined on consumer side,
> see Patch 1 or 3 for examples.

I really like this approach, it looks better than I imagined.

> Thanks a lot to the existing gpio-latch driver where I could learn/copy
> from a lot for creating this driver!

Yeah that one is really neat, and also kind of sets a standard for
how we should do these things.

This patch set overall:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [RFC PATCH v2 0/3] gpio: Add gpio-delay support

[Rob Herring]

On Thu, Dec 15, 2022 at 7:16 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein
> <alexander.stein@ew.tq-group.com> wrote:
>
> > thanks for the feedback I've received. This is the reworked RFC for
> > adressing a platform specific ramp-up/ramp-down delay on GPIO outputs.
> > Now the delays are neither specified as gpio-controller nor
> > consumer-specific properties.
> >
> > v2 is a different approach than v1 in that it adds a new driver which will
> > simply forward setting the GPIO output of specified GPIOs in OF node.
> > The ramp-up/ramp-down delay can now be actually defined on consumer side,
> > see Patch 1 or 3 for examples.
>
> I really like this approach, it looks better than I imagined.

It seems over-engineered to me. So far no comments on my 3 suggestions either...

One is to just use some GPIO flag bits. Say 4-bits of GPIO flags
encoded as power of 2 ramp delay. We have to pick the units. For
example, 100us*2^N, which gives you 200us-3.2s of delay. Anything less
is short enough to just hard code in a driver.

Rob

Re: [RFC PATCH v2 0/3] gpio: Add gpio-delay support

[Laurent Pinchart]

On Thu, Dec 15, 2022 at 12:21:33PM -0600, Rob Herring wrote:
> On Thu, Dec 15, 2022 at 7:16 AM Linus Walleij wrote:
> > On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein wrote:
> >
> > > thanks for the feedback I've received. This is the reworked RFC for
> > > adressing a platform specific ramp-up/ramp-down delay on GPIO outputs.
> > > Now the delays are neither specified as gpio-controller nor
> > > consumer-specific properties.
> > >
> > > v2 is a different approach than v1 in that it adds a new driver which will
> > > simply forward setting the GPIO output of specified GPIOs in OF node.
> > > The ramp-up/ramp-down delay can now be actually defined on consumer side,
> > > see Patch 1 or 3 for examples.
> >
> > I really like this approach, it looks better than I imagined.
> 
> It seems over-engineered to me. So far no comments on my 3 suggestions either...

I like the idea of handling this on the consumer's side, possibly with
standard foo-gpios-ramp-{up,down}-delay-us (name to be bikeshedded)
properties as you mentioned in the review of v1.

> One is to just use some GPIO flag bits. Say 4-bits of GPIO flags
> encoded as power of 2 ramp delay. We have to pick the units. For
> example, 100us*2^N, which gives you 200us-3.2s of delay.

This could probably work too.

> Anything less is short enough to just hard code in a driver.

In which driver though ? The whole point is that we should avoid
handling this in particular drivers.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v2 0/3] gpio: Add gpio-delay support

[Rob Herring]

On Thu, Dec 15, 2022 at 3:26 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Thu, Dec 15, 2022 at 12:21:33PM -0600, Rob Herring wrote:
> > On Thu, Dec 15, 2022 at 7:16 AM Linus Walleij wrote:
> > > On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein wrote:
> > >
> > > > thanks for the feedback I've received. This is the reworked RFC for
> > > > adressing a platform specific ramp-up/ramp-down delay on GPIO outputs.
> > > > Now the delays are neither specified as gpio-controller nor
> > > > consumer-specific properties.
> > > >
> > > > v2 is a different approach than v1 in that it adds a new driver which will
> > > > simply forward setting the GPIO output of specified GPIOs in OF node.
> > > > The ramp-up/ramp-down delay can now be actually defined on consumer side,
> > > > see Patch 1 or 3 for examples.
> > >
> > > I really like this approach, it looks better than I imagined.
> >
> > It seems over-engineered to me. So far no comments on my 3 suggestions either...
>
> I like the idea of handling this on the consumer's side, possibly with
> standard foo-gpios-ramp-{up,down}-delay-us (name to be bikeshedded)
> properties as you mentioned in the review of v1.
>
> > One is to just use some GPIO flag bits. Say 4-bits of GPIO flags
> > encoded as power of 2 ramp delay. We have to pick the units. For
> > example, 100us*2^N, which gives you 200us-3.2s of delay.
>
> This could probably work too.
>
> > Anything less is short enough to just hard code in a driver.
>
> In which driver though ? The whole point is that we should avoid
> handling this in particular drivers.

Okay, make the range 100us-1.63s and the minimum delay is 100us. Or
50us-819ms? What's a small enough minimum that no one will care about
the extra delay?

One thing we don't want is DT authors putting a device's delay needs
in here. Then we'll get coupling to the OS implementation or double
delays. Something like this should be clear:

#define GPIO_THIS_IS_ONLY_THE_SIGNAL_RC_RAMP_TIME_100us

;)

Rob

Re: [RFC PATCH v2 0/3] gpio: Add gpio-delay support

[Alexander Stein]

Hi all,

thanks for your comments.

Am Donnerstag, 15. Dezember 2022, 23:44:19 CET schrieb Rob Herring:
> On Thu, Dec 15, 2022 at 3:26 PM Laurent Pinchart
> 
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Thu, Dec 15, 2022 at 12:21:33PM -0600, Rob Herring wrote:
> > > On Thu, Dec 15, 2022 at 7:16 AM Linus Walleij wrote:
> > > > On Wed, Dec 14, 2022 at 10:53 AM Alexander Stein wrote:
> > > > > thanks for the feedback I've received. This is the reworked RFC for
> > > > > adressing a platform specific ramp-up/ramp-down delay on GPIO
> > > > > outputs.
> > > > > Now the delays are neither specified as gpio-controller nor
> > > > > consumer-specific properties.
> > > > > 
> > > > > v2 is a different approach than v1 in that it adds a new driver
> > > > > which will
> > > > > simply forward setting the GPIO output of specified GPIOs in OF
> > > > > node.
> > > > > The ramp-up/ramp-down delay can now be actually defined on consumer
> > > > > side,
> > > > > see Patch 1 or 3 for examples.
> > > > 
> > > > I really like this approach, it looks better than I imagined.
> > > 
> > > It seems over-engineered to me. So far no comments on my 3 suggestions
> > > either...> 
> > I like the idea of handling this on the consumer's side, possibly with
> > standard foo-gpios-ramp-{up,down}-delay-us (name to be bikeshedded)
> > properties as you mentioned in the review of v1.

Rob mentioned 4 possible delays: pre and post ramp up and down.
Is there a need for a pre ramp delay, ever? If there is need to wait until a 
GPIO can be switched this seems highly device specific to me.
Also reading back the requested output level on the GPIO is not possible in 
every case. Looking at the example in Patch 1 you can only read back the state 
of VCC_A, but the actual delay happens on VCC_B.

It might seem over-engineered, but I'm getting more and more inclined to this 
v2 approach. Having an explicit delay node, its obvious there is some 
dedicated circuit inducing this delay. But it is not caused by the GPIO 
controller nor by the consumer (LVDS Bridge in this case), but something 
passive in between.

Considering the hypothetical case there is a configurable IC instead, inducing 
this delay as well. The DT setup would look similar, but having a "regular" 
device instead of "gpio-delay" virtual device.

> > > One is to just use some GPIO flag bits. Say 4-bits of GPIO flags
> > > encoded as power of 2 ramp delay. We have to pick the units. For
> > > example, 100us*2^N, which gives you 200us-3.2s of delay.
> > 
> > This could probably work too.
> > 
> > > Anything less is short enough to just hard code in a driver.
> > 
> > In which driver though ? The whole point is that we should avoid
> > handling this in particular drivers.
> 
> Okay, make the range 100us-1.63s and the minimum delay is 100us. Or
> 50us-819ms? What's a small enough minimum that no one will care about
> the extra delay?

Is there a definite answer to this at all? Realtime (RT_PREMPT) people might 
have a different answer to these ranges. But I'm not really fond of using a 
bitmask in GPIO flags.

> One thing we don't want is DT authors putting a device's delay needs
> in here. Then we'll get coupling to the OS implementation or double
> delays.

Can you actually avoid that? There is no difference of behavior in software if 
you have
a) waiting/locking/... time once device is enabled, with an immediate ramp up
b) ramp up time until device is enabled, but it can be used immediately

In both cases you enable the GPIO and you have to wait for some specific time. 
But the reasoning for waiting are different. You can "solve" both cases on two 
ways:
1. device specific, configurable/hard-coded enable delays
2. general GPIO switch delays (this series)

I'm not sure if a property 'foo-gpios-ramp-us' on the consumer side is prone 
to hide the fact this delay is not actually related to the consumer.

Maybe it's even better to specify the delay in the "gpio-delay" consumer node.
Resulting in an example like this:

gpio_delay: gpio-delay {
	compatible = "gpio-delay";
	#gpio-cells = <1>;
	gpio-controller;
	gpios = <&gpio0 3 GPIO_ACTIVE_LOW>,
	        <&gpio3 1 GPIO_ACTIVE_HIGH>;
	gpios-ramp-us = <56000 0>, <130000 30000>;
};

consumer {
	enable-gpios = <&gpio_delay 0>;
};

Best regards,
Alexander

> Something like this should be clear:
> 
> #define GPIO_THIS_IS_ONLY_THE_SIGNAL_RC_RAMP_TIME_100us
> 
> ;)
> 
> Rob