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

[Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
> > ---
> > 
> >  .../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@xxxxxxxxxxxxxxx>
> > +
> > +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