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 <[email protected]>
Reviewed-by: Ahmad Fatoum <[email protected]>
---
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 <[email protected]>
+R: Pengutronix Kernel Team <[email protected]>
+L: [email protected]
+F: Documentation/devicetree/bindings/counter/interrupt-counter.yaml
+F: drivers/counter/interrupt-cnt.c
+
INVENSENSE ICM-426xx IMU DRIVER
M: Jean-Baptiste Maneyrol <[email protected]>
L: [email protected]
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 <[email protected]>
+ */
+
+#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 <[email protected]>");
+MODULE_DESCRIPTION("Interrupt counter driver");
+MODULE_LICENSE("GPL v2");
--
2.29.2
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 <[email protected]>
> Reviewed-by: Ahmad Fatoum <[email protected]>
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 <[email protected]>
> +R: Pengutronix Kernel Team <[email protected]>
> +L: [email protected]
> +F: Documentation/devicetree/bindings/counter/interrupt-counter.yaml
> +F: drivers/counter/interrupt-cnt.c
> +
> INVENSENSE ICM-426xx IMU DRIVER
> M: Jean-Baptiste Maneyrol <[email protected]>
> L: [email protected]
> 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 <[email protected]>
> + */
> +
> +#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 <[email protected]>");
> +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 <[email protected]>
> > Reviewed-by: Ahmad Fatoum <[email protected]>
>
> 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 <[email protected]>
> > +R: Pengutronix Kernel Team <[email protected]>
> > +L: [email protected]
> > +F: Documentation/devicetree/bindings/counter/interrupt-counter.yaml
> > +F: drivers/counter/interrupt-cnt.c
> > +
> > INVENSENSE ICM-426xx IMU DRIVER
> > M: Jean-Baptiste Maneyrol <[email protected]>
> > L: [email protected]
> > 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 <[email protected]>
> > + */
> > +
> > +#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
--
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 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