Re: [PATCH v5 2/2] counter: add IRQ or GPIO based event counter

[William Breathitt Gray]

On Mon, Feb 15, 2021 at 10:17:37AM +0100, Oleksij Rempel wrote:
> > > +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.

> > > +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

Attachment: signature.asc
Description: PGP signature