| Message ID | 20210226090830.10927-3-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | add support for GPIO or IRQ based evemt counter | expand |
On Fri, Feb 26, 2021 at 10:08:30AM +0100, Oleksij Rempel wrote: > Add simple IRQ or GPIO base 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, We're almost there, but I spotted a couple of mistakes below. > --- > MAINTAINERS | 7 + > drivers/counter/Kconfig | 10 ++ > drivers/counter/Makefile | 1 + > drivers/counter/interrupt-cnt.c | 243 ++++++++++++++++++++++++++++++++ > 4 files changed, 261 insertions(+) > create mode 100644 drivers/counter/interrupt-cnt.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a50a543e3c81..ad0a4455afec 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9217,6 +9217,13 @@ F: include/dt-bindings/interconnect/ > F: include/linux/interconnect-provider.h > F: include/linux/interconnect.h > > +INTERRUPT COUNTER DRIVER > +M: Oleksij Rempel <o.rempel@pengutronix.de> > +R: Pengutronix Kernel Team <kernel@pengutronix.de> > +L: linux-iio@vger.kernel.org > +F: Documentation/devicetree/bindings/counter/interrupt-counter.yaml > +F: drivers/counter/interrupt-cnt.c > + > INVENSENSE ICM-426xx IMU DRIVER > M: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> > L: linux-iio@vger.kernel.org > diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig > index 2de53ab0dd25..dcad13229134 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 INTERRUPT_CNT > + tristate "Interrupt counter driver" > + depends on GPIOLIB > + help > + Select this option to enable interrupt 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 interrupt-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..cb646ed2f039 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_INTERRUPT_CNT) += interrupt-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/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c > new file mode 100644 > index 000000000000..550383b6b591 > --- /dev/null > +++ b/drivers/counter/interrupt-cnt.c > @@ -0,0 +1,243 @@ > +// 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 INTERRUPT_CNT_NAME "interrupt-cnt" > + > +struct interrupt_cnt_priv { > + atomic_t count; > + struct counter_device counter; > + struct gpio_desc *gpio; > + int irq; > + bool enabled; > + struct counter_signal signals; > + struct counter_synapse synapses; > + struct counter_count cnts; > +}; > + > +static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id) > +{ > + struct interrupt_cnt_priv *priv = dev_id; > + > + atomic_inc(&priv->count); > + > + return IRQ_HANDLED; > +} > + > +static ssize_t interrupt_cnt_enable_read(struct counter_device *counter, > + struct counter_count *count, > + void *private, char *buf) > +{ > + struct interrupt_cnt_priv *priv = counter->priv; > + > + return sysfs_emit(buf, "%d\n", priv->enabled); > +} > + > +static ssize_t interrupt_cnt_enable_write(struct counter_device *counter, > + struct counter_count *count, > + void *private, const char *buf, > + size_t len) > +{ > + struct interrupt_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 = true; > + enable_irq(priv->irq); > + } else { > + disable_irq(priv->irq); > + priv->enabled = false; > + } > + > + return len; > +} > + > +static const struct counter_count_ext interrupt_cnt_ext[] = { > + { > + .name = "enable", > + .read = interrupt_cnt_enable_read, > + .write = interrupt_cnt_enable_write, > + }, > +}; > + > +static enum counter_synapse_action interrupt_cnt_synapse_actionss[] = { > + COUNTER_SYNAPSE_ACTION_RISING_EDGE, > +}; > + > +static int interrupt_cnt_action_get(struct counter_device *counter, > + struct counter_count *count, > + struct counter_synapse *synapse, > + size_t *action) > +{ > + *action = interrupt_cnt_synapse_actionss[0]; This needs to be set to the index of the element, not the value: *action = 0; > + > + return 0; > +} > + > +static int interrupt_cnt_read(struct counter_device *counter, > + struct counter_count *count, unsigned long *val) > +{ > + struct interrupt_cnt_priv *priv = counter->priv; > + > + *val = atomic_read(&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; > + > + atomic_set(&priv->count, val); > + > + return 0; > +} > + > +static enum counter_count_function interrupt_cnt_functions[] = { > + COUNTER_COUNT_FUNCTION_INCREASE, > +}; > + > +static int interrupt_cnt_function_get(struct counter_device *counter, > + struct counter_count *count, > + size_t *function) > +{ > + *function = interrupt_cnt_functions[0]; Same problem as action_get(); needs to be index, not value: *function = 0; > + > + return 0; > +} > + > +static int interrupt_cnt_signal_read(struct counter_device *counter, > + struct counter_signal *signal, > + enum counter_signal_value *val) > +{ > + struct interrupt_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 const struct counter_ops interrupt_cnt_ops = { > + .action_get = interrupt_cnt_action_get, > + .count_read = interrupt_cnt_read, > + .count_write = interrupt_cnt_write, > + .function_get = interrupt_cnt_function_get, > + .signal_read = interrupt_cnt_signal_read, > +}; > + > +static int interrupt_cnt_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct interrupt_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; > + } > + > + if (priv->gpio) { This if statement can be removed. There's no need to restrict this to just GPIO because we're always dealing with an IRQ, so allocate the "IRQ #" name unconditionally and set signals/num_signals. William Breathitt Gray > + priv->signals.name = devm_kasprintf(dev, GFP_KERNEL, "IRQ %d", > + priv->irq); > + if (!priv->signals.name) > + return -ENOMEM; > + > + priv->counter.signals = &priv->signals; > + priv->counter.num_signals = 1; > + } > + > + priv->synapses.actions_list = interrupt_cnt_synapse_actionss; > + priv->synapses.num_actions = ARRAY_SIZE(interrupt_cnt_synapse_actionss); > + priv->synapses.signal = &priv->signals; > + > + priv->cnts.name = "Channel 0 Count"; > + priv->cnts.functions_list = interrupt_cnt_functions; > + priv->cnts.num_functions = ARRAY_SIZE(interrupt_cnt_functions); > + priv->cnts.synapses = &priv->synapses; > + priv->cnts.num_synapses = 1; > + priv->cnts.ext = interrupt_cnt_ext; > + priv->cnts.num_ext = ARRAY_SIZE(interrupt_cnt_ext); > + > + priv->counter.priv = priv; > + priv->counter.name = dev_name(dev); > + priv->counter.parent = dev; > + priv->counter.ops = &interrupt_cnt_ops; > + priv->counter.counts = &priv->cnts; > + priv->counter.num_counts = 1; > + > + irq_set_status_flags(priv->irq, IRQ_NOAUTOEN); > + ret = devm_request_irq(dev, priv->irq, interrupt_cnt_isr, > + IRQF_TRIGGER_RISING | IRQF_NO_THREAD, > + dev_name(dev), priv); > + if (ret) > + return ret; > + > + return devm_counter_register(dev, &priv->counter); > +} > + > +static const struct of_device_id interrupt_cnt_of_match[] = { > + { .compatible = "interrupt-counter", }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, interrupt_cnt_of_match); > + > +static struct platform_driver interrupt_cnt_driver = { > + .probe = interrupt_cnt_probe, > + .driver = { > + .name = INTERRUPT_CNT_NAME, > + .of_match_table = interrupt_cnt_of_match, > + }, > +}; > +module_platform_driver(interrupt_cnt_driver); > + > +MODULE_ALIAS("platform:interrupt-counter"); > +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>"); > +MODULE_DESCRIPTION("Interrupt counter driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.29.2 >
Hi, On Fri, Feb 26, 2021 at 06:45:20PM +0900, William Breathitt Gray wrote: > On Fri, Feb 26, 2021 at 10:08:30AM +0100, Oleksij Rempel wrote: > > Add simple IRQ or GPIO base 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, > > We're almost there, but I spotted a couple of mistakes below. > > > --- > > MAINTAINERS | 7 + > > drivers/counter/Kconfig | 10 ++ > > drivers/counter/Makefile | 1 + > > drivers/counter/interrupt-cnt.c | 243 ++++++++++++++++++++++++++++++++ > > 4 files changed, 261 insertions(+) > > create mode 100644 drivers/counter/interrupt-cnt.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index a50a543e3c81..ad0a4455afec 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -9217,6 +9217,13 @@ F: include/dt-bindings/interconnect/ > > F: include/linux/interconnect-provider.h > > F: include/linux/interconnect.h > > > > +INTERRUPT COUNTER DRIVER > > +M: Oleksij Rempel <o.rempel@pengutronix.de> > > +R: Pengutronix Kernel Team <kernel@pengutronix.de> > > +L: linux-iio@vger.kernel.org > > +F: Documentation/devicetree/bindings/counter/interrupt-counter.yaml > > +F: drivers/counter/interrupt-cnt.c > > + > > INVENSENSE ICM-426xx IMU DRIVER > > M: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> > > L: linux-iio@vger.kernel.org > > diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig > > index 2de53ab0dd25..dcad13229134 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 INTERRUPT_CNT > > + tristate "Interrupt counter driver" > > + depends on GPIOLIB > > + help > > + Select this option to enable interrupt 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 interrupt-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..cb646ed2f039 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_INTERRUPT_CNT) += interrupt-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/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c > > new file mode 100644 > > index 000000000000..550383b6b591 > > --- /dev/null > > +++ b/drivers/counter/interrupt-cnt.c > > @@ -0,0 +1,243 @@ > > +// 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 INTERRUPT_CNT_NAME "interrupt-cnt" > > + > > +struct interrupt_cnt_priv { > > + atomic_t count; > > + struct counter_device counter; > > + struct gpio_desc *gpio; > > + int irq; > > + bool enabled; > > + struct counter_signal signals; > > + struct counter_synapse synapses; > > + struct counter_count cnts; > > +}; > > + > > +static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id) > > +{ > > + struct interrupt_cnt_priv *priv = dev_id; > > + > > + atomic_inc(&priv->count); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static ssize_t interrupt_cnt_enable_read(struct counter_device *counter, > > + struct counter_count *count, > > + void *private, char *buf) > > +{ > > + struct interrupt_cnt_priv *priv = counter->priv; > > + > > + return sysfs_emit(buf, "%d\n", priv->enabled); > > +} > > + > > +static ssize_t interrupt_cnt_enable_write(struct counter_device *counter, > > + struct counter_count *count, > > + void *private, const char *buf, > > + size_t len) > > +{ > > + struct interrupt_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 = true; > > + enable_irq(priv->irq); > > + } else { > > + disable_irq(priv->irq); > > + priv->enabled = false; > > + } > > + > > + return len; > > +} > > + > > +static const struct counter_count_ext interrupt_cnt_ext[] = { > > + { > > + .name = "enable", > > + .read = interrupt_cnt_enable_read, > > + .write = interrupt_cnt_enable_write, > > + }, > > +}; > > + > > +static enum counter_synapse_action interrupt_cnt_synapse_actionss[] = { > > + COUNTER_SYNAPSE_ACTION_RISING_EDGE, > > +}; > > + > > +static int interrupt_cnt_action_get(struct counter_device *counter, > > + struct counter_count *count, > > + struct counter_synapse *synapse, > > + size_t *action) > > +{ > > + *action = interrupt_cnt_synapse_actionss[0]; > > This needs to be set to the index of the element, not the value: > > *action = 0; > > > + > > + return 0; > > +} > > + > > +static int interrupt_cnt_read(struct counter_device *counter, > > + struct counter_count *count, unsigned long *val) > > +{ > > + struct interrupt_cnt_priv *priv = counter->priv; > > + > > + *val = atomic_read(&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; > > + > > + atomic_set(&priv->count, val); > > + > > + return 0; > > +} > > + > > +static enum counter_count_function interrupt_cnt_functions[] = { > > + COUNTER_COUNT_FUNCTION_INCREASE, > > +}; > > + > > +static int interrupt_cnt_function_get(struct counter_device *counter, > > + struct counter_count *count, > > + size_t *function) > > +{ > > + *function = interrupt_cnt_functions[0]; > > Same problem as action_get(); needs to be index, not value: > > *function = 0; ok > > + > > + return 0; > > +} > > + > > +static int interrupt_cnt_signal_read(struct counter_device *counter, > > + struct counter_signal *signal, > > + enum counter_signal_value *val) > > +{ > > + struct interrupt_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 const struct counter_ops interrupt_cnt_ops = { > > + .action_get = interrupt_cnt_action_get, > > + .count_read = interrupt_cnt_read, > > + .count_write = interrupt_cnt_write, > > + .function_get = interrupt_cnt_function_get, > > + .signal_read = interrupt_cnt_signal_read, > > +}; > > + > > +static int interrupt_cnt_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct interrupt_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; > > + } > > + > > + if (priv->gpio) { > > This if statement can be removed. There's no need to restrict this to > just GPIO because we're always dealing with an IRQ, so allocate the > "IRQ #" name unconditionally and set signals/num_signals. Your previous suggestion was to no assign signals if there is no gpios. What should I do? Regards, Oleksij
On Fri, Feb 26, 2021 at 01:14:55PM +0100, Oleksij Rempel wrote: > On Fri, Feb 26, 2021 at 06:45:20PM +0900, William Breathitt Gray wrote: > > On Fri, Feb 26, 2021 at 10:08:30AM +0100, Oleksij Rempel wrote: > > > +static int interrupt_cnt_signal_read(struct counter_device *counter, > > > + struct counter_signal *signal, > > > + enum counter_signal_value *val) > > > +{ > > > + struct interrupt_cnt_priv *priv = counter->priv; > > > + int ret; I forgot about this function. Add a check here to return -EINVAL if we're not dealing with a GPIO: if (!priv->gpio) return -EINVAL; > > > + > > > + ret = gpiod_get_value(priv->gpio); > > > + if (ret < 0) > > > + return ret; > > > + > > > + *val = ret ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW; > > > + > > > + return 0; > > > +} > > > + > > > +static const struct counter_ops interrupt_cnt_ops = { > > > + .action_get = interrupt_cnt_action_get, > > > + .count_read = interrupt_cnt_read, > > > + .count_write = interrupt_cnt_write, > > > + .function_get = interrupt_cnt_function_get, > > > + .signal_read = interrupt_cnt_signal_read, > > > +}; > > > + > > > +static int interrupt_cnt_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + struct interrupt_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; > > > + } > > > + > > > + if (priv->gpio) { > > > > This if statement can be removed. There's no need to restrict this to > > just GPIO because we're always dealing with an IRQ, so allocate the > > "IRQ #" name unconditionally and set signals/num_signals. > > Your previous suggestion was to no assign signals if there is no gpios. > What should I do? > > 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 | I'm sorry for not being clear. I'm saying there is no need to differentiate here because there will always be a respective IRQ line whether there is a GPIO line or not. So removing the if statement is all you need to do. Instead of: if (priv->gpio) { priv->signals.name = devm_kasprintf(dev, GFP_KERNEL, "IRQ %d", priv->irq); if (!priv->signals.name) return -ENOMEM; priv->counter.signals = &priv->signals; priv->counter.num_signals = 1; } priv->synapses.actions_list = interrupt_cnt_synapse_actionss; priv->synapses.num_actions = ARRAY_SIZE(interrupt_cnt_synapse_actionss); priv->synapses.signal = &priv->signals; ... You can just have those lines execute unconditionally even if there are no gpios: priv->signals.name = devm_kasprintf(dev, GFP_KERNEL, "IRQ %d", priv->irq); if (!priv->signals.name) return -ENOMEM; priv->counter.signals = &priv->signals; priv->counter.num_signals = 1; priv->synapses.actions_list = interrupt_cnt_synapse_actionss; priv->synapses.num_actions = ARRAY_SIZE(interrupt_cnt_synapse_actionss); priv->synapses.signal = &priv->signals; ... William Breathitt Gray
diff --git a/MAINTAINERS b/MAINTAINERS index a50a543e3c81..ad0a4455afec 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -9217,6 +9217,13 @@ F: include/dt-bindings/interconnect/ F: include/linux/interconnect-provider.h F: include/linux/interconnect.h +INTERRUPT COUNTER DRIVER +M: Oleksij Rempel <o.rempel@pengutronix.de> +R: Pengutronix Kernel Team <kernel@pengutronix.de> +L: linux-iio@vger.kernel.org +F: Documentation/devicetree/bindings/counter/interrupt-counter.yaml +F: drivers/counter/interrupt-cnt.c + INVENSENSE ICM-426xx IMU DRIVER M: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> L: linux-iio@vger.kernel.org diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig index 2de53ab0dd25..dcad13229134 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 INTERRUPT_CNT + tristate "Interrupt counter driver" + depends on GPIOLIB + help + Select this option to enable interrupt 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 interrupt-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..cb646ed2f039 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_INTERRUPT_CNT) += interrupt-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/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c new file mode 100644 index 000000000000..550383b6b591 --- /dev/null +++ b/drivers/counter/interrupt-cnt.c @@ -0,0 +1,243 @@ +// 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 INTERRUPT_CNT_NAME "interrupt-cnt" + +struct interrupt_cnt_priv { + atomic_t count; + struct counter_device counter; + struct gpio_desc *gpio; + int irq; + bool enabled; + struct counter_signal signals; + struct counter_synapse synapses; + struct counter_count cnts; +}; + +static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id) +{ + struct interrupt_cnt_priv *priv = dev_id; + + atomic_inc(&priv->count); + + return IRQ_HANDLED; +} + +static ssize_t interrupt_cnt_enable_read(struct counter_device *counter, + struct counter_count *count, + void *private, char *buf) +{ + struct interrupt_cnt_priv *priv = counter->priv; + + return sysfs_emit(buf, "%d\n", priv->enabled); +} + +static ssize_t interrupt_cnt_enable_write(struct counter_device *counter, + struct counter_count *count, + void *private, const char *buf, + size_t len) +{ + struct interrupt_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 = true; + enable_irq(priv->irq); + } else { + disable_irq(priv->irq); + priv->enabled = false; + } + + return len; +} + +static const struct counter_count_ext interrupt_cnt_ext[] = { + { + .name = "enable", + .read = interrupt_cnt_enable_read, + .write = interrupt_cnt_enable_write, + }, +}; + +static enum counter_synapse_action interrupt_cnt_synapse_actionss[] = { + COUNTER_SYNAPSE_ACTION_RISING_EDGE, +}; + +static int interrupt_cnt_action_get(struct counter_device *counter, + struct counter_count *count, + struct counter_synapse *synapse, + size_t *action) +{ + *action = interrupt_cnt_synapse_actionss[0]; + + return 0; +} + +static int interrupt_cnt_read(struct counter_device *counter, + struct counter_count *count, unsigned long *val) +{ + struct interrupt_cnt_priv *priv = counter->priv; + + *val = atomic_read(&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; + + atomic_set(&priv->count, val); + + return 0; +} + +static enum counter_count_function interrupt_cnt_functions[] = { + COUNTER_COUNT_FUNCTION_INCREASE, +}; + +static int interrupt_cnt_function_get(struct counter_device *counter, + struct counter_count *count, + size_t *function) +{ + *function = interrupt_cnt_functions[0]; + + return 0; +} + +static int interrupt_cnt_signal_read(struct counter_device *counter, + struct counter_signal *signal, + enum counter_signal_value *val) +{ + struct interrupt_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 const struct counter_ops interrupt_cnt_ops = { + .action_get = interrupt_cnt_action_get, + .count_read = interrupt_cnt_read, + .count_write = interrupt_cnt_write, + .function_get = interrupt_cnt_function_get, + .signal_read = interrupt_cnt_signal_read, +}; + +static int interrupt_cnt_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct interrupt_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; + } + + if (priv->gpio) { + priv->signals.name = devm_kasprintf(dev, GFP_KERNEL, "IRQ %d", + priv->irq); + if (!priv->signals.name) + return -ENOMEM; + + priv->counter.signals = &priv->signals; + priv->counter.num_signals = 1; + } + + priv->synapses.actions_list = interrupt_cnt_synapse_actionss; + priv->synapses.num_actions = ARRAY_SIZE(interrupt_cnt_synapse_actionss); + priv->synapses.signal = &priv->signals; + + priv->cnts.name = "Channel 0 Count"; + priv->cnts.functions_list = interrupt_cnt_functions; + priv->cnts.num_functions = ARRAY_SIZE(interrupt_cnt_functions); + priv->cnts.synapses = &priv->synapses; + priv->cnts.num_synapses = 1; + priv->cnts.ext = interrupt_cnt_ext; + priv->cnts.num_ext = ARRAY_SIZE(interrupt_cnt_ext); + + priv->counter.priv = priv; + priv->counter.name = dev_name(dev); + priv->counter.parent = dev; + priv->counter.ops = &interrupt_cnt_ops; + priv->counter.counts = &priv->cnts; + priv->counter.num_counts = 1; + + irq_set_status_flags(priv->irq, IRQ_NOAUTOEN); + ret = devm_request_irq(dev, priv->irq, interrupt_cnt_isr, + IRQF_TRIGGER_RISING | IRQF_NO_THREAD, + dev_name(dev), priv); + if (ret) + return ret; + + return devm_counter_register(dev, &priv->counter); +} + +static const struct of_device_id interrupt_cnt_of_match[] = { + { .compatible = "interrupt-counter", }, + {} +}; +MODULE_DEVICE_TABLE(of, interrupt_cnt_of_match); + +static struct platform_driver interrupt_cnt_driver = { + .probe = interrupt_cnt_probe, + .driver = { + .name = INTERRUPT_CNT_NAME, + .of_match_table = interrupt_cnt_of_match, + }, +}; +module_platform_driver(interrupt_cnt_driver); + +MODULE_ALIAS("platform:interrupt-counter"); +MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>"); +MODULE_DESCRIPTION("Interrupt counter driver"); +MODULE_LICENSE("GPL v2");