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