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

[William Breathitt Gray]

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@xxxxxxxxxxxxxx>
> Reviewed-by: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxx>
> +R:	Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>
> +L:	linux-iio@xxxxxxxxxxxxxxx
> +F:	Documentation/devicetree/bindings/counter/interrupt-counter.yaml
> +F:	drivers/counter/interrupt-cnt.c
> +
>  INVENSENSE ICM-426xx IMU DRIVER
>  M:	Jean-Baptiste Maneyrol <jmaneyrol@xxxxxxxxxxxxxx>
>  L:	linux-iio@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxx>
> + */
> +
> +#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@xxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Interrupt counter driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.29.2
>

Attachment: signature.asc
Description: PGP signature