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

[Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>]

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(+)