[v5,1/2] dt-bindings: counter: add event-counter binding

Commit Message

Oleksij Rempel Feb. 8, 2021, 1:53 p.m. UTC

Add binding for the event counter node

Signed-off-by: Oleksij Rempel <[e-mail]>
---
 .../bindings/counter/event-counter.yaml       | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/counter/event-counter.yaml

Comments

Rob Herring Feb. 10, 2021, 6:41 p.m. UTC | #1

On Mon, Feb 08, 2021 at 02:53:46PM +0100, Oleksij Rempel wrote:
> Add binding for the event counter node
> 
> Signed-off-by: Oleksij Rempel <[e-mail]>
> ---
>  .../bindings/counter/event-counter.yaml       | 56 +++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/counter/event-counter.yaml
> 
> diff --git a/Documentation/devicetree/bindings/counter/event-counter.yaml b/Documentation/devicetree/bindings/counter/event-counter.yaml
> new file mode 100644
> index 000000000000..bd05c1b38f20
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/counter/event-counter.yaml
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/counter/event-counter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Event counter
> +
> +maintainers:
> +  - Oleksij Rempel <[e-mail]>
> +
> +description: |
> +  A generic event counter to measure event frequency. It was developed and used
> +  for agricultural devices to measure rotation speed of wheels or other tools.
> +  Since the direction of rotation is not important, only one signal line is
> +  needed.
> +
> +properties:
> +  compatible:
> +    const: event-counter
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  gpios:
> +    description: Optional diagnostic interface to measure signal level

This description seems wrong in the case of only having a GPIO.

Also, a GPIO only implies polled mode because if the GPIO is interrupt 
capable, 'interrupts' should be required. For gpio-keys, we split the 
compatible strings in this case. I leave it to you if you want to make 
it more explicit.

> +    maxItems: 1
> +
> +required:
> +  - compatible

anyOf:
  - required: [ interrupts ]
  - required: [ gpios ]

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    counter-0 {
> +        compatible = "event-counter";
> +        interrupts-extended = <&gpio 0 IRQ_TYPE_EDGE_RISING>;
> +    };
> +
> +    counter-1 {
> +        compatible = "event-counter";
> +        gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> +    };
> +
> +    counter-2 {
> +        compatible = "event-counter";
> +        interrupts-extended = <&gpio 2 IRQ_TYPE_EDGE_RISING>;
> +        gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
> +    };
> +
> +...
> -- 
> 2.30.0
>

Linus Walleij Feb. 12, 2021, 9:22 a.m. UTC | #2

On Wed, Feb 10, 2021 at 7:41 PM Rob Herring <[e-mail]> wrote:
> On Mon, Feb 08, 2021 at 02:53:46PM +0100, Oleksij Rempel wrote:

> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  gpios:
> > +    description: Optional diagnostic interface to measure signal level
>
> This description seems wrong in the case of only having a GPIO.
>
> Also, a GPIO only implies polled mode because if the GPIO is interrupt
> capable, 'interrupts' should be required. For gpio-keys, we split the
> compatible strings in this case. I leave it to you if you want to make
> it more explicit.

Ouch. This is a bit of semantic confusion where I see different things
if I put my Linux hat on than if I put my DT hat on ... :/

Linux (or some other OS I suppose) has the ability to look up an
interrupt resource for a GPIO line and that is used quite extensively.

In this case it is certainly possible to write a Linux driver that only take
a GPIO resource and looks up a corresponding interrupt without the
involvement of any DT interrupt resources. This happens a lot.

A typical example is cd-gpios in
Documentation/devicetree/bindings/mmc/mmc-controller.yaml

The operating system will take cd-gpios and infer the corresponding
IRQ from the GPIO hardware by OS-internal mechanisms (in the Linux
case simply using irqdomain) in almost cases, and that is how that is
used today. It is an hardware interrupt
that gets allocated and used but the DT is blissfully ignorant about.

The reason is that GPIO is "general purpose" so they don't have very
specific use cases and the interrupts are general purpose as well.
A certain GPIO line may not even have a certain interrupt associated
with it: there exist GPIO controllers with e.g. 32 GPIO lines but
only 8 interrupts that can be assigned to GPIO lines on a
first-come-first-serve basis so there could not be anything like
a cell in the bindings pointing to a certain interrupt: it has to be
resolved by software, at runtime.

In many cases the corresponsing GPIO hardware will have both
gpio-controller and interrupt-controller flags, but I bet there exist
cases that are only flagged with gpio-controller and then the drivers
in the OS goes and implement interrupts using its abstractions and
assign them anyway.

I don't know if this can be solved in a generic way (solved as in DT
needs to know all about the systems interrupt resources, and the OS
should not be handing out interrupts behind the back of the DT description
for things that are not flagged as interrupt-controller) or if this ambiguity
around GPIO chips just has to stay around forever.

I think it may be one of those cases where DT bindings can't be
all that imperialistic about controlling every resource, but there may
be other views on this.

Yours,
Linus Walleij

Linus Walleij Feb. 12, 2021, 9:29 a.m. UTC | #3

On Mon, Feb 8, 2021 at 2:53 PM Oleksij Rempel <[e-mail]> wrote:

> Add binding for the event counter node
>
> Signed-off-by: Oleksij Rempel <[e-mail]>

Works for me:
Reviewed-by: Linus Walleij <[e-mail]>

> +required:
> +  - compatible

Ideally it should be "interrupts OR gpios required"
(at least one of them must be present) but I don't know if
we can express that.

Perhaps also write that if both are defined, the interrupt will
take precedence for counting events.

Yours,
Linus Walleij

Patch

diff --git a/Documentation/devicetree/bindings/counter/event-counter.yaml b/Documentation/devicetree/bindings/counter/event-counter.yaml
new file mode 100644
index 000000000000..bd05c1b38f20
--- /dev/null
+++ b/Documentation/devicetree/bindings/counter/event-counter.yaml
@@ -0,0 +1,56 @@ 
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/counter/event-counter.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Event counter
+
+maintainers:
+  - Oleksij Rempel <[e-mail]>
+
+description: |
+  A generic event counter to measure event frequency. It was developed and used
+  for agricultural devices to measure rotation speed of wheels or other tools.
+  Since the direction of rotation is not important, only one signal line is
+  needed.
+
+properties:
+  compatible:
+    const: event-counter
+
+  interrupts:
+    maxItems: 1
+
+  gpios:
+    description: Optional diagnostic interface to measure signal level
+    maxItems: 1
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    counter-0 {
+        compatible = "event-counter";
+        interrupts-extended = <&gpio 0 IRQ_TYPE_EDGE_RISING>;
+    };
+
+    counter-1 {
+        compatible = "event-counter";
+        gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+    };
+
+    counter-2 {
+        compatible = "event-counter";
+        interrupts-extended = <&gpio 2 IRQ_TYPE_EDGE_RISING>;
+        gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+    };
+
+...