[PATCH v4 0/3] ECAP support on TI AM62x SoC

[PATCH v4 0/3] ECAP support on TI AM62x SoC

[Julien Panis]

The Enhanced Capture (ECAP) module can be used to timestamp events
detected on signal input pin. It can be used for time measurements
of pulse train signals.

ECAP module includes 4 timestamp capture registers. For all 4 sequenced
timestamp capture events (1->2->3->4->1->...), edge polarity (falling/rising
edge) can be selected.

This driver leverages counter subsystem to :
- select edge polarity for all 4 capture events (event mode)
- log timestamps for each capture event
Event polarity, and CAP1/2/3/4 timestamps give all the information
about the input pulse train. Further information can easily be computed :
period and/or duty cycle if frequency is constant, elapsed time between
pulses, etc...

Modifications since v3:
	- Migrate driver from IIO to Counter subsystem
	- Minor modification in yaml ($id) to match Counter subsystem
	- Add ABI documentation

Userspace commands :
	### SIGNAL ###
	cd /sys/bus/counter/devices/counter0/signal0

	# Get available polarities for each capture event
	cat polarity1_available
	cat polarity2_available
	cat polarity3_available
	cat polarity4_available

	# Get polarity for each capture event
	cat polarity1
	cat polarity2
	cat polarity3
	cat polarity4

	# Set polarity for each capture event
	echo rising > polarity1
	echo falling > polarity2
	echo rising > polarity3
	echo falling > polarity4

	### COUNT ###
	cd /sys/bus/counter/devices/counter0/count0

	# Run ECAP
	echo 1 > enable

	# Get current timebase counter value
	cat count

	# Get captured timestamps
	cat capture1
	cat capture2
	cat capture3
	cat capture4

	# Note that counter watches can also be used to get
	# data from userspace application
	# -> see tools/counter/counter_example.c

	# Stop ECAP
	echo 0 > enable

Julien Panis (3):
  dt-binding: counter: add ti,am62-ecap-capture.yaml
  Documentation: ABI: add sysfs-bus-counter-ecap
  counter: capture-tiecap: capture driver support for ECAP

 .../ABI/testing/sysfs-bus-counter-ecap        |  64 ++
 .../counter/ti,am62-ecap-capture.yaml         |  61 ++
 drivers/counter/Kconfig                       |  14 +
 drivers/counter/Makefile                      |   1 +
 drivers/counter/capture-tiecap.c              | 634 ++++++++++++++++++
 include/uapi/linux/counter.h                  |   2 +
 6 files changed, 776 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ecap
 create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
 create mode 100644 drivers/counter/capture-tiecap.c

-- 
2.25.1

[PATCH v4 1/3] dt-binding: counter: add ti,am62-ecap-capture.yaml

[Julien Panis]

This commit adds a YAML binding for TI ECAP used in capture operating mode.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../counter/ti,am62-ecap-capture.yaml         | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml

diff --git a/Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml b/Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
new file mode 100644
index 000000000000..4e0b2d2b303e
--- /dev/null
+++ b/Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/counter/ti,am62-ecap-capture.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments Enhanced Capture (eCAP) Module
+
+maintainers:
+  - Julien Panis <jpanis@baylibre.com>
+
+description: |
+  The eCAP module resources can be used to capture timestamps
+  on input signal events (falling/rising edges).
+
+properties:
+  compatible:
+    const: ti,am62-ecap-capture
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: fck
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/soc/ti,sci_pm_domain.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        capture@23100000 { /* eCAP in capture mode on am62x */
+            compatible = "ti,am62-ecap-capture";
+            reg = <0x00 0x23100000 0x00 0x100>;
+            interrupts = <GIC_SPI 113 IRQ_TYPE_EDGE_RISING>;
+            power-domains = <&k3_pds 51 TI_SCI_PD_EXCLUSIVE>;
+            clocks = <&k3_clks 51 0>;
+            clock-names = "fck";
+        };
+    };
-- 
2.25.1

Re: [PATCH v4 1/3] dt-binding: counter: add ti,am62-ecap-capture.yaml

[Krzysztof Kozlowski]

On 10/08/2022 17:07, Julien Panis wrote:
> This commit adds a YAML binding for TI ECAP used in capture operating mode.
> 
> Signed-off-by: Julien Panis <jpanis@baylibre.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Subject prefix is "dt-bindings".


Best regards,
Krzysztof

[PATCH v4 2/3] Documentation: ABI: add sysfs-bus-counter-ecap

[Julien Panis]

This commit adds an ABI file for TI ECAP used in capture operating mode.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
---
 .../ABI/testing/sysfs-bus-counter-ecap        | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ecap

diff --git a/Documentation/ABI/testing/sysfs-bus-counter-ecap b/Documentation/ABI/testing/sysfs-bus-counter-ecap
new file mode 100644
index 000000000000..ca530a6806de
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-counter-ecap
@@ -0,0 +1,64 @@
+What:		/sys/bus/counter/devices/counter0/signal0/polarity1
+What:		/sys/bus/counter/devices/counter0/signal0/polarity2
+What:		/sys/bus/counter/devices/counter0/signal0/polarity3
+What:		/sys/bus/counter/devices/counter0/signal0/polarity4
+KernelVersion:	5.19
+Contact:	jpanis@baylibre.com
+Description:
+		ECAP module includes 4 timestamp capture registers.
+		For all 4 sequenced timestamp capture events (1->2->3->4->1->...),
+		edge polarity can be selected. Then, each capture register can be
+		tuned to capture event by either:
+
+		- "rising"
+		- "falling"
+
+		Reading returns current trigger polarity.
+
+		Writing value before enabling capture sets trigger polarity.
+
+What:		/sys/bus/counter/devices/counter0/signal0/polarity1_available
+What:		/sys/bus/counter/devices/counter0/signal0/polarity2_available
+What:		/sys/bus/counter/devices/counter0/signal0/polarity3_available
+What:		/sys/bus/counter/devices/counter0/signal0/polarity4_available
+KernelVersion:	5.19
+Contact:	jpanis@baylibre.com
+Description:
+		Discrete set of available values for the respective polarity X
+		configuration are listed in this file. Values are delimited by
+		newline characters.
+
+What:		/sys/bus/counter/devices/counter0/count0/enable
+KernelVersion:	5.19
+Contact:	jpanis@baylibre.com
+Description:
+		Whether ECAP is enabled. Valid attribute values are boolean.
+
+		This attribute is intended to serve as a start/stop mechanism
+		for ECAP.
+
+What:		/sys/bus/counter/devices/counter0/count0/count
+KernelVersion:	5.19
+Contact:	jpanis@baylibre.com
+Description:
+		Read-only attribute that indicates the current base counter value.
+
+What:		/sys/bus/counter/devices/counter0/count0/capture1
+What:		/sys/bus/counter/devices/counter0/count0/capture2
+What:		/sys/bus/counter/devices/counter0/count0/capture3
+What:		/sys/bus/counter/devices/counter0/count0/capture4
+KernelVersion:	5.19
+Contact:	jpanis@baylibre.com
+Description:
+		Read-only attributes that indicate the last timestamp captured
+		for the respective capture X register.
+
+What:		/sys/bus/counter/devices/counter0/count0/capture1_component_id
+What:		/sys/bus/counter/devices/counter0/count0/capture2_component_id
+What:		/sys/bus/counter/devices/counter0/count0/capture3_component_id
+What:		/sys/bus/counter/devices/counter0/count0/capture4_component_id
+KernelVersion:	5.19
+Contact:	jpanis@baylibre.com
+Description:
+		Read-only attributes that indicate the component ID of the
+		respective extension or synapse.
-- 
2.25.1

Re: [PATCH v4 2/3] Documentation: ABI: add sysfs-bus-counter-ecap

[William Breathitt Gray]

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

On Wed, Aug 10, 2022 at 04:07:23PM +0200, Julien Panis wrote:
> This commit adds an ABI file for TI ECAP used in capture operating mode.
> 
> Signed-off-by: Julien Panis <jpanis@baylibre.com>

Hi Julien,

Duplicate ABIs are no longer valid, so we consolidated the Counter sysfs
attributes documentation to a single sysfs-bus-counter file in commit
769841c966fd. You can document the new sysfs attributes you introduce
for the TI eCAP counter driver to that sysfs-bus-counter file as well.

William Breathitt Gray

> ---
>  .../ABI/testing/sysfs-bus-counter-ecap        | 64 +++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ecap
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter-ecap b/Documentation/ABI/testing/sysfs-bus-counter-ecap
> new file mode 100644
> index 000000000000..ca530a6806de
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-counter-ecap
> @@ -0,0 +1,64 @@
> +What:		/sys/bus/counter/devices/counter0/signal0/polarity1
> +What:		/sys/bus/counter/devices/counter0/signal0/polarity2
> +What:		/sys/bus/counter/devices/counter0/signal0/polarity3
> +What:		/sys/bus/counter/devices/counter0/signal0/polarity4
> +KernelVersion:	5.19
> +Contact:	jpanis@baylibre.com
> +Description:
> +		ECAP module includes 4 timestamp capture registers.
> +		For all 4 sequenced timestamp capture events (1->2->3->4->1->...),
> +		edge polarity can be selected. Then, each capture register can be
> +		tuned to capture event by either:
> +
> +		- "rising"
> +		- "falling"
> +
> +		Reading returns current trigger polarity.
> +
> +		Writing value before enabling capture sets trigger polarity.
> +
> +What:		/sys/bus/counter/devices/counter0/signal0/polarity1_available
> +What:		/sys/bus/counter/devices/counter0/signal0/polarity2_available
> +What:		/sys/bus/counter/devices/counter0/signal0/polarity3_available
> +What:		/sys/bus/counter/devices/counter0/signal0/polarity4_available
> +KernelVersion:	5.19
> +Contact:	jpanis@baylibre.com
> +Description:
> +		Discrete set of available values for the respective polarity X
> +		configuration are listed in this file. Values are delimited by
> +		newline characters.
> +
> +What:		/sys/bus/counter/devices/counter0/count0/enable
> +KernelVersion:	5.19
> +Contact:	jpanis@baylibre.com
> +Description:
> +		Whether ECAP is enabled. Valid attribute values are boolean.
> +
> +		This attribute is intended to serve as a start/stop mechanism
> +		for ECAP.
> +
> +What:		/sys/bus/counter/devices/counter0/count0/count
> +KernelVersion:	5.19
> +Contact:	jpanis@baylibre.com
> +Description:
> +		Read-only attribute that indicates the current base counter value.
> +
> +What:		/sys/bus/counter/devices/counter0/count0/capture1
> +What:		/sys/bus/counter/devices/counter0/count0/capture2
> +What:		/sys/bus/counter/devices/counter0/count0/capture3
> +What:		/sys/bus/counter/devices/counter0/count0/capture4
> +KernelVersion:	5.19
> +Contact:	jpanis@baylibre.com
> +Description:
> +		Read-only attributes that indicate the last timestamp captured
> +		for the respective capture X register.
> +
> +What:		/sys/bus/counter/devices/counter0/count0/capture1_component_id
> +What:		/sys/bus/counter/devices/counter0/count0/capture2_component_id
> +What:		/sys/bus/counter/devices/counter0/count0/capture3_component_id
> +What:		/sys/bus/counter/devices/counter0/count0/capture4_component_id
> +KernelVersion:	5.19
> +Contact:	jpanis@baylibre.com
> +Description:
> +		Read-only attributes that indicate the component ID of the
> +		respective extension or synapse.
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v4 3/3] counter: capture-tiecap: capture driver support for ECAP

[Julien Panis]

ECAP hardware on AM62x SoC supports capture feature. It can be used
to timestamp events (falling/rising edges) detected on signal input pin.

This commit adds capture driver support for ECAP hardware on AM62x SoC.

In the ECAP hardware, capture pin can also be configured to be in
PWM mode. Current implementation only supports capture operating mode.
Hardware also supports timebase sync between multiple instances, but
this driver supports simple independent capture functionality.

Signed-off-by: Julien Panis <jpanis@baylibre.com>
---
 drivers/counter/Kconfig          |  14 +
 drivers/counter/Makefile         |   1 +
 drivers/counter/capture-tiecap.c | 634 +++++++++++++++++++++++++++++++
 include/uapi/linux/counter.h     |   2 +
 4 files changed, 651 insertions(+)
 create mode 100644 drivers/counter/capture-tiecap.c

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 5edd155f1911..580640807a94 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -101,4 +101,18 @@ config INTEL_QEP
 	  To compile this driver as a module, choose M here: the module
 	  will be called intel-qep.
 
+config CAPTURE_TIECAP
+	tristate "TI eCAP capture driver"
+	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Select this option to enable the Texas Instruments Enhanced Capture
+	  (eCAP) driver in input mode.
+
+	  It can be used to timestamp events (falling/rising edges) detected
+	  on signal input pin.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called capture-tiecap.
+
 endif # COUNTER
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 8fde6c100ebc..7fac3e0985b5 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
 obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
 obj-$(CONFIG_MICROCHIP_TCB_CAPTURE)	+= microchip-tcb-capture.o
 obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
+obj-$(CONFIG_CAPTURE_TIECAP)	+= capture-tiecap.o
diff --git a/drivers/counter/capture-tiecap.c b/drivers/counter/capture-tiecap.c
new file mode 100644
index 000000000000..0601c65ef203
--- /dev/null
+++ b/drivers/counter/capture-tiecap.c
@@ -0,0 +1,634 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * ECAP Capture driver
+ *
+ * Copyright (C) 2022 Julien Panis <jpanis@baylibre.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/counter.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+#define ECAP_DRV_NAME "ecap"
+
+/* Registers */
+#define ECAP_NB_CAP			4
+
+#define ECAP_TSCNT_REG			0x00
+
+#define ECAP_CAP_REG(i)		(((i) << 2) + 0x08)
+
+#define ECAP_ECCTL_REG			0x28
+#define ECAP_CAPPOL_BIT(i)		BIT((i) << 1)
+#define ECAP_EV_MODE_MASK		GENMASK(7, 0)
+#define ECAP_CAPLDEN_BIT		BIT(8)
+#define ECAP_CONT_ONESHT_BIT		BIT(16)
+#define ECAP_STOPVALUE_MASK		GENMASK(18, 17)
+#define ECAP_REARM_RESET_BIT		BIT(19)
+#define ECAP_TSCNTSTP_BIT		BIT(20)
+#define ECAP_SYNCO_DIS_MASK		GENMASK(23, 22)
+#define ECAP_CAP_APWM_BIT		BIT(25)
+#define ECAP_ECCTL_EN_MASK		(ECAP_CAPLDEN_BIT | ECAP_TSCNTSTP_BIT)
+#define ECAP_ECCTL_CFG_MASK		(ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK	\
+					| ECAP_ECCTL_EN_MASK | ECAP_REARM_RESET_BIT	\
+					| ECAP_CAP_APWM_BIT | ECAP_CONT_ONESHT_BIT)
+
+#define ECAP_ECINT_EN_FLG_REG		0x2c
+#define ECAP_NB_CEVT			(ECAP_NB_CAP + 1)
+#define ECAP_CEVT_EN_MASK		GENMASK(ECAP_NB_CEVT, ECAP_NB_CAP)
+#define ECAP_CEVT_FLG_BIT(i)		BIT((i) + 17)
+
+#define ECAP_ECINT_CLR_FRC_REG	0x30
+#define ECAP_INT_CLR_BIT		BIT(0)
+#define ECAP_CEVT_CLR_BIT(i)		BIT((i) + 1)
+#define ECAP_CEVT_CLR_MASK		GENMASK(ECAP_NB_CEVT, 0)
+
+#define ECAP_PID_REG			0x5c
+
+/*
+ * Event modes
+ * One bit for each CAPx register : 1 = falling edge / 0 = rising edge
+ * e.g. mode = 13 = 0xd = 0b1101
+ * -> falling edge for CAP1-3-4 / rising edge for CAP2
+ */
+#define ECAP_EV_MODE_BIT(i)		BIT(i)
+
+/* ECAP input */
+#define ECAP_SIGNAL 0
+
+static const struct regmap_config ecap_cnt_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = ECAP_PID_REG,
+};
+
+/**
+ * struct ecap_cnt_dev - device private data structure
+ * @enabled:      device state
+ * @clk:          device clock
+ * @clk_rate:     device clock rate
+ * @regmap:       device register map
+ * @elapsed_time: elapsed time since capture start
+ * @lock:         synchronization lock to prevent race conditions when computing times
+ * @pm_ctx:       device context for PM operations
+ */
+struct ecap_cnt_dev {
+	bool enabled;
+	struct clk *clk;
+	unsigned long clk_rate;
+	struct regmap *regmap;
+	__aligned_u64 elapsed_time;
+	spinlock_t lock;
+	struct {
+		u8 ev_mode;
+		unsigned int time_cntr;
+	} pm_ctx;
+};
+
+static u8 ecap_cnt_capture_get_evmode(struct counter_device *counter)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+	u8 ev_mode = 0;
+	unsigned int regval;
+	int i;
+
+	pm_runtime_get_sync(counter->parent);
+	regmap_read(ecap_dev->regmap, ECAP_ECCTL_REG, &regval);
+	pm_runtime_put_sync(counter->parent);
+
+	for (i = 0 ; i < ECAP_NB_CAP ; i++) {
+		if (regval & ECAP_CAPPOL_BIT(i))
+			ev_mode |= ECAP_EV_MODE_BIT(i);
+	}
+
+	return ev_mode;
+}
+
+static void ecap_cnt_capture_set_evmode(struct counter_device *counter, u8 ev_mode)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+	unsigned int regval = 0;
+	int i;
+
+	for (i = 0 ; i < ECAP_NB_CAP ; i++) {
+		if (ev_mode & ECAP_EV_MODE_BIT(i))
+			regval |= ECAP_CAPPOL_BIT(i);
+	}
+
+	pm_runtime_get_sync(counter->parent);
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_EV_MODE_MASK, regval);
+	pm_runtime_put_sync(counter->parent);
+}
+
+static void ecap_cnt_capture_enable(struct counter_device *counter, bool rearm)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+	unsigned int regval;
+
+	pm_runtime_get_sync(counter->parent);
+
+	/* Enable interrupts on events */
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG,
+			   ECAP_CEVT_EN_MASK, ECAP_CEVT_EN_MASK);
+
+	/* Run counter */
+	regval = ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK;
+	if (rearm) {
+		ecap_dev->elapsed_time = 0;
+		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, 0);
+		regval |= ECAP_REARM_RESET_BIT;
+	}
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_CFG_MASK, regval);
+}
+
+static void ecap_cnt_capture_disable(struct counter_device *counter)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	/* Disable interrupts on events */
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, ECAP_CEVT_EN_MASK, 0);
+
+	/* Stop counter */
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_EN_MASK, 0);
+
+	pm_runtime_put_sync(counter->parent);
+}
+
+static int ecap_cnt_count_get_ns(struct counter_device *counter, unsigned int reg, u64 *val)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+	unsigned int regval;
+	unsigned long flags;
+
+	pm_runtime_get_sync(counter->parent);
+	spin_lock_irqsave(&ecap_dev->lock, flags);
+
+	/* time_ns = 10^9 * (time_cycles + time_cumul) / clk_rate */
+	regmap_read(ecap_dev->regmap, reg, &regval);
+	*val = (regval + ecap_dev->elapsed_time) * NSEC_PER_SEC;
+	do_div(*val, ecap_dev->clk_rate);
+
+	spin_unlock_irqrestore(&ecap_dev->lock, flags);
+	pm_runtime_put_sync(counter->parent);
+
+	return 0;
+}
+
+static inline int ecap_cnt_count_read(struct counter_device *counter,
+				      struct counter_count *count,
+				      u64 *val)
+{
+	return ecap_cnt_count_get_ns(counter, ECAP_TSCNT_REG, val);
+}
+
+static int ecap_cnt_function_read(struct counter_device *counter,
+				  struct counter_count *count,
+				  enum counter_function *function)
+{
+	*function = COUNTER_FUNCTION_INCREASE;
+
+	return 0;
+}
+
+static int ecap_cnt_action_read(struct counter_device *counter,
+				struct counter_count *count,
+				struct counter_synapse *synapse,
+				enum counter_synapse_action *action)
+{
+	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
+
+	return 0;
+}
+
+static int ecap_cnt_watch_validate(struct counter_device *counter,
+				   const struct counter_watch *watch)
+{
+	struct counter_event_node *event_node;
+
+	if (watch->channel || watch->event != COUNTER_EVENT_CAPTURE)
+		return -EINVAL;
+
+	list_for_each_entry(event_node, &counter->next_events_list, l)
+		if (watch->component.type != COUNTER_COMPONENT_EXTENSION ||
+		    watch->component.scope != COUNTER_SCOPE_COUNT ||
+		    watch->component.parent ||
+		    watch->component.id >= ECAP_NB_CAP)
+			return -EINVAL;
+
+	return 0;
+}
+
+static int ecap_cnt_enable_read(struct counter_device *counter,
+				struct counter_count *count,
+				u8 *enable)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	*enable = ecap_dev->enabled;
+
+	return 0;
+}
+
+static int ecap_cnt_enable_write(struct counter_device *counter,
+				 struct counter_count *count,
+				 u8 enable)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	if (enable > 1)
+		return -EINVAL;
+	if (enable == ecap_dev->enabled)
+		return 0;
+	if (enable)
+		ecap_cnt_capture_enable(counter, true);
+	else
+		ecap_cnt_capture_disable(counter);
+	ecap_dev->enabled = enable;
+
+	return 0;
+}
+
+static int ecap_cnt_cap_get_pol(struct counter_device *counter,
+				struct counter_signal *signal,
+				u32 *pol, u8 inst)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	pm_runtime_get_sync(counter->parent);
+	*pol = regmap_test_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
+	pm_runtime_put_sync(counter->parent);
+
+	return 0;
+}
+
+static int ecap_cnt_cap_set_pol(struct counter_device *counter,
+				struct counter_signal *signal,
+				u32 pol, u8 inst)
+{
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
+
+	if (ecap_dev->enabled)
+		return -EBUSY;
+	if (pol > 1)
+		return -EINVAL;
+
+	pm_runtime_get_sync(counter->parent);
+	if (pol)
+		regmap_set_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
+	else
+		regmap_clear_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
+	pm_runtime_put_sync(counter->parent);
+
+	return 0;
+}
+
+static inline int ecap_cnt_cap_read(struct counter_device *counter,
+				    struct counter_count *count,
+				    u64 *cap, u8 inst)
+{
+	return ecap_cnt_count_get_ns(counter, ECAP_CAP_REG(inst), cap);
+}
+
+static inline int ecap_cnt_cap1_get_pol(struct counter_device *counter,
+					struct counter_signal *signal, u32 *pol)
+{
+	return ecap_cnt_cap_get_pol(counter, signal, pol, 0);
+}
+
+static inline int ecap_cnt_cap2_get_pol(struct counter_device *counter,
+					struct counter_signal *signal, u32 *pol)
+{
+	return ecap_cnt_cap_get_pol(counter, signal, pol, 1);
+}
+
+static inline int ecap_cnt_cap3_get_pol(struct counter_device *counter,
+					struct counter_signal *signal, u32 *pol)
+{
+	return ecap_cnt_cap_get_pol(counter, signal, pol, 2);
+}
+
+static inline int ecap_cnt_cap4_get_pol(struct counter_device *counter,
+					struct counter_signal *signal, u32 *pol)
+{
+	return ecap_cnt_cap_get_pol(counter, signal, pol, 3);
+}
+
+static inline int ecap_cnt_cap1_set_pol(struct counter_device *counter,
+					struct counter_signal *signal, u32 pol)
+{
+	return ecap_cnt_cap_set_pol(counter, signal, pol, 0);
+}
+
+static inline int ecap_cnt_cap2_set_pol(struct counter_device *counter,
+					struct counter_signal *signal, u32 pol)
+{
+	return ecap_cnt_cap_set_pol(counter, signal, pol, 1);
+}
+
+static inline int ecap_cnt_cap3_set_pol(struct counter_device *counter,
+					struct counter_signal *signal, u32 pol)
+{
+	return ecap_cnt_cap_set_pol(counter, signal, pol, 2);
+}
+
+static inline int ecap_cnt_cap4_set_pol(struct counter_device *counter,
+					struct counter_signal *signal, u32 pol)
+{
+	return ecap_cnt_cap_set_pol(counter, signal, pol, 3);
+}
+
+static inline int ecap_cnt_cap1_read(struct counter_device *counter,
+				     struct counter_count *count, u64 *cap)
+{
+	return ecap_cnt_cap_read(counter, count, cap, 0);
+}
+
+static inline int ecap_cnt_cap2_read(struct counter_device *counter,
+				     struct counter_count *count, u64 *cap)
+{
+	return ecap_cnt_cap_read(counter, count, cap, 1);
+}
+
+static inline int ecap_cnt_cap3_read(struct counter_device *counter,
+				     struct counter_count *count, u64 *cap)
+{
+	return ecap_cnt_cap_read(counter, count, cap, 2);
+}
+
+static inline int ecap_cnt_cap4_read(struct counter_device *counter,
+				     struct counter_count *count, u64 *cap)
+{
+	return ecap_cnt_cap_read(counter, count, cap, 3);
+}
+
+static const struct counter_ops ecap_cnt_ops = {
+	.count_read = ecap_cnt_count_read,
+	.function_read = ecap_cnt_function_read,
+	.action_read = ecap_cnt_action_read,
+	.watch_validate = ecap_cnt_watch_validate,
+};
+
+static const enum counter_function ecap_cnt_functions[] = {
+	COUNTER_FUNCTION_INCREASE,
+};
+
+static const enum counter_synapse_action ecap_cnt_actions[] = {
+	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
+};
+
+static const char *const ecap_cnt_cap_polarities[] = {
+	"rising",
+	"falling",
+};
+
+static DEFINE_COUNTER_ENUM(ecap_cnt_cap_avail_pol, ecap_cnt_cap_polarities);
+
+static struct counter_comp ecap_cnt_signal_ext[] = {
+	COUNTER_COMP_SIGNAL_ENUM("polarity1", ecap_cnt_cap1_get_pol,
+				 ecap_cnt_cap1_set_pol, ecap_cnt_cap_avail_pol),
+	COUNTER_COMP_SIGNAL_ENUM("polarity2", ecap_cnt_cap2_get_pol,
+				 ecap_cnt_cap2_set_pol, ecap_cnt_cap_avail_pol),
+	COUNTER_COMP_SIGNAL_ENUM("polarity3", ecap_cnt_cap3_get_pol,
+				 ecap_cnt_cap3_set_pol, ecap_cnt_cap_avail_pol),
+	COUNTER_COMP_SIGNAL_ENUM("polarity4", ecap_cnt_cap4_get_pol,
+				 ecap_cnt_cap4_set_pol, ecap_cnt_cap_avail_pol),
+};
+
+static struct counter_signal ecap_cnt_signals[] = {
+	{
+		.id = ECAP_SIGNAL,
+		.name = "ECAPSIG",
+		.ext = ecap_cnt_signal_ext,
+		.num_ext = ARRAY_SIZE(ecap_cnt_signal_ext),
+	},
+};
+
+static struct counter_synapse ecap_cnt_synapses[] = {
+	{
+		.actions_list = ecap_cnt_actions,
+		.num_actions = ARRAY_SIZE(ecap_cnt_actions),
+		.signal = &ecap_cnt_signals[ECAP_SIGNAL],
+	},
+};
+
+static struct counter_comp ecap_cnt_count_ext[] = {
+	COUNTER_COMP_COUNT_U64("capture1", ecap_cnt_cap1_read, NULL),
+	COUNTER_COMP_COUNT_U64("capture2", ecap_cnt_cap2_read, NULL),
+	COUNTER_COMP_COUNT_U64("capture3", ecap_cnt_cap3_read, NULL),
+	COUNTER_COMP_COUNT_U64("capture4", ecap_cnt_cap4_read, NULL),
+	COUNTER_COMP_ENABLE(ecap_cnt_enable_read, ecap_cnt_enable_write),
+};
+
+static struct counter_count ecap_cnt_counts[] = {
+	{
+		.id = 0,
+		.name = "ECAPCNT",
+		.functions_list = ecap_cnt_functions,
+		.num_functions = ARRAY_SIZE(ecap_cnt_functions),
+		.synapses = ecap_cnt_synapses,
+		.num_synapses = ARRAY_SIZE(ecap_cnt_synapses),
+		.ext = ecap_cnt_count_ext,
+		.num_ext = ARRAY_SIZE(ecap_cnt_count_ext),
+	},
+};
+
+static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
+{
+	struct counter_device *counter_dev = dev_id;
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
+	unsigned int clr = 0;
+	unsigned int flg;
+	int i;
+	unsigned long flags;
+
+	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
+
+	for (i = ECAP_NB_CAP - 1 ; i < ECAP_NB_CEVT ; i++) {
+		if (flg & ECAP_CEVT_FLG_BIT(i)) {
+			if (i < ECAP_NB_CAP) {
+				/* Input signal edge detected on last CAP (CAP4) */
+				counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, 0);
+			} else {
+				/* Counter overflow */
+				spin_lock_irqsave(&ecap_dev->lock, flags);
+				ecap_dev->elapsed_time += (u64)U32_MAX + 1;
+				spin_unlock_irqrestore(&ecap_dev->lock, flags);
+			}
+
+			clr |= ECAP_CEVT_CLR_BIT(i);
+		}
+	}
+
+	clr |= ECAP_INT_CLR_BIT;
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_CLR_FRC_REG, ECAP_CEVT_CLR_MASK, clr);
+
+	return IRQ_HANDLED;
+}
+
+static void ecap_cnt_clk_disable(void *clk)
+{
+	clk_disable_unprepare(clk);
+}
+
+static int ecap_cnt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ecap_cnt_dev *ecap_dev;
+	struct counter_device *counter_dev;
+	void __iomem *mmio_base;
+	int ret;
+
+	counter_dev = devm_counter_alloc(dev, sizeof(*ecap_dev));
+	if (IS_ERR(counter_dev))
+		return PTR_ERR(counter_dev);
+
+	counter_dev->name = ECAP_DRV_NAME;
+	counter_dev->parent = dev;
+	counter_dev->ops = &ecap_cnt_ops;
+	counter_dev->signals = ecap_cnt_signals;
+	counter_dev->num_signals = ARRAY_SIZE(ecap_cnt_signals);
+	counter_dev->counts = ecap_cnt_counts;
+	counter_dev->num_counts = ARRAY_SIZE(ecap_cnt_counts);
+
+	ecap_dev = counter_priv(counter_dev);
+
+	ecap_dev->clk = devm_clk_get(dev, "fck");
+	if (IS_ERR(ecap_dev->clk))
+		return dev_err_probe(dev, PTR_ERR(ecap_dev->clk), "failed to get clock\n");
+
+	ret = clk_prepare_enable(ecap_dev->clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clock\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, ecap_cnt_clk_disable, ecap_dev->clk);
+	if (ret) {
+		dev_err(dev, "failed to add clock disable action\n");
+		return ret;
+	}
+
+	ecap_dev->clk_rate = clk_get_rate(ecap_dev->clk);
+	if (!ecap_dev->clk_rate) {
+		dev_err(dev, "failed to get clock rate\n");
+		return -EINVAL;
+	}
+
+	mmio_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mmio_base))
+		return PTR_ERR(mmio_base);
+
+	ecap_dev->regmap = devm_regmap_init_mmio(dev, mmio_base, &ecap_cnt_regmap_config);
+	if (IS_ERR(ecap_dev->regmap))
+		return dev_err_probe(dev, PTR_ERR(ecap_dev->regmap), "failed to init regmap\n");
+
+	spin_lock_init(&ecap_dev->lock);
+
+	ret = platform_get_irq(pdev, 0);
+	if (ret < 0) {
+		dev_err(dev, "failed to get irq\n");
+		return ret;
+	}
+
+	ret = devm_request_irq(dev, ret, ecap_cnt_isr, 0, pdev->name, counter_dev);
+	if (ret) {
+		dev_err(dev, "failed to request irq\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, counter_dev);
+	pm_runtime_enable(dev);
+
+	ecap_dev->enabled = 0;
+	ecap_cnt_capture_set_evmode(counter_dev, 0);
+
+	ret = devm_counter_add(dev, counter_dev);
+	if (ret) {
+		dev_err(dev, "failed to add counter\n");
+		pm_runtime_disable(dev);
+	}
+
+	return ret;
+}
+
+static int ecap_cnt_remove(struct platform_device *pdev)
+{
+	struct counter_device *counter_dev = platform_get_drvdata(pdev);
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
+
+	if (ecap_dev->enabled)
+		ecap_cnt_capture_disable(counter_dev);
+
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static int ecap_cnt_suspend(struct device *dev)
+{
+	struct counter_device *counter_dev = dev_get_drvdata(dev);
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
+
+	/* If eCAP is running, stop capture then save timestamp counter */
+	if (ecap_dev->enabled) {
+		ecap_cnt_capture_disable(counter_dev);
+
+		pm_runtime_get_sync(dev);
+		regmap_read(ecap_dev->regmap, ECAP_TSCNT_REG, &ecap_dev->pm_ctx.time_cntr);
+		pm_runtime_put_sync(dev);
+	}
+
+	ecap_dev->pm_ctx.ev_mode = ecap_cnt_capture_get_evmode(counter_dev);
+
+	return 0;
+}
+
+static int ecap_cnt_resume(struct device *dev)
+{
+	struct counter_device *counter_dev = dev_get_drvdata(dev);
+	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
+
+	ecap_cnt_capture_set_evmode(counter_dev, ecap_dev->pm_ctx.ev_mode);
+
+	/* If eCAP was running, restore timestamp counter then run capture */
+	if (ecap_dev->enabled) {
+		pm_runtime_get_sync(dev);
+		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, ecap_dev->pm_ctx.time_cntr);
+		pm_runtime_put_sync(dev);
+
+		ecap_cnt_capture_enable(counter_dev, false);
+	}
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(ecap_cnt_pm_ops, ecap_cnt_suspend, ecap_cnt_resume);
+
+static const struct of_device_id ecap_cnt_of_match[] = {
+	{ .compatible	= "ti,am62-ecap-capture" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ecap_cnt_of_match);
+
+static struct platform_driver ecap_cnt_driver = {
+	.probe = ecap_cnt_probe,
+	.remove = ecap_cnt_remove,
+	.driver = {
+		.name = "ecap-capture",
+		.of_match_table = ecap_cnt_of_match,
+		.pm = pm_sleep_ptr(&ecap_cnt_pm_ops),
+	},
+};
+module_platform_driver(ecap_cnt_driver);
+
+MODULE_DESCRIPTION("ECAP Capture driver");
+MODULE_AUTHOR("Julien Panis <jpanis@baylibre.com>");
+MODULE_LICENSE("GPL");
diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
index 96c5ffd368ad..4c5372c6f2a3 100644
--- a/include/uapi/linux/counter.h
+++ b/include/uapi/linux/counter.h
@@ -63,6 +63,8 @@ enum counter_event_type {
 	COUNTER_EVENT_INDEX,
 	/* State of counter is changed */
 	COUNTER_EVENT_CHANGE_OF_STATE,
+	/* Count value is captured */
+	COUNTER_EVENT_CAPTURE,
 };
 
 /**
-- 
2.25.1

Re: [PATCH v4 3/3] counter: capture-tiecap: capture driver support for ECAP

[Jonathan Cameron]

On Wed, 10 Aug 2022 16:07:24 +0200
Julien Panis <jpanis@baylibre.com> wrote:

> ECAP hardware on AM62x SoC supports capture feature. It can be used
> to timestamp events (falling/rising edges) detected on signal input pin.
> 
> This commit adds capture driver support for ECAP hardware on AM62x SoC.
> 
> In the ECAP hardware, capture pin can also be configured to be in
> PWM mode. Current implementation only supports capture operating mode.
> Hardware also supports timebase sync between multiple instances, but
> this driver supports simple independent capture functionality.
> 
> Signed-off-by: Julien Panis <jpanis@baylibre.com>
> ---
Hi Julien,

Been too long since I reviewed a counter driver, so I'll have
to leave that side of things to William (or find a spare few days
to remind myself of the details!)

A few superficial things I notice below whilst taking a quick look.

It might be worth adding a bit of description around what the
result of the various runtime pm calls is (what's actually getting
powered down?)

Jonathan

> +static int ecap_cnt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ecap_cnt_dev *ecap_dev;
> +	struct counter_device *counter_dev;
> +	void __iomem *mmio_base;
> +	int ret;
> +
> +	counter_dev = devm_counter_alloc(dev, sizeof(*ecap_dev));
> +	if (IS_ERR(counter_dev))
> +		return PTR_ERR(counter_dev);
> +
> +	counter_dev->name = ECAP_DRV_NAME;
> +	counter_dev->parent = dev;
> +	counter_dev->ops = &ecap_cnt_ops;
> +	counter_dev->signals = ecap_cnt_signals;
> +	counter_dev->num_signals = ARRAY_SIZE(ecap_cnt_signals);
> +	counter_dev->counts = ecap_cnt_counts;
> +	counter_dev->num_counts = ARRAY_SIZE(ecap_cnt_counts);
> +
> +	ecap_dev = counter_priv(counter_dev);
> +
> +	ecap_dev->clk = devm_clk_get(dev, "fck");
> +	if (IS_ERR(ecap_dev->clk))
> +		return dev_err_probe(dev, PTR_ERR(ecap_dev->clk), "failed to get clock\n");
> +
> +	ret = clk_prepare_enable(ecap_dev->clk);

We now have dev_clk_get_enabled() in upstream - finally!
Text book usecase for it here.

> +	if (ret) {
> +		dev_err(dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, ecap_cnt_clk_disable, ecap_dev->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to add clock disable action\n");
> +		return ret;
> +	}
> +
> +	ecap_dev->clk_rate = clk_get_rate(ecap_dev->clk);
> +	if (!ecap_dev->clk_rate) {
> +		dev_err(dev, "failed to get clock rate\n");
> +		return -EINVAL;
> +	}
> +
> +	mmio_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mmio_base))
> +		return PTR_ERR(mmio_base);
> +
> +	ecap_dev->regmap = devm_regmap_init_mmio(dev, mmio_base, &ecap_cnt_regmap_config);
> +	if (IS_ERR(ecap_dev->regmap))
> +		return dev_err_probe(dev, PTR_ERR(ecap_dev->regmap), "failed to init regmap\n");
> +
> +	spin_lock_init(&ecap_dev->lock);
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get irq\n");
> +		return ret;
> +	}
> +
> +	ret = devm_request_irq(dev, ret, ecap_cnt_isr, 0, pdev->name, counter_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, counter_dev);
> +	pm_runtime_enable(dev);
> +
> +	ecap_dev->enabled = 0;
> +	ecap_cnt_capture_set_evmode(counter_dev, 0);
> +
> +	ret = devm_counter_add(dev, counter_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to add counter\n");
> +		pm_runtime_disable(dev);

Unless there is a very good reason to mix devm and unmanaged
code, it's best to not do so as it leads to much head scratching over
whether there are race conditions.  Here I can't see a reason not
to use devm_add_action_or_reset() to make the pm_runtime_disabled()
into devm managed.

Any setting of enabled occured after probe() was done so that's
fine as unmanaged or you could register another devm_ callback
if you prefer, but with a comment explaining the path to it needing
to do anything.

> +	}
> +
> +	return ret;
> +}
> +
> +static int ecap_cnt_remove(struct platform_device *pdev)
> +{
> +	struct counter_device *counter_dev = platform_get_drvdata(pdev);
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> +
> +	if (ecap_dev->enabled)
> +		ecap_cnt_capture_disable(counter_dev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	return 0;
> +}
> +

Re: [PATCH v4 3/3] counter: capture-tiecap: capture driver support for ECAP

[William Breathitt Gray]

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

On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
> ECAP hardware on AM62x SoC supports capture feature. It can be used
> to timestamp events (falling/rising edges) detected on signal input pin.
> 
> This commit adds capture driver support for ECAP hardware on AM62x SoC.
> 
> In the ECAP hardware, capture pin can also be configured to be in
> PWM mode. Current implementation only supports capture operating mode.
> Hardware also supports timebase sync between multiple instances, but
> this driver supports simple independent capture functionality.
> 
> Signed-off-by: Julien Panis <jpanis@baylibre.com>
Hi Julien,

You picked up the Counter paradigm pretty quickly, good job! Some
suggestions and comments inline below.

> ---
>  drivers/counter/Kconfig          |  14 +
>  drivers/counter/Makefile         |   1 +
>  drivers/counter/capture-tiecap.c | 634 +++++++++++++++++++++++++++++++
>  include/uapi/linux/counter.h     |   2 +
>  4 files changed, 651 insertions(+)
>  create mode 100644 drivers/counter/capture-tiecap.c
> 
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 5edd155f1911..580640807a94 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -101,4 +101,18 @@ config INTEL_QEP
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called intel-qep.
>  
> +config CAPTURE_TIECAP
> +	tristate "TI eCAP capture driver"
> +	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  Select this option to enable the Texas Instruments Enhanced Capture
> +	  (eCAP) driver in input mode.
> +
> +	  It can be used to timestamp events (falling/rising edges) detected
> +	  on signal input pin.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called capture-tiecap.
> +

The naming convention I typically see is to lead with the company name
followed by device model; e.g. ti-ecap-capture. That should result in
better grouping with the existing drivers in the drivers/counter
directory. It's a minor benefit, so let's use that convention unless
there's a reason why capture-tiecap is better here.

>  endif # COUNTER
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index 8fde6c100ebc..7fac3e0985b5 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
>  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
>  obj-$(CONFIG_MICROCHIP_TCB_CAPTURE)	+= microchip-tcb-capture.o
>  obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
> +obj-$(CONFIG_CAPTURE_TIECAP)	+= capture-tiecap.o
> diff --git a/drivers/counter/capture-tiecap.c b/drivers/counter/capture-tiecap.c
> new file mode 100644
> index 000000000000..0601c65ef203
> --- /dev/null
> +++ b/drivers/counter/capture-tiecap.c
> @@ -0,0 +1,634 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * ECAP Capture driver
> + *
> + * Copyright (C) 2022 Julien Panis <jpanis@baylibre.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/counter.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +#define ECAP_DRV_NAME "ecap"
> +
> +/* Registers */
> +#define ECAP_NB_CAP			4
> +
> +#define ECAP_TSCNT_REG			0x00
> +
> +#define ECAP_CAP_REG(i)		(((i) << 2) + 0x08)
> +
> +#define ECAP_ECCTL_REG			0x28
> +#define ECAP_CAPPOL_BIT(i)		BIT((i) << 1)
> +#define ECAP_EV_MODE_MASK		GENMASK(7, 0)
> +#define ECAP_CAPLDEN_BIT		BIT(8)
> +#define ECAP_CONT_ONESHT_BIT		BIT(16)
> +#define ECAP_STOPVALUE_MASK		GENMASK(18, 17)
> +#define ECAP_REARM_RESET_BIT		BIT(19)
> +#define ECAP_TSCNTSTP_BIT		BIT(20)
> +#define ECAP_SYNCO_DIS_MASK		GENMASK(23, 22)
> +#define ECAP_CAP_APWM_BIT		BIT(25)
> +#define ECAP_ECCTL_EN_MASK		(ECAP_CAPLDEN_BIT | ECAP_TSCNTSTP_BIT)
> +#define ECAP_ECCTL_CFG_MASK		(ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK	\
> +					| ECAP_ECCTL_EN_MASK | ECAP_REARM_RESET_BIT	\
> +					| ECAP_CAP_APWM_BIT | ECAP_CONT_ONESHT_BIT)
> +
> +#define ECAP_ECINT_EN_FLG_REG		0x2c
> +#define ECAP_NB_CEVT			(ECAP_NB_CAP + 1)
> +#define ECAP_CEVT_EN_MASK		GENMASK(ECAP_NB_CEVT, ECAP_NB_CAP)
> +#define ECAP_CEVT_FLG_BIT(i)		BIT((i) + 17)
> +
> +#define ECAP_ECINT_CLR_FRC_REG	0x30
> +#define ECAP_INT_CLR_BIT		BIT(0)
> +#define ECAP_CEVT_CLR_BIT(i)		BIT((i) + 1)
> +#define ECAP_CEVT_CLR_MASK		GENMASK(ECAP_NB_CEVT, 0)
> +
> +#define ECAP_PID_REG			0x5c
> +
> +/*
> + * Event modes
> + * One bit for each CAPx register : 1 = falling edge / 0 = rising edge
> + * e.g. mode = 13 = 0xd = 0b1101
> + * -> falling edge for CAP1-3-4 / rising edge for CAP2
> + */
> +#define ECAP_EV_MODE_BIT(i)		BIT(i)
> +
> +/* ECAP input */
> +#define ECAP_SIGNAL 0
> +
> +static const struct regmap_config ecap_cnt_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.max_register = ECAP_PID_REG,
> +};
> +
> +/**
> + * struct ecap_cnt_dev - device private data structure
> + * @enabled:      device state
> + * @clk:          device clock
> + * @clk_rate:     device clock rate
> + * @regmap:       device register map
> + * @elapsed_time: elapsed time since capture start
> + * @lock:         synchronization lock to prevent race conditions when computing times
> + * @pm_ctx:       device context for PM operations
> + */
> +struct ecap_cnt_dev {
> +	bool enabled;
> +	struct clk *clk;
> +	unsigned long clk_rate;
> +	struct regmap *regmap;
> +	__aligned_u64 elapsed_time;
> +	spinlock_t lock;
> +	struct {
> +		u8 ev_mode;
> +		unsigned int time_cntr;
> +	} pm_ctx;
> +};
> +
> +static u8 ecap_cnt_capture_get_evmode(struct counter_device *counter)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +	u8 ev_mode = 0;
> +	unsigned int regval;
> +	int i;
> +
> +	pm_runtime_get_sync(counter->parent);
> +	regmap_read(ecap_dev->regmap, ECAP_ECCTL_REG, &regval);
> +	pm_runtime_put_sync(counter->parent);
> +
> +	for (i = 0 ; i < ECAP_NB_CAP ; i++) {
> +		if (regval & ECAP_CAPPOL_BIT(i))
> +			ev_mode |= ECAP_EV_MODE_BIT(i);
> +	}
> +
> +	return ev_mode;
> +}
> +
> +static void ecap_cnt_capture_set_evmode(struct counter_device *counter, u8 ev_mode)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +	unsigned int regval = 0;
> +	int i;
> +
> +	for (i = 0 ; i < ECAP_NB_CAP ; i++) {
> +		if (ev_mode & ECAP_EV_MODE_BIT(i))
> +			regval |= ECAP_CAPPOL_BIT(i);
> +	}
> +
> +	pm_runtime_get_sync(counter->parent);
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_EV_MODE_MASK, regval);
> +	pm_runtime_put_sync(counter->parent);
> +}
> +
> +static void ecap_cnt_capture_enable(struct counter_device *counter, bool rearm)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +	unsigned int regval;
> +
> +	pm_runtime_get_sync(counter->parent);
> +
> +	/* Enable interrupts on events */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG,
> +			   ECAP_CEVT_EN_MASK, ECAP_CEVT_EN_MASK);
> +
> +	/* Run counter */
> +	regval = ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK;
> +	if (rearm) {
> +		ecap_dev->elapsed_time = 0;
> +		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, 0);
> +		regval |= ECAP_REARM_RESET_BIT;
> +	}
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_CFG_MASK, regval);
> +}
> +
> +static void ecap_cnt_capture_disable(struct counter_device *counter)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	/* Disable interrupts on events */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, ECAP_CEVT_EN_MASK, 0);
> +
> +	/* Stop counter */
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_EN_MASK, 0);
> +
> +	pm_runtime_put_sync(counter->parent);
> +}
> +
> +static int ecap_cnt_count_get_ns(struct counter_device *counter, unsigned int reg, u64 *val)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +	unsigned int regval;
> +	unsigned long flags;
> +
> +	pm_runtime_get_sync(counter->parent);
> +	spin_lock_irqsave(&ecap_dev->lock, flags);
> +
> +	/* time_ns = 10^9 * (time_cycles + time_cumul) / clk_rate */
> +	regmap_read(ecap_dev->regmap, reg, &regval);
> +	*val = (regval + ecap_dev->elapsed_time) * NSEC_PER_SEC;
> +	do_div(*val, ecap_dev->clk_rate);
> +
> +	spin_unlock_irqrestore(&ecap_dev->lock, flags);
> +	pm_runtime_put_sync(counter->parent);
> +
> +	return 0;
> +}
> +
> +static inline int ecap_cnt_count_read(struct counter_device *counter,
> +				      struct counter_count *count,
> +				      u64 *val)
> +{
> +	return ecap_cnt_count_get_ns(counter, ECAP_TSCNT_REG, val);
> +}

This conversion to nanoseconds is an interpretation of the count value
that belongs in userspace. Instead of converting to nanoseconds here,
simply expose the count value directly and add extensions to expose the
elapsed time (a Count extension) and clock rate (a Signal extension) as
well; with these three values you can derive nanoseconds in userspace
using the time_ns equation in your ecap_cnt_count_get_ns() comment.

> +static int ecap_cnt_function_read(struct counter_device *counter,
> +				  struct counter_count *count,
> +				  enum counter_function *function)
> +{
> +	*function = COUNTER_FUNCTION_INCREASE;
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_action_read(struct counter_device *counter,
> +				struct counter_count *count,
> +				struct counter_synapse *synapse,
> +				enum counter_synapse_action *action)
> +{
> +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> +
> +	return 0;
> +}

Right now you have a Signal defined for the ECAPSIG line, but there is
at least one more relevant Signal to define: the clock updating ECAPCNT.
The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
clock Signal, but for the ECAPSIG Signal you will need to report a
Synapse action based on the polarity of the next capture (i.e. whether
high or low).

> +
> +static int ecap_cnt_watch_validate(struct counter_device *counter,
> +				   const struct counter_watch *watch)
> +{
> +	struct counter_event_node *event_node;
> +
> +	if (watch->channel || watch->event != COUNTER_EVENT_CAPTURE)
> +		return -EINVAL;

The intention of this conditional is more obvious if you explicitly
check for watch->channel != 0.

> +
> +	list_for_each_entry(event_node, &counter->next_events_list, l)
> +		if (watch->component.type != COUNTER_COMPONENT_EXTENSION ||
> +		    watch->component.scope != COUNTER_SCOPE_COUNT ||
> +		    watch->component.parent ||
> +		    watch->component.id >= ECAP_NB_CAP)
> +			return -EINVAL;

You don't need this list_for_each_entry loop at all. The purpose of the
watch_validate() callback is to make sure a watch isn't added with a
channel or event configuration that conflicts with existing watches in
the next_events_list list.

For example, the 104-QUAD-8 device only allows one particular event to
occur on any given channel. The quad8_watch_validate() function makes
sure that all watches for a particular channel are set for the same
event; if a watch for that channel is set for a different event, then
that watch is invalid because the 104-QUAD-8 device cannot handle such
a request for two different events on the same channel.

Regarding the TI ECAP device, it looks like it only supports that one
COUNTER_EVENT_CAPTURE event so you don't need to check the other watches
in the next_events_list list. Your conditional check above for
watch->channel and watch->event should be enough by itself.

> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_enable_read(struct counter_device *counter,
> +				struct counter_count *count,
> +				u8 *enable)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	*enable = ecap_dev->enabled;
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_enable_write(struct counter_device *counter,
> +				 struct counter_count *count,
> +				 u8 enable)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	if (enable > 1)
> +		return -EINVAL;
> +	if (enable == ecap_dev->enabled)
> +		return 0;
> +	if (enable)
> +		ecap_cnt_capture_enable(counter, true);
> +	else
> +		ecap_cnt_capture_disable(counter);
> +	ecap_dev->enabled = enable;
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_cap_get_pol(struct counter_device *counter,
> +				struct counter_signal *signal,
> +				u32 *pol, u8 inst)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	pm_runtime_get_sync(counter->parent);
> +	*pol = regmap_test_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
> +	pm_runtime_put_sync(counter->parent);
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_cap_set_pol(struct counter_device *counter,
> +				struct counter_signal *signal,
> +				u32 pol, u8 inst)
> +{
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
> +
> +	if (ecap_dev->enabled)
> +		return -EBUSY;
> +	if (pol > 1)
> +		return -EINVAL;
> +
> +	pm_runtime_get_sync(counter->parent);
> +	if (pol)
> +		regmap_set_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
> +	else
> +		regmap_clear_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
> +	pm_runtime_put_sync(counter->parent);
> +
> +	return 0;
> +}
> +
> +static inline int ecap_cnt_cap_read(struct counter_device *counter,
> +				    struct counter_count *count,
> +				    u64 *cap, u8 inst)
> +{
> +	return ecap_cnt_count_get_ns(counter, ECAP_CAP_REG(inst), cap);
> +}

Return the captured count value directly here; userspace can handle the
nanosecond interpretation for the same reason explained earlier for the
ECAPCNT count value.

> +static inline int ecap_cnt_cap1_get_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 *pol)
> +{
> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 0);
> +}
> +
> +static inline int ecap_cnt_cap2_get_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 *pol)
> +{
> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 1);
> +}
> +
> +static inline int ecap_cnt_cap3_get_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 *pol)
> +{
> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 2);
> +}
> +
> +static inline int ecap_cnt_cap4_get_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 *pol)
> +{
> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 3);
> +}
> +
> +static inline int ecap_cnt_cap1_set_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 pol)
> +{
> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 0);
> +}
> +
> +static inline int ecap_cnt_cap2_set_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 pol)
> +{
> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 1);
> +}
> +
> +static inline int ecap_cnt_cap3_set_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 pol)
> +{
> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 2);
> +}
> +
> +static inline int ecap_cnt_cap4_set_pol(struct counter_device *counter,
> +					struct counter_signal *signal, u32 pol)
> +{
> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 3);
> +}
> +
> +static inline int ecap_cnt_cap1_read(struct counter_device *counter,
> +				     struct counter_count *count, u64 *cap)
> +{
> +	return ecap_cnt_cap_read(counter, count, cap, 0);
> +}
> +
> +static inline int ecap_cnt_cap2_read(struct counter_device *counter,
> +				     struct counter_count *count, u64 *cap)
> +{
> +	return ecap_cnt_cap_read(counter, count, cap, 1);
> +}
> +
> +static inline int ecap_cnt_cap3_read(struct counter_device *counter,
> +				     struct counter_count *count, u64 *cap)
> +{
> +	return ecap_cnt_cap_read(counter, count, cap, 2);
> +}
> +
> +static inline int ecap_cnt_cap4_read(struct counter_device *counter,
> +				     struct counter_count *count, u64 *cap)
> +{
> +	return ecap_cnt_cap_read(counter, count, cap, 3);
> +}

There's a lot of boilerplate code here for each ECAP register read/write
callback. Try defining a macro to build these so you can define each
read/write callback pair more succinctly.

> +static const struct counter_ops ecap_cnt_ops = {
> +	.count_read = ecap_cnt_count_read,
> +	.function_read = ecap_cnt_function_read,
> +	.action_read = ecap_cnt_action_read,
> +	.watch_validate = ecap_cnt_watch_validate,
> +};
> +
> +static const enum counter_function ecap_cnt_functions[] = {
> +	COUNTER_FUNCTION_INCREASE,
> +};
> +
> +static const enum counter_synapse_action ecap_cnt_actions[] = {
> +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
> +};
> +
> +static const char *const ecap_cnt_cap_polarities[] = {
> +	"rising",
> +	"falling",
> +};
> +
> +static DEFINE_COUNTER_ENUM(ecap_cnt_cap_avail_pol, ecap_cnt_cap_polarities);
> +
> +static struct counter_comp ecap_cnt_signal_ext[] = {
> +	COUNTER_COMP_SIGNAL_ENUM("polarity1", ecap_cnt_cap1_get_pol,
> +				 ecap_cnt_cap1_set_pol, ecap_cnt_cap_avail_pol),
> +	COUNTER_COMP_SIGNAL_ENUM("polarity2", ecap_cnt_cap2_get_pol,
> +				 ecap_cnt_cap2_set_pol, ecap_cnt_cap_avail_pol),
> +	COUNTER_COMP_SIGNAL_ENUM("polarity3", ecap_cnt_cap3_get_pol,
> +				 ecap_cnt_cap3_set_pol, ecap_cnt_cap_avail_pol),
> +	COUNTER_COMP_SIGNAL_ENUM("polarity4", ecap_cnt_cap4_get_pol,
> +				 ecap_cnt_cap4_set_pol, ecap_cnt_cap_avail_pol),
> +};
> +
> +static struct counter_signal ecap_cnt_signals[] = {
> +	{
> +		.id = ECAP_SIGNAL,
> +		.name = "ECAPSIG",
> +		.ext = ecap_cnt_signal_ext,
> +		.num_ext = ARRAY_SIZE(ecap_cnt_signal_ext),
> +	},
> +};

As mentioned earlier, define another Signal here representing the clock
feeding ECAPCNT. You should also define another extensions array for
this new Signal that will provide the clock rate for userspace to read.

> +static struct counter_synapse ecap_cnt_synapses[] = {
> +	{
> +		.actions_list = ecap_cnt_actions,
> +		.num_actions = ARRAY_SIZE(ecap_cnt_actions),
> +		.signal = &ecap_cnt_signals[ECAP_SIGNAL],
> +	},
> +};

You will need to define another Synapse here for the clock Signal
mentioned above; similarly, you need to define another actions array for
the possible edge triggers.

> +static struct counter_comp ecap_cnt_count_ext[] = {
> +	COUNTER_COMP_COUNT_U64("capture1", ecap_cnt_cap1_read, NULL),
> +	COUNTER_COMP_COUNT_U64("capture2", ecap_cnt_cap2_read, NULL),
> +	COUNTER_COMP_COUNT_U64("capture3", ecap_cnt_cap3_read, NULL),
> +	COUNTER_COMP_COUNT_U64("capture4", ecap_cnt_cap4_read, NULL),
> +	COUNTER_COMP_ENABLE(ecap_cnt_enable_read, ecap_cnt_enable_write),

I just want to verify: this enable extension should disable the ECAPCNT
count itself (i.e. no more increasing count value). Is that what's
happening here, or is this meant to disable just the captures?

> +};
> +
> +static struct counter_count ecap_cnt_counts[] = {
> +	{
> +		.id = 0,
> +		.name = "ECAPCNT",
> +		.functions_list = ecap_cnt_functions,
> +		.num_functions = ARRAY_SIZE(ecap_cnt_functions),
> +		.synapses = ecap_cnt_synapses,
> +		.num_synapses = ARRAY_SIZE(ecap_cnt_synapses),
> +		.ext = ecap_cnt_count_ext,
> +		.num_ext = ARRAY_SIZE(ecap_cnt_count_ext),
> +	},
> +};
> +
> +static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
> +{
> +	struct counter_device *counter_dev = dev_id;
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> +	unsigned int clr = 0;
> +	unsigned int flg;
> +	int i;
> +	unsigned long flags;
> +
> +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
> +
> +	for (i = ECAP_NB_CAP - 1 ; i < ECAP_NB_CEVT ; i++) {

Would you walk me through the logic for this loop. Is this for-loop
intended to loop through all four capture indices? ECAP_NB_CAP and
ECAP_NB_CEVT are defines, so your for-loop has i=3 and i<5; is this what
you want?

> +		if (flg & ECAP_CEVT_FLG_BIT(i)) {
> +			if (i < ECAP_NB_CAP) {
> +				/* Input signal edge detected on last CAP (CAP4) */
> +				counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, 0);
> +			} else {
> +				/* Counter overflow */
> +				spin_lock_irqsave(&ecap_dev->lock, flags);
> +				ecap_dev->elapsed_time += (u64)U32_MAX + 1;
> +				spin_unlock_irqrestore(&ecap_dev->lock, flags);

Should we push COUNTER_EVENT_OVERFLOW here? Since elapsed_time always
increments by the same constant (U32_MAX), it would be better to replace
this with an atomic_t "num_overflows" to track the number of overflows
with atomic_inc() and compute the particular elapsed_time later.

William Breathitt Gray

> +			}
> +
> +			clr |= ECAP_CEVT_CLR_BIT(i);
> +		}
> +	}
> +
> +	clr |= ECAP_INT_CLR_BIT;
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_CLR_FRC_REG, ECAP_CEVT_CLR_MASK, clr);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static void ecap_cnt_clk_disable(void *clk)
> +{
> +	clk_disable_unprepare(clk);
> +}
> +
> +static int ecap_cnt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ecap_cnt_dev *ecap_dev;
> +	struct counter_device *counter_dev;
> +	void __iomem *mmio_base;
> +	int ret;
> +
> +	counter_dev = devm_counter_alloc(dev, sizeof(*ecap_dev));
> +	if (IS_ERR(counter_dev))
> +		return PTR_ERR(counter_dev);
> +
> +	counter_dev->name = ECAP_DRV_NAME;
> +	counter_dev->parent = dev;
> +	counter_dev->ops = &ecap_cnt_ops;
> +	counter_dev->signals = ecap_cnt_signals;
> +	counter_dev->num_signals = ARRAY_SIZE(ecap_cnt_signals);
> +	counter_dev->counts = ecap_cnt_counts;
> +	counter_dev->num_counts = ARRAY_SIZE(ecap_cnt_counts);
> +
> +	ecap_dev = counter_priv(counter_dev);
> +
> +	ecap_dev->clk = devm_clk_get(dev, "fck");
> +	if (IS_ERR(ecap_dev->clk))
> +		return dev_err_probe(dev, PTR_ERR(ecap_dev->clk), "failed to get clock\n");
> +
> +	ret = clk_prepare_enable(ecap_dev->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, ecap_cnt_clk_disable, ecap_dev->clk);
> +	if (ret) {
> +		dev_err(dev, "failed to add clock disable action\n");
> +		return ret;
> +	}
> +
> +	ecap_dev->clk_rate = clk_get_rate(ecap_dev->clk);
> +	if (!ecap_dev->clk_rate) {
> +		dev_err(dev, "failed to get clock rate\n");
> +		return -EINVAL;
> +	}
> +
> +	mmio_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mmio_base))
> +		return PTR_ERR(mmio_base);
> +
> +	ecap_dev->regmap = devm_regmap_init_mmio(dev, mmio_base, &ecap_cnt_regmap_config);
> +	if (IS_ERR(ecap_dev->regmap))
> +		return dev_err_probe(dev, PTR_ERR(ecap_dev->regmap), "failed to init regmap\n");
> +
> +	spin_lock_init(&ecap_dev->lock);
> +
> +	ret = platform_get_irq(pdev, 0);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get irq\n");
> +		return ret;
> +	}
> +
> +	ret = devm_request_irq(dev, ret, ecap_cnt_isr, 0, pdev->name, counter_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, counter_dev);
> +	pm_runtime_enable(dev);
> +
> +	ecap_dev->enabled = 0;
> +	ecap_cnt_capture_set_evmode(counter_dev, 0);
> +
> +	ret = devm_counter_add(dev, counter_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to add counter\n");
> +		pm_runtime_disable(dev);
> +	}
> +
> +	return ret;
> +}
> +
> +static int ecap_cnt_remove(struct platform_device *pdev)
> +{
> +	struct counter_device *counter_dev = platform_get_drvdata(pdev);
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> +
> +	if (ecap_dev->enabled)
> +		ecap_cnt_capture_disable(counter_dev);
> +
> +	pm_runtime_disable(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_suspend(struct device *dev)
> +{
> +	struct counter_device *counter_dev = dev_get_drvdata(dev);
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> +
> +	/* If eCAP is running, stop capture then save timestamp counter */
> +	if (ecap_dev->enabled) {
> +		ecap_cnt_capture_disable(counter_dev);
> +
> +		pm_runtime_get_sync(dev);
> +		regmap_read(ecap_dev->regmap, ECAP_TSCNT_REG, &ecap_dev->pm_ctx.time_cntr);
> +		pm_runtime_put_sync(dev);
> +	}
> +
> +	ecap_dev->pm_ctx.ev_mode = ecap_cnt_capture_get_evmode(counter_dev);
> +
> +	return 0;
> +}
> +
> +static int ecap_cnt_resume(struct device *dev)
> +{
> +	struct counter_device *counter_dev = dev_get_drvdata(dev);
> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> +
> +	ecap_cnt_capture_set_evmode(counter_dev, ecap_dev->pm_ctx.ev_mode);
> +
> +	/* If eCAP was running, restore timestamp counter then run capture */
> +	if (ecap_dev->enabled) {
> +		pm_runtime_get_sync(dev);
> +		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, ecap_dev->pm_ctx.time_cntr);
> +		pm_runtime_put_sync(dev);
> +
> +		ecap_cnt_capture_enable(counter_dev, false);
> +	}
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(ecap_cnt_pm_ops, ecap_cnt_suspend, ecap_cnt_resume);
> +
> +static const struct of_device_id ecap_cnt_of_match[] = {
> +	{ .compatible	= "ti,am62-ecap-capture" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ecap_cnt_of_match);
> +
> +static struct platform_driver ecap_cnt_driver = {
> +	.probe = ecap_cnt_probe,
> +	.remove = ecap_cnt_remove,
> +	.driver = {
> +		.name = "ecap-capture",
> +		.of_match_table = ecap_cnt_of_match,
> +		.pm = pm_sleep_ptr(&ecap_cnt_pm_ops),
> +	},
> +};
> +module_platform_driver(ecap_cnt_driver);
> +
> +MODULE_DESCRIPTION("ECAP Capture driver");
> +MODULE_AUTHOR("Julien Panis <jpanis@baylibre.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> index 96c5ffd368ad..4c5372c6f2a3 100644
> --- a/include/uapi/linux/counter.h
> +++ b/include/uapi/linux/counter.h
> @@ -63,6 +63,8 @@ enum counter_event_type {
>  	COUNTER_EVENT_INDEX,
>  	/* State of counter is changed */
>  	COUNTER_EVENT_CHANGE_OF_STATE,
> +	/* Count value is captured */
> +	COUNTER_EVENT_CAPTURE,
>  };
>  
>  /**
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 3/3] counter: capture-tiecap: capture driver support for ECAP

[William Breathitt Gray]

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

On Sun, Aug 14, 2022 at 01:03:48PM -0400, William Breathitt Gray wrote:
> On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
> > +static int ecap_cnt_function_read(struct counter_device *counter,
> > +				  struct counter_count *count,
> > +				  enum counter_function *function)
> > +{
> > +	*function = COUNTER_FUNCTION_INCREASE;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ecap_cnt_action_read(struct counter_device *counter,
> > +				struct counter_count *count,
> > +				struct counter_synapse *synapse,
> > +				enum counter_synapse_action *action)
> > +{
> > +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > +
> > +	return 0;
> > +}
> 
> Right now you have a Signal defined for the ECAPSIG line, but there is
> at least one more relevant Signal to define: the clock updating ECAPCNT.
> The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
> clock Signal, but for the ECAPSIG Signal you will need to report a
> Synapse action based on the polarity of the next capture (i.e. whether
> high or low).

I need to make a correction here. IIUC, the ECAPSIG signal doesn't
affect the count value of ECAPCNT (ECAPSIG only triggers the captures),
so the Synapse action for ECAPSIG should always be
COUNTER_SYNAPSE_ACTION_NONE. You don't need to account for the capture
polarities because they're not relevant in this particular situation:
ECAPSIG doesn't trigger the ECAPCNT count function.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 3/3] counter: capture-tiecap: capture driver support for ECAP

[Julien Panis]

On 15/08/2022 13:20, William Breathitt Gray wrote:
> On Sun, Aug 14, 2022 at 01:03:48PM -0400, William Breathitt Gray wrote:
>> On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
>>> +static int ecap_cnt_function_read(struct counter_device *counter,
>>> +				  struct counter_count *count,
>>> +				  enum counter_function *function)
>>> +{
>>> +	*function = COUNTER_FUNCTION_INCREASE;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int ecap_cnt_action_read(struct counter_device *counter,
>>> +				struct counter_count *count,
>>> +				struct counter_synapse *synapse,
>>> +				enum counter_synapse_action *action)
>>> +{
>>> +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
>>> +
>>> +	return 0;
>>> +}
>> Right now you have a Signal defined for the ECAPSIG line, but there is
>> at least one more relevant Signal to define: the clock updating ECAPCNT.
>> The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
>> clock Signal, but for the ECAPSIG Signal you will need to report a
>> Synapse action based on the polarity of the next capture (i.e. whether
>> high or low).
> I need to make a correction here. IIUC, the ECAPSIG signal doesn't
> affect the count value of ECAPCNT (ECAPSIG only triggers the captures),
> so the Synapse action for ECAPSIG should always be
> COUNTER_SYNAPSE_ACTION_NONE. You don't need to account for the capture
> polarities because they're not relevant in this particular situation:
> ECAPSIG doesn't trigger the ECAPCNT count function.
>
> William Breathitt Gray

It appears to me that you spoke about TSCNT register content (32 bits). 
So, you were
not talking about the Mod4 counter (2 bits).
Do you confirm that ?

Re: [PATCH v4 3/3] counter: capture-tiecap: capture driver support for ECAP

[William Breathitt Gray]

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

On Tue, Aug 16, 2022 at 10:11:35AM +0200, Julien Panis wrote:
> On 15/08/2022 13:20, William Breathitt Gray wrote:
> > On Sun, Aug 14, 2022 at 01:03:48PM -0400, William Breathitt Gray wrote:
> > > On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
> > > > +static int ecap_cnt_function_read(struct counter_device *counter,
> > > > +				  struct counter_count *count,
> > > > +				  enum counter_function *function)
> > > > +{
> > > > +	*function = COUNTER_FUNCTION_INCREASE;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int ecap_cnt_action_read(struct counter_device *counter,
> > > > +				struct counter_count *count,
> > > > +				struct counter_synapse *synapse,
> > > > +				enum counter_synapse_action *action)
> > > > +{
> > > > +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > > > +
> > > > +	return 0;
> > > > +}
> > > Right now you have a Signal defined for the ECAPSIG line, but there is
> > > at least one more relevant Signal to define: the clock updating ECAPCNT.
> > > The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
> > > clock Signal, but for the ECAPSIG Signal you will need to report a
> > > Synapse action based on the polarity of the next capture (i.e. whether
> > > high or low).
> > I need to make a correction here. IIUC, the ECAPSIG signal doesn't
> > affect the count value of ECAPCNT (ECAPSIG only triggers the captures),
> > so the Synapse action for ECAPSIG should always be
> > COUNTER_SYNAPSE_ACTION_NONE. You don't need to account for the capture
> > polarities because they're not relevant in this particular situation:
> > ECAPSIG doesn't trigger the ECAPCNT count function.
> > 
> > William Breathitt Gray
> 
> It appears to me that you spoke about TSCNT register content (32 bits). So,
> you were
> not talking about the Mod4 counter (2 bits).
> Do you confirm that ?

Yes, I meant the TSCNT register. Sorry about that!

The Counter subsystem is providing an abstraction for users, so although
the ECAP pin signal is serving physically on the device as a clock for
the Mod4 counter, it also serves the TSCNT counter in an abstract sense
as the trigger signal to capture the TSCNT value. The Counter subsystem
paradigm allows you to define these abstract relationships between
Counts and Signals as Synapses.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 3/3] counter: capture-tiecap: capture driver support for ECAP

[Julien Panis]

On 14/08/2022 19:03, William Breathitt Gray wrote:
> On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
>> ECAP hardware on AM62x SoC supports capture feature. It can be used
>> to timestamp events (falling/rising edges) detected on signal input pin.
>>
>> This commit adds capture driver support for ECAP hardware on AM62x SoC.
>>
>> In the ECAP hardware, capture pin can also be configured to be in
>> PWM mode. Current implementation only supports capture operating mode.
>> Hardware also supports timebase sync between multiple instances, but
>> this driver supports simple independent capture functionality.
>>
>> Signed-off-by: Julien Panis <jpanis@baylibre.com>
> Hi Julien,
>
> You picked up the Counter paradigm pretty quickly, good job! Some
> suggestions and comments inline below.

Hi William,
Thank you for your review.
My answers to your questions below.

>
>> ---
>>   drivers/counter/Kconfig          |  14 +
>>   drivers/counter/Makefile         |   1 +
>>   drivers/counter/capture-tiecap.c | 634 +++++++++++++++++++++++++++++++
>>   include/uapi/linux/counter.h     |   2 +
>>   4 files changed, 651 insertions(+)
>>   create mode 100644 drivers/counter/capture-tiecap.c
>>
>> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
>> index 5edd155f1911..580640807a94 100644
>> --- a/drivers/counter/Kconfig
>> +++ b/drivers/counter/Kconfig
>> @@ -101,4 +101,18 @@ config INTEL_QEP
>>   	  To compile this driver as a module, choose M here: the module
>>   	  will be called intel-qep.
>>   
>> +config CAPTURE_TIECAP
>> +	tristate "TI eCAP capture driver"
>> +	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>> +	depends on HAS_IOMEM
>> +	help
>> +	  Select this option to enable the Texas Instruments Enhanced Capture
>> +	  (eCAP) driver in input mode.
>> +
>> +	  It can be used to timestamp events (falling/rising edges) detected
>> +	  on signal input pin.
>> +
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called capture-tiecap.
>> +
> The naming convention I typically see is to lead with the company name
> followed by device model; e.g. ti-ecap-capture. That should result in
> better grouping with the existing drivers in the drivers/counter
> directory. It's a minor benefit, so let's use that convention unless
> there's a reason why capture-tiecap is better here.

Another driver implementing ECAP PWM functionality was named "pwm-tiecap".
That's why I chose this "capture-tiecap" name. But that's OK for 
ti-ecap-capture,
it's better indeed to use the same naming convention for all counter 
drivers.
I will also add "select REGMAP_MMIO" to Kconfig, similarly to others drivers
(ti-eqep for instance).

>
>>   endif # COUNTER
>> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
>> index 8fde6c100ebc..7fac3e0985b5 100644
>> --- a/drivers/counter/Makefile
>> +++ b/drivers/counter/Makefile
>> @@ -14,3 +14,4 @@ obj-$(CONFIG_TI_EQEP)		+= ti-eqep.o
>>   obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
>>   obj-$(CONFIG_MICROCHIP_TCB_CAPTURE)	+= microchip-tcb-capture.o
>>   obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
>> +obj-$(CONFIG_CAPTURE_TIECAP)	+= capture-tiecap.o
>> diff --git a/drivers/counter/capture-tiecap.c b/drivers/counter/capture-tiecap.c
>> new file mode 100644
>> index 000000000000..0601c65ef203
>> --- /dev/null
>> +++ b/drivers/counter/capture-tiecap.c
>> @@ -0,0 +1,634 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * ECAP Capture driver
>> + *
>> + * Copyright (C) 2022 Julien Panis <jpanis@baylibre.com>
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/counter.h>
>> +#include <linux/err.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +
>> +#define ECAP_DRV_NAME "ecap"
>> +
>> +/* Registers */
>> +#define ECAP_NB_CAP			4
>> +
>> +#define ECAP_TSCNT_REG			0x00
>> +
>> +#define ECAP_CAP_REG(i)		(((i) << 2) + 0x08)
>> +
>> +#define ECAP_ECCTL_REG			0x28
>> +#define ECAP_CAPPOL_BIT(i)		BIT((i) << 1)
>> +#define ECAP_EV_MODE_MASK		GENMASK(7, 0)
>> +#define ECAP_CAPLDEN_BIT		BIT(8)
>> +#define ECAP_CONT_ONESHT_BIT		BIT(16)
>> +#define ECAP_STOPVALUE_MASK		GENMASK(18, 17)
>> +#define ECAP_REARM_RESET_BIT		BIT(19)
>> +#define ECAP_TSCNTSTP_BIT		BIT(20)
>> +#define ECAP_SYNCO_DIS_MASK		GENMASK(23, 22)
>> +#define ECAP_CAP_APWM_BIT		BIT(25)
>> +#define ECAP_ECCTL_EN_MASK		(ECAP_CAPLDEN_BIT | ECAP_TSCNTSTP_BIT)
>> +#define ECAP_ECCTL_CFG_MASK		(ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK	\
>> +					| ECAP_ECCTL_EN_MASK | ECAP_REARM_RESET_BIT	\
>> +					| ECAP_CAP_APWM_BIT | ECAP_CONT_ONESHT_BIT)
>> +
>> +#define ECAP_ECINT_EN_FLG_REG		0x2c
>> +#define ECAP_NB_CEVT			(ECAP_NB_CAP + 1)
>> +#define ECAP_CEVT_EN_MASK		GENMASK(ECAP_NB_CEVT, ECAP_NB_CAP)
>> +#define ECAP_CEVT_FLG_BIT(i)		BIT((i) + 17)
>> +
>> +#define ECAP_ECINT_CLR_FRC_REG	0x30
>> +#define ECAP_INT_CLR_BIT		BIT(0)
>> +#define ECAP_CEVT_CLR_BIT(i)		BIT((i) + 1)
>> +#define ECAP_CEVT_CLR_MASK		GENMASK(ECAP_NB_CEVT, 0)
>> +
>> +#define ECAP_PID_REG			0x5c
>> +
>> +/*
>> + * Event modes
>> + * One bit for each CAPx register : 1 = falling edge / 0 = rising edge
>> + * e.g. mode = 13 = 0xd = 0b1101
>> + * -> falling edge for CAP1-3-4 / rising edge for CAP2
>> + */
>> +#define ECAP_EV_MODE_BIT(i)		BIT(i)
>> +
>> +/* ECAP input */
>> +#define ECAP_SIGNAL 0
>> +
>> +static const struct regmap_config ecap_cnt_regmap_config = {
>> +	.reg_bits = 32,
>> +	.reg_stride = 4,
>> +	.val_bits = 32,
>> +	.max_register = ECAP_PID_REG,
>> +};
>> +
>> +/**
>> + * struct ecap_cnt_dev - device private data structure
>> + * @enabled:      device state
>> + * @clk:          device clock
>> + * @clk_rate:     device clock rate
>> + * @regmap:       device register map
>> + * @elapsed_time: elapsed time since capture start
>> + * @lock:         synchronization lock to prevent race conditions when computing times
>> + * @pm_ctx:       device context for PM operations
>> + */
>> +struct ecap_cnt_dev {
>> +	bool enabled;
>> +	struct clk *clk;
>> +	unsigned long clk_rate;
>> +	struct regmap *regmap;
>> +	__aligned_u64 elapsed_time;
>> +	spinlock_t lock;
>> +	struct {
>> +		u8 ev_mode;
>> +		unsigned int time_cntr;
>> +	} pm_ctx;
>> +};
>> +
>> +static u8 ecap_cnt_capture_get_evmode(struct counter_device *counter)
>> +{
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +	u8 ev_mode = 0;
>> +	unsigned int regval;
>> +	int i;
>> +
>> +	pm_runtime_get_sync(counter->parent);
>> +	regmap_read(ecap_dev->regmap, ECAP_ECCTL_REG, &regval);
>> +	pm_runtime_put_sync(counter->parent);
>> +
>> +	for (i = 0 ; i < ECAP_NB_CAP ; i++) {
>> +		if (regval & ECAP_CAPPOL_BIT(i))
>> +			ev_mode |= ECAP_EV_MODE_BIT(i);
>> +	}
>> +
>> +	return ev_mode;
>> +}
>> +
>> +static void ecap_cnt_capture_set_evmode(struct counter_device *counter, u8 ev_mode)
>> +{
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +	unsigned int regval = 0;
>> +	int i;
>> +
>> +	for (i = 0 ; i < ECAP_NB_CAP ; i++) {
>> +		if (ev_mode & ECAP_EV_MODE_BIT(i))
>> +			regval |= ECAP_CAPPOL_BIT(i);
>> +	}
>> +
>> +	pm_runtime_get_sync(counter->parent);
>> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_EV_MODE_MASK, regval);
>> +	pm_runtime_put_sync(counter->parent);
>> +}
>> +
>> +static void ecap_cnt_capture_enable(struct counter_device *counter, bool rearm)
>> +{
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +	unsigned int regval;
>> +
>> +	pm_runtime_get_sync(counter->parent);
>> +
>> +	/* Enable interrupts on events */
>> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG,
>> +			   ECAP_CEVT_EN_MASK, ECAP_CEVT_EN_MASK);
>> +
>> +	/* Run counter */
>> +	regval = ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK;
>> +	if (rearm) {
>> +		ecap_dev->elapsed_time = 0;
>> +		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, 0);
>> +		regval |= ECAP_REARM_RESET_BIT;
>> +	}
>> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_CFG_MASK, regval);
>> +}
>> +
>> +static void ecap_cnt_capture_disable(struct counter_device *counter)
>> +{
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> +	/* Disable interrupts on events */
>> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, ECAP_CEVT_EN_MASK, 0);
>> +
>> +	/* Stop counter */
>> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_ECCTL_EN_MASK, 0);
>> +
>> +	pm_runtime_put_sync(counter->parent);
>> +}
>> +
>> +static int ecap_cnt_count_get_ns(struct counter_device *counter, unsigned int reg, u64 *val)
>> +{
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +	unsigned int regval;
>> +	unsigned long flags;
>> +
>> +	pm_runtime_get_sync(counter->parent);
>> +	spin_lock_irqsave(&ecap_dev->lock, flags);
>> +
>> +	/* time_ns = 10^9 * (time_cycles + time_cumul) / clk_rate */
>> +	regmap_read(ecap_dev->regmap, reg, &regval);
>> +	*val = (regval + ecap_dev->elapsed_time) * NSEC_PER_SEC;
>> +	do_div(*val, ecap_dev->clk_rate);
>> +
>> +	spin_unlock_irqrestore(&ecap_dev->lock, flags);
>> +	pm_runtime_put_sync(counter->parent);
>> +
>> +	return 0;
>> +}
>> +
>> +static inline int ecap_cnt_count_read(struct counter_device *counter,
>> +				      struct counter_count *count,
>> +				      u64 *val)
>> +{
>> +	return ecap_cnt_count_get_ns(counter, ECAP_TSCNT_REG, val);
>> +}
> This conversion to nanoseconds is an interpretation of the count value
> that belongs in userspace. Instead of converting to nanoseconds here,
> simply expose the count value directly and add extensions to expose the
> elapsed time (a Count extension) and clock rate (a Signal extension) as
> well; with these three values you can derive nanoseconds in userspace
> using the time_ns equation in your ecap_cnt_count_get_ns() comment.
>
>> +static int ecap_cnt_function_read(struct counter_device *counter,
>> +				  struct counter_count *count,
>> +				  enum counter_function *function)
>> +{
>> +	*function = COUNTER_FUNCTION_INCREASE;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ecap_cnt_action_read(struct counter_device *counter,
>> +				struct counter_count *count,
>> +				struct counter_synapse *synapse,
>> +				enum counter_synapse_action *action)
>> +{
>> +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
>> +
>> +	return 0;
>> +}
> Right now you have a Signal defined for the ECAPSIG line, but there is
> at least one more relevant Signal to define: the clock updating ECAPCNT.
> The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
> clock Signal, but for the ECAPSIG Signal you will need to report a
> Synapse action based on the polarity of the next capture (i.e. whether
> high or low).

Just to be sure : by using the word ECAPCNT, I guess that you speak 
about the
Mod4 counter (0->1->2->3->0...), don't you ? (2 bits)
Or do you speak about ECAP_TSCNT_REG register content ? (32 bits)

>
>> +
>> +static int ecap_cnt_watch_validate(struct counter_device *counter,
>> +				   const struct counter_watch *watch)
>> +{
>> +	struct counter_event_node *event_node;
>> +
>> +	if (watch->channel || watch->event != COUNTER_EVENT_CAPTURE)
>> +		return -EINVAL;
> The intention of this conditional is more obvious if you explicitly
> check for watch->channel != 0.
>
>> +
>> +	list_for_each_entry(event_node, &counter->next_events_list, l)
>> +		if (watch->component.type != COUNTER_COMPONENT_EXTENSION ||
>> +		    watch->component.scope != COUNTER_SCOPE_COUNT ||
>> +		    watch->component.parent ||
>> +		    watch->component.id >= ECAP_NB_CAP)
>> +			return -EINVAL;
> You don't need this list_for_each_entry loop at all. The purpose of the
> watch_validate() callback is to make sure a watch isn't added with a
> channel or event configuration that conflicts with existing watches in
> the next_events_list list.
>
> For example, the 104-QUAD-8 device only allows one particular event to
> occur on any given channel. The quad8_watch_validate() function makes
> sure that all watches for a particular channel are set for the same
> event; if a watch for that channel is set for a different event, then
> that watch is invalid because the 104-QUAD-8 device cannot handle such
> a request for two different events on the same channel.
>
> Regarding the TI ECAP device, it looks like it only supports that one
> COUNTER_EVENT_CAPTURE event so you don't need to check the other watches
> in the next_events_list list. Your conditional check above for
> watch->channel and watch->event should be enough by itself.
>
>> +
>> +	return 0;
>> +}
>> +
>> +static int ecap_cnt_enable_read(struct counter_device *counter,
>> +				struct counter_count *count,
>> +				u8 *enable)
>> +{
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> +	*enable = ecap_dev->enabled;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ecap_cnt_enable_write(struct counter_device *counter,
>> +				 struct counter_count *count,
>> +				 u8 enable)
>> +{
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> +	if (enable > 1)
>> +		return -EINVAL;
>> +	if (enable == ecap_dev->enabled)
>> +		return 0;
>> +	if (enable)
>> +		ecap_cnt_capture_enable(counter, true);
>> +	else
>> +		ecap_cnt_capture_disable(counter);
>> +	ecap_dev->enabled = enable;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ecap_cnt_cap_get_pol(struct counter_device *counter,
>> +				struct counter_signal *signal,
>> +				u32 *pol, u8 inst)
>> +{
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> +	pm_runtime_get_sync(counter->parent);
>> +	*pol = regmap_test_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
>> +	pm_runtime_put_sync(counter->parent);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ecap_cnt_cap_set_pol(struct counter_device *counter,
>> +				struct counter_signal *signal,
>> +				u32 pol, u8 inst)
>> +{
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter);
>> +
>> +	if (ecap_dev->enabled)
>> +		return -EBUSY;
>> +	if (pol > 1)
>> +		return -EINVAL;
>> +
>> +	pm_runtime_get_sync(counter->parent);
>> +	if (pol)
>> +		regmap_set_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
>> +	else
>> +		regmap_clear_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_CAPPOL_BIT(inst));
>> +	pm_runtime_put_sync(counter->parent);
>> +
>> +	return 0;
>> +}
>> +
>> +static inline int ecap_cnt_cap_read(struct counter_device *counter,
>> +				    struct counter_count *count,
>> +				    u64 *cap, u8 inst)
>> +{
>> +	return ecap_cnt_count_get_ns(counter, ECAP_CAP_REG(inst), cap);
>> +}
> Return the captured count value directly here; userspace can handle the
> nanosecond interpretation for the same reason explained earlier for the
> ECAPCNT count value.
>
>> +static inline int ecap_cnt_cap1_get_pol(struct counter_device *counter,
>> +					struct counter_signal *signal, u32 *pol)
>> +{
>> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 0);
>> +}
>> +
>> +static inline int ecap_cnt_cap2_get_pol(struct counter_device *counter,
>> +					struct counter_signal *signal, u32 *pol)
>> +{
>> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 1);
>> +}
>> +
>> +static inline int ecap_cnt_cap3_get_pol(struct counter_device *counter,
>> +					struct counter_signal *signal, u32 *pol)
>> +{
>> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 2);
>> +}
>> +
>> +static inline int ecap_cnt_cap4_get_pol(struct counter_device *counter,
>> +					struct counter_signal *signal, u32 *pol)
>> +{
>> +	return ecap_cnt_cap_get_pol(counter, signal, pol, 3);
>> +}
>> +
>> +static inline int ecap_cnt_cap1_set_pol(struct counter_device *counter,
>> +					struct counter_signal *signal, u32 pol)
>> +{
>> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 0);
>> +}
>> +
>> +static inline int ecap_cnt_cap2_set_pol(struct counter_device *counter,
>> +					struct counter_signal *signal, u32 pol)
>> +{
>> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 1);
>> +}
>> +
>> +static inline int ecap_cnt_cap3_set_pol(struct counter_device *counter,
>> +					struct counter_signal *signal, u32 pol)
>> +{
>> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 2);
>> +}
>> +
>> +static inline int ecap_cnt_cap4_set_pol(struct counter_device *counter,
>> +					struct counter_signal *signal, u32 pol)
>> +{
>> +	return ecap_cnt_cap_set_pol(counter, signal, pol, 3);
>> +}
>> +
>> +static inline int ecap_cnt_cap1_read(struct counter_device *counter,
>> +				     struct counter_count *count, u64 *cap)
>> +{
>> +	return ecap_cnt_cap_read(counter, count, cap, 0);
>> +}
>> +
>> +static inline int ecap_cnt_cap2_read(struct counter_device *counter,
>> +				     struct counter_count *count, u64 *cap)
>> +{
>> +	return ecap_cnt_cap_read(counter, count, cap, 1);
>> +}
>> +
>> +static inline int ecap_cnt_cap3_read(struct counter_device *counter,
>> +				     struct counter_count *count, u64 *cap)
>> +{
>> +	return ecap_cnt_cap_read(counter, count, cap, 2);
>> +}
>> +
>> +static inline int ecap_cnt_cap4_read(struct counter_device *counter,
>> +				     struct counter_count *count, u64 *cap)
>> +{
>> +	return ecap_cnt_cap_read(counter, count, cap, 3);
>> +}
> There's a lot of boilerplate code here for each ECAP register read/write
> callback. Try defining a macro to build these so you can define each
> read/write callback pair more succinctly.

I tried to use a macro but did not find any clean way to do that. I will 
try again, maybe
some Zephyr code could be inspiring for that kind of static definition 
using macros.

>
>> +static const struct counter_ops ecap_cnt_ops = {
>> +	.count_read = ecap_cnt_count_read,
>> +	.function_read = ecap_cnt_function_read,
>> +	.action_read = ecap_cnt_action_read,
>> +	.watch_validate = ecap_cnt_watch_validate,
>> +};
>> +
>> +static const enum counter_function ecap_cnt_functions[] = {
>> +	COUNTER_FUNCTION_INCREASE,
>> +};
>> +
>> +static const enum counter_synapse_action ecap_cnt_actions[] = {
>> +	COUNTER_SYNAPSE_ACTION_BOTH_EDGES,
>> +};
>> +
>> +static const char *const ecap_cnt_cap_polarities[] = {
>> +	"rising",
>> +	"falling",
>> +};
>> +
>> +static DEFINE_COUNTER_ENUM(ecap_cnt_cap_avail_pol, ecap_cnt_cap_polarities);
>> +
>> +static struct counter_comp ecap_cnt_signal_ext[] = {
>> +	COUNTER_COMP_SIGNAL_ENUM("polarity1", ecap_cnt_cap1_get_pol,
>> +				 ecap_cnt_cap1_set_pol, ecap_cnt_cap_avail_pol),
>> +	COUNTER_COMP_SIGNAL_ENUM("polarity2", ecap_cnt_cap2_get_pol,
>> +				 ecap_cnt_cap2_set_pol, ecap_cnt_cap_avail_pol),
>> +	COUNTER_COMP_SIGNAL_ENUM("polarity3", ecap_cnt_cap3_get_pol,
>> +				 ecap_cnt_cap3_set_pol, ecap_cnt_cap_avail_pol),
>> +	COUNTER_COMP_SIGNAL_ENUM("polarity4", ecap_cnt_cap4_get_pol,
>> +				 ecap_cnt_cap4_set_pol, ecap_cnt_cap_avail_pol),
>> +};
>> +
>> +static struct counter_signal ecap_cnt_signals[] = {
>> +	{
>> +		.id = ECAP_SIGNAL,
>> +		.name = "ECAPSIG",
>> +		.ext = ecap_cnt_signal_ext,
>> +		.num_ext = ARRAY_SIZE(ecap_cnt_signal_ext),
>> +	},
>> +};
> As mentioned earlier, define another Signal here representing the clock
> feeding ECAPCNT. You should also define another extensions array for
> this new Signal that will provide the clock rate for userspace to read.
>
>> +static struct counter_synapse ecap_cnt_synapses[] = {
>> +	{
>> +		.actions_list = ecap_cnt_actions,
>> +		.num_actions = ARRAY_SIZE(ecap_cnt_actions),
>> +		.signal = &ecap_cnt_signals[ECAP_SIGNAL],
>> +	},
>> +};
> You will need to define another Synapse here for the clock Signal
> mentioned above; similarly, you need to define another actions array for
> the possible edge triggers.
>
>> +static struct counter_comp ecap_cnt_count_ext[] = {
>> +	COUNTER_COMP_COUNT_U64("capture1", ecap_cnt_cap1_read, NULL),
>> +	COUNTER_COMP_COUNT_U64("capture2", ecap_cnt_cap2_read, NULL),
>> +	COUNTER_COMP_COUNT_U64("capture3", ecap_cnt_cap3_read, NULL),
>> +	COUNTER_COMP_COUNT_U64("capture4", ecap_cnt_cap4_read, NULL),
>> +	COUNTER_COMP_ENABLE(ecap_cnt_enable_read, ecap_cnt_enable_write),
> I just want to verify: this enable extension should disable the ECAPCNT
> count itself (i.e. no more increasing count value). Is that what's
> happening here, or is this meant to disable just the captures?

Yes, it is what's happening here : no more increasing count value.

>
>> +};
>> +
>> +static struct counter_count ecap_cnt_counts[] = {
>> +	{
>> +		.id = 0,
>> +		.name = "ECAPCNT",
>> +		.functions_list = ecap_cnt_functions,
>> +		.num_functions = ARRAY_SIZE(ecap_cnt_functions),
>> +		.synapses = ecap_cnt_synapses,
>> +		.num_synapses = ARRAY_SIZE(ecap_cnt_synapses),
>> +		.ext = ecap_cnt_count_ext,
>> +		.num_ext = ARRAY_SIZE(ecap_cnt_count_ext),
>> +	},
>> +};
>> +
>> +static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
>> +{
>> +	struct counter_device *counter_dev = dev_id;
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
>> +	unsigned int clr = 0;
>> +	unsigned int flg;
>> +	int i;
>> +	unsigned long flags;
>> +
>> +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
>> +
>> +	for (i = ECAP_NB_CAP - 1 ; i < ECAP_NB_CEVT ; i++) {
> Would you walk me through the logic for this loop. Is this for-loop
> intended to loop through all four capture indices? ECAP_NB_CAP and
> ECAP_NB_CEVT are defines, so your for-loop has i=3 and i<5; is this what
> you want?

In previous versions (IIO subsys), this for-loop was intended to loop 
through all 4 capture indices
and overflow flag.
In this version, it has been modified to loop only for the last capture 
indice (the 4th)
and overflow flag : yes, this is intentional. Only 1 event has to be 
pushed so that the user
gets all 4 captured timestamps in a single-reading (using 4 watches).
But if I understand well your previous suggestion, you would like 
tracking Mod4 counter value,
don't you ? So, I will change back this for-loop, so that it loops for 
all capture indices (and
overflow flag) -> For all 4 capture indices, Mod4 count will be updated. 
And event will still be
pushed only for the 4th capture indice.

>
>> +		if (flg & ECAP_CEVT_FLG_BIT(i)) {
>> +			if (i < ECAP_NB_CAP) {
>> +				/* Input signal edge detected on last CAP (CAP4) */
>> +				counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, 0);
>> +			} else {
>> +				/* Counter overflow */
>> +				spin_lock_irqsave(&ecap_dev->lock, flags);
>> +				ecap_dev->elapsed_time += (u64)U32_MAX + 1;
>> +				spin_unlock_irqrestore(&ecap_dev->lock, flags);
> Should we push COUNTER_EVENT_OVERFLOW here? Since elapsed_time always
> increments by the same constant (U32_MAX), it would be better to replace
> this with an atomic_t "num_overflows" to track the number of overflows
> with atomic_inc() and compute the particular elapsed_time later.

I agree with this suggestion.

>
> William Breathitt Gray
>
>> +			}
>> +
>> +			clr |= ECAP_CEVT_CLR_BIT(i);
>> +		}
>> +	}
>> +
>> +	clr |= ECAP_INT_CLR_BIT;
>> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECINT_CLR_FRC_REG, ECAP_CEVT_CLR_MASK, clr);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void ecap_cnt_clk_disable(void *clk)
>> +{
>> +	clk_disable_unprepare(clk);
>> +}
>> +
>> +static int ecap_cnt_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct ecap_cnt_dev *ecap_dev;
>> +	struct counter_device *counter_dev;
>> +	void __iomem *mmio_base;
>> +	int ret;
>> +
>> +	counter_dev = devm_counter_alloc(dev, sizeof(*ecap_dev));
>> +	if (IS_ERR(counter_dev))
>> +		return PTR_ERR(counter_dev);
>> +
>> +	counter_dev->name = ECAP_DRV_NAME;
>> +	counter_dev->parent = dev;
>> +	counter_dev->ops = &ecap_cnt_ops;
>> +	counter_dev->signals = ecap_cnt_signals;
>> +	counter_dev->num_signals = ARRAY_SIZE(ecap_cnt_signals);
>> +	counter_dev->counts = ecap_cnt_counts;
>> +	counter_dev->num_counts = ARRAY_SIZE(ecap_cnt_counts);
>> +
>> +	ecap_dev = counter_priv(counter_dev);
>> +
>> +	ecap_dev->clk = devm_clk_get(dev, "fck");
>> +	if (IS_ERR(ecap_dev->clk))
>> +		return dev_err_probe(dev, PTR_ERR(ecap_dev->clk), "failed to get clock\n");
>> +
>> +	ret = clk_prepare_enable(ecap_dev->clk);
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable clock\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_add_action_or_reset(dev, ecap_cnt_clk_disable, ecap_dev->clk);
>> +	if (ret) {
>> +		dev_err(dev, "failed to add clock disable action\n");
>> +		return ret;
>> +	}
>> +
>> +	ecap_dev->clk_rate = clk_get_rate(ecap_dev->clk);
>> +	if (!ecap_dev->clk_rate) {
>> +		dev_err(dev, "failed to get clock rate\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	mmio_base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(mmio_base))
>> +		return PTR_ERR(mmio_base);
>> +
>> +	ecap_dev->regmap = devm_regmap_init_mmio(dev, mmio_base, &ecap_cnt_regmap_config);
>> +	if (IS_ERR(ecap_dev->regmap))
>> +		return dev_err_probe(dev, PTR_ERR(ecap_dev->regmap), "failed to init regmap\n");
>> +
>> +	spin_lock_init(&ecap_dev->lock);
>> +
>> +	ret = platform_get_irq(pdev, 0);
>> +	if (ret < 0) {
>> +		dev_err(dev, "failed to get irq\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_request_irq(dev, ret, ecap_cnt_isr, 0, pdev->name, counter_dev);
>> +	if (ret) {
>> +		dev_err(dev, "failed to request irq\n");
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, counter_dev);
>> +	pm_runtime_enable(dev);
>> +
>> +	ecap_dev->enabled = 0;
>> +	ecap_cnt_capture_set_evmode(counter_dev, 0);
>> +
>> +	ret = devm_counter_add(dev, counter_dev);
>> +	if (ret) {
>> +		dev_err(dev, "failed to add counter\n");
>> +		pm_runtime_disable(dev);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int ecap_cnt_remove(struct platform_device *pdev)
>> +{
>> +	struct counter_device *counter_dev = platform_get_drvdata(pdev);
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
>> +
>> +	if (ecap_dev->enabled)
>> +		ecap_cnt_capture_disable(counter_dev);
>> +
>> +	pm_runtime_disable(&pdev->dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ecap_cnt_suspend(struct device *dev)
>> +{
>> +	struct counter_device *counter_dev = dev_get_drvdata(dev);
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
>> +
>> +	/* If eCAP is running, stop capture then save timestamp counter */
>> +	if (ecap_dev->enabled) {
>> +		ecap_cnt_capture_disable(counter_dev);
>> +
>> +		pm_runtime_get_sync(dev);
>> +		regmap_read(ecap_dev->regmap, ECAP_TSCNT_REG, &ecap_dev->pm_ctx.time_cntr);
>> +		pm_runtime_put_sync(dev);
>> +	}
>> +
>> +	ecap_dev->pm_ctx.ev_mode = ecap_cnt_capture_get_evmode(counter_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ecap_cnt_resume(struct device *dev)
>> +{
>> +	struct counter_device *counter_dev = dev_get_drvdata(dev);
>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
>> +
>> +	ecap_cnt_capture_set_evmode(counter_dev, ecap_dev->pm_ctx.ev_mode);
>> +
>> +	/* If eCAP was running, restore timestamp counter then run capture */
>> +	if (ecap_dev->enabled) {
>> +		pm_runtime_get_sync(dev);
>> +		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, ecap_dev->pm_ctx.time_cntr);
>> +		pm_runtime_put_sync(dev);
>> +
>> +		ecap_cnt_capture_enable(counter_dev, false);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static DEFINE_SIMPLE_DEV_PM_OPS(ecap_cnt_pm_ops, ecap_cnt_suspend, ecap_cnt_resume);
>> +
>> +static const struct of_device_id ecap_cnt_of_match[] = {
>> +	{ .compatible	= "ti,am62-ecap-capture" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ecap_cnt_of_match);
>> +
>> +static struct platform_driver ecap_cnt_driver = {
>> +	.probe = ecap_cnt_probe,
>> +	.remove = ecap_cnt_remove,
>> +	.driver = {
>> +		.name = "ecap-capture",
>> +		.of_match_table = ecap_cnt_of_match,
>> +		.pm = pm_sleep_ptr(&ecap_cnt_pm_ops),
>> +	},
>> +};
>> +module_platform_driver(ecap_cnt_driver);
>> +
>> +MODULE_DESCRIPTION("ECAP Capture driver");
>> +MODULE_AUTHOR("Julien Panis <jpanis@baylibre.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
>> index 96c5ffd368ad..4c5372c6f2a3 100644
>> --- a/include/uapi/linux/counter.h
>> +++ b/include/uapi/linux/counter.h
>> @@ -63,6 +63,8 @@ enum counter_event_type {
>>   	COUNTER_EVENT_INDEX,
>>   	/* State of counter is changed */
>>   	COUNTER_EVENT_CHANGE_OF_STATE,
>> +	/* Count value is captured */
>> +	COUNTER_EVENT_CAPTURE,
>>   };
>>   
>>   /**
>> -- 
>> 2.25.1
>>

Re: [PATCH v4 3/3] counter: capture-tiecap: capture driver support for ECAP

[William Breathitt Gray]

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

On Tue, Aug 16, 2022 at 09:58:10AM +0200, Julien Panis wrote:
> On 14/08/2022 19:03, William Breathitt Gray wrote:
> > On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
> > > +static int ecap_cnt_function_read(struct counter_device *counter,
> > > +				  struct counter_count *count,
> > > +				  enum counter_function *function)
> > > +{
> > > +	*function = COUNTER_FUNCTION_INCREASE;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ecap_cnt_action_read(struct counter_device *counter,
> > > +				struct counter_count *count,
> > > +				struct counter_synapse *synapse,
> > > +				enum counter_synapse_action *action)
> > > +{
> > > +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
> > > +
> > > +	return 0;
> > > +}
> > Right now you have a Signal defined for the ECAPSIG line, but there is
> > at least one more relevant Signal to define: the clock updating ECAPCNT.
> > The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
> > clock Signal, but for the ECAPSIG Signal you will need to report a
> > Synapse action based on the polarity of the next capture (i.e. whether
> > high or low).
> 
> Just to be sure : by using the word ECAPCNT, I guess that you speak about
> the
> Mod4 counter (0->1->2->3->0...), don't you ? (2 bits)
> Or do you speak about ECAP_TSCNT_REG register content ? (32 bits)

Sorry for the confusion, I'm talking about ECAP_TSCNT_REG (32-bit) here.
You should rename this Count in your ecap_cnt_counts array from
"ECAPCNT" to "Time-Stamp Counter" to make it clearer to users as well;
it would be prudent to rename "ECAPSIG" too.

I didn't know that there was a register exposing the Mod4 counter value.
If that's true, then define a Count for the Mod4 counter in your
ecap_cnt_counts array.

> > > +static struct counter_comp ecap_cnt_count_ext[] = {
> > > +	COUNTER_COMP_COUNT_U64("capture1", ecap_cnt_cap1_read, NULL),
> > > +	COUNTER_COMP_COUNT_U64("capture2", ecap_cnt_cap2_read, NULL),
> > > +	COUNTER_COMP_COUNT_U64("capture3", ecap_cnt_cap3_read, NULL),
> > > +	COUNTER_COMP_COUNT_U64("capture4", ecap_cnt_cap4_read, NULL),
> > > +	COUNTER_COMP_ENABLE(ecap_cnt_enable_read, ecap_cnt_enable_write),
> > I just want to verify: this enable extension should disable the ECAPCNT
> > count itself (i.e. no more increasing count value). Is that what's
> > happening here, or is this meant to disable just the captures?
> 
> Yes, it is what's happening here : no more increasing count value.

Okay that's good. By the way, COUNTER_COMP_ENABLE ensures the enable
value passed to ecap_cnt_enable_write() is either 0 or 1, so you don't
need the enable > 1 check in your callback.

> > > +static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
> > > +{
> > > +	struct counter_device *counter_dev = dev_id;
> > > +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
> > > +	unsigned int clr = 0;
> > > +	unsigned int flg;
> > > +	int i;
> > > +	unsigned long flags;
> > > +
> > > +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
> > > +
> > > +	for (i = ECAP_NB_CAP - 1 ; i < ECAP_NB_CEVT ; i++) {
> > Would you walk me through the logic for this loop. Is this for-loop
> > intended to loop through all four capture indices? ECAP_NB_CAP and
> > ECAP_NB_CEVT are defines, so your for-loop has i=3 and i<5; is this what
> > you want?
> 
> In previous versions (IIO subsys), this for-loop was intended to loop
> through all 4 capture indices
> and overflow flag.
> In this version, it has been modified to loop only for the last capture
> indice (the 4th)
> and overflow flag : yes, this is intentional. Only 1 event has to be pushed
> so that the user
> gets all 4 captured timestamps in a single-reading (using 4 watches).
> But if I understand well your previous suggestion, you would like tracking
> Mod4 counter value,
> don't you ? So, I will change back this for-loop, so that it loops for all
> capture indices (and
> overflow flag) -> For all 4 capture indices, Mod4 count will be updated. And
> event will still be
> pushed only for the 4th capture indice.

Instead of limiting the event push to just the 4th capture, I'd push
COUNTER_EVENT_CAPTURE on every capture but delegate them to their own
channels::

    counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, i);

The captures operate as a circular buffer, so the user can determine the
current capture index based on the watch channel reported and perform a
modulo to read the buffers in right sequence. Alternatively, they can
watch just channel 3 if they want to process only four captures at a
time.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 3/3] counter: capture-tiecap: capture driver support for ECAP

[Julien Panis]

On 16/08/2022 17:12, William Breathitt Gray wrote:
> On Tue, Aug 16, 2022 at 09:58:10AM +0200, Julien Panis wrote:
>> On 14/08/2022 19:03, William Breathitt Gray wrote:
>>> On Wed, Aug 10, 2022 at 04:07:24PM +0200, Julien Panis wrote:
>>>> +static int ecap_cnt_function_read(struct counter_device *counter,
>>>> +				  struct counter_count *count,
>>>> +				  enum counter_function *function)
>>>> +{
>>>> +	*function = COUNTER_FUNCTION_INCREASE;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int ecap_cnt_action_read(struct counter_device *counter,
>>>> +				struct counter_count *count,
>>>> +				struct counter_synapse *synapse,
>>>> +				enum counter_synapse_action *action)
>>>> +{
>>>> +	*action = COUNTER_SYNAPSE_ACTION_BOTH_EDGES;
>>>> +
>>>> +	return 0;
>>>> +}
>>> Right now you have a Signal defined for the ECAPSIG line, but there is
>>> at least one more relevant Signal to define: the clock updating ECAPCNT.
>>> The Synapse action of COUNTER_SYNAPSE_ACTION_BOTH_EDGES is for that
>>> clock Signal, but for the ECAPSIG Signal you will need to report a
>>> Synapse action based on the polarity of the next capture (i.e. whether
>>> high or low).
>> Just to be sure : by using the word ECAPCNT, I guess that you speak about
>> the
>> Mod4 counter (0->1->2->3->0...), don't you ? (2 bits)
>> Or do you speak about ECAP_TSCNT_REG register content ? (32 bits)
> Sorry for the confusion, I'm talking about ECAP_TSCNT_REG (32-bit) here.
> You should rename this Count in your ecap_cnt_counts array from
> "ECAPCNT" to "Time-Stamp Counter" to make it clearer to users as well;
> it would be prudent to rename "ECAPSIG" too.
>
> I didn't know that there was a register exposing the Mod4 counter value.
> If that's true, then define a Count for the Mod4 counter in your
> ecap_cnt_counts array.

There is no dedicated register for that, but it would be possible to 
expose this value
(when interruptions are triggered, we would just need to parse flags 
1/2/3/4 to determine
mod4 counter current state). That would not be very useful, though.

>
>>>> +static struct counter_comp ecap_cnt_count_ext[] = {
>>>> +	COUNTER_COMP_COUNT_U64("capture1", ecap_cnt_cap1_read, NULL),
>>>> +	COUNTER_COMP_COUNT_U64("capture2", ecap_cnt_cap2_read, NULL),
>>>> +	COUNTER_COMP_COUNT_U64("capture3", ecap_cnt_cap3_read, NULL),
>>>> +	COUNTER_COMP_COUNT_U64("capture4", ecap_cnt_cap4_read, NULL),
>>>> +	COUNTER_COMP_ENABLE(ecap_cnt_enable_read, ecap_cnt_enable_write),
>>> I just want to verify: this enable extension should disable the ECAPCNT
>>> count itself (i.e. no more increasing count value). Is that what's
>>> happening here, or is this meant to disable just the captures?
>> Yes, it is what's happening here : no more increasing count value.
> Okay that's good. By the way, COUNTER_COMP_ENABLE ensures the enable
> value passed to ecap_cnt_enable_write() is either 0 or 1, so you don't
> need the enable > 1 check in your callback.
>
>>>> +static irqreturn_t ecap_cnt_isr(int irq, void *dev_id)
>>>> +{
>>>> +	struct counter_device *counter_dev = dev_id;
>>>> +	struct ecap_cnt_dev *ecap_dev = counter_priv(counter_dev);
>>>> +	unsigned int clr = 0;
>>>> +	unsigned int flg;
>>>> +	int i;
>>>> +	unsigned long flags;
>>>> +
>>>> +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
>>>> +
>>>> +	for (i = ECAP_NB_CAP - 1 ; i < ECAP_NB_CEVT ; i++) {
>>> Would you walk me through the logic for this loop. Is this for-loop
>>> intended to loop through all four capture indices? ECAP_NB_CAP and
>>> ECAP_NB_CEVT are defines, so your for-loop has i=3 and i<5; is this what
>>> you want?
>> In previous versions (IIO subsys), this for-loop was intended to loop
>> through all 4 capture indices
>> and overflow flag.
>> In this version, it has been modified to loop only for the last capture
>> indice (the 4th)
>> and overflow flag : yes, this is intentional. Only 1 event has to be pushed
>> so that the user
>> gets all 4 captured timestamps in a single-reading (using 4 watches).
>> But if I understand well your previous suggestion, you would like tracking
>> Mod4 counter value,
>> don't you ? So, I will change back this for-loop, so that it loops for all
>> capture indices (and
>> overflow flag) -> For all 4 capture indices, Mod4 count will be updated. And
>> event will still be
>> pushed only for the 4th capture indice.
> Instead of limiting the event push to just the 4th capture, I'd push
> COUNTER_EVENT_CAPTURE on every capture but delegate them to their own
> channels::
>
>      counter_push_event(counter_dev, COUNTER_EVENT_CAPTURE, i);

I prefer using only 1 signal if you don't mind. I think it's less 
confusing for the user.

>
> The captures operate as a circular buffer, so the user can determine the
> current capture index based on the watch channel reported and perform a
> modulo to read the buffers in right sequence. Alternatively, they can
> watch just channel 3 if they want to process only four captures at a
> time.
>
> William Breathitt Gray

Re: [PATCH v4 0/3] ECAP support on TI AM62x SoC

[William Breathitt Gray]

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

On Wed, Aug 10, 2022 at 04:07:21PM +0200, Julien Panis wrote:
> The Enhanced Capture (ECAP) module can be used to timestamp events
> detected on signal input pin. It can be used for time measurements
> of pulse train signals.
> 
> ECAP module includes 4 timestamp capture registers. For all 4 sequenced
> timestamp capture events (1->2->3->4->1->...), edge polarity (falling/rising
> edge) can be selected.
> 
> This driver leverages counter subsystem to :
> - select edge polarity for all 4 capture events (event mode)
> - log timestamps for each capture event
> Event polarity, and CAP1/2/3/4 timestamps give all the information
> about the input pulse train. Further information can easily be computed :
> period and/or duty cycle if frequency is constant, elapsed time between
> pulses, etc...
> 
> Modifications since v3:
> 	- Migrate driver from IIO to Counter subsystem
> 	- Minor modification in yaml ($id) to match Counter subsystem
> 	- Add ABI documentation
> 
> Userspace commands :
> 	### SIGNAL ###
> 	cd /sys/bus/counter/devices/counter0/signal0
> 
> 	# Get available polarities for each capture event
> 	cat polarity1_available
> 	cat polarity2_available
> 	cat polarity3_available
> 	cat polarity4_available
> 
> 	# Get polarity for each capture event
> 	cat polarity1
> 	cat polarity2
> 	cat polarity3
> 	cat polarity4
> 
> 	# Set polarity for each capture event
> 	echo rising > polarity1
> 	echo falling > polarity2
> 	echo rising > polarity3
> 	echo falling > polarity4
> 
> 	### COUNT ###
> 	cd /sys/bus/counter/devices/counter0/count0
> 
> 	# Run ECAP
> 	echo 1 > enable
> 
> 	# Get current timebase counter value
> 	cat count
> 
> 	# Get captured timestamps
> 	cat capture1
> 	cat capture2
> 	cat capture3
> 	cat capture4
> 
> 	# Note that counter watches can also be used to get
> 	# data from userspace application
> 	# -> see tools/counter/counter_example.c
> 
> 	# Stop ECAP
> 	echo 0 > enable
> 
> Julien Panis (3):
>   dt-binding: counter: add ti,am62-ecap-capture.yaml
>   Documentation: ABI: add sysfs-bus-counter-ecap
>   counter: capture-tiecap: capture driver support for ECAP
> 
>  .../ABI/testing/sysfs-bus-counter-ecap        |  64 ++
>  .../counter/ti,am62-ecap-capture.yaml         |  61 ++
>  drivers/counter/Kconfig                       |  14 +
>  drivers/counter/Makefile                      |   1 +
>  drivers/counter/capture-tiecap.c              | 634 ++++++++++++++++++
>  include/uapi/linux/counter.h                  |   2 +
>  6 files changed, 776 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ecap
>  create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
>  create mode 100644 drivers/counter/capture-tiecap.c
> 
> -- 
> 2.25.1

Something that has become apparent to me is the code repetition in this
driver in order to support the capture buffer. Buffers are common
components in devices, so it'll be good for us to standardize some of
what we're exploring here into an interface that other drivers can also
use. We have two ABIs to consider: the driver interface and the sysfs
interface.

For the sysfs interface, I think we'll have to expose each element
individually (e.g. capture1, capture2, etc.) because sysfs attributes
are suppose to expose only a single datum for any given attribute.

For the driver side, we might want to introduce a new Counter component
type for buffers and respective macros to streamline some of the code
for driver authors. For example, a new COUNTER_COMP_BUFFER_U64 enum
counter_comp_type constant could be introduced to represent a u64 buffer
element; respective struct counter_comp read callbacks could be
introduced::

    int (*count_buffer_u64_read)(struct counter_device *counter,
                                 struct counter_count *count,
				 size_t index, u64 *val);

So a driver author can use the "index" parameter to locate the buffer
element and pass back its value via the "val" parameter. To define the
buffer, maybe helper macros like this could be introduced::

    COUNTER_COMP_COUNT_BUFFER_U64("capture", ecap_cnt_cap_read, 4)

This would define four u64 buffer elements each named prefixed with
"capture" and with their read callbacks set to ecap_cnt_cap_read().

One problem however is that I'm not sure if the C preprocessor would be
able to unroll the COUNTER_COMP_COUNT_BUFFER_U64 to a dynamic number of
elements based on a macro parameter (maybe there is a GCC extension).

I'm just throwing out ideas, so I'd like to hear some comments and
suggestions from others about how we should add buffer support to the
Counter subsystem.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 0/3] ECAP support on TI AM62x SoC

[Julien Panis]

On 14/08/2022 20:00, William Breathitt Gray wrote:
> On Wed, Aug 10, 2022 at 04:07:21PM +0200, Julien Panis wrote:
>> The Enhanced Capture (ECAP) module can be used to timestamp events
>> detected on signal input pin. It can be used for time measurements
>> of pulse train signals.
>>
>> ECAP module includes 4 timestamp capture registers. For all 4 sequenced
>> timestamp capture events (1->2->3->4->1->...), edge polarity (falling/rising
>> edge) can be selected.
>>
>> This driver leverages counter subsystem to :
>> - select edge polarity for all 4 capture events (event mode)
>> - log timestamps for each capture event
>> Event polarity, and CAP1/2/3/4 timestamps give all the information
>> about the input pulse train. Further information can easily be computed :
>> period and/or duty cycle if frequency is constant, elapsed time between
>> pulses, etc...
>>
>> Modifications since v3:
>> 	- Migrate driver from IIO to Counter subsystem
>> 	- Minor modification in yaml ($id) to match Counter subsystem
>> 	- Add ABI documentation
>>
>> Userspace commands :
>> 	### SIGNAL ###
>> 	cd /sys/bus/counter/devices/counter0/signal0
>>
>> 	# Get available polarities for each capture event
>> 	cat polarity1_available
>> 	cat polarity2_available
>> 	cat polarity3_available
>> 	cat polarity4_available
>>
>> 	# Get polarity for each capture event
>> 	cat polarity1
>> 	cat polarity2
>> 	cat polarity3
>> 	cat polarity4
>>
>> 	# Set polarity for each capture event
>> 	echo rising > polarity1
>> 	echo falling > polarity2
>> 	echo rising > polarity3
>> 	echo falling > polarity4
>>
>> 	### COUNT ###
>> 	cd /sys/bus/counter/devices/counter0/count0
>>
>> 	# Run ECAP
>> 	echo 1 > enable
>>
>> 	# Get current timebase counter value
>> 	cat count
>>
>> 	# Get captured timestamps
>> 	cat capture1
>> 	cat capture2
>> 	cat capture3
>> 	cat capture4
>>
>> 	# Note that counter watches can also be used to get
>> 	# data from userspace application
>> 	# -> see tools/counter/counter_example.c
>>
>> 	# Stop ECAP
>> 	echo 0 > enable
>>
>> Julien Panis (3):
>>    dt-binding: counter: add ti,am62-ecap-capture.yaml
>>    Documentation: ABI: add sysfs-bus-counter-ecap
>>    counter: capture-tiecap: capture driver support for ECAP
>>
>>   .../ABI/testing/sysfs-bus-counter-ecap        |  64 ++
>>   .../counter/ti,am62-ecap-capture.yaml         |  61 ++
>>   drivers/counter/Kconfig                       |  14 +
>>   drivers/counter/Makefile                      |   1 +
>>   drivers/counter/capture-tiecap.c              | 634 ++++++++++++++++++
>>   include/uapi/linux/counter.h                  |   2 +
>>   6 files changed, 776 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ecap
>>   create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
>>   create mode 100644 drivers/counter/capture-tiecap.c
>>
>> -- 
>> 2.25.1
> Something that has become apparent to me is the code repetition in this
> driver in order to support the capture buffer. Buffers are common
> components in devices, so it'll be good for us to standardize some of
> what we're exploring here into an interface that other drivers can also
> use. We have two ABIs to consider: the driver interface and the sysfs
> interface.
>
> For the sysfs interface, I think we'll have to expose each element
> individually (e.g. capture1, capture2, etc.) because sysfs attributes
> are suppose to expose only a single datum for any given attribute.
>
> For the driver side, we might want to introduce a new Counter component
> type for buffers and respective macros to streamline some of the code
> for driver authors. For example, a new COUNTER_COMP_BUFFER_U64 enum
> counter_comp_type constant could be introduced to represent a u64 buffer
> element; respective struct counter_comp read callbacks could be
> introduced::
>
>      int (*count_buffer_u64_read)(struct counter_device *counter,
>                                   struct counter_count *count,
> 				 size_t index, u64 *val);
>
> So a driver author can use the "index" parameter to locate the buffer
> element and pass back its value via the "val" parameter.To define the
> buffer, maybe helper macros like this could be introduced::
>
>      COUNTER_COMP_COUNT_BUFFER_U64("capture", ecap_cnt_cap_read, 4)
>
> This would define four u64 buffer elements each named prefixed with
> "capture" and with their read callbacks set to ecap_cnt_cap_read().

Somehow, this "index" parameter would make things much easier for driver 
author.
Besides, defining all these similar functions (cap1_read, ..., 
cap4_read) does not really
make sense for ECAP driver.
So, accessing this index parameter through callback and having the 
possibility to use
the same function (cap_read) for several attributes would be great for 
my driver,
and probably for future ones.

>
> One problem however is that I'm not sure if the C preprocessor would be
> able to unroll the COUNTER_COMP_COUNT_BUFFER_U64 to a dynamic number of
> elements based on a macro parameter (maybe there is a GCC extension).
>
> I'm just throwing out ideas, so I'd like to hear some comments and
> suggestions from others about how we should add buffer support to the
> Counter subsystem.
>
> William Breathitt Gray

Re: [PATCH v4 0/3] ECAP support on TI AM62x SoC

[Julien Panis]

On 14/08/2022 20:00, William Breathitt Gray wrote:
> On Wed, Aug 10, 2022 at 04:07:21PM +0200, Julien Panis wrote:
>> The Enhanced Capture (ECAP) module can be used to timestamp events
>> detected on signal input pin. It can be used for time measurements
>> of pulse train signals.
>>
>> ECAP module includes 4 timestamp capture registers. For all 4 sequenced
>> timestamp capture events (1->2->3->4->1->...), edge polarity (falling/rising
>> edge) can be selected.
>>
>> This driver leverages counter subsystem to :
>> - select edge polarity for all 4 capture events (event mode)
>> - log timestamps for each capture event
>> Event polarity, and CAP1/2/3/4 timestamps give all the information
>> about the input pulse train. Further information can easily be computed :
>> period and/or duty cycle if frequency is constant, elapsed time between
>> pulses, etc...
>>
>> Modifications since v3:
>> 	- Migrate driver from IIO to Counter subsystem
>> 	- Minor modification in yaml ($id) to match Counter subsystem
>> 	- Add ABI documentation
>>
>> Userspace commands :
>> 	### SIGNAL ###
>> 	cd /sys/bus/counter/devices/counter0/signal0
>>
>> 	# Get available polarities for each capture event
>> 	cat polarity1_available
>> 	cat polarity2_available
>> 	cat polarity3_available
>> 	cat polarity4_available
>>
>> 	# Get polarity for each capture event
>> 	cat polarity1
>> 	cat polarity2
>> 	cat polarity3
>> 	cat polarity4
>>
>> 	# Set polarity for each capture event
>> 	echo rising > polarity1
>> 	echo falling > polarity2
>> 	echo rising > polarity3
>> 	echo falling > polarity4
>>
>> 	### COUNT ###
>> 	cd /sys/bus/counter/devices/counter0/count0
>>
>> 	# Run ECAP
>> 	echo 1 > enable
>>
>> 	# Get current timebase counter value
>> 	cat count
>>
>> 	# Get captured timestamps
>> 	cat capture1
>> 	cat capture2
>> 	cat capture3
>> 	cat capture4
>>
>> 	# Note that counter watches can also be used to get
>> 	# data from userspace application
>> 	# -> see tools/counter/counter_example.c
>>
>> 	# Stop ECAP
>> 	echo 0 > enable
>>
>> Julien Panis (3):
>>    dt-binding: counter: add ti,am62-ecap-capture.yaml
>>    Documentation: ABI: add sysfs-bus-counter-ecap
>>    counter: capture-tiecap: capture driver support for ECAP
>>
>>   .../ABI/testing/sysfs-bus-counter-ecap        |  64 ++
>>   .../counter/ti,am62-ecap-capture.yaml         |  61 ++
>>   drivers/counter/Kconfig                       |  14 +
>>   drivers/counter/Makefile                      |   1 +
>>   drivers/counter/capture-tiecap.c              | 634 ++++++++++++++++++
>>   include/uapi/linux/counter.h                  |   2 +
>>   6 files changed, 776 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-bus-counter-ecap
>>   create mode 100644 Documentation/devicetree/bindings/counter/ti,am62-ecap-capture.yaml
>>   create mode 100644 drivers/counter/capture-tiecap.c
>>
>> -- 
>> 2.25.1
> Something that has become apparent to me is the code repetition in this
> driver in order to support the capture buffer. Buffers are common
> components in devices, so it'll be good for us to standardize some of
> what we're exploring here into an interface that other drivers can also
> use. We have two ABIs to consider: the driver interface and the sysfs
> interface.
>
> For the sysfs interface, I think we'll have to expose each element
> individually (e.g. capture1, capture2, etc.) because sysfs attributes
> are suppose to expose only a single datum for any given attribute.
>
> For the driver side, we might want to introduce a new Counter component
> type for buffers and respective macros to streamline some of the code
> for driver authors. For example, a new COUNTER_COMP_BUFFER_U64 enum
> counter_comp_type constant could be introduced to represent a u64 buffer
> element; respective struct counter_comp read callbacks could be
> introduced::
>
>      int (*count_buffer_u64_read)(struct counter_device *counter,
>                                   struct counter_count *count,
> 				 size_t index, u64 *val);
>
> So a driver author can use the "index" parameter to locate the buffer
> element and pass back its value via the "val" parameter. To define the
> buffer, maybe helper macros like this could be introduced::
>
>      COUNTER_COMP_COUNT_BUFFER_U64("capture", ecap_cnt_cap_read, 4)
>
> This would define four u64 buffer elements each named prefixed with
> "capture" and with their read callbacks set to ecap_cnt_cap_read().
>
> One problem however is that I'm not sure if the C preprocessor would be
> able to unroll the COUNTER_COMP_COUNT_BUFFER_U64 to a dynamic number of
> elements based on a macro parameter (maybe there is a GCC extension).
>
> I'm just throwing out ideas, so I'd like to hear some comments and
> suggestions from others about how we should add buffer support to the
> Counter subsystem.
>
> William Breathitt Gray

Hi William,

I am going to send a new version (PATCH v5) without buffers as described 
above
(I added macros for cap1/2/3/4 similar functions). I tried taking into 
account the
comments made by Jonathan and you.

Regarding buffers, I don't know how driver upstream process is done when 
'significant'
subsystem modifications are being considered. Is it a problem for you if 
we continue
driver upstream process until it's merged, and then modify counter 
subsystem (and
ECAP driver with it) to add some buffer functionalities ? Or do you 
prefer adding such
functionalities to counter subsystem while upstreaming ECAP driver, 
without any
intermediate ECAP driver version ?

Julien Panis