linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] add support for GPIO based counter
@ 2021-01-26 13:12 Oleksij Rempel
  2021-01-26 13:12 ` [PATCH v4 1/2] dt-bindings: counter: add pulse-counter binding Oleksij Rempel
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Oleksij Rempel @ 2021-01-26 13:12 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

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 pulse-counter binding
  counter: add IRQ or GPIO based pulse counter

 .../bindings/counter/pulse-counter.yaml       |  52 ++++
 drivers/counter/Kconfig                       |  10 +
 drivers/counter/Makefile                      |   1 +
 drivers/counter/pulse-cnt.c                   | 235 ++++++++++++++++++
 4 files changed, 298 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/counter/pulse-counter.yaml
 create mode 100644 drivers/counter/pulse-cnt.c

-- 
2.30.0


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v4 1/2] dt-bindings: counter: add pulse-counter binding
  2021-01-26 13:12 [PATCH v4 0/2] add support for GPIO based counter Oleksij Rempel
@ 2021-01-26 13:12 ` Oleksij Rempel
  2021-01-28  8:17   ` Linus Walleij
  2021-01-26 13:12 ` [PATCH v4 2/2] counter: add IRQ or GPIO based pulse counter Oleksij Rempel
  2021-01-26 13:18 ` [PATCH v4 0/2] add support for GPIO based counter Marc Kleine-Budde
  2 siblings, 1 reply; 12+ messages in thread
From: Oleksij Rempel @ 2021-01-26 13:12 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

Add binding for the pulse counter node

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 .../bindings/counter/pulse-counter.yaml       | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/counter/pulse-counter.yaml

diff --git a/Documentation/devicetree/bindings/counter/pulse-counter.yaml b/Documentation/devicetree/bindings/counter/pulse-counter.yaml
new file mode 100644
index 000000000000..8a82091edd65
--- /dev/null
+++ b/Documentation/devicetree/bindings/counter/pulse-counter.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/counter/pulse-counter.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Pulse counter
+
+maintainers:
+  - Oleksij Rempel <o.rempel@pengutronix.de>
+
+description: |
+  A generic pulse counter to measure pulse 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.
+
+properties:
+  compatible:
+    const: virtual,pulse-counter
+
+  interrupts:
+    maxItems: 1
+
+  gpios:
+    description: Optional diagnostic interface to measure signal level
+    maxItems: 1
+
+required:
+  - compatible
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    counter-0 {
+        compatible = "virtual,pulse-counter";
+        interrupts-extended = <&gpio 0 IRQ_TYPE_EDGE_RISING>;
+    };
+
+    counter-1 {
+        compatible = "virtual,pulse-counter";
+        interrupts-extended = <&gpio 1 IRQ_TYPE_EDGE_RISING>;
+        gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
+    };
+
+...
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v4 2/2] counter: add IRQ or GPIO based pulse counter
  2021-01-26 13:12 [PATCH v4 0/2] add support for GPIO based counter Oleksij Rempel
  2021-01-26 13:12 ` [PATCH v4 1/2] dt-bindings: counter: add pulse-counter binding Oleksij Rempel
@ 2021-01-26 13:12 ` Oleksij Rempel
  2021-01-27  8:52   ` kernel test robot
  2021-01-28  8:24   ` Linus Walleij
  2021-01-26 13:18 ` [PATCH v4 0/2] add support for GPIO based counter Marc Kleine-Budde
  2 siblings, 2 replies; 12+ messages in thread
From: Oleksij Rempel @ 2021-01-26 13:12 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

Add simple IRQ or GPIO base pulse 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>
---
 drivers/counter/Kconfig     |  10 ++
 drivers/counter/Makefile    |   1 +
 drivers/counter/pulse-cnt.c | 235 ++++++++++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+)
 create mode 100644 drivers/counter/pulse-cnt.c

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2de53ab0dd25..73107a861648 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 PULSE_CNT
+	tristate "Pulse counter driver"
+	depends on GPIOLIB
+	help
+	  Select this option to enable pulse counter driver. Any interrupt source
+	  can be used by this driver as the pulse source.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gpio-pulse-cnt.
+
 config STM32_TIMER_CNT
 	tristate "STM32 Timer encoder counter driver"
 	depends on MFD_STM32_TIMERS || COMPILE_TEST
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 0a393f71e481..73999e39e984 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_PULSE_CNT)		+= pulse-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/pulse-cnt.c b/drivers/counter/pulse-cnt.c
new file mode 100644
index 000000000000..4128b4b3661e
--- /dev/null
+++ b/drivers/counter/pulse-cnt.c
@@ -0,0 +1,235 @@
+// 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/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define PULSE_CNT_NAME		"pulse-cnt"
+
+struct pulse_cnt_priv {
+	struct counter_device counter;
+	struct counter_ops ops;
+	struct gpio_desc *gpio;
+	int irq;
+	bool enabled;
+	atomic_t count;
+};
+
+static irqreturn_t pulse_cnt_isr(int irq, void *dev_id)
+{
+	struct pulse_cnt_priv *priv = dev_id;
+
+	atomic_inc(&priv->count);
+
+	return IRQ_HANDLED;
+}
+
+static ssize_t pulse_cnt_enable_read(struct counter_device *counter,
+				     struct counter_count *count, void *private,
+				     char *buf)
+{
+	struct pulse_cnt_priv *priv = counter->priv;
+
+	return sysfs_emit(buf, "%d\n", priv->enabled);
+}
+
+static ssize_t pulse_cnt_enable_write(struct counter_device *counter,
+				      struct counter_count *count,
+				      void *private, const char *buf,
+				      size_t len)
+{
+	struct pulse_cnt_priv *priv = counter->priv;
+	bool enable;
+	ssize_t ret;
+
+	ret = kstrtobool(buf, &enable);
+	if (ret)
+		return ret;
+
+	if (priv->enabled == enable)
+		return len;
+
+	if (enable) {
+		priv->enabled = enable;
+		enable_irq(priv->irq);
+	} else {
+		disable_irq(priv->irq);
+		priv->enabled = enable;
+	}
+
+	return len;
+}
+
+static const struct counter_count_ext pulse_cnt_ext[] = {
+	{
+		.name = "enable",
+		.read = pulse_cnt_enable_read,
+		.write = pulse_cnt_enable_write,
+	},
+};
+
+static enum counter_synapse_action pulse_cnt_synapse_actionss[] = {
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+};
+
+static int pulse_cnt_action_get(struct counter_device *counter,
+			    struct counter_count *count,
+			    struct counter_synapse *synapse,
+			    size_t *action)
+{
+	*action = COUNTER_SYNAPSE_ACTION_RISING_EDGE;
+
+	return 0;
+}
+
+static int pulse_cnt_read(struct counter_device *counter,
+				 struct counter_count *count,
+				 unsigned long *val)
+{
+	struct pulse_cnt_priv *priv = counter->priv;
+
+	*val = atomic_read(&priv->count);
+
+	return 0;
+}
+
+static int pulse_cnt_write(struct counter_device *counter,
+				  struct counter_count *count,
+				  const unsigned long val)
+{
+	struct pulse_cnt_priv *priv = counter->priv;
+
+	atomic_set(&priv->count, val);
+
+	return 0;
+}
+
+static int pulse_cnt_function_get(struct counter_device *counter,
+				  struct counter_count *count, size_t *function)
+{
+	*function = COUNTER_COUNT_FUNCTION_INCREASE;
+
+	return 0;
+}
+
+static int pulse_cnt_signal_read(struct counter_device *counter,
+				 struct counter_signal *signal,
+				 enum counter_signal_value *val)
+{
+	struct pulse_cnt_priv *priv = counter->priv;
+	int ret;
+
+	ret = gpiod_get_value(priv->gpio);
+	if (ret < 0)
+		return ret;
+
+	*val = ret ? COUNTER_SIGNAL_HIGH : COUNTER_SIGNAL_LOW;
+
+	return 0;
+}
+
+static struct counter_signal pulse_cnt_signals[] = {
+	{
+		.id = 0,
+		.name = "Channel 0 signal",
+	},
+};
+
+static struct counter_synapse pulse_cnt_synapses[] = {
+	{
+		.actions_list = pulse_cnt_synapse_actionss,
+		.num_actions = ARRAY_SIZE(pulse_cnt_synapse_actionss),
+		.signal = &pulse_cnt_signals[0]
+	},
+};
+
+static enum counter_count_function pulse_cnt_functions[] = {
+	COUNTER_COUNT_FUNCTION_INCREASE,
+};
+
+static struct counter_count pulse_cnts[] = {
+	{
+		.id = 0,
+		.name = "Channel 1 Count",
+		.functions_list = pulse_cnt_functions,
+		.num_functions = ARRAY_SIZE(pulse_cnt_functions),
+		.synapses = pulse_cnt_synapses,
+		.num_synapses = ARRAY_SIZE(pulse_cnt_synapses),
+		.ext = pulse_cnt_ext,
+		.num_ext = ARRAY_SIZE(pulse_cnt_ext),
+	},
+};
+
+static int pulse_cnt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pulse_cnt_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->irq = platform_get_irq(pdev,  0);
+	if (priv->irq < 0) {
+		dev_err(dev, "failed to map GPIO to IRQ: %d\n", priv->irq);
+		return priv->irq;
+	}
+
+	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");
+
+	priv->ops.action_get = pulse_cnt_action_get;
+	priv->ops.count_read = pulse_cnt_read;
+	priv->ops.count_write = pulse_cnt_write;
+	priv->ops.function_get = pulse_cnt_function_get;
+	if (priv->gpio)
+		priv->ops.signal_read  = pulse_cnt_signal_read;
+
+	priv->counter.name = dev_name(dev);
+	priv->counter.parent = dev;
+	priv->counter.ops = &priv->ops;
+	priv->counter.counts = pulse_cnts;
+	priv->counter.num_counts = ARRAY_SIZE(pulse_cnts);
+	priv->counter.signals = pulse_cnt_signals;
+	priv->counter.num_signals = ARRAY_SIZE(pulse_cnt_signals);
+	priv->counter.priv = priv;
+
+	irq_set_status_flags(priv->irq, IRQ_NOAUTOEN);
+	ret = devm_request_irq(dev, priv->irq, pulse_cnt_isr,
+			       IRQF_TRIGGER_RISING | IRQF_NO_THREAD,
+			       PULSE_CNT_NAME, priv);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, priv);
+
+	return devm_counter_register(dev, &priv->counter);
+}
+
+static const struct of_device_id pulse_cnt_of_match[] = {
+	{ .compatible = "virtual,pulse-counter", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pulse_cnt_of_match);
+
+static struct platform_driver pulse_cnt_driver = {
+	.probe = pulse_cnt_probe,
+	.driver = {
+		.name = PULSE_CNT_NAME,
+		.of_match_table = pulse_cnt_of_match,
+	},
+};
+module_platform_driver(pulse_cnt_driver);
+
+MODULE_ALIAS("platform:pulse-counter");
+MODULE_AUTHOR("Oleksij Rempel <o.rempel@pengutronix.de>");
+MODULE_DESCRIPTION("Pulse counter driver");
+MODULE_LICENSE("GPL v2");
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 0/2] add support for GPIO based counter
  2021-01-26 13:12 [PATCH v4 0/2] add support for GPIO based counter Oleksij Rempel
  2021-01-26 13:12 ` [PATCH v4 1/2] dt-bindings: counter: add pulse-counter binding Oleksij Rempel
  2021-01-26 13:12 ` [PATCH v4 2/2] counter: add IRQ or GPIO based pulse counter Oleksij Rempel
@ 2021-01-26 13:18 ` Marc Kleine-Budde
  2021-01-27  6:44   ` Oleksij Rempel
  2 siblings, 1 reply; 12+ messages in thread
From: Marc Kleine-Budde @ 2021-01-26 13:18 UTC (permalink / raw)
  To: Oleksij Rempel, Rob Herring, William Breathitt Gray
  Cc: devicetree, linux-iio, Robin van der Gracht, linux-kernel,
	Pengutronix Kernel Team, David Jander


[-- Attachment #1.1: Type: text/plain, Size: 613 bytes --]

On 1/26/21 2:12 PM, Oleksij Rempel wrote:
> 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

What's the guaranteed width of atomic_t? IIRC a long time ago it was 24 bit only....

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 0/2] add support for GPIO based counter
  2021-01-26 13:18 ` [PATCH v4 0/2] add support for GPIO based counter Marc Kleine-Budde
@ 2021-01-27  6:44   ` Oleksij Rempel
  2021-01-27  7:10     ` Marc Kleine-Budde
  0 siblings, 1 reply; 12+ messages in thread
From: Oleksij Rempel @ 2021-01-27  6:44 UTC (permalink / raw)
  To: Marc Kleine-Budde
  Cc: Rob Herring, William Breathitt Gray, devicetree, linux-iio,
	Robin van der Gracht, linux-kernel, Pengutronix Kernel Team,
	David Jander

On Tue, Jan 26, 2021 at 02:18:34PM +0100, Marc Kleine-Budde wrote:
> On 1/26/21 2:12 PM, Oleksij Rempel wrote:
> > 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
> 
> What's the guaranteed width of atomic_t? IIRC a long time ago it was 24 bit only....

32 bits. The counter is wrapped after 4294967295

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] 12+ messages in thread

* Re: [PATCH v4 0/2] add support for GPIO based counter
  2021-01-27  6:44   ` Oleksij Rempel
@ 2021-01-27  7:10     ` Marc Kleine-Budde
  0 siblings, 0 replies; 12+ messages in thread
From: Marc Kleine-Budde @ 2021-01-27  7:10 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: devicetree, linux-iio, Robin van der Gracht,
	William Breathitt Gray, linux-kernel, Rob Herring,
	Pengutronix Kernel Team, David Jander


[-- Attachment #1.1: Type: text/plain, Size: 618 bytes --]

On 1/27/21 7:44 AM, Oleksij Rempel wrote:
>>> changes v3:
>>> - convert counter to atomic_t
>>
>> What's the guaranteed width of atomic_t? IIRC a long time ago it was 24 bit only....
> 
> 32 bits. The counter is wrapped after 4294967295

Note: This is only true for >= v2.6.3 :D

https://lwn.net/Articles/71732/

Feeling old,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 2/2] counter: add IRQ or GPIO based pulse counter
  2021-01-26 13:12 ` [PATCH v4 2/2] counter: add IRQ or GPIO based pulse counter Oleksij Rempel
@ 2021-01-27  8:52   ` kernel test robot
  2021-01-28  8:24   ` Linus Walleij
  1 sibling, 0 replies; 12+ messages in thread
From: kernel test robot @ 2021-01-27  8:52 UTC (permalink / raw)
  To: Oleksij Rempel, Rob Herring, William Breathitt Gray
  Cc: kbuild-all, Oleksij Rempel, Ahmad Fatoum, devicetree,
	linux-kernel, Pengutronix Kernel Team, David Jander,
	Robin van der Gracht, linux-iio

[-- Attachment #1: Type: text/plain, Size: 3817 bytes --]

Hi Oleksij,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v5.11-rc5 next-20210125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Oleksij-Rempel/add-support-for-GPIO-based-counter/20210127-085034
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/39e2c0023dba4e0e0f2bb9bfa1caeadb40df6356
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Oleksij-Rempel/add-support-for-GPIO-based-counter/20210127-085034
        git checkout 39e2c0023dba4e0e0f2bb9bfa1caeadb40df6356
        # save the attached .config to linux build tree
        make W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/counter/pulse-cnt.c: In function 'pulse_cnt_probe':
>> drivers/counter/pulse-cnt.c:205:2: error: implicit declaration of function 'irq_set_status_flags' [-Werror=implicit-function-declaration]
     205 |  irq_set_status_flags(priv->irq, IRQ_NOAUTOEN);
         |  ^~~~~~~~~~~~~~~~~~~~
>> drivers/counter/pulse-cnt.c:205:34: error: 'IRQ_NOAUTOEN' undeclared (first use in this function)
     205 |  irq_set_status_flags(priv->irq, IRQ_NOAUTOEN);
         |                                  ^~~~~~~~~~~~
   drivers/counter/pulse-cnt.c:205:34: note: each undeclared identifier is reported only once for each function it appears in
   cc1: some warnings being treated as errors


vim +/irq_set_status_flags +205 drivers/counter/pulse-cnt.c

   168	
   169	static int pulse_cnt_probe(struct platform_device *pdev)
   170	{
   171		struct device *dev = &pdev->dev;
   172		struct pulse_cnt_priv *priv;
   173		int ret;
   174	
   175		priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
   176		if (!priv)
   177			return -ENOMEM;
   178	
   179		priv->irq = platform_get_irq(pdev,  0);
   180		if (priv->irq < 0) {
   181			dev_err(dev, "failed to map GPIO to IRQ: %d\n", priv->irq);
   182			return priv->irq;
   183		}
   184	
   185		priv->gpio = devm_gpiod_get_optional(dev, NULL, GPIOD_IN);
   186		if (IS_ERR(priv->gpio))
   187			return dev_err_probe(dev, PTR_ERR(priv->gpio), "failed to get gpio\n");
   188	
   189		priv->ops.action_get = pulse_cnt_action_get;
   190		priv->ops.count_read = pulse_cnt_read;
   191		priv->ops.count_write = pulse_cnt_write;
   192		priv->ops.function_get = pulse_cnt_function_get;
   193		if (priv->gpio)
   194			priv->ops.signal_read  = pulse_cnt_signal_read;
   195	
   196		priv->counter.name = dev_name(dev);
   197		priv->counter.parent = dev;
   198		priv->counter.ops = &priv->ops;
   199		priv->counter.counts = pulse_cnts;
   200		priv->counter.num_counts = ARRAY_SIZE(pulse_cnts);
   201		priv->counter.signals = pulse_cnt_signals;
   202		priv->counter.num_signals = ARRAY_SIZE(pulse_cnt_signals);
   203		priv->counter.priv = priv;
   204	
 > 205		irq_set_status_flags(priv->irq, IRQ_NOAUTOEN);
   206		ret = devm_request_irq(dev, priv->irq, pulse_cnt_isr,
   207				       IRQF_TRIGGER_RISING | IRQF_NO_THREAD,
   208				       PULSE_CNT_NAME, priv);
   209		if (ret)
   210			return ret;
   211	
   212		platform_set_drvdata(pdev, priv);
   213	
   214		return devm_counter_register(dev, &priv->counter);
   215	}
   216	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 76691 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: counter: add pulse-counter binding
  2021-01-26 13:12 ` [PATCH v4 1/2] dt-bindings: counter: add pulse-counter binding Oleksij Rempel
@ 2021-01-28  8:17   ` Linus Walleij
  2021-01-28 13:39     ` Oleksij Rempel
  0 siblings, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2021-01-28  8:17 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Rob Herring, William Breathitt Gray,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, Pengutronix Kernel Team, David Jander,
	Robin van der Gracht, linux-iio

Hi Oleksij,

thanks for your patch!

On Tue, Jan 26, 2021 at 2:15 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Add binding for the pulse counter node
>
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
(...)

> +properties:
> +  compatible:
> +    const: virtual,pulse-counter

What is so virtual about this? The device seems very real.
However it is certainly a GPIO counter.

I would call it "gpio-counter" simply.

Define:
  $nodename:
     pattern: "^counter(@.*)?$"

> +    counter-0 {

counter@0 {

> +    counter-1 {

counter@1 {

Thanks!
Linus Walleij

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 2/2] counter: add IRQ or GPIO based pulse counter
  2021-01-26 13:12 ` [PATCH v4 2/2] counter: add IRQ or GPIO based pulse counter Oleksij Rempel
  2021-01-27  8:52   ` kernel test robot
@ 2021-01-28  8:24   ` Linus Walleij
  2021-01-28 13:58     ` Oleksij Rempel
  1 sibling, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2021-01-28  8:24 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Rob Herring, William Breathitt Gray, Ahmad Fatoum,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, Pengutronix Kernel Team, David Jander,
	Robin van der Gracht, linux-iio

Hi Oleksij,

thanks for your patch!

On Tue, Jan 26, 2021 at 2:15 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> +       priv->irq = platform_get_irq(pdev,  0);
> +       if (priv->irq < 0) {
> +               dev_err(dev, "failed to map GPIO to IRQ: %d\n", priv->irq);
> +               return priv->irq;
> +       }
> +
> +       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");

I would attempt to get the IRQ from the GPIO if not defined explicitly
in the device tree.

priv->gpio = devm_gpiod_get_optional(...)
if (priv->gpio) {
    /* Attempt to look up IRQ */
    irq = gpiod_to_irq(priv->irq);
}
priv->irq = platfform_get_irq(...)
if (priv->irq < 0 && irq > 0) {
    /* Use the GPIO-related IRQ */
    priv->irq = irq;
} else if (priv->irq < 0) {
   /* Error */
}

This way the example in the device tree binding which only defines
a GPIO and no interrupt will work if the GPIO chip provides an
IRQ mapping.

Yours.
Linus Walleij

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: counter: add pulse-counter binding
  2021-01-28  8:17   ` Linus Walleij
@ 2021-01-28 13:39     ` Oleksij Rempel
  2021-02-05 23:34       ` Rob Herring
  0 siblings, 1 reply; 12+ messages in thread
From: Oleksij Rempel @ 2021-01-28 13:39 UTC (permalink / raw)
  To: Linus Walleij
  Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-iio, Robin van der Gracht, William Breathitt Gray,
	linux-kernel, Rob Herring, Pengutronix Kernel Team, David Jander,
	Jonathan Cameron

On Thu, Jan 28, 2021 at 09:17:23AM +0100, Linus Walleij wrote:
> Hi Oleksij,
> 
> thanks for your patch!
> 
> On Tue, Jan 26, 2021 at 2:15 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > Add binding for the pulse counter node
> >
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> (...)
> 
> > +properties:
> > +  compatible:
> > +    const: virtual,pulse-counter
> 
> What is so virtual about this? The device seems very real.

Currently there are two ways:
1. use "virtual" or "linux" vendor. Same as "virtual,mdio-gpio"
2. Extend the list of "not vendor" prefixes in the prefixes list:
   Documentation/devicetree/bindings/vendor-prefixes.yaml

Since both ways seems to be valid, i personally prefer to use existing
prefix instead of maintaining the vendor-prefixes.yaml

@Rob, what do you prefer?

> However it is certainly a GPIO counter.

This was my first implementation. @Jonathan you suggest to use GPIO-free
way, can you and Linus please decide what is the way to go.

I personally can imagine that this driver can be attached to any IRQ
source, including drivers/iio/trigger/iio-trig-sysfs.c

> I would call it "gpio-counter" simply.
> 
> Define:
>   $nodename:
>      pattern: "^counter(@.*)?$"
> 
> > +    counter-0 {
> 
> counter@0 {
> 
> > +    counter-1 {
> 
> counter@1 {

In this case the dtc compiler will say:
/counter@0: node has a unit name, but no reg property

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] 12+ messages in thread

* Re: [PATCH v4 2/2] counter: add IRQ or GPIO based pulse counter
  2021-01-28  8:24   ` Linus Walleij
@ 2021-01-28 13:58     ` Oleksij Rempel
  0 siblings, 0 replies; 12+ messages in thread
From: Oleksij Rempel @ 2021-01-28 13:58 UTC (permalink / raw)
  To: Linus Walleij
  Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Ahmad Fatoum, linux-iio, Robin van der Gracht,
	William Breathitt Gray, linux-kernel, Rob Herring,
	Pengutronix Kernel Team, David Jander

On Thu, Jan 28, 2021 at 09:24:08AM +0100, Linus Walleij wrote:
> Hi Oleksij,
> 
> thanks for your patch!
> 
> On Tue, Jan 26, 2021 at 2:15 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> 
> > +       priv->irq = platform_get_irq(pdev,  0);
> > +       if (priv->irq < 0) {
> > +               dev_err(dev, "failed to map GPIO to IRQ: %d\n", priv->irq);
> > +               return priv->irq;
> > +       }
> > +
> > +       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");
> 
> I would attempt to get the IRQ from the GPIO if not defined explicitly
> in the device tree.
> 
> priv->gpio = devm_gpiod_get_optional(...)
> if (priv->gpio) {
>     /* Attempt to look up IRQ */
>     irq = gpiod_to_irq(priv->irq);
> }
> priv->irq = platfform_get_irq(...)
> if (priv->irq < 0 && irq > 0) {
>     /* Use the GPIO-related IRQ */
>     priv->irq = irq;
> } else if (priv->irq < 0) {
>    /* Error */
> }
> 
> This way the example in the device tree binding which only defines
> a GPIO and no interrupt will work if the GPIO chip provides an
> IRQ mapping.
> 

Ok, thx!
I'll send updated version after dt-binding discussion

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] 12+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: counter: add pulse-counter binding
  2021-01-28 13:39     ` Oleksij Rempel
@ 2021-02-05 23:34       ` Rob Herring
  0 siblings, 0 replies; 12+ messages in thread
From: Rob Herring @ 2021-02-05 23:34 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Linus Walleij,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-iio, Robin van der Gracht, William Breathitt Gray,
	linux-kernel, Pengutronix Kernel Team, David Jander,
	Jonathan Cameron

On Thu, Jan 28, 2021 at 02:39:22PM +0100, Oleksij Rempel wrote:
> On Thu, Jan 28, 2021 at 09:17:23AM +0100, Linus Walleij wrote:
> > Hi Oleksij,
> > 
> > thanks for your patch!
> > 
> > On Tue, Jan 26, 2021 at 2:15 PM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > 
> > > Add binding for the pulse counter node
> > >
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > (...)
> > 
> > > +properties:
> > > +  compatible:
> > > +    const: virtual,pulse-counter
> > 
> > What is so virtual about this? The device seems very real.
> 
> Currently there are two ways:
> 1. use "virtual" or "linux" vendor. Same as "virtual,mdio-gpio"

virtual is used by exactly one case. linux for a few more, mostly 
linux,spdif-dit and extcon (deprecated).

> 2. Extend the list of "not vendor" prefixes in the prefixes list:
>    Documentation/devicetree/bindings/vendor-prefixes.yaml

Pretty sure that says 'DON'T ADD MORE'. Maybe I forgot to scream it.

> 
> Since both ways seems to be valid, i personally prefer to use existing
> prefix instead of maintaining the vendor-prefixes.yaml
> 
> @Rob, what do you prefer?

For vendorless bindings, no vendor prefix! 'gpio-counter' if only gpio 
interfaced. No idea what other options would be.

> 
> > However it is certainly a GPIO counter.
> 
> This was my first implementation. @Jonathan you suggest to use GPIO-free
> way, can you and Linus please decide what is the way to go.
> 
> I personally can imagine that this driver can be attached to any IRQ
> source, including drivers/iio/trigger/iio-trig-sysfs.c
> 
> > I would call it "gpio-counter" simply.
> > 
> > Define:
> >   $nodename:
> >      pattern: "^counter(@.*)?$"
> > 
> > > +    counter-0 {
> > 
> > counter@0 {
> > 
> > > +    counter-1 {
> > 
> > counter@1 {
> 
> In this case the dtc compiler will say:
> /counter@0: node has a unit name, but no reg property

counter-0 then.

> 
> 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] 12+ messages in thread

end of thread, other threads:[~2021-02-06  5:02 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-26 13:12 [PATCH v4 0/2] add support for GPIO based counter Oleksij Rempel
2021-01-26 13:12 ` [PATCH v4 1/2] dt-bindings: counter: add pulse-counter binding Oleksij Rempel
2021-01-28  8:17   ` Linus Walleij
2021-01-28 13:39     ` Oleksij Rempel
2021-02-05 23:34       ` Rob Herring
2021-01-26 13:12 ` [PATCH v4 2/2] counter: add IRQ or GPIO based pulse counter Oleksij Rempel
2021-01-27  8:52   ` kernel test robot
2021-01-28  8:24   ` Linus Walleij
2021-01-28 13:58     ` Oleksij Rempel
2021-01-26 13:18 ` [PATCH v4 0/2] add support for GPIO based counter Marc Kleine-Budde
2021-01-27  6:44   ` Oleksij Rempel
2021-01-27  7:10     ` Marc Kleine-Budde

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).