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

[Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxx> 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