| Message ID | 20210208135347.18494-1-o.rempel@pengutronix.de |
|---|---|
| Headers | show |
| Series | add support for GPIO or IRQ based evemt counter | expand |
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 <o.rempel@pengutronix.de> > --- > .../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 <o.rempel@pengutronix.de> > + > +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 >
On Wed, Feb 10, 2021 at 7:41 PM Rob Herring <robh@kernel.org> 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
On Mon, Feb 8, 2021 at 2:53 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote: > Add binding for the event counter node > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> Works for me: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > +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
On Mon, Feb 08, 2021 at 02:53:45PM +0100, Oleksij Rempel wrote: > changes v5: > - rename it to event counter, since it support different event sources At the risk of bikeshedding, I feel "event counter" is too general of a phrase to be useful here -- not to mention that the Counter subsystem concept of an "event" does not necessarily correlate to unique interrupts (a Counter driver may generate multiple events for a given interrupt, or even events for non-interrupt state changes). Instead, if I understand the behavior of this particular driver correctly, it is more apt to call this an "interrupt counter" because it is counting interrupts regardless of the source (GPIO or not). William Breathitt Gray > - make it work with gpio-only or irq-only configuration > - update yaml binding > > changes v4: > - use IRQ_NOAUTOEN to not enable IRQ by default > - rename gpio_ from name pattern and make this driver work any IRQ > source. > > changes v3: > - convert counter to atomic_t > > changes v2: > - add commas > - avoid possible unhandled interrupts in the enable path > - do not use of_ specific gpio functions > > Add support for GPIO based pulse counter. For now it can only count > pulses. With counter char device support, we will be able to attach > timestamps and measure actual pulse frequency. > > Never the less, it is better to mainline this driver now (before chardev > patches go mainline), to provide developers additional use case for the counter > framework with chardev support. > > Oleksij Rempel (2): > dt-bindings: counter: add event-counter binding > counter: add IRQ or GPIO based event counter > > .../bindings/counter/event-counter.yaml | 56 ++++ > drivers/counter/Kconfig | 10 + > drivers/counter/Makefile | 1 + > drivers/counter/event-cnt.c | 250 ++++++++++++++++++ > 4 files changed, 317 insertions(+) > create mode 100644 Documentation/devicetree/bindings/counter/event-counter.yaml > create mode 100644 drivers/counter/event-cnt.c > > -- > 2.30.0 >