[RFC PATCH 0/3] gpiolib: ramp-up delay support

[RFC PATCH 0/3] gpiolib: ramp-up delay support

[Alexander Stein]

Hi all,

this series is an RFC for a general approach to solve the issue at [1]. While
a device specific property works as well, a more generic approach is preferred.
In short: When enabling a GPIO the actual ramp-up time might be (much) bigger
than what software usually assume, in my case >100ms. Adding a delay to each
driver is cumbersome.
Instead the (optional) ramp-up delay is added to each gpio_desc. The delays can
be specified per gpio-controller, similar to 'gpio-line-names'. Actually the
parsing code is almost a 1:1 copy of devprop_gpiochip_set_names(). Due to
(temporary) memory allocation, I opted for a separate function, there is code
duplication, but handling both properties in a single function seemed too
tedious, let alone the to be added ramp-down delays.

This feature could also be added as a callback in gpio_chip, but the callbacks
have to be added to each driver then. I would prefer a single one-fits-all
implementation and another indirection in the GPIO call chain.

Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT
complexity unnecessarily. But comments are welcome.

The following 3 patches are a proof-of-concept on my platform, consisting of:
Patch 1 is the proposed bindings and straight forward.
Patch 2 is the current implementation
Patch 3 is an actual usage example for specifying the delays

TODO:
1. Adding ramp-down delays (Just the inverse copy of ramp-up delay)
2. Should these delays take active low flags into account?
3. How to deal with setting multiple GPIOs at once?

I skipped 1. for now, because this is just a copy with ramp-up being replaced
with ramp-down.

I'm not that well versed in gpiolib code, so I'm not sure if I got all placed
where GPIOs are set. So patch 2 might be incomplete.

For now I skipped setting multiple GPIOs at once completely, so to get some
feedback on this approach. A possible solution is to check for the bigest delay
in the set and use that for all afterwards. But I'm not sure about the overhead
in this case.

I hope there is some feedback. While thinking about this issue appears to be
more widespread than I expected.

Best regards,
Alexander

[1] https://lore.kernel.org/all/20221209083339.3780776-1-alexander.stein@ew.tq-group.com/

Alexander Stein (3):
  dt-bindings: gpio: Add optional ramp-up delay property
  gpiolib: Add support for optional ramp-up delays
  arm64: dts: mba8mx: Add GPIO ramp-up delays

 .../devicetree/bindings/gpio/gpio.txt         | 22 +++++
 arch/arm64/boot/dts/freescale/mba8mx.dtsi     |  5 ++
 drivers/gpio/gpiolib.c                        | 80 +++++++++++++++++++
 drivers/gpio/gpiolib.h                        |  3 +
 4 files changed, 110 insertions(+)

-- 
2.34.1

[RFC PATCH 1/3] dt-bindings: gpio: Add optional ramp-up delay property

[Alexander Stein]

This adds a ramp-up delay (in us) for adding a delay after enabling
GPIO. This is necessary if the ramp-up time is increased by some external
components. Usually this is quite fast, but certain combinations can
increase this to grater than 100ms.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 .../devicetree/bindings/gpio/gpio.txt         | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/gpio.txt b/Documentation/devicetree/bindings/gpio/gpio.txt
index 5663e71b751fc..b87669dce9a61 100644
--- a/Documentation/devicetree/bindings/gpio/gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/gpio.txt
@@ -182,6 +182,28 @@ gpio-controller@00000000 {
 		"poweroff", "reset";
 }
 
+Optionally, a GPIO controller may have a "gpio-ramp-up-delays-us" property.
+This is an array of u32 defining the ramp-up delays of the GPIO lines
+going out of the GPIO controller. This might be necessary if external
+components (e.g. capacitors, resistors) increase the ramp up time notably.
+The delay are assigned starting from line offset 0 from
+left to right from the passed array. An incomplete array (where the number
+of passed delays are less than ngpios) will still be used up until the last
+provided valid line index. Setting to will not add any delay.
+
+Example:
+
+gpio-controller@00000000 {
+	compatible = "foo";
+	reg = <0x00000000 0x1000>;
+	gpio-controller;
+	#gpio-cells = <2>;
+	gpio-ramp-up-delays-us = <0>, <0>, <0>, <0>,
+				 <0>, <0>, <120000>, <0>,
+				 <0>, <0>, <0>, <0>,
+				 <0>, <0>, <0>, <0>;
+}
+
 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.
-- 
2.34.1

[RFC PATCH 2/3] gpiolib: Add support for optional ramp-up delays

[Alexander Stein]

These delays are added when specified per GPIO line and GPIO is set
to high level, including any active low flag.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/gpio/gpiolib.c | 80 ++++++++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.h |  3 ++
 2 files changed, 83 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 5a66d9616d7cc..5848caf0b1e12 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -20,6 +20,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/fs.h>
 #include <linux/compat.h>
+#include <linux/delay.h>
 #include <linux/file.h>
 #include <uapi/linux/gpio.h>
 
@@ -432,6 +433,73 @@ static int devprop_gpiochip_set_names(struct gpio_chip *chip)
 	return 0;
 }
 
+/*
+ * devprop_gpiochip_set_delays - Set GPIO line delays using device properties
+ * @chip: GPIO chip whose delays should be set, if possible
+ *
+ * Looks for device property "gpio-ramp-up-delays-us" and if it exists assigns
+ * GPIO delays for the chip.
+ */
+static int devprop_gpiochip_set_delays(struct gpio_chip *chip)
+{
+	struct gpio_device *gdev = chip->gpiodev;
+	struct device *dev = &gdev->dev;
+	u32 *delays;
+	int ret, i;
+	int count;
+
+	count = device_property_count_u32(dev, "gpio-ramp-up-delays-us");
+	if (count < 0)
+		return 0;
+
+	/*
+	 * When offset is set in the driver side we assume the driver internally
+	 * is using more than one gpiochip per the same device. We have to stop
+	 * setting delays if the specified ones with 'gpio-ramp-up-delays-us'
+	 * are less than the offset in the device itself. This means all the
+	 * lines are not present for every single pin within all the internal
+	 * gpiochips.
+	 */
+	if (count <= chip->offset) {
+		dev_warn(dev, "gpio-ramp-up-delays-us too short (length %d), cannot map delays for the gpiochip at offset %u\n",
+			 count, chip->offset);
+		return 0;
+	}
+
+	delays = kcalloc(count, sizeof(*delays), GFP_KERNEL);
+	if (!delays)
+		return -ENOMEM;
+
+	ret = device_property_read_u32_array(dev, "gpio-ramp-up-delays-us",
+					     delays, count);
+	if (ret < 0) {
+		dev_warn(dev, "failed to read GPIO rampup delays: %d\n", ret);
+		kfree(delays);
+		return ret;
+	}
+
+	/*
+	 * When more than one gpiochip per device is used, 'count' can
+	 * contain at most number gpiochips x chip->ngpio. We have to
+	 * correctly distribute all defined lines taking into account
+	 * chip->offset as starting point from where we will assign
+	 * the delays to pins from the 'delays' array. Since property
+	 * 'gpio-ramp-up-delays-us' cannot contains gaps, we have to be sure
+	 * we only assign those pins that really exists since chip->ngpio
+	 * can be different of the chip->offset.
+	 */
+	count = (count > chip->offset) ? count - chip->offset : count;
+	if (count > chip->ngpio)
+		count = chip->ngpio;
+
+	for (i = 0; i < count; i++)
+		gdev->descs[i].ramp_up_delay_us = delays[chip->offset + i];
+
+	kfree(delays);
+
+	return 0;
+}
+
 static unsigned long *gpiochip_allocate_mask(struct gpio_chip *gc)
 {
 	unsigned long *p;
@@ -806,6 +874,9 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
 			goto err_remove_from_list;
 	}
 	ret = devprop_gpiochip_set_names(gc);
+	if (ret)
+		goto err_remove_from_list;
+	ret = devprop_gpiochip_set_delays(gc);
 	if (ret)
 		goto err_remove_from_list;
 
@@ -2962,6 +3033,9 @@ static void gpio_set_open_drain_value_commit(struct gpio_desc *desc, bool value)
 		gpiod_err(desc,
 			  "%s: Error in set_value for open drain err %d\n",
 			  __func__, ret);
+
+	if (desc->ramp_up_delay_us && value)
+		fsleep(desc->ramp_up_delay_us);
 }
 
 /*
@@ -2987,6 +3061,9 @@ static void gpio_set_open_source_value_commit(struct gpio_desc *desc, bool value
 		gpiod_err(desc,
 			  "%s: Error in set_value for open source err %d\n",
 			  __func__, ret);
+
+	if (desc->ramp_up_delay_us && value)
+		fsleep(desc->ramp_up_delay_us);
 }
 
 static void gpiod_set_raw_value_commit(struct gpio_desc *desc, bool value)
@@ -2996,6 +3073,9 @@ static void gpiod_set_raw_value_commit(struct gpio_desc *desc, bool value)
 	gc = desc->gdev->chip;
 	trace_gpio_value(desc_to_gpio(desc), 0, value);
 	gc->set(gc, gpio_chip_hwgpio(desc), value);
+
+	if (desc->ramp_up_delay_us && value)
+		fsleep(desc->ramp_up_delay_us);
 }
 
 /*
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index b3c2db6eba80c..3d7e5781e00d2 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -143,6 +143,7 @@ extern struct list_head gpio_devices;
  * @name:		Line name
  * @hog:		Pointer to the device node that hogs this line (if any)
  * @debounce_period_us:	Debounce period in microseconds
+ * @ramp_up_delay_us:	Enable propagation delay in microseconds
  *
  * These are obtained using gpiod_get() and are preferable to the old
  * integer-based handles.
@@ -184,6 +185,8 @@ struct gpio_desc {
 	/* debounce period in microseconds */
 	unsigned int		debounce_period_us;
 #endif
+	/* enable propagation delay in microseconds */
+	unsigned int		ramp_up_delay_us;
 };
 
 #define gpiod_not_found(desc)		(IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
-- 
2.34.1

[RFC PATCH 3/3] arm64: dts: mba8mx: Add GPIO ramp-up delays

[Alexander Stein]

GPIO6 on expander0 has a ramp-up time of ~113ms, so add a 120ms delay

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

diff --git a/arch/arm64/boot/dts/freescale/mba8mx.dtsi b/arch/arm64/boot/dts/freescale/mba8mx.dtsi
index cac8a44cba604..b5de77acc3f9c 100644
--- a/arch/arm64/boot/dts/freescale/mba8mx.dtsi
+++ b/arch/arm64/boot/dts/freescale/mba8mx.dtsi
@@ -192,6 +192,11 @@ expander0: gpio@23 {
 		interrupt-controller;
 		#interrupt-cells = <2>;
 
+		gpio-ramp-up-delays-us = <0>, <0>, <0>, <0>,
+					 <0>, <0>, <120000>, <0>,
+					 <0>, <0>, <0>, <0>,
+					 <0>, <0>, <0>, <0>;
+
 		sd-mux-oe-hog {
 			gpio-hog;
 			gpios = <8 0>;
-- 
2.34.1

Re: [RFC PATCH 0/3] gpiolib: ramp-up delay support

[Laurent Pinchart]

Hi Alexander,

On Mon, Dec 12, 2022 at 11:35:22AM +0100, Alexander Stein wrote:
> Hi all,
> 
> this series is an RFC for a general approach to solve the issue at [1]. While

I'm impressed by how fast you came up with a solution :-)

> a device specific property works as well, a more generic approach is preferred.
> In short: When enabling a GPIO the actual ramp-up time might be (much) bigger
> than what software usually assume, in my case >100ms. Adding a delay to each
> driver is cumbersome.
> Instead the (optional) ramp-up delay is added to each gpio_desc. The delays can
> be specified per gpio-controller, similar to 'gpio-line-names'. Actually the
> parsing code is almost a 1:1 copy of devprop_gpiochip_set_names().

While I like consistency, I wonder if it wouldn't be better in this case
to use a list of <gpio-number delay> cells in gpio-ramp-up-delays-us. In
typical use cases, very few GPIOs will need a delay, and a GPIO
controller could support a very large number of GPIOs, which would make
your current proposal cumbersome.

> Due to
> (temporary) memory allocation, I opted for a separate function, there is code
> duplication, but handling both properties in a single function seemed too
> tedious, let alone the to be added ramp-down delays.
> 
> This feature could also be added as a callback in gpio_chip, but the callbacks
> have to be added to each driver then. I would prefer a single one-fits-all
> implementation and another indirection in the GPIO call chain.

Agreed.

> Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT
> complexity unnecessarily. But comments are welcome.

It's an alternative approach that could be considered if this one is
rejected, but I have a preference for your solution.

> The following 3 patches are a proof-of-concept on my platform, consisting of:
> Patch 1 is the proposed bindings and straight forward.
> Patch 2 is the current implementation
> Patch 3 is an actual usage example for specifying the delays
> 
> TODO:
> 1. Adding ramp-down delays (Just the inverse copy of ramp-up delay)
> 2. Should these delays take active low flags into account?

How so ?

> 3. How to deal with setting multiple GPIOs at once?
> 
> I skipped 1. for now, because this is just a copy with ramp-up being replaced
> with ramp-down.
> 
> I'm not that well versed in gpiolib code, so I'm not sure if I got all placed
> where GPIOs are set. So patch 2 might be incomplete.
> 
> For now I skipped setting multiple GPIOs at once completely, so to get some
> feedback on this approach. A possible solution is to check for the bigest delay
> in the set and use that for all afterwards. But I'm not sure about the overhead
> in this case.

I assume you're talking about the gpiod_set_array_value() API. That
sounds OK as an initial implementation, a caller of that function needs
to be prepared for the GPIOs being set in a random order due to hardware
delays, so it shouldn't break the API contract. I would however state
this explicitly in the function documentation.

> I hope there is some feedback. While thinking about this issue appears to be
> more widespread than I expected.
> 
> Best regards,
> Alexander
> 
> [1] https://lore.kernel.org/all/20221209083339.3780776-1-alexander.stein@ew.tq-group.com/
> 
> Alexander Stein (3):
>   dt-bindings: gpio: Add optional ramp-up delay property
>   gpiolib: Add support for optional ramp-up delays
>   arm64: dts: mba8mx: Add GPIO ramp-up delays
> 
>  .../devicetree/bindings/gpio/gpio.txt         | 22 +++++
>  arch/arm64/boot/dts/freescale/mba8mx.dtsi     |  5 ++
>  drivers/gpio/gpiolib.c                        | 80 +++++++++++++++++++
>  drivers/gpio/gpiolib.h                        |  3 +
>  4 files changed, 110 insertions(+)

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/3] gpiolib: ramp-up delay support

[Alexander Stein]

Hello Laurent,

thanks for your fast comments.

Am Montag, 12. Dezember 2022, 13:40:50 CET schrieb Laurent Pinchart:
> Hi Alexander,
> 
> On Mon, Dec 12, 2022 at 11:35:22AM +0100, Alexander Stein wrote:
> > Hi all,
> > 
> > this series is an RFC for a general approach to solve the issue at [1].
> > While
> I'm impressed by how fast you came up with a solution :-)
> 
> > a device specific property works as well, a more generic approach is
> > preferred. In short: When enabling a GPIO the actual ramp-up time might
> > be (much) bigger than what software usually assume, in my case >100ms.
> > Adding a delay to each driver is cumbersome.
> > Instead the (optional) ramp-up delay is added to each gpio_desc. The
> > delays can be specified per gpio-controller, similar to
> > 'gpio-line-names'. Actually the parsing code is almost a 1:1 copy of
> > devprop_gpiochip_set_names().
> While I like consistency, I wonder if it wouldn't be better in this case
> to use a list of <gpio-number delay> cells in gpio-ramp-up-delays-us. In
> typical use cases, very few GPIOs will need a delay, and a GPIO
> controller could support a very large number of GPIOs, which would make
> your current proposal cumbersome.

That's a good idea. I would even go a step further to specify both ramp-up and 
ramp-down in one cell, e.g. <gpio-number ramp-up ramp-down>. This way a second 
property is not needed.

> > Due to
> > (temporary) memory allocation, I opted for a separate function, there is
> > code duplication, but handling both properties in a single function
> > seemed too tedious, let alone the to be added ramp-down delays.
> > 
> > This feature could also be added as a callback in gpio_chip, but the
> > callbacks have to be added to each driver then. I would prefer a single
> > one-fits-all implementation and another indirection in the GPIO call
> > chain.
> 
> Agreed.
> 
> > Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT
> > complexity unnecessarily. But comments are welcome.
> 
> It's an alternative approach that could be considered if this one is
> rejected, but I have a preference for your solution.
> 
> > The following 3 patches are a proof-of-concept on my platform, consisting
> > of: Patch 1 is the proposed bindings and straight forward.
> > Patch 2 is the current implementation
> > Patch 3 is an actual usage example for specifying the delays
> > 
> > TODO:
> > 1. Adding ramp-down delays (Just the inverse copy of ramp-up delay)
> > 2. Should these delays take active low flags into account?
> 
> How so ?

Given the name ramp-up (& ramp-down) I would assume they affect the voltage 
low -> high change (resp. high -> low), not just gpiod_set_value(..., 1).

> > 3. How to deal with setting multiple GPIOs at once?
> > 
> > I skipped 1. for now, because this is just a copy with ramp-up being
> > replaced with ramp-down.
> > 
> > I'm not that well versed in gpiolib code, so I'm not sure if I got all
> > placed where GPIOs are set. So patch 2 might be incomplete.
> > 
> > For now I skipped setting multiple GPIOs at once completely, so to get
> > some
> > feedback on this approach. A possible solution is to check for the bigest
> > delay in the set and use that for all afterwards. But I'm not sure about
> > the overhead in this case.
> 
> I assume you're talking about the gpiod_set_array_value() API. That
> sounds OK as an initial implementation, a caller of that function needs
> to be prepared for the GPIOs being set in a random order due to hardware
> delays, so it shouldn't break the API contract. I would however state
> this explicitly in the function documentation.

Okay, that seems sensible. Will do it.

Best regards,
Alexander

> > I hope there is some feedback. While thinking about this issue appears to
> > be more widespread than I expected.
> > 
> > Best regards,
> > Alexander
> > 
> > [1]
> > https://lore.kernel.org/all/20221209083339.3780776-1-alexander.stein@ew.t
> > q-group.com/> 
> > Alexander Stein (3):
> >   dt-bindings: gpio: Add optional ramp-up delay property
> >   gpiolib: Add support for optional ramp-up delays
> >   arm64: dts: mba8mx: Add GPIO ramp-up delays
> >  
> >  .../devicetree/bindings/gpio/gpio.txt         | 22 +++++
> >  arch/arm64/boot/dts/freescale/mba8mx.dtsi     |  5 ++
> >  drivers/gpio/gpiolib.c                        | 80 +++++++++++++++++++
> >  drivers/gpio/gpiolib.h                        |  3 +
> >  4 files changed, 110 insertions(+)

Re: [RFC PATCH 1/3] dt-bindings: gpio: Add optional ramp-up delay property

[Linus Walleij]

On Mon, Dec 12, 2022 at 11:35 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:

> This adds a ramp-up delay (in us) for adding a delay after enabling
> GPIO. This is necessary if the ramp-up time is increased by some external
> components. Usually this is quite fast, but certain combinations can
> increase this to grater than 100ms.
>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
(...)
> +gpio-controller@00000000 {
> +       compatible = "foo";
> +       reg = <0x00000000 0x1000>;
> +       gpio-controller;
> +       #gpio-cells = <2>;
> +       gpio-ramp-up-delays-us = <0>, <0>, <0>, <0>,
> +                                <0>, <0>, <120000>, <0>,
> +                                <0>, <0>, <0>, <0>,
> +                                <0>, <0>, <0>, <0>;

Why would this be different per-gpio-line?

If this should be on the GPIO controller, this should be the ramp-up for the
GPIO controller output itself, not for the random electronics that may or may
not be attached to the line.

Otherwise the ramp-up should certainly be on the consumer side. And that
seems very much like what is going on here.

Consider a gpio-regulator:


Documentation/devicetree/bindings/regulator/fixed-regulator.yamlproperties:
  compatible:
    enum:
      - regulator-fixed
      - regulator-fixed-clock
      - regulator-fixed-domain

  regulator-name: true

  gpio:
    description: gpio to use for enable control
    maxItems: 1
(...)
  startup-delay-us:
    description: startup time in microseconds

  off-on-delay-us:
    description: off delay time in microseconds


There is one consumer, and if you add ramp-up and ramp-down delays to the
GPIO lines like this you have just created two ways of doing the same thing.
When there is a ramp-up for a regulator now the used can choose to put it
on the regulator or on the gpio.

This is clearly ambiguous so NAK to this approach. IMO the property goes
on the consumer due to precedence.

[Other context]
> Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT
> complexity unnecessarily. But comments are welcome.

If the consumer subsystem don't want it, I guess this is where you would
have to go in and add more DT descriptions for the electronics on the
board, which I understand is a bit frustrating, and it is hard to find the
right trade-off. It makes me think about the classical problem "how long is
the coast of Britain?" by Benoit Mandelbrot:
https://en.wikipedia.org/wiki/How_Long_Is_the_Coast_of_Britain%3F_Statistical_Self-Similarity_and_Fractional_Dimension

The DT maintainers will have the final word on it I guess.

Yours,
Linus Walleij

Re: [RFC PATCH 2/3] gpiolib: Add support for optional ramp-up delays

[Linus Walleij]

On Mon, Dec 12, 2022 at 11:35 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:

> These delays are added when specified per GPIO line and GPIO is set
> to high level, including any active low flag.
>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

As stated in the binding patch this can be considered as a global ramp-up
for the entire chip, certainly there are not different driver stages on the
different outputs (in that case it would be a pin control driver BTW).

This is created to swipe the details of random board electronics under the
carpet and this doesn't work because we already have consumers
such as regulators specifying ramp-up on the consumer side.

Yours,
Linus Walleij

Re: [RFC PATCH 0/3] gpiolib: ramp-up delay support

[Laurent Pinchart]

Hi Alexander,

On Tue, Dec 13, 2022 at 08:49:06AM +0100, Alexander Stein wrote:
> Am Montag, 12. Dezember 2022, 13:40:50 CET schrieb Laurent Pinchart:
> > On Mon, Dec 12, 2022 at 11:35:22AM +0100, Alexander Stein wrote:
> > > Hi all,
> > > 
> > > this series is an RFC for a general approach to solve the issue at [1].
> > > While
> >
> > I'm impressed by how fast you came up with a solution :-)
> > 
> > > a device specific property works as well, a more generic approach is
> > > preferred. In short: When enabling a GPIO the actual ramp-up time might
> > > be (much) bigger than what software usually assume, in my case >100ms.
> > > Adding a delay to each driver is cumbersome.
> > > Instead the (optional) ramp-up delay is added to each gpio_desc. The
> > > delays can be specified per gpio-controller, similar to
> > > 'gpio-line-names'. Actually the parsing code is almost a 1:1 copy of
> > > devprop_gpiochip_set_names().
> >
> > While I like consistency, I wonder if it wouldn't be better in this case
> > to use a list of <gpio-number delay> cells in gpio-ramp-up-delays-us. In
> > typical use cases, very few GPIOs will need a delay, and a GPIO
> > controller could support a very large number of GPIOs, which would make
> > your current proposal cumbersome.
> 
> That's a good idea. I would even go a step further to specify both ramp-up and 
> ramp-down in one cell, e.g. <gpio-number ramp-up ramp-down>. This way a second 
> property is not needed.
> 
> > > Due to
> > > (temporary) memory allocation, I opted for a separate function, there is
> > > code duplication, but handling both properties in a single function
> > > seemed too tedious, let alone the to be added ramp-down delays.
> > > 
> > > This feature could also be added as a callback in gpio_chip, but the
> > > callbacks have to be added to each driver then. I would prefer a single
> > > one-fits-all implementation and another indirection in the GPIO call
> > > chain.
> > 
> > Agreed.
> > 
> > > Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT
> > > complexity unnecessarily. But comments are welcome.
> > 
> > It's an alternative approach that could be considered if this one is
> > rejected, but I have a preference for your solution.
> > 
> > > The following 3 patches are a proof-of-concept on my platform, consisting
> > > of: Patch 1 is the proposed bindings and straight forward.
> > > Patch 2 is the current implementation
> > > Patch 3 is an actual usage example for specifying the delays
> > > 
> > > TODO:
> > > 1. Adding ramp-down delays (Just the inverse copy of ramp-up delay)
> > > 2. Should these delays take active low flags into account?
> > 
> > How so ?
> 
> Given the name ramp-up (& ramp-down) I would assume they affect the voltage 
> low -> high change (resp. high -> low), not just gpiod_set_value(..., 1).

Good point. They should indeed.

> > > 3. How to deal with setting multiple GPIOs at once?
> > > 
> > > I skipped 1. for now, because this is just a copy with ramp-up being
> > > replaced with ramp-down.
> > > 
> > > I'm not that well versed in gpiolib code, so I'm not sure if I got all
> > > placed where GPIOs are set. So patch 2 might be incomplete.
> > > 
> > > For now I skipped setting multiple GPIOs at once completely, so to get some
> > > feedback on this approach. A possible solution is to check for the bigest
> > > delay in the set and use that for all afterwards. But I'm not sure about
> > > the overhead in this case.
> > 
> > I assume you're talking about the gpiod_set_array_value() API. That
> > sounds OK as an initial implementation, a caller of that function needs
> > to be prepared for the GPIOs being set in a random order due to hardware
> > delays, so it shouldn't break the API contract. I would however state
> > this explicitly in the function documentation.
> 
> Okay, that seems sensible. Will do it.
> 
> > > I hope there is some feedback. While thinking about this issue appears to
> > > be more widespread than I expected.
> > > 
> > > Best regards,
> > > Alexander
> > > 
> > > [1] https://lore.kernel.org/all/20221209083339.3780776-1-alexander.stein@ew.tq-group.com/
> > >
> > > Alexander Stein (3):
> > >   dt-bindings: gpio: Add optional ramp-up delay property
> > >   gpiolib: Add support for optional ramp-up delays
> > >   arm64: dts: mba8mx: Add GPIO ramp-up delays
> > >  
> > >  .../devicetree/bindings/gpio/gpio.txt         | 22 +++++
> > >  arch/arm64/boot/dts/freescale/mba8mx.dtsi     |  5 ++
> > >  drivers/gpio/gpiolib.c                        | 80 +++++++++++++++++++
> > >  drivers/gpio/gpiolib.h                        |  3 +
> > >  4 files changed, 110 insertions(+)

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/3] dt-bindings: gpio: Add optional ramp-up delay property

[Laurent Pinchart]

Hi Linus,

On Tue, Dec 13, 2022 at 10:08:19AM +0100, Linus Walleij wrote:
> On Mon, Dec 12, 2022 at 11:35 AM Alexander Stein wrote:
> 
> > This adds a ramp-up delay (in us) for adding a delay after enabling
> > GPIO. This is necessary if the ramp-up time is increased by some external
> > components. Usually this is quite fast, but certain combinations can
> > increase this to grater than 100ms.
> >
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> (...)
> > +gpio-controller@00000000 {
> > +       compatible = "foo";
> > +       reg = <0x00000000 0x1000>;
> > +       gpio-controller;
> > +       #gpio-cells = <2>;
> > +       gpio-ramp-up-delays-us = <0>, <0>, <0>, <0>,
> > +                                <0>, <0>, <120000>, <0>,
> > +                                <0>, <0>, <0>, <0>,
> > +                                <0>, <0>, <0>, <0>;
> 
> Why would this be different per-gpio-line?
> 
> If this should be on the GPIO controller, this should be the ramp-up for the
> GPIO controller output itself, not for the random electronics that may or may
> not be attached to the line.

The goal of this series is to cover the delay of the RC filter on the
signal between the GPIO controller and the GPIO consumer.

> Otherwise the ramp-up should certainly be on the consumer side. And that
> seems very much like what is going on here.

The circuit we're looking at is

  +----------+           +-----------+
  | SoC      |           |    VCC    |
  |          |           |     |     |
  |          |           |     _     |
  |          |           |    | | R  |
  |          |           |    |_|    |
  |          |           |     |     |
  |      [IOx|-----+-----|EN]--+     |
  |          |     |     |           |
  |          |     |     | SN65DSI83 |
  +----------+    --- C  +-----------+
                  ---
		   |
		   -
		  GND

The IOx pin is an open-drain output, the board has a 470nF capacitor to
ground, and the SN65DSI83 has an internal pull-up off 200kΩ. This gives
an RC time constant of 94ms, far from being negligible.

The delay is caused by the combination of the open-drain nature of the
output (an intrinsic property of the GPIO controller), the pull-up
resistor (an intrinsic property of the SN65DSI83) and the capacitor on
the line (a property of the board). DT is notoriously bad at modelling
this kind of setup.

Note that we would have a similar issue with a GPIO controller that
would drive the signal actively and a consumer without an internal
pull-up if the board had an RC filter on the line.

Alexander started with a patch that added a ti,enable-delay-us property
to the SN65DSI83 DT binding. I don't think that's a good idea, as the
issue is far from being specific to the SN65DSI83, patching all GPIO
consumers as needed to support signal propagation delays doesn't scale.
Patching every GPIO controller driver would be equally bad. I've thus
proposed modelling this with a standard property on the GPIO provider
side, handled by gpiolib, to have a centralized solution.

The alternative I proposed, adding a "GPIO delay" DT node to model this,
would also offer a centralized solution to the problem, but with
additional complexity both at probe time and runtime.

> Consider a gpio-regulator:
> 
> 
> Documentation/devicetree/bindings/regulator/fixed-regulator.yamlproperties:
>   compatible:
>     enum:
>       - regulator-fixed
>       - regulator-fixed-clock
>       - regulator-fixed-domain
> 
>   regulator-name: true
> 
>   gpio:
>     description: gpio to use for enable control
>     maxItems: 1
> (...)
>   startup-delay-us:
>     description: startup time in microseconds
> 
>   off-on-delay-us:
>     description: off delay time in microseconds
> 
> 
> There is one consumer, and if you add ramp-up and ramp-down delays to the
> GPIO lines like this you have just created two ways of doing the same thing.
> When there is a ramp-up for a regulator now the used can choose to put it
> on the regulator or on the gpio.

The regulator delays model the intrinsic delays when enabling or
disabling a regulator, and they should stay. They address a different
problem.

> This is clearly ambiguous so NAK to this approach. IMO the property goes
> on the consumer due to precedence.
> 
> [Other context]
> > Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT
> > complexity unnecessarily. But comments are welcome.
> 
> If the consumer subsystem don't want it, I guess this is where you would
> have to go in and add more DT descriptions for the electronics on the
> board, which I understand is a bit frustrating, and it is hard to find the
> right trade-off. It makes me think about the classical problem "how long is
> the coast of Britain?" by Benoit Mandelbrot:
> https://en.wikipedia.org/wiki/How_Long_Is_the_Coast_of_Britain%3F_Statistical_Self-Similarity_and_Fractional_Dimension
> 
> The DT maintainers will have the final word on it I guess.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/3] gpiolib: ramp-up delay support

[Rob Herring]

On Mon, Dec 12, 2022 at 4:35 AM Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hi all,
>
> this series is an RFC for a general approach to solve the issue at [1]. While
> a device specific property works as well, a more generic approach is preferred.
> In short: When enabling a GPIO the actual ramp-up time might be (much) bigger
> than what software usually assume, in my case >100ms. Adding a delay to each
> driver is cumbersome.

At least for DT, I think this belongs (if at all) in the consumers,
rather than a producer property. The options there are
'foo-gpios-ramp-us' for 'foo-gpios' or add some delay bits to GPIO
flags. We already have some of the former for various 'generic' power
sequencing related delays. Of course, there's no real pattern to them
as they all get added as we go without much foresight. In this case
even, there are 4 possible delays: pre and post ramp up and down.

> Instead the (optional) ramp-up delay is added to each gpio_desc. The delays can
> be specified per gpio-controller, similar to 'gpio-line-names'. Actually the
> parsing code is almost a 1:1 copy of devprop_gpiochip_set_names(). Due to
> (temporary) memory allocation, I opted for a separate function, there is code
> duplication, but handling both properties in a single function seemed too
> tedious, let alone the to be added ramp-down delays.
>
> This feature could also be added as a callback in gpio_chip, but the callbacks
> have to be added to each driver then. I would prefer a single one-fits-all
> implementation and another indirection in the GPIO call chain.
>
> Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT
> complexity unnecessarily. But comments are welcome.
>
> The following 3 patches are a proof-of-concept on my platform, consisting of:
> Patch 1 is the proposed bindings and straight forward.
> Patch 2 is the current implementation
> Patch 3 is an actual usage example for specifying the delays
>
> TODO:
> 1. Adding ramp-down delays (Just the inverse copy of ramp-up delay)
> 2. Should these delays take active low flags into account?
> 3. How to deal with setting multiple GPIOs at once?
>
> I skipped 1. for now, because this is just a copy with ramp-up being replaced
> with ramp-down.
>
> I'm not that well versed in gpiolib code, so I'm not sure if I got all placed
> where GPIOs are set. So patch 2 might be incomplete.
>
> For now I skipped setting multiple GPIOs at once completely, so to get some
> feedback on this approach. A possible solution is to check for the bigest delay
> in the set and use that for all afterwards. But I'm not sure about the overhead
> in this case.
>
> I hope there is some feedback. While thinking about this issue appears to be
> more widespread than I expected.

Many/most GPIO controllers can read the actual state of an output
(IIRC, i.MX ctrlr can). Perhaps that capability could be used to delay
until the state of the signal matches the set state. And you'd
probably want to measure how long that took and then add some more
time based on it. This of course gets into the electricals of at what
levels a low or high state will register. If you can't read the state,
then you would be stuck with some maximum timeout.

Rob

Re: [RFC PATCH 0/3] gpiolib: ramp-up delay support

[Laurent Pinchart]

Hi Rob,

On Tue, Dec 13, 2022 at 08:20:57AM -0600, Rob Herring wrote:
> On Mon, Dec 12, 2022 at 4:35 AM Alexander Stein wrote:
> >
> > Hi all,
> >
> > this series is an RFC for a general approach to solve the issue at [1]. While
> > a device specific property works as well, a more generic approach is preferred.
> > In short: When enabling a GPIO the actual ramp-up time might be (much) bigger
> > than what software usually assume, in my case >100ms. Adding a delay to each
> > driver is cumbersome.
> 
> At least for DT, I think this belongs (if at all) in the consumers,
> rather than a producer property. The options there are
> 'foo-gpios-ramp-us' for 'foo-gpios' or add some delay bits to GPIO
> flags.

My main requirement is to handle these properties in a central place,
without having to patch individual drivers, so this would work for me.

> We already have some of the former for various 'generic' power
> sequencing related delays. Of course, there's no real pattern to them
> as they all get added as we go without much foresight. In this case
> even, there are 4 possible delays: pre and post ramp up and down.
> 
> > Instead the (optional) ramp-up delay is added to each gpio_desc. The delays can
> > be specified per gpio-controller, similar to 'gpio-line-names'. Actually the
> > parsing code is almost a 1:1 copy of devprop_gpiochip_set_names(). Due to
> > (temporary) memory allocation, I opted for a separate function, there is code
> > duplication, but handling both properties in a single function seemed too
> > tedious, let alone the to be added ramp-down delays.
> >
> > This feature could also be added as a callback in gpio_chip, but the callbacks
> > have to be added to each driver then. I would prefer a single one-fits-all
> > implementation and another indirection in the GPIO call chain.
> >
> > Laurent suggest to add a GPIO delay node in DT. IMHO this increased the DT
> > complexity unnecessarily. But comments are welcome.
> >
> > The following 3 patches are a proof-of-concept on my platform, consisting of:
> > Patch 1 is the proposed bindings and straight forward.
> > Patch 2 is the current implementation
> > Patch 3 is an actual usage example for specifying the delays
> >
> > TODO:
> > 1. Adding ramp-down delays (Just the inverse copy of ramp-up delay)
> > 2. Should these delays take active low flags into account?
> > 3. How to deal with setting multiple GPIOs at once?
> >
> > I skipped 1. for now, because this is just a copy with ramp-up being replaced
> > with ramp-down.
> >
> > I'm not that well versed in gpiolib code, so I'm not sure if I got all placed
> > where GPIOs are set. So patch 2 might be incomplete.
> >
> > For now I skipped setting multiple GPIOs at once completely, so to get some
> > feedback on this approach. A possible solution is to check for the bigest delay
> > in the set and use that for all afterwards. But I'm not sure about the overhead
> > in this case.
> >
> > I hope there is some feedback. While thinking about this issue appears to be
> > more widespread than I expected.
> 
> Many/most GPIO controllers can read the actual state of an output
> (IIRC, i.MX ctrlr can). Perhaps that capability could be used to delay
> until the state of the signal matches the set state. And you'd
> probably want to measure how long that took and then add some more
> time based on it. This of course gets into the electricals of at what
> levels a low or high state will register. If you can't read the state,
> then you would be stuck with some maximum timeout.

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/3] dt-bindings: gpio: Add optional ramp-up delay property

[Linus Walleij]

Hi Laurent,

thanks for the detailed brief!

On Tue, Dec 13, 2022 at 12:45 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> The circuit we're looking at is
>
>   +----------+           +-----------+
>   | SoC      |           |    VCC    |
>   |          |           |     |     |
>   |          |           |     _     |
>   |          |           |    | | R  |
>   |          |           |    |_|    |
>   |          |           |     |     |
>   |      [IOx|-----+-----|EN]--+     |
>   |          |     |     |           |
>   |          |     |     | SN65DSI83 |
>   +----------+    --- C  +-----------+
>                   ---
>                    |
>                    -
>                   GND
>
> The IOx pin is an open-drain output, the board has a 470nF capacitor to
> ground, and the SN65DSI83 has an internal pull-up off 200kΩ. This gives
> an RC time constant of 94ms, far from being negligible.
>
> The delay is caused by the combination of the open-drain nature of the
> output (an intrinsic property of the GPIO controller), the pull-up
> resistor (an intrinsic property of the SN65DSI83) and the capacitor on
> the line (a property of the board). DT is notoriously bad at modelling
> this kind of setup.

Yeah :/

It's not like we don't model discrete electronics, we do that a lot,
but as you say, it is really hard to know where to draw the line
in cases like this.

> The alternative I proposed, adding a "GPIO delay" DT node to model this,
> would also offer a centralized solution to the problem, but with
> additional complexity both at probe time and runtime.

I have a slight preference for this, as it will be very explicit in the
device tree and we can just put all the code inside its own file and
depend on GPIO_OF so other HW description systems do not
need to include it.

At the same time it feels a bit overengineered, so maybe just adding
this delay as in the patch with some strings attached like comments
and docs is yet the best. It feels like we need some more input to
reach consensus.

> The regulator delays model the intrinsic delays when enabling or
> disabling a regulator, and they should stay. They address a different
> problem.

OK right. But someone not knowing exactly what they are doing
will end up abusing the delay property on the delay line
also for this delay. The risk of that is lesser with a separate
delay box.

Yours,
Linus Walleij

Re: [RFC PATCH 1/3] dt-bindings: gpio: Add optional ramp-up delay property

[Laurent Pinchart]

On Thu, Dec 15, 2022 at 11:56:57AM +0100, Linus Walleij wrote:
> Hi Laurent,
> 
> thanks for the detailed brief!
> 
> On Tue, Dec 13, 2022 at 12:45 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> 
> > The circuit we're looking at is
> >
> >   +----------+           +-----------+
> >   | SoC      |           |    VCC    |
> >   |          |           |     |     |
> >   |          |           |     _     |
> >   |          |           |    | | R  |
> >   |          |           |    |_|    |
> >   |          |           |     |     |
> >   |      [IOx|-----+-----|EN]--+     |
> >   |          |     |     |           |
> >   |          |     |     | SN65DSI83 |
> >   +----------+    --- C  +-----------+
> >                   ---
> >                    |
> >                    -
> >                   GND
> >
> > The IOx pin is an open-drain output, the board has a 470nF capacitor to
> > ground, and the SN65DSI83 has an internal pull-up off 200kΩ. This gives
> > an RC time constant of 94ms, far from being negligible.
> >
> > The delay is caused by the combination of the open-drain nature of the
> > output (an intrinsic property of the GPIO controller), the pull-up
> > resistor (an intrinsic property of the SN65DSI83) and the capacitor on
> > the line (a property of the board). DT is notoriously bad at modelling
> > this kind of setup.
> 
> Yeah :/
> 
> It's not like we don't model discrete electronics, we do that a lot,
> but as you say, it is really hard to know where to draw the line
> in cases like this.
> 
> > The alternative I proposed, adding a "GPIO delay" DT node to model this,
> > would also offer a centralized solution to the problem, but with
> > additional complexity both at probe time and runtime.
> 
> I have a slight preference for this, as it will be very explicit in the
> device tree and we can just put all the code inside its own file and
> depend on GPIO_OF so other HW description systems do not
> need to include it.
> 
> At the same time it feels a bit overengineered, so maybe just adding
> this delay as in the patch with some strings attached like comments
> and docs is yet the best. It feels like we need some more input to
> reach consensus.
> 
> > The regulator delays model the intrinsic delays when enabling or
> > disabling a regulator, and they should stay. They address a different
> > problem.
> 
> OK right. But someone not knowing exactly what they are doing
> will end up abusing the delay property on the delay line
> also for this delay. The risk of that is lesser with a separate
> delay box.

That may be true, but I think we can also try to catch abuses in
reviews. I would be a bit sad if we made life more difficult (and less
efficient at runtime too) for legitimate users just because we are
worried about abuses.

Another thing I've been thinking about is that we may not always want to
wait for the GPIO delay. Some consumers may not care when the GPIO line
reaches the desired state as long as it eventually does, or maybe they
need to perform multiple operations (such as enabling/disabling
regulators and/or clocks) and only need a synchronization point for a
group of operations. All that would be pretty hard to handle, and maybe
it's a problem we'll look at only when needed (and hopefully never).

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/3] dt-bindings: gpio: Add optional ramp-up delay property

[Alexander Stein]

Hi Laurent,

Am Dienstag, 20. Dezember 2022, 00:01:23 CET schrieb Laurent Pinchart:
> On Thu, Dec 15, 2022 at 11:56:57AM +0100, Linus Walleij wrote:
> > Hi Laurent,
> > 
> > thanks for the detailed brief!
> > 
> > On Tue, Dec 13, 2022 at 12:45 PM Laurent Pinchart
> > 
> > <laurent.pinchart@ideasonboard.com> wrote:
> > > The circuit we're looking at is
> > > 
> > >   +----------+           +-----------+
> > >   
> > >   | SoC      |           |    VCC    |
> > >   | 
> > >   |          |           |     _     |
> > >   |          |           |     
> > >   |          |           |    | | R  |
> > >   |          |           |    |
> > >   |          |           |    |_|    |
> > >   |      
> > >   |      [IOx|-----+-----|EN]--+     |
> > >   |      
> > >   |          |     |     | SN65DSI83 |
> > >   
> > >   +----------+    --- C  +-----------+
> > >   
> > >                   ---
> > >                   
> > >                    -
> > >                   
> > >                   GND
> > > 
> > > The IOx pin is an open-drain output, the board has a 470nF capacitor to
> > > ground, and the SN65DSI83 has an internal pull-up off 200kΩ. This gives
> > > an RC time constant of 94ms, far from being negligible.
> > > 
> > > The delay is caused by the combination of the open-drain nature of the
> > > output (an intrinsic property of the GPIO controller), the pull-up
> > > resistor (an intrinsic property of the SN65DSI83) and the capacitor on
> > > the line (a property of the board). DT is notoriously bad at modelling
> > > this kind of setup.
> > 
> > Yeah :/
> > 
> > It's not like we don't model discrete electronics, we do that a lot,
> > but as you say, it is really hard to know where to draw the line
> > in cases like this.
> > 
> > > The alternative I proposed, adding a "GPIO delay" DT node to model this,
> > > would also offer a centralized solution to the problem, but with
> > > additional complexity both at probe time and runtime.
> > 
> > I have a slight preference for this, as it will be very explicit in the
> > device tree and we can just put all the code inside its own file and
> > depend on GPIO_OF so other HW description systems do not
> > need to include it.
> > 
> > At the same time it feels a bit overengineered, so maybe just adding
> > this delay as in the patch with some strings attached like comments
> > and docs is yet the best. It feels like we need some more input to
> > reach consensus.
> > 
> > > The regulator delays model the intrinsic delays when enabling or
> > > disabling a regulator, and they should stay. They address a different
> > > problem.
> > 
> > OK right. But someone not knowing exactly what they are doing
> > will end up abusing the delay property on the delay line
> > also for this delay. The risk of that is lesser with a separate
> > delay box.
> 
> That may be true, but I think we can also try to catch abuses in
> reviews. I would be a bit sad if we made life more difficult (and less
> efficient at runtime too) for legitimate users just because we are
> worried about abuses.

What is a legitimate user for you? Given the example in v2 of this series it's 
clear that this feature is an opt-in, both for the DT node as well as for 
specifying a delay.
Another benefit of using a dedicated driver: It also automatically handles 
things like setting multiple GPIOs at once.

> Another thing I've been thinking about is that we may not always want to
> wait for the GPIO delay. Some consumers may not care when the GPIO line
> reaches the desired state as long as it eventually does, or maybe they
> need to perform multiple operations (such as enabling/disabling
> regulators and/or clocks) and only need a synchronization point for a
> group of operations. All that would be pretty hard to handle, and maybe
> it's a problem we'll look at only when needed (and hopefully never).

If you don't care about rising time, do not use gpio-delay for that GPIO, or 
just don't specify a ramp-up delay in the gpio-cells, aka setting to 0.
The more complex synchronisation example you mentioned probably needs a 
similar dedicated driver for grouping those resources.

Best regards,
Alexander

Re: [RFC PATCH 1/3] dt-bindings: gpio: Add optional ramp-up delay property

[Laurent Pinchart]

On Tue, Jan 03, 2023 at 12:56:38PM +0100, Alexander Stein wrote:
> Am Dienstag, 20. Dezember 2022, 00:01:23 CET schrieb Laurent Pinchart:
> > On Thu, Dec 15, 2022 at 11:56:57AM +0100, Linus Walleij wrote:
> > > On Tue, Dec 13, 2022 at 12:45 PM Laurent Pinchart wrote:
> > > > The circuit we're looking at is
> > > > 
> > > >   +----------+           +-----------+
> > > >   | SoC      |           |    VCC    |
> > > >   |          |           |     _     |
> > > >   |          |           |    | | R  |
> > > >   |          |           |    |_|    |
> > > >   |          |           |     |     |
> > > >   |      [IOx|-----+-----|EN]--+     |
> > > >   |          |     |     | SN65DSI83 |
> > > >   +----------+    --- C  +-----------+
> > > >                   ---
> > > >                    -
> > > >                   GND
> > > > 
> > > > The IOx pin is an open-drain output, the board has a 470nF capacitor to
> > > > ground, and the SN65DSI83 has an internal pull-up off 200kΩ. This gives
> > > > an RC time constant of 94ms, far from being negligible.
> > > > 
> > > > The delay is caused by the combination of the open-drain nature of the
> > > > output (an intrinsic property of the GPIO controller), the pull-up
> > > > resistor (an intrinsic property of the SN65DSI83) and the capacitor on
> > > > the line (a property of the board). DT is notoriously bad at modelling
> > > > this kind of setup.
> > > 
> > > Yeah :/
> > > 
> > > It's not like we don't model discrete electronics, we do that a lot,
> > > but as you say, it is really hard to know where to draw the line
> > > in cases like this.
> > > 
> > > > The alternative I proposed, adding a "GPIO delay" DT node to model this,
> > > > would also offer a centralized solution to the problem, but with
> > > > additional complexity both at probe time and runtime.
> > > 
> > > I have a slight preference for this, as it will be very explicit in the
> > > device tree and we can just put all the code inside its own file and
> > > depend on GPIO_OF so other HW description systems do not
> > > need to include it.
> > > 
> > > At the same time it feels a bit overengineered, so maybe just adding
> > > this delay as in the patch with some strings attached like comments
> > > and docs is yet the best. It feels like we need some more input to
> > > reach consensus.
> > > 
> > > > The regulator delays model the intrinsic delays when enabling or
> > > > disabling a regulator, and they should stay. They address a different
> > > > problem.
> > > 
> > > OK right. But someone not knowing exactly what they are doing
> > > will end up abusing the delay property on the delay line
> > > also for this delay. The risk of that is lesser with a separate
> > > delay box.
> > 
> > That may be true, but I think we can also try to catch abuses in
> > reviews. I would be a bit sad if we made life more difficult (and less
> > efficient at runtime too) for legitimate users just because we are
> > worried about abuses.
> 
> What is a legitimate user for you? Given the example in v2 of this series it's 
> clear that this feature is an opt-in, both for the DT node as well as for 
> specifying a delay.
> Another benefit of using a dedicated driver: It also automatically handles 
> things like setting multiple GPIOs at once.

Your use case is totally legitimate I think. An illegitimate case would
be someone modelling the internal enable delay of a regulator controlled
by a GPIO as a GPIO delay instead of using the regulator enable delay DT
property defined by the regulators bindings.

> > Another thing I've been thinking about is that we may not always want to
> > wait for the GPIO delay. Some consumers may not care when the GPIO line
> > reaches the desired state as long as it eventually does, or maybe they
> > need to perform multiple operations (such as enabling/disabling
> > regulators and/or clocks) and only need a synchronization point for a
> > group of operations. All that would be pretty hard to handle, and maybe
> > it's a problem we'll look at only when needed (and hopefully never).
> 
> If you don't care about rising time, do not use gpio-delay for that GPIO, or 
> just don't specify a ramp-up delay in the gpio-cells, aka setting to 0.
> The more complex synchronisation example you mentioned probably needs a 
> similar dedicated driver for grouping those resources.

You're right that in cases where there is a single consumer, and the
consumer is well known, and the fact that it doesn't care about rising
time is an intrinsic property of that consumer, then DT should simply
not specify any delay. For more complex cases, I'd say it's likely
overkill to try and design a solution now without real use cases.

-- 
Regards,

Laurent Pinchart