| Message ID | 20221214095342.937303-1-alexander.stein@ew.tq-group.com |
|---|---|
| Headers | show |
| Series | gpio: Add gpio-delay support | expand |
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
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
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.
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
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