[PATCH v3 0/2] ECAP support on TI AM62x SoC

[PATCH v3 0/2] 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 (0->1->2->3->0->...), edge polarity (falling/rising
edge) can be selected. Moreover, input signal can be prescaled.

This driver leverages IIO subsystem to :
- select edge polarity for all 4 capture events (event mode)
- log both event index and timestamp for each capture event
Event polarity, event indexes, and 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 v2:
	- Fix yaml file name
	- Remove unnecessary node in yaml 'examples' section
	- Remove unnecessary error messages in probe function

Userspace commands :
	cd /sys/devices/platform/bus@f0000/23120000.capture/iio\:device2/

	# Configure/Enable data buffers
	echo 1 > scan_elements/in_index_en
	echo 1 > scan_elements/in_timestamp_en
	echo 100 > buffer/length
	echo 1 > buffer/enable

	# Set event mode in range [0 ; 15]
	# 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
	echo 13 > events/change_falling_value

	# Run ECAP
	echo 1 > en

	# Get the number of available data
	cat buffer/data_available

	# Read data
	hexdump -v /dev/iio\:device2

	# Stop ECAP
	echo 0 > en

Julien Panis (2):
  dt-binding: iio: time: add ti,am62-ecap-capture.yaml
  iio: time: capture-tiecap: capture driver support for ECAP

 .../iio/time/ti,am62-ecap-capture.yaml        |  61 +++
 drivers/iio/Kconfig                           |   1 +
 drivers/iio/Makefile                          |   1 +
 drivers/iio/time/Kconfig                      |  22 +
 drivers/iio/time/Makefile                     |   6 +
 drivers/iio/time/capture-tiecap.c             | 517 ++++++++++++++++++
 6 files changed, 608 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/time/ti,am62-ecap-capture.yaml
 create mode 100644 drivers/iio/time/Kconfig
 create mode 100644 drivers/iio/time/Makefile
 create mode 100644 drivers/iio/time/capture-tiecap.c

-- 
2.25.1

[PATCH v3 1/2] dt-binding: iio: time: 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>
---
 .../iio/time/ti,am62-ecap-capture.yaml        | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/time/ti,am62-ecap-capture.yaml

diff --git a/Documentation/devicetree/bindings/iio/time/ti,am62-ecap-capture.yaml b/Documentation/devicetree/bindings/iio/time/ti,am62-ecap-capture.yaml
new file mode 100644
index 000000000000..5c358592942b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/time/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/iio/time/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 v3 1/2] dt-binding: iio: time: add ti,am62-ecap-capture.yaml

[Krzysztof Kozlowski]

On 28/07/2022 19:51, 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>


Best regards,
Krzysztof

[PATCH v3 2/2] iio: time: 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/iio/Kconfig               |   1 +
 drivers/iio/Makefile              |   1 +
 drivers/iio/time/Kconfig          |  22 ++
 drivers/iio/time/Makefile         |   6 +
 drivers/iio/time/capture-tiecap.c | 517 ++++++++++++++++++++++++++++++
 5 files changed, 547 insertions(+)
 create mode 100644 drivers/iio/time/Kconfig
 create mode 100644 drivers/iio/time/Makefile
 create mode 100644 drivers/iio/time/capture-tiecap.c

diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index b190846c3dc2..ba11b8824071 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -99,5 +99,6 @@ source "drivers/iio/pressure/Kconfig"
 source "drivers/iio/proximity/Kconfig"
 source "drivers/iio/resolver/Kconfig"
 source "drivers/iio/temperature/Kconfig"
+source "drivers/iio/time/Kconfig"
 
 endif # IIO
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index 3be08cdadd7e..09283402a2c6 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -42,4 +42,5 @@ obj-y += proximity/
 obj-y += resolver/
 obj-y += temperature/
 obj-y += test/
+obj-y += time/
 obj-y += trigger/
diff --git a/drivers/iio/time/Kconfig b/drivers/iio/time/Kconfig
new file mode 100644
index 000000000000..02f6cf7ff79e
--- /dev/null
+++ b/drivers/iio/time/Kconfig
@@ -0,0 +1,22 @@
+#
+# Time drivers
+#
+
+menu "Time"
+
+config CAPTURE_TIECAP
+	tristate "ECAP capture support"
+	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
+	depends on HAS_IOMEM
+	select IIO_BUFFER
+	select IIO_KFIFO_BUF
+	help
+	  IIO driver support for the ECAP capture hardware found on TI SoCs.
+
+	  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.
+
+endmenu
diff --git a/drivers/iio/time/Makefile b/drivers/iio/time/Makefile
new file mode 100644
index 000000000000..3a27f3557d1e
--- /dev/null
+++ b/drivers/iio/time/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for industrial I/O Time drivers
+#
+
+obj-$(CONFIG_CAPTURE_TIECAP) += capture-tiecap.o
diff --git a/drivers/iio/time/capture-tiecap.c b/drivers/iio/time/capture-tiecap.c
new file mode 100644
index 000000000000..305011836ef3
--- /dev/null
+++ b/drivers/iio/time/capture-tiecap.c
@@ -0,0 +1,517 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * ECAP Capture driver
+ *
+ * Copyright (C) 2022 Julien Panis <jpanis@baylibre.com>
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <linux/pm_runtime.h>
+#include <linux/of_device.h>
+#include <linux/bitfield.h>
+#include <linux/regmap.h>
+#include <linux/interrupt.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/kfifo_buf.h>
+
+/* 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_EVTFLTPS_MASK		GENMASK(13, 9)
+#define ECAP_PS_DEFAULT_VAL		0
+#define ECAP_PS_MAX_VAL		31
+#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_EVTFLTPS_MASK | ECAP_SYNCO_DIS_MASK	\
+					| ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK	\
+					| ECAP_CAP_APWM_BIT | ECAP_CONT_ONESHT_BIT	\
+					| ECAP_REARM_RESET_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, 1)
+#define ECAP_CEVT_FLG_BIT(i)		BIT((i) + 17)
+#define ECAP_OVF_VAL			0xff
+
+#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_NB_EV_MODES		GENMASK(ECAP_NB_CAP - 1, 0)
+#define ECAP_EV_MODE_BIT(i)		BIT(i)
+
+static unsigned int prescaler = ECAP_PS_DEFAULT_VAL;
+module_param(prescaler, uint, 0644);
+MODULE_PARM_DESC(prescaler, "Input capture signal prescaler from 0 to "
+		 __MODULE_STRING(ECAP_PS_MAX_VAL)", default "
+		 __MODULE_STRING(ECAP_PS_DEFAULT_VAL));
+
+static const struct regmap_config ecap_iio_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.max_register = ECAP_PID_REG,
+};
+
+/*
+ * struct ecap_iio_context - IIO device context
+ * @ev_mode:   event mode describing falling/rising edges for captures 1 to 4
+ * @time_cntr: timestamp counter value
+ */
+struct ecap_iio_context {
+	u8 ev_mode;
+	unsigned int time_cntr;
+};
+
+/*
+ * struct ecap_iio_data - IIO device data
+ * @ev_idx:  event index (0 to 3 for CAP1 to CAP4)
+ * @ev_time: falling/rising edge timestamp
+ */
+struct ecap_iio_data {
+	u8 ev_idx;
+	s64 ev_time __aligned(sizeof(s64));
+};
+
+/*
+ * struct ecap_iio_dev - IIO device private data structure
+ * @enabled:  device state
+ * @clk:      device clock
+ * @clk_rate: device clock rate
+ * @regmap:   device register map
+ * @pm_ctx:   device context for PM operations
+ * @data:     device data
+ */
+struct ecap_iio_dev {
+	bool enabled;
+	struct clk *clk;
+	unsigned long clk_rate;
+	struct regmap *regmap;
+	struct ecap_iio_context pm_ctx;
+	struct ecap_iio_data data;
+};
+
+static u8 ecap_iio_capture_get_evmode(struct iio_dev *indio_dev)
+{
+	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
+	u8 ev_mode = 0;
+	unsigned int regval;
+	int i;
+
+	pm_runtime_get_sync(indio_dev->dev.parent);
+	regmap_read(ecap_dev->regmap, ECAP_ECCTL_REG, &regval);
+	pm_runtime_put_sync(indio_dev->dev.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_iio_capture_set_evmode(struct iio_dev *indio_dev, u8 ev_mode)
+{
+	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
+	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(indio_dev->dev.parent);
+	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_EV_MODE_MASK, regval);
+	pm_runtime_put_sync(indio_dev->dev.parent);
+}
+
+static void ecap_iio_capture_enable(struct iio_dev *indio_dev, bool rearm)
+{
+	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
+	unsigned int regval = 0;
+
+	pm_runtime_get_sync(indio_dev->dev.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 |= FIELD_PREP(ECAP_EVTFLTPS_MASK, prescaler)
+		| ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK;
+	if (rearm) {
+		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_iio_capture_disable(struct iio_dev *indio_dev)
+{
+	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
+
+	/* 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(indio_dev->dev.parent);
+}
+
+static int ecap_iio_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long info)
+{
+	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_ENABLE:
+		*val = ecap_dev->enabled;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ecap_iio_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long info)
+{
+	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_ENABLE:
+		if (val < 0 || val > 1)
+			return -EINVAL;
+		if (val == ecap_dev->enabled)
+			return 0;
+		if (val)
+			ecap_iio_capture_enable(indio_dev, true);
+		else
+			ecap_iio_capture_disable(indio_dev);
+		ecap_dev->enabled = val;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ecap_iio_read_event_value(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir,
+				     enum iio_event_info info,
+				     int *val, int *val2)
+{
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (dir) {
+		case IIO_EV_DIR_FALLING:
+			*val = ecap_iio_capture_get_evmode(indio_dev);
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ecap_iio_write_event_value(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      enum iio_event_info info,
+				      int val, int val2)
+{
+	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		switch (dir) {
+		case IIO_EV_DIR_FALLING:
+			if (val < 0 || val > ECAP_NB_EV_MODES)
+				return -EINVAL;
+			if (ecap_dev->enabled)
+				return -EBUSY;
+			ecap_iio_capture_set_evmode(indio_dev, val);
+			return 0;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ecap_iio_info = {
+	.read_raw = ecap_iio_read_raw,
+	.write_raw = ecap_iio_write_raw,
+	.read_event_value = ecap_iio_read_event_value,
+	.write_event_value = ecap_iio_write_event_value,
+};
+
+static const struct iio_event_spec ecap_iio_events[] = {
+	{
+		.type = IIO_EV_TYPE_CHANGE,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
+	},
+};
+
+static const struct iio_chan_spec ecap_iio_channels[] = {
+	{
+		.scan_index = 0,
+		.type = IIO_INDEX,
+		.address = 0,
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_ENABLE),
+		.modified = 0,
+		.event_spec = ecap_iio_events,
+		.num_event_specs = ARRAY_SIZE(ecap_iio_events),
+		.scan_type = {
+			.sign = 'u',
+			.endianness = IIO_LE,
+			.realbits = 2,
+			.storagebits = 8,
+			.shift = 0,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static irqreturn_t ecap_iio_isr(int irq, void *dev_id)
+{
+	struct iio_dev *indio_dev = dev_id;
+	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
+	struct ecap_iio_data *ecap_data = &ecap_dev->data;
+	unsigned int clr = 0;
+	unsigned int flg;
+	unsigned int cap_time;
+	int i;
+
+	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
+
+	for (i = 0 ; i < ECAP_NB_CEVT ; i++) {
+		if (flg & ECAP_CEVT_FLG_BIT(i)) {
+			if (i < ECAP_NB_CAP) {
+				/*
+				 * Input signal edge detected
+				 * time_ns = 10^9 * time_cycles / clk_rate
+				 */
+				ecap_data->ev_idx = i;
+				regmap_read(ecap_dev->regmap, ECAP_CAP_REG(i), &cap_time);
+				ecap_data->ev_time = cap_time * NSEC_PER_SEC;
+				do_div(ecap_data->ev_time, ecap_dev->clk_rate);
+			} else {
+				/* Counter overflow */
+				ecap_data->ev_idx = ECAP_OVF_VAL;
+				ecap_data->ev_time = 0;
+			}
+			iio_push_to_buffers(indio_dev, ecap_data);
+
+			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_iio_clk_disable(void *clk)
+{
+	clk_disable_unprepare(clk);
+}
+
+static int ecap_iio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ecap_iio_dev *ecap_dev;
+	struct iio_dev *indio_dev;
+	void __iomem *mmio_base;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*ecap_dev));
+	if (IS_ERR(indio_dev))
+		return PTR_ERR(indio_dev);
+
+	ecap_dev = iio_priv(indio_dev);
+
+	ecap_dev->clk = devm_clk_get(dev, "fck");
+	if (IS_ERR(ecap_dev->clk)) {
+		dev_err(dev, "failed to get clock\n");
+		return PTR_ERR(ecap_dev->clk);
+	}
+
+	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_iio_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;
+	}
+
+	if (prescaler > ECAP_PS_MAX_VAL) {
+		prescaler = ECAP_PS_MAX_VAL;
+		dev_warn(dev, "prescaler out of range, forced to %d\n", prescaler);
+	}
+
+	mmio_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mmio_base))
+		return PTR_ERR(mmio_base);
+
+	ecap_dev->regmap = regmap_init_mmio(dev, mmio_base, &ecap_iio_regmap_config);
+	if (IS_ERR(ecap_dev->regmap)) {
+		dev_err(dev, "failed to init regmap\n");
+		return PTR_ERR(ecap_dev->regmap);
+	}
+
+	indio_dev->name = devm_kasprintf(dev, GFP_KERNEL,
+					 "ecap-iio-%p", mmio_base);
+	indio_dev->info = &ecap_iio_info;
+	indio_dev->channels = ecap_iio_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ecap_iio_channels);
+	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
+
+	ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, NULL, NULL);
+	if (ret) {
+		dev_err(dev, "failed to setup iio buffer\n");
+		return ret;
+	}
+
+	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_iio_isr, 0, pdev->name, indio_dev);
+	if (ret) {
+		dev_err(dev, "failed to request irq\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, indio_dev);
+	pm_runtime_enable(&pdev->dev);
+
+	ecap_dev->enabled = 0;
+	ecap_iio_capture_set_evmode(indio_dev, 0);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int ecap_iio_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
+
+	if (ecap_dev->enabled)
+		ecap_iio_capture_disable(indio_dev);
+
+	regmap_exit(ecap_dev->regmap);
+
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static __maybe_unused int ecap_iio_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
+
+	/* If eCAP is running, stop capture then save timestamp counter */
+	if (ecap_dev->enabled) {
+		ecap_iio_capture_disable(indio_dev);
+
+		pm_runtime_get_sync(indio_dev->dev.parent);
+		regmap_read(ecap_dev->regmap, ECAP_TSCNT_REG, &ecap_dev->pm_ctx.time_cntr);
+		pm_runtime_put_sync(indio_dev->dev.parent);
+	}
+
+	ecap_dev->pm_ctx.ev_mode = ecap_iio_capture_get_evmode(indio_dev);
+
+	return 0;
+}
+
+static __maybe_unused int ecap_iio_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
+
+	ecap_iio_capture_set_evmode(indio_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(indio_dev->dev.parent);
+		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, ecap_dev->pm_ctx.time_cntr);
+		pm_runtime_put_sync(indio_dev->dev.parent);
+
+		ecap_iio_capture_enable(indio_dev, false);
+	}
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(ecap_iio_pm_ops, ecap_iio_suspend, ecap_iio_resume);
+
+static const struct of_device_id ecap_iio_of_match[] = {
+	{ .compatible	= "ti,am62-ecap-capture" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ecap_iio_of_match);
+
+static struct platform_driver ecap_iio_driver = {
+	.probe = ecap_iio_probe,
+	.remove = ecap_iio_remove,
+	.driver = {
+		.name = "ecap-capture",
+		.of_match_table = of_match_ptr(ecap_iio_of_match),
+		.pm = &ecap_iio_pm_ops,
+	},
+};
+module_platform_driver(ecap_iio_driver);
+
+MODULE_DESCRIPTION("ECAP Capture driver");
+MODULE_AUTHOR("Julien Panis <jpanis@baylibre.com>");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Christophe JAILLET]

Le 28/07/2022 à 19:51, Julien Panis a écrit :
> 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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>

[...]

> +static int ecap_iio_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +
> +	if (ecap_dev->enabled)
> +		ecap_iio_capture_disable(indio_dev);
> +
> +	regmap_exit(ecap_dev->regmap);
> +
> +	pm_runtime_disable(&pdev->dev);

Hi,

should these 2 functions be part of an error handling path of the probe, 
or handled with a devm_add_action_or_reset()?

Just my 2c,

CJ

> +
> +	return 0;
> +}
> +

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Julien Panis]

On 28/07/2022 22:02, Christophe JAILLET wrote:
> Le 28/07/2022 à 19:51, Julien Panis a écrit :
>> 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-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>
> [...]
>
>> +static int ecap_iio_remove(struct platform_device *pdev)
>> +{
>> +    struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +    struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +
>> +    if (ecap_dev->enabled)
>> +        ecap_iio_capture_disable(indio_dev);
>> +
>> +    regmap_exit(ecap_dev->regmap);
>> +
>> +    pm_runtime_disable(&pdev->dev);
>
> Hi,
>
> should these 2 functions be part of an error handling path of the 
> probe, or handled with a devm_add_action_or_reset()?
>
> Just my 2c,
>
> CJ

Hi Christophe,
That's right, that will be cleaner. Thank you for your suggestion, that 
will be done in next version.
Julien

>
>> +
>> +    return 0;
>> +}
>> +

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Jonathan Cameron]

On Thu, 28 Jul 2022 19:51: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,

So this isn't the first ecap driver we've had proposed, but the previous
one was a few years ago and never reached v2.
https://lore.kernel.org/all/20200818153614.6438-1-dts86@cornell.edu/

Honestly I can't remember much about it, but maybe the discussion around
that will be worth a reread.

The use of ABI here is unusual. So I'd definitely like to see some documentation
probably as a file in the main kernel documentation to explain what the interface
is an how that relates to what is being captured.

First thing to note here is the channel type of IIO_INDEX is now not actually
used any more because we moved all the relevant drivers over to the counter
subsystem (and we failed to mark it deprecated). 

Anyhow, I've reviewed below, but need docs to discuss this in depth.  In particular
the mix of buffers and events interfaces is unlikely to be an acceptable path
forwards.

Jonathan

> ---
>  drivers/iio/Kconfig               |   1 +
>  drivers/iio/Makefile              |   1 +
>  drivers/iio/time/Kconfig          |  22 ++
>  drivers/iio/time/Makefile         |   6 +
>  drivers/iio/time/capture-tiecap.c | 517 ++++++++++++++++++++++++++++++
>  5 files changed, 547 insertions(+)
>  create mode 100644 drivers/iio/time/Kconfig
>  create mode 100644 drivers/iio/time/Makefile
>  create mode 100644 drivers/iio/time/capture-tiecap.c
> 
> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
> index b190846c3dc2..ba11b8824071 100644
> --- a/drivers/iio/Kconfig
> +++ b/drivers/iio/Kconfig
> @@ -99,5 +99,6 @@ source "drivers/iio/pressure/Kconfig"
>  source "drivers/iio/proximity/Kconfig"
>  source "drivers/iio/resolver/Kconfig"
>  source "drivers/iio/temperature/Kconfig"
> +source "drivers/iio/time/Kconfig"
>  
>  endif # IIO
> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
> index 3be08cdadd7e..09283402a2c6 100644
> --- a/drivers/iio/Makefile
> +++ b/drivers/iio/Makefile
> @@ -42,4 +42,5 @@ obj-y += proximity/
>  obj-y += resolver/
>  obj-y += temperature/
>  obj-y += test/
> +obj-y += time/
>  obj-y += trigger/
> diff --git a/drivers/iio/time/Kconfig b/drivers/iio/time/Kconfig
> new file mode 100644
> index 000000000000..02f6cf7ff79e
> --- /dev/null
> +++ b/drivers/iio/time/Kconfig
> @@ -0,0 +1,22 @@
> +#
> +# Time drivers
> +#
> +
> +menu "Time"
> +
> +config CAPTURE_TIECAP
> +	tristate "ECAP capture support"
> +	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	select IIO_BUFFER
> +	select IIO_KFIFO_BUF
> +	help
> +	  IIO driver support for the ECAP capture hardware found on TI SoCs.
> +
> +	  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.
> +
> +endmenu
> diff --git a/drivers/iio/time/Makefile b/drivers/iio/time/Makefile
> new file mode 100644
> index 000000000000..3a27f3557d1e
> --- /dev/null
> +++ b/drivers/iio/time/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for industrial I/O Time drivers
> +#
> +
> +obj-$(CONFIG_CAPTURE_TIECAP) += capture-tiecap.o
> diff --git a/drivers/iio/time/capture-tiecap.c b/drivers/iio/time/capture-tiecap.c
> new file mode 100644
> index 000000000000..305011836ef3
> --- /dev/null
> +++ b/drivers/iio/time/capture-tiecap.c
> @@ -0,0 +1,517 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * ECAP Capture driver
> + *
> + * Copyright (C) 2022 Julien Panis <jpanis@baylibre.com>
> + */
> +
> +#include <linux/module.h>
Headers preferred in alphabetical order but with more specific
headers in groups.

#include <linux/clk.h
...
#include <linux/regmap.h>

#include <linux/iio/buffer.h>
#include <linux/iio/iio.h>
#include <linux/iio/kfifo_buf.h>


> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/clk.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/of_device.h>
Bad sign.  What's this used for?  Unless absolutely necessary please use
linux/property.h interfaces.
Probably you should have 
linux/mod_devicetable.h for the of_device_id structure definition.


> +#include <linux/bitfield.h>
> +#include <linux/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +
> +/* 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_EVTFLTPS_MASK		GENMASK(13, 9)
> +#define ECAP_PS_DEFAULT_VAL		0
> +#define ECAP_PS_MAX_VAL		31
> +#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_EVTFLTPS_MASK | ECAP_SYNCO_DIS_MASK	\
> +					| ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK	\
> +					| ECAP_CAP_APWM_BIT | ECAP_CONT_ONESHT_BIT	\
> +					| ECAP_REARM_RESET_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, 1)
> +#define ECAP_CEVT_FLG_BIT(i)		BIT((i) + 17)
> +#define ECAP_OVF_VAL			0xff
> +
> +#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_NB_EV_MODES		GENMASK(ECAP_NB_CAP - 1, 0)
> +#define ECAP_EV_MODE_BIT(i)		BIT(i)
> +
> +static unsigned int prescaler = ECAP_PS_DEFAULT_VAL;
> +module_param(prescaler, uint, 0644);
> +MODULE_PARM_DESC(prescaler, "Input capture signal prescaler from 0 to "
> +		 __MODULE_STRING(ECAP_PS_MAX_VAL)", default "
> +		 __MODULE_STRING(ECAP_PS_DEFAULT_VAL));

What is this?  Needs a lot more description...  Right now I'd suggest it probably
either wants to be firmware provided (DT) or userspace controlled via proper ABI.

> +
> +static const struct regmap_config ecap_iio_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.max_register = ECAP_PID_REG,
> +};
> +
> +/*
> + * struct ecap_iio_context - IIO device context
> + * @ev_mode:   event mode describing falling/rising edges for captures 1 to 4
> + * @time_cntr: timestamp counter value
> + */
> +struct ecap_iio_context {
> +	u8 ev_mode;
> +	unsigned int time_cntr;
> +};

I don't see advantage in defining this outside of ecap_iio_dev.
If the grouping is particular useful, feel free to embed the structure in there
and give it a name.

> +
> +/*
when in kernel-doc format use /**

> + * struct ecap_iio_data - IIO device data
> + * @ev_idx:  event index (0 to 3 for CAP1 to CAP4)

That's a long way from standard ABI.

> + * @ev_time: falling/rising edge timestamp
> + */
> +struct ecap_iio_data {
> +	u8 ev_idx;
> +	s64 ev_time __aligned(sizeof(s64));
> +};
> +
> +/*
> + * struct ecap_iio_dev - IIO device private data structure
> + * @enabled:  device state
> + * @clk:      device clock
> + * @clk_rate: device clock rate
> + * @regmap:   device register map
> + * @pm_ctx:   device context for PM operations
> + * @data:     device data
> + */
> +struct ecap_iio_dev {
> +	bool enabled;
> +	struct clk *clk;
> +	unsigned long clk_rate;
> +	struct regmap *regmap;
> +	struct ecap_iio_context pm_ctx;
> +	struct ecap_iio_data data;
> +};
> +
> +static u8 ecap_iio_capture_get_evmode(struct iio_dev *indio_dev)
> +{
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +	u8 ev_mode = 0;
> +	unsigned int regval;
> +	int i;
> +
> +	pm_runtime_get_sync(indio_dev->dev.parent);
> +	regmap_read(ecap_dev->regmap, ECAP_ECCTL_REG, &regval);
> +	pm_runtime_put_sync(indio_dev->dev.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_iio_capture_set_evmode(struct iio_dev *indio_dev, u8 ev_mode)
> +{
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +	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(indio_dev->dev.parent);
> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_EV_MODE_MASK, regval);
> +	pm_runtime_put_sync(indio_dev->dev.parent);
> +}
> +
> +static void ecap_iio_capture_enable(struct iio_dev *indio_dev, bool rearm)
> +{
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +	unsigned int regval = 0;
> +
> +	pm_runtime_get_sync(indio_dev->dev.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 |= FIELD_PREP(ECAP_EVTFLTPS_MASK, prescaler)
> +		| ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK;
Should simply be
regval = ...
and don't set to 0 above.

> +	if (rearm) {
> +		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_iio_capture_disable(struct iio_dev *indio_dev)
> +{
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +
> +	/* 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(indio_dev->dev.parent);
> +}
> +
> +static int ecap_iio_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long info)
> +{
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_ENABLE:
> +		*val = ecap_dev->enabled;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ecap_iio_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long info)
> +{
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_ENABLE:
> +		if (val < 0 || val > 1)
> +			return -EINVAL;
> +		if (val == ecap_dev->enabled)
> +			return 0;
> +		if (val)
> +			ecap_iio_capture_enable(indio_dev, true);
> +		else
> +			ecap_iio_capture_disable(indio_dev);
> +		ecap_dev->enabled = val;
Blank line here.   Generally add one before any simple return
statements as it makes the flow a little more readable.

> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ecap_iio_read_event_value(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir,
> +				     enum iio_event_info info,
> +				     int *val, int *val2)
> +{
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		switch (dir) {
> +		case IIO_EV_DIR_FALLING:
> +			*val = ecap_iio_capture_get_evmode(indio_dev);
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ecap_iio_write_event_value(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir,
> +				      enum iio_event_info info,
> +				      int val, int val2)
> +{
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		switch (dir) {
> +		case IIO_EV_DIR_FALLING:
> +			if (val < 0 || val > ECAP_NB_EV_MODES)

So you are using a value as a magic enum to set mode?

Don't do that without documentation (and probably not acceptable then).
This is device specific ABI pretending to be generic.

> +				return -EINVAL;
> +			if (ecap_dev->enabled)
> +				return -EBUSY;
> +			ecap_iio_capture_set_evmode(indio_dev, val);
> +			return 0;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ecap_iio_info = {
> +	.read_raw = ecap_iio_read_raw,
> +	.write_raw = ecap_iio_write_raw,
> +	.read_event_value = ecap_iio_read_event_value,
> +	.write_event_value = ecap_iio_write_event_value,
> +};
> +
> +static const struct iio_event_spec ecap_iio_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_CHANGE,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
> +	},
> +};
> +
> +static const struct iio_chan_spec ecap_iio_channels[] = {
> +	{
> +		.scan_index = 0,
> +		.type = IIO_INDEX,
> +		.address = 0,
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_ENABLE),
> +		.modified = 0,
C will initialize anything not specificed to 0 anyway, so just add
0 entries if they aren't the natural default (i.e. enums etc).

> +		.event_spec = ecap_iio_events,
> +		.num_event_specs = ARRAY_SIZE(ecap_iio_events),
> +		.scan_type = {
> +			.sign = 'u',
> +			.endianness = IIO_LE,
> +			.realbits = 2,
> +			.storagebits = 8,
> +			.shift = 0,
> +		},
> +	},
> +	IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static irqreturn_t ecap_iio_isr(int irq, void *dev_id)
> +{
> +	struct iio_dev *indio_dev = dev_id;
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +	struct ecap_iio_data *ecap_data = &ecap_dev->data;
> +	unsigned int clr = 0;
> +	unsigned int flg;
> +	unsigned int cap_time;
> +	int i;
> +
> +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
> +
> +	for (i = 0 ; i < ECAP_NB_CEVT ; i++) {
> +		if (flg & ECAP_CEVT_FLG_BIT(i)) {
> +			if (i < ECAP_NB_CAP) {
> +				/*
> +				 * Input signal edge detected
> +				 * time_ns = 10^9 * time_cycles / clk_rate
> +				 */
> +				ecap_data->ev_idx = i;
> +				regmap_read(ecap_dev->regmap, ECAP_CAP_REG(i), &cap_time);
> +				ecap_data->ev_time = cap_time * NSEC_PER_SEC;
> +				do_div(ecap_data->ev_time, ecap_dev->clk_rate);
Is there any attempt to align that timestamp with the iio clock used for software timestamps?
> +			} else {
> +				/* Counter overflow */
> +				ecap_data->ev_idx = ECAP_OVF_VAL;
> +				ecap_data->ev_time = 0;

Don't push it if you've lost data.

> +			}
> +			iio_push_to_buffers(indio_dev, ecap_data);
> +
> +			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_iio_clk_disable(void *clk)
> +{
> +	clk_disable_unprepare(clk);
> +}
> +
> +static int ecap_iio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ecap_iio_dev *ecap_dev;
> +	struct iio_dev *indio_dev;
> +	void __iomem *mmio_base;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*ecap_dev));
> +	if (IS_ERR(indio_dev))
> +		return PTR_ERR(indio_dev);
> +
> +	ecap_dev = iio_priv(indio_dev);
> +
> +	ecap_dev->clk = devm_clk_get(dev, "fck");
> +	if (IS_ERR(ecap_dev->clk)) {
> +		dev_err(dev, "failed to get clock\n");
> +		return PTR_ERR(ecap_dev->clk);

Preference in probe() for using dev_err_probe() throughout.
It will handle any deferred probes that show up + does nicer message printing.

> +	}
> +
> +	ret = clk_prepare_enable(ecap_dev->clk);
There is a continuing effort to add devm support for clock management.
Not sure if it will land this cycle, but if it does please update to use
that.
https://lore.kernel.org/all/20220520075737.758761-1-u.kleine-koenig@pengutronix.de/

> +	if (ret) {
> +		dev_err(dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, ecap_iio_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;
> +	}
> +
> +	if (prescaler > ECAP_PS_MAX_VAL) {
> +		prescaler = ECAP_PS_MAX_VAL;
> +		dev_warn(dev, "prescaler out of range, forced to %d\n", prescaler);
> +	}
> +
> +	mmio_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(mmio_base))
> +		return PTR_ERR(mmio_base);
> +
> +	ecap_dev->regmap = regmap_init_mmio(dev, mmio_base, &ecap_iio_regmap_config);

Mixing devm and non devm just makes for possible race conditions.
devm_regmap_init_mmio() should be fine here. If it's not I'd expect to see clear
comments saying why not.

> +	if (IS_ERR(ecap_dev->regmap)) {
> +		dev_err(dev, "failed to init regmap\n");
> +		return PTR_ERR(ecap_dev->regmap);
> +	}
> +
> +	indio_dev->name = devm_kasprintf(dev, GFP_KERNEL,
> +					 "ecap-iio-%p", mmio_base);
The name is a part number only, not instances specific.  The instance can usually
be easily established from other available info.  If you need to provide more information
there is a label attribute in the ABI (normally used for things like 'where' the sensor
is, but could be used for this as well).

> +	indio_dev->info = &ecap_iio_info;
> +	indio_dev->channels = ecap_iio_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ecap_iio_channels);
> +	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
> +
> +	ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, NULL, NULL);

Might as well use the non _ext() variant that is just a wrapper around this
with one fewer parameter.

> +	if (ret) {
> +		dev_err(dev, "failed to setup iio buffer\n");
> +		return ret;
> +	}
> +
> +	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_iio_isr, 0, pdev->name, indio_dev);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, indio_dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ecap_dev->enabled = 0;

Will be set by default anyway as iio_priv() is kzalloc'd but
fine to leave it here if you think it adds documentation.

> +	ecap_iio_capture_set_evmode(indio_dev, 0);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static int ecap_iio_remove(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +
> +	if (ecap_dev->enabled)
> +		ecap_iio_capture_disable(indio_dev);
> +
> +	regmap_exit(ecap_dev->regmap);
> +
> +	pm_runtime_disable(&pdev->dev);

Order in remove should be reverse of probe() unless there is really
good reason why not.  Otherwise there tend to be race conditions
hiding.

> +
> +	return 0;
> +}
> +
> +static __maybe_unused int ecap_iio_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +
> +	/* If eCAP is running, stop capture then save timestamp counter */
> +	if (ecap_dev->enabled) {
> +		ecap_iio_capture_disable(indio_dev);
> +
> +		pm_runtime_get_sync(indio_dev->dev.parent);
> +		regmap_read(ecap_dev->regmap, ECAP_TSCNT_REG, &ecap_dev->pm_ctx.time_cntr);
> +		pm_runtime_put_sync(indio_dev->dev.parent);
> +	}
> +
> +	ecap_dev->pm_ctx.ev_mode = ecap_iio_capture_get_evmode(indio_dev);
> +
> +	return 0;
> +}
> +
> +static __maybe_unused int ecap_iio_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> +
> +	ecap_iio_capture_set_evmode(indio_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(indio_dev->dev.parent);
> +		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, ecap_dev->pm_ctx.time_cntr);
> +		pm_runtime_put_sync(indio_dev->dev.parent);
> +
> +		ecap_iio_capture_enable(indio_dev, false);
> +	}
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ecap_iio_pm_ops, ecap_iio_suspend, ecap_iio_resume);

DEFINE_SIMPLE_DEV_PM_OPS() as per docs in pm.h which mark this version as deprecated.

> +
> +static const struct of_device_id ecap_iio_of_match[] = {
> +	{ .compatible	= "ti,am62-ecap-capture" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ecap_iio_of_match);
> +
> +static struct platform_driver ecap_iio_driver = {
> +	.probe = ecap_iio_probe,
> +	.remove = ecap_iio_remove,
> +	.driver = {
> +		.name = "ecap-capture",
> +		.of_match_table = of_match_ptr(ecap_iio_of_match),
Drop the of_match_ptr() protection.  It provides no real advantages and breaks
some non dt firmwares.

> +		.pm = &ecap_iio_pm_ops,

pm_sleep_ptr() and remove the __maybe_unused markings above as this new set
of pm macros lets the compiler remove the code without the need for that trickery.
 
> +	},
> +};
> +module_platform_driver(ecap_iio_driver);
> +
> +MODULE_DESCRIPTION("ECAP Capture driver");
> +MODULE_AUTHOR("Julien Panis <jpanis@baylibre.com>");
> +MODULE_LICENSE("GPL");

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Julien Panis]

On 31/07/2022 17:41, Jonathan Cameron wrote:
> On Thu, 28 Jul 2022 19:51: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,
>
> So this isn't the first ecap driver we've had proposed, but the previous
> one was a few years ago and never reached v2.
> https://lore.kernel.org/all/20200818153614.6438-1-dts86@cornell.edu/
>
> Honestly I can't remember much about it, but maybe the discussion around
> that will be worth a reread.

Hi Jonathan, thank you for your review.

I read the discussion about previous attempt, before submitting this 
patch. There were
interesting comments indeed.

But in this previous attempt, only one-shot pulses were handled 
(moreover, global IRQ flag
was not cleared, so I'm not sure that IRQ could be raised more than once).

However, ECAP can be used to make time measurements for any type of 
"square waveform".
That's why I tried to make this event mode configurable. Besides, using 
a continuous mode allows
handling much more signal types (not only single pulses).

>
> The use of ABI here is unusual. So I'd definitely like to see some documentation
> probably as a file in the main kernel documentation to explain what the interface
> is an how that relates to what is being captured.

OK, I will add some userspace documentation.

>
> First thing to note here is the channel type of IIO_INDEX is now not actually
> used any more because we moved all the relevant drivers over to the counter
> subsystem (and we failed to mark it deprecated).

I evaluated this counter subsystem before starting development. Counting 
events is not "a priori"
the goal when using ECAP.

Nevertheless, maybe "counter_push_event" function could do the job. If I 
use counter API :
# Option 1 : CAP1/2/3/4 registers could be seen as 4 channels of the 
same counter...
but there are not channels, there are just sequential timestamps 
actually. So I'm afraid this leads
to misunderstanding for the user.
Moreover, the user will have to read several entries (counts 1/2/3/4) to 
gather timestamps from
the same input signal, which is not very convenient.
# Option 2 : Either CAP 1/2/3/4 events could be gathered in a single 
channel...but then it will not
be possible to configure their polarity (rising/falling edge) 
individually (unless I did
not understand well counter framework documentation).

So, even with counter framework, it will lead to some diverted use of 
the framwork, since ECAP
is a very specific hardware that do not fit 100% counter philosophy.

I admit that ECAP do not fit 100% IIO philosophy either.

Maybe misc API would be more relevant actually. Any opinion about it 
will be welcome. :-)

>
> Anyhow, I've reviewed below, but need docs to discuss this in depth.  In particular
> the mix of buffers and events interfaces is unlikely to be an acceptable path
> forwards.

OK, I will consider alternatives.

>
> Jonathan
>
>> ---
>>   drivers/iio/Kconfig               |   1 +
>>   drivers/iio/Makefile              |   1 +
>>   drivers/iio/time/Kconfig          |  22 ++
>>   drivers/iio/time/Makefile         |   6 +
>>   drivers/iio/time/capture-tiecap.c | 517 ++++++++++++++++++++++++++++++
>>   5 files changed, 547 insertions(+)
>>   create mode 100644 drivers/iio/time/Kconfig
>>   create mode 100644 drivers/iio/time/Makefile
>>   create mode 100644 drivers/iio/time/capture-tiecap.c
>>
>> diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
>> index b190846c3dc2..ba11b8824071 100644
>> --- a/drivers/iio/Kconfig
>> +++ b/drivers/iio/Kconfig
>> @@ -99,5 +99,6 @@ source "drivers/iio/pressure/Kconfig"
>>   source "drivers/iio/proximity/Kconfig"
>>   source "drivers/iio/resolver/Kconfig"
>>   source "drivers/iio/temperature/Kconfig"
>> +source "drivers/iio/time/Kconfig"
>>   
>>   endif # IIO
>> diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
>> index 3be08cdadd7e..09283402a2c6 100644
>> --- a/drivers/iio/Makefile
>> +++ b/drivers/iio/Makefile
>> @@ -42,4 +42,5 @@ obj-y += proximity/
>>   obj-y += resolver/
>>   obj-y += temperature/
>>   obj-y += test/
>> +obj-y += time/
>>   obj-y += trigger/
>> diff --git a/drivers/iio/time/Kconfig b/drivers/iio/time/Kconfig
>> new file mode 100644
>> index 000000000000..02f6cf7ff79e
>> --- /dev/null
>> +++ b/drivers/iio/time/Kconfig
>> @@ -0,0 +1,22 @@
>> +#
>> +# Time drivers
>> +#
>> +
>> +menu "Time"
>> +
>> +config CAPTURE_TIECAP
>> +	tristate "ECAP capture support"
>> +	depends on ARCH_OMAP2PLUS || ARCH_DAVINCI_DA8XX || ARCH_KEYSTONE || ARCH_K3 || COMPILE_TEST
>> +	depends on HAS_IOMEM
>> +	select IIO_BUFFER
>> +	select IIO_KFIFO_BUF
>> +	help
>> +	  IIO driver support for the ECAP capture hardware found on TI SoCs.
>> +
>> +	  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.
>> +
>> +endmenu
>> diff --git a/drivers/iio/time/Makefile b/drivers/iio/time/Makefile
>> new file mode 100644
>> index 000000000000..3a27f3557d1e
>> --- /dev/null
>> +++ b/drivers/iio/time/Makefile
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# Makefile for industrial I/O Time drivers
>> +#
>> +
>> +obj-$(CONFIG_CAPTURE_TIECAP) += capture-tiecap.o
>> diff --git a/drivers/iio/time/capture-tiecap.c b/drivers/iio/time/capture-tiecap.c
>> new file mode 100644
>> index 000000000000..305011836ef3
>> --- /dev/null
>> +++ b/drivers/iio/time/capture-tiecap.c
>> @@ -0,0 +1,517 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * ECAP Capture driver
>> + *
>> + * Copyright (C) 2022 Julien Panis <jpanis@baylibre.com>
>> + */
>> +
>> +#include <linux/module.h>
> Headers preferred in alphabetical order but with more specific
> headers in groups.
>
> #include <linux/clk.h
> ...
> #include <linux/regmap.h>
>
> #include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/kfifo_buf.h>

OK

>
>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +#include <linux/err.h>
>> +#include <linux/clk.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/of_device.h>
> Bad sign.  What's this used for?  Unless absolutely necessary please use
> linux/property.h interfaces.
> Probably you should have
> linux/mod_devicetable.h for the of_device_id structure definition.

OK

>
>
>> +#include <linux/bitfield.h>
>> +#include <linux/regmap.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/kfifo_buf.h>
>> +
>> +/* 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_EVTFLTPS_MASK		GENMASK(13, 9)
>> +#define ECAP_PS_DEFAULT_VAL		0
>> +#define ECAP_PS_MAX_VAL		31
>> +#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_EVTFLTPS_MASK | ECAP_SYNCO_DIS_MASK	\
>> +					| ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK	\
>> +					| ECAP_CAP_APWM_BIT | ECAP_CONT_ONESHT_BIT	\
>> +					| ECAP_REARM_RESET_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, 1)
>> +#define ECAP_CEVT_FLG_BIT(i)		BIT((i) + 17)
>> +#define ECAP_OVF_VAL			0xff
>> +
>> +#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_NB_EV_MODES		GENMASK(ECAP_NB_CAP - 1, 0)
>> +#define ECAP_EV_MODE_BIT(i)		BIT(i)
>> +
>> +static unsigned int prescaler = ECAP_PS_DEFAULT_VAL;
>> +module_param(prescaler, uint, 0644);
>> +MODULE_PARM_DESC(prescaler, "Input capture signal prescaler from 0 to "
>> +		 __MODULE_STRING(ECAP_PS_MAX_VAL)", default "
>> +		 __MODULE_STRING(ECAP_PS_DEFAULT_VAL));
> What is this?  Needs a lot more description...  Right now I'd suggest it probably
> either wants to be firmware provided (DT) or userspace controlled via proper ABI.

OK

>
>> +
>> +static const struct regmap_config ecap_iio_regmap_config = {
>> +	.reg_bits = 32,
>> +	.reg_stride = 4,
>> +	.val_bits = 32,
>> +	.max_register = ECAP_PID_REG,
>> +};
>> +
>> +/*
>> + * struct ecap_iio_context - IIO device context
>> + * @ev_mode:   event mode describing falling/rising edges for captures 1 to 4
>> + * @time_cntr: timestamp counter value
>> + */
>> +struct ecap_iio_context {
>> +	u8 ev_mode;
>> +	unsigned int time_cntr;
>> +};
> I don't see advantage in defining this outside of ecap_iio_dev.
> If the grouping is particular useful, feel free to embed the structure in there
> and give it a name.

OK

>
>> +
>> +/*
> when in kernel-doc format use /**

OK

>
>> + * struct ecap_iio_data - IIO device data
>> + * @ev_idx:  event index (0 to 3 for CAP1 to CAP4)
> That's a long way from standard ABI.

OK, I will consider alternatives.

>
>> + * @ev_time: falling/rising edge timestamp
>> + */
>> +struct ecap_iio_data {
>> +	u8 ev_idx;
>> +	s64 ev_time __aligned(sizeof(s64));
>> +};
>> +
>> +/*
>> + * struct ecap_iio_dev - IIO device private data structure
>> + * @enabled:  device state
>> + * @clk:      device clock
>> + * @clk_rate: device clock rate
>> + * @regmap:   device register map
>> + * @pm_ctx:   device context for PM operations
>> + * @data:     device data
>> + */
>> +struct ecap_iio_dev {
>> +	bool enabled;
>> +	struct clk *clk;
>> +	unsigned long clk_rate;
>> +	struct regmap *regmap;
>> +	struct ecap_iio_context pm_ctx;
>> +	struct ecap_iio_data data;
>> +};
>> +
>> +static u8 ecap_iio_capture_get_evmode(struct iio_dev *indio_dev)
>> +{
>> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +	u8 ev_mode = 0;
>> +	unsigned int regval;
>> +	int i;
>> +
>> +	pm_runtime_get_sync(indio_dev->dev.parent);
>> +	regmap_read(ecap_dev->regmap, ECAP_ECCTL_REG, &regval);
>> +	pm_runtime_put_sync(indio_dev->dev.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_iio_capture_set_evmode(struct iio_dev *indio_dev, u8 ev_mode)
>> +{
>> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +	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(indio_dev->dev.parent);
>> +	regmap_update_bits(ecap_dev->regmap, ECAP_ECCTL_REG, ECAP_EV_MODE_MASK, regval);
>> +	pm_runtime_put_sync(indio_dev->dev.parent);
>> +}
>> +
>> +static void ecap_iio_capture_enable(struct iio_dev *indio_dev, bool rearm)
>> +{
>> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +	unsigned int regval = 0;
>> +
>> +	pm_runtime_get_sync(indio_dev->dev.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 |= FIELD_PREP(ECAP_EVTFLTPS_MASK, prescaler)
>> +		| ECAP_SYNCO_DIS_MASK | ECAP_STOPVALUE_MASK | ECAP_ECCTL_EN_MASK;
> Should simply be
> regval = ...
> and don't set to 0 above.

OK

>
>> +	if (rearm) {
>> +		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_iio_capture_disable(struct iio_dev *indio_dev)
>> +{
>> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +
>> +	/* 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(indio_dev->dev.parent);
>> +}
>> +
>> +static int ecap_iio_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan,
>> +			     int *val, int *val2, long info)
>> +{
>> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +
>> +	switch (info) {
>> +	case IIO_CHAN_INFO_ENABLE:
>> +		*val = ecap_dev->enabled;
>> +		return IIO_VAL_INT;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int ecap_iio_write_raw(struct iio_dev *indio_dev,
>> +			      struct iio_chan_spec const *chan,
>> +			      int val, int val2, long info)
>> +{
>> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +
>> +	switch (info) {
>> +	case IIO_CHAN_INFO_ENABLE:
>> +		if (val < 0 || val > 1)
>> +			return -EINVAL;
>> +		if (val == ecap_dev->enabled)
>> +			return 0;
>> +		if (val)
>> +			ecap_iio_capture_enable(indio_dev, true);
>> +		else
>> +			ecap_iio_capture_disable(indio_dev);
>> +		ecap_dev->enabled = val;
> Blank line here.   Generally add one before any simple return
> statements as it makes the flow a little more readable.

OK

>
>> +		return 0;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int ecap_iio_read_event_value(struct iio_dev *indio_dev,
>> +				     const struct iio_chan_spec *chan,
>> +				     enum iio_event_type type,
>> +				     enum iio_event_direction dir,
>> +				     enum iio_event_info info,
>> +				     int *val, int *val2)
>> +{
>> +	switch (info) {
>> +	case IIO_EV_INFO_VALUE:
>> +		switch (dir) {
>> +		case IIO_EV_DIR_FALLING:
>> +			*val = ecap_iio_capture_get_evmode(indio_dev);
>> +			return IIO_VAL_INT;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int ecap_iio_write_event_value(struct iio_dev *indio_dev,
>> +				      const struct iio_chan_spec *chan,
>> +				      enum iio_event_type type,
>> +				      enum iio_event_direction dir,
>> +				      enum iio_event_info info,
>> +				      int val, int val2)
>> +{
>> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +
>> +	switch (info) {
>> +	case IIO_EV_INFO_VALUE:
>> +		switch (dir) {
>> +		case IIO_EV_DIR_FALLING:
>> +			if (val < 0 || val > ECAP_NB_EV_MODES)
> So you are using a value as a magic enum to set mode?
>
> Don't do that without documentation (and probably not acceptable then).
> This is device specific ABI pretending to be generic.

OK, I will consider alternatives.

>
>> +				return -EINVAL;
>> +			if (ecap_dev->enabled)
>> +				return -EBUSY;
>> +			ecap_iio_capture_set_evmode(indio_dev, val);
>> +			return 0;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static const struct iio_info ecap_iio_info = {
>> +	.read_raw = ecap_iio_read_raw,
>> +	.write_raw = ecap_iio_write_raw,
>> +	.read_event_value = ecap_iio_read_event_value,
>> +	.write_event_value = ecap_iio_write_event_value,
>> +};
>> +
>> +static const struct iio_event_spec ecap_iio_events[] = {
>> +	{
>> +		.type = IIO_EV_TYPE_CHANGE,
>> +		.dir = IIO_EV_DIR_FALLING,
>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
>> +	},
>> +};
>> +
>> +static const struct iio_chan_spec ecap_iio_channels[] = {
>> +	{
>> +		.scan_index = 0,
>> +		.type = IIO_INDEX,
>> +		.address = 0,
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_ENABLE),
>> +		.modified = 0,
> C will initialize anything not specificed to 0 anyway, so just add
> 0 entries if they aren't the natural default (i.e. enums etc).

OK

>
>> +		.event_spec = ecap_iio_events,
>> +		.num_event_specs = ARRAY_SIZE(ecap_iio_events),
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.endianness = IIO_LE,
>> +			.realbits = 2,
>> +			.storagebits = 8,
>> +			.shift = 0,
>> +		},
>> +	},
>> +	IIO_CHAN_SOFT_TIMESTAMP(1),
>> +};
>> +
>> +static irqreturn_t ecap_iio_isr(int irq, void *dev_id)
>> +{
>> +	struct iio_dev *indio_dev = dev_id;
>> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +	struct ecap_iio_data *ecap_data = &ecap_dev->data;
>> +	unsigned int clr = 0;
>> +	unsigned int flg;
>> +	unsigned int cap_time;
>> +	int i;
>> +
>> +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
>> +
>> +	for (i = 0 ; i < ECAP_NB_CEVT ; i++) {
>> +		if (flg & ECAP_CEVT_FLG_BIT(i)) {
>> +			if (i < ECAP_NB_CAP) {
>> +				/*
>> +				 * Input signal edge detected
>> +				 * time_ns = 10^9 * time_cycles / clk_rate
>> +				 */
>> +				ecap_data->ev_idx = i;
>> +				regmap_read(ecap_dev->regmap, ECAP_CAP_REG(i), &cap_time);
>> +				ecap_data->ev_time = cap_time * NSEC_PER_SEC;
>> +				do_div(ecap_data->ev_time, ecap_dev->clk_rate);
> Is there any attempt to align that timestamp with the iio clock used for software timestamps?

No, this is not my goal. I just need to log hardware timestamp.
But I am not sure that I fully understand what you mean (?).

>> +			} else {
>> +				/* Counter overflow */
>> +				ecap_data->ev_idx = ECAP_OVF_VAL;
>> +				ecap_data->ev_time = 0;
> Don't push it if you've lost data.

This is not a lost data.
That was intentional (equivalent in counter subsystem would be 
COUNTER_EVENT_OVERFLOW).
For a long duration signal, user will be aware that counter overflow 
occurred (this can avoid misleading
consecutive timestamp interpretations).
Do you confirm that I should not push it ? Or maybe just explaining this 
overflow better ?

>
>> +			}
>> +			iio_push_to_buffers(indio_dev, ecap_data);
>> +
>> +			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_iio_clk_disable(void *clk)
>> +{
>> +	clk_disable_unprepare(clk);
>> +}
>> +
>> +static int ecap_iio_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct ecap_iio_dev *ecap_dev;
>> +	struct iio_dev *indio_dev;
>> +	void __iomem *mmio_base;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*ecap_dev));
>> +	if (IS_ERR(indio_dev))
>> +		return PTR_ERR(indio_dev);
>> +
>> +	ecap_dev = iio_priv(indio_dev);
>> +
>> +	ecap_dev->clk = devm_clk_get(dev, "fck");
>> +	if (IS_ERR(ecap_dev->clk)) {
>> +		dev_err(dev, "failed to get clock\n");
>> +		return PTR_ERR(ecap_dev->clk);
> Preference in probe() for using dev_err_probe() throughout.
> It will handle any deferred probes that show up + does nicer message printing.

OK

>
>> +	}
>> +
>> +	ret = clk_prepare_enable(ecap_dev->clk);
> There is a continuing effort to add devm support for clock management.
> Not sure if it will land this cycle, but if it does please update to use
> that.
> https://lore.kernel.org/all/20220520075737.758761-1-u.kleine-koenig@pengutronix.de/

OK, I've seen the discussion about it.

>
>> +	if (ret) {
>> +		dev_err(dev, "failed to enable clock\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_add_action_or_reset(dev, ecap_iio_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;
>> +	}
>> +
>> +	if (prescaler > ECAP_PS_MAX_VAL) {
>> +		prescaler = ECAP_PS_MAX_VAL;
>> +		dev_warn(dev, "prescaler out of range, forced to %d\n", prescaler);
>> +	}
>> +
>> +	mmio_base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(mmio_base))
>> +		return PTR_ERR(mmio_base);
>> +
>> +	ecap_dev->regmap = regmap_init_mmio(dev, mmio_base, &ecap_iio_regmap_config);
> Mixing devm and non devm just makes for possible race conditions.
> devm_regmap_init_mmio() should be fine here. If it's not I'd expect to see clear
> comments saying why not.

OK

>
>> +	if (IS_ERR(ecap_dev->regmap)) {
>> +		dev_err(dev, "failed to init regmap\n");
>> +		return PTR_ERR(ecap_dev->regmap);
>> +	}
>> +
>> +	indio_dev->name = devm_kasprintf(dev, GFP_KERNEL,
>> +					 "ecap-iio-%p", mmio_base);
> The name is a part number only, not instances specific.  The instance can usually
> be easily established from other available info.  If you need to provide more information
> there is a label attribute in the ABI (normally used for things like 'where' the sensor
> is, but could be used for this as well).

OK

>
>> +	indio_dev->info = &ecap_iio_info;
>> +	indio_dev->channels = ecap_iio_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(ecap_iio_channels);
>> +	indio_dev->modes = INDIO_BUFFER_SOFTWARE;
>> +
>> +	ret = devm_iio_kfifo_buffer_setup_ext(dev, indio_dev, NULL, NULL);
> Might as well use the non _ext() variant that is just a wrapper around this
> with one fewer parameter.

OK

>
>> +	if (ret) {
>> +		dev_err(dev, "failed to setup iio buffer\n");
>> +		return ret;
>> +	}
>> +
>> +	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_iio_isr, 0, pdev->name, indio_dev);
>> +	if (ret) {
>> +		dev_err(dev, "failed to request irq\n");
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, indio_dev);
>> +	pm_runtime_enable(&pdev->dev);
>> +
>> +	ecap_dev->enabled = 0;
> Will be set by default anyway as iio_priv() is kzalloc'd but
> fine to leave it here if you think it adds documentation.

OK

>
>> +	ecap_iio_capture_set_evmode(indio_dev, 0);
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>> +}
>> +
>> +static int ecap_iio_remove(struct platform_device *pdev)
>> +{
>> +	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +
>> +	if (ecap_dev->enabled)
>> +		ecap_iio_capture_disable(indio_dev);
>> +
>> +	regmap_exit(ecap_dev->regmap);
>> +
>> +	pm_runtime_disable(&pdev->dev);
> Order in remove should be reverse of probe() unless there is really
> good reason why not.  Otherwise there tend to be race conditions
> hiding.

OK

>
>> +
>> +	return 0;
>> +}
>> +
>> +static __maybe_unused int ecap_iio_suspend(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +
>> +	/* If eCAP is running, stop capture then save timestamp counter */
>> +	if (ecap_dev->enabled) {
>> +		ecap_iio_capture_disable(indio_dev);
>> +
>> +		pm_runtime_get_sync(indio_dev->dev.parent);
>> +		regmap_read(ecap_dev->regmap, ECAP_TSCNT_REG, &ecap_dev->pm_ctx.time_cntr);
>> +		pm_runtime_put_sync(indio_dev->dev.parent);
>> +	}
>> +
>> +	ecap_dev->pm_ctx.ev_mode = ecap_iio_capture_get_evmode(indio_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static __maybe_unused int ecap_iio_resume(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
>> +
>> +	ecap_iio_capture_set_evmode(indio_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(indio_dev->dev.parent);
>> +		regmap_write(ecap_dev->regmap, ECAP_TSCNT_REG, ecap_dev->pm_ctx.time_cntr);
>> +		pm_runtime_put_sync(indio_dev->dev.parent);
>> +
>> +		ecap_iio_capture_enable(indio_dev, false);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(ecap_iio_pm_ops, ecap_iio_suspend, ecap_iio_resume);
> DEFINE_SIMPLE_DEV_PM_OPS() as per docs in pm.h which mark this version as deprecated.

OK

>
>> +
>> +static const struct of_device_id ecap_iio_of_match[] = {
>> +	{ .compatible	= "ti,am62-ecap-capture" },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ecap_iio_of_match);
>> +
>> +static struct platform_driver ecap_iio_driver = {
>> +	.probe = ecap_iio_probe,
>> +	.remove = ecap_iio_remove,
>> +	.driver = {
>> +		.name = "ecap-capture",
>> +		.of_match_table = of_match_ptr(ecap_iio_of_match),
> Drop the of_match_ptr() protection.  It provides no real advantages and breaks
> some non dt firmwares.

OK

>
>> +		.pm = &ecap_iio_pm_ops,
> pm_sleep_ptr() and remove the __maybe_unused markings above as this new set
> of pm macros lets the compiler remove the code without the need for that trickery.

OK

>   
>> +	},
>> +};
>> +module_platform_driver(ecap_iio_driver);
>> +
>> +MODULE_DESCRIPTION("ECAP Capture driver");
>> +MODULE_AUTHOR("Julien Panis <jpanis@baylibre.com>");
>> +MODULE_LICENSE("GPL");

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Julien Panis]

On 01/08/2022 16:08, Julien Panis wrote:
>
>
> On 31/07/2022 17:41, Jonathan Cameron wrote:
>> On Thu, 28 Jul 2022 19:51: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,
>>
>> So this isn't the first ecap driver we've had proposed, but the previous
>> one was a few years ago and never reached v2.
>> https://lore.kernel.org/all/20200818153614.6438-1-dts86@cornell.edu/
>>
>> Honestly I can't remember much about it, but maybe the discussion around
>> that will be worth a reread.
>
> Hi Jonathan, thank you for your review.
>
> I read the discussion about previous attempt, before submitting this 
> patch. There were
> interesting comments indeed.
>
> But in this previous attempt, only one-shot pulses were handled 
> (moreover, global IRQ flag
> was not cleared, so I'm not sure that IRQ could be raised more than 
> once).
>
> However, ECAP can be used to make time measurements for any type of 
> "square waveform".
> That's why I tried to make this event mode configurable. Besides, 
> using a continuous mode allows
> handling much more signal types (not only single pulses).
>
>>
>> The use of ABI here is unusual. So I'd definitely like to see some 
>> documentation
>> probably as a file in the main kernel documentation to explain what 
>> the interface
>> is an how that relates to what is being captured.
>
> OK, I will add some userspace documentation.
>
>>
>> First thing to note here is the channel type of IIO_INDEX is now not 
>> actually
>> used any more because we moved all the relevant drivers over to the 
>> counter
>> subsystem (and we failed to mark it deprecated).
>
> I evaluated this counter subsystem before starting development. 
> Counting events is not "a priori"
> the goal when using ECAP.
>
> Nevertheless, maybe "counter_push_event" function could do the job. If 
> I use counter API :
> # Option 1 : CAP1/2/3/4 registers could be seen as 4 channels of the 
> same counter...
> but there are not channels, there are just sequential timestamps 
> actually. So I'm afraid this leads
> to misunderstanding for the user.
> Moreover, the user will have to read several entries (counts 1/2/3/4) 
> to gather timestamps from
> the same input signal, which is not very convenient.
> # Option 2 : Either CAP 1/2/3/4 events could be gathered in a single 
> channel...but then it will not
> be possible to configure their polarity (rising/falling edge) 
> individually (unless I did
> not understand well counter framework documentation).
>
> So, even with counter framework, it will lead to some diverted use of 
> the framwork, since ECAP
> is a very specific hardware that do not fit 100% counter philosophy.
>
> I admit that ECAP do not fit 100% IIO philosophy either.
>
> Maybe misc API would be more relevant actually. Any opinion about it 
> will be welcome. :-)

[Answering my own mail]

I got a closer look at counter framework. It is not suitable at all for 
ECAP. Initially, I thought that
"counter_push_event" function could be used, but the only timestamp 
handled by this function
is a software timestamp. I strongly doubt that counter framework 
maintainer would accept
some modification here to support hardware timestamp : a patch rejection 
would be
legitimate, since a counter is dedicated to "event counting". Whereas 
ECAP is dedicated to
"event timestamping".

Beside, ECAP has 4 timestamp registers but they are used to capture 
timestamps for a
single input pin (only 1 channel). In ECAP context, 'index X" is used to 
identify CAP X
(used to capture event X detected on a single pin, with X = 0/1/2/3/0...).
In counter framework, "index X" is used to identify channel X (among 
several pins).
So, the word "index" has not the same meaning in counter framework than 
in ECAP device.
Somehow, this ECAP index (0/1/2/3 for CAP1/2/3/4 registers) must be 
logged with timestamp
because it is an important part of signal info for the user (raw 
consecutive timestamps
are not enough).

So, here is my proposal for my next version :
(1) Replace IIO_INDEX by IIO_COUNT channel (already used in 
"stm32-timer-trigger.c" driver)
# In ECAP documentation, the word "index" is not used. The word used to 
speak about this
0->1->2->3->0 sequenced counter is "Mod4 counter".
(2) Configure event mode with 4 sysfs entries (to remove the mix of 
buffers and events interfaces)
# User will see 4 files (1 file for each CAP timestamp) named 
"falling_edge_active_0/1/2/3".
Writing 1 will select falling edge/ Writing 0 will select rising edge.

Would it be an acceptable alternative for you, Jonathan ? Would either 
(1) and/or (2) be a "no-go" ?

>
>>
>> Anyhow, I've reviewed below, but need docs to discuss this in depth.  
>> In particular
>> the mix of buffers and events interfaces is unlikely to be an 
>> acceptable path
>> forwards.
>
> OK, I will consider alternatives.
>
>>
>> Jonathan

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Jonathan Cameron]

On Tue, 2 Aug 2022 15:28:09 +0200
Julien Panis <jpanis@baylibre.com> wrote:

> On 01/08/2022 16:08, Julien Panis wrote:
> >
> >
> > On 31/07/2022 17:41, Jonathan Cameron wrote:  
> >> On Thu, 28 Jul 2022 19:51: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,
> >>
> >> So this isn't the first ecap driver we've had proposed, but the previous
> >> one was a few years ago and never reached v2.
> >> https://lore.kernel.org/all/20200818153614.6438-1-dts86@cornell.edu/
> >>
> >> Honestly I can't remember much about it, but maybe the discussion around
> >> that will be worth a reread.  
> >
> > Hi Jonathan, thank you for your review.
> >
> > I read the discussion about previous attempt, before submitting this 
> > patch. There were
> > interesting comments indeed.
> >
> > But in this previous attempt, only one-shot pulses were handled 
> > (moreover, global IRQ flag
> > was not cleared, so I'm not sure that IRQ could be raised more than 
> > once).
> >
> > However, ECAP can be used to make time measurements for any type of 
> > "square waveform".
> > That's why I tried to make this event mode configurable. Besides, 
> > using a continuous mode allows
> > handling much more signal types (not only single pulses).
> >  
> >>
> >> The use of ABI here is unusual. So I'd definitely like to see some 
> >> documentation
> >> probably as a file in the main kernel documentation to explain what 
> >> the interface
> >> is an how that relates to what is being captured.  
> >
> > OK, I will add some userspace documentation.
> >  
> >>
> >> First thing to note here is the channel type of IIO_INDEX is now not 
> >> actually
> >> used any more because we moved all the relevant drivers over to the 
> >> counter
> >> subsystem (and we failed to mark it deprecated).  
> >
> > I evaluated this counter subsystem before starting development. 
> > Counting events is not "a priori"
> > the goal when using ECAP.
> >
> > Nevertheless, maybe "counter_push_event" function could do the job. If 
> > I use counter API :
> > # Option 1 : CAP1/2/3/4 registers could be seen as 4 channels of the 
> > same counter...
> > but there are not channels, there are just sequential timestamps 
> > actually. So I'm afraid this leads
> > to misunderstanding for the user.
> > Moreover, the user will have to read several entries (counts 1/2/3/4) 
> > to gather timestamps from
> > the same input signal, which is not very convenient.
> > # Option 2 : Either CAP 1/2/3/4 events could be gathered in a single 
> > channel...but then it will not
> > be possible to configure their polarity (rising/falling edge) 
> > individually (unless I did
> > not understand well counter framework documentation).
> >
> > So, even with counter framework, it will lead to some diverted use of 
> > the framwork, since ECAP
> > is a very specific hardware that do not fit 100% counter philosophy.
> >
> > I admit that ECAP do not fit 100% IIO philosophy either.
> >
> > Maybe misc API would be more relevant actually. Any opinion about it 
> > will be welcome. :-)  
> 
> [Answering my own mail]
> 
> I got a closer look at counter framework. It is not suitable at all for 
> ECAP. Initially, I thought that
> "counter_push_event" function could be used, but the only timestamp 
> handled by this function
> is a software timestamp. I strongly doubt that counter framework 
> maintainer would accept
> some modification here to support hardware timestamp : a patch rejection 
> would be
> legitimate, since a counter is dedicated to "event counting". Whereas 
> ECAP is dedicated to
> "event timestamping".
+CC William.  

one thing to note is that a hardware timestamp is just a freerunning
counter.  As such, you can capture it alongside other data in a similar
fashion to any mutliple counter acquisition (which is the same as we
will be doing with this in IIO anyway).

> 
> Beside, ECAP has 4 timestamp registers but they are used to capture 
> timestamps for a
> single input pin (only 1 channel). In ECAP context, 'index X" is used to 
> identify CAP X
> (used to capture event X detected on a single pin, with X = 0/1/2/3/0...).
> In counter framework, "index X" is used to identify channel X (among 
> several pins).
> So, the word "index" has not the same meaning in counter framework than 
> in ECAP device.
> Somehow, this ECAP index (0/1/2/3 for CAP1/2/3/4 registers) must be 
> logged with timestamp
> because it is an important part of signal info for the user (raw 
> consecutive timestamps
> are not enough).
> 
> So, here is my proposal for my next version :
> (1) Replace IIO_INDEX by IIO_COUNT channel (already used in 
> "stm32-timer-trigger.c" driver)

IIO_COUNT might work, but don't use that driver as a basis. It's a leftover
that was complex to clean up that no one has had the time to do yet.

We want to be careful that any meaning we assign to ABI via this driver
doesn't cause legacy problems.

> # In ECAP documentation, the word "index" is not used. The word used to 
> speak about this
> 0->1->2->3->0 sequenced counter is "Mod4 counter".
> (2) Configure event mode with 4 sysfs entries (to remove the mix of 
> buffers and events interfaces)
> # User will see 4 files (1 file for each CAP timestamp) named 
> "falling_edge_active_0/1/2/3".
> Writing 1 will select falling edge/ Writing 0 will select rising edge.

Naming should reflect the channel to which it applies. So add a prefix to
that. Maybe in_countX_falling_edge_active etc (I'm not sure how this
would fit together as a whole ABI however).

A few questions / comments that might help me get my head around this.

There isn't a direct association between a wire and a channel in IIO.
We have various derived channels.  It's more about measuring 'one thing'.

So we could treat these various signals as aspects of a given thing.
Perhaps simply as events (though the interface for those assumes a common
timestamp basis with the host OS).  In that case we'd have 4 events
and every time one fired we'd push an even across to userspace.

Or we treat them as 4 aspects in a 'scan'.  That means we only get
data once all enabled 'edges' are seen, as we push them all together
as 4 channels.  Those channels contain the 4 timestamps from one
sequence of 0,1,2,3.  Either could be abstract counts, or could be
4 timestamps.

I'm not really sure which approach would work out better.

Rule of thumb. IIO events are for asynchronous things like threshold crossing
detections.  Arguably your detections here are in that category, but because
they happen, 0,1,2,3,0,1,2,3 etc they also look like channel scans - where
we normally pretend they are simultaneous measurements of different signals.
They are rarely actually simultaneous as normally there is a mux involved
somewhere and they channels are sampled sequentially and when a scan is finished
the hardware may well immediately start on the next one.



> 
> Would it be an acceptable alternative for you, Jonathan ? Would either 
> (1) and/or (2) be a "no-go" ?

It may take us a few more iterations to come to a conclusion and even if
the counter subsystem is looking unlikely I'd still like William's input
on this even if it ends up in IIO.  He spent quite some time hammering
odd devices into IIO before there was a counter subsystem.

Thanks,

Jonathan

> 
> >  
> >>
> >> Anyhow, I've reviewed below, but need docs to discuss this in depth.  
> >> In particular
> >> the mix of buffers and events interfaces is unlikely to be an 
> >> acceptable path
> >> forwards.  
> >
> > OK, I will consider alternatives.
> >  
> >>
> >> Jonathan  
> 

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[William Breathitt Gray]

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

On Tue, Aug 02, 2022 at 03:28:09PM +0200, Julien Panis wrote:
> 
> 
> On 01/08/2022 16:08, Julien Panis wrote:
> > 
> > 
> > On 31/07/2022 17:41, Jonathan Cameron wrote:
> > > On Thu, 28 Jul 2022 19:51: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,
> > > 
> > > So this isn't the first ecap driver we've had proposed, but the previous
> > > one was a few years ago and never reached v2.
> > > https://lore.kernel.org/all/20200818153614.6438-1-dts86@cornell.edu/
> > > 
> > > Honestly I can't remember much about it, but maybe the discussion around
> > > that will be worth a reread.
> > 
> > Hi Jonathan, thank you for your review.
> > 
> > I read the discussion about previous attempt, before submitting this
> > patch. There were
> > interesting comments indeed.
> > 
> > But in this previous attempt, only one-shot pulses were handled
> > (moreover, global IRQ flag
> > was not cleared, so I'm not sure that IRQ could be raised more than
> > once).
> > 
> > However, ECAP can be used to make time measurements for any type of
> > "square waveform".
> > That's why I tried to make this event mode configurable. Besides, using
> > a continuous mode allows
> > handling much more signal types (not only single pulses).
> > 
> > > 
> > > The use of ABI here is unusual. So I'd definitely like to see some
> > > documentation
> > > probably as a file in the main kernel documentation to explain what
> > > the interface
> > > is an how that relates to what is being captured.
> > 
> > OK, I will add some userspace documentation.
> > 
> > > 
> > > First thing to note here is the channel type of IIO_INDEX is now not
> > > actually
> > > used any more because we moved all the relevant drivers over to the
> > > counter
> > > subsystem (and we failed to mark it deprecated).
> > 
> > I evaluated this counter subsystem before starting development. Counting
> > events is not "a priori"
> > the goal when using ECAP.
> > 
> > Nevertheless, maybe "counter_push_event" function could do the job. If I
> > use counter API :
> > # Option 1 : CAP1/2/3/4 registers could be seen as 4 channels of the
> > same counter...
> > but there are not channels, there are just sequential timestamps
> > actually. So I'm afraid this leads
> > to misunderstanding for the user.
> > Moreover, the user will have to read several entries (counts 1/2/3/4) to
> > gather timestamps from
> > the same input signal, which is not very convenient.
> > # Option 2 : Either CAP 1/2/3/4 events could be gathered in a single
> > channel...but then it will not
> > be possible to configure their polarity (rising/falling edge)
> > individually (unless I did
> > not understand well counter framework documentation).
> > 
> > So, even with counter framework, it will lead to some diverted use of
> > the framwork, since ECAP
> > is a very specific hardware that do not fit 100% counter philosophy.
> > 
> > I admit that ECAP do not fit 100% IIO philosophy either.
> > 
> > Maybe misc API would be more relevant actually. Any opinion about it
> > will be welcome. :-)
> 
> [Answering my own mail]
> 
> I got a closer look at counter framework. It is not suitable at all for
> ECAP. Initially, I thought that
> "counter_push_event" function could be used, but the only timestamp handled
> by this function
> is a software timestamp. I strongly doubt that counter framework maintainer
> would accept
> some modification here to support hardware timestamp : a patch rejection
> would be
> legitimate, since a counter is dedicated to "event counting". Whereas ECAP
> is dedicated to
> "event timestamping".
> 
> Beside, ECAP has 4 timestamp registers but they are used to capture
> timestamps for a
> single input pin (only 1 channel). In ECAP context, 'index X" is used to
> identify CAP X
> (used to capture event X detected on a single pin, with X = 0/1/2/3/0...).
> In counter framework, "index X" is used to identify channel X (among several
> pins).
> So, the word "index" has not the same meaning in counter framework than in
> ECAP device.
> Somehow, this ECAP index (0/1/2/3 for CAP1/2/3/4 registers) must be logged
> with timestamp
> because it is an important part of signal info for the user (raw consecutive
> timestamps
> are not enough).
> 
> So, here is my proposal for my next version :
> (1) Replace IIO_INDEX by IIO_COUNT channel (already used in
> "stm32-timer-trigger.c" driver)
> # In ECAP documentation, the word "index" is not used. The word used to
> speak about this
> 0->1->2->3->0 sequenced counter is "Mod4 counter".
> (2) Configure event mode with 4 sysfs entries (to remove the mix of buffers
> and events interfaces)
> # User will see 4 files (1 file for each CAP timestamp) named
> "falling_edge_active_0/1/2/3".
> Writing 1 will select falling edge/ Writing 0 will select rising edge.
> 
> Would it be an acceptable alternative for you, Jonathan ? Would either (1)
> and/or (2) be a "no-go" ?

Hi Julien,

I've taken a cursory look over the TI ECAP reference guide and your
descriptions in this thread. I think a device driver for this would fit
better in the Counter subsystem than IIO.

First I want to correct a minor misunderstanding: the "timestamp"
member of struct counter_event is simply a way to identify Counter
events on the system as a way of grouping multiple Counter watches. In
other words, the "timestamp" member here represents when a Counter event
was detected by the system, not when an event was logged on the counter
device hardware. Instead, hardware timestamps such as the CAPx registers
would be provided by the "value" member of struct counter_event.

Now, I have a few ideas for how we could expose the timestamps using a
Counter device driver, but first I want to make sure I understand
correctly what's happening in this device. If I understand correctly, we
have the following device components:

* CTR: 32-bit counter timer
* Mod4: 2-bit counter
* CAP1-CAP4: four 32-bit registers, each indepedently store a timestamp
* ECAP: input signal providing event trigger edges

Four edge polarities are configured corresponding to each CAPx register,
yet the input signal is still the same single ECAP pin. The event that
is fired is instead determined by the Mod4 counter: when Mod4 is 0 and
the edge of ECAP matches the polarity configured for CAP1 then an event
is triggered which saves the current CTR value to CAP1 and increments
Mod4 to 1, etc.

Is my understanding of how this device behaves correct?

If so, then one possible way to represent this device in the Counter
sysfs tree is something like this:

* CTR: /sys/bus/counter/devices/counterX/count0/count
* Mod4: /sys/bus/counter/devices/counterX/count1/count
* CAP1: /sys/bus/counter/devices/counterX/count1/cap1
* CAP2: /sys/bus/counter/devices/counterX/count1/cap2
* CAP3: /sys/bus/counter/devices/counterX/count1/cap3
* CAP4: /sys/bus/counter/devices/counterX/count1/cap4
* ECAP: /sys/bus/counter/devices/counterX/signal0/signal
* polarity1: /sys/bus/counter/devices/counterX/signal0/cap1_polarity
* polarity2: /sys/bus/counter/devices/counterX/signal0/cap2_polarity
* polarity3: /sys/bus/counter/devices/counterX/signal0/cap3_polarity
* polarity4: /sys/bus/counter/devices/counterX/signal0/cap4_polarity

This is just a tentative arrangement (you could also include "enable"
attributes as well), but it should give you an idea of how it could be
organized.

In your driver, you could then use counter_push_event() whenever you get
an event triggered. In userspace, your application will add Counter
watches for the CAPx registers they want. When an event triggers,
userspace can then received all four CAP register values at the same
time via the respective /dev/counterX character device node.

Would this design work for your needs?

William Breathitt Gray

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

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Julien Panis]

On 08/08/2022 02:30, William Breathitt Gray wrote:
> Hi Julien,
>
> I've taken a cursory look over the TI ECAP reference guide and your
> descriptions in this thread. I think a device driver for this would fit
> better in the Counter subsystem than IIO.
>
> First I want to correct a minor misunderstanding: the "timestamp"
> member of struct counter_event is simply a way to identify Counter
> events on the system as a way of grouping multiple Counter watches. In
> other words, the "timestamp" member here represents when a Counter event
> was detected by the system, not when an event was logged on the counter
> device hardware. Instead, hardware timestamps such as the CAPx registers
> would be provided by the "value" member of struct counter_event.
>
> Now, I have a few ideas for how we could expose the timestamps using a
> Counter device driver, but first I want to make sure I understand
> correctly what's happening in this device. If I understand correctly, we
> have the following device components:
>
> * CTR: 32-bit counter timer
> * Mod4: 2-bit counter
> * CAP1-CAP4: four 32-bit registers, each indepedently store a timestamp
> * ECAP: input signal providing event trigger edges
>
> Four edge polarities are configured corresponding to each CAPx register,
> yet the input signal is still the same single ECAP pin. The event that
> is fired is instead determined by the Mod4 counter: when Mod4 is 0 and
> the edge of ECAP matches the polarity configured for CAP1 then an event
> is triggered which saves the current CTR value to CAP1 and increments
> Mod4 to 1, etc.
>
> Is my understanding of how this device behaves correct?

Hi William. Thank you for your help.
Yes, your understanding of how this device behaves is correct.

>
> If so, then one possible way to represent this device in the Counter
> sysfs tree is something like this:
>
> * CTR: /sys/bus/counter/devices/counterX/count0/count
> * Mod4: /sys/bus/counter/devices/counterX/count1/count
> * CAP1: /sys/bus/counter/devices/counterX/count1/cap1
> * CAP2: /sys/bus/counter/devices/counterX/count1/cap2
> * CAP3: /sys/bus/counter/devices/counterX/count1/cap3
> * CAP4: /sys/bus/counter/devices/counterX/count1/cap4
> * ECAP: /sys/bus/counter/devices/counterX/signal0/signal
> * polarity1: /sys/bus/counter/devices/counterX/signal0/cap1_polarity
> * polarity2: /sys/bus/counter/devices/counterX/signal0/cap2_polarity
> * polarity3: /sys/bus/counter/devices/counterX/signal0/cap3_polarity
> * polarity4: /sys/bus/counter/devices/counterX/signal0/cap4_polarity
>
> This is just a tentative arrangement (you could also include "enable"
> attributes as well), but it should give you an idea of how it could be
> organized.
>
> In your driver, you could then use counter_push_event() whenever you get
> an event triggered. In userspace, your application will add Counter
> watches for the CAPx registers they want. When an event triggers,
> userspace can then received all four CAP register values at the same
> time via the respective /dev/counterX character device node.
>
> Would this design work for your needs?

Yes, that would work for my needs.
The "how" is not fully clear to me yet, since I never used counter 
subsystem. But the
best way to understand better how it works is probably to start working 
with it. :-)
So, next patch version will be based on counter subsystem.

>
> William Breathitt Gray

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[William Breathitt Gray]

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

On Mon, Aug 08, 2022 at 10:58:22AM +0200, Julien Panis wrote:
> On 08/08/2022 02:30, William Breathitt Gray wrote:
> > Hi Julien,
> > 
> > I've taken a cursory look over the TI ECAP reference guide and your
> > descriptions in this thread. I think a device driver for this would fit
> > better in the Counter subsystem than IIO.
> > 
> > First I want to correct a minor misunderstanding: the "timestamp"
> > member of struct counter_event is simply a way to identify Counter
> > events on the system as a way of grouping multiple Counter watches. In
> > other words, the "timestamp" member here represents when a Counter event
> > was detected by the system, not when an event was logged on the counter
> > device hardware. Instead, hardware timestamps such as the CAPx registers
> > would be provided by the "value" member of struct counter_event.
> > 
> > Now, I have a few ideas for how we could expose the timestamps using a
> > Counter device driver, but first I want to make sure I understand
> > correctly what's happening in this device. If I understand correctly, we
> > have the following device components:
> > 
> > * CTR: 32-bit counter timer
> > * Mod4: 2-bit counter
> > * CAP1-CAP4: four 32-bit registers, each indepedently store a timestamp
> > * ECAP: input signal providing event trigger edges
> > 
> > Four edge polarities are configured corresponding to each CAPx register,
> > yet the input signal is still the same single ECAP pin. The event that
> > is fired is instead determined by the Mod4 counter: when Mod4 is 0 and
> > the edge of ECAP matches the polarity configured for CAP1 then an event
> > is triggered which saves the current CTR value to CAP1 and increments
> > Mod4 to 1, etc.
> > 
> > Is my understanding of how this device behaves correct?
> 
> Hi William. Thank you for your help.
> Yes, your understanding of how this device behaves is correct.
> 
> > 
> > If so, then one possible way to represent this device in the Counter
> > sysfs tree is something like this:
> > 
> > * CTR: /sys/bus/counter/devices/counterX/count0/count
> > * Mod4: /sys/bus/counter/devices/counterX/count1/count
> > * CAP1: /sys/bus/counter/devices/counterX/count1/cap1
> > * CAP2: /sys/bus/counter/devices/counterX/count1/cap2
> > * CAP3: /sys/bus/counter/devices/counterX/count1/cap3
> > * CAP4: /sys/bus/counter/devices/counterX/count1/cap4
> > * ECAP: /sys/bus/counter/devices/counterX/signal0/signal
> > * polarity1: /sys/bus/counter/devices/counterX/signal0/cap1_polarity
> > * polarity2: /sys/bus/counter/devices/counterX/signal0/cap2_polarity
> > * polarity3: /sys/bus/counter/devices/counterX/signal0/cap3_polarity
> > * polarity4: /sys/bus/counter/devices/counterX/signal0/cap4_polarity
> > 
> > This is just a tentative arrangement (you could also include "enable"
> > attributes as well), but it should give you an idea of how it could be
> > organized.
> > 
> > In your driver, you could then use counter_push_event() whenever you get
> > an event triggered. In userspace, your application will add Counter
> > watches for the CAPx registers they want. When an event triggers,
> > userspace can then received all four CAP register values at the same
> > time via the respective /dev/counterX character device node.
> > 
> > Would this design work for your needs?
> 
> Yes, that would work for my needs.
> The "how" is not fully clear to me yet, since I never used counter
> subsystem. But the
> best way to understand better how it works is probably to start working with
> it. :-)
> So, next patch version will be based on counter subsystem.

The Counter subsystem is relatively nascent so the number of existing
Counter device drivers to study is unfortunately sparse. If you
encounter any trouble along the way as you work on this, please don't
hestitate to reach out and I'll be happy to answer any questions you may
have. That said, here are some more hints that can help guide you. :-)

Although we've been using CAPx to refer to these registers, in the
context of sysfs it'll be better to call the attributes "capture1",
"capture2", etc.; that will make their use as capture buffers more
obvious. Furthermore, despite my previous example, I think it's better
to have these exist underneath the CTR hierarchy rather than Mod4
because they are captures of the CTR value:

* CTR: /sys/bus/counter/devices/counterX/count0/count
* CAP1: /sys/bus/counter/devices/counterX/count0/capture1
* CAP2: /sys/bus/counter/devices/counterX/count0/capture2
* CAP3: /sys/bus/counter/devices/counterX/count0/capture3
* CAP4: /sys/bus/counter/devices/counterX/count0/capture4

In your device driver, you would define a struct counter_count to
represent CTR. In this struct counter_count there is an "ext" member
where you provide an array of struct count_comp. Each CAPx will have a
corresponding struct count_comp; it'll look something like this::

        static struct counter_comp ctr_count_ext[] = {
                COUNTER_COMP_COUNT_U64("capture1", cap1_read, NULL),
                COUNTER_COMP_COUNT_U64("capture2", cap2_read, NULL),
                COUNTER_COMP_COUNT_U64("capture3", cap3_read, NULL),
                COUNTER_COMP_COUNT_U64("capture4", cap4_read, NULL),
        };

As you already know, counter_push_event() lets you push Counter events
in your interrupt handler. I recommend introducing a new event type
under enum counter_event_type in the include/uapi/linux/counter.h header
file; something like COUNTER_EVENT_CAPTURE should be descriptive enough.

The "channel" member of struct counter_watch refers to Counter event
channels; The purpose here is to allow us to support concurrent events
of the same type (e.g. two COUNTER_EVENT_OVERFLOW but for different
Counts). If I understand the TI ECAP device correctly, we'll be getting
a COUNTER_EVENT_CAPTURE event whenever a CAPx register is updated with a
new capture. It's up to you if you want to group them under the same
channel or separate channels for each CAPx; you would just pass the
channel in counter_push_event() to indicate which COUNTER_EVENT_CAPTURE
event is being handled.

Finally, you can take a look at tools/counter/counter_example.c as an
example userspace application. The interesting bits for you are
COUNTER_ADD_WATCH_IOCTL/COUNTER_ENABLE_EVENTS_IOCTL ioctl calls and
reading the event data out of the queue. You will need to first define
an array of struct counter_watch indicating that you want to watch the
"capture" attributes of the CTR count; something like this (assuming
event channel 0)::

        /* assuming capture attributes are under the count0 directory */
        #define CAPTURE_WATCH(_id, _channel) \
        { \
                .component.type = COUNTER_COMPONENT_EXTENSION, \
                .component.scope = COUNTER_SCOPE_COUNT, \
                .component.parent = 0, \
                .component.id = _id, \
                .event = COUNTER_EVENT_CAPTURE, \
                .channel = _channel, \
        }
        /* get id from respective "captureX_component_id" attributes */
        static struct counter_watch watches[4] = {
                CAPTURE_WATCH(42, 0),
                CAPTURE_WATCH(43, 0),
                CAPTURE_WATCH(44, 0),
                CAPTURE_WATCH(45, 0),
        };

Later you add the watches, enable events, and finally read the event
data as it arrives::

	struct counter_event event_data[4];
        
        ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[0]);
        ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[1]);
        ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[2]);
        ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[3]);
	ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
        
	for (;;) {
	        read(fd, event_data, sizeof(event_data));
		printf("cap1: %llu", event_data[0].value);
		printf("cap2: %llu", event_data[1].value);
		printf("cap3: %llu", event_data[2].value);
		printf("cap4: %llu", event_data[3].value);
	}

If you want to keep track of channel, you can take a look under the
event_data[i].watch.channel member.

William Breathitt Gray

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

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Julien Panis]

On 08/08/2022 18:19, William Breathitt Gray wrote:
> On Mon, Aug 08, 2022 at 10:58:22AM +0200, Julien Panis wrote:
>> On 08/08/2022 02:30, William Breathitt Gray wrote:
>>> Hi Julien,
>>>
>>> I've taken a cursory look over the TI ECAP reference guide and your
>>> descriptions in this thread. I think a device driver for this would fit
>>> better in the Counter subsystem than IIO.
>>>
>>> First I want to correct a minor misunderstanding: the "timestamp"
>>> member of struct counter_event is simply a way to identify Counter
>>> events on the system as a way of grouping multiple Counter watches. In
>>> other words, the "timestamp" member here represents when a Counter event
>>> was detected by the system, not when an event was logged on the counter
>>> device hardware. Instead, hardware timestamps such as the CAPx registers
>>> would be provided by the "value" member of struct counter_event.
>>>
>>> Now, I have a few ideas for how we could expose the timestamps using a
>>> Counter device driver, but first I want to make sure I understand
>>> correctly what's happening in this device. If I understand correctly, we
>>> have the following device components:
>>>
>>> * CTR: 32-bit counter timer
>>> * Mod4: 2-bit counter
>>> * CAP1-CAP4: four 32-bit registers, each indepedently store a timestamp
>>> * ECAP: input signal providing event trigger edges
>>>
>>> Four edge polarities are configured corresponding to each CAPx register,
>>> yet the input signal is still the same single ECAP pin. The event that
>>> is fired is instead determined by the Mod4 counter: when Mod4 is 0 and
>>> the edge of ECAP matches the polarity configured for CAP1 then an event
>>> is triggered which saves the current CTR value to CAP1 and increments
>>> Mod4 to 1, etc.
>>>
>>> Is my understanding of how this device behaves correct?
>> Hi William. Thank you for your help.
>> Yes, your understanding of how this device behaves is correct.
>>
>>> If so, then one possible way to represent this device in the Counter
>>> sysfs tree is something like this:
>>>
>>> * CTR: /sys/bus/counter/devices/counterX/count0/count
>>> * Mod4: /sys/bus/counter/devices/counterX/count1/count
>>> * CAP1: /sys/bus/counter/devices/counterX/count1/cap1
>>> * CAP2: /sys/bus/counter/devices/counterX/count1/cap2
>>> * CAP3: /sys/bus/counter/devices/counterX/count1/cap3
>>> * CAP4: /sys/bus/counter/devices/counterX/count1/cap4
>>> * ECAP: /sys/bus/counter/devices/counterX/signal0/signal
>>> * polarity1: /sys/bus/counter/devices/counterX/signal0/cap1_polarity
>>> * polarity2: /sys/bus/counter/devices/counterX/signal0/cap2_polarity
>>> * polarity3: /sys/bus/counter/devices/counterX/signal0/cap3_polarity
>>> * polarity4: /sys/bus/counter/devices/counterX/signal0/cap4_polarity
>>>
>>> This is just a tentative arrangement (you could also include "enable"
>>> attributes as well), but it should give you an idea of how it could be
>>> organized.
>>>
>>> In your driver, you could then use counter_push_event() whenever you get
>>> an event triggered. In userspace, your application will add Counter
>>> watches for the CAPx registers they want. When an event triggers,
>>> userspace can then received all four CAP register values at the same
>>> time via the respective /dev/counterX character device node.
>>>
>>> Would this design work for your needs?
>> Yes, that would work for my needs.
>> The "how" is not fully clear to me yet, since I never used counter
>> subsystem. But the
>> best way to understand better how it works is probably to start working with
>> it. :-)
>> So, next patch version will be based on counter subsystem.
> The Counter subsystem is relatively nascent so the number of existing
> Counter device drivers to study is unfortunately sparse. If you
> encounter any trouble along the way as you work on this, please don't
> hestitate to reach out and I'll be happy to answer any questions you may
> have. That said, here are some more hints that can help guide you. :-)
>
> Although we've been using CAPx to refer to these registers, in the
> context of sysfs it'll be better to call the attributes "capture1",
> "capture2", etc.; that will make their use as capture buffers more
> obvious. Furthermore, despite my previous example, I think it's better
> to have these exist underneath the CTR hierarchy rather than Mod4
> because they are captures of the CTR value:
>
> * CTR: /sys/bus/counter/devices/counterX/count0/count
> * CAP1: /sys/bus/counter/devices/counterX/count0/capture1
> * CAP2: /sys/bus/counter/devices/counterX/count0/capture2
> * CAP3: /sys/bus/counter/devices/counterX/count0/capture3
> * CAP4: /sys/bus/counter/devices/counterX/count0/capture4
>
> In your device driver, you would define a struct counter_count to
> represent CTR. In this struct counter_count there is an "ext" member
> where you provide an array of struct count_comp. Each CAPx will have a
> corresponding struct count_comp; it'll look something like this::
>
>          static struct counter_comp ctr_count_ext[] = {
>                  COUNTER_COMP_COUNT_U64("capture1", cap1_read, NULL),
>                  COUNTER_COMP_COUNT_U64("capture2", cap2_read, NULL),
>                  COUNTER_COMP_COUNT_U64("capture3", cap3_read, NULL),
>                  COUNTER_COMP_COUNT_U64("capture4", cap4_read, NULL),
>          };
>
> As you already know, counter_push_event() lets you push Counter events
> in your interrupt handler. I recommend introducing a new event type
> under enum counter_event_type in the include/uapi/linux/counter.h header
> file; something like COUNTER_EVENT_CAPTURE should be descriptive enough.
>
> The "channel" member of struct counter_watch refers to Counter event
> channels; The purpose here is to allow us to support concurrent events
> of the same type (e.g. two COUNTER_EVENT_OVERFLOW but for different
> Counts). If I understand the TI ECAP device correctly, we'll be getting
> a COUNTER_EVENT_CAPTURE event whenever a CAPx register is updated with a
> new capture. It's up to you if you want to group them under the same
> channel or separate channels for each CAPx; you would just pass the
> channel in counter_push_event() to indicate which COUNTER_EVENT_CAPTURE
> event is being handled.

2 options, indeed :
1) By grouping them under the same channel, the only thing I'm not a 
great fan of is that
I will need to use separate functions to read capture registers 
(cap1_read/cap2_read/
cap3_read/cap4_read), and also to set/get polarity.
2) By using separate channels, code will look more simple but it is 
likely to be a little bit
confusing for the user.
I will probably use option 2.

> Finally, you can take a look at tools/counter/counter_example.c as an
> example userspace application. The interesting bits for you are
> COUNTER_ADD_WATCH_IOCTL/COUNTER_ENABLE_EVENTS_IOCTL ioctl calls and
> reading the event data out of the queue. You will need to first define
> an array of struct counter_watch indicating that you want to watch the
> "capture" attributes of the CTR count; something like this (assuming
> event channel 0)::
>
>          /* assuming capture attributes are under the count0 directory */
>          #define CAPTURE_WATCH(_id, _channel) \
>          { \
>                  .component.type = COUNTER_COMPONENT_EXTENSION, \
>                  .component.scope = COUNTER_SCOPE_COUNT, \
>                  .component.parent = 0, \
>                  .component.id = _id, \
>                  .event = COUNTER_EVENT_CAPTURE, \
>                  .channel = _channel, \
>          }
>          /* get id from respective "captureX_component_id" attributes */
>          static struct counter_watch watches[4] = {
>                  CAPTURE_WATCH(42, 0),
>                  CAPTURE_WATCH(43, 0),
>                  CAPTURE_WATCH(44, 0),
>                  CAPTURE_WATCH(45, 0),
>          };
>
> Later you add the watches, enable events, and finally read the event
> data as it arrives::
>
> 	struct counter_event event_data[4];
>          
>          ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[0]);
>          ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[1]);
>          ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[2]);
>          ioctl(fd, COUNTER_ADD_WATCH_IOCTL, &watches[3]);
> 	ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
>          
> 	for (;;) {
> 	        read(fd, event_data, sizeof(event_data));
> 		printf("cap1: %llu", event_data[0].value);
> 		printf("cap2: %llu", event_data[1].value);
> 		printf("cap3: %llu", event_data[2].value);
> 		printf("cap4: %llu", event_data[3].value);
> 	}
>
> If you want to keep track of channel, you can take a look under the
> event_data[i].watch.channel member.
>
> William Breathitt Gray

Thank you for all these explanations. After a few hours working with 
counter subsystem
I began to understand how it works, but now it's fully clear.

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Julien Panis]

On 09/08/2022 08:19, Julien Panis wrote:
>
>
> On 08/08/2022 18:19, William Breathitt Gray wrote:
>> The Counter subsystem is relatively nascent so the number of existing
>> Counter device drivers to study is unfortunately sparse. If you
>> encounter any trouble along the way as you work on this, please don't
>> hestitate to reach out and I'll be happy to answer any questions you may
>> have. That said, here are some more hints that can help guide you. :-)
>>
>> Although we've been using CAPx to refer to these registers, in the
>> context of sysfs it'll be better to call the attributes "capture1",
>> "capture2", etc.; that will make their use as capture buffers more
>> obvious. Furthermore, despite my previous example, I think it's better
>> to have these exist underneath the CTR hierarchy rather than Mod4
>> because they are captures of the CTR value:
>>
>> * CTR: /sys/bus/counter/devices/counterX/count0/count
>> * CAP1: /sys/bus/counter/devices/counterX/count0/capture1
>> * CAP2: /sys/bus/counter/devices/counterX/count0/capture2
>> * CAP3: /sys/bus/counter/devices/counterX/count0/capture3
>> * CAP4: /sys/bus/counter/devices/counterX/count0/capture4
>>
>> In your device driver, you would define a struct counter_count to
>> represent CTR. In this struct counter_count there is an "ext" member
>> where you provide an array of struct count_comp. Each CAPx will have a
>> corresponding struct count_comp; it'll look something like this::
>>
>>          static struct counter_comp ctr_count_ext[] = {
>>                  COUNTER_COMP_COUNT_U64("capture1", cap1_read, NULL),
>>                  COUNTER_COMP_COUNT_U64("capture2", cap2_read, NULL),
>>                  COUNTER_COMP_COUNT_U64("capture3", cap3_read, NULL),
>>                  COUNTER_COMP_COUNT_U64("capture4", cap4_read, NULL),
>>          };
>>
>> As you already know, counter_push_event() lets you push Counter events
>> in your interrupt handler. I recommend introducing a new event type
>> under enum counter_event_type in the include/uapi/linux/counter.h header
>> file; something like COUNTER_EVENT_CAPTURE should be descriptive enough.
>>
>> The "channel" member of struct counter_watch refers to Counter event
>> channels; The purpose here is to allow us to support concurrent events
>> of the same type (e.g. two COUNTER_EVENT_OVERFLOW but for different
>> Counts). If I understand the TI ECAP device correctly, we'll be getting
>> a COUNTER_EVENT_CAPTURE event whenever a CAPx register is updated with a
>> new capture. It's up to you if you want to group them under the same
>> channel or separate channels for each CAPx; you would just pass the
>> channel in counter_push_event() to indicate which COUNTER_EVENT_CAPTURE
>> event is being handled.
>
> 2 options, indeed :
> 1) By grouping them under the same channel, the only thing I'm not a 
> great fan of is that
> I will need to use separate functions to read capture registers 
> (cap1_read/cap2_read/
> cap3_read/cap4_read), and also to set/get polarity.
> 2) By using separate channels, code will look more simple but it is 
> likely to be a little bit
> confusing for the user.
> I will probably use option 2.

[ERRATUM] : Sorry, I meant that I will probably use option 1 (not 2), to 
avoid confusion for the user.

Re: [PATCH v3 2/2] iio: time: capture-tiecap: capture driver support for ECAP

[Jonathan Cameron]

On Mon, 1 Aug 2022 16:08:02 +0200
Julien Panis <jpanis@baylibre.com> wrote:

> On 31/07/2022 17:41, Jonathan Cameron wrote:
> > On Thu, 28 Jul 2022 19:51: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,
> >
> > So this isn't the first ecap driver we've had proposed, but the previous
> > one was a few years ago and never reached v2.
> > https://lore.kernel.org/all/20200818153614.6438-1-dts86@cornell.edu/
> >
> > Honestly I can't remember much about it, but maybe the discussion around
> > that will be worth a reread.  
> 
> Hi Jonathan, thank you for your review.
> 
> I read the discussion about previous attempt, before submitting this 
> patch. There were
> interesting comments indeed.
> 
> But in this previous attempt, only one-shot pulses were handled 
> (moreover, global IRQ flag
> was not cleared, so I'm not sure that IRQ could be raised more than once).

:)

> 
> However, ECAP can be used to make time measurements for any type of 
> "square waveform".
> That's why I tried to make this event mode configurable. Besides, using 
> a continuous mode allows
> handling much more signal types (not only single pulses).

Makes sense.

> 
> >
> > The use of ABI here is unusual. So I'd definitely like to see some documentation
> > probably as a file in the main kernel documentation to explain what the interface
> > is an how that relates to what is being captured.  
> 
> OK, I will add some userspace documentation.
> 
> >
> > First thing to note here is the channel type of IIO_INDEX is now not actually
> > used any more because we moved all the relevant drivers over to the counter
> > subsystem (and we failed to mark it deprecated).  
> 
> I evaluated this counter subsystem before starting development. Counting 
> events is not "a priori"
> the goal when using ECAP.
> 
> Nevertheless, maybe "counter_push_event" function could do the job. If I 
> use counter API :
> # Option 1 : CAP1/2/3/4 registers could be seen as 4 channels of the 
> same counter...
> but there are not channels, there are just sequential timestamps 
> actually. So I'm afraid this leads
> to misunderstanding for the user.
> Moreover, the user will have to read several entries (counts 1/2/3/4) to 
> gather timestamps from
> the same input signal, which is not very convenient.
> # Option 2 : Either CAP 1/2/3/4 events could be gathered in a single 
> channel...but then it will not
> be possible to configure their polarity (rising/falling edge) 
> individually (unless I did
> not understand well counter framework documentation).
> 
> So, even with counter framework, it will lead to some diverted use of 
> the framwork, since ECAP
> is a very specific hardware that do not fit 100% counter philosophy.
> 
> I admit that ECAP do not fit 100% IIO philosophy either.

Understood.  +CC William to see if we can form a consensus on how
at least which subsystem to hammer this into :)

> 
> Maybe misc API would be more relevant actually. Any opinion about it 
> will be welcome. :-)

It's possible we'll fall back to misc, but if at all possible it would
be better to make whatever extensions are necessary to a subsystem so that
other similar devices end up with a home in the long run.

Key here is definitely documentation!

> 
> >
> > Anyhow, I've reviewed below, but need docs to discuss this in depth.  In particular
> > the mix of buffers and events interfaces is unlikely to be an acceptable path
> > forwards.  
> 
> OK, I will consider alternatives.
> 
> >
> > Jonathan
A quick process comment.  If you agree with something in a review
no need to say that. Just delete the relevant code block from your reply.
Only need to discuss the other stuff!   Reviewers read a lot of email
so cutting down the volume makes them less grumpy :)

> >  
> >> + * struct ecap_iio_data - IIO device data
> >> + * @ev_idx:  event index (0 to 3 for CAP1 to CAP4)  
> > That's a long way from standard ABI.  
> 
> OK, I will consider alternatives.

We might to make new ABI though.   Need to discuss that against
a description rather than finding it buried deep in code.
One possibility is that we have an events only IIO device
as perhaps this fits better int he events interface (the
timestamp part is problematic though).
> 
> >  
> >> + * @ev_time: falling/rising edge timestamp
> >> + *

...

...

> >> +static irqreturn_t ecap_iio_isr(int irq, void *dev_id)
> >> +{
> >> +	struct iio_dev *indio_dev = dev_id;
> >> +	struct ecap_iio_dev *ecap_dev = iio_priv(indio_dev);
> >> +	struct ecap_iio_data *ecap_data = &ecap_dev->data;
> >> +	unsigned int clr = 0;
> >> +	unsigned int flg;
> >> +	unsigned int cap_time;
> >> +	int i;
> >> +
> >> +	regmap_read(ecap_dev->regmap, ECAP_ECINT_EN_FLG_REG, &flg);
> >> +
> >> +	for (i = 0 ; i < ECAP_NB_CEVT ; i++) {
> >> +		if (flg & ECAP_CEVT_FLG_BIT(i)) {
> >> +			if (i < ECAP_NB_CAP) {
> >> +				/*
> >> +				 * Input signal edge detected
> >> +				 * time_ns = 10^9 * time_cycles / clk_rate
> >> +				 */
> >> +				ecap_data->ev_idx = i;
> >> +				regmap_read(ecap_dev->regmap, ECAP_CAP_REG(i), &cap_time);
> >> +				ecap_data->ev_time = cap_time * NSEC_PER_SEC;
> >> +				do_div(ecap_data->ev_time, ecap_dev->clk_rate);  
> > Is there any attempt to align that timestamp with the iio clock used for software timestamps?  
> 
> No, this is not my goal. I just need to log hardware timestamp.
> But I am not sure that I fully understand what you mean (?).

IIO timestamps are meant to be useful for aligning multiple data sources. As such, where
we have hardware timestamps we normally attempt to apply a calibration to them so that they
map into the time base of the clocks used for IIO software timestamps.

If that's not possible then we need to think how to indicate to userspace that these
particular timestamps have a different meaning.

> 
> >> +			} else {
> >> +				/* Counter overflow */
> >> +				ecap_data->ev_idx = ECAP_OVF_VAL;
> >> +				ecap_data->ev_time = 0;  
> > Don't push it if you've lost data.  
> 
> This is not a lost data.

> That was intentional (equivalent in counter subsystem would be 
> COUNTER_EVENT_OVERFLOW).
> For a long duration signal, user will be aware that counter overflow 
> occurred (this can avoid misleading
> consecutive timestamp interpretations).
Handle that in software.  I assume the hardware is sensible enough to provide
a mechanism to notify you about a single overflow? (so that you don't get a
double overflow without knowing about it?)

If so, use a software counter of larger size and have that not overflow.
Typically a 64 bit counter will do fine for this for much longer than the
device is likely to be running (unless this thing has very high clock rates).

> Do you confirm that I should not push it ? Or maybe just explaining this 
> overflow better ?

It's not something userspace should ever need to know about.  We have
he equivalent for Performance Monitoring Units (drivers/perf)
There devices generally provide an overflow interrupt to allow us to
keep track of how many overflows have occurred and hence out a timestamp
that doesn't overflow often enough to matter.

> 
> >  
> >> +			}
> >> +			iio_push_to_buffers(indio_dev, ecap_data);
> >> +
> >> +			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;
> >> +}


Thanks,

Jonathan