[PATCH v5 0/2] add support for GPIO or IRQ based evemt counter

changes v5:
- rename it to event counter, since it support different event sources
- 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

Add simple IRQ or GPIO base event counter. This device is used to measure
rotation speed of some agricultural devices, so no high frequency on the
counter pin is expected.

The maximal measurement frequency depends on the CPU and system load. On
the idle iMX6S I was able to measure up to 20kHz without count drops.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/counter/Kconfig     |  10 ++
 drivers/counter/Makefile    |   1 +
 drivers/counter/event-cnt.c | 250 ++++++++++++++++++++++++++++++++++++
 3 files changed, 261 insertions(+)
 create mode 100644 drivers/counter/event-cnt.c

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2de53ab0dd25..3284987e070a 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig

@@ -29,6 +29,16 @@ config 104_QUAD_8
 	  The base port addresses for the devices may be configured via the base
 	  array module parameter.
 
+config EVENT_CNT
+	tristate "Event counter driver"
+	depends on GPIOLIB
+	help
+	  Select this option to enable event counter driver. Any interrupt source
+	  can be used by this driver as the event source.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gpio-pulse-cnt.
+
 config STM32_TIMER_CNT
 	tristate "STM32 Timer encoder counter driver"
 	depends on MFD_STM32_TIMERS || COMPILE_TEST

diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 0a393f71e481..6626900468f6 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile

@@ -6,6 +6,7 @@
 obj-$(CONFIG_COUNTER) += counter.o
 
 obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
+obj-$(CONFIG_EVENT_CNT)		+= event-cnt.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
 obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o

diff --git a/drivers/counter/event-cnt.c b/drivers/counter/event-cnt.c
new file mode 100644
index 000000000000..a394fe72c4e4
--- /dev/null
+++ b/drivers/counter/event-cnt.c

@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021 Pengutronix, Oleksij Rempel <kernel@pengutronix.de>
+ */
+
+#include <linux/counter.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define EVENT_CNT_NAME		"event-cnt"
+
+struct event_cnt_priv {
+	struct counter_device counter;
+	struct counter_ops ops;
+	struct gpio_desc *gpio;
+	int irq;
+	bool enabled;
+	atomic_t count;
+};
+
+static irqreturn_t event_cnt_isr(int irq, void *dev_id)
+{
+	struct event_cnt_priv *priv = dev_id;
+
+	atomic_inc(&priv->count);
+
+	return IRQ_HANDLED;
+}
+
+static ssize_t event_cnt_enable_read(struct counter_device *counter,
+				     struct counter_count *count, void *private,
+				     char *buf)
+{
+	struct event_cnt_priv *priv = counter->priv;
+
+	return sysfs_emit(buf, "%d\n", priv->enabled);
+}
+
+static ssize_t event_cnt_enable_write(struct counter_device *counter,
+				      struct counter_count *count,
+				      void *private, const char *buf,
+				      size_t len)
+{
+	struct event_cnt_priv *priv = counter->priv;
+	bool enable;
+	ssize_t ret;
+
+	ret = kstrtobool(buf, &enable);
+	if (ret)
+		return ret;
+
+	if (priv->enabled == enable)
+		return len;
+
+	if (enable) {
+		priv->enabled = enable;
+		enable_irq(priv->irq);
+	} else {
+		disable_irq(priv->irq);
+		priv->enabled = enable;
+	}
+
+	return len;
+}
+
+static const struct counter_count_ext event_cnt_ext[] = {
+	{
+		.name = "enable",
+		.read = event_cnt_enable_read,
+		.write = event_cnt_enable_write,
+	},
+};
+
+static enum counter_synapse_action event_cnt_synapse_actionss[] = {
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+};
+
+static int event_cnt_action_get(struct counter_device *counter,
+			    struct counter_count *count,
+			    struct counter_synapse *synapse,
+			    size_t *action)
+{
+	*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
+
+	return 0;
+}
+
+static int event_cnt_read(struct counter_device *counter,
+				 struct counter_count *count,
+				 unsigned long *val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+
+	*val = atomic_read(&priv->count);
+
+	return 0;
+}
+
+static int event_cnt_write(struct counter_device *counter,
+				  struct counter_count *count,
+				  const unsigned long val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+
+	atomic_set(&priv->count, val);
+
+	return 0;
+}
+
+static int event_cnt_function_get(struct counter_device *counter,
+				  struct counter_count *count, size_t *function)
+{
+	*function = COUNTER_COUNT_FUNCTION_INCREASE;
+
+	return 0;
+}
+
+static int event_cnt_signal_read(struct counter_device *counter,
+				 struct counter_signal *signal,
+				 enum counter_signal_value *val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+	int ret;
+
+	ret = gpiod_get_value(priv->gpio);
+	if (ret < 0)
+		return ret;
+
+	*val = ret ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
+
+	return 0;
+}
+
+static struct counter_signal event_cnt_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 0 signal",
+	},
+};
+
+static struct counter_synapse event_cnt_synapses[] = {
+	{
+		.actions_list = event_cnt_synapse_actionss,
+		.num_actions = ARRAY_SIZE(event_cnt_synapse_actionss),
+		.signal = &event_cnt_signals[0]
+	},
+};
+
+static enum counter_count_function event_cnt_functions[] = {
+	COUNTER_COUNT_FUNCTION_INCREASE,
+};
+
+static struct counter_count event_cnts[] = {
+	{
+		.id = 0,
+		.name = "Channel 1 Count",
+		.functions_list = event_cnt_functions,
+		.num_functions = ARRAY_SIZE(event_cnt_functions),
+		.synapses = event_cnt_synapses,
+		.num_synapses = ARRAY_SIZE(event_cnt_synapses),
+		.ext = event_cnt_ext,
+		.num_ext = ARRAY_SIZE(event_cnt_ext),
+	},
+};
+
+static int event_cnt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct event_cnt_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->irq = platform_get_irq_optional(pdev,  0);
+	if (priv->irq == -ENXIO)
+		priv->irq = 0;
+	else if (priv->irq < 0)
+		return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
+
+	priv->gpio = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
+	if (IS_ERR(priv->gpio))
+		return dev_err_probe(dev, PTR_ERR(priv->gpio), "failed to get GPIO\n");
+
+	if (!priv->irq && !priv->gpio) {
+		dev_err(dev, "IRQ and GPIO are not found. At least one source should be provided\n");
+		return -ENODEV;
+	}
+
+	if (!priv->irq) {
+		int irq = gpiod_to_irq(priv->gpio);
+
+		if (irq < 0)
+			return dev_err_probe(dev, irq, "failed to get IRQ from GPIO\n");
+
+		priv->irq = irq;
+	}
+
+	priv->ops.action_get = event_cnt_action_get;
+	priv->ops.count_read = event_cnt_read;
+	priv->ops.count_write = event_cnt_write;
+	priv->ops.function_get = event_cnt_function_get;
+	if (priv->gpio)
+		priv->ops.signal_read  = event_cnt_signal_read;
+
+	priv->counter.name = dev_name(dev);
+	priv->counter.parent = dev;
+	priv->counter.ops = &priv->ops;
+	priv->counter.counts = event_cnts;
+	priv->counter.num_counts = ARRAY_SIZE(event_cnts);
+	priv->counter.signals = event_cnt_signals;
+	priv->counter.num_signals = ARRAY_SIZE(event_cnt_signals);
+	priv->counter.priv = priv;
+
+	irq_set_status_flags(priv->irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(dev, priv->irq, event_cnt_isr,
+			       IRQF_TRIGGER_RISING | IRQF_NO_THREAD,
+			       EVENT_CNT_NAME, priv);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, priv);
+
+	return devm_counter_register(dev, &priv->counter);
+}
+
+static const struct of_device_id event_cnt_of_match[] = {
+	{ .compatible = "event-counter", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, event_cnt_of_match);
+
+static struct platform_driver event_cnt_driver = {
+	.probe = event_cnt_probe,
+	.driver = {
+		.name = EVENT_CNT_NAME,
+		.of_match_table = event_cnt_of_match,
+	},
+};
+module_platform_driver(event_cnt_driver);
+
+MODULE_ALIAS("platform:event-counter");
+MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
+MODULE_DESCRIPTION("Event counter driver");
+MODULE_LICENSE("GPL v2");

-- 
2.30.0

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
+    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>;
+    };
+
+...

-- 
2.30.0

On Mon, Feb 08, 2021 at 02:53:47PM +0100, Oleksij Rempel wrote:

Add simple IRQ or GPIO base event counter. This device is used to measure
rotation speed of some agricultural devices, so no high frequency on the
counter pin is expected.

The maximal measurement frequency depends on the CPU and system load. On
the idle iMX6S I was able to measure up to 20kHz without count drops.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Hi Oleksij,

I haven't had a chance to do a proper review just yet, but a couple
things caught my eye as I skimmed so I want to mention them inline
below. I'll do a more complete review this weekend or soon after

The value pointed by this "action" parameter should be set to the index
of the action you want from your Synapse's ations[] array.

+
+	return 0;
+}
+
+static int event_cnt_read(struct counter_device *counter,
+				 struct counter_count *count,
+				 unsigned long *val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+
+	*val = atomic_read(&priv->count);
+
+	return 0;
+}
+
+static int event_cnt_write(struct counter_device *counter,
+				  struct counter_count *count,
+				  const unsigned long val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+
+	atomic_set(&priv->count, val);
+
+	return 0;
+}
+
+static int event_cnt_function_get(struct counter_device *counter,
+				  struct counter_count *count, size_t *function)
+{
+	*function = COUNTER_COUNT_FUNCTION_INCREASE;

Same problem here as before: should be set to the index of the function
you want from your functions[] array.

William Breathitt Gray

+
+	return 0;
+}
+
+static int event_cnt_signal_read(struct counter_device *counter,
+				 struct counter_signal *signal,
+				 enum counter_signal_value *val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+	int ret;
+
+	ret = gpiod_get_value(priv->gpio);
+	if (ret < 0)
+		return ret;
+
+	*val = ret ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
+
+	return 0;
+}
+
+static struct counter_signal event_cnt_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 0 signal",
+	},
+};
+
+static struct counter_synapse event_cnt_synapses[] = {
+	{
+		.actions_list = event_cnt_synapse_actionss,
+		.num_actions = ARRAY_SIZE(event_cnt_synapse_actionss),
+		.signal = &event_cnt_signals[0]
+	},
+};
+
+static enum counter_count_function event_cnt_functions[] = {
+	COUNTER_COUNT_FUNCTION_INCREASE,
+};
+
+static struct counter_count event_cnts[] = {
+	{
+		.id = 0,
+		.name = "Channel 1 Count",
+		.functions_list = event_cnt_functions,
+		.num_functions = ARRAY_SIZE(event_cnt_functions),
+		.synapses = event_cnt_synapses,
+		.num_synapses = ARRAY_SIZE(event_cnt_synapses),
+		.ext = event_cnt_ext,
+		.num_ext = ARRAY_SIZE(event_cnt_ext),
+	},
+};
+
+static int event_cnt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct event_cnt_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->irq = platform_get_irq_optional(pdev,  0);
+	if (priv->irq == -ENXIO)
+		priv->irq = 0;
+	else if (priv->irq < 0)
+		return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
+
+	priv->gpio = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
+	if (IS_ERR(priv->gpio))
+		return dev_err_probe(dev, PTR_ERR(priv->gpio), "failed to get GPIO\n");
+
+	if (!priv->irq && !priv->gpio) {
+		dev_err(dev, "IRQ and GPIO are not found. At least one source should be provided\n");
+		return -ENODEV;
+	}
+
+	if (!priv->irq) {
+		int irq = gpiod_to_irq(priv->gpio);
+
+		if (irq < 0)
+			return dev_err_probe(dev, irq, "failed to get IRQ from GPIO\n");
+
+		priv->irq = irq;
+	}
+
+	priv->ops.action_get = event_cnt_action_get;
+	priv->ops.count_read = event_cnt_read;
+	priv->ops.count_write = event_cnt_write;
+	priv->ops.function_get = event_cnt_function_get;
+	if (priv->gpio)
+		priv->ops.signal_read  = event_cnt_signal_read;
+
+	priv->counter.name = dev_name(dev);
+	priv->counter.parent = dev;
+	priv->counter.ops = &priv->ops;
+	priv->counter.counts = event_cnts;
+	priv->counter.num_counts = ARRAY_SIZE(event_cnts);
+	priv->counter.signals = event_cnt_signals;
+	priv->counter.num_signals = ARRAY_SIZE(event_cnt_signals);
+	priv->counter.priv = priv;
+
+	irq_set_status_flags(priv->irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(dev, priv->irq, event_cnt_isr,
+			       IRQF_TRIGGER_RISING | IRQF_NO_THREAD,
+			       EVENT_CNT_NAME, priv);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, priv);
+
+	return devm_counter_register(dev, &priv->counter);
+}
+
+static const struct of_device_id event_cnt_of_match[] = {
+	{ .compatible = "event-counter", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, event_cnt_of_match);
+
+static struct platform_driver event_cnt_driver = {
+	.probe = event_cnt_probe,
+	.driver = {
+		.name = EVENT_CNT_NAME,
+		.of_match_table = event_cnt_of_match,
+	},
+};
+module_platform_driver(event_cnt_driver);
+
+MODULE_ALIAS("platform:event-counter");
+MODULE_AUTHOR("Oleksij Rempel [off-list ref]");
+MODULE_DESCRIPTION("Event counter driver");
+MODULE_LICENSE("GPL v2");
-- 
2.30.0

On Mon, Feb 08, 2021 at 02:53:46PM +0100, Oleksij Rempel wrote:

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 [off-list ref] wrote:

On Mon, Feb 08, 2021 at 02:53:46PM +0100, Oleksij Rempel wrote:

quoted

+  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 [off-list ref] wrote:

Add simple IRQ or GPIO base event counter. This device is used to measure
rotation speed of some agricultural devices, so no high frequency on the
counter pin is expected.

The maximal measurement frequency depends on the CPU and system load. On
the idle iMX6S I was able to measure up to 20kHz without count drops.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

From GPIO and interrupt point of view this driver looks good to me.
I don't know about the userspace interface etc.

Yours,
Linus Walleij

On Mon, Feb 8, 2021 at 2:53 PM Oleksij Rempel [off-list ref] wrote:

Add binding for the event counter node

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Works for me:
Reviewed-by: Linus Walleij <redacted>

+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

On Mon, Feb 08, 2021 at 02:53:47PM +0100, Oleksij Rempel wrote:

Add simple IRQ or GPIO base event counter. This device is used to measure
rotation speed of some agricultural devices, so no high frequency on the
counter pin is expected.

The maximal measurement frequency depends on the CPU and system load. On
the idle iMX6S I was able to measure up to 20kHz without count drops.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Hi Oleksij,

You're adding a new driver here so I'd like to see a new entry added to
the MAINTAINERS file so users know who to contact when they have
questions or want to submit patches.

This is just used to count the number of interrupts right? I wonder if
we can do this smarter. For example, the kernel already keeps track of
number of interrupts that has occurred for any particular IRQ line on a
CPU (see the 'kstat_irqs' member of struct irq_desc, and the
show_interrupts() function in kernel/irq/proc.c). Would it make sense to
simply store the initial interrupt count on driver load or enablement,
and then return the difference during a count_read() callback?

+
+	return IRQ_HANDLED;
+}
+
+static ssize_t event_cnt_enable_read(struct counter_device *counter,
+				     struct counter_count *count, void *private,
+				     char *buf)
+{
+	struct event_cnt_priv *priv = counter->priv;
+
+	return sysfs_emit(buf, "%d\n", priv->enabled);
+}
+
+static ssize_t event_cnt_enable_write(struct counter_device *counter,
+				      struct counter_count *count,
+				      void *private, const char *buf,
+				      size_t len)
+{
+	struct event_cnt_priv *priv = counter->priv;
+	bool enable;
+	ssize_t ret;
+
+	ret = kstrtobool(buf, &enable);
+	if (ret)
+		return ret;
+
+	if (priv->enabled == enable)
+		return len;
+
+	if (enable) {
+		priv->enabled = enable;
+		enable_irq(priv->irq);
+	} else {
+		disable_irq(priv->irq);
+		priv->enabled = enable;
+	}

Given the conditional path we know the value "enable" will hold here.
it'll be good to set priv->enabled explicitly here so there's no
confusion to a future reviewer:

	if (enable) {
		priv->enabled = 1;
		enable_irq(priv->irq);
	} else {
		disable_irq(priv->irq);
		priv->enabled = 0;
	}

+
+	return len;
+}
+
+static const struct counter_count_ext event_cnt_ext[] = {
+	{
+		.name = "enable",
+		.read = event_cnt_enable_read,
+		.write = event_cnt_enable_write,
+	},
+};
+
+static enum counter_synapse_action event_cnt_synapse_actionss[] = {
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+};
+
+static int event_cnt_action_get(struct counter_device *counter,
+			    struct counter_count *count,
+			    struct counter_synapse *synapse,
+			    size_t *action)
+{
+	*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;

Fix this as mentioned in my earlier reply:
https://lore.kernel.org/linux-iio/YCFHRGbiVxpNgkQS@shinobu/

+
+	return 0;
+}
+
+static int event_cnt_read(struct counter_device *counter,
+				 struct counter_count *count,
+				 unsigned long *val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+
+	*val = atomic_read(&priv->count);
+
+	return 0;
+}
+
+static int event_cnt_write(struct counter_device *counter,
+				  struct counter_count *count,
+				  const unsigned long val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+
+	atomic_set(&priv->count, val);
+
+	return 0;
+}
+
+static int event_cnt_function_get(struct counter_device *counter,
+				  struct counter_count *count, size_t *function)
+{
+	*function = COUNTER_COUNT_FUNCTION_INCREASE;

Fix this as mentioned in my earlier reply:
https://lore.kernel.org/linux-iio/YCFHRGbiVxpNgkQS@shinobu/

+
+	return 0;
+}
+
+static int event_cnt_signal_read(struct counter_device *counter,
+				 struct counter_signal *signal,
+				 enum counter_signal_value *val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+	int ret;
+
+	ret = gpiod_get_value(priv->gpio);
+	if (ret < 0)
+		return ret;
+
+	*val = ret ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
+
+	return 0;
+}
+
+static struct counter_signal event_cnt_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 0 signal",

You should choose a more description name for this Signal;
"Channel 0 signal" isn't very useful information for the user. Is this
signal the respective GPIO line state?

+	},
+};
+
+static struct counter_synapse event_cnt_synapses[] = {
+	{
+		.actions_list = event_cnt_synapse_actionss,
+		.num_actions = ARRAY_SIZE(event_cnt_synapse_actionss),
+		.signal = &event_cnt_signals[0]
+	},
+};
+
+static enum counter_count_function event_cnt_functions[] = {
+	COUNTER_COUNT_FUNCTION_INCREASE,
+};
+
+static struct counter_count event_cnts[] = {
+	{
+		.id = 0,
+		.name = "Channel 1 Count",
+		.functions_list = event_cnt_functions,
+		.num_functions = ARRAY_SIZE(event_cnt_functions),
+		.synapses = event_cnt_synapses,
+		.num_synapses = ARRAY_SIZE(event_cnt_synapses),
+		.ext = event_cnt_ext,
+		.num_ext = ARRAY_SIZE(event_cnt_ext),
+	},
+};
+
+static int event_cnt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct event_cnt_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->irq = platform_get_irq_optional(pdev,  0);
+	if (priv->irq == -ENXIO)
+		priv->irq = 0;
+	else if (priv->irq < 0)
+		return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
+
+	priv->gpio = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
+	if (IS_ERR(priv->gpio))
+		return dev_err_probe(dev, PTR_ERR(priv->gpio), "failed to get GPIO\n");
+
+	if (!priv->irq && !priv->gpio) {
+		dev_err(dev, "IRQ and GPIO are not found. At least one source should be provided\n");
+		return -ENODEV;
+	}
+
+	if (!priv->irq) {
+		int irq = gpiod_to_irq(priv->gpio);
+
+		if (irq < 0)
+			return dev_err_probe(dev, irq, "failed to get IRQ from GPIO\n");
+
+		priv->irq = irq;
+	}
+
+	priv->ops.action_get = event_cnt_action_get;
+	priv->ops.count_read = event_cnt_read;
+	priv->ops.count_write = event_cnt_write;
+	priv->ops.function_get = event_cnt_function_get;
+	if (priv->gpio)
+		priv->ops.signal_read  = event_cnt_signal_read;

Move ops out of priv and make it static const. You can get rid of this
priv->gpio conditional here and instead perform it for num_signals
as I explain inline below.

+
+	priv->counter.name = dev_name(dev);
+	priv->counter.parent = dev;
+	priv->counter.ops = &priv->ops;
+	priv->counter.counts = event_cnts;
+	priv->counter.num_counts = ARRAY_SIZE(event_cnts);
+	priv->counter.signals = event_cnt_signals;
+	priv->counter.num_signals = ARRAY_SIZE(event_cnt_signals);

If the Signal provided is only valid for GPIO sources, then you should
add a conditional check here. Simply setting num_signals to 0 should be
enough to disable the creation of the Signal attribute for non-GPIO
sources:

	priv->counter.num_signals = priv->gpio ?
				    ARRAY_SIZE(event_cnt_signals) : 0;

+	priv->counter.priv = priv;
+
+	irq_set_status_flags(priv->irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(dev, priv->irq, event_cnt_isr,
+			       IRQF_TRIGGER_RISING | IRQF_NO_THREAD,
+			       EVENT_CNT_NAME, priv);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, priv);

This line isn't needed; you don't ever deal with struct device directly
do you?

William Breathitt Gray

+
+	return devm_counter_register(dev, &priv->counter);
+}
+
+static const struct of_device_id event_cnt_of_match[] = {
+	{ .compatible = "event-counter", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, event_cnt_of_match);
+
+static struct platform_driver event_cnt_driver = {
+	.probe = event_cnt_probe,
+	.driver = {
+		.name = EVENT_CNT_NAME,
+		.of_match_table = event_cnt_of_match,
+	},
+};
+module_platform_driver(event_cnt_driver);
+
+MODULE_ALIAS("platform:event-counter");
+MODULE_AUTHOR("Oleksij Rempel [off-list ref]");
+MODULE_DESCRIPTION("Event counter driver");
+MODULE_LICENSE("GPL v2");
-- 
2.30.0

On Fri, Feb 12, 2021 at 10:26:39AM +0100, Linus Walleij wrote:

On Mon, Feb 8, 2021 at 2:53 PM Oleksij Rempel [off-list ref] wrote:

quoted

Add simple IRQ or GPIO base event counter. This device is used to measure
rotation speed of some agricultural devices, so no high frequency on the
counter pin is expected.

The maximal measurement frequency depends on the CPU and system load. On
the idle iMX6S I was able to measure up to 20kHz without count drops.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

From GPIO and interrupt point of view this driver looks good to me.
I don't know about the userspace interface etc.

May I have your Reviewed-by?

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Hi William,

On Sun, Feb 14, 2021 at 05:54:22PM +0900, William Breathitt Gray wrote:

On Mon, Feb 08, 2021 at 02:53:47PM +0100, Oleksij Rempel wrote:

quoted

Add simple IRQ or GPIO base event counter. This device is used to measure
rotation speed of some agricultural devices, so no high frequency on the
counter pin is expected.

The maximal measurement frequency depends on the CPU and system load. On
the idle iMX6S I was able to measure up to 20kHz without count drops.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

Hi Oleksij,

You're adding a new driver here so I'd like to see a new entry added to
the MAINTAINERS file so users know who to contact when they have
questions or want to submit patches.

done

quoted

---
 drivers/counter/Kconfig     |  10 ++
 drivers/counter/Makefile    |   1 +
 drivers/counter/event-cnt.c | 250 ++++++++++++++++++++++++++++++++++++
 3 files changed, 261 insertions(+)
 create mode 100644 drivers/counter/event-cnt.c

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2de53ab0dd25..3284987e070a 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig

@@ -29,6 +29,16 @@ config 104_QUAD_8
 	  The base port addresses for the devices may be configured via the base
 	  array module parameter.
 
+config EVENT_CNT
+	tristate "Event counter driver"
+	depends on GPIOLIB
+	help
+	  Select this option to enable event counter driver. Any interrupt source
+	  can be used by this driver as the event source.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gpio-pulse-cnt.
+
 config STM32_TIMER_CNT
 	tristate "STM32 Timer encoder counter driver"
 	depends on MFD_STM32_TIMERS || COMPILE_TEST

diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 0a393f71e481..6626900468f6 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile

@@ -6,6 +6,7 @@
 obj-$(CONFIG_COUNTER) += counter.o
 
 obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
+obj-$(CONFIG_EVENT_CNT)		+= event-cnt.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
 obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o

diff --git a/drivers/counter/event-cnt.c b/drivers/counter/event-cnt.c
new file mode 100644
index 000000000000..a394fe72c4e4
--- /dev/null
+++ b/drivers/counter/event-cnt.c

@@ -0,0 +1,250 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2021 Pengutronix, Oleksij Rempel <kernel@pengutronix.de>
+ */
+
+#include <linux/counter.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define EVENT_CNT_NAME		"event-cnt"
+
+struct event_cnt_priv {
+	struct counter_device counter;
+	struct counter_ops ops;
+	struct gpio_desc *gpio;
+	int irq;
+	bool enabled;
+	atomic_t count;
+};
+
+static irqreturn_t event_cnt_isr(int irq, void *dev_id)
+{
+	struct event_cnt_priv *priv = dev_id;
+
+	atomic_inc(&priv->count);

This is just used to count the number of interrupts right? I wonder if
we can do this smarter. For example, the kernel already keeps track of
number of interrupts that has occurred for any particular IRQ line on a
CPU (see the 'kstat_irqs' member of struct irq_desc, and the
show_interrupts() function in kernel/irq/proc.c). Would it make sense to
simply store the initial interrupt count on driver load or enablement,
and then return the difference during a count_read() callback?

This driver do not makes a lot of sense without your chardev patches. As
soon as this patches go mainline, this driver will be able to send
event with a timestamp and counter state to the user space.

With other words, we will need an irq handler anyway. In this case we
can't save more RAM or CPU cycles by using system irq counters.

quoted

+
+	return IRQ_HANDLED;
+}
+
+static ssize_t event_cnt_enable_read(struct counter_device *counter,
+				     struct counter_count *count, void *private,
+				     char *buf)
+{
+	struct event_cnt_priv *priv = counter->priv;
+
+	return sysfs_emit(buf, "%d\n", priv->enabled);
+}
+
+static ssize_t event_cnt_enable_write(struct counter_device *counter,
+				      struct counter_count *count,
+				      void *private, const char *buf,
+				      size_t len)
+{
+	struct event_cnt_priv *priv = counter->priv;
+	bool enable;
+	ssize_t ret;
+
+	ret = kstrtobool(buf, &enable);
+	if (ret)
+		return ret;
+
+	if (priv->enabled == enable)
+		return len;
+
+	if (enable) {
+		priv->enabled = enable;
+		enable_irq(priv->irq);
+	} else {
+		disable_irq(priv->irq);
+		priv->enabled = enable;
+	}

Given the conditional path we know the value "enable" will hold here.
it'll be good to set priv->enabled explicitly here so there's no
confusion to a future reviewer:

	if (enable) {
		priv->enabled = 1;
		enable_irq(priv->irq);
	} else {
		disable_irq(priv->irq);
		priv->enabled = 0;
	}

Done

quoted

+
+	return len;
+}
+
+static const struct counter_count_ext event_cnt_ext[] = {
+	{
+		.name = "enable",
+		.read = event_cnt_enable_read,
+		.write = event_cnt_enable_write,
+	},
+};
+
+static enum counter_synapse_action event_cnt_synapse_actionss[] = {
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+};
+
+static int event_cnt_action_get(struct counter_device *counter,
+			    struct counter_count *count,
+			    struct counter_synapse *synapse,
+			    size_t *action)
+{
+	*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;

Fix this as mentioned in my earlier reply:
https://lore.kernel.org/linux-iio/YCFHRGbiVxpNgkQS@shinobu/

quoted

+
+	return 0;
+}
+
+static int event_cnt_read(struct counter_device *counter,
+				 struct counter_count *count,
+				 unsigned long *val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+
+	*val = atomic_read(&priv->count);
+
+	return 0;
+}
+
+static int event_cnt_write(struct counter_device *counter,
+				  struct counter_count *count,
+				  const unsigned long val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+
+	atomic_set(&priv->count, val);
+
+	return 0;
+}
+
+static int event_cnt_function_get(struct counter_device *counter,
+				  struct counter_count *count, size_t *function)
+{
+	*function = COUNTER_COUNT_FUNCTION_INCREASE;

Fix this as mentioned in my earlier reply:
https://lore.kernel.org/linux-iio/YCFHRGbiVxpNgkQS@shinobu/

done

quoted

+
+	return 0;
+}
+
+static int event_cnt_signal_read(struct counter_device *counter,
+				 struct counter_signal *signal,
+				 enum counter_signal_value *val)
+{
+	struct event_cnt_priv *priv = counter->priv;
+	int ret;
+
+	ret = gpiod_get_value(priv->gpio);
+	if (ret < 0)
+		return ret;
+
+	*val = ret ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
+
+	return 0;
+}
+
+static struct counter_signal event_cnt_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 0 signal",

You should choose a more description name for this Signal;
"Channel 0 signal" isn't very useful information for the user. Is this
signal the respective GPIO line state?

Sounds plausible. How about "Channel 0, GPIO line state"?

quoted

+	},
+};
+
+static struct counter_synapse event_cnt_synapses[] = {
+	{
+		.actions_list = event_cnt_synapse_actionss,
+		.num_actions = ARRAY_SIZE(event_cnt_synapse_actionss),
+		.signal = &event_cnt_signals[0]
+	},
+};
+
+static enum counter_count_function event_cnt_functions[] = {
+	COUNTER_COUNT_FUNCTION_INCREASE,
+};
+
+static struct counter_count event_cnts[] = {
+	{
+		.id = 0,
+		.name = "Channel 1 Count",
+		.functions_list = event_cnt_functions,
+		.num_functions = ARRAY_SIZE(event_cnt_functions),
+		.synapses = event_cnt_synapses,
+		.num_synapses = ARRAY_SIZE(event_cnt_synapses),
+		.ext = event_cnt_ext,
+		.num_ext = ARRAY_SIZE(event_cnt_ext),
+	},
+};
+
+static int event_cnt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct event_cnt_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->irq = platform_get_irq_optional(pdev,  0);
+	if (priv->irq == -ENXIO)
+		priv->irq = 0;
+	else if (priv->irq < 0)
+		return dev_err_probe(dev, priv->irq, "failed to get IRQ\n");
+
+	priv->gpio = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
+	if (IS_ERR(priv->gpio))
+		return dev_err_probe(dev, PTR_ERR(priv->gpio), "failed to get GPIO\n");
+
+	if (!priv->irq && !priv->gpio) {
+		dev_err(dev, "IRQ and GPIO are not found. At least one source should be provided\n");
+		return -ENODEV;
+	}
+
+	if (!priv->irq) {
+		int irq = gpiod_to_irq(priv->gpio);
+
+		if (irq < 0)
+			return dev_err_probe(dev, irq, "failed to get IRQ from GPIO\n");
+
+		priv->irq = irq;
+	}
+
+	priv->ops.action_get = event_cnt_action_get;
+	priv->ops.count_read = event_cnt_read;
+	priv->ops.count_write = event_cnt_write;
+	priv->ops.function_get = event_cnt_function_get;
+	if (priv->gpio)
+		priv->ops.signal_read  = event_cnt_signal_read;

Move ops out of priv and make it static const. You can get rid of this
priv->gpio conditional here and instead perform it for num_signals
as I explain inline below.

done

quoted

+
+	priv->counter.name = dev_name(dev);
+	priv->counter.parent = dev;
+	priv->counter.ops = &priv->ops;
+	priv->counter.counts = event_cnts;
+	priv->counter.num_counts = ARRAY_SIZE(event_cnts);
+	priv->counter.signals = event_cnt_signals;
+	priv->counter.num_signals = ARRAY_SIZE(event_cnt_signals);

If the Signal provided is only valid for GPIO sources, then you should
add a conditional check here. Simply setting num_signals to 0 should be
enough to disable the creation of the Signal attribute for non-GPIO
sources:

	priv->counter.num_signals = priv->gpio ?
				    ARRAY_SIZE(event_cnt_signals) : 0;

done

quoted

+	priv->counter.priv = priv;
+
+	irq_set_status_flags(priv->irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(dev, priv->irq, event_cnt_isr,
+			       IRQF_TRIGGER_RISING | IRQF_NO_THREAD,
+			       EVENT_CNT_NAME, priv);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, priv);

This line isn't needed; you don't ever deal with struct device directly
do you?

no. removed.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Mon, Feb 15, 2021 at 10:17:37AM +0100, Oleksij Rempel wrote:

quoted

quoted

+static irqreturn_t event_cnt_isr(int irq, void *dev_id)
+{
+	struct event_cnt_priv *priv = dev_id;
+
+	atomic_inc(&priv->count);

This is just used to count the number of interrupts right? I wonder if
we can do this smarter. For example, the kernel already keeps track of
number of interrupts that has occurred for any particular IRQ line on a
CPU (see the 'kstat_irqs' member of struct irq_desc, and the
show_interrupts() function in kernel/irq/proc.c). Would it make sense to
simply store the initial interrupt count on driver load or enablement,
and then return the difference during a count_read() callback?

This driver do not makes a lot of sense without your chardev patches. As
soon as this patches go mainline, this driver will be able to send
event with a timestamp and counter state to the user space.

With other words, we will need an irq handler anyway. In this case we
can't save more RAM or CPU cycles by using system irq counters.

It's true that this driver will need an IRQ handler when the timestamp
functionality is added, but deriving the count value is different matter
regardless. There's already code in the kernel to retrieve the number of
interrupts, so it makes sense that we use that rather than rolling our
own -- at the very least to ensure the value we provide to users is
consistent with the ones already provided by other areas of the kernel.

To that end, I'd like to see your cnt_isr() function removed for this
patchset (you can bring it back once timestamp support is added).
Reimplement your cnt_read/cnt_write() functions to instead use
kstat_irqs_usr() from <linux/kernel_stat.h> to get the current number of
interrupts the IRQ line and use it to derive your count value for this
driver.

quoted

quoted

+static struct counter_signal event_cnt_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 0 signal",

You should choose a more description name for this Signal;
"Channel 0 signal" isn't very useful information for the user. Is this
signal the respective GPIO line state?

Sounds plausible. How about "Channel 0, GPIO line state"?

Ideally, this would match the GPIO name (or I suppose the IRQ number if
not a GPIO line). So in your probe() function you can do something like
this I believe:

	cnt_signals[0].name = priv->gpio->name;

Of course, you should first check whether this is a GPIO line or IRQ
line and set the name accordingly.

William Breathitt Gray

On Mon, Feb 22, 2021 at 10:43:00AM +0900, William Breathitt Gray wrote:

On Mon, Feb 15, 2021 at 10:17:37AM +0100, Oleksij Rempel wrote:

quoted

quoted

quoted

+static irqreturn_t event_cnt_isr(int irq, void *dev_id)
+{
+	struct event_cnt_priv *priv = dev_id;
+
+	atomic_inc(&priv->count);

This is just used to count the number of interrupts right? I wonder if
we can do this smarter. For example, the kernel already keeps track of
number of interrupts that has occurred for any particular IRQ line on a
CPU (see the 'kstat_irqs' member of struct irq_desc, and the
show_interrupts() function in kernel/irq/proc.c). Would it make sense to
simply store the initial interrupt count on driver load or enablement,
and then return the difference during a count_read() callback?

This driver do not makes a lot of sense without your chardev patches. As
soon as this patches go mainline, this driver will be able to send
event with a timestamp and counter state to the user space.

With other words, we will need an irq handler anyway. In this case we
can't save more RAM or CPU cycles by using system irq counters.

It's true that this driver will need an IRQ handler when the timestamp
functionality is added, but deriving the count value is different matter
regardless. There's already code in the kernel to retrieve the number of
interrupts, so it makes sense that we use that rather than rolling our
own -- at the very least to ensure the value we provide to users is
consistent with the ones already provided by other areas of the kernel.

We are talking about one or two code lines. If we will take some
duplication search engine, it will find that major part of the kernel
is matching against it.

Newer the less, this driver provides a way to reset the counter. Why
should we drop this functionality no advantage?

To that end, I'd like to see your cnt_isr() function removed for this
patchset (you can bring it back once timestamp support is added).

Are you suggesting to enable IRQ without interrupt handler? May be i'm
missing some thing.. I do not understand it.

Reimplement your cnt_read/cnt_write() functions to instead use
kstat_irqs_usr() from <linux/kernel_stat.h> to get the current number of
interrupts the IRQ line and use it to derive your count value for this
driver.

I can follow the counter read way, but overwriting system wide counter
for local use is bad idea.

quoted

quoted

quoted

+static struct counter_signal event_cnt_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 0 signal",

You should choose a more description name for this Signal;
"Channel 0 signal" isn't very useful information for the user. Is this
signal the respective GPIO line state?

Sounds plausible. How about "Channel 0, GPIO line state"?

Ideally, this would match the GPIO name (or I suppose the IRQ number if
not a GPIO line). So in your probe() function you can do something like
this I believe:

	cnt_signals[0].name = priv->gpio->name;

to make this possible, i would need hack gpiolib framework and add
name/label exporter. But after endless rounds of pingponging me for
renaming the driver and removing interrupt handler, i feel like we are
not having serious discussion for mainlining this driver.

Is it some expensive way to prepare me for 1. April joke?

Of course, you should first check whether this is a GPIO line or IRQ
line and set the name accordingly.

Please, let's stop bike-shed for now. This driver has no limitless
budget. If there are serious problem, I would love to fix it, but if we
still discussing name of the driver or how to misuse kernel interrupt
handling, then it makes no sense to continue.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Hello William,

Here is cooled down technical answer. Excuse me for over reacting.

On Tue, Feb 23, 2021 at 11:06:56AM +0100, Oleksij Rempel wrote:

On Mon, Feb 22, 2021 at 10:43:00AM +0900, William Breathitt Gray wrote:

quoted

On Mon, Feb 15, 2021 at 10:17:37AM +0100, Oleksij Rempel wrote:

quoted

quoted

quoted

+static irqreturn_t event_cnt_isr(int irq, void *dev_id)
+{
+	struct event_cnt_priv *priv = dev_id;
+
+	atomic_inc(&priv->count);

This is just used to count the number of interrupts right? I wonder if
we can do this smarter. For example, the kernel already keeps track of
number of interrupts that has occurred for any particular IRQ line on a
CPU (see the 'kstat_irqs' member of struct irq_desc, and the
show_interrupts() function in kernel/irq/proc.c). Would it make sense to
simply store the initial interrupt count on driver load or enablement,
and then return the difference during a count_read() callback?

This driver do not makes a lot of sense without your chardev patches. As
soon as this patches go mainline, this driver will be able to send
event with a timestamp and counter state to the user space.

With other words, we will need an irq handler anyway. In this case we
can't save more RAM or CPU cycles by using system irq counters.

It's true that this driver will need an IRQ handler when the timestamp
functionality is added, but deriving the count value is different matter
regardless. There's already code in the kernel to retrieve the number of
interrupts, so it makes sense that we use that rather than rolling our
own -- at the very least to ensure the value we provide to users is
consistent with the ones already provided by other areas of the kernel.

The value provided by the driver is consistent only if it is not
overwritten by user. The driver provides an interface to reset/overwrite it.
At least after this step the value is not consistent.

We are talking about one or two code lines. If we will take some
duplication search engine, it will find that major part of the kernel
is matching against it.

Newer the less, this driver provides a way to reset the counter. Why
should we drop this functionality no advantage?

quoted

To that end, I'd like to see your cnt_isr() function removed for this
patchset (you can bring it back once timestamp support is added).

It make no sense to request an interrupt without interrupt service
routine.

https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L2072if
	if (!handler) {
		if (!thread_fn)
			return -EINVAL;

As you can see, requesting an irq need at least handler or thread_fn.

enable_irq: this will explode at least here:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L778

If he have no IRQ handler and some how was able to enable it, at
some point this IRQ will be disabled by this code:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/spurious.c#L410
	if (unlikely(desc->irqs_unhandled > 99900)) {
		/*
		 * The interrupt is stuck
		 */
		__report_bad_irq(desc, action_ret);
		/*
		 * Now kill the IRQ
		 */
		printk(KERN_EMERG "Disabling IRQ #%d\n", irq);
		desc->istate |= IRQS_SPURIOUS_DISABLED;
		desc->depth++;
		irq_disable(desc);

With current code, we can't request or enable IRQ without cnt_isr(). Not
that it is not possible, but it make no sense to me.

Are you suggesting to enable IRQ without interrupt handler? May be i'm
missing some thing.. I do not understand it.

quoted

Reimplement your cnt_read/cnt_write() functions to instead use
kstat_irqs_usr() from <linux/kernel_stat.h> to get the current number of
interrupts the IRQ line and use it to derive your count value for this
driver.

irq descriptor has 3 counters:
- irq_count: this value can be reset any time by the kernel at least by
  the note_interrupt()
- irqs_unhandled: this value is increased in case of missing irq
  handler. Or if handler has returns IRQ_NONE.
- tot_count: this value should not be reset.

Non of this values is suitable for cnt_read() and cnt_write(). Only
tot_count would be suitable if cnt_write() is removed. I do not see it
as acceptable option.

For this driver, we still need extra counter, where only this driver is
responsible for writing to it.

I can follow the counter read way, but overwriting system wide counter
for local use is bad idea.

quoted

quoted

quoted

quoted

+static struct counter_signal event_cnt_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 0 signal",

You should choose a more description name for this Signal;
"Channel 0 signal" isn't very useful information for the user. Is this
signal the respective GPIO line state?

Sounds plausible. How about "Channel 0, GPIO line state"?

Ideally, this would match the GPIO name (or I suppose the IRQ number if
not a GPIO line). So in your probe() function you can do something like
this I believe:

	cnt_signals[0].name = priv->gpio->name;

to make this possible, i would need hack gpiolib framework and add
name/label exporter. But after endless rounds of pingponging me for
renaming the driver and removing interrupt handler, i feel like we are
not having serious discussion for mainlining this driver.

Probably for good reason, struct gpio_desc was made local and is located
in the drivers/gpio/gpiolib.h. It feels like additional hack to include
it. I assume, it should be done properly so there is a function to
provide gpio name or label.

@Linus Walleij are there any good way to get the GPIO name? And which
name will be actually used? A label provided over devicetree?

If I see it correctly, it would need more work to make the kernel infrastructure
suitable for this suggestions. Some of them are only needed before
chardev support will go mainline and , in long term, not worth to
spend time on it.

Probably I do not understand you and i missing some thing?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Tue, Feb 23, 2021 at 06:45:16PM +0100, Oleksij Rempel wrote:

Hello William,

Here is cooled down technical answer. Excuse me for over reacting.

Hello Oleksij,

Let me apologize too if I offended you in some way in with previous
response, I assure you that was not my intention. I truly do believe
this is a useful driver to have in the kernel and I want to make that
happen; my concerns with your patch are purely technical in nature and 
I'm certain we can find a solution working together.

On Tue, Feb 23, 2021 at 11:06:56AM +0100, Oleksij Rempel wrote:

quoted

On Mon, Feb 22, 2021 at 10:43:00AM +0900, William Breathitt Gray wrote:

quoted

On Mon, Feb 15, 2021 at 10:17:37AM +0100, Oleksij Rempel wrote:

quoted

quoted

quoted

+static irqreturn_t event_cnt_isr(int irq, void *dev_id)
+{
+	struct event_cnt_priv *priv = dev_id;
+
+	atomic_inc(&priv->count);

This is just used to count the number of interrupts right? I wonder if
we can do this smarter. For example, the kernel already keeps track of
number of interrupts that has occurred for any particular IRQ line on a
CPU (see the 'kstat_irqs' member of struct irq_desc, and the
show_interrupts() function in kernel/irq/proc.c). Would it make sense to
simply store the initial interrupt count on driver load or enablement,
and then return the difference during a count_read() callback?

This driver do not makes a lot of sense without your chardev patches. As
soon as this patches go mainline, this driver will be able to send
event with a timestamp and counter state to the user space.

With other words, we will need an irq handler anyway. In this case we
can't save more RAM or CPU cycles by using system irq counters.

It's true that this driver will need an IRQ handler when the timestamp
functionality is added, but deriving the count value is different matter
regardless. There's already code in the kernel to retrieve the number of
interrupts, so it makes sense that we use that rather than rolling our
own -- at the very least to ensure the value we provide to users is
consistent with the ones already provided by other areas of the kernel.

The value provided by the driver is consistent only if it is not
overwritten by user. The driver provides an interface to reset/overwrite it.
At least after this step the value is not consistent.

I wasn't clear here so I apologize. What I would like is for this driver
to maintain its own local count value derived from kstat_irqs_usr(). So
for example, you can use the "count" member of your struct
interrupt_cnt_priv to maintain this value (it can be unsigned int
instead of atomic_t):

static int interrupt_cnt_read(struct counter_device *counter,
			      struct counter_count *count, unsigned long *val)
{
	struct interrupt_cnt_priv *priv = counter->priv;

	*val = kstat_irqs_usr(priv->irq) - priv->count;

	return 0;
}

static int interrupt_cnt_write(struct counter_device *counter,
			       struct counter_count *count,
			       const unsigned long val)
{
	struct interrupt_cnt_priv *priv = counter->priv;

	/* kstat_irqs_usr() returns unsigned int */
	if (val != (unsigned int)val)
		return -ERANGE;

	priv->count = val;

	return 0;
}

quoted

We are talking about one or two code lines. If we will take some
duplication search engine, it will find that major part of the kernel
is matching against it.

Newer the less, this driver provides a way to reset the counter. Why
should we drop this functionality no advantage?

quoted

To that end, I'd like to see your cnt_isr() function removed for this
patchset (you can bring it back once timestamp support is added).

It make no sense to request an interrupt without interrupt service
routine.

https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L2072if
	if (!handler) {
		if (!thread_fn)
			return -EINVAL;

As you can see, requesting an irq need at least handler or thread_fn.

enable_irq: this will explode at least here:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L778

If he have no IRQ handler and some how was able to enable it, at
some point this IRQ will be disabled by this code:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/spurious.c#L410
	if (unlikely(desc->irqs_unhandled > 99900)) {
		/*
		 * The interrupt is stuck
		 */
		__report_bad_irq(desc, action_ret);
		/*
		 * Now kill the IRQ
		 */
		printk(KERN_EMERG "Disabling IRQ #%d\n", irq);
		desc->istate |= IRQS_SPURIOUS_DISABLED;
		desc->depth++;
		irq_disable(desc);

With current code, we can't request or enable IRQ without cnt_isr(). Not
that it is not possible, but it make no sense to me.

What I'm requesting is to remove the interrupt code from this driver for
now including the cnt_enable_write() callback. Yes, we will need it when
timestamp functionality is added, but currently the Counter subsystem
does not have that functionality yet. Once the Counter character device
interface is merged, then it makes sense to add the interrupt service
routine to push timestamps to the user.

It is still useful to have this driver mainlined even without the
interrupt code: getting the body of this driver merged means a much
easier review of the timestamp code in the future, and users can start
using current Counter sysfs interface to track their GPIO interrupts.

quoted

Are you suggesting to enable IRQ without interrupt handler? May be i'm
missing some thing.. I do not understand it.

quoted

Reimplement your cnt_read/cnt_write() functions to instead use
kstat_irqs_usr() from <linux/kernel_stat.h> to get the current number of
interrupts the IRQ line and use it to derive your count value for this
driver.

irq descriptor has 3 counters:
- irq_count: this value can be reset any time by the kernel at least by
  the note_interrupt()
- irqs_unhandled: this value is increased in case of missing irq
  handler. Or if handler has returns IRQ_NONE.
- tot_count: this value should not be reset.

Non of this values is suitable for cnt_read() and cnt_write(). Only
tot_count would be suitable if cnt_write() is removed. I do not see it
as acceptable option.

For this driver, we still need extra counter, where only this driver is
responsible for writing to it.

Yes, I'm sorry for not being clear before. Please use priv->count for
this; there's no need to adjust directly the system irq count.

quoted

I can follow the counter read way, but overwriting system wide counter
for local use is bad idea.

quoted

quoted

quoted

quoted

+static struct counter_signal event_cnt_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 0 signal",

You should choose a more description name for this Signal;
"Channel 0 signal" isn't very useful information for the user. Is this
signal the respective GPIO line state?

Sounds plausible. How about "Channel 0, GPIO line state"?

Ideally, this would match the GPIO name (or I suppose the IRQ number if
not a GPIO line). So in your probe() function you can do something like
this I believe:

	cnt_signals[0].name = priv->gpio->name;

quoted

to make this possible, i would need hack gpiolib framework and add
name/label exporter. But after endless rounds of pingponging me for
renaming the driver and removing interrupt handler, i feel like we are
not having serious discussion for mainlining this driver.

Probably for good reason, struct gpio_desc was made local and is located
in the drivers/gpio/gpiolib.h. It feels like additional hack to include
it. I assume, it should be done properly so there is a function to
provide gpio name or label.

@Linus Walleij are there any good way to get the GPIO name? And which
name will be actually used? A label provided over devicetree?

Perhaps one of the GPIO subsystem maintainers can provide more guidance
here, but I was under the impression that this name was provided
statically by the respective GPIO driver via their struct gpio_chip. I
think you can see the array of names via priv->gpio->gdev->chip->names.

Alternatively, we can take a more generic approach: ignore the GPIO
names and focus solely on the IRQ lines; because the GPIO lines will
always be tied to respective IRQ lines here, using the IRQ as the basis
of the name should always be valid. The "name" member of the struct
irq_chip can work for this. I haven't tested this, but I think something
like this would work:

	cnt_signals[0].name = irq_get_chip(priv->irq)->name;

If I see it correctly, it would need more work to make the kernel infrastructure
suitable for this suggestions. Some of them are only needed before
chardev support will go mainline and , in long term, not worth to
spend time on it.

I disagree, I think there is benefit in getting this driver merged
even if we don't have the interrupt service routine. Although I
recommend we keep this initial patch simple to introduce the driver,
later on you can for example add support for other Counter sysfs
attributes such as "ceiling" and "floor" if users want to specify count
limits, or perhaps alternative count functions (maybe a user wants to
the count to decrease instead of increase with every interrupt).

These other functionalities are tangental to the your timestamp interest
for this driver, but I believe they will be useful to users at large as
a convenient way to evaluate, track, and express the interrupt counts on
their system.

William Breathitt Gray

Probably I do not understand you and i missing some thing?

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Hello William,

On Wed, Feb 24, 2021 at 11:34:06AM +0900, William Breathitt Gray wrote:

On Tue, Feb 23, 2021 at 06:45:16PM +0100, Oleksij Rempel wrote:

quoted

Hello William,

Here is cooled down technical answer. Excuse me for over reacting.

Hello Oleksij,

Let me apologize too if I offended you in some way in with previous
response, I assure you that was not my intention. I truly do believe
this is a useful driver to have in the kernel and I want to make that
happen; my concerns with your patch are purely technical in nature and 
I'm certain we can find a solution working together.

No problem :)

quoted

On Tue, Feb 23, 2021 at 11:06:56AM +0100, Oleksij Rempel wrote:

quoted

On Mon, Feb 22, 2021 at 10:43:00AM +0900, William Breathitt Gray wrote:

quoted

On Mon, Feb 15, 2021 at 10:17:37AM +0100, Oleksij Rempel wrote:

quoted

quoted

quoted

+static irqreturn_t event_cnt_isr(int irq, void *dev_id)
+{
+	struct event_cnt_priv *priv = dev_id;
+
+	atomic_inc(&priv->count);

This is just used to count the number of interrupts right? I wonder if
we can do this smarter. For example, the kernel already keeps track of
number of interrupts that has occurred for any particular IRQ line on a
CPU (see the 'kstat_irqs' member of struct irq_desc, and the
show_interrupts() function in kernel/irq/proc.c). Would it make sense to
simply store the initial interrupt count on driver load or enablement,
and then return the difference during a count_read() callback?

This driver do not makes a lot of sense without your chardev patches. As
soon as this patches go mainline, this driver will be able to send
event with a timestamp and counter state to the user space.

With other words, we will need an irq handler anyway. In this case we
can't save more RAM or CPU cycles by using system irq counters.

It's true that this driver will need an IRQ handler when the timestamp
functionality is added, but deriving the count value is different matter
regardless. There's already code in the kernel to retrieve the number of
interrupts, so it makes sense that we use that rather than rolling our
own -- at the very least to ensure the value we provide to users is
consistent with the ones already provided by other areas of the kernel.

The value provided by the driver is consistent only if it is not
overwritten by user. The driver provides an interface to reset/overwrite it.
At least after this step the value is not consistent.

I wasn't clear here so I apologize. What I would like is for this driver
to maintain its own local count value derived from kstat_irqs_usr(). So
for example, you can use the "count" member of your struct
interrupt_cnt_priv to maintain this value (it can be unsigned int
instead of atomic_t):

static int interrupt_cnt_read(struct counter_device *counter,
			      struct counter_count *count, unsigned long *val)
{
	struct interrupt_cnt_priv *priv = counter->priv;

	*val = kstat_irqs_usr(priv->irq) - priv->count;

	return 0;
}

static int interrupt_cnt_write(struct counter_device *counter,
			       struct counter_count *count,
			       const unsigned long val)
{
	struct interrupt_cnt_priv *priv = counter->priv;

	/* kstat_irqs_usr() returns unsigned int */
	if (val != (unsigned int)val)
		return -ERANGE;

	priv->count = val;

	return 0;
}

I understand this part. There is no need to spend extra CPU cycles if
the interrupt was already counted. Just read it on user request and
calculate the offset if needed.

As soon as timestamp support is available, I will need to go back to
local counter, because the kstat_irqs_usr() will take a lot more CPU
cycles compared to private counter (it sums over all CPU local
counters). So it's better to increment a single variable, then to call
kstat_irqs_usr() from interrupt handler at IRQ rate several 10 thousands
interrupts per second.

quoted

quoted

We are talking about one or two code lines. If we will take some
duplication search engine, it will find that major part of the kernel
is matching against it.

Newer the less, this driver provides a way to reset the counter. Why
should we drop this functionality no advantage?

quoted

To that end, I'd like to see your cnt_isr() function removed for this
patchset (you can bring it back once timestamp support is added).

It make no sense to request an interrupt without interrupt service
routine.

https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L2072if
	if (!handler) {
		if (!thread_fn)
			return -EINVAL;

As you can see, requesting an irq need at least handler or thread_fn.

enable_irq: this will explode at least here:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L778

If he have no IRQ handler and some how was able to enable it, at
some point this IRQ will be disabled by this code:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/spurious.c#L410
	if (unlikely(desc->irqs_unhandled > 99900)) {
		/*
		 * The interrupt is stuck
		 */
		__report_bad_irq(desc, action_ret);
		/*
		 * Now kill the IRQ
		 */
		printk(KERN_EMERG "Disabling IRQ #%d\n", irq);
		desc->istate |= IRQS_SPURIOUS_DISABLED;
		desc->depth++;
		irq_disable(desc);

With current code, we can't request or enable IRQ without cnt_isr(). Not
that it is not possible, but it make no sense to me.

What I'm requesting is to remove the interrupt code from this driver for
now including the cnt_enable_write() callback. Yes, we will need it when
timestamp functionality is added, but currently the Counter subsystem
does not have that functionality yet. Once the Counter character device
interface is merged, then it makes sense to add the interrupt service
routine to push timestamps to the user.

It is still useful to have this driver mainlined even without the
interrupt code: getting the body of this driver merged means a much
easier review of the timestamp code in the future, and users can start
using current Counter sysfs interface to track their GPIO interrupts.

This driver, even without timestamping support, can't work without
interrupt code. request_irq is the core functionality of it. The kernel
interrupt infrastructure will not enable a IRQ and it will not provide
stats without request_irq().

The minimal requirement to make it work is to call request_irq() with
a handler which will return IRQ_HANDLED value.

It makes no sense to mainline this driver without IRQ code, because it
will not work. And if we need an IRQ handler anyway, and will need local
count anyway, why should we go extra way around? :)

quoted

quoted

Are you suggesting to enable IRQ without interrupt handler? May be i'm
missing some thing.. I do not understand it.

quoted

Reimplement your cnt_read/cnt_write() functions to instead use
kstat_irqs_usr() from <linux/kernel_stat.h> to get the current number of
interrupts the IRQ line and use it to derive your count value for this
driver.

irq descriptor has 3 counters:
- irq_count: this value can be reset any time by the kernel at least by
  the note_interrupt()
- irqs_unhandled: this value is increased in case of missing irq
  handler. Or if handler has returns IRQ_NONE.
- tot_count: this value should not be reset.

Non of this values is suitable for cnt_read() and cnt_write(). Only
tot_count would be suitable if cnt_write() is removed. I do not see it
as acceptable option.

For this driver, we still need extra counter, where only this driver is
responsible for writing to it.

Yes, I'm sorry for not being clear before. Please use priv->count for
this; there's no need to adjust directly the system irq count.

quoted

quoted

I can follow the counter read way, but overwriting system wide counter
for local use is bad idea.

quoted

quoted

quoted

quoted

+static struct counter_signal event_cnt_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 0 signal",

You should choose a more description name for this Signal;
"Channel 0 signal" isn't very useful information for the user. Is this
signal the respective GPIO line state?

Sounds plausible. How about "Channel 0, GPIO line state"?

Ideally, this would match the GPIO name (or I suppose the IRQ number if
not a GPIO line). So in your probe() function you can do something like
this I believe:

	cnt_signals[0].name = priv->gpio->name;

quoted

to make this possible, i would need hack gpiolib framework and add
name/label exporter. But after endless rounds of pingponging me for
renaming the driver and removing interrupt handler, i feel like we are
not having serious discussion for mainlining this driver.

Probably for good reason, struct gpio_desc was made local and is located
in the drivers/gpio/gpiolib.h. It feels like additional hack to include
it. I assume, it should be done properly so there is a function to
provide gpio name or label.

@Linus Walleij are there any good way to get the GPIO name? And which
name will be actually used? A label provided over devicetree?

Perhaps one of the GPIO subsystem maintainers can provide more guidance
here, but I was under the impression that this name was provided
statically by the respective GPIO driver via their struct gpio_chip. I
think you can see the array of names via priv->gpio->gdev->chip->names.

Alternatively, we can take a more generic approach: ignore the GPIO
names and focus solely on the IRQ lines; because the GPIO lines will
always be tied to respective IRQ lines here, using the IRQ as the basis
of the name should always be valid. The "name" member of the struct
irq_chip can work for this. I haven't tested this, but I think something
like this would work:

	cnt_signals[0].name = irq_get_chip(priv->irq)->name;

ok, i'll take a look at it.

quoted

If I see it correctly, it would need more work to make the kernel infrastructure
suitable for this suggestions. Some of them are only needed before
chardev support will go mainline and , in long term, not worth to
spend time on it.

I disagree, I think there is benefit in getting this driver merged
even if we don't have the interrupt service routine. Although I
recommend we keep this initial patch simple to introduce the driver,
later on you can for example add support for other Counter sysfs
attributes such as "ceiling" and "floor" if users want to specify count
limits, or perhaps alternative count functions (maybe a user wants to
the count to decrease instead of increase with every interrupt).

These other functionalities are tangental to the your timestamp interest
for this driver, but I believe they will be useful to users at large as
a convenient way to evaluate, track, and express the interrupt counts on
their system.

William Breathitt Gray

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Wed, Feb 24, 2021 at 08:35:06AM +0100, Oleksij Rempel wrote:

On Wed, Feb 24, 2021 at 11:34:06AM +0900, William Breathitt Gray wrote:

quoted

On Tue, Feb 23, 2021 at 06:45:16PM +0100, Oleksij Rempel wrote:

quoted

On Tue, Feb 23, 2021 at 11:06:56AM +0100, Oleksij Rempel wrote:

quoted

On Mon, Feb 22, 2021 at 10:43:00AM +0900, William Breathitt Gray wrote:

quoted

On Mon, Feb 15, 2021 at 10:17:37AM +0100, Oleksij Rempel wrote:

quoted

quoted

quoted

+static irqreturn_t event_cnt_isr(int irq, void *dev_id)
+{
+	struct event_cnt_priv *priv = dev_id;
+
+	atomic_inc(&priv->count);

This is just used to count the number of interrupts right? I wonder if
we can do this smarter. For example, the kernel already keeps track of
number of interrupts that has occurred for any particular IRQ line on a
CPU (see the 'kstat_irqs' member of struct irq_desc, and the
show_interrupts() function in kernel/irq/proc.c). Would it make sense to
simply store the initial interrupt count on driver load or enablement,
and then return the difference during a count_read() callback?

This driver do not makes a lot of sense without your chardev patches. As
soon as this patches go mainline, this driver will be able to send
event with a timestamp and counter state to the user space.

With other words, we will need an irq handler anyway. In this case we
can't save more RAM or CPU cycles by using system irq counters.

It's true that this driver will need an IRQ handler when the timestamp
functionality is added, but deriving the count value is different matter
regardless. There's already code in the kernel to retrieve the number of
interrupts, so it makes sense that we use that rather than rolling our
own -- at the very least to ensure the value we provide to users is
consistent with the ones already provided by other areas of the kernel.

The value provided by the driver is consistent only if it is not
overwritten by user. The driver provides an interface to reset/overwrite it.
At least after this step the value is not consistent.

I wasn't clear here so I apologize. What I would like is for this driver
to maintain its own local count value derived from kstat_irqs_usr(). So
for example, you can use the "count" member of your struct
interrupt_cnt_priv to maintain this value (it can be unsigned int
instead of atomic_t):

static int interrupt_cnt_read(struct counter_device *counter,
			      struct counter_count *count, unsigned long *val)
{
	struct interrupt_cnt_priv *priv = counter->priv;

	*val = kstat_irqs_usr(priv->irq) - priv->count;

	return 0;
}

static int interrupt_cnt_write(struct counter_device *counter,
			       struct counter_count *count,
			       const unsigned long val)
{
	struct interrupt_cnt_priv *priv = counter->priv;

	/* kstat_irqs_usr() returns unsigned int */
	if (val != (unsigned int)val)
		return -ERANGE;

	priv->count = val;

	return 0;
}

I understand this part. There is no need to spend extra CPU cycles if
the interrupt was already counted. Just read it on user request and
calculate the offset if needed.

As soon as timestamp support is available, I will need to go back to
local counter, because the kstat_irqs_usr() will take a lot more CPU
cycles compared to private counter (it sums over all CPU local
counters). So it's better to increment a single variable, then to call
kstat_irqs_usr() from interrupt handler at IRQ rate several 10 thousands
interrupts per second.

All right, I see what you mean. Let's keep your current implementation
then.

quoted

quoted

quoted

We are talking about one or two code lines. If we will take some
duplication search engine, it will find that major part of the kernel
is matching against it.

Newer the less, this driver provides a way to reset the counter. Why
should we drop this functionality no advantage?

quoted

To that end, I'd like to see your cnt_isr() function removed for this
patchset (you can bring it back once timestamp support is added).

It make no sense to request an interrupt without interrupt service
routine.

https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L2072if
	if (!handler) {
		if (!thread_fn)
			return -EINVAL;

As you can see, requesting an irq need at least handler or thread_fn.

enable_irq: this will explode at least here:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L778

If he have no IRQ handler and some how was able to enable it, at
some point this IRQ will be disabled by this code:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/spurious.c#L410
	if (unlikely(desc->irqs_unhandled > 99900)) {
		/*
		 * The interrupt is stuck
		 */
		__report_bad_irq(desc, action_ret);
		/*
		 * Now kill the IRQ
		 */
		printk(KERN_EMERG "Disabling IRQ #%d\n", irq);
		desc->istate |= IRQS_SPURIOUS_DISABLED;
		desc->depth++;
		irq_disable(desc);

With current code, we can't request or enable IRQ without cnt_isr(). Not
that it is not possible, but it make no sense to me.

What I'm requesting is to remove the interrupt code from this driver for
now including the cnt_enable_write() callback. Yes, we will need it when
timestamp functionality is added, but currently the Counter subsystem
does not have that functionality yet. Once the Counter character device
interface is merged, then it makes sense to add the interrupt service
routine to push timestamps to the user.

It is still useful to have this driver mainlined even without the
interrupt code: getting the body of this driver merged means a much
easier review of the timestamp code in the future, and users can start
using current Counter sysfs interface to track their GPIO interrupts.

This driver, even without timestamping support, can't work without
interrupt code. request_irq is the core functionality of it. The kernel
interrupt infrastructure will not enable a IRQ and it will not provide
stats without request_irq().

The minimal requirement to make it work is to call request_irq() with
a handler which will return IRQ_HANDLED value.

It makes no sense to mainline this driver without IRQ code, because it
will not work. And if we need an IRQ handler anyway, and will need local
count anyway, why should we go extra way around? :)

Ack.

quoted

quoted

quoted

Are you suggesting to enable IRQ without interrupt handler? May be i'm
missing some thing.. I do not understand it.

quoted

Reimplement your cnt_read/cnt_write() functions to instead use
kstat_irqs_usr() from <linux/kernel_stat.h> to get the current number of
interrupts the IRQ line and use it to derive your count value for this
driver.

irq descriptor has 3 counters:
- irq_count: this value can be reset any time by the kernel at least by
  the note_interrupt()
- irqs_unhandled: this value is increased in case of missing irq
  handler. Or if handler has returns IRQ_NONE.
- tot_count: this value should not be reset.

Non of this values is suitable for cnt_read() and cnt_write(). Only
tot_count would be suitable if cnt_write() is removed. I do not see it
as acceptable option.

For this driver, we still need extra counter, where only this driver is
responsible for writing to it.

Yes, I'm sorry for not being clear before. Please use priv->count for
this; there's no need to adjust directly the system irq count.

quoted

quoted

I can follow the counter read way, but overwriting system wide counter
for local use is bad idea.

quoted

quoted

quoted

quoted

+static struct counter_signal event_cnt_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 0 signal",

You should choose a more description name for this Signal;
"Channel 0 signal" isn't very useful information for the user. Is this
signal the respective GPIO line state?

Sounds plausible. How about "Channel 0, GPIO line state"?

Ideally, this would match the GPIO name (or I suppose the IRQ number if
not a GPIO line). So in your probe() function you can do something like
this I believe:

	cnt_signals[0].name = priv->gpio->name;

quoted

to make this possible, i would need hack gpiolib framework and add
name/label exporter. But after endless rounds of pingponging me for
renaming the driver and removing interrupt handler, i feel like we are
not having serious discussion for mainlining this driver.

Probably for good reason, struct gpio_desc was made local and is located
in the drivers/gpio/gpiolib.h. It feels like additional hack to include
it. I assume, it should be done properly so there is a function to
provide gpio name or label.

@Linus Walleij are there any good way to get the GPIO name? And which
name will be actually used? A label provided over devicetree?

Perhaps one of the GPIO subsystem maintainers can provide more guidance
here, but I was under the impression that this name was provided
statically by the respective GPIO driver via their struct gpio_chip. I
think you can see the array of names via priv->gpio->gdev->chip->names.

Alternatively, we can take a more generic approach: ignore the GPIO
names and focus solely on the IRQ lines; because the GPIO lines will
always be tied to respective IRQ lines here, using the IRQ as the basis
of the name should always be valid. The "name" member of the struct
irq_chip can work for this. I haven't tested this, but I think something
like this would work:

	cnt_signals[0].name = irq_get_chip(priv->irq)->name;

ok, i'll take a look at it.

If that doesn't work, then use devm_kasprintf() to generate the name
based on the IRQ line number. The idea here is that the user should be
able to identify that the Signal component for this Count is the
respective IRQ.

William Breathitt Gray

On Wed, Feb 24, 2021 at 05:11:03PM +0900, William Breathitt Gray wrote:

On Wed, Feb 24, 2021 at 08:35:06AM +0100, Oleksij Rempel wrote:

quoted

On Wed, Feb 24, 2021 at 11:34:06AM +0900, William Breathitt Gray wrote:

quoted

Alternatively, we can take a more generic approach: ignore the GPIO
names and focus solely on the IRQ lines; because the GPIO lines will
always be tied to respective IRQ lines here, using the IRQ as the basis
of the name should always be valid. The "name" member of the struct
irq_chip can work for this. I haven't tested this, but I think something
like this would work:

	cnt_signals[0].name = irq_get_chip(priv->irq)->name;

ok, i'll take a look at it.

If that doesn't work, then use devm_kasprintf() to generate the name
based on the IRQ line number. The idea here is that the user should be
able to identify that the Signal component for this Count is the
respective IRQ.

William Breathitt Gray

I realized that these irq_chip names are often just the device name
which isn't very useful either. :-(

In that case, I suppose we really are just left with generating the name
based on the IRQ line number then. This should be fine then:

	cnt_signals[0].name = devm_kasprintf(dev, GFP_KERNEL, "IRQ %d",
					     priv->irq);
	if (!cnt_signals[0].name)
		return -ENOMEM;

I think this would make it clear to the user that this Signal is the
respective IRQ (whether sourced from GPIO or not).

William Breathitt Gray

On Wed, Feb 24, 2021 at 05:20:21PM +0900, William Breathitt Gray wrote:

On Wed, Feb 24, 2021 at 05:11:03PM +0900, William Breathitt Gray wrote:

quoted

On Wed, Feb 24, 2021 at 08:35:06AM +0100, Oleksij Rempel wrote:

quoted

On Wed, Feb 24, 2021 at 11:34:06AM +0900, William Breathitt Gray wrote:

quoted

Alternatively, we can take a more generic approach: ignore the GPIO
names and focus solely on the IRQ lines; because the GPIO lines will
always be tied to respective IRQ lines here, using the IRQ as the basis
of the name should always be valid. The "name" member of the struct
irq_chip can work for this. I haven't tested this, but I think something
like this would work:

	cnt_signals[0].name = irq_get_chip(priv->irq)->name;

ok, i'll take a look at it.

If that doesn't work, then use devm_kasprintf() to generate the name
based on the IRQ line number. The idea here is that the user should be
able to identify that the Signal component for this Count is the
respective IRQ.

William Breathitt Gray

I realized that these irq_chip names are often just the device name
which isn't very useful either. :-(

In that case, I suppose we really are just left with generating the name
based on the IRQ line number then. This should be fine then:

	cnt_signals[0].name = devm_kasprintf(dev, GFP_KERNEL, "IRQ %d",
					     priv->irq);
	if (!cnt_signals[0].name)
		return -ENOMEM;

I think this would make it clear to the user that this Signal is the
respective IRQ (whether sourced from GPIO or not).

ack, with one correction. cnt_signals should be allocated, otherwise
this value will be set per driver not per device.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

On Fri, Feb 26, 2021 at 07:46:01AM +0100, Oleksij Rempel wrote:

On Wed, Feb 24, 2021 at 05:20:21PM +0900, William Breathitt Gray wrote:

quoted

On Wed, Feb 24, 2021 at 05:11:03PM +0900, William Breathitt Gray wrote:

quoted

On Wed, Feb 24, 2021 at 08:35:06AM +0100, Oleksij Rempel wrote:

quoted

On Wed, Feb 24, 2021 at 11:34:06AM +0900, William Breathitt Gray wrote:

quoted

Alternatively, we can take a more generic approach: ignore the GPIO
names and focus solely on the IRQ lines; because the GPIO lines will
always be tied to respective IRQ lines here, using the IRQ as the basis
of the name should always be valid. The "name" member of the struct
irq_chip can work for this. I haven't tested this, but I think something
like this would work:

	cnt_signals[0].name = irq_get_chip(priv->irq)->name;

ok, i'll take a look at it.

If that doesn't work, then use devm_kasprintf() to generate the name
based on the IRQ line number. The idea here is that the user should be
able to identify that the Signal component for this Count is the
respective IRQ.

William Breathitt Gray

I realized that these irq_chip names are often just the device name
which isn't very useful either. :-(

In that case, I suppose we really are just left with generating the name
based on the IRQ line number then. This should be fine then:

	cnt_signals[0].name = devm_kasprintf(dev, GFP_KERNEL, "IRQ %d",
					     priv->irq);
	if (!cnt_signals[0].name)
		return -ENOMEM;

I think this would make it clear to the user that this Signal is the
respective IRQ (whether sourced from GPIO or not).

ack, with one correction. cnt_signals should be allocated, otherwise
this value will be set per driver not per device.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Yes you're right, cnt_signals will need to be allocated then because
these will be different per device.

Thanks,

William Breathitt Gray

On Wed, Feb 24, 2021 at 3:34 AM William Breathitt Gray
[off-list ref] wrote:

On Tue, Feb 23, 2021 at 06:45:16PM +0100, Oleksij Rempel wrote:

quoted

quoted

to make this possible, i would need hack gpiolib framework and add
name/label exporter. But after endless rounds of pingponging me for
renaming the driver and removing interrupt handler, i feel like we are
not having serious discussion for mainlining this driver.

Probably for good reason, struct gpio_desc was made local and is located
in the drivers/gpio/gpiolib.h. It feels like additional hack to include
it. I assume, it should be done properly so there is a function to
provide gpio name or label.

@Linus Walleij are there any good way to get the GPIO name? And which
name will be actually used? A label provided over devicetree?

Perhaps one of the GPIO subsystem maintainers can provide more guidance
here, but I was under the impression that this name was provided
statically by the respective GPIO driver via their struct gpio_chip. I
think you can see the array of names via priv->gpio->gdev->chip->names.

These names can be set either through device properties on the
GPIO chip "line-names" such as through device tree, or as static
names in the .names array on struct gpio_chip for chips that
are e.g. hot-pluggable and does not have a hardware
description associated. These names should be something
like what the signal is called on the circuit board rail.

gpiolib further has a function:
gpiod_set_consumer_name() that can be used by consumers to
set their use case for the line, which makes it appear in debugfs
etc. The consumer name does not need to be unique.

These names have no practical use other than debugging or
userspace representation.

I hope this helps.

Yours,
Linus Walleij