* [PATCH v7 0/2] add support for GPIO or IRQ based evemt counter
@ 2021-02-26 9:08 Oleksij Rempel
2021-02-26 9:08 ` [PATCH v7 1/2] dt-bindings: counter: add interrupt-counter binding Oleksij Rempel
2021-02-26 9:08 ` [PATCH v7 2/2] counter: add IRQ or GPIO based counter Oleksij Rempel
0 siblings, 2 replies; 6+ messages in thread
From: Oleksij Rempel @ 2021-02-26 9:08 UTC (permalink / raw)
To: Rob Herring, William Breathitt Gray
Cc: Oleksij Rempel, devicetree, linux-kernel, Pengutronix Kernel Team,
David Jander, Robin van der Gracht, linux-iio, Linus Walleij,
Jonathan Cameron
changes v7:
- make most of structs dynamically allocatable to assign IRQ based
description to the signal
- assign dev name instead for driver name to the IRQ
changes v6:
- rename it to interrupt-counter
- driver fixes
- device tree fixes
changes v5:
- rename it to event counter, since it support different event sources
- make it work with gpio-only or irq-only configuration
- update yaml binding
changes v4:
- use IRQ_NOAUTOEN to not enable IRQ by default
- rename gpio_ from name pattern and make this driver work any IRQ
source.
changes v3:
- convert counter to atomic_t
changes v2:
- add commas
- avoid possible unhandled interrupts in the enable path
- do not use of_ specific gpio functions
Add support for GPIO based pulse counter. For now it can only count
pulses. With counter char device support, we will be able to attach
timestamps and measure actual pulse frequency.
Never the less, it is better to mainline this driver now (before chardev
patches go mainline), to provide developers additional use case for the counter
framework with chardev support.
Oleksij Rempel (2):
dt-bindings: counter: add interrupt-counter binding
counter: add IRQ or GPIO based counter
.../bindings/counter/interrupt-counter.yaml | 62 +++++
MAINTAINERS | 7 +
drivers/counter/Kconfig | 10 +
drivers/counter/Makefile | 1 +
drivers/counter/interrupt-cnt.c | 243 ++++++++++++++++++
5 files changed, 323 insertions(+)
create mode 100644 Documentation/devicetree/bindings/counter/interrupt-counter.yaml
create mode 100644 drivers/counter/interrupt-cnt.c
--
2.29.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v7 1/2] dt-bindings: counter: add interrupt-counter binding
2021-02-26 9:08 [PATCH v7 0/2] add support for GPIO or IRQ based evemt counter Oleksij Rempel
@ 2021-02-26 9:08 ` Oleksij Rempel
2021-02-26 9:08 ` [PATCH v7 2/2] counter: add IRQ or GPIO based counter Oleksij Rempel
1 sibling, 0 replies; 6+ messages in thread
From: Oleksij Rempel @ 2021-02-26 9:08 UTC (permalink / raw)
To: Rob Herring, William Breathitt Gray
Cc: Oleksij Rempel, Linus Walleij, devicetree, linux-kernel,
Pengutronix Kernel Team, David Jander, Robin van der Gracht,
linux-iio, Jonathan Cameron
Add binding for the interrupt counter node
Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
.../bindings/counter/interrupt-counter.yaml | 62 +++++++++++++++++++
1 file changed, 62 insertions(+)
create mode 100644 Documentation/devicetree/bindings/counter/interrupt-counter.yaml
diff --git a/Documentation/devicetree/bindings/counter/interrupt-counter.yaml b/Documentation/devicetree/bindings/counter/interrupt-counter.yaml
new file mode 100644
index 000000000000..fd075d104631
--- /dev/null
+++ b/Documentation/devicetree/bindings/counter/interrupt-counter.yaml
@@ -0,0 +1,62 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/counter/interrupt-counter.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Interrupt counter
+
+maintainers:
+ - Oleksij Rempel <o.rempel@pengutronix.de>
+
+description: |
+ A generic interrupt counter to measure interrupt frequency. It was developed
+ and used for agricultural devices to measure rotation speed of wheels or
+ other tools. Since the direction of rotation is not important, only one
+ signal line is needed.
+ Interrupts or gpios are required. If both are defined, the interrupt will
+ take precedence for counting interrupts.
+
+properties:
+ compatible:
+ const: interrupt-counter
+
+ interrupts:
+ maxItems: 1
+
+ gpios:
+ maxItems: 1
+
+required:
+ - compatible
+
+anyOf:
+ - required: [ interrupts-extended ]
+ - required: [ interrupts ]
+ - required: [ gpios ]
+
+additionalProperties: false
+
+examples:
+ - |
+
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+
+ counter-0 {
+ compatible = "interrupt-counter";
+ interrupts-extended = <&gpio 0 IRQ_TYPE_EDGE_RISING>;
+ };
+
+ counter-1 {
+ compatible = "interrupt-counter";
+ gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+ };
+
+ counter-2 {
+ compatible = "interrupt-counter";
+ interrupts-extended = <&gpio 2 IRQ_TYPE_EDGE_RISING>;
+ gpios = <&gpio 2 GPIO_ACTIVE_HIGH>;
+ };
+
+...
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v7 2/2] counter: add IRQ or GPIO based counter
2021-02-26 9:08 [PATCH v7 0/2] add support for GPIO or IRQ based evemt counter Oleksij Rempel
2021-02-26 9:08 ` [PATCH v7 1/2] dt-bindings: counter: add interrupt-counter binding Oleksij Rempel
@ 2021-02-26 9:08 ` Oleksij Rempel
2021-02-26 9:45 ` William Breathitt Gray
1 sibling, 1 reply; 6+ messages in thread
From: Oleksij Rempel @ 2021-02-26 9:08 UTC (permalink / raw)
To: Rob Herring, William Breathitt Gray
Cc: Oleksij Rempel, Ahmad Fatoum, devicetree, linux-kernel,
Pengutronix Kernel Team, David Jander, Robin van der Gracht,
linux-iio, Linus Walleij, Jonathan Cameron
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>
---
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];
+
+ 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");
--
2.29.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v7 2/2] counter: add IRQ or GPIO based counter
2021-02-26 9:08 ` [PATCH v7 2/2] counter: add IRQ or GPIO based counter Oleksij Rempel
@ 2021-02-26 9:45 ` William Breathitt Gray
2021-02-26 12:14 ` Oleksij Rempel
0 siblings, 1 reply; 6+ messages in thread
From: William Breathitt Gray @ 2021-02-26 9:45 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Rob Herring, Ahmad Fatoum, devicetree, linux-kernel,
Pengutronix Kernel Team, David Jander, Robin van der Gracht,
linux-iio, Linus Walleij, Jonathan Cameron
[-- Attachment #1: Type: text/plain, Size: 10524 bytes --]
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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7 2/2] counter: add IRQ or GPIO based counter
2021-02-26 9:45 ` William Breathitt Gray
@ 2021-02-26 12:14 ` Oleksij Rempel
2021-02-26 12:41 ` William Breathitt Gray
0 siblings, 1 reply; 6+ messages in thread
From: Oleksij Rempel @ 2021-02-26 12:14 UTC (permalink / raw)
To: William Breathitt Gray
Cc: Rob Herring, Ahmad Fatoum, devicetree, linux-kernel,
Pengutronix Kernel Team, David Jander, Robin van der Gracht,
linux-iio, Linus Walleij, Jonathan Cameron
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
--
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 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7 2/2] counter: add IRQ or GPIO based counter
2021-02-26 12:14 ` Oleksij Rempel
@ 2021-02-26 12:41 ` William Breathitt Gray
0 siblings, 0 replies; 6+ messages in thread
From: William Breathitt Gray @ 2021-02-26 12:41 UTC (permalink / raw)
To: Oleksij Rempel
Cc: Rob Herring, Ahmad Fatoum, devicetree, linux-kernel,
Pengutronix Kernel Team, David Jander, Robin van der Gracht,
linux-iio, Linus Walleij, Jonathan Cameron
[-- Attachment #1: Type: text/plain, Size: 4181 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-26 12:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-02-26 9:08 [PATCH v7 0/2] add support for GPIO or IRQ based evemt counter Oleksij Rempel
2021-02-26 9:08 ` [PATCH v7 1/2] dt-bindings: counter: add interrupt-counter binding Oleksij Rempel
2021-02-26 9:08 ` [PATCH v7 2/2] counter: add IRQ or GPIO based counter Oleksij Rempel
2021-02-26 9:45 ` William Breathitt Gray
2021-02-26 12:14 ` Oleksij Rempel
2021-02-26 12:41 ` William Breathitt Gray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).