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 > Reviewed-by: Ahmad Fatoum 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. > --- > 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 > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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? > + > + 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 "); > +MODULE_DESCRIPTION("Event counter driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.30.0 >