[RFC/PATCH] counter: introduce support for Intel QEP Encoder

[RFC/PATCH] counter: introduce support for Intel QEP Encoder

[Felipe Balbi]

Add support for Intel PSE Quadrature Encoder

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---

Hi William,

Here's a first cut of the ported driver. Note that this is really an RFC as
there's still quite a bit I don't fully understand. I've left some of my old
sysfs files in there just to keep track of which features I'm still missing.

If understood this correctly, I should add noise filtering as a signal
extension, right? Same for Input Swap.

How should we handle the reset mode of the device? And mode of operation?

If you have any ideas, let me know.

cheers

 drivers/counter/Kconfig     |   6 +
 drivers/counter/Makefile    |   1 +
 drivers/counter/intel-qep.c | 874 ++++++++++++++++++++++++++++++++++++
 3 files changed, 881 insertions(+)
 create mode 100644 drivers/counter/intel-qep.c

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2967d0a9ff91..f280cd721350 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -59,4 +59,10 @@ config FTM_QUADDEC
 	  To compile this driver as a module, choose M here: the
 	  module will be called ftm-quaddec.
 
+config INTEL_QEP
+	tristate "Intel Quadrature Encoder"
+	depends on PCI
+	help
+	  Support for Intel Quadrature Encoder Devices
+
 endif # COUNTER
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 40d35522937d..cf291cfd8cf0 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
 obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
+obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
new file mode 100644
index 000000000000..5b208362ab73
--- /dev/null
+++ b/drivers/counter/intel-qep.c
@@ -0,0 +1,874 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * intel-qep.c - Intel Quadrature Encoder Driver
+ *
+ * Copyright (C) 2019 Intel Corporation - https://www.intel.com
+ *
+ * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
+ */
+#include <linux/bitops.h>
+#include <linux/counter.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/sysfs.h>
+
+#define INTEL_QEPCON		0x00
+#define INTEL_QEPFLT		0x04
+#define INTEL_QEPCOUNT		0x08
+#define INTEL_QEPMAX		0x0c
+#define INTEL_QEPWDT		0x10
+#define INTEL_QEPCAPDIV		0x14
+#define INTEL_QEPCNTR		0x18
+#define INTEL_QEPCAPBUF		0x1c
+#define INTEL_QEPINT_STAT	0x20
+#define INTEL_QEPINT_MASK	0x24
+
+/* QEPCON */
+#define INTEL_QEPCON_EN		BIT(0)
+#define INTEL_QEPCON_FLT_EN	BIT(1)
+#define INTEL_QEPCON_EDGE_A	BIT(2)
+#define INTEL_QEPCON_EDGE_B	BIT(3)
+#define INTEL_QEPCON_EDGE_INDX	BIT(4)
+#define INTEL_QEPCON_SWPAB	BIT(5)
+#define INTEL_QEPCON_OP_MODE	BIT(6)
+#define INTEL_QEPCON_PH_ERR	BIT(7)
+#define INTEL_QEPCON_COUNT_RST_MODE BIT(8)
+#define INTEL_QEPCON_INDX_GATING_MASK GENMASK(10, 9)
+#define INTEL_QEPCON_INDX_GATING(n) (((n) & 3) << 9)
+#define INTEL_QEPCON_INDX_PAL_PBL INTEL_QEPCON_INDX_GATING(0)
+#define INTEL_QEPCON_INDX_PAL_PBH INTEL_QEPCON_INDX_GATING(1)
+#define INTEL_QEPCON_INDX_PAH_PBL INTEL_QEPCON_INDX_GATING(2)
+#define INTEL_QEPCON_INDX_PAH_PBH INTEL_QEPCON_INDX_GATING(3)
+#define INTEL_QEPCON_CAP_MODE	BIT(11)
+#define INTEL_QEPCON_FIFO_THRE_MASK GENMASK(14, 12)
+#define INTEL_QEPCON_FIFO_THRE(n) ((((n) - 1) & 7) << 12)
+#define INTEL_QEPCON_FIFO_EMPTY	BIT(15)
+
+/* QEPFLT */
+#define INTEL_QEPFLT_MAX_COUNT(n) ((n) & 0x1fffff)
+
+/* QEPINT */
+#define INTEL_QEPINT_FIFOCRIT	BIT(5)
+#define INTEL_QEPINT_FIFOENTRY	BIT(4)
+#define INTEL_QEPINT_QEPDIR	BIT(3)
+#define INTEL_QEPINT_QEPRST_UP	BIT(2)
+#define INTEL_QEPINT_QEPRST_DOWN BIT(1)
+#define INTEL_QEPINT_WDT	BIT(0)
+
+#define INTEL_QEP_DIRECTION_UP 0
+#define INTEL_QEP_DIRECTION_DOWN 1
+
+struct intel_qep {
+	struct counter_device counter;
+	struct mutex lock;
+	struct pci_dev *pci;
+	struct device *dev;
+	void __iomem *regs;
+	u32 interrupt;
+	int direction;
+	bool enabled;
+};
+
+#define counter_to_qep(c)	(container_of((c), struct intel_qep, counter))
+
+static inline u32 intel_qep_readl(void __iomem *base, u32 offset)
+{
+	return readl(base + offset);
+}
+
+static inline void intel_qep_writel(void __iomem *base, u32 offset, u32 value)
+{
+	writel(value, base + offset);
+}
+
+static ssize_t reset_mode_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_COUNT_RST_MODE ?
+			"maximum" : "index");
+}
+
+static ssize_t reset_mode_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	if (qep->enabled)
+		return -EINVAL;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (sysfs_streq(buf, "maximum"))
+		reg |= INTEL_QEPCON_COUNT_RST_MODE;
+	else if (sysfs_streq(buf, "index"))
+		reg &= ~INTEL_QEPCON_COUNT_RST_MODE;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return count;
+}
+static DEVICE_ATTR_RW(reset_mode);
+
+static ssize_t state_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			qep->enabled ? "enabled" : "disabled");
+}
+
+static ssize_t state_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	pm_runtime_get_sync(dev);
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (sysfs_streq(buf, "enable")) {
+		reg |= INTEL_QEPCON_EN;
+		qep->enabled = true;
+
+	} else if (sysfs_streq(buf, "disable")) {
+		reg &= ~INTEL_QEPCON_EN;
+		qep->enabled = false;
+
+		pm_runtime_put(dev);
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_put_autosuspend(dev);
+	}
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return count;
+}
+static DEVICE_ATTR_RW(state);
+
+static ssize_t current_position_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCOUNT);
+
+	return snprintf(buf, PAGE_SIZE, "%08x\n", reg);
+}
+static DEVICE_ATTR_RO(current_position);
+
+static ssize_t max_position_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPMAX);
+
+	return snprintf(buf, PAGE_SIZE, "%08x\n", reg);
+}
+
+static ssize_t max_position_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 max;
+	int ret;
+
+	if (qep->enabled)
+		return -EINVAL;
+
+	ret = kstrtou32(buf, 0, &max);
+	if (ret < 0)
+		return ret;
+
+	intel_qep_writel(qep->regs, INTEL_QEPMAX, max);
+
+	return count;
+}
+static DEVICE_ATTR_RW(max_position);
+
+static ssize_t filter_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_FLT_EN ? "enabled" : "disabled");
+}
+
+static ssize_t filter_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	if (qep->enabled)
+		return -EINVAL;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (sysfs_streq(buf, "enable"))
+		reg |= INTEL_QEPCON_FLT_EN;
+	else if (sysfs_streq(buf, "disable"))
+		reg &= ~INTEL_QEPCON_FLT_EN;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return count;
+}
+static DEVICE_ATTR_RW(filter);
+
+static ssize_t filter_count_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPFLT);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", INTEL_QEPFLT_MAX_COUNT(reg));
+}
+
+static ssize_t filter_count_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 max;
+	int ret;
+
+	if (qep->enabled)
+		return -EINVAL;
+
+	ret = kstrtou32(buf, 0, &max);
+	if (ret < 0)
+		return ret;
+
+	if (max > 0x1fffff)
+		max = 0x1ffff;
+
+	intel_qep_writel(qep->regs, INTEL_QEPFLT, max);
+
+	return count;
+}
+static DEVICE_ATTR_RW(filter_count);
+
+static ssize_t swap_inputs_show(struct device *dev, struct device_attribute *attr,
+		char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_SWPAB ? "swapped" : "normal");
+}
+
+static ssize_t swap_inputs_store(struct device *dev, struct device_attribute *attr,
+		const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	if (qep->enabled)
+		return -EINVAL;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (sysfs_streq(buf, "swap"))
+		reg |= INTEL_QEPCON_SWPAB;
+	else if (sysfs_streq(buf, "normal"))
+		reg &= ~INTEL_QEPCON_SWPAB;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return count;
+}
+static DEVICE_ATTR_RW(swap_inputs);
+
+static ssize_t phase_error_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_PH_ERR ? "error" : "okay");
+}
+static DEVICE_ATTR_RO(phase_error);
+
+static ssize_t direction_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", qep->direction ? "down" : "up");
+}
+static DEVICE_ATTR_RO(direction);
+
+static ssize_t fifo_empty_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_FIFO_EMPTY ? "empty" : "not empty");
+}
+static DEVICE_ATTR_RO(fifo_empty);
+
+static ssize_t operating_mode_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_OP_MODE ? "capture" : "quadrature");
+}
+
+static ssize_t operating_mode_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	if (qep->enabled)
+		return -EINVAL;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (sysfs_streq(buf, "capture"))
+		reg |= INTEL_QEPCON_OP_MODE;
+	else if (sysfs_streq(buf, "quadrature"))
+		reg &= ~INTEL_QEPCON_OP_MODE;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return count;
+}
+static DEVICE_ATTR_RW(operating_mode);
+
+static ssize_t capture_mode_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_CAP_MODE ? "both" : "single");
+}
+
+static ssize_t capture_mode_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	if (qep->enabled)
+		return -EINVAL;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (sysfs_streq(buf, "both"))
+		reg |= INTEL_QEPCON_CAP_MODE;
+	else if (sysfs_streq(buf, "single"))
+		reg &= ~INTEL_QEPCON_CAP_MODE;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return count;
+}
+static DEVICE_ATTR_RW(capture_mode);
+
+static const struct attribute *intel_qep_attrs[] = {
+	&dev_attr_capture_mode.attr,
+	&dev_attr_current_position.attr,
+	&dev_attr_direction.attr,
+	&dev_attr_fifo_empty.attr,
+	&dev_attr_filter.attr,
+	&dev_attr_filter_count.attr,
+	&dev_attr_max_position.attr,
+	&dev_attr_operating_mode.attr,
+	&dev_attr_phase_error.attr,
+	&dev_attr_reset_mode.attr,
+	&dev_attr_state.attr,
+	&dev_attr_swap_inputs.attr,
+	NULL	/* Terminating Entry */
+};
+
+static ssize_t capture_data_read(struct file *filp, struct kobject *kobj,
+		struct bin_attribute *attr, char *buf,
+		loff_t offset, size_t count)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+	int i;
+
+	mutex_lock(&qep->lock);
+	for (i = 0; i < count; i += 4) {
+		reg = intel_qep_readl(qep->regs, INTEL_QEPCAPBUF);
+
+		buf[i + 0] = reg & 0xff;
+		buf[i + 1] = (reg >> 8) & 0xff;
+		buf[i + 2] = (reg >> 16) & 0xff;
+		buf[i + 3] = (reg >> 24) & 0xff;
+	}
+	mutex_unlock(&qep->lock);
+
+	return count;
+}
+
+static BIN_ATTR_RO(capture_data, 4);
+
+static struct bin_attribute *intel_qep_bin_attrs[] = {
+	&bin_attr_capture_data,
+	NULL	/* Terminating Entry */
+};
+
+static const struct attribute_group intel_qep_device_attr_group = {
+	.name = "qep",
+	.attrs = (struct attribute **) intel_qep_attrs,
+	.bin_attrs = intel_qep_bin_attrs,
+};
+
+static const struct pci_device_id intel_qep_id_table[] = {
+	/* EHL */
+	{ PCI_VDEVICE(INTEL, 0x4bc3), },
+	{ PCI_VDEVICE(INTEL, 0x4b81), },
+	{ PCI_VDEVICE(INTEL, 0x4b82), },
+	{ PCI_VDEVICE(INTEL, 0x4b83), },
+	{  } /* Terminating Entry */
+};
+MODULE_DEVICE_TABLE(pci, intel_qep_id_table);
+
+static void intel_qep_init(struct intel_qep *qep, bool reset)
+{
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	reg &= ~INTEL_QEPCON_EN;
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	/* make sure periperal is disabled by reading one more time */
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (reset) {
+		reg &= ~(INTEL_QEPCON_OP_MODE | INTEL_QEPCON_FLT_EN);
+		reg |= INTEL_QEPCON_EDGE_A | INTEL_QEPCON_EDGE_B |
+			INTEL_QEPCON_EDGE_INDX | INTEL_QEPCON_COUNT_RST_MODE;
+	}
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	intel_qep_writel(qep->regs, INTEL_QEPWDT, 0x1000);
+	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x0);
+
+	qep->direction = INTEL_QEP_DIRECTION_UP;
+}
+
+static irqreturn_t intel_qep_irq_thread(int irq, void *_qep)
+{
+	struct intel_qep	*qep = _qep;
+	u32			stat;
+
+	mutex_lock(&qep->lock);
+
+	stat = qep->interrupt;
+	if (stat & INTEL_QEPINT_FIFOCRIT)
+		sysfs_notify(&qep->dev->kobj, "qep", "capture_buffer");
+
+	if (stat & INTEL_QEPINT_FIFOENTRY)
+		sysfs_notify(&qep->dev->kobj, "qep", "capture_buffer");
+
+	if (stat & INTEL_QEPINT_QEPDIR) {
+		qep->direction = !(qep->direction);
+		sysfs_notify(&qep->dev->kobj, "qep", "direction");
+	}
+
+	if (stat & INTEL_QEPINT_QEPRST_UP) {
+		qep->direction = INTEL_QEP_DIRECTION_UP;
+		sysfs_notify(&qep->dev->kobj, "qep", "direction");
+	}
+
+	if (stat & INTEL_QEPINT_QEPRST_DOWN) {
+		qep->direction = INTEL_QEP_DIRECTION_DOWN;
+		sysfs_notify(&qep->dev->kobj, "qep", "direction");
+	}
+
+	if (stat & INTEL_QEPINT_WDT)
+		dev_dbg(qep->dev, "Watchdog\n");
+
+	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x00);
+	mutex_unlock(&qep->lock);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t intel_qep_irq(int irq, void *_qep)
+{
+	struct intel_qep	*qep = _qep;
+	u32			stat;
+
+	stat = intel_qep_readl(qep->regs, INTEL_QEPINT_STAT);
+	if (stat) {
+		qep->interrupt = stat;
+		intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0xffffffff);
+		intel_qep_writel(qep->regs, INTEL_QEPINT_STAT, stat);
+		return IRQ_WAKE_THREAD;
+	}
+
+	return IRQ_HANDLED;
+}
+
+enum intel_qep_synapse_action {
+	INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE,
+	INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE,
+};
+
+static enum counter_synapse_action intel_qep_synapse_actions[] = {
+	[INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE] =
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+	
+	[INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE] =
+	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
+};
+
+enum intel_qep_count_function {
+	INTEL_QEP_ENCODER_MODE_1,
+};
+
+static const enum counter_count_function intel_qep_count_functions[] = {
+	[INTEL_QEP_ENCODER_MODE_1] =
+	COUNTER_COUNT_FUNCTION_QUADRATURE_X4
+};
+
+static int intel_qep_count_read(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_count_read_value *val)
+{
+	struct intel_qep *const qep = counter->priv;
+	uint32_t cntval;
+
+	cntval = intel_qep_readl(qep, INTEL_QEPCOUNT);
+	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cntval);
+
+	return 0;
+}
+
+static int intel_qep_count_write(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_count_write_value *val)
+{
+	struct intel_qep *const qep = counter->priv;
+	u32 cnt;
+	int err;
+
+	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
+	if (err)
+		return err;
+
+	intel_qep_writel(qep->regs, INTEL_QEPMAX, cnt);
+
+	return 0;
+}
+
+static int intel_qep_function_get(struct counter_device *counter,
+		struct counter_count *count, size_t *function)
+{
+	*function = INTEL_QEP_ENCODER_MODE_1;
+
+	return 0;
+}
+
+static int intel_qep_action_get(struct counter_device *counter,
+		struct counter_count *count, struct counter_synapse *synapse,
+		size_t *action)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	*action = reg & synapse->signal->id ?
+		INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE :
+		INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE;
+
+	return 0;
+}
+
+static int intel_qep_action_set(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_synapse *synapse, size_t action)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (action == INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE)
+		reg |= synapse->signal->id;
+	else
+		reg &= ~synapse->signal->id;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return 0;
+}
+
+static const struct counter_ops intel_qep_counter_ops = {
+	.count_read = intel_qep_count_read,
+	.count_write = intel_qep_count_write,
+	.function_get = intel_qep_function_get,
+
+	.action_get = intel_qep_action_get,
+	.action_set = intel_qep_action_set,
+};
+
+static struct counter_signal intel_qep_signals[] = {
+	{
+		.id = INTEL_QEPCON_EDGE_A,
+		.name = "Phase A",
+	},
+	{
+		.id = INTEL_QEPCON_EDGE_B,
+		.name = "Phase B",
+	},
+	{
+		.id = INTEL_QEPCON_EDGE_INDX,
+		.name = "Index",
+	},
+};
+
+static struct counter_synapse intel_qep_count_synapses[] = {
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[0],
+	},
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[1],
+	},
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[2],
+	},
+};
+
+static const char * const intel_qep_clock_dividers[] = {
+	"1", "2", "4", "8", "16", "32", "64", "128"
+};
+
+static int intel_qep_clock_divider_get(struct counter_device *counter,
+		struct counter_count *count, size_t *mode)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	
+	*mode = intel_qep_readl(qep->regs, INTEL_QEPCAPDIV);
+
+	return 0;
+}
+
+static int intel_qep_clock_divider_set(struct counter_device *counter,
+		struct counter_count *count, size_t mode)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+
+	intel_qep_writel(qep->regs, INTEL_QEPCAPDIV, ffs(mode) - 1);
+
+	return 0;
+}
+
+static struct counter_count_enum_ext intel_qep_clock_divider_enum = {
+	.items = intel_qep_clock_dividers,
+	.num_items = ARRAY_SIZE(intel_qep_clock_dividers),
+	.get = intel_qep_clock_divider_get,
+	.set = intel_qep_clock_divider_set,
+};
+
+static const struct counter_count_ext intel_qep_count_ext[] = {
+	COUNTER_COUNT_ENUM("clock_divider", &intel_qep_clock_divider_enum),
+	COUNTER_COUNT_ENUM_AVAILABLE("clock_divider",
+			&intel_qep_clock_divider_enum),
+};
+
+static struct counter_count intel_qep_counter_count = {
+	.id = 0,
+	.name = "Channel 1 Count",
+	.functions_list = intel_qep_count_functions,
+	.num_functions = ARRAY_SIZE(intel_qep_count_functions),
+	.synapses = intel_qep_count_synapses,
+	.num_synapses = ARRAY_SIZE(intel_qep_count_synapses),
+};
+
+static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	struct intel_qep	*qep;
+	struct device		*dev = &pci->dev;
+	void __iomem		*regs;
+	int			ret;
+	int			irq;
+
+	qep = devm_kzalloc(dev, sizeof(*qep), GFP_KERNEL);
+	if (!qep)
+		return -ENOMEM;
+
+	ret = pcim_enable_device(pci);
+	if (ret)
+		return ret;
+
+	pci_set_master(pci);
+
+	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
+	if (ret)
+		return ret;
+
+	regs = pcim_iomap_table(pci)[0];
+	if (!regs)
+		return -ENOMEM;
+
+	qep->pci = pci;
+	qep->dev = dev;
+	qep->regs = regs;
+	mutex_init(&qep->lock);
+
+	intel_qep_init(qep, true);
+	pci_set_drvdata(pci, qep);
+
+	qep->counter.name = pci_name(pci);
+	qep->counter.parent = dev;
+	qep->counter.ops = &intel_qep_counter_ops;
+	qep->counter.counts = &intel_qep_counter_count;
+	qep->counter.num_counts = 1;
+	qep->counter.signals = intel_qep_signals;
+	qep->counter.num_signals = ARRAY_SIZE(intel_qep_signals);
+	qep->counter.priv = qep;
+
+	ret = counter_register(&qep->counter);
+	if (ret)
+		return ret;
+
+	ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		goto err_irq_vectors;
+
+	irq = pci_irq_vector(pci, 0);
+	ret = devm_request_threaded_irq(&pci->dev, irq, intel_qep_irq,
+			intel_qep_irq_thread, IRQF_SHARED | IRQF_TRIGGER_RISING,
+			"intel-qep", qep);
+	if (ret)
+		goto err_irq;
+
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_allow(dev);
+
+	return 0;
+
+err_irq:
+	pci_free_irq_vectors(pci);
+
+err_irq_vectors:
+	counter_unregister(&qep->counter);
+
+	return ret;
+}
+
+static void intel_qep_remove(struct pci_dev *pci)
+{
+	struct intel_qep	*qep = pci_get_drvdata(pci);
+	struct device		*dev = &pci->dev;
+
+	pm_runtime_forbid(dev);
+	pm_runtime_get_noresume(dev);
+
+	pci_free_irq_vectors(pci);
+	counter_unregister(&qep->counter);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int intel_qep_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int intel_qep_resume(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct intel_qep *qep = pci_get_drvdata(pdev);
+
+	intel_qep_init(qep, false);
+
+	return 0;
+}
+
+static int intel_qep_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int intel_qep_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct intel_qep *qep = pci_get_drvdata(pdev);
+
+	intel_qep_init(qep, false);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops intel_qep_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(intel_qep_suspend,
+				intel_qep_resume)
+	SET_RUNTIME_PM_OPS(intel_qep_runtime_suspend, intel_qep_runtime_resume,
+				NULL)
+};
+
+static struct pci_driver intel_qep_driver = {
+	.name		= "intel-qep",
+	.id_table	= intel_qep_id_table,
+	.probe		= intel_qep_probe,
+	.remove		= intel_qep_remove,
+	.driver = {
+		.pm = &intel_qep_pm_ops,
+	}
+};
+
+module_pci_driver(intel_qep_driver);
+
+MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel Quadrature Encoder Driver");
-- 
2.23.0

Re: [RFC/PATCH] counter: introduce support for Intel QEP Encoder

[William Breathitt Gray]

On Mon, Sep 16, 2019 at 12:34:31PM +0300, Felipe Balbi wrote:
> Add support for Intel PSE Quadrature Encoder
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
> 
> Hi William,
> 
> Here's a first cut of the ported driver. Note that this is really an RFC as
> there's still quite a bit I don't fully understand. I've left some of my old
> sysfs files in there just to keep track of which features I'm still missing.

Hi Felipe,

That is understandable since this is an RFC; I expect in your final
version those sysfs files will be Counter subsystem extension
attributes.

For reference, the existing Counter subsystem attributes are documented
in the Documentation/ABI/testing/sysfs-bus-counter file. If you have any
new attributes specific to the Intel QEP driver, add them as a new
Documentation/ABI/testing/sysfs-bus-counter-intel-qep file.

> If understood this correctly, I should add noise filtering as a signal
> extension, right? Same for Input Swap.

Determining the type of extension to create is a matter of scope.

* Signal extensions are attributes which expose information/control
  specific to a Signal. These types of attributes will exist under a
  Signal's directory in sysfs.

  For example, if have an invert feature for a Signal, you can have a
  Signal extension called "invert" that toggles that feature:
  /sys/bus/counter/devices/counterX/signalY/invert

* Count extensions are attributes which expose information/control
  specific to a Count. These type of attributes will exist under a
  Count's directory in sysfs.

  For example, if you want to pause/unpause a Count from updating, you
  could have a Count extension called "enable" that toggles such:
  /sys/bus/counter/devices/counterX/countY/enable

* Device extensions are attributes which expose information/control
  non-specific to a particular Count or Signal. This is where you would
  put your global features or other miscellanous functionality.

  For example, if your device has an overtemp sensor, you could report
  the chip overheated via a device extension called "error_overtemp":
  /sys/bus/counter/devices/counterX/error_overtemp

I'm not very familiar with the Intel QEP features, so I'll need your
help clarifying some of it for me. I'm assuming "noise filtering" is
a feature you can enable per individual Signal (is this correct?). If
so, then it would be implemented as a Signal extension, maybe as a
/sys/bus/counter/devices/counterX/signalY/noise_filter_enable file.
Otherwise, it would be a device attribute if it enables noise filtering
over all Signals.

Is "Input Swap" swapping the Phase A and Phase B signal lines for each
Count? If so, this would be a Count extension if you can configure it
per Count; otherwise a device extension if it's globally enabled for all
Counts.

If you're still having trouble figuring out which type of extension to
use, give me a simple breakdown of the features you are trying to
support with these attributes and I should be able to specify which type
works best for each.
 
> How should we handle the reset mode of the device? And mode of operation?

Is "reset mode" specifying the condition that causes a Count's count to
be reset back to a value of 0? If so, I assume "index" means an active
level on the index line will reset the count; does "maximum" mean the
index line is ignored and the reset operation is simply activated once
the ceiling is reached? As well, I'm assuming this is a global
configuration for all counts.

If my assumptions are correct here, then this behavior can be exposed
by creating two device extensions: "preset" and "preset_enable"; these
should be based on the existing two same-named Count attributes (see the
Documentation/ABI/testing/sysfs-bus-counter file).

* /sys/bus/counter/devices/counterX/preset
  For the Intel QEP, this will be a read-only that always report "0".

* /sys/bus/counter/devices/counterX/preset_enable
  Assuming "maximum" is just equivalent to ignoring index, preset_enable
  can toggle between the two modes as a simple boolean: "1" = "index"
  and "0" = "maximum".

  If I assumed incorrectly, please let me know and we can discuss the
  possibility of a new attribute (perhaps "preset_mode").

Take a look inside 104-quad-8.c to see how it handles index lines and
reseting the counts; "preset" and "preset_enable" extensions are
specified in the quad8_count_ext array.

> If you have any ideas, let me know.
> 
> cheers

Since I expect a good amount of this code to change once the extensions
are added, I'll hold off on a more in-depth code review until we get it
closer to what it'll actually look like. From skimming the code in this
RFC, it seems like you have the core Counter subsystem functions and
structures use correct, so I think it'll be the implementations of the
extensions that we'll be focusing on.

William Breathitt Gray

Re: [RFC/PATCH] counter: introduce support for Intel QEP Encoder

[Felipe Balbi]

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


Hi William,

William Breathitt Gray <vilhelm.gray@gmail.com> writes:

> On Mon, Sep 16, 2019 at 12:34:31PM +0300, Felipe Balbi wrote:
>> Add support for Intel PSE Quadrature Encoder
>> 
>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> ---
>> 
>> Hi William,
>> 
>> Here's a first cut of the ported driver. Note that this is really an RFC as
>> there's still quite a bit I don't fully understand. I've left some of my old
>> sysfs files in there just to keep track of which features I'm still missing.
>
> Hi Felipe,

before anything, thanks for the extensive background on the ideas of the
framework :-) Much appreciated

> That is understandable since this is an RFC; I expect in your final
> version those sysfs files will be Counter subsystem extension
> attributes.
>
> For reference, the existing Counter subsystem attributes are documented
> in the Documentation/ABI/testing/sysfs-bus-counter file. If you have any
> new attributes specific to the Intel QEP driver, add them as a new
> Documentation/ABI/testing/sysfs-bus-counter-intel-qep file.

Will do, thanks

>> If understood this correctly, I should add noise filtering as a signal
>> extension, right? Same for Input Swap.
>
> Determining the type of extension to create is a matter of scope.
>
> * Signal extensions are attributes which expose information/control
>   specific to a Signal. These types of attributes will exist under a
>   Signal's directory in sysfs.
>
>   For example, if have an invert feature for a Signal, you can have a
>   Signal extension called "invert" that toggles that feature:
>   /sys/bus/counter/devices/counterX/signalY/invert
>
> * Count extensions are attributes which expose information/control
>   specific to a Count. These type of attributes will exist under a
>   Count's directory in sysfs.
>
>   For example, if you want to pause/unpause a Count from updating, you
>   could have a Count extension called "enable" that toggles such:
>   /sys/bus/counter/devices/counterX/countY/enable
>
> * Device extensions are attributes which expose information/control
>   non-specific to a particular Count or Signal. This is where you would
>   put your global features or other miscellanous functionality.
>
>   For example, if your device has an overtemp sensor, you could report
>   the chip overheated via a device extension called "error_overtemp":
>   /sys/bus/counter/devices/counterX/error_overtemp

This clarifies a lot of my doubts.

> I'm not very familiar with the Intel QEP features, so I'll need your
> help clarifying some of it for me. I'm assuming "noise filtering" is
> a feature you can enable per individual Signal (is this correct?). If
> so, then it would be implemented as a Signal extension, maybe as a
> /sys/bus/counter/devices/counterX/signalY/noise_filter_enable file.
> Otherwise, it would be a device attribute if it enables noise filtering
> over all Signals.

The device, at least as is right now, has a single counter. It's a bit
difficult to decide whether the noise filter is global or per count :-)
I'll assume it to be per count as that would allow extension in case a
future version comes out with more than one counter. What do you think?

> Is "Input Swap" swapping the Phase A and Phase B signal lines for each
> Count? If so, this would be a Count extension if you can configure it
> per Count; otherwise a device extension if it's globally enabled for all
> Counts.

Yeah, it swaps A and B signals. Again, with a single count, difficult to
decide on global vs per-count.

> If you're still having trouble figuring out which type of extension to
> use, give me a simple breakdown of the features you are trying to
> support with these attributes and I should be able to specify which type
> works best for each.
>  
>> How should we handle the reset mode of the device? And mode of operation?
>
> Is "reset mode" specifying the condition that causes a Count's count to
> be reset back to a value of 0? If so, I assume "index" means an active
> level on the index line will reset the count; does "maximum" mean the
> index line is ignored and the reset operation is simply activated once
> the ceiling is reached? As well, I'm assuming this is a global
> configuration for all counts.

This seems more logical to be global, yes. The other thing this QEP
controller has is that we can configure the maximum count value. By
default it's UINT_MAX (0xffffffff) but it could be less and that's
configurable. So I'm assuming I would have a "maximum_count" device
extension in this case.

> If my assumptions are correct here, then this behavior can be exposed
> by creating two device extensions: "preset" and "preset_enable"; these
> should be based on the existing two same-named Count attributes (see the
> Documentation/ABI/testing/sysfs-bus-counter file).
>
> * /sys/bus/counter/devices/counterX/preset
>   For the Intel QEP, this will be a read-only that always report "0".
>
> * /sys/bus/counter/devices/counterX/preset_enable
>   Assuming "maximum" is just equivalent to ignoring index, preset_enable
>   can toggle between the two modes as a simple boolean: "1" = "index"
>   and "0" = "maximum".

Understood. This will be nice from userspace point of view.

>   If I assumed incorrectly, please let me know and we can discuss the
>   possibility of a new attribute (perhaps "preset_mode").
>
> Take a look inside 104-quad-8.c to see how it handles index lines and
> reseting the counts; "preset" and "preset_enable" extensions are
> specified in the quad8_count_ext array.
>
>> If you have any ideas, let me know.
>> 
>> cheers
>
> Since I expect a good amount of this code to change once the extensions
> are added, I'll hold off on a more in-depth code review until we get it
> closer to what it'll actually look like. From skimming the code in this
> RFC, it seems like you have the core Counter subsystem functions and
> structures use correct, so I think it'll be the implementations of the
> extensions that we'll be focusing on.

Cool, thanks a lot. I'll start moving some more of those private sysfs
files to extensions.

Again, thanks for the background information. Helps a lot.

cheers

-- 
balbi

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

Re: [RFC/PATCH] counter: introduce support for Intel QEP Encoder

[William Breathitt Gray]

On Tue, Sep 17, 2019 at 03:07:40PM +0300, Felipe Balbi wrote:
> 
> Hi William,
> 
> William Breathitt Gray <vilhelm.gray@gmail.com> writes:
> 
> > On Mon, Sep 16, 2019 at 12:34:31PM +0300, Felipe Balbi wrote:
> >> Add support for Intel PSE Quadrature Encoder
> >> 
> >> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> >> ---
> >> 
> >> Hi William,
> >> 
> >> Here's a first cut of the ported driver. Note that this is really an RFC as
> >> there's still quite a bit I don't fully understand. I've left some of my old
> >> sysfs files in there just to keep track of which features I'm still missing.
> >
> > Hi Felipe,
> 
> before anything, thanks for the extensive background on the ideas of the
> framework :-) Much appreciated
> 
> > That is understandable since this is an RFC; I expect in your final
> > version those sysfs files will be Counter subsystem extension
> > attributes.
> >
> > For reference, the existing Counter subsystem attributes are documented
> > in the Documentation/ABI/testing/sysfs-bus-counter file. If you have any
> > new attributes specific to the Intel QEP driver, add them as a new
> > Documentation/ABI/testing/sysfs-bus-counter-intel-qep file.
> 
> Will do, thanks
> 
> >> If understood this correctly, I should add noise filtering as a signal
> >> extension, right? Same for Input Swap.
> >
> > Determining the type of extension to create is a matter of scope.
> >
> > * Signal extensions are attributes which expose information/control
> >   specific to a Signal. These types of attributes will exist under a
> >   Signal's directory in sysfs.
> >
> >   For example, if have an invert feature for a Signal, you can have a
> >   Signal extension called "invert" that toggles that feature:
> >   /sys/bus/counter/devices/counterX/signalY/invert
> >
> > * Count extensions are attributes which expose information/control
> >   specific to a Count. These type of attributes will exist under a
> >   Count's directory in sysfs.
> >
> >   For example, if you want to pause/unpause a Count from updating, you
> >   could have a Count extension called "enable" that toggles such:
> >   /sys/bus/counter/devices/counterX/countY/enable
> >
> > * Device extensions are attributes which expose information/control
> >   non-specific to a particular Count or Signal. This is where you would
> >   put your global features or other miscellanous functionality.
> >
> >   For example, if your device has an overtemp sensor, you could report
> >   the chip overheated via a device extension called "error_overtemp":
> >   /sys/bus/counter/devices/counterX/error_overtemp
> 
> This clarifies a lot of my doubts.
> 
> > I'm not very familiar with the Intel QEP features, so I'll need your
> > help clarifying some of it for me. I'm assuming "noise filtering" is
> > a feature you can enable per individual Signal (is this correct?). If
> > so, then it would be implemented as a Signal extension, maybe as a
> > /sys/bus/counter/devices/counterX/signalY/noise_filter_enable file.
> > Otherwise, it would be a device attribute if it enables noise filtering
> > over all Signals.
> 
> The device, at least as is right now, has a single counter. It's a bit
> difficult to decide whether the noise filter is global or per count :-)
> I'll assume it to be per count as that would allow extension in case a
> future version comes out with more than one counter. What do you think?

Ah, I see what you mean, that was causing me confusion as well. However,
I would suggest making this a device extension instead of a Count
extension.

From an ontological perspective, Counts are essentially independent from
Signals -- they are affected by Synapses, not Signals. So in the sense
of the Generic Counter paradigm, it is wrong to go to a Count's
directory to configure a feature for a Signal.

The practical reason not to do this is that a single Signal can in
theory be associated to several Counts via multiple Synapses. In these
cases, it would be wrong for one of those Count attributes to affect the
configuration of a Signal that is also used by another Count. This is
the reason we have the distinction between Signal extensions and Count
extensions: in the Generic Counter paradigm, these components are
independent.

> > Is "Input Swap" swapping the Phase A and Phase B signal lines for each
> > Count? If so, this would be a Count extension if you can configure it
> > per Count; otherwise a device extension if it's globally enabled for all
> > Counts.
> 
> Yeah, it swaps A and B signals. Again, with a single count, difficult to
> decide on global vs per-count.

Hmm, this may make more sense as a new function mode. You can see the
existing available function modes in the sysfs-bus-counter Documentation
file, under the "/sys/bus/counter/devices/counterX/countY/function"
description section.

I suggest implementing a new one called "quadrature x4 inverted" or
similar (whatever name you think is most apt). Define a function_set
callback and add it to your intel_qep_counter_ops structure -- when a
user wants to swap the signals, they can select the new function mode.

In a separate patch from your Intel QEP introduction, implement this new
function mode by doing the following:

* include/linux/counter.h
  Add a new constant to the counter_count_function enum structure.

* drivers/counter/counter.c
  Add a respective entry to the counter_count_function_str string array.

> > If you're still having trouble figuring out which type of extension to
> > use, give me a simple breakdown of the features you are trying to
> > support with these attributes and I should be able to specify which type
> > works best for each.
> >  
> >> How should we handle the reset mode of the device? And mode of operation?
> >
> > Is "reset mode" specifying the condition that causes a Count's count to
> > be reset back to a value of 0? If so, I assume "index" means an active
> > level on the index line will reset the count; does "maximum" mean the
> > index line is ignored and the reset operation is simply activated once
> > the ceiling is reached? As well, I'm assuming this is a global
> > configuration for all counts.
> 
> This seems more logical to be global, yes. The other thing this QEP
> controller has is that we can configure the maximum count value. By
> default it's UINT_MAX (0xffffffff) but it could be less and that's
> configurable. So I'm assuming I would have a "maximum_count" device
> extension in this case.

You can implement this as a writable "ceiling" Count extension. It's
already documented in the Documentation/ABI/testing/sysfs-bus-counter
file so no need to worry since some of the other existing counter
drivers already have support for such functionality.

> > If my assumptions are correct here, then this behavior can be exposed
> > by creating two device extensions: "preset" and "preset_enable"; these
> > should be based on the existing two same-named Count attributes (see the
> > Documentation/ABI/testing/sysfs-bus-counter file).
> >
> > * /sys/bus/counter/devices/counterX/preset
> >   For the Intel QEP, this will be a read-only that always report "0".
> >
> > * /sys/bus/counter/devices/counterX/preset_enable
> >   Assuming "maximum" is just equivalent to ignoring index, preset_enable
> >   can toggle between the two modes as a simple boolean: "1" = "index"
> >   and "0" = "maximum".
> 
> Understood. This will be nice from userspace point of view.
> 
> >   If I assumed incorrectly, please let me know and we can discuss the
> >   possibility of a new attribute (perhaps "preset_mode").
> >
> > Take a look inside 104-quad-8.c to see how it handles index lines and
> > reseting the counts; "preset" and "preset_enable" extensions are
> > specified in the quad8_count_ext array.
> >
> >> If you have any ideas, let me know.
> >> 
> >> cheers
> >
> > Since I expect a good amount of this code to change once the extensions
> > are added, I'll hold off on a more in-depth code review until we get it
> > closer to what it'll actually look like. From skimming the code in this
> > RFC, it seems like you have the core Counter subsystem functions and
> > structures use correct, so I think it'll be the implementations of the
> > extensions that we'll be focusing on.
> 
> Cool, thanks a lot. I'll start moving some more of those private sysfs
> files to extensions.
> 
> Again, thanks for the background information. Helps a lot.
> 
> cheers
> 
> -- 
> balbi

[RFC/PATCH 1/2] counter: add support for Quadrature x4 with swapped inputs

[Felipe Balbi]

Some Quadrature Encoders can swap phase inputs A and B
internally. This new function will allow drivers to configure input
swap mode.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 drivers/counter/counter.c | 3 ++-
 include/linux/counter.h   | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
index 106bc7180cd8..b818ae9e85f2 100644
--- a/drivers/counter/counter.c
+++ b/drivers/counter/counter.c
@@ -823,7 +823,8 @@ static const char *const counter_count_function_str[] = {
 	[COUNTER_COUNT_FUNCTION_QUADRATURE_X1_B] = "quadrature x1 b",
 	[COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A] = "quadrature x2 a",
 	[COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B] = "quadrature x2 b",
-	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4] = "quadrature x4"
+	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4] = "quadrature x4",
+	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED] = "quadrature x4 swapped"
 };
 
 static ssize_t counter_function_show(struct device *dev,
diff --git a/include/linux/counter.h b/include/linux/counter.h
index a061cdcdef7c..860769250f89 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -170,7 +170,8 @@ enum counter_count_function {
 	COUNTER_COUNT_FUNCTION_QUADRATURE_X1_B,
 	COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A,
 	COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B,
-	COUNTER_COUNT_FUNCTION_QUADRATURE_X4
+	COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
+	COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED,
 };
 
 /**
-- 
2.23.0

[RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[Felipe Balbi]

Add support for Intel PSE Quadrature Encoder

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---

Changes since v1:
	- Many more private sysfs files converted over to counter interface


How do you want me to model this device's Capture Compare Mode (see
below)?

For the few features which I couldn't find a matching property in
counter framework, I still leave them as private sysfs files so we can
discuss how to model them in the framework.

Do you want Capture Compare to be a new function mode?

BTW, I know I'm missing a Documentation file documenting sysfs files
introduced by this driver, I'll do that once we agree how to move all
other sysfs files to the framework. No worries.


Details about the controller (do you want this in commit log?):


Controller has 2 modes of operation: QEP and Capture. Both modes are
mutually exclusive. We also have a set of maskable interrupts. Further
details about each mode below.


Quadrature Encoder Mode
=======================

Used to measure rotational speed, direction and angle of rotation of a
motor shaft. Feature list:

	- Quadrature decoder providing counter pulses with x4 count
	  resolution and count direction

	- 32-bit up/down Position Counter for position measurement

	- Two modes of position counter reset:
		> Maximum Count (ceiling) to reset the position counter
		> Index pulse to reset the position counter

	- Watchdog timer functionality for detecting ‘stall’ events

Capture Compare Mode
====================

Used to measure phase input signal Period or Duty Cycle. Feature List:

	- Capture function operates either on phase A or phase B input
	  and can be configured to operate on lo/hi or hi/lo or both hi
	  and lo transitions.

	- Free-running 32-bit counter to be configured to run at greater
          than or equal to 4x input signal frequency

	- Clock post-scaler to derive the counter clock source from the
	  peripheral clock

	- 32B wide FIFO to capture 32-bit timestamps of up to 8
	  transition events

 drivers/counter/Kconfig     |   6 +
 drivers/counter/Makefile    |   1 +
 drivers/counter/intel-qep.c | 862 ++++++++++++++++++++++++++++++++++++
 3 files changed, 869 insertions(+)
 create mode 100644 drivers/counter/intel-qep.c

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2967d0a9ff91..f280cd721350 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -59,4 +59,10 @@ config FTM_QUADDEC
 	  To compile this driver as a module, choose M here: the
 	  module will be called ftm-quaddec.
 
+config INTEL_QEP
+	tristate "Intel Quadrature Encoder"
+	depends on PCI
+	help
+	  Support for Intel Quadrature Encoder Devices
+
 endif # COUNTER
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 40d35522937d..cf291cfd8cf0 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
 obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
+obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
new file mode 100644
index 000000000000..8347f9fa8e37
--- /dev/null
+++ b/drivers/counter/intel-qep.c
@@ -0,0 +1,862 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * intel-qep.c - Intel Quadrature Encoder Driver
+ *
+ * Copyright (C) 2019 Intel Corporation - https://www.intel.com
+ *
+ * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
+ */
+#include <linux/bitops.h>
+#include <linux/counter.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/sysfs.h>
+
+#define INTEL_QEPCON		0x00
+#define INTEL_QEPFLT		0x04
+#define INTEL_QEPCOUNT		0x08
+#define INTEL_QEPMAX		0x0c
+#define INTEL_QEPWDT		0x10
+#define INTEL_QEPCAPDIV		0x14
+#define INTEL_QEPCNTR		0x18
+#define INTEL_QEPCAPBUF		0x1c
+#define INTEL_QEPINT_STAT	0x20
+#define INTEL_QEPINT_MASK	0x24
+
+/* QEPCON */
+#define INTEL_QEPCON_EN		BIT(0)
+#define INTEL_QEPCON_FLT_EN	BIT(1)
+#define INTEL_QEPCON_EDGE_A	BIT(2)
+#define INTEL_QEPCON_EDGE_B	BIT(3)
+#define INTEL_QEPCON_EDGE_INDX	BIT(4)
+#define INTEL_QEPCON_SWPAB	BIT(5)
+#define INTEL_QEPCON_OP_MODE	BIT(6)
+#define INTEL_QEPCON_PH_ERR	BIT(7)
+#define INTEL_QEPCON_COUNT_RST_MODE BIT(8)
+#define INTEL_QEPCON_INDX_GATING_MASK GENMASK(10, 9)
+#define INTEL_QEPCON_INDX_GATING(n) (((n) & 3) << 9)
+#define INTEL_QEPCON_INDX_PAL_PBL INTEL_QEPCON_INDX_GATING(0)
+#define INTEL_QEPCON_INDX_PAL_PBH INTEL_QEPCON_INDX_GATING(1)
+#define INTEL_QEPCON_INDX_PAH_PBL INTEL_QEPCON_INDX_GATING(2)
+#define INTEL_QEPCON_INDX_PAH_PBH INTEL_QEPCON_INDX_GATING(3)
+#define INTEL_QEPCON_CAP_MODE	BIT(11)
+#define INTEL_QEPCON_FIFO_THRE_MASK GENMASK(14, 12)
+#define INTEL_QEPCON_FIFO_THRE(n) ((((n) - 1) & 7) << 12)
+#define INTEL_QEPCON_FIFO_EMPTY	BIT(15)
+
+/* QEPFLT */
+#define INTEL_QEPFLT_MAX_COUNT(n) ((n) & 0x1fffff)
+
+/* QEPINT */
+#define INTEL_QEPINT_FIFOCRIT	BIT(5)
+#define INTEL_QEPINT_FIFOENTRY	BIT(4)
+#define INTEL_QEPINT_QEPDIR	BIT(3)
+#define INTEL_QEPINT_QEPRST_UP	BIT(2)
+#define INTEL_QEPINT_QEPRST_DOWN BIT(1)
+#define INTEL_QEPINT_WDT	BIT(0)
+
+#define INTEL_QEP_DIRECTION_FORWARD 1
+#define INTEL_QEP_DIRECTION_BACKWARD !INTEL_QEP_DIRECTION_FORWARD
+
+#define INTEL_QEP_COUNTER_EXT_RW(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+	.write = _name##_write, \
+}
+
+#define INTEL_QEP_COUNTER_EXT_RO(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+}
+
+#define INTEL_QEP_COUNTER_COUNT_EXT_RW(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+	.write = _name##_write, \
+}
+
+#define INTEL_QEP_COUNTER_COUNT_EXT_RO(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+}
+
+struct intel_qep {
+	struct counter_device counter;
+	struct mutex lock;
+	struct pci_dev *pci;
+	struct device *dev;
+	void __iomem *regs;
+	u32 interrupt;
+	int direction;
+	bool enabled;
+};
+
+#define counter_to_qep(c)	(container_of((c), struct intel_qep, counter))
+
+static inline u32 intel_qep_readl(void __iomem *base, u32 offset)
+{
+	return readl(base + offset);
+}
+
+static inline void intel_qep_writel(void __iomem *base, u32 offset, u32 value)
+{
+	writel(value, base + offset);
+}
+
+static ssize_t phase_error_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_PH_ERR ? "error" : "okay");
+}
+static DEVICE_ATTR_RO(phase_error);
+
+static ssize_t fifo_empty_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_FIFO_EMPTY ? "empty" : "not empty");
+}
+static DEVICE_ATTR_RO(fifo_empty);
+
+static ssize_t operating_mode_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_OP_MODE ? "capture" : "quadrature");
+}
+
+static ssize_t operating_mode_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	if (qep->enabled)
+		return -EINVAL;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (sysfs_streq(buf, "capture"))
+		reg |= INTEL_QEPCON_OP_MODE;
+	else if (sysfs_streq(buf, "quadrature"))
+		reg &= ~INTEL_QEPCON_OP_MODE;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return count;
+}
+static DEVICE_ATTR_RW(operating_mode);
+
+static ssize_t capture_mode_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n",
+			reg & INTEL_QEPCON_CAP_MODE ? "both" : "single");
+}
+
+static ssize_t capture_mode_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+
+	if (qep->enabled)
+		return -EINVAL;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (sysfs_streq(buf, "both"))
+		reg |= INTEL_QEPCON_CAP_MODE;
+	else if (sysfs_streq(buf, "single"))
+		reg &= ~INTEL_QEPCON_CAP_MODE;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return count;
+}
+static DEVICE_ATTR_RW(capture_mode);
+
+static const struct attribute *intel_qep_attrs[] = {
+	&dev_attr_capture_mode.attr,
+	&dev_attr_fifo_empty.attr,
+	&dev_attr_operating_mode.attr,
+	&dev_attr_phase_error.attr,
+	NULL	/* Terminating Entry */
+};
+
+static ssize_t capture_data_read(struct file *filp, struct kobject *kobj,
+		struct bin_attribute *attr, char *buf,
+		loff_t offset, size_t count)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct intel_qep *qep = dev_get_drvdata(dev);
+	u32 reg;
+	int i;
+
+	mutex_lock(&qep->lock);
+	for (i = 0; i < count; i += 4) {
+		reg = intel_qep_readl(qep->regs, INTEL_QEPCAPBUF);
+
+		buf[i + 0] = reg & 0xff;
+		buf[i + 1] = (reg >> 8) & 0xff;
+		buf[i + 2] = (reg >> 16) & 0xff;
+		buf[i + 3] = (reg >> 24) & 0xff;
+	}
+	mutex_unlock(&qep->lock);
+
+	return count;
+}
+
+static BIN_ATTR_RO(capture_data, 4);
+
+static struct bin_attribute *intel_qep_bin_attrs[] = {
+	&bin_attr_capture_data,
+	NULL	/* Terminating Entry */
+};
+
+static const struct attribute_group intel_qep_device_aattr_group = {
+	.name = "qep",
+	.attrs = (struct attribute **) intel_qep_attrs,
+	.bin_attrs = intel_qep_bin_attrs,
+};
+
+static const struct pci_device_id intel_qep_id_table[] = {
+	/* EHL */
+	{ PCI_VDEVICE(INTEL, 0x4bc3), },
+	{ PCI_VDEVICE(INTEL, 0x4b81), },
+	{ PCI_VDEVICE(INTEL, 0x4b82), },
+	{ PCI_VDEVICE(INTEL, 0x4b83), },
+	{  } /* Terminating Entry */
+};
+MODULE_DEVICE_TABLE(pci, intel_qep_id_table);
+
+static void intel_qep_init(struct intel_qep *qep, bool reset)
+{
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	reg &= ~INTEL_QEPCON_EN;
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	/* make sure periperal is disabled by reading one more time */
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (reset) {
+		reg &= ~(INTEL_QEPCON_OP_MODE | INTEL_QEPCON_FLT_EN);
+		reg |= INTEL_QEPCON_EDGE_A | INTEL_QEPCON_EDGE_B |
+			INTEL_QEPCON_EDGE_INDX | INTEL_QEPCON_COUNT_RST_MODE;
+	}
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	intel_qep_writel(qep->regs, INTEL_QEPWDT, 0x1000);
+	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x0);
+
+	qep->direction = INTEL_QEP_DIRECTION_FORWARD;
+}
+
+static irqreturn_t intel_qep_irq_thread(int irq, void *_qep)
+{
+	struct intel_qep	*qep = _qep;
+	u32			stat;
+
+	mutex_lock(&qep->lock);
+
+	stat = qep->interrupt;
+	if (stat & INTEL_QEPINT_FIFOCRIT)
+		sysfs_notify(&qep->dev->kobj, "qep", "capture_buffer");
+
+	if (stat & INTEL_QEPINT_FIFOENTRY)
+		sysfs_notify(&qep->dev->kobj, "qep", "capture_buffer");
+
+	if (stat & INTEL_QEPINT_QEPDIR)
+		qep->direction = !qep->direction;
+
+	if (stat & INTEL_QEPINT_QEPRST_UP)
+		qep->direction = INTEL_QEP_DIRECTION_FORWARD;
+
+	if (stat & INTEL_QEPINT_QEPRST_DOWN)
+		qep->direction = INTEL_QEP_DIRECTION_BACKWARD;
+
+	if (stat & INTEL_QEPINT_WDT)
+		dev_dbg(qep->dev, "Watchdog\n");
+
+	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x00);
+	mutex_unlock(&qep->lock);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t intel_qep_irq(int irq, void *_qep)
+{
+	struct intel_qep	*qep = _qep;
+	u32			stat;
+
+	stat = intel_qep_readl(qep->regs, INTEL_QEPINT_STAT);
+	if (stat) {
+		qep->interrupt = stat;
+		intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0xffffffff);
+		intel_qep_writel(qep->regs, INTEL_QEPINT_STAT, stat);
+		return IRQ_WAKE_THREAD;
+	}
+
+	return IRQ_HANDLED;
+}
+
+enum intel_qep_synapse_action {
+	INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE,
+	INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE,
+};
+
+static enum counter_synapse_action intel_qep_synapse_actions[] = {
+	[INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE] =
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+	
+	[INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE] =
+	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
+};
+
+enum intel_qep_count_function {
+	INTEL_QEP_ENCODER_MODE_NORMAL,
+	INTEL_QEP_ENCODER_MODE_SWAPPED,
+};
+
+static const enum counter_count_function intel_qep_count_functions[] = {
+	[INTEL_QEP_ENCODER_MODE_NORMAL] =
+	COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
+
+	[INTEL_QEP_ENCODER_MODE_SWAPPED] =
+	COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED,
+};
+
+static int intel_qep_count_read(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_count_read_value *val)
+{
+	struct intel_qep *const qep = counter->priv;
+	uint32_t cntval;
+
+	cntval = intel_qep_readl(qep, INTEL_QEPCOUNT);
+	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cntval);
+
+	return 0;
+}
+
+static int intel_qep_count_write(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_count_write_value *val)
+{
+	struct intel_qep *const qep = counter->priv;
+	u32 cnt;
+	int err;
+
+	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
+	if (err)
+		return err;
+
+	intel_qep_writel(qep->regs, INTEL_QEPMAX, cnt);
+
+	return 0;
+}
+
+static int intel_qep_function_get(struct counter_device *counter,
+		struct counter_count *count, size_t *function)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	if (req & INTEL_QEPCON_SWPAB)
+		*function = INTEL_QEP_ENCODER_MODE_SWAPPED;
+	else
+		*function = INTEL_QEP_ENCODER_MODE_NORMAL;
+
+	return 0;
+}
+
+static int intel_qep_function_set(struct counter_device *counter,
+		struct counter_count *count, size_t *function)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	if (*function == INTEL_QEP_ENCODER_MODE_SWAPPED)
+		reg |= INTEL_QEPCON_SWPAB;
+	else
+		reg &= ~INTEL_QEPCON_SWPAB;
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return 0;
+}
+
+static int intel_qep_action_get(struct counter_device *counter,
+		struct counter_count *count, struct counter_synapse *synapse,
+		size_t *action)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	*action = reg & synapse->signal->id ?
+		INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE :
+		INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE;
+
+	return 0;
+}
+
+static int intel_qep_action_set(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_synapse *synapse, size_t action)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (action == INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE)
+		reg |= synapse->signal->id;
+	else
+		reg &= ~synapse->signal->id;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return 0;
+}
+
+static const struct counter_ops intel_qep_counter_ops = {
+	.count_read = intel_qep_count_read,
+	.count_write = intel_qep_count_write,
+
+	.function_get = intel_qep_function_get,
+	.function_set = intel_qep_function_set,
+
+	.action_get = intel_qep_action_get,
+	.action_set = intel_qep_action_set,
+};
+
+static struct counter_signal intel_qep_signals[] = {
+	{
+		.id = INTEL_QEPCON_EDGE_A,
+		.name = "Phase A",
+	},
+	{
+		.id = INTEL_QEPCON_EDGE_B,
+		.name = "Phase B",
+	},
+	{
+		.id = INTEL_QEPCON_EDGE_INDX,
+		.name = "Index",
+	},
+};
+
+static struct counter_synapse intel_qep_count_synapses[] = {
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[0],
+	},
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[1],
+	},
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[2],
+	},
+};
+
+static const char * const intel_qep_clock_prescalers[] = {
+	"1", "2", "4", "8", "16", "32", "64", "128"
+};
+
+static int intel_qep_clock_prescaler_get(struct counter_device *counter,
+		struct counter_count *count, size_t *mode)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	
+	*mode = intel_qep_readl(qep->regs, INTEL_QEPCAPDIV);
+
+	return 0;
+}
+
+static int intel_qep_clock_prescaler_set(struct counter_device *counter,
+		struct counter_count *count, size_t mode)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+
+	intel_qep_writel(qep->regs, INTEL_QEPCAPDIV, ffs(mode) - 1);
+
+	return 0;
+}
+
+static struct counter_count_enum_ext intel_qep_clock_prescaler_enum = {
+	.items = intel_qep_clock_prescalers,
+	.num_items = ARRAY_SIZE(intel_qep_clock_prescalers),
+	.get = intel_qep_clock_prescaler_get,
+	.set = intel_qep_clock_prescaler_set,
+};
+
+static ssize_t ceiling_read(struct counter_device *counter,
+		struct counter_count *count, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPMAX);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", reg);
+}
+
+static ssize_t ceiling_write(struct counter_device *counter,
+		struct counter_count *count, void *priv, const char *buf,
+		size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 max;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &max);
+	if (ret < 0)
+		return ret;
+
+	intel_qep_writel(qep->regs, INTEL_QEPMAX, max);
+
+	return len;
+}
+
+static ssize_t enable_read(struct counter_device *counter,
+		struct counter_count *count, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", !!(reg & INTEL_QEPCON_EN));
+}
+
+static ssize_t enable_write(struct counter_device *counter,
+		struct counter_count *count, void *priv, const char *buf,
+		size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+	u32 val;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret < 0)
+		return ret;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (val)
+		reg |= INTEL_QEPCON_EN;
+	else
+		reg &= ~INTEL_QEPCON_EN;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return len;
+}
+
+static ssize_t direction_read(struct counter_device *counter,
+		struct counter_count *count, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", qep->direction ?
+			"forward" : "backward");
+}
+
+static const struct counter_count_ext intel_qep_count_ext[] = {
+	COUNTER_COUNT_ENUM("prescaler", &intel_qep_clock_prescaler_enum),
+	COUNTER_COUNT_ENUM_AVAILABLE("prescaler",
+			&intel_qep_clock_prescaler_enum),
+
+	INTEL_QEP_COUNTER_COUNT_EXT_RW(ceiling),
+	INTEL_QEP_COUNTER_COUNT_EXT_RW(enable),
+	INTEL_QEP_COUNTER_COUNT_EXT_RO(direction),
+};
+
+static struct counter_count intel_qep_counter_count[] = {
+	{
+		.id = 0,
+		.name = "Channel 1 Count",
+		.functions_list = intel_qep_count_functions,
+		.num_functions = ARRAY_SIZE(intel_qep_count_functions),
+		.synapses = intel_qep_count_synapses,
+		.num_synapses = ARRAY_SIZE(intel_qep_count_synapses),
+		.ext = intel_qep_count_ext,
+		.num_ext = ARRAY_SIZE(intel_qep_count_ext),
+	},
+};
+
+static ssize_t noise_read(struct counter_device *counter, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (!(reg & INTEL_QEPCON_FLT_EN))
+		return snprintf(buf, PAGE_SIZE, "0\n");
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPFLT);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", INTEL_QEPFLT_MAX_COUNT(reg));
+}
+
+static ssize_t noise_write(struct counter_device *counter, void *priv,
+		const char *buf, size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+	u32 max;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &max);
+	if (ret < 0)
+		return ret;
+
+	if (max > 0x1fffff)
+		max = 0x1ffff;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (max == 0) {
+		reg &= ~INTEL_QEPCON_FLT_EN;
+	} else {
+		reg |= INTEL_QEPCON_FLT_EN;
+		intel_qep_writel(qep->regs, INTEL_QEPFLT, max);
+	}
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+	
+	return len;
+}
+
+static ssize_t preset_read(struct counter_device *counter, void *priv, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0\n");
+}
+
+static ssize_t preset_enable_read(struct counter_device *counter, void *priv,
+		char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+			!(reg & INTEL_QEPCON_COUNT_RST_MODE));
+}
+
+static ssize_t preset_enable_write(struct counter_device *counter, void *priv,
+		const char *buf, size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+	u32 val;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret < 0)
+		return ret;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (val)
+		reg &= ~INTEL_QEPCON_COUNT_RST_MODE;
+	else
+		reg |= INTEL_QEPCON_COUNT_RST_MODE;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+	
+	return len;
+}
+
+static const struct counter_device_ext intel_qep_ext[] = {
+	INTEL_QEP_COUNTER_EXT_RW(noise),
+	INTEL_QEP_COUNTER_EXT_RO(preset),
+	INTEL_QEP_COUNTER_EXT_RW(preset_enable)
+};
+
+static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	struct intel_qep	*qep;
+	struct device		*dev = &pci->dev;
+	void __iomem		*regs;
+	int			ret;
+	int			irq;
+
+	qep = devm_kzalloc(dev, sizeof(*qep), GFP_KERNEL);
+	if (!qep)
+		return -ENOMEM;
+
+	ret = pcim_enable_device(pci);
+	if (ret)
+		return ret;
+
+	pci_set_master(pci);
+
+	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
+	if (ret)
+		return ret;
+
+	regs = pcim_iomap_table(pci)[0];
+	if (!regs)
+		return -ENOMEM;
+
+	qep->pci = pci;
+	qep->dev = dev;
+	qep->regs = regs;
+	mutex_init(&qep->lock);
+
+	intel_qep_init(qep, true);
+	pci_set_drvdata(pci, qep);
+
+	qep->counter.name = pci_name(pci);
+	qep->counter.parent = dev;
+	qep->counter.ops = &intel_qep_counter_ops;
+	qep->counter.counts = intel_qep_counter_count;
+	qep->counter.num_counts = ARRAY_SIZE(intel_qep_counter_count);
+	qep->counter.signals = intel_qep_signals;
+	qep->counter.num_signals = ARRAY_SIZE(intel_qep_signals);
+	qep->counter.ext = intel_qep_ext;
+	qep->counter.num_ext = ARRAY_SIZE(intel_qep_ext);
+	qep->counter.priv = qep;
+
+	ret = counter_register(&qep->counter);
+	if (ret)
+		return ret;
+
+	ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		goto err_irq_vectors;
+
+	irq = pci_irq_vector(pci, 0);
+	ret = devm_request_threaded_irq(&pci->dev, irq, intel_qep_irq,
+			intel_qep_irq_thread, IRQF_SHARED | IRQF_TRIGGER_RISING,
+			"intel-qep", qep);
+	if (ret)
+		goto err_irq;
+
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_allow(dev);
+
+	return 0;
+
+err_irq:
+	pci_free_irq_vectors(pci);
+
+err_irq_vectors:
+	counter_unregister(&qep->counter);
+
+	return ret;
+}
+
+static void intel_qep_remove(struct pci_dev *pci)
+{
+	struct intel_qep	*qep = pci_get_drvdata(pci);
+	struct device		*dev = &pci->dev;
+
+	pm_runtime_forbid(dev);
+	pm_runtime_get_noresume(dev);
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, 0);
+	pci_free_irq_vectors(pci);
+	counter_unregister(&qep->counter);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int intel_qep_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int intel_qep_resume(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct intel_qep *qep = pci_get_drvdata(pdev);
+
+	intel_qep_init(qep, false);
+
+	return 0;
+}
+
+static int intel_qep_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int intel_qep_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct intel_qep *qep = pci_get_drvdata(pdev);
+
+	intel_qep_init(qep, false);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops intel_qep_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(intel_qep_suspend,
+				intel_qep_resume)
+	SET_RUNTIME_PM_OPS(intel_qep_runtime_suspend, intel_qep_runtime_resume,
+				NULL)
+};
+
+static struct pci_driver intel_qep_driver = {
+	.name		= "intel-qep",
+	.id_table	= intel_qep_id_table,
+	.probe		= intel_qep_probe,
+	.remove		= intel_qep_remove,
+	.driver = {
+		.pm = &intel_qep_pm_ops,
+	}
+};
+
+module_pci_driver(intel_qep_driver);
+
+MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel Quadrature Encoder Driver");
-- 
2.23.0

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[William Breathitt Gray]

On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
> Add support for Intel PSE Quadrature Encoder
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
> 
> Changes since v1:
> 	- Many more private sysfs files converted over to counter interface
> 
> 
> How do you want me to model this device's Capture Compare Mode (see
> below)?

Hi Felipe,

I'm CCing Fabien and David as they may be interested in the timestamps
discussion. See below for some ideas I have on implementing this.

> For the few features which I couldn't find a matching property in
> counter framework, I still leave them as private sysfs files so we can
> discuss how to model them in the framework.
> 
> Do you want Capture Compare to be a new function mode?
> 
> BTW, I know I'm missing a Documentation file documenting sysfs files
> introduced by this driver, I'll do that once we agree how to move all
> other sysfs files to the framework. No worries.
> 
> 
> Details about the controller (do you want this in commit log?):
> 
> 
> Controller has 2 modes of operation: QEP and Capture. Both modes are
> mutually exclusive. We also have a set of maskable interrupts. Further
> details about each mode below.

I noticed your interrupt handler takes care of a number of different
scenarios. Would you be able to summarize a bit further here the
conditions for this device that cause an interrupt to be fired (watchdog
timeout, FIFO updates, etc.)?

> Quadrature Encoder Mode
> =======================
> 
> Used to measure rotational speed, direction and angle of rotation of a
> motor shaft. Feature list:
> 
> 	- Quadrature decoder providing counter pulses with x4 count
> 	  resolution and count direction
> 
> 	- 32-bit up/down Position Counter for position measurement
> 
> 	- Two modes of position counter reset:
> 		> Maximum Count (ceiling) to reset the position counter
> 		> Index pulse to reset the position counter
> 
> 	- Watchdog timer functionality for detecting ‘stall’ events
> 
> Capture Compare Mode
> ====================
> 
> Used to measure phase input signal Period or Duty Cycle. Feature List:
> 
> 	- Capture function operates either on phase A or phase B input
> 	  and can be configured to operate on lo/hi or hi/lo or both hi
> 	  and lo transitions.
> 
> 	- Free-running 32-bit counter to be configured to run at greater
>           than or equal to 4x input signal frequency

So in "Capture Compare" mode, the counter value is just increasing when
a state condition transition occurs. In that case we won't need a new
function mode to represent this behavior since one already exists:
"increase".

You can add it to your intel_qep_count_functions array like so:

        [INTEL_QEP_ENCODER_MODE_CAPTURE] =
        COUNTER_COUNT_FUNCTION_INCREASE,

The various configurations for this mode are just Synapse action modes.
If you want only Phase A, you would set the action mode for Phase A
("rising edge", "falling edge", or "both edges") and change the action
mode for Phase B to "none"; vice-versa configuration for Phase B instead
of Phase A.

One thing to keep in mind is that action_set will need to maintain valid
configurations -- so if the user tries to set the action mode for Phase
A to something other than "none", you need to automatically set Phase
B's action mode to "none" (and vice-versa).

> 	- Clock post-scaler to derive the counter clock source from the
> 	  peripheral clock

I see you already have a "prescaler" extension in your code. Is this
different from the "post-scaler" you mentioned here?

> 	- 32B wide FIFO to capture 32-bit timestamps of up to 8
> 	  transition events

You can implement this as a Count extension called "timestamps" or
similar. What we can do is have a read on this attribute return the
entire FIFO data buffer as unsigned integers, where each timestamp is
deliminated by a space.

In addition, it may be useful to have another extension called
"timestamps_layout", or something along those lines, that will report
the ordering of the buffer (i.e. whether it's "fifo", "lifo", etc.).

Would this design work for your needs?

William Breathitt Gray

>  drivers/counter/Kconfig     |   6 +
>  drivers/counter/Makefile    |   1 +
>  drivers/counter/intel-qep.c | 862 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 869 insertions(+)
>  create mode 100644 drivers/counter/intel-qep.c
> 
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 2967d0a9ff91..f280cd721350 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -59,4 +59,10 @@ config FTM_QUADDEC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ftm-quaddec.
>  
> +config INTEL_QEP
> +	tristate "Intel Quadrature Encoder"
> +	depends on PCI
> +	help
> +	  Support for Intel Quadrature Encoder Devices
> +
>  endif # COUNTER
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index 40d35522937d..cf291cfd8cf0 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
>  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
>  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
> +obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
> diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
> new file mode 100644
> index 000000000000..8347f9fa8e37
> --- /dev/null
> +++ b/drivers/counter/intel-qep.c
> @@ -0,0 +1,862 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel-qep.c - Intel Quadrature Encoder Driver
> + *
> + * Copyright (C) 2019 Intel Corporation - https://www.intel.com
> + *
> + * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> + */
> +#include <linux/bitops.h>
> +#include <linux/counter.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/sysfs.h>
> +
> +#define INTEL_QEPCON		0x00
> +#define INTEL_QEPFLT		0x04
> +#define INTEL_QEPCOUNT		0x08
> +#define INTEL_QEPMAX		0x0c
> +#define INTEL_QEPWDT		0x10
> +#define INTEL_QEPCAPDIV		0x14
> +#define INTEL_QEPCNTR		0x18
> +#define INTEL_QEPCAPBUF		0x1c
> +#define INTEL_QEPINT_STAT	0x20
> +#define INTEL_QEPINT_MASK	0x24
> +
> +/* QEPCON */
> +#define INTEL_QEPCON_EN		BIT(0)
> +#define INTEL_QEPCON_FLT_EN	BIT(1)
> +#define INTEL_QEPCON_EDGE_A	BIT(2)
> +#define INTEL_QEPCON_EDGE_B	BIT(3)
> +#define INTEL_QEPCON_EDGE_INDX	BIT(4)
> +#define INTEL_QEPCON_SWPAB	BIT(5)
> +#define INTEL_QEPCON_OP_MODE	BIT(6)
> +#define INTEL_QEPCON_PH_ERR	BIT(7)
> +#define INTEL_QEPCON_COUNT_RST_MODE BIT(8)
> +#define INTEL_QEPCON_INDX_GATING_MASK GENMASK(10, 9)
> +#define INTEL_QEPCON_INDX_GATING(n) (((n) & 3) << 9)
> +#define INTEL_QEPCON_INDX_PAL_PBL INTEL_QEPCON_INDX_GATING(0)
> +#define INTEL_QEPCON_INDX_PAL_PBH INTEL_QEPCON_INDX_GATING(1)
> +#define INTEL_QEPCON_INDX_PAH_PBL INTEL_QEPCON_INDX_GATING(2)
> +#define INTEL_QEPCON_INDX_PAH_PBH INTEL_QEPCON_INDX_GATING(3)
> +#define INTEL_QEPCON_CAP_MODE	BIT(11)
> +#define INTEL_QEPCON_FIFO_THRE_MASK GENMASK(14, 12)
> +#define INTEL_QEPCON_FIFO_THRE(n) ((((n) - 1) & 7) << 12)
> +#define INTEL_QEPCON_FIFO_EMPTY	BIT(15)
> +
> +/* QEPFLT */
> +#define INTEL_QEPFLT_MAX_COUNT(n) ((n) & 0x1fffff)
> +
> +/* QEPINT */
> +#define INTEL_QEPINT_FIFOCRIT	BIT(5)
> +#define INTEL_QEPINT_FIFOENTRY	BIT(4)
> +#define INTEL_QEPINT_QEPDIR	BIT(3)
> +#define INTEL_QEPINT_QEPRST_UP	BIT(2)
> +#define INTEL_QEPINT_QEPRST_DOWN BIT(1)
> +#define INTEL_QEPINT_WDT	BIT(0)
> +
> +#define INTEL_QEP_DIRECTION_FORWARD 1
> +#define INTEL_QEP_DIRECTION_BACKWARD !INTEL_QEP_DIRECTION_FORWARD
> +
> +#define INTEL_QEP_COUNTER_EXT_RW(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +	.write = _name##_write, \
> +}
> +
> +#define INTEL_QEP_COUNTER_EXT_RO(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +}
> +
> +#define INTEL_QEP_COUNTER_COUNT_EXT_RW(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +	.write = _name##_write, \
> +}
> +
> +#define INTEL_QEP_COUNTER_COUNT_EXT_RO(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +}
> +
> +struct intel_qep {
> +	struct counter_device counter;
> +	struct mutex lock;
> +	struct pci_dev *pci;
> +	struct device *dev;
> +	void __iomem *regs;
> +	u32 interrupt;
> +	int direction;
> +	bool enabled;
> +};
> +
> +#define counter_to_qep(c)	(container_of((c), struct intel_qep, counter))
> +
> +static inline u32 intel_qep_readl(void __iomem *base, u32 offset)
> +{
> +	return readl(base + offset);
> +}
> +
> +static inline void intel_qep_writel(void __iomem *base, u32 offset, u32 value)
> +{
> +	writel(value, base + offset);
> +}
> +
> +static ssize_t phase_error_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			reg & INTEL_QEPCON_PH_ERR ? "error" : "okay");
> +}
> +static DEVICE_ATTR_RO(phase_error);
> +
> +static ssize_t fifo_empty_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			reg & INTEL_QEPCON_FIFO_EMPTY ? "empty" : "not empty");
> +}
> +static DEVICE_ATTR_RO(fifo_empty);
> +
> +static ssize_t operating_mode_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			reg & INTEL_QEPCON_OP_MODE ? "capture" : "quadrature");
> +}
> +
> +static ssize_t operating_mode_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	if (qep->enabled)
> +		return -EINVAL;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (sysfs_streq(buf, "capture"))
> +		reg |= INTEL_QEPCON_OP_MODE;
> +	else if (sysfs_streq(buf, "quadrature"))
> +		reg &= ~INTEL_QEPCON_OP_MODE;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(operating_mode);
> +
> +static ssize_t capture_mode_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n",
> +			reg & INTEL_QEPCON_CAP_MODE ? "both" : "single");
> +}
> +
> +static ssize_t capture_mode_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	if (qep->enabled)
> +		return -EINVAL;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (sysfs_streq(buf, "both"))
> +		reg |= INTEL_QEPCON_CAP_MODE;
> +	else if (sysfs_streq(buf, "single"))
> +		reg &= ~INTEL_QEPCON_CAP_MODE;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(capture_mode);
> +
> +static const struct attribute *intel_qep_attrs[] = {
> +	&dev_attr_capture_mode.attr,
> +	&dev_attr_fifo_empty.attr,
> +	&dev_attr_operating_mode.attr,
> +	&dev_attr_phase_error.attr,
> +	NULL	/* Terminating Entry */
> +};
> +
> +static ssize_t capture_data_read(struct file *filp, struct kobject *kobj,
> +		struct bin_attribute *attr, char *buf,
> +		loff_t offset, size_t count)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct intel_qep *qep = dev_get_drvdata(dev);
> +	u32 reg;
> +	int i;
> +
> +	mutex_lock(&qep->lock);
> +	for (i = 0; i < count; i += 4) {
> +		reg = intel_qep_readl(qep->regs, INTEL_QEPCAPBUF);
> +
> +		buf[i + 0] = reg & 0xff;
> +		buf[i + 1] = (reg >> 8) & 0xff;
> +		buf[i + 2] = (reg >> 16) & 0xff;
> +		buf[i + 3] = (reg >> 24) & 0xff;
> +	}
> +	mutex_unlock(&qep->lock);
> +
> +	return count;
> +}
> +
> +static BIN_ATTR_RO(capture_data, 4);
> +
> +static struct bin_attribute *intel_qep_bin_attrs[] = {
> +	&bin_attr_capture_data,
> +	NULL	/* Terminating Entry */
> +};
> +
> +static const struct attribute_group intel_qep_device_aattr_group = {
> +	.name = "qep",
> +	.attrs = (struct attribute **) intel_qep_attrs,
> +	.bin_attrs = intel_qep_bin_attrs,
> +};
> +
> +static const struct pci_device_id intel_qep_id_table[] = {
> +	/* EHL */
> +	{ PCI_VDEVICE(INTEL, 0x4bc3), },
> +	{ PCI_VDEVICE(INTEL, 0x4b81), },
> +	{ PCI_VDEVICE(INTEL, 0x4b82), },
> +	{ PCI_VDEVICE(INTEL, 0x4b83), },
> +	{  } /* Terminating Entry */
> +};
> +MODULE_DEVICE_TABLE(pci, intel_qep_id_table);
> +
> +static void intel_qep_init(struct intel_qep *qep, bool reset)
> +{
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	reg &= ~INTEL_QEPCON_EN;
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	/* make sure periperal is disabled by reading one more time */
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (reset) {
> +		reg &= ~(INTEL_QEPCON_OP_MODE | INTEL_QEPCON_FLT_EN);
> +		reg |= INTEL_QEPCON_EDGE_A | INTEL_QEPCON_EDGE_B |
> +			INTEL_QEPCON_EDGE_INDX | INTEL_QEPCON_COUNT_RST_MODE;
> +	}
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPWDT, 0x1000);
> +	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x0);
> +
> +	qep->direction = INTEL_QEP_DIRECTION_FORWARD;
> +}
> +
> +static irqreturn_t intel_qep_irq_thread(int irq, void *_qep)
> +{
> +	struct intel_qep	*qep = _qep;
> +	u32			stat;
> +
> +	mutex_lock(&qep->lock);
> +
> +	stat = qep->interrupt;
> +	if (stat & INTEL_QEPINT_FIFOCRIT)
> +		sysfs_notify(&qep->dev->kobj, "qep", "capture_buffer");
> +
> +	if (stat & INTEL_QEPINT_FIFOENTRY)
> +		sysfs_notify(&qep->dev->kobj, "qep", "capture_buffer");
> +
> +	if (stat & INTEL_QEPINT_QEPDIR)
> +		qep->direction = !qep->direction;
> +
> +	if (stat & INTEL_QEPINT_QEPRST_UP)
> +		qep->direction = INTEL_QEP_DIRECTION_FORWARD;
> +
> +	if (stat & INTEL_QEPINT_QEPRST_DOWN)
> +		qep->direction = INTEL_QEP_DIRECTION_BACKWARD;
> +
> +	if (stat & INTEL_QEPINT_WDT)
> +		dev_dbg(qep->dev, "Watchdog\n");
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x00);
> +	mutex_unlock(&qep->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t intel_qep_irq(int irq, void *_qep)
> +{
> +	struct intel_qep	*qep = _qep;
> +	u32			stat;
> +
> +	stat = intel_qep_readl(qep->regs, INTEL_QEPINT_STAT);
> +	if (stat) {
> +		qep->interrupt = stat;
> +		intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0xffffffff);
> +		intel_qep_writel(qep->regs, INTEL_QEPINT_STAT, stat);
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +enum intel_qep_synapse_action {
> +	INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE,
> +	INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE,
> +};
> +
> +static enum counter_synapse_action intel_qep_synapse_actions[] = {
> +	[INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE] =
> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> +	
> +	[INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE] =
> +	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> +};
> +
> +enum intel_qep_count_function {
> +	INTEL_QEP_ENCODER_MODE_NORMAL,
> +	INTEL_QEP_ENCODER_MODE_SWAPPED,
> +};
> +
> +static const enum counter_count_function intel_qep_count_functions[] = {
> +	[INTEL_QEP_ENCODER_MODE_NORMAL] =
> +	COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
> +
> +	[INTEL_QEP_ENCODER_MODE_SWAPPED] =
> +	COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED,
> +};
> +
> +static int intel_qep_count_read(struct counter_device *counter,
> +		struct counter_count *count,
> +		struct counter_count_read_value *val)
> +{
> +	struct intel_qep *const qep = counter->priv;
> +	uint32_t cntval;
> +
> +	cntval = intel_qep_readl(qep, INTEL_QEPCOUNT);
> +	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cntval);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_count_write(struct counter_device *counter,
> +		struct counter_count *count,
> +		struct counter_count_write_value *val)
> +{
> +	struct intel_qep *const qep = counter->priv;
> +	u32 cnt;
> +	int err;
> +
> +	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
> +	if (err)
> +		return err;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPMAX, cnt);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_function_get(struct counter_device *counter,
> +		struct counter_count *count, size_t *function)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	if (req & INTEL_QEPCON_SWPAB)
> +		*function = INTEL_QEP_ENCODER_MODE_SWAPPED;
> +	else
> +		*function = INTEL_QEP_ENCODER_MODE_NORMAL;
> +
> +	return 0;
> +}
> +
> +static int intel_qep_function_set(struct counter_device *counter,
> +		struct counter_count *count, size_t *function)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	if (*function == INTEL_QEP_ENCODER_MODE_SWAPPED)
> +		reg |= INTEL_QEPCON_SWPAB;
> +	else
> +		reg &= ~INTEL_QEPCON_SWPAB;
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_action_get(struct counter_device *counter,
> +		struct counter_count *count, struct counter_synapse *synapse,
> +		size_t *action)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	*action = reg & synapse->signal->id ?
> +		INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE :
> +		INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE;
> +
> +	return 0;
> +}
> +
> +static int intel_qep_action_set(struct counter_device *counter,
> +		struct counter_count *count,
> +		struct counter_synapse *synapse, size_t action)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (action == INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE)
> +		reg |= synapse->signal->id;
> +	else
> +		reg &= ~synapse->signal->id;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return 0;
> +}
> +
> +static const struct counter_ops intel_qep_counter_ops = {
> +	.count_read = intel_qep_count_read,
> +	.count_write = intel_qep_count_write,
> +
> +	.function_get = intel_qep_function_get,
> +	.function_set = intel_qep_function_set,
> +
> +	.action_get = intel_qep_action_get,
> +	.action_set = intel_qep_action_set,
> +};
> +
> +static struct counter_signal intel_qep_signals[] = {
> +	{
> +		.id = INTEL_QEPCON_EDGE_A,
> +		.name = "Phase A",
> +	},
> +	{
> +		.id = INTEL_QEPCON_EDGE_B,
> +		.name = "Phase B",
> +	},
> +	{
> +		.id = INTEL_QEPCON_EDGE_INDX,
> +		.name = "Index",
> +	},
> +};
> +
> +static struct counter_synapse intel_qep_count_synapses[] = {
> +	{
> +		.actions_list = intel_qep_synapse_actions,
> +		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
> +		.signal = &intel_qep_signals[0],
> +	},
> +	{
> +		.actions_list = intel_qep_synapse_actions,
> +		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
> +		.signal = &intel_qep_signals[1],
> +	},
> +	{
> +		.actions_list = intel_qep_synapse_actions,
> +		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
> +		.signal = &intel_qep_signals[2],
> +	},
> +};
> +
> +static const char * const intel_qep_clock_prescalers[] = {
> +	"1", "2", "4", "8", "16", "32", "64", "128"
> +};
> +
> +static int intel_qep_clock_prescaler_get(struct counter_device *counter,
> +		struct counter_count *count, size_t *mode)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	
> +	*mode = intel_qep_readl(qep->regs, INTEL_QEPCAPDIV);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_clock_prescaler_set(struct counter_device *counter,
> +		struct counter_count *count, size_t mode)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCAPDIV, ffs(mode) - 1);
> +
> +	return 0;
> +}
> +
> +static struct counter_count_enum_ext intel_qep_clock_prescaler_enum = {
> +	.items = intel_qep_clock_prescalers,
> +	.num_items = ARRAY_SIZE(intel_qep_clock_prescalers),
> +	.get = intel_qep_clock_prescaler_get,
> +	.set = intel_qep_clock_prescaler_set,
> +};
> +
> +static ssize_t ceiling_read(struct counter_device *counter,
> +		struct counter_count *count, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPMAX);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", reg);
> +}
> +
> +static ssize_t ceiling_write(struct counter_device *counter,
> +		struct counter_count *count, void *priv, const char *buf,
> +		size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 max;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &max);
> +	if (ret < 0)
> +		return ret;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPMAX, max);
> +
> +	return len;
> +}
> +
> +static ssize_t enable_read(struct counter_device *counter,
> +		struct counter_count *count, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", !!(reg & INTEL_QEPCON_EN));
> +}
> +
> +static ssize_t enable_write(struct counter_device *counter,
> +		struct counter_count *count, void *priv, const char *buf,
> +		size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +	u32 val;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (val)
> +		reg |= INTEL_QEPCON_EN;
> +	else
> +		reg &= ~INTEL_QEPCON_EN;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return len;
> +}
> +
> +static ssize_t direction_read(struct counter_device *counter,
> +		struct counter_count *count, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", qep->direction ?
> +			"forward" : "backward");
> +}
> +
> +static const struct counter_count_ext intel_qep_count_ext[] = {
> +	COUNTER_COUNT_ENUM("prescaler", &intel_qep_clock_prescaler_enum),
> +	COUNTER_COUNT_ENUM_AVAILABLE("prescaler",
> +			&intel_qep_clock_prescaler_enum),
> +
> +	INTEL_QEP_COUNTER_COUNT_EXT_RW(ceiling),
> +	INTEL_QEP_COUNTER_COUNT_EXT_RW(enable),
> +	INTEL_QEP_COUNTER_COUNT_EXT_RO(direction),
> +};
> +
> +static struct counter_count intel_qep_counter_count[] = {
> +	{
> +		.id = 0,
> +		.name = "Channel 1 Count",
> +		.functions_list = intel_qep_count_functions,
> +		.num_functions = ARRAY_SIZE(intel_qep_count_functions),
> +		.synapses = intel_qep_count_synapses,
> +		.num_synapses = ARRAY_SIZE(intel_qep_count_synapses),
> +		.ext = intel_qep_count_ext,
> +		.num_ext = ARRAY_SIZE(intel_qep_count_ext),
> +	},
> +};
> +
> +static ssize_t noise_read(struct counter_device *counter, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (!(reg & INTEL_QEPCON_FLT_EN))
> +		return snprintf(buf, PAGE_SIZE, "0\n");
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPFLT);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", INTEL_QEPFLT_MAX_COUNT(reg));
> +}
> +
> +static ssize_t noise_write(struct counter_device *counter, void *priv,
> +		const char *buf, size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +	u32 max;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &max);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (max > 0x1fffff)
> +		max = 0x1ffff;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (max == 0) {
> +		reg &= ~INTEL_QEPCON_FLT_EN;
> +	} else {
> +		reg |= INTEL_QEPCON_FLT_EN;
> +		intel_qep_writel(qep->regs, INTEL_QEPFLT, max);
> +	}
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +	
> +	return len;
> +}
> +
> +static ssize_t preset_read(struct counter_device *counter, void *priv, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0\n");
> +}
> +
> +static ssize_t preset_enable_read(struct counter_device *counter, void *priv,
> +		char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	return snprintf(buf, PAGE_SIZE, "%d\n",
> +			!(reg & INTEL_QEPCON_COUNT_RST_MODE));
> +}
> +
> +static ssize_t preset_enable_write(struct counter_device *counter, void *priv,
> +		const char *buf, size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +	u32 val;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (val)
> +		reg &= ~INTEL_QEPCON_COUNT_RST_MODE;
> +	else
> +		reg |= INTEL_QEPCON_COUNT_RST_MODE;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +	
> +	return len;
> +}
> +
> +static const struct counter_device_ext intel_qep_ext[] = {
> +	INTEL_QEP_COUNTER_EXT_RW(noise),
> +	INTEL_QEP_COUNTER_EXT_RO(preset),
> +	INTEL_QEP_COUNTER_EXT_RW(preset_enable)
> +};
> +
> +static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
> +{
> +	struct intel_qep	*qep;
> +	struct device		*dev = &pci->dev;
> +	void __iomem		*regs;
> +	int			ret;
> +	int			irq;
> +
> +	qep = devm_kzalloc(dev, sizeof(*qep), GFP_KERNEL);
> +	if (!qep)
> +		return -ENOMEM;
> +
> +	ret = pcim_enable_device(pci);
> +	if (ret)
> +		return ret;
> +
> +	pci_set_master(pci);
> +
> +	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
> +	if (ret)
> +		return ret;
> +
> +	regs = pcim_iomap_table(pci)[0];
> +	if (!regs)
> +		return -ENOMEM;
> +
> +	qep->pci = pci;
> +	qep->dev = dev;
> +	qep->regs = regs;
> +	mutex_init(&qep->lock);
> +
> +	intel_qep_init(qep, true);
> +	pci_set_drvdata(pci, qep);
> +
> +	qep->counter.name = pci_name(pci);
> +	qep->counter.parent = dev;
> +	qep->counter.ops = &intel_qep_counter_ops;
> +	qep->counter.counts = intel_qep_counter_count;
> +	qep->counter.num_counts = ARRAY_SIZE(intel_qep_counter_count);
> +	qep->counter.signals = intel_qep_signals;
> +	qep->counter.num_signals = ARRAY_SIZE(intel_qep_signals);
> +	qep->counter.ext = intel_qep_ext;
> +	qep->counter.num_ext = ARRAY_SIZE(intel_qep_ext);
> +	qep->counter.priv = qep;
> +
> +	ret = counter_register(&qep->counter);
> +	if (ret)
> +		return ret;
> +
> +	ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
> +	if (ret < 0)
> +		goto err_irq_vectors;
> +
> +	irq = pci_irq_vector(pci, 0);
> +	ret = devm_request_threaded_irq(&pci->dev, irq, intel_qep_irq,
> +			intel_qep_irq_thread, IRQF_SHARED | IRQF_TRIGGER_RISING,
> +			"intel-qep", qep);
> +	if (ret)
> +		goto err_irq;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_allow(dev);
> +
> +	return 0;
> +
> +err_irq:
> +	pci_free_irq_vectors(pci);
> +
> +err_irq_vectors:
> +	counter_unregister(&qep->counter);
> +
> +	return ret;
> +}
> +
> +static void intel_qep_remove(struct pci_dev *pci)
> +{
> +	struct intel_qep	*qep = pci_get_drvdata(pci);
> +	struct device		*dev = &pci->dev;
> +
> +	pm_runtime_forbid(dev);
> +	pm_runtime_get_noresume(dev);
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, 0);
> +	pci_free_irq_vectors(pci);
> +	counter_unregister(&qep->counter);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int intel_qep_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int intel_qep_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct intel_qep *qep = pci_get_drvdata(pdev);
> +
> +	intel_qep_init(qep, false);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_runtime_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int intel_qep_runtime_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct intel_qep *qep = pci_get_drvdata(pdev);
> +
> +	intel_qep_init(qep, false);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops intel_qep_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(intel_qep_suspend,
> +				intel_qep_resume)
> +	SET_RUNTIME_PM_OPS(intel_qep_runtime_suspend, intel_qep_runtime_resume,
> +				NULL)
> +};
> +
> +static struct pci_driver intel_qep_driver = {
> +	.name		= "intel-qep",
> +	.id_table	= intel_qep_id_table,
> +	.probe		= intel_qep_probe,
> +	.remove		= intel_qep_remove,
> +	.driver = {
> +		.pm = &intel_qep_pm_ops,
> +	}
> +};
> +
> +module_pci_driver(intel_qep_driver);
> +
> +MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Intel Quadrature Encoder Driver");
> -- 
> 2.23.0
> 

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[Felipe Balbi]

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


Hi,

William Breathitt Gray <vilhelm.gray@gmail.com> writes:
> On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
>> Add support for Intel PSE Quadrature Encoder
>> 
>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> ---
>> 
>> Changes since v1:
>> 	- Many more private sysfs files converted over to counter interface
>> 
>> 
>> How do you want me to model this device's Capture Compare Mode (see
>> below)?
>
> Hi Felipe,
>
> I'm CCing Fabien and David as they may be interested in the timestamps
> discussion. See below for some ideas I have on implementing this.
>
>> For the few features which I couldn't find a matching property in
>> counter framework, I still leave them as private sysfs files so we can
>> discuss how to model them in the framework.
>> 
>> Do you want Capture Compare to be a new function mode?
>> 
>> BTW, I know I'm missing a Documentation file documenting sysfs files
>> introduced by this driver, I'll do that once we agree how to move all
>> other sysfs files to the framework. No worries.
>> 
>> 
>> Details about the controller (do you want this in commit log?):
>> 
>> 
>> Controller has 2 modes of operation: QEP and Capture. Both modes are
>> mutually exclusive. We also have a set of maskable interrupts. Further
>> details about each mode below.
>
> I noticed your interrupt handler takes care of a number of different
> scenarios. Would you be able to summarize a bit further here the
> conditions for this device that cause an interrupt to be fired (watchdog
> timeout, FIFO updates, etc.)?
>
>> Quadrature Encoder Mode
>> =======================
>> 
>> Used to measure rotational speed, direction and angle of rotation of a
>> motor shaft. Feature list:
>> 
>> 	- Quadrature decoder providing counter pulses with x4 count
>> 	  resolution and count direction
>> 
>> 	- 32-bit up/down Position Counter for position measurement
>> 
>> 	- Two modes of position counter reset:
>> 		> Maximum Count (ceiling) to reset the position counter
>> 		> Index pulse to reset the position counter
>> 
>> 	- Watchdog timer functionality for detecting ‘stall’ events
>> 
>> Capture Compare Mode
>> ====================
>> 
>> Used to measure phase input signal Period or Duty Cycle. Feature List:
>> 
>> 	- Capture function operates either on phase A or phase B input
>> 	  and can be configured to operate on lo/hi or hi/lo or both hi
>> 	  and lo transitions.
>> 
>> 	- Free-running 32-bit counter to be configured to run at greater
>>           than or equal to 4x input signal frequency
>
> So in "Capture Compare" mode, the counter value is just increasing when
> a state condition transition occurs. In that case we won't need a new
> function mode to represent this behavior since one already exists:
> "increase".
>
> You can add it to your intel_qep_count_functions array like so:
>
>         [INTEL_QEP_ENCODER_MODE_CAPTURE] =
>         COUNTER_COUNT_FUNCTION_INCREASE,
>
> The various configurations for this mode are just Synapse action modes.
> If you want only Phase A, you would set the action mode for Phase A
> ("rising edge", "falling edge", or "both edges") and change the action
> mode for Phase B to "none"; vice-versa configuration for Phase B instead
> of Phase A.
>
> One thing to keep in mind is that action_set will need to maintain valid
> configurations -- so if the user tries to set the action mode for Phase
> A to something other than "none", you need to automatically set Phase
> B's action mode to "none" (and vice-versa).

interesting, thanks

>> 	- Clock post-scaler to derive the counter clock source from the
>> 	  peripheral clock
>
> I see you already have a "prescaler" extension in your code. Is this
> different from the "post-scaler" you mentioned here?

This was probably a brain-fart on my side. It should be post-scaler, but
that's only valid for capture compare mode.

>> 	- 32B wide FIFO to capture 32-bit timestamps of up to 8
>> 	  transition events
>
> You can implement this as a Count extension called "timestamps" or
> similar. What we can do is have a read on this attribute return the
> entire FIFO data buffer as unsigned integers, where each timestamp is
> deliminated by a space.
>
> In addition, it may be useful to have another extension called
> "timestamps_layout", or something along those lines, that will report
> the ordering of the buffer (i.e. whether it's "fifo", "lifo", etc.).
>
> Would this design work for your needs?

Perhaps it would be best to have a pollable binary sysfs file (meaning,
we need to be able to call sysfs_notify() at will) and userspace just
receives POLLIN whenever there's data read. Then a read returns an array
of e.g. struct counter_event where struct counter_event could be defined
as:

	struct counter_event {
        	uint32_t	event_type;
		struct timespec64 timestamp;
                uint8_t		reserved[32];
        };

Userspace would do something along the lines of:

	fd = open("/sys/bus/counter/foo/capture/timestamps",...);
	pollfd[0].fd = fd;
        pollfd[0].events = POLLIN;
        poll(pollfd, 1, -1);

	if (pollfd[0].revents & POLLIN) {
        	ret = read(fd, events, sizeof(struct counter_event) * 8);

		for (i = 0; i < ret / sizeof(struct counter_event); i++)
			process_event(events[i]);
        }
        
Or something like that.

I could, also, remove this part from the driver for now, so we can
discuss how the capture-compare buffer read would look like. At least we
could get QDP feature merged while we come to a consensus about capture
compare.

What do you think?

-- 
balbi

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

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[William Breathitt Gray]

On Tue, Sep 24, 2019 at 12:46:39PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> William Breathitt Gray <vilhelm.gray@gmail.com> writes:
> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
> >> Add support for Intel PSE Quadrature Encoder
> >> 
> >> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> >> ---
> >> 
> >> Changes since v1:
> >> 	- Many more private sysfs files converted over to counter interface
> >> 
> >> 
> >> How do you want me to model this device's Capture Compare Mode (see
> >> below)?
> >
> > Hi Felipe,
> >
> > I'm CCing Fabien and David as they may be interested in the timestamps
> > discussion. See below for some ideas I have on implementing this.
> >
> >> For the few features which I couldn't find a matching property in
> >> counter framework, I still leave them as private sysfs files so we can
> >> discuss how to model them in the framework.
> >> 
> >> Do you want Capture Compare to be a new function mode?
> >> 
> >> BTW, I know I'm missing a Documentation file documenting sysfs files
> >> introduced by this driver, I'll do that once we agree how to move all
> >> other sysfs files to the framework. No worries.
> >> 
> >> 
> >> Details about the controller (do you want this in commit log?):
> >> 
> >> 
> >> Controller has 2 modes of operation: QEP and Capture. Both modes are
> >> mutually exclusive. We also have a set of maskable interrupts. Further
> >> details about each mode below.
> >
> > I noticed your interrupt handler takes care of a number of different
> > scenarios. Would you be able to summarize a bit further here the
> > conditions for this device that cause an interrupt to be fired (watchdog
> > timeout, FIFO updates, etc.)?
> >
> >> Quadrature Encoder Mode
> >> =======================
> >> 
> >> Used to measure rotational speed, direction and angle of rotation of a
> >> motor shaft. Feature list:
> >> 
> >> 	- Quadrature decoder providing counter pulses with x4 count
> >> 	  resolution and count direction
> >> 
> >> 	- 32-bit up/down Position Counter for position measurement
> >> 
> >> 	- Two modes of position counter reset:
> >> 		> Maximum Count (ceiling) to reset the position counter
> >> 		> Index pulse to reset the position counter
> >> 
> >> 	- Watchdog timer functionality for detecting ‘stall’ events
> >> 
> >> Capture Compare Mode
> >> ====================
> >> 
> >> Used to measure phase input signal Period or Duty Cycle. Feature List:
> >> 
> >> 	- Capture function operates either on phase A or phase B input
> >> 	  and can be configured to operate on lo/hi or hi/lo or both hi
> >> 	  and lo transitions.
> >> 
> >> 	- Free-running 32-bit counter to be configured to run at greater
> >>           than or equal to 4x input signal frequency
> >
> > So in "Capture Compare" mode, the counter value is just increasing when
> > a state condition transition occurs. In that case we won't need a new
> > function mode to represent this behavior since one already exists:
> > "increase".
> >
> > You can add it to your intel_qep_count_functions array like so:
> >
> >         [INTEL_QEP_ENCODER_MODE_CAPTURE] =
> >         COUNTER_COUNT_FUNCTION_INCREASE,
> >
> > The various configurations for this mode are just Synapse action modes.
> > If you want only Phase A, you would set the action mode for Phase A
> > ("rising edge", "falling edge", or "both edges") and change the action
> > mode for Phase B to "none"; vice-versa configuration for Phase B instead
> > of Phase A.
> >
> > One thing to keep in mind is that action_set will need to maintain valid
> > configurations -- so if the user tries to set the action mode for Phase
> > A to something other than "none", you need to automatically set Phase
> > B's action mode to "none" (and vice-versa).
> 
> interesting, thanks
> 
> >> 	- Clock post-scaler to derive the counter clock source from the
> >> 	  peripheral clock
> >
> > I see you already have a "prescaler" extension in your code. Is this
> > different from the "post-scaler" you mentioned here?
> 
> This was probably a brain-fart on my side. It should be post-scaler, but
> that's only valid for capture compare mode.

In that case you're implementation seems fine for this; perhaps a more
generic name for that extension might be better such as "scale", but
I'll decide later.
 
> >> 	- 32B wide FIFO to capture 32-bit timestamps of up to 8
> >> 	  transition events
> >
> > You can implement this as a Count extension called "timestamps" or
> > similar. What we can do is have a read on this attribute return the
> > entire FIFO data buffer as unsigned integers, where each timestamp is
> > deliminated by a space.
> >
> > In addition, it may be useful to have another extension called
> > "timestamps_layout", or something along those lines, that will report
> > the ordering of the buffer (i.e. whether it's "fifo", "lifo", etc.).
> >
> > Would this design work for your needs?
> 
> Perhaps it would be best to have a pollable binary sysfs file (meaning,
> we need to be able to call sysfs_notify() at will) and userspace just
> receives POLLIN whenever there's data read. Then a read returns an array
> of e.g. struct counter_event where struct counter_event could be defined
> as:
> 
> 	struct counter_event {
>         	uint32_t	event_type;
> 		struct timespec64 timestamp;
>                 uint8_t		reserved[32];
>         };
> 
> Userspace would do something along the lines of:
> 
> 	fd = open("/sys/bus/counter/foo/capture/timestamps",...);
> 	pollfd[0].fd = fd;
>         pollfd[0].events = POLLIN;
>         poll(pollfd, 1, -1);
> 
> 	if (pollfd[0].revents & POLLIN) {
>         	ret = read(fd, events, sizeof(struct counter_event) * 8);
> 
> 		for (i = 0; i < ret / sizeof(struct counter_event); i++)
> 			process_event(events[i]);
>         }
>         
> Or something like that.

One concern is that returning binary data like that isn't friendly for
human interaction. However, alternatively printing a human-readable
array would add latency for userspace software that has to interpret it,
so that would a problem as well. This is something we'll have to think
more about then.

> I could, also, remove this part from the driver for now, so we can
> discuss how the capture-compare buffer read would look like. At least we
> could get QDP feature merged while we come to a consensus about capture
> compare.
> 
> What do you think?
> 
> -- 
> balbi

That sounds like a good idea. Most of this driver can be implemented
using the existing Counter subsystem components, so we can do as you
suggest and focus on just getting this driver merged in with the
functionality it can for now.

After it's accepted and merged, we can turn our attention to the new
extension features such as the timestamps buffer return. This will make
it easier for us to discuss ideas since we won't have to worry about
merging in nonrelated functionality.

William Breathitt Gray

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[Felipe Balbi]

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


Hi,

William Breathitt Gray <vilhelm.gray@gmail.com> writes:
>> William Breathitt Gray <vilhelm.gray@gmail.com> writes:
>> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
>> >> Add support for Intel PSE Quadrature Encoder
>> >> 
>> >> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> >> ---
>> >> 
>> >> Changes since v1:
>> >> 	- Many more private sysfs files converted over to counter interface
>> >> 
>> >> 
>> >> How do you want me to model this device's Capture Compare Mode (see
>> >> below)?
>> >
>> > Hi Felipe,
>> >
>> > I'm CCing Fabien and David as they may be interested in the timestamps
>> > discussion. See below for some ideas I have on implementing this.
>> >
>> >> For the few features which I couldn't find a matching property in
>> >> counter framework, I still leave them as private sysfs files so we can
>> >> discuss how to model them in the framework.
>> >> 
>> >> Do you want Capture Compare to be a new function mode?
>> >> 
>> >> BTW, I know I'm missing a Documentation file documenting sysfs files
>> >> introduced by this driver, I'll do that once we agree how to move all
>> >> other sysfs files to the framework. No worries.
>> >> 
>> >> 
>> >> Details about the controller (do you want this in commit log?):
>> >> 
>> >> 
>> >> Controller has 2 modes of operation: QEP and Capture. Both modes are
>> >> mutually exclusive. We also have a set of maskable interrupts. Further
>> >> details about each mode below.
>> >
>> > I noticed your interrupt handler takes care of a number of different
>> > scenarios. Would you be able to summarize a bit further here the
>> > conditions for this device that cause an interrupt to be fired (watchdog
>> > timeout, FIFO updates, etc.)?
>> >
>> >> Quadrature Encoder Mode
>> >> =======================
>> >> 
>> >> Used to measure rotational speed, direction and angle of rotation of a
>> >> motor shaft. Feature list:
>> >> 
>> >> 	- Quadrature decoder providing counter pulses with x4 count
>> >> 	  resolution and count direction
>> >> 
>> >> 	- 32-bit up/down Position Counter for position measurement
>> >> 
>> >> 	- Two modes of position counter reset:
>> >> 		> Maximum Count (ceiling) to reset the position counter
>> >> 		> Index pulse to reset the position counter
>> >> 
>> >> 	- Watchdog timer functionality for detecting ‘stall’ events
>> >> 
>> >> Capture Compare Mode
>> >> ====================
>> >> 
>> >> Used to measure phase input signal Period or Duty Cycle. Feature List:
>> >> 
>> >> 	- Capture function operates either on phase A or phase B input
>> >> 	  and can be configured to operate on lo/hi or hi/lo or both hi
>> >> 	  and lo transitions.
>> >> 
>> >> 	- Free-running 32-bit counter to be configured to run at greater
>> >>           than or equal to 4x input signal frequency
>> >
>> > So in "Capture Compare" mode, the counter value is just increasing when
>> > a state condition transition occurs. In that case we won't need a new
>> > function mode to represent this behavior since one already exists:
>> > "increase".
>> >
>> > You can add it to your intel_qep_count_functions array like so:
>> >
>> >         [INTEL_QEP_ENCODER_MODE_CAPTURE] =
>> >         COUNTER_COUNT_FUNCTION_INCREASE,
>> >
>> > The various configurations for this mode are just Synapse action modes.
>> > If you want only Phase A, you would set the action mode for Phase A
>> > ("rising edge", "falling edge", or "both edges") and change the action
>> > mode for Phase B to "none"; vice-versa configuration for Phase B instead
>> > of Phase A.
>> >
>> > One thing to keep in mind is that action_set will need to maintain valid
>> > configurations -- so if the user tries to set the action mode for Phase
>> > A to something other than "none", you need to automatically set Phase
>> > B's action mode to "none" (and vice-versa).
>> 
>> interesting, thanks
>> 
>> >> 	- Clock post-scaler to derive the counter clock source from the
>> >> 	  peripheral clock
>> >
>> > I see you already have a "prescaler" extension in your code. Is this
>> > different from the "post-scaler" you mentioned here?
>> 
>> This was probably a brain-fart on my side. It should be post-scaler, but
>> that's only valid for capture compare mode.
>
> In that case you're implementation seems fine for this; perhaps a more
> generic name for that extension might be better such as "scale", but
> I'll decide later.
>  
>> >> 	- 32B wide FIFO to capture 32-bit timestamps of up to 8
>> >> 	  transition events
>> >
>> > You can implement this as a Count extension called "timestamps" or
>> > similar. What we can do is have a read on this attribute return the
>> > entire FIFO data buffer as unsigned integers, where each timestamp is
>> > deliminated by a space.
>> >
>> > In addition, it may be useful to have another extension called
>> > "timestamps_layout", or something along those lines, that will report
>> > the ordering of the buffer (i.e. whether it's "fifo", "lifo", etc.).
>> >
>> > Would this design work for your needs?
>> 
>> Perhaps it would be best to have a pollable binary sysfs file (meaning,
>> we need to be able to call sysfs_notify() at will) and userspace just
>> receives POLLIN whenever there's data read. Then a read returns an array
>> of e.g. struct counter_event where struct counter_event could be defined
>> as:
>> 
>> 	struct counter_event {
>>         	uint32_t	event_type;
>> 		struct timespec64 timestamp;
>>                 uint8_t		reserved[32];
>>         };
>> 
>> Userspace would do something along the lines of:
>> 
>> 	fd = open("/sys/bus/counter/foo/capture/timestamps",...);
>> 	pollfd[0].fd = fd;
>>         pollfd[0].events = POLLIN;
>>         poll(pollfd, 1, -1);
>> 
>> 	if (pollfd[0].revents & POLLIN) {
>>         	ret = read(fd, events, sizeof(struct counter_event) * 8);
>> 
>> 		for (i = 0; i < ret / sizeof(struct counter_event); i++)
>> 			process_event(events[i]);
>>         }
>>         
>> Or something like that.
>
> One concern is that returning binary data like that isn't friendly for
> human interaction. However, alternatively printing a human-readable
> array would add latency for userspace software that has to interpret it,
> so that would a problem as well. This is something we'll have to think
> more about then.
>
>> I could, also, remove this part from the driver for now, so we can
>> discuss how the capture-compare buffer read would look like. At least we
>> could get QDP feature merged while we come to a consensus about capture
>> compare.
>> 
>> What do you think?
>> 
>> -- 
>> balbi
>
> That sounds like a good idea. Most of this driver can be implemented
> using the existing Counter subsystem components, so we can do as you
> suggest and focus on just getting this driver merged in with the
> functionality it can for now.
>
> After it's accepted and merged, we can turn our attention to the new
> extension features such as the timestamps buffer return. This will make
> it easier for us to discuss ideas since we won't have to worry about
> merging in nonrelated functionality.

That's great. I'll prepare a version removing capture compare support
and all the extra sysfs files. That could potentially be merged for v5.5
merge window.

In parallel, we discuss capture compare buffer and how to expose it.

cheers

-- 

balbi

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

Re: [RFC/PATCH 1/2] counter: add support for Quadrature x4 with swapped inputs

[William Breathitt Gray]

On Thu, Sep 19, 2019 at 11:03:04AM +0300, Felipe Balbi wrote:
> Some Quadrature Encoders can swap phase inputs A and B
> internally. This new function will allow drivers to configure input
> swap mode.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
>  drivers/counter/counter.c | 3 ++-
>  include/linux/counter.h   | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
> index 106bc7180cd8..b818ae9e85f2 100644
> --- a/drivers/counter/counter.c
> +++ b/drivers/counter/counter.c
> @@ -823,7 +823,8 @@ static const char *const counter_count_function_str[] = {
>  	[COUNTER_COUNT_FUNCTION_QUADRATURE_X1_B] = "quadrature x1 b",
>  	[COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A] = "quadrature x2 a",
>  	[COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B] = "quadrature x2 b",
> -	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4] = "quadrature x4"
> +	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4] = "quadrature x4",
> +	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED] = "quadrature x4 swapped"
>  };
>  
>  static ssize_t counter_function_show(struct device *dev,
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index a061cdcdef7c..860769250f89 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -170,7 +170,8 @@ enum counter_count_function {
>  	COUNTER_COUNT_FUNCTION_QUADRATURE_X1_B,
>  	COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A,
>  	COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B,
> -	COUNTER_COUNT_FUNCTION_QUADRATURE_X4
> +	COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
> +	COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED,
>  };
>  
>  /**
> -- 
> 2.23.0

Add in an additional patch to update the documentation files to list
this new mode.

Thanks,

William Breathitt Gray

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[David Lechner]

On 9/22/19 6:35 PM, William Breathitt Gray wrote:
> On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
>> Add support for Intel PSE Quadrature Encoder
>>
>> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> ---
>>
>> Changes since v1:
>> 	- Many more private sysfs files converted over to counter interface
>>
>>
>> How do you want me to model this device's Capture Compare Mode (see
>> below)?
> 
> Hi Felipe,
> 
> I'm CCing Fabien and David as they may be interested in the timestamps
> discussion. See below for some ideas I have on implementing this.
> 

Could be an interesting read (thread from my first counter driver):

https://lore.kernel.org/linux-iio/1b913919-beb9-34e7-d915-6bcc40eeee1d@lechnology.com/

What would be useful to me is something like the buffer feature in iio
where a timestamp is associated with a count and stored in a buffer so that
we can look at a window of all values recorded in the last 20ms. Being able
to access this via mmap would be very helpful for performance (running on
300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
unless it is a rare event, like a watchdog timeout.

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[William Breathitt Gray]

On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:
> On 9/22/19 6:35 PM, William Breathitt Gray wrote:
> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
> >> Add support for Intel PSE Quadrature Encoder
> >>
> >> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> >> ---
> >>
> >> Changes since v1:
> >> 	- Many more private sysfs files converted over to counter interface
> >>
> >>
> >> How do you want me to model this device's Capture Compare Mode (see
> >> below)?
> > 
> > Hi Felipe,
> > 
> > I'm CCing Fabien and David as they may be interested in the timestamps
> > discussion. See below for some ideas I have on implementing this.
> > 
> 
> Could be an interesting read (thread from my first counter driver):
> 
> https://lore.kernel.org/linux-iio/1b913919-beb9-34e7-d915-6bcc40eeee1d@lechnology.com/
> 
> What would be useful to me is something like the buffer feature in iio
> where a timestamp is associated with a count and stored in a buffer so that
> we can look at a window of all values recorded in the last 20ms. Being able
> to access this via mmap would be very helpful for performance (running on
> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
> unless it is a rare event, like a watchdog timeout.

I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
timestamps and buffers. I don't want to reinvent something that is
working well, so hopefully we can reuse the IIO timestamp design for the
Counter subsystem.

I would argue that a human-readable timestamps printout is useful for
certain applications (e.g. a tally counter attached to a fault line: a
human administrator will be able to review previous fault times).
However as you point out, a low latency operation is necessary for
performance critical applications.

Although you are correct that mmap is a good low latency operation to
get access to a timestamp buffer, I'm afraid giving direct access to
memory like that will lead to many incompatible representations of
timestamp data (e.g. variations in endianness, signedness, data size,
etc.). I would like a standardized representation for this data that
userspace applications can expect to receive and interpret, especially
when time is widely represented as an unsigned integer.

Felipe suggested the creation of a counter_event structure so that users
can poll on an attribute. This kind of behavior is useful for notifying
users of interrupts and other events, but I think we should restrict the
use of the read call on these sysfs attributes to just human-readable
data. Instead, perhaps ioctl calls can be used to facilitate binary data
transfers.

For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
that returns a counter_timestamps structure with a timestamps array
populated:

        struct counter_timestamps{
                size_t num_timestamps;
        	unsigned int *timestamps;
        }

That would allow quick access to the timestamps data, while also
restricting it to a standard representation that all userspace
applications can follow and interpret. In addition, this won't interfer
with polling, so users can still wait for an interrupt and then decide
whether they want to use the slower human-readable printout (via read)
or the faster binary data access (via ioctl).

William Breathitt Gray

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[Felipe Balbi]

Hi,

William Breathitt Gray <vilhelm.gray@gmail.com> writes:
> On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:
>> On 9/22/19 6:35 PM, William Breathitt Gray wrote:
>> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
>> >> Add support for Intel PSE Quadrature Encoder
>> >>
>> >> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>> >> ---
>> >>
>> >> Changes since v1:
>> >> 	- Many more private sysfs files converted over to counter interface
>> >>
>> >>
>> >> How do you want me to model this device's Capture Compare Mode (see
>> >> below)?
>> > 
>> > Hi Felipe,
>> > 
>> > I'm CCing Fabien and David as they may be interested in the timestamps
>> > discussion. See below for some ideas I have on implementing this.
>> > 
>> 
>> Could be an interesting read (thread from my first counter driver):
>> 
>> https://lore.kernel.org/linux-iio/1b913919-beb9-34e7-d915-6bcc40eeee1d@lechnology.com/
>> 
>> What would be useful to me is something like the buffer feature in iio
>> where a timestamp is associated with a count and stored in a buffer so that
>> we can look at a window of all values recorded in the last 20ms. Being able
>> to access this via mmap would be very helpful for performance (running on
>> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
>> unless it is a rare event, like a watchdog timeout.
>
> I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
> timestamps and buffers. I don't want to reinvent something that is
> working well, so hopefully we can reuse the IIO timestamp design for the
> Counter subsystem.
>
> I would argue that a human-readable timestamps printout is useful for
> certain applications (e.g. a tally counter attached to a fault line: a
> human administrator will be able to review previous fault times).
> However as you point out, a low latency operation is necessary for
> performance critical applications.
>
> Although you are correct that mmap is a good low latency operation to
> get access to a timestamp buffer, I'm afraid giving direct access to
> memory like that will lead to many incompatible representations of
> timestamp data (e.g. variations in endianness, signedness, data size,
> etc.). I would like a standardized representation for this data that
> userspace applications can expect to receive and interpret, especially
> when time is widely represented as an unsigned integer.
>
> Felipe suggested the creation of a counter_event structure so that users
> can poll on an attribute. This kind of behavior is useful for notifying
> users of interrupts and other events, but I think we should restrict the
> use of the read call on these sysfs attributes to just human-readable
> data. Instead, perhaps ioctl calls can be used to facilitate binary data
> transfers.
>
> For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
> that returns a counter_timestamps structure with a timestamps array
> populated:
>
>         struct counter_timestamps{
>                 size_t num_timestamps;
>         	unsigned int *timestamps;
>         }
>
> That would allow quick access to the timestamps data, while also
> restricting it to a standard representation that all userspace
> applications can follow and interpret. In addition, this won't interfer
> with polling, so users can still wait for an interrupt and then decide
> whether they want to use the slower human-readable printout (via read)
> or the faster binary data access (via ioctl).

Seems like we're starting to build the need for a /dev/counter[0123...]
representation of the subsystem. If that's the case, then it may very
well be that sysfs becomes somewhat optional.

I think is makes sense to rely more on character devices specially since
I know of devices running linux with so little memory that sysfs (and a
bunch of other features) are removed from the kernel. Having a character
device representation would allow counter subsystem to be used on such
devices.

cheers

-- 
balbi

[PATCH v3 1/2] counter: add support for Quadrature x4 with swapped inputs

[Felipe Balbi]

Some Quadrature Encoders can swap phase inputs A and B
internally. This new function will allow drivers to configure input
swap mode.

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
 Documentation/ABI/testing/sysfs-bus-counter | 4 ++++
 drivers/counter/counter.c                   | 3 ++-
 include/linux/counter.h                     | 3 ++-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 566bd99fe0a5..8f1e3de88c77 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -146,6 +146,10 @@ Description:
 			updates	the respective count. Quadrature encoding
 			determines the direction.
 
+		quadrature x4 swapped:
+			Same as quadrature x4, however Phase A and Phase B
+			signals are swapped.
+
 What:		/sys/bus/counter/devices/counterX/countY/name
 KernelVersion:	5.2
 Contact:	linux-iio@vger.kernel.org
diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
index 106bc7180cd8..b818ae9e85f2 100644
--- a/drivers/counter/counter.c
+++ b/drivers/counter/counter.c
@@ -823,7 +823,8 @@ static const char *const counter_count_function_str[] = {
 	[COUNTER_COUNT_FUNCTION_QUADRATURE_X1_B] = "quadrature x1 b",
 	[COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A] = "quadrature x2 a",
 	[COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B] = "quadrature x2 b",
-	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4] = "quadrature x4"
+	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4] = "quadrature x4",
+	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED] = "quadrature x4 swapped"
 };
 
 static ssize_t counter_function_show(struct device *dev,
diff --git a/include/linux/counter.h b/include/linux/counter.h
index a061cdcdef7c..860769250f89 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -170,7 +170,8 @@ enum counter_count_function {
 	COUNTER_COUNT_FUNCTION_QUADRATURE_X1_B,
 	COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A,
 	COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B,
-	COUNTER_COUNT_FUNCTION_QUADRATURE_X4
+	COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
+	COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED,
 };
 
 /**
-- 
2.23.0

[PATCH v3 2/2] counter: introduce support for Intel QEP Encoder

[Felipe Balbi]

Add support for Intel PSE Quadrature Encoder

Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
---
Changes since v1:
	- Many more private sysfs files converted over to counter interface

Changes since v2:
	- Completed conversion to counter framework
	- Removed Capture Compare for now
	- Removed RFC from subject

 drivers/counter/Kconfig     |   6 +
 drivers/counter/Makefile    |   1 +
 drivers/counter/intel-qep.c | 689 ++++++++++++++++++++++++++++++++++++
 3 files changed, 696 insertions(+)
 create mode 100644 drivers/counter/intel-qep.c

diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2967d0a9ff91..f280cd721350 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -59,4 +59,10 @@ config FTM_QUADDEC
 	  To compile this driver as a module, choose M here: the
 	  module will be called ftm-quaddec.
 
+config INTEL_QEP
+	tristate "Intel Quadrature Encoder"
+	depends on PCI
+	help
+	  Support for Intel Quadrature Encoder Devices
+
 endif # COUNTER
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 40d35522937d..cf291cfd8cf0 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
 obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
+obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
new file mode 100644
index 000000000000..fa410a333b05
--- /dev/null
+++ b/drivers/counter/intel-qep.c
@@ -0,0 +1,689 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * intel-qep.c - Intel Quadrature Encoder Driver
+ *
+ * Copyright (C) 2019 Intel Corporation - https://www.intel.com
+ *
+ * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
+ */
+#include <linux/bitops.h>
+#include <linux/counter.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/pm_runtime.h>
+#include <linux/sysfs.h>
+
+#define INTEL_QEPCON		0x00
+#define INTEL_QEPFLT		0x04
+#define INTEL_QEPCOUNT		0x08
+#define INTEL_QEPMAX		0x0c
+#define INTEL_QEPWDT		0x10
+#define INTEL_QEPCAPDIV		0x14
+#define INTEL_QEPCNTR		0x18
+#define INTEL_QEPCAPBUF		0x1c
+#define INTEL_QEPINT_STAT	0x20
+#define INTEL_QEPINT_MASK	0x24
+
+/* QEPCON */
+#define INTEL_QEPCON_EN		BIT(0)
+#define INTEL_QEPCON_FLT_EN	BIT(1)
+#define INTEL_QEPCON_EDGE_A	BIT(2)
+#define INTEL_QEPCON_EDGE_B	BIT(3)
+#define INTEL_QEPCON_EDGE_INDX	BIT(4)
+#define INTEL_QEPCON_SWPAB	BIT(5)
+#define INTEL_QEPCON_OP_MODE	BIT(6)
+#define INTEL_QEPCON_PH_ERR	BIT(7)
+#define INTEL_QEPCON_COUNT_RST_MODE BIT(8)
+#define INTEL_QEPCON_INDX_GATING_MASK GENMASK(10, 9)
+#define INTEL_QEPCON_INDX_GATING(n) (((n) & 3) << 9)
+#define INTEL_QEPCON_INDX_PAL_PBL INTEL_QEPCON_INDX_GATING(0)
+#define INTEL_QEPCON_INDX_PAL_PBH INTEL_QEPCON_INDX_GATING(1)
+#define INTEL_QEPCON_INDX_PAH_PBL INTEL_QEPCON_INDX_GATING(2)
+#define INTEL_QEPCON_INDX_PAH_PBH INTEL_QEPCON_INDX_GATING(3)
+#define INTEL_QEPCON_CAP_MODE	BIT(11)
+#define INTEL_QEPCON_FIFO_THRE_MASK GENMASK(14, 12)
+#define INTEL_QEPCON_FIFO_THRE(n) ((((n) - 1) & 7) << 12)
+#define INTEL_QEPCON_FIFO_EMPTY	BIT(15)
+
+/* QEPFLT */
+#define INTEL_QEPFLT_MAX_COUNT(n) ((n) & 0x1fffff)
+
+/* QEPINT */
+#define INTEL_QEPINT_FIFOCRIT	BIT(5)
+#define INTEL_QEPINT_FIFOENTRY	BIT(4)
+#define INTEL_QEPINT_QEPDIR	BIT(3)
+#define INTEL_QEPINT_QEPRST_UP	BIT(2)
+#define INTEL_QEPINT_QEPRST_DOWN BIT(1)
+#define INTEL_QEPINT_WDT	BIT(0)
+
+#define INTEL_QEP_DIRECTION_FORWARD 1
+#define INTEL_QEP_DIRECTION_BACKWARD !INTEL_QEP_DIRECTION_FORWARD
+
+#define INTEL_QEP_COUNTER_EXT_RW(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+	.write = _name##_write, \
+}
+
+#define INTEL_QEP_COUNTER_EXT_RO(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+}
+
+#define INTEL_QEP_COUNTER_COUNT_EXT_RW(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+	.write = _name##_write, \
+}
+
+#define INTEL_QEP_COUNTER_COUNT_EXT_RO(_name) \
+{ \
+	.name = #_name, \
+	.read = _name##_read, \
+}
+
+struct intel_qep {
+	struct counter_device counter;
+	struct mutex lock;
+	struct pci_dev *pci;
+	struct device *dev;
+	void __iomem *regs;
+	u32 interrupt;
+	int direction;
+	bool enabled;
+};
+
+#define counter_to_qep(c)	(container_of((c), struct intel_qep, counter))
+
+static inline u32 intel_qep_readl(void __iomem *base, u32 offset)
+{
+	return readl(base + offset);
+}
+
+static inline void intel_qep_writel(void __iomem *base, u32 offset, u32 value)
+{
+	writel(value, base + offset);
+}
+
+static const struct pci_device_id intel_qep_id_table[] = {
+	/* EHL */
+	{ PCI_VDEVICE(INTEL, 0x4bc3), },
+	{ PCI_VDEVICE(INTEL, 0x4b81), },
+	{ PCI_VDEVICE(INTEL, 0x4b82), },
+	{ PCI_VDEVICE(INTEL, 0x4b83), },
+	{  } /* Terminating Entry */
+};
+MODULE_DEVICE_TABLE(pci, intel_qep_id_table);
+
+static void intel_qep_init(struct intel_qep *qep, bool reset)
+{
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	reg &= ~INTEL_QEPCON_EN;
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	/* make sure periperal is disabled by reading one more time */
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (reset) {
+		reg &= ~(INTEL_QEPCON_OP_MODE | INTEL_QEPCON_FLT_EN);
+		reg |= INTEL_QEPCON_EDGE_A | INTEL_QEPCON_EDGE_B |
+			INTEL_QEPCON_EDGE_INDX | INTEL_QEPCON_COUNT_RST_MODE;
+	}
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	intel_qep_writel(qep->regs, INTEL_QEPWDT, 0x1000);
+	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x0);
+
+	qep->direction = INTEL_QEP_DIRECTION_FORWARD;
+}
+
+static irqreturn_t intel_qep_irq_thread(int irq, void *_qep)
+{
+	struct intel_qep	*qep = _qep;
+	u32			stat;
+
+	mutex_lock(&qep->lock);
+
+	stat = qep->interrupt;
+	if (stat & INTEL_QEPINT_FIFOCRIT)
+		dev_dbg(qep->dev, "Fifo Critical\n");
+
+	if (stat & INTEL_QEPINT_FIFOENTRY)
+		dev_dbg(qep->dev, "Fifo Entry\n");
+
+	if (stat & INTEL_QEPINT_QEPDIR)
+		qep->direction = !qep->direction;
+
+	if (stat & INTEL_QEPINT_QEPRST_UP)
+		qep->direction = INTEL_QEP_DIRECTION_FORWARD;
+
+	if (stat & INTEL_QEPINT_QEPRST_DOWN)
+		qep->direction = INTEL_QEP_DIRECTION_BACKWARD;
+
+	if (stat & INTEL_QEPINT_WDT)
+		dev_dbg(qep->dev, "Watchdog\n");
+
+	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x00);
+	mutex_unlock(&qep->lock);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t intel_qep_irq(int irq, void *_qep)
+{
+	struct intel_qep	*qep = _qep;
+	u32			stat;
+
+	stat = intel_qep_readl(qep->regs, INTEL_QEPINT_STAT);
+	if (stat) {
+		qep->interrupt = stat;
+		intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0xffffffff);
+		intel_qep_writel(qep->regs, INTEL_QEPINT_STAT, stat);
+		return IRQ_WAKE_THREAD;
+	}
+
+	return IRQ_HANDLED;
+}
+
+enum intel_qep_synapse_action {
+	INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE,
+	INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE,
+};
+
+static enum counter_synapse_action intel_qep_synapse_actions[] = {
+	[INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE] =
+	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
+	
+	[INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE] =
+	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
+};
+
+enum intel_qep_count_function {
+	INTEL_QEP_ENCODER_MODE_NORMAL,
+	INTEL_QEP_ENCODER_MODE_SWAPPED,
+};
+
+static const enum counter_count_function intel_qep_count_functions[] = {
+	[INTEL_QEP_ENCODER_MODE_NORMAL] =
+	COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
+
+	[INTEL_QEP_ENCODER_MODE_SWAPPED] =
+	COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED,
+};
+
+static int intel_qep_count_read(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_count_read_value *val)
+{
+	struct intel_qep *const qep = counter->priv;
+	uint32_t cntval;
+
+	cntval = intel_qep_readl(qep, INTEL_QEPCOUNT);
+	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cntval);
+
+	return 0;
+}
+
+static int intel_qep_count_write(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_count_write_value *val)
+{
+	struct intel_qep *const qep = counter->priv;
+	u32 cnt;
+	int err;
+
+	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
+	if (err)
+		return err;
+
+	intel_qep_writel(qep->regs, INTEL_QEPMAX, cnt);
+
+	return 0;
+}
+
+static int intel_qep_function_get(struct counter_device *counter,
+		struct counter_count *count, size_t *function)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	if (reg & INTEL_QEPCON_SWPAB)
+		*function = INTEL_QEP_ENCODER_MODE_SWAPPED;
+	else
+		*function = INTEL_QEP_ENCODER_MODE_NORMAL;
+
+	return 0;
+}
+
+static int intel_qep_function_set(struct counter_device *counter,
+		struct counter_count *count, size_t function)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	if (function == INTEL_QEP_ENCODER_MODE_SWAPPED)
+		reg |= INTEL_QEPCON_SWPAB;
+	else
+		reg &= ~INTEL_QEPCON_SWPAB;
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return 0;
+}
+
+static int intel_qep_action_get(struct counter_device *counter,
+		struct counter_count *count, struct counter_synapse *synapse,
+		size_t *action)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	*action = reg & synapse->signal->id ?
+		INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE :
+		INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE;
+
+	return 0;
+}
+
+static int intel_qep_action_set(struct counter_device *counter,
+		struct counter_count *count,
+		struct counter_synapse *synapse, size_t action)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (action == INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE)
+		reg |= synapse->signal->id;
+	else
+		reg &= ~synapse->signal->id;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return 0;
+}
+
+static const struct counter_ops intel_qep_counter_ops = {
+	.count_read = intel_qep_count_read,
+	.count_write = intel_qep_count_write,
+
+	.function_get = intel_qep_function_get,
+	.function_set = intel_qep_function_set,
+
+	.action_get = intel_qep_action_get,
+	.action_set = intel_qep_action_set,
+};
+
+static struct counter_signal intel_qep_signals[] = {
+	{
+		.id = INTEL_QEPCON_EDGE_A,
+		.name = "Phase A",
+	},
+	{
+		.id = INTEL_QEPCON_EDGE_B,
+		.name = "Phase B",
+	},
+	{
+		.id = INTEL_QEPCON_EDGE_INDX,
+		.name = "Index",
+	},
+};
+
+static struct counter_synapse intel_qep_count_synapses[] = {
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[0],
+	},
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[1],
+	},
+	{
+		.actions_list = intel_qep_synapse_actions,
+		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
+		.signal = &intel_qep_signals[2],
+	},
+};
+
+static ssize_t ceiling_read(struct counter_device *counter,
+		struct counter_count *count, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPMAX);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", reg);
+}
+
+static ssize_t ceiling_write(struct counter_device *counter,
+		struct counter_count *count, void *priv, const char *buf,
+		size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 max;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &max);
+	if (ret < 0)
+		return ret;
+
+	intel_qep_writel(qep->regs, INTEL_QEPMAX, max);
+
+	return len;
+}
+
+static ssize_t enable_read(struct counter_device *counter,
+		struct counter_count *count, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", !!(reg & INTEL_QEPCON_EN));
+}
+
+static ssize_t enable_write(struct counter_device *counter,
+		struct counter_count *count, void *priv, const char *buf,
+		size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+	u32 val;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret < 0)
+		return ret;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (val)
+		reg |= INTEL_QEPCON_EN;
+	else
+		reg &= ~INTEL_QEPCON_EN;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+
+	return len;
+}
+
+static ssize_t direction_read(struct counter_device *counter,
+		struct counter_count *count, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+
+	return snprintf(buf, PAGE_SIZE, "%s\n", qep->direction ?
+			"forward" : "backward");
+}
+
+static const struct counter_count_ext intel_qep_count_ext[] = {
+	INTEL_QEP_COUNTER_COUNT_EXT_RW(ceiling),
+	INTEL_QEP_COUNTER_COUNT_EXT_RW(enable),
+	INTEL_QEP_COUNTER_COUNT_EXT_RO(direction),
+};
+
+static struct counter_count intel_qep_counter_count[] = {
+	{
+		.id = 0,
+		.name = "Channel 1 Count",
+		.functions_list = intel_qep_count_functions,
+		.num_functions = ARRAY_SIZE(intel_qep_count_functions),
+		.synapses = intel_qep_count_synapses,
+		.num_synapses = ARRAY_SIZE(intel_qep_count_synapses),
+		.ext = intel_qep_count_ext,
+		.num_ext = ARRAY_SIZE(intel_qep_count_ext),
+	},
+};
+
+static ssize_t noise_read(struct counter_device *counter, void *priv, char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (!(reg & INTEL_QEPCON_FLT_EN))
+		return snprintf(buf, PAGE_SIZE, "0\n");
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPFLT);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", INTEL_QEPFLT_MAX_COUNT(reg));
+}
+
+static ssize_t noise_write(struct counter_device *counter, void *priv,
+		const char *buf, size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+	u32 max;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &max);
+	if (ret < 0)
+		return ret;
+
+	if (max > 0x1fffff)
+		max = 0x1ffff;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (max == 0) {
+		reg &= ~INTEL_QEPCON_FLT_EN;
+	} else {
+		reg |= INTEL_QEPCON_FLT_EN;
+		intel_qep_writel(qep->regs, INTEL_QEPFLT, max);
+	}
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+	
+	return len;
+}
+
+static ssize_t preset_read(struct counter_device *counter, void *priv, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "0\n");
+}
+
+static ssize_t preset_enable_read(struct counter_device *counter, void *priv,
+		char *buf)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+			!(reg & INTEL_QEPCON_COUNT_RST_MODE));
+}
+
+static ssize_t preset_enable_write(struct counter_device *counter, void *priv,
+		const char *buf, size_t len)
+{
+	struct intel_qep *qep = counter_to_qep(counter);
+	u32 reg;
+	u32 val;
+	int ret;
+
+	ret = kstrtou32(buf, 0, &val);
+	if (ret < 0)
+		return ret;
+
+	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
+
+	if (val)
+		reg &= ~INTEL_QEPCON_COUNT_RST_MODE;
+	else
+		reg |= INTEL_QEPCON_COUNT_RST_MODE;
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
+	
+	return len;
+}
+
+static const struct counter_device_ext intel_qep_ext[] = {
+	INTEL_QEP_COUNTER_EXT_RW(noise),
+	INTEL_QEP_COUNTER_EXT_RO(preset),
+	INTEL_QEP_COUNTER_EXT_RW(preset_enable)
+};
+
+static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
+{
+	struct intel_qep	*qep;
+	struct device		*dev = &pci->dev;
+	void __iomem		*regs;
+	int			ret;
+	int			irq;
+
+	qep = devm_kzalloc(dev, sizeof(*qep), GFP_KERNEL);
+	if (!qep)
+		return -ENOMEM;
+
+	ret = pcim_enable_device(pci);
+	if (ret)
+		return ret;
+
+	pci_set_master(pci);
+
+	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
+	if (ret)
+		return ret;
+
+	regs = pcim_iomap_table(pci)[0];
+	if (!regs)
+		return -ENOMEM;
+
+	qep->pci = pci;
+	qep->dev = dev;
+	qep->regs = regs;
+	mutex_init(&qep->lock);
+
+	intel_qep_init(qep, true);
+	pci_set_drvdata(pci, qep);
+
+	qep->counter.name = pci_name(pci);
+	qep->counter.parent = dev;
+	qep->counter.ops = &intel_qep_counter_ops;
+	qep->counter.counts = intel_qep_counter_count;
+	qep->counter.num_counts = ARRAY_SIZE(intel_qep_counter_count);
+	qep->counter.signals = intel_qep_signals;
+	qep->counter.num_signals = ARRAY_SIZE(intel_qep_signals);
+	qep->counter.ext = intel_qep_ext;
+	qep->counter.num_ext = ARRAY_SIZE(intel_qep_ext);
+	qep->counter.priv = qep;
+
+	ret = counter_register(&qep->counter);
+	if (ret)
+		return ret;
+
+	ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		goto err_irq_vectors;
+
+	irq = pci_irq_vector(pci, 0);
+	ret = devm_request_threaded_irq(&pci->dev, irq, intel_qep_irq,
+			intel_qep_irq_thread, IRQF_SHARED | IRQF_TRIGGER_RISING,
+			"intel-qep", qep);
+	if (ret)
+		goto err_irq;
+
+	pm_runtime_set_autosuspend_delay(dev, 1000);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_allow(dev);
+
+	return 0;
+
+err_irq:
+	pci_free_irq_vectors(pci);
+
+err_irq_vectors:
+	counter_unregister(&qep->counter);
+
+	return ret;
+}
+
+static void intel_qep_remove(struct pci_dev *pci)
+{
+	struct intel_qep	*qep = pci_get_drvdata(pci);
+	struct device		*dev = &pci->dev;
+
+	pm_runtime_forbid(dev);
+	pm_runtime_get_noresume(dev);
+
+	intel_qep_writel(qep->regs, INTEL_QEPCON, 0);
+	pci_free_irq_vectors(pci);
+	counter_unregister(&qep->counter);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int intel_qep_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int intel_qep_resume(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct intel_qep *qep = pci_get_drvdata(pdev);
+
+	intel_qep_init(qep, false);
+
+	return 0;
+}
+
+static int intel_qep_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int intel_qep_runtime_resume(struct device *dev)
+{
+	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
+	struct intel_qep *qep = pci_get_drvdata(pdev);
+
+	intel_qep_init(qep, false);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops intel_qep_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(intel_qep_suspend,
+				intel_qep_resume)
+	SET_RUNTIME_PM_OPS(intel_qep_runtime_suspend, intel_qep_runtime_resume,
+				NULL)
+};
+
+static struct pci_driver intel_qep_driver = {
+	.name		= "intel-qep",
+	.id_table	= intel_qep_id_table,
+	.probe		= intel_qep_probe,
+	.remove		= intel_qep_remove,
+	.driver = {
+		.pm = &intel_qep_pm_ops,
+	}
+};
+
+module_pci_driver(intel_qep_driver);
+
+MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel Quadrature Encoder Driver");
-- 
2.23.0

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[William Breathitt Gray]

On Mon, Sep 30, 2019 at 08:22:39AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> William Breathitt Gray <vilhelm.gray@gmail.com> writes:
> > On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:
> >> On 9/22/19 6:35 PM, William Breathitt Gray wrote:
> >> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:
> >> >> Add support for Intel PSE Quadrature Encoder
> >> >>
> >> >> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> >> >> ---
> >> >>
> >> >> Changes since v1:
> >> >> 	- Many more private sysfs files converted over to counter interface
> >> >>
> >> >>
> >> >> How do you want me to model this device's Capture Compare Mode (see
> >> >> below)?
> >> > 
> >> > Hi Felipe,
> >> > 
> >> > I'm CCing Fabien and David as they may be interested in the timestamps
> >> > discussion. See below for some ideas I have on implementing this.
> >> > 
> >> 
> >> Could be an interesting read (thread from my first counter driver):
> >> 
> >> https://lore.kernel.org/linux-iio/1b913919-beb9-34e7-d915-6bcc40eeee1d@lechnology.com/
> >> 
> >> What would be useful to me is something like the buffer feature in iio
> >> where a timestamp is associated with a count and stored in a buffer so that
> >> we can look at a window of all values recorded in the last 20ms. Being able
> >> to access this via mmap would be very helpful for performance (running on
> >> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
> >> unless it is a rare event, like a watchdog timeout.
> >
> > I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
> > timestamps and buffers. I don't want to reinvent something that is
> > working well, so hopefully we can reuse the IIO timestamp design for the
> > Counter subsystem.
> >
> > I would argue that a human-readable timestamps printout is useful for
> > certain applications (e.g. a tally counter attached to a fault line: a
> > human administrator will be able to review previous fault times).
> > However as you point out, a low latency operation is necessary for
> > performance critical applications.
> >
> > Although you are correct that mmap is a good low latency operation to
> > get access to a timestamp buffer, I'm afraid giving direct access to
> > memory like that will lead to many incompatible representations of
> > timestamp data (e.g. variations in endianness, signedness, data size,
> > etc.). I would like a standardized representation for this data that
> > userspace applications can expect to receive and interpret, especially
> > when time is widely represented as an unsigned integer.
> >
> > Felipe suggested the creation of a counter_event structure so that users
> > can poll on an attribute. This kind of behavior is useful for notifying
> > users of interrupts and other events, but I think we should restrict the
> > use of the read call on these sysfs attributes to just human-readable
> > data. Instead, perhaps ioctl calls can be used to facilitate binary data
> > transfers.
> >
> > For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
> > that returns a counter_timestamps structure with a timestamps array
> > populated:
> >
> >         struct counter_timestamps{
> >                 size_t num_timestamps;
> >         	unsigned int *timestamps;
> >         }
> >
> > That would allow quick access to the timestamps data, while also
> > restricting it to a standard representation that all userspace
> > applications can follow and interpret. In addition, this won't interfer
> > with polling, so users can still wait for an interrupt and then decide
> > whether they want to use the slower human-readable printout (via read)
> > or the faster binary data access (via ioctl).
> 
> Seems like we're starting to build the need for a /dev/counter[0123...]
> representation of the subsystem. If that's the case, then it may very
> well be that sysfs becomes somewhat optional.
> 
> I think is makes sense to rely more on character devices specially since
> I know of devices running linux with so little memory that sysfs (and a
> bunch of other features) are removed from the kernel. Having a character
> device representation would allow counter subsystem to be used on such
> devices.
> 
> cheers
> 
> -- 
> balbi

A character device node for a counter might be a good idea. If a
performance critical application can't depend on parsing a sysfs
printout for timestamps, then it probably doesn't want to do so for the
other attributes either. I think you are right that certain systems
would have sysfs disabled for that very reason.

I think latency concerns are the same reason the GPIO subsystem started
providing character device nodes as well. We can do similar with the
Counter subsystem: provide character device nodes by default, and
optionally provide the human-readable sysfs interface as well. This
would allow applications with latency concerns to use a standard
interface for the Counter subsystem, while optionally providing a
simpler sysfs interface for other users.

William Breathitt Gray

Re: [PATCH v3 1/2] counter: add support for Quadrature x4 with swapped inputs

[William Breathitt Gray]

On Tue, Oct 01, 2019 at 12:32:36PM +0300, Felipe Balbi wrote:
> Some Quadrature Encoders can swap phase inputs A and B
> internally. This new function will allow drivers to configure input
> swap mode.
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-counter | 4 ++++
>  drivers/counter/counter.c                   | 3 ++-
>  include/linux/counter.h                     | 3 ++-
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 566bd99fe0a5..8f1e3de88c77 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -146,6 +146,10 @@ Description:
>  			updates	the respective count. Quadrature encoding
>  			determines the direction.
>  
> +		quadrature x4 swapped:
> +			Same as quadrature x4, however Phase A and Phase B
> +			signals are swapped.
> +
>  What:		/sys/bus/counter/devices/counterX/countY/name
>  KernelVersion:	5.2
>  Contact:	linux-iio@vger.kernel.org

Thank you for adding an entry for this in the Counter sysfs interface
documentation file. Make a similar addition to the Counter driver API
documentation in the Documentation/driver-api/generic-counter.rst file.

> diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
> index 106bc7180cd8..b818ae9e85f2 100644
> --- a/drivers/counter/counter.c
> +++ b/drivers/counter/counter.c
> @@ -823,7 +823,8 @@ static const char *const counter_count_function_str[] = {
>  	[COUNTER_COUNT_FUNCTION_QUADRATURE_X1_B] = "quadrature x1 b",
>  	[COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A] = "quadrature x2 a",
>  	[COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B] = "quadrature x2 b",
> -	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4] = "quadrature x4"
> +	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4] = "quadrature x4",
> +	[COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED] = "quadrature x4 swapped"

Add a comma to the end of this line so that we don't have to change it
if we add another function mode in the future.

William Breathitt Gray

>  };
>  
>  static ssize_t counter_function_show(struct device *dev,
> diff --git a/include/linux/counter.h b/include/linux/counter.h
> index a061cdcdef7c..860769250f89 100644
> --- a/include/linux/counter.h
> +++ b/include/linux/counter.h
> @@ -170,7 +170,8 @@ enum counter_count_function {
>  	COUNTER_COUNT_FUNCTION_QUADRATURE_X1_B,
>  	COUNTER_COUNT_FUNCTION_QUADRATURE_X2_A,
>  	COUNTER_COUNT_FUNCTION_QUADRATURE_X2_B,
> -	COUNTER_COUNT_FUNCTION_QUADRATURE_X4
> +	COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
> +	COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED,
>  };
>  
>  /**
> -- 
> 2.23.0
> 

Re: [PATCH v3 2/2] counter: introduce support for Intel QEP Encoder

[William Breathitt Gray]

On Tue, Oct 01, 2019 at 12:32:37PM +0300, Felipe Balbi wrote:
> Add support for Intel PSE Quadrature Encoder
> 
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
> Changes since v1:
> 	- Many more private sysfs files converted over to counter interface
> 
> Changes since v2:
> 	- Completed conversion to counter framework
> 	- Removed Capture Compare for now
> 	- Removed RFC from subject
> 
>  drivers/counter/Kconfig     |   6 +
>  drivers/counter/Makefile    |   1 +
>  drivers/counter/intel-qep.c | 689 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 696 insertions(+)
>  create mode 100644 drivers/counter/intel-qep.c

Hi Felipe,

Make sure to add an entry for this driver in the top-level MAINTAINERS
file so that we have a way to contact the maintainer in the future.

> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 2967d0a9ff91..f280cd721350 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -59,4 +59,10 @@ config FTM_QUADDEC
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ftm-quaddec.
>  
> +config INTEL_QEP
> +	tristate "Intel Quadrature Encoder"
> +	depends on PCI
> +	help
> +	  Support for Intel Quadrature Encoder Devices

This help text seems rather short -- are you able to expand it a bit
further? The current description here gives me the impression that this
is a generic driver for all Intel quadrature encoder devices in
existence, but I suspect the Intel QEP counter driver has a much
narrower list of devices it supports; perhaps you can list those support
device models here.

> +
>  endif # COUNTER
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index 40d35522937d..cf291cfd8cf0 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>  obj-$(CONFIG_STM32_TIMER_CNT)	+= stm32-timer-cnt.o
>  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
>  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
> +obj-$(CONFIG_INTEL_QEP)		+= intel-qep.o
> diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
> new file mode 100644
> index 000000000000..fa410a333b05
> --- /dev/null
> +++ b/drivers/counter/intel-qep.c
> @@ -0,0 +1,689 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * intel-qep.c - Intel Quadrature Encoder Driver
> + *
> + * Copyright (C) 2019 Intel Corporation - https://www.intel.com
> + *
> + * Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> + */
> +#include <linux/bitops.h>
> +#include <linux/counter.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/sysfs.h>

Is the sysfs.h file include needed anymore?

> +
> +#define INTEL_QEPCON		0x00
> +#define INTEL_QEPFLT		0x04
> +#define INTEL_QEPCOUNT		0x08
> +#define INTEL_QEPMAX		0x0c
> +#define INTEL_QEPWDT		0x10
> +#define INTEL_QEPCAPDIV		0x14
> +#define INTEL_QEPCNTR		0x18
> +#define INTEL_QEPCAPBUF		0x1c
> +#define INTEL_QEPINT_STAT	0x20
> +#define INTEL_QEPINT_MASK	0x24
> +
> +/* QEPCON */
> +#define INTEL_QEPCON_EN		BIT(0)
> +#define INTEL_QEPCON_FLT_EN	BIT(1)
> +#define INTEL_QEPCON_EDGE_A	BIT(2)
> +#define INTEL_QEPCON_EDGE_B	BIT(3)
> +#define INTEL_QEPCON_EDGE_INDX	BIT(4)
> +#define INTEL_QEPCON_SWPAB	BIT(5)
> +#define INTEL_QEPCON_OP_MODE	BIT(6)
> +#define INTEL_QEPCON_PH_ERR	BIT(7)
> +#define INTEL_QEPCON_COUNT_RST_MODE BIT(8)
> +#define INTEL_QEPCON_INDX_GATING_MASK GENMASK(10, 9)
> +#define INTEL_QEPCON_INDX_GATING(n) (((n) & 3) << 9)
> +#define INTEL_QEPCON_INDX_PAL_PBL INTEL_QEPCON_INDX_GATING(0)
> +#define INTEL_QEPCON_INDX_PAL_PBH INTEL_QEPCON_INDX_GATING(1)
> +#define INTEL_QEPCON_INDX_PAH_PBL INTEL_QEPCON_INDX_GATING(2)
> +#define INTEL_QEPCON_INDX_PAH_PBH INTEL_QEPCON_INDX_GATING(3)
> +#define INTEL_QEPCON_CAP_MODE	BIT(11)
> +#define INTEL_QEPCON_FIFO_THRE_MASK GENMASK(14, 12)
> +#define INTEL_QEPCON_FIFO_THRE(n) ((((n) - 1) & 7) << 12)
> +#define INTEL_QEPCON_FIFO_EMPTY	BIT(15)
> +
> +/* QEPFLT */
> +#define INTEL_QEPFLT_MAX_COUNT(n) ((n) & 0x1fffff)
> +
> +/* QEPINT */
> +#define INTEL_QEPINT_FIFOCRIT	BIT(5)
> +#define INTEL_QEPINT_FIFOENTRY	BIT(4)
> +#define INTEL_QEPINT_QEPDIR	BIT(3)
> +#define INTEL_QEPINT_QEPRST_UP	BIT(2)
> +#define INTEL_QEPINT_QEPRST_DOWN BIT(1)
> +#define INTEL_QEPINT_WDT	BIT(0)
> +
> +#define INTEL_QEP_DIRECTION_FORWARD 1
> +#define INTEL_QEP_DIRECTION_BACKWARD !INTEL_QEP_DIRECTION_FORWARD
> +
> +#define INTEL_QEP_COUNTER_EXT_RW(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +	.write = _name##_write, \
> +}
> +
> +#define INTEL_QEP_COUNTER_EXT_RO(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +}
> +
> +#define INTEL_QEP_COUNTER_COUNT_EXT_RW(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +	.write = _name##_write, \
> +}
> +
> +#define INTEL_QEP_COUNTER_COUNT_EXT_RO(_name) \
> +{ \
> +	.name = #_name, \
> +	.read = _name##_read, \
> +}

You're using the same naming convention for all your extensions, so we
can reduce some complexity here by removing one pair of defines such as
INTEL_QEP_COUNTER_COUNT_EXT_RW/INTEL_QEP_COUNTER_COUNT_EXT_RO and just
use INTEL_QEP_COUNTER_EXT_RW/INTEL_QEP_COUNTER_EXT_RO instead for all
extensions. If you start using a different naming convention for Count
extensions versus device extensions, then you can separate them out with
dedicated defines.

> +
> +struct intel_qep {
> +	struct counter_device counter;
> +	struct mutex lock;
> +	struct pci_dev *pci;
> +	struct device *dev;
> +	void __iomem *regs;
> +	u32 interrupt;
> +	int direction;
> +	bool enabled;
> +};
> +
> +#define counter_to_qep(c)	(container_of((c), struct intel_qep, counter))

This macro isn't needed since you set the counter_device priv member to
your intel_qep structure in the intel_qep_probe. What you can do instead
is simply define a pointer directly:

        struct intel_qep *const qep = counter->priv;

You do so already in several of your main callbacks, so it seems like
the counter_to_qep macro use in the extension callbacks are just
leftovers from when you converted the extensions over from direct sysfs
callbacks.

> +
> +static inline u32 intel_qep_readl(void __iomem *base, u32 offset)
> +{
> +	return readl(base + offset);
> +}
> +
> +static inline void intel_qep_writel(void __iomem *base, u32 offset, u32 value)
> +{
> +	writel(value, base + offset);
> +}
> +
> +static const struct pci_device_id intel_qep_id_table[] = {
> +	/* EHL */
> +	{ PCI_VDEVICE(INTEL, 0x4bc3), },
> +	{ PCI_VDEVICE(INTEL, 0x4b81), },
> +	{ PCI_VDEVICE(INTEL, 0x4b82), },
> +	{ PCI_VDEVICE(INTEL, 0x4b83), },
> +	{  } /* Terminating Entry */
> +};
> +MODULE_DEVICE_TABLE(pci, intel_qep_id_table);
> +
> +static void intel_qep_init(struct intel_qep *qep, bool reset)
> +{
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	reg &= ~INTEL_QEPCON_EN;
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	/* make sure periperal is disabled by reading one more time */
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (reset) {
> +		reg &= ~(INTEL_QEPCON_OP_MODE | INTEL_QEPCON_FLT_EN);
> +		reg |= INTEL_QEPCON_EDGE_A | INTEL_QEPCON_EDGE_B |
> +			INTEL_QEPCON_EDGE_INDX | INTEL_QEPCON_COUNT_RST_MODE;
> +	}
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPWDT, 0x1000);
> +	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x0);
> +
> +	qep->direction = INTEL_QEP_DIRECTION_FORWARD;
> +}
> +
> +static irqreturn_t intel_qep_irq_thread(int irq, void *_qep)
> +{
> +	struct intel_qep	*qep = _qep;
> +	u32			stat;
> +
> +	mutex_lock(&qep->lock);
> +
> +	stat = qep->interrupt;
> +	if (stat & INTEL_QEPINT_FIFOCRIT)
> +		dev_dbg(qep->dev, "Fifo Critical\n");
> +
> +	if (stat & INTEL_QEPINT_FIFOENTRY)
> +		dev_dbg(qep->dev, "Fifo Entry\n");
> +
> +	if (stat & INTEL_QEPINT_QEPDIR)
> +		qep->direction = !qep->direction;
> +
> +	if (stat & INTEL_QEPINT_QEPRST_UP)
> +		qep->direction = INTEL_QEP_DIRECTION_FORWARD;
> +
> +	if (stat & INTEL_QEPINT_QEPRST_DOWN)
> +		qep->direction = INTEL_QEP_DIRECTION_BACKWARD;
> +
> +	if (stat & INTEL_QEPINT_WDT)
> +		dev_dbg(qep->dev, "Watchdog\n");
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0x00);
> +	mutex_unlock(&qep->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t intel_qep_irq(int irq, void *_qep)
> +{
> +	struct intel_qep	*qep = _qep;
> +	u32			stat;
> +
> +	stat = intel_qep_readl(qep->regs, INTEL_QEPINT_STAT);
> +	if (stat) {
> +		qep->interrupt = stat;
> +		intel_qep_writel(qep->regs, INTEL_QEPINT_MASK, 0xffffffff);
> +		intel_qep_writel(qep->regs, INTEL_QEPINT_STAT, stat);
> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +enum intel_qep_synapse_action {
> +	INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE,
> +	INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE,
> +};
> +
> +static enum counter_synapse_action intel_qep_synapse_actions[] = {
> +	[INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE] =
> +	COUNTER_SYNAPSE_ACTION_RISING_EDGE,
> +	

Trim the trailing whitespace here.

> +	[INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE] =
> +	COUNTER_SYNAPSE_ACTION_FALLING_EDGE,
> +};
> +
> +enum intel_qep_count_function {
> +	INTEL_QEP_ENCODER_MODE_NORMAL,
> +	INTEL_QEP_ENCODER_MODE_SWAPPED,
> +};
> +
> +static const enum counter_count_function intel_qep_count_functions[] = {
> +	[INTEL_QEP_ENCODER_MODE_NORMAL] =
> +	COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
> +
> +	[INTEL_QEP_ENCODER_MODE_SWAPPED] =
> +	COUNTER_COUNT_FUNCTION_QUADRATURE_X4_SWAPPED,
> +};
> +
> +static int intel_qep_count_read(struct counter_device *counter,
> +		struct counter_count *count,
> +		struct counter_count_read_value *val)
> +{
> +	struct intel_qep *const qep = counter->priv;
> +	uint32_t cntval;
> +
> +	cntval = intel_qep_readl(qep, INTEL_QEPCOUNT);
> +	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cntval);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_count_write(struct counter_device *counter,
> +		struct counter_count *count,
> +		struct counter_count_write_value *val)
> +{
> +	struct intel_qep *const qep = counter->priv;
> +	u32 cnt;
> +	int err;
> +
> +	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
> +	if (err)
> +		return err;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPMAX, cnt);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_function_get(struct counter_device *counter,
> +		struct counter_count *count, size_t *function)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	if (reg & INTEL_QEPCON_SWPAB)
> +		*function = INTEL_QEP_ENCODER_MODE_SWAPPED;
> +	else
> +		*function = INTEL_QEP_ENCODER_MODE_NORMAL;
> +
> +	return 0;
> +}
> +
> +static int intel_qep_function_set(struct counter_device *counter,
> +		struct counter_count *count, size_t function)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	if (function == INTEL_QEP_ENCODER_MODE_SWAPPED)
> +		reg |= INTEL_QEPCON_SWPAB;
> +	else
> +		reg &= ~INTEL_QEPCON_SWPAB;
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_action_get(struct counter_device *counter,
> +		struct counter_count *count, struct counter_synapse *synapse,
> +		size_t *action)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	*action = reg & synapse->signal->id ?
> +		INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE :
> +		INTEL_QEP_SYNAPSE_ACTION_FALLING_EDGE;
> +
> +	return 0;
> +}
> +
> +static int intel_qep_action_set(struct counter_device *counter,
> +		struct counter_count *count,
> +		struct counter_synapse *synapse, size_t action)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (action == INTEL_QEP_SYNAPSE_ACTION_RISING_EDGE)
> +		reg |= synapse->signal->id;
> +	else
> +		reg &= ~synapse->signal->id;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return 0;
> +}
> +
> +static const struct counter_ops intel_qep_counter_ops = {
> +	.count_read = intel_qep_count_read,
> +	.count_write = intel_qep_count_write,
> +
> +	.function_get = intel_qep_function_get,
> +	.function_set = intel_qep_function_set,
> +
> +	.action_get = intel_qep_action_get,
> +	.action_set = intel_qep_action_set,
> +};
> +
> +static struct counter_signal intel_qep_signals[] = {
> +	{
> +		.id = INTEL_QEPCON_EDGE_A,
> +		.name = "Phase A",
> +	},
> +	{
> +		.id = INTEL_QEPCON_EDGE_B,
> +		.name = "Phase B",
> +	},
> +	{
> +		.id = INTEL_QEPCON_EDGE_INDX,
> +		.name = "Index",
> +	},
> +};
> +
> +static struct counter_synapse intel_qep_count_synapses[] = {
> +	{
> +		.actions_list = intel_qep_synapse_actions,
> +		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
> +		.signal = &intel_qep_signals[0],
> +	},
> +	{
> +		.actions_list = intel_qep_synapse_actions,
> +		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
> +		.signal = &intel_qep_signals[1],
> +	},
> +	{
> +		.actions_list = intel_qep_synapse_actions,
> +		.num_actions = ARRAY_SIZE(intel_qep_synapse_actions),
> +		.signal = &intel_qep_signals[2],
> +	},
> +};
> +
> +static ssize_t ceiling_read(struct counter_device *counter,
> +		struct counter_count *count, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPMAX);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", reg);

Normally it's good to make sure you don't write over the PAGE_SIZE
limit, but this check isn't necessary here in the Counter extension
callbacks; the PAGE_SIZE check for buf will happen later on internally
so you don't have to worry about it as a Counter driver author.

Using sprintf for all the extension callbacks in your driver is fine:

        return sprintf(buf, "%d\n", reg)

> +}
> +
> +static ssize_t ceiling_write(struct counter_device *counter,
> +		struct counter_count *count, void *priv, const char *buf,
> +		size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 max;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &max);
> +	if (ret < 0)
> +		return ret;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPMAX, max);
> +
> +	return len;
> +}
> +
> +static ssize_t enable_read(struct counter_device *counter,
> +		struct counter_count *count, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", !!(reg & INTEL_QEPCON_EN));
> +}
> +
> +static ssize_t enable_write(struct counter_device *counter,
> +		struct counter_count *count, void *priv, const char *buf,
> +		size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +	u32 val;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &val);

Use kstrtobool here instead so that values such as "y" and "n" can be
interpreted correctly as well.

> +	if (ret < 0)
> +		return ret;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (val)
> +		reg |= INTEL_QEPCON_EN;
> +	else
> +		reg &= ~INTEL_QEPCON_EN;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +
> +	return len;
> +}
> +
> +static ssize_t direction_read(struct counter_device *counter,
> +		struct counter_count *count, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +
> +	return snprintf(buf, PAGE_SIZE, "%s\n", qep->direction ?
> +			"forward" : "backward");

We standardized the direction extension values, so instead define a
enum counter_count_direction dir and set it to the direction you want.
When you're done, you can return like this:

        return sprintf(buf, "%s\n", counter_count_direction_str[dir])

The counter_count_direction enum constants are provided in the counter.h
header file.

> +}
> +
> +static const struct counter_count_ext intel_qep_count_ext[] = {
> +	INTEL_QEP_COUNTER_COUNT_EXT_RW(ceiling),
> +	INTEL_QEP_COUNTER_COUNT_EXT_RW(enable),
> +	INTEL_QEP_COUNTER_COUNT_EXT_RO(direction),
> +};
> +
> +static struct counter_count intel_qep_here counter_count[] = {
> +	{
> +		.id = 0,
> +		.name = "Channel 1 Count",
> +		.functions_list = intel_qep_count_functions,
> +		.num_functions = ARRAY_SIZE(intel_qep_count_functions),
> +		.synapses = intel_qep_count_synapses,
> +		.num_synapses = ARRAY_SIZE(intel_qep_count_synapses),
> +		.ext = intel_qep_count_ext,
> +		.num_ext = ARRAY_SIZE(intel_qep_count_ext),
> +	},
> +};
> +
> +static ssize_t noise_read(struct counter_device *counter, void *priv, char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (!(reg & INTEL_QEPCON_FLT_EN))
> +		return snprintf(buf, PAGE_SIZE, "0\n");
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPFLT);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", INTEL_QEPFLT_MAX_COUNT(reg));
> +}
> +
> +static ssize_t noise_write(struct counter_device *counter, void *priv,
> +		const char *buf, size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +	u32 max;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &max);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (max > 0x1fffff)
> +		max = 0x1ffff;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (max == 0) {
> +		reg &= ~INTEL_QEPCON_FLT_EN;
> +	} else {
> +		reg |= INTEL_QEPCON_FLT_EN;
> +		intel_qep_writel(qep->regs, INTEL_QEPFLT, max);
> +	}
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +	

Trim the trailing whitespace here.

> +	return len;
> +}

Your "noise" extension attribute will need documentation, so create a
Documentation/ABI/testing/sysfs-bus-counter-intel-qep file with all the
information for it inside.

> +
> +static ssize_t preset_read(struct counter_device *counter, void *priv, char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "0\n");
> +}
> +
> +static ssize_t preset_enable_read(struct counter_device *counter, void *priv,
> +		char *buf)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +	return snprintf(buf, PAGE_SIZE, "%d\n",
> +			!(reg & INTEL_QEPCON_COUNT_RST_MODE));
> +}
> +
> +static ssize_t preset_enable_write(struct counter_device *counter, void *priv,
> +		const char *buf, size_t len)
> +{
> +	struct intel_qep *qep = counter_to_qep(counter);
> +	u32 reg;
> +	u32 val;
> +	int ret;
> +
> +	ret = kstrtou32(buf, 0, &val);

Use kstrtobool here for the same reason as the "enable" extension.

> +	if (ret < 0)
> +		return ret;
> +
> +	reg = intel_qep_readl(qep->regs, INTEL_QEPCON);
> +
> +	if (val)
> +		reg &= ~INTEL_QEPCON_COUNT_RST_MODE;
> +	else
> +		reg |= INTEL_QEPCON_COUNT_RST_MODE;
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, reg);
> +	

Trim the trailing whitespace here.

William Breathitt Gray

> +	return len;
> +}
> +
> +static const struct counter_device_ext intel_qep_ext[] = {
> +	INTEL_QEP_COUNTER_EXT_RW(noise),
> +	INTEL_QEP_COUNTER_EXT_RO(preset),
> +	INTEL_QEP_COUNTER_EXT_RW(preset_enable)
> +};
> +
> +static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
> +{
> +	struct intel_qep	*qep;
> +	struct device		*dev = &pci->dev;
> +	void __iomem		*regs;
> +	int			ret;
> +	int			irq;
> +
> +	qep = devm_kzalloc(dev, sizeof(*qep), GFP_KERNEL);
> +	if (!qep)
> +		return -ENOMEM;
> +
> +	ret = pcim_enable_device(pci);
> +	if (ret)
> +		return ret;
> +
> +	pci_set_master(pci);
> +
> +	ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
> +	if (ret)
> +		return ret;
> +
> +	regs = pcim_iomap_table(pci)[0];
> +	if (!regs)
> +		return -ENOMEM;
> +
> +	qep->pci = pci;
> +	qep->dev = dev;
> +	qep->regs = regs;
> +	mutex_init(&qep->lock);
> +
> +	intel_qep_init(qep, true);
> +	pci_set_drvdata(pci, qep);
> +
> +	qep->counter.name = pci_name(pci);
> +	qep->counter.parent = dev;
> +	qep->counter.ops = &intel_qep_counter_ops;
> +	qep->counter.counts = intel_qep_counter_count;
> +	qep->counter.num_counts = ARRAY_SIZE(intel_qep_counter_count);
> +	qep->counter.signals = intel_qep_signals;
> +	qep->counter.num_signals = ARRAY_SIZE(intel_qep_signals);
> +	qep->counter.ext = intel_qep_ext;
> +	qep->counter.num_ext = ARRAY_SIZE(intel_qep_ext);
> +	qep->counter.priv = qep;
> +
> +	ret = counter_register(&qep->counter);
> +	if (ret)
> +		return ret;
> +
> +	ret = pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_ALL_TYPES);
> +	if (ret < 0)
> +		goto err_irq_vectors;
> +
> +	irq = pci_irq_vector(pci, 0);
> +	ret = devm_request_threaded_irq(&pci->dev, irq, intel_qep_irq,
> +			intel_qep_irq_thread, IRQF_SHARED | IRQF_TRIGGER_RISING,
> +			"intel-qep", qep);
> +	if (ret)
> +		goto err_irq;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 1000);
> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_put_noidle(dev);
> +	pm_runtime_allow(dev);
> +
> +	return 0;
> +
> +err_irq:
> +	pci_free_irq_vectors(pci);
> +
> +err_irq_vectors:
> +	counter_unregister(&qep->counter);
> +
> +	return ret;
> +}
> +
> +static void intel_qep_remove(struct pci_dev *pci)
> +{
> +	struct intel_qep	*qep = pci_get_drvdata(pci);
> +	struct device		*dev = &pci->dev;
> +
> +	pm_runtime_forbid(dev);
> +	pm_runtime_get_noresume(dev);
> +
> +	intel_qep_writel(qep->regs, INTEL_QEPCON, 0);
> +	pci_free_irq_vectors(pci);
> +	counter_unregister(&qep->counter);
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int intel_qep_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int intel_qep_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct intel_qep *qep = pci_get_drvdata(pdev);
> +
> +	intel_qep_init(qep, false);
> +
> +	return 0;
> +}
> +
> +static int intel_qep_runtime_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int intel_qep_runtime_resume(struct device *dev)
> +{
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +	struct intel_qep *qep = pci_get_drvdata(pdev);
> +
> +	intel_qep_init(qep, false);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops intel_qep_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(intel_qep_suspend,
> +				intel_qep_resume)
> +	SET_RUNTIME_PM_OPS(intel_qep_runtime_suspend, intel_qep_runtime_resume,
> +				NULL)
> +};
> +
> +static struct pci_driver intel_qep_driver = {
> +	.name		= "intel-qep",
> +	.id_table	= intel_qep_id_table,
> +	.probe		= intel_qep_probe,
> +	.remove		= intel_qep_remove,
> +	.driver = {
> +		.pm = &intel_qep_pm_ops,
> +	}
> +};
> +
> +module_pci_driver(intel_qep_driver);
> +
> +MODULE_AUTHOR("Felipe Balbi <felipe.balbi@linux.intel.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Intel Quadrature Encoder Driver");
> -- 
> 2.23.0
> 

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[Jonathan Cameron]

On Tue, 24 Sep 2019 20:32:58 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Tue, Sep 24, 2019 at 12:46:39PM +0300, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > William Breathitt Gray <vilhelm.gray@gmail.com> writes:  
> > > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:  
> > >> Add support for Intel PSE Quadrature Encoder
> > >> 
> > >> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > >> ---
> > >> 
> > >> Changes since v1:
> > >> 	- Many more private sysfs files converted over to counter interface
> > >> 
> > >> 
> > >> How do you want me to model this device's Capture Compare Mode (see
> > >> below)?  
> > >
> > > Hi Felipe,
> > >
> > > I'm CCing Fabien and David as they may be interested in the timestamps
> > > discussion. See below for some ideas I have on implementing this.
> > >  
> > >> For the few features which I couldn't find a matching property in
> > >> counter framework, I still leave them as private sysfs files so we can
> > >> discuss how to model them in the framework.
> > >> 
> > >> Do you want Capture Compare to be a new function mode?
> > >> 
> > >> BTW, I know I'm missing a Documentation file documenting sysfs files
> > >> introduced by this driver, I'll do that once we agree how to move all
> > >> other sysfs files to the framework. No worries.
> > >> 
> > >> 
> > >> Details about the controller (do you want this in commit log?):
> > >> 
> > >> 
> > >> Controller has 2 modes of operation: QEP and Capture. Both modes are
> > >> mutually exclusive. We also have a set of maskable interrupts. Further
> > >> details about each mode below.  
> > >
> > > I noticed your interrupt handler takes care of a number of different
> > > scenarios. Would you be able to summarize a bit further here the
> > > conditions for this device that cause an interrupt to be fired (watchdog
> > > timeout, FIFO updates, etc.)?
> > >  
> > >> Quadrature Encoder Mode
> > >> =======================
> > >> 
> > >> Used to measure rotational speed, direction and angle of rotation of a
> > >> motor shaft. Feature list:
> > >> 
> > >> 	- Quadrature decoder providing counter pulses with x4 count
> > >> 	  resolution and count direction
> > >> 
> > >> 	- 32-bit up/down Position Counter for position measurement
> > >> 
> > >> 	- Two modes of position counter reset:  
> > >> 		> Maximum Count (ceiling) to reset the position counter
> > >> 		> Index pulse to reset the position counter  
> > >> 
> > >> 	- Watchdog timer functionality for detecting ‘stall’ events
> > >> 
> > >> Capture Compare Mode
> > >> ====================
> > >> 
> > >> Used to measure phase input signal Period or Duty Cycle. Feature List:
> > >> 
> > >> 	- Capture function operates either on phase A or phase B input
> > >> 	  and can be configured to operate on lo/hi or hi/lo or both hi
> > >> 	  and lo transitions.
> > >> 
> > >> 	- Free-running 32-bit counter to be configured to run at greater
> > >>           than or equal to 4x input signal frequency  
> > >
> > > So in "Capture Compare" mode, the counter value is just increasing when
> > > a state condition transition occurs. In that case we won't need a new
> > > function mode to represent this behavior since one already exists:
> > > "increase".
> > >
> > > You can add it to your intel_qep_count_functions array like so:
> > >
> > >         [INTEL_QEP_ENCODER_MODE_CAPTURE] =
> > >         COUNTER_COUNT_FUNCTION_INCREASE,
> > >
> > > The various configurations for this mode are just Synapse action modes.
> > > If you want only Phase A, you would set the action mode for Phase A
> > > ("rising edge", "falling edge", or "both edges") and change the action
> > > mode for Phase B to "none"; vice-versa configuration for Phase B instead
> > > of Phase A.
> > >
> > > One thing to keep in mind is that action_set will need to maintain valid
> > > configurations -- so if the user tries to set the action mode for Phase
> > > A to something other than "none", you need to automatically set Phase
> > > B's action mode to "none" (and vice-versa).  
> > 
> > interesting, thanks
> >   
> > >> 	- Clock post-scaler to derive the counter clock source from the
> > >> 	  peripheral clock  
> > >
> > > I see you already have a "prescaler" extension in your code. Is this
> > > different from the "post-scaler" you mentioned here?  
> > 
> > This was probably a brain-fart on my side. It should be post-scaler, but
> > that's only valid for capture compare mode.  
> 
> In that case you're implementation seems fine for this; perhaps a more
> generic name for that extension might be better such as "scale", but
> I'll decide later.
>  
> > >> 	- 32B wide FIFO to capture 32-bit timestamps of up to 8
> > >> 	  transition events  
> > >
> > > You can implement this as a Count extension called "timestamps" or
> > > similar. What we can do is have a read on this attribute return the
> > > entire FIFO data buffer as unsigned integers, where each timestamp is
> > > deliminated by a space.
> > >
> > > In addition, it may be useful to have another extension called
> > > "timestamps_layout", or something along those lines, that will report
> > > the ordering of the buffer (i.e. whether it's "fifo", "lifo", etc.).
> > >
> > > Would this design work for your needs?  
> > 
> > Perhaps it would be best to have a pollable binary sysfs file (meaning,
> > we need to be able to call sysfs_notify() at will) and userspace just
> > receives POLLIN whenever there's data read. Then a read returns an array
> > of e.g. struct counter_event where struct counter_event could be defined
> > as:
> > 
> > 	struct counter_event {
> >         	uint32_t	event_type;
> > 		struct timespec64 timestamp;
> >                 uint8_t		reserved[32];
> >         };
> > 
> > Userspace would do something along the lines of:
> > 
> > 	fd = open("/sys/bus/counter/foo/capture/timestamps",...);
> > 	pollfd[0].fd = fd;
> >         pollfd[0].events = POLLIN;
> >         poll(pollfd, 1, -1);
> > 
> > 	if (pollfd[0].revents & POLLIN) {
> >         	ret = read(fd, events, sizeof(struct counter_event) * 8);
> > 
> > 		for (i = 0; i < ret / sizeof(struct counter_event); i++)
> > 			process_event(events[i]);
> >         }
> >         
> > Or something like that.  
> 
> One concern is that returning binary data like that isn't friendly for
> human interaction. However, alternatively printing a human-readable
> array would add latency for userspace software that has to interpret it,
> so that would a problem as well. This is something we'll have to think
> more about then.

I've been rather offline for a little while so just catching up.
Seems the conversation has moved on from this, but to avoid us circling
back, there are distinct rules for sysfs.

1. Single value - whilst this gets stretched a bit to allow things like
   rotation matrices where they are representing one 'thing', a fifo isn't
   going to be acceptable.

2. Binary sysfs files are probably not a path for this sort of thing either.
   IIRC as a general rule they are blocked for usecases like this (and most
   others in new drivers).

Jonathan

> 
> > I could, also, remove this part from the driver for now, so we can
> > discuss how the capture-compare buffer read would look like. At least we
> > could get QDP feature merged while we come to a consensus about capture
> > compare.
> > 
> > What do you think?
> > 
> > -- 
> > balbi  
> 
> That sounds like a good idea. Most of this driver can be implemented
> using the existing Counter subsystem components, so we can do as you
> suggest and focus on just getting this driver merged in with the
> functionality it can for now.
> 
> After it's accepted and merged, we can turn our attention to the new
> extension features such as the timestamps buffer return. This will make
> it easier for us to discuss ideas since we won't have to worry about
> merging in nonrelated functionality.
> 
> William Breathitt Gray

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[Jonathan Cameron]

On Tue, 1 Oct 2019 20:34:42 -0400
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Mon, Sep 30, 2019 at 08:22:39AM +0300, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > William Breathitt Gray <vilhelm.gray@gmail.com> writes:  
> > > On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:  
> > >> On 9/22/19 6:35 PM, William Breathitt Gray wrote:  
> > >> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:  
> > >> >> Add support for Intel PSE Quadrature Encoder
> > >> >>
> > >> >> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > >> >> ---
> > >> >>
> > >> >> Changes since v1:
> > >> >> 	- Many more private sysfs files converted over to counter interface
> > >> >>
> > >> >>
> > >> >> How do you want me to model this device's Capture Compare Mode (see
> > >> >> below)?  
> > >> > 
> > >> > Hi Felipe,
> > >> > 
> > >> > I'm CCing Fabien and David as they may be interested in the timestamps
> > >> > discussion. See below for some ideas I have on implementing this.
> > >> >   
> > >> 
> > >> Could be an interesting read (thread from my first counter driver):
> > >> 
> > >> https://lore.kernel.org/linux-iio/1b913919-beb9-34e7-d915-6bcc40eeee1d@lechnology.com/
> > >> 
> > >> What would be useful to me is something like the buffer feature in iio
> > >> where a timestamp is associated with a count and stored in a buffer so that
> > >> we can look at a window of all values recorded in the last 20ms. Being able
> > >> to access this via mmap would be very helpful for performance (running on
> > >> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
> > >> unless it is a rare event, like a watchdog timeout.  
> > >
> > > I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
> > > timestamps and buffers. I don't want to reinvent something that is
> > > working well, so hopefully we can reuse the IIO timestamp design for the
> > > Counter subsystem.

Simple approach, push them both into a kfifo. Userspace then reads this
(with poll/select/blocking read + things like watermarks all available).
The description of what is there in each record is handled via sysfs as
the IIO model is that the format of each record only changes with userspace
intervention.

The complexities mostly come from allowing the hardware side of what is there
and the software side to differ, but this is only really needed if you want
to have multiple readers and need to split up the data so each thinks it has
the device to itself.  I would guess not needed for counter usecases, but who
knows down the line.

> > >
> > > I would argue that a human-readable timestamps printout is useful for
> > > certain applications (e.g. a tally counter attached to a fault line: a
> > > human administrator will be able to review previous fault times).
> > > However as you point out, a low latency operation is necessary for
> > > performance critical applications.

For this sort of case you could use a sysfs file that just returns the oldest
entry when read.  A seek back to the start and reread or a reopen of the file
can then give you the next one and an appropriate error return indicates none
left.

> > >
> > > Although you are correct that mmap is a good low latency operation to
> > > get access to a timestamp buffer, I'm afraid giving direct access to
> > > memory like that will lead to many incompatible representations of
> > > timestamp data (e.g. variations in endianness, signedness, data size,
> > > etc.). I would like a standardized representation for this data that
> > > userspace applications can expect to receive and interpret, especially
> > > when time is widely represented as an unsigned integer.

It is moderately hard to get that sort of interface right, but it can be done.
IIO does it for DMA type buffers, but not for anything where a software
fifo is involved.   You would need to guarantee a fixed format etc.

> > >
> > > Felipe suggested the creation of a counter_event structure so that users
> > > can poll on an attribute. This kind of behavior is useful for notifying
> > > users of interrupts and other events, but I think we should restrict the
> > > use of the read call on these sysfs attributes to just human-readable
> > > data. Instead, perhaps ioctl calls can be used to facilitate binary data
> > > transfers.
> > >
> > > For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
> > > that returns a counter_timestamps structure with a timestamps array
> > > populated:
> > >
> > >         struct counter_timestamps{
> > >                 size_t num_timestamps;
> > >         	unsigned int *timestamps;
> > >         }
> > >
> > > That would allow quick access to the timestamps data, while also
> > > restricting it to a standard representation that all userspace
> > > applications can follow and interpret. In addition, this won't interfer
> > > with polling, so users can still wait for an interrupt and then decide
> > > whether they want to use the slower human-readable printout (via read)
> > > or the faster binary data access (via ioctl).  
> > 
> > Seems like we're starting to build the need for a /dev/counter[0123...]
> > representation of the subsystem. If that's the case, then it may very
> > well be that sysfs becomes somewhat optional.

Agreed.  Don't map this stuff onto sysfs where a chardev is more sensible.

> > 
> > I think is makes sense to rely more on character devices specially since
> > I know of devices running linux with so little memory that sysfs (and a
> > bunch of other features) are removed from the kernel. Having a character
> > device representation would allow counter subsystem to be used on such
> > devices.
> > 
> > cheers
> > 
> > -- 
> > balbi  
> 
> A character device node for a counter might be a good idea. If a
> performance critical application can't depend on parsing a sysfs
> printout for timestamps, then it probably doesn't want to do so for the
> other attributes either. I think you are right that certain systems
> would have sysfs disabled for that very reason.
> 

I am a little curious to whether people often run sysfs free any more?
I know it used to happen a fair bit, but a lot of the kernel is now rather
dependent on it.

Of course you can add IOCTLs to do all the configuration of a device, but
you can end up with a nasty and inflexible interface + add a burden
on simple users of them having to use a library to unwind it all rather
than simply poking text files. 

> I think latency concerns are the same reason the GPIO subsystem started
> providing character device nodes as well. 

That was part of it, but a big element was also to provide the ability
to set+get multiple lines in parallel.  Kind of the same as you have
here with a timestamp matching to a count.  For low latency gpio on an
SoC people often just directly write the registers from a userspace
mapping.

> We can do similar with the
> Counter subsystem: provide character device nodes by default, and
> optionally provide the human-readable sysfs interface as well. This
> would allow applications with latency concerns to use a standard
> interface for the Counter subsystem, while optionally providing a
> simpler sysfs interface for other users.

I wouldn't necessarily go for making the sysfs interface optional.
It is extremely convenient for describing complex devices. It is
possible to do all that via complex IOCTL query interfaces (see
the mediabus framework for example), but it's much heavier weight.

The IIO approach was to use the chardev for fast path and sysfs
for slow.  Maybe we'd do it differently if starting now, but I'm
not sure we would end up different in this aspect.  Other things
would change though ;)


Jonathan

> 
> William Breathitt Gray

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[William Breathitt Gray]

On Thu, Oct 03, 2019 at 02:14:43PM +0100, Jonathan Cameron wrote:
> On Tue, 1 Oct 2019 20:34:42 -0400
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > On Mon, Sep 30, 2019 at 08:22:39AM +0300, Felipe Balbi wrote:
> > > 
> > > Hi,
> > > 
> > > William Breathitt Gray <vilhelm.gray@gmail.com> writes:  
> > > > On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:  
> > > >> On 9/22/19 6:35 PM, William Breathitt Gray wrote:  
> > > >> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:  
> > > >> >> Add support for Intel PSE Quadrature Encoder
> > > >> >>
> > > >> >> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > > >> >> ---
> > > >> >>
> > > >> >> Changes since v1:
> > > >> >> 	- Many more private sysfs files converted over to counter interface
> > > >> >>
> > > >> >>
> > > >> >> How do you want me to model this device's Capture Compare Mode (see
> > > >> >> below)?  
> > > >> > 
> > > >> > Hi Felipe,
> > > >> > 
> > > >> > I'm CCing Fabien and David as they may be interested in the timestamps
> > > >> > discussion. See below for some ideas I have on implementing this.
> > > >> >   
> > > >> 
> > > >> Could be an interesting read (thread from my first counter driver):
> > > >> 
> > > >> https://lore.kernel.org/linux-iio/1b913919-beb9-34e7-d915-6bcc40eeee1d@lechnology.com/
> > > >> 
> > > >> What would be useful to me is something like the buffer feature in iio
> > > >> where a timestamp is associated with a count and stored in a buffer so that
> > > >> we can look at a window of all values recorded in the last 20ms. Being able
> > > >> to access this via mmap would be very helpful for performance (running on
> > > >> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
> > > >> unless it is a rare event, like a watchdog timeout.  
> > > >
> > > > I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
> > > > timestamps and buffers. I don't want to reinvent something that is
> > > > working well, so hopefully we can reuse the IIO timestamp design for the
> > > > Counter subsystem.
> 
> Simple approach, push them both into a kfifo. Userspace then reads this
> (with poll/select/blocking read + things like watermarks all available).
> The description of what is there in each record is handled via sysfs as
> the IIO model is that the format of each record only changes with userspace
> intervention.
> 
> The complexities mostly come from allowing the hardware side of what is there
> and the software side to differ, but this is only really needed if you want
> to have multiple readers and need to split up the data so each thinks it has
> the device to itself.  I would guess not needed for counter usecases, but who
> knows down the line.

Is the kfifo updating only when a user interacts with the timestamp
sysfs attributes? If so, why store the timestamps in a kfifo instead of
serving them directly on demand; or is the kfifo there to allow multiple
users to access the same timestamp fifo?

> > > >
> > > > I would argue that a human-readable timestamps printout is useful for
> > > > certain applications (e.g. a tally counter attached to a fault line: a
> > > > human administrator will be able to review previous fault times).
> > > > However as you point out, a low latency operation is necessary for
> > > > performance critical applications.
> 
> For this sort of case you could use a sysfs file that just returns the oldest
> entry when read.  A seek back to the start and reread or a reopen of the file
> can then give you the next one and an appropriate error return indicates none
> left.

The downside to this interface is that it's volatile, in the sense that
once the fifo is read you can't reread it unless you saved those
timestamps yourself. However, as you've pointed out in another email,
sysfs has a "single value" rule so returning the latest timestamp is
likely the best solution given the restriction that multiple values
cannot be returned via a single attribute.

> > > >
> > > > Although you are correct that mmap is a good low latency operation to
> > > > get access to a timestamp buffer, I'm afraid giving direct access to
> > > > memory like that will lead to many incompatible representations of
> > > > timestamp data (e.g. variations in endianness, signedness, data size,
> > > > etc.). I would like a standardized representation for this data that
> > > > userspace applications can expect to receive and interpret, especially
> > > > when time is widely represented as an unsigned integer.
> 
> It is moderately hard to get that sort of interface right, but it can be done.
> IIO does it for DMA type buffers, but not for anything where a software
> fifo is involved.   You would need to guarantee a fixed format etc.
> 
> > > >
> > > > Felipe suggested the creation of a counter_event structure so that users
> > > > can poll on an attribute. This kind of behavior is useful for notifying
> > > > users of interrupts and other events, but I think we should restrict the
> > > > use of the read call on these sysfs attributes to just human-readable
> > > > data. Instead, perhaps ioctl calls can be used to facilitate binary data
> > > > transfers.
> > > >
> > > > For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
> > > > that returns a counter_timestamps structure with a timestamps array
> > > > populated:
> > > >
> > > >         struct counter_timestamps{
> > > >                 size_t num_timestamps;
> > > >         	unsigned int *timestamps;
> > > >         }
> > > >
> > > > That would allow quick access to the timestamps data, while also
> > > > restricting it to a standard representation that all userspace
> > > > applications can follow and interpret. In addition, this won't interfer
> > > > with polling, so users can still wait for an interrupt and then decide
> > > > whether they want to use the slower human-readable printout (via read)
> > > > or the faster binary data access (via ioctl).  
> > > 
> > > Seems like we're starting to build the need for a /dev/counter[0123...]
> > > representation of the subsystem. If that's the case, then it may very
> > > well be that sysfs becomes somewhat optional.
> 
> Agreed.  Don't map this stuff onto sysfs where a chardev is more sensible.
> 
> > > 
> > > I think is makes sense to rely more on character devices specially since
> > > I know of devices running linux with so little memory that sysfs (and a
> > > bunch of other features) are removed from the kernel. Having a character
> > > device representation would allow counter subsystem to be used on such
> > > devices.
> > > 
> > > cheers
> > > 
> > > -- 
> > > balbi  
> > 
> > A character device node for a counter might be a good idea. If a
> > performance critical application can't depend on parsing a sysfs
> > printout for timestamps, then it probably doesn't want to do so for the
> > other attributes either. I think you are right that certain systems
> > would have sysfs disabled for that very reason.
> > 
> 
> I am a little curious to whether people often run sysfs free any more?
> I know it used to happen a fair bit, but a lot of the kernel is now rather
> dependent on it.
> 
> Of course you can add IOCTLs to do all the configuration of a device, but
> you can end up with a nasty and inflexible interface + add a burden
> on simple users of them having to use a library to unwind it all rather
> than simply poking text files. 
> 
> > I think latency concerns are the same reason the GPIO subsystem started
> > providing character device nodes as well. 
> 
> That was part of it, but a big element was also to provide the ability
> to set+get multiple lines in parallel.  Kind of the same as you have
> here with a timestamp matching to a count.  For low latency gpio on an
> SoC people often just directly write the registers from a userspace
> mapping.
> 
> > We can do similar with the
> > Counter subsystem: provide character device nodes by default, and
> > optionally provide the human-readable sysfs interface as well. This
> > would allow applications with latency concerns to use a standard
> > interface for the Counter subsystem, while optionally providing a
> > simpler sysfs interface for other users.
> 
> I wouldn't necessarily go for making the sysfs interface optional.
> It is extremely convenient for describing complex devices. It is
> possible to do all that via complex IOCTL query interfaces (see
> the mediabus framework for example), but it's much heavier weight.
> 
> The IIO approach was to use the chardev for fast path and sysfs
> for slow.  Maybe we'd do it differently if starting now, but I'm
> not sure we would end up different in this aspect.  Other things
> would change though ;)
> 
> 
> Jonathan
> 
> > 
> > William Breathitt Gray

I agree about the utility and convenience of sysfs; it's an interface
that is simple enough for a human to understand and provides information
in a universally parsable format: text. We can leave the sysfs interface
available by default since disabling it can be left to the power users
that have a need to do so (and flipping a Kconfig option shouldn't be
that difficult to accomplish).

Regardless, I do see the benefits of providing a chardev interface --
not just for timestamps evaluation but in other counter applications as
well, such as two-dimensional positioning tables where multiple counter
values would need to be read at relatively the same time (a situation
where multiple sysfs reads would impede precision).

My concern with this is similar: a growing list of IOCTLs that are added
with new drivers trying to expose their device-specific extensions. I
think exposing every single extension with a respective dedicated IOCTL
is not the way to go; this would quickly lead to an unmaintainable list
of IOCTLs that most users would never need

A more maintainable approach is to dedicate specialized IOCTLs only for
those features that are used by multiple counter drivers (i.e. listed in
Documentation/ABI/testing/sysfs-bus-counter); this will help curb
feature creep. The other driver-specific extensions can be accessed via
a few IOCTLs made for such interaction: perhaps one to poll for a list
of available extensions and another to read/write.

The only change required to the Counter driver API is to allow for a
more opaque extension structure. This will allow us to supply dedicated
extension callbacks to handle binary data rather than text (though in an
opaque way such as the existing counter_count_direction enum); however,
driver-specific extensions can still be handled by text for simplicity.

The main change would be in the drivers/counter/counter.c file. This can
be split into three file: counter-core, counter-sysfs, counter-chardev.
The counter-core file would provide the core Counter functionality and
handle the driver API; the counter-sysfs and counter-chardev files can
expose the Counter subsystem to userspace in their respective formats.

William Breathitt Gray

Re: [RFC/PATCHv2 2/2] counter: introduce support for Intel QEP Encoder

[Jonathan Cameron]

On Wed, 16 Oct 2019 16:20:35 -0400
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Thu, Oct 03, 2019 at 02:14:43PM +0100, Jonathan Cameron wrote:
> > On Tue, 1 Oct 2019 20:34:42 -0400
> > William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> >   
> > > On Mon, Sep 30, 2019 at 08:22:39AM +0300, Felipe Balbi wrote:  
> > > > 
> > > > Hi,
> > > > 
> > > > William Breathitt Gray <vilhelm.gray@gmail.com> writes:    
> > > > > On Tue, Sep 24, 2019 at 04:46:57PM -0500, David Lechner wrote:    
> > > > >> On 9/22/19 6:35 PM, William Breathitt Gray wrote:    
> > > > >> > On Thu, Sep 19, 2019 at 11:03:05AM +0300, Felipe Balbi wrote:    
> > > > >> >> Add support for Intel PSE Quadrature Encoder
> > > > >> >>
> > > > >> >> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> > > > >> >> ---
> > > > >> >>
> > > > >> >> Changes since v1:
> > > > >> >> 	- Many more private sysfs files converted over to counter interface
> > > > >> >>
> > > > >> >>
> > > > >> >> How do you want me to model this device's Capture Compare Mode (see
> > > > >> >> below)?    
> > > > >> > 
> > > > >> > Hi Felipe,
> > > > >> > 
> > > > >> > I'm CCing Fabien and David as they may be interested in the timestamps
> > > > >> > discussion. See below for some ideas I have on implementing this.
> > > > >> >     
> > > > >> 
> > > > >> Could be an interesting read (thread from my first counter driver):
> > > > >> 
> > > > >> https://lore.kernel.org/linux-iio/1b913919-beb9-34e7-d915-6bcc40eeee1d@lechnology.com/
> > > > >> 
> > > > >> What would be useful to me is something like the buffer feature in iio
> > > > >> where a timestamp is associated with a count and stored in a buffer so that
> > > > >> we can look at a window of all values recorded in the last 20ms. Being able
> > > > >> to access this via mmap would be very helpful for performance (running on
> > > > >> 300MHz ARM). Anything to do with timestamps in sysfs is probably not useful
> > > > >> unless it is a rare event, like a watchdog timeout.    
> > > > >
> > > > > I'm CCing Jonathan Cameron since I'm not familiar with how IIO handles
> > > > > timestamps and buffers. I don't want to reinvent something that is
> > > > > working well, so hopefully we can reuse the IIO timestamp design for the
> > > > > Counter subsystem.  
> > 
> > Simple approach, push them both into a kfifo. Userspace then reads this
> > (with poll/select/blocking read + things like watermarks all available).
> > The description of what is there in each record is handled via sysfs as
> > the IIO model is that the format of each record only changes with userspace
> > intervention.
> > 
> > The complexities mostly come from allowing the hardware side of what is there
> > and the software side to differ, but this is only really needed if you want
> > to have multiple readers and need to split up the data so each thinks it has
> > the device to itself.  I would guess not needed for counter usecases, but who
> > knows down the line.  
> 
> Is the kfifo updating only when a user interacts with the timestamp
> sysfs attributes? If so, why store the timestamps in a kfifo instead of
> serving them directly on demand; or is the kfifo there to allow multiple
> users to access the same timestamp fifo?

I'm confused here.  We have to update 'what' is stored in the kfifo when
a change is made to what userspace wants us to store there.

The kfifo is because userspace doesn't want to have to constantly read
data to avoid loosing it.  A count + timestamp is grabbed based either on
the something from the hardware, or another in kernel source of "grab data now".
The kernel pushes that into the kfifo.  Userspace then reads out when it
is ready.

Multiple users would require multiple kfifos. Which is what evdev does IIRC.

The logic to fill those multiple kfifos gets complex if different users have
asked for different things to be stored.  Eg. one wants timestamps and position,
the other just the timestamps (odd case, but seems people do want to do that!).
Then you have to split the data up appropriately before pushing it to the buffers.

> 
> > > > >
> > > > > I would argue that a human-readable timestamps printout is useful for
> > > > > certain applications (e.g. a tally counter attached to a fault line: a
> > > > > human administrator will be able to review previous fault times).
> > > > > However as you point out, a low latency operation is necessary for
> > > > > performance critical applications.  
> > 
> > For this sort of case you could use a sysfs file that just returns the oldest
> > entry when read.  A seek back to the start and reread or a reopen of the file
> > can then give you the next one and an appropriate error return indicates none
> > left.  
> 
> The downside to this interface is that it's volatile, in the sense that
> once the fifo is read you can't reread it unless you saved those
> timestamps yourself. However, as you've pointed out in another email,
> sysfs has a "single value" rule so returning the latest timestamp is
> likely the best solution given the restriction that multiple values
> cannot be returned via a single attribute.

Even a sysfs interface would need to be effectively volatile.  Might require
an explicit 'clear' signal, but those are racy to deal with.  If not volatile
it would rapidly become very large.

> 
> > > > >
> > > > > Although you are correct that mmap is a good low latency operation to
> > > > > get access to a timestamp buffer, I'm afraid giving direct access to
> > > > > memory like that will lead to many incompatible representations of
> > > > > timestamp data (e.g. variations in endianness, signedness, data size,
> > > > > etc.). I would like a standardized representation for this data that
> > > > > userspace applications can expect to receive and interpret, especially
> > > > > when time is widely represented as an unsigned integer.  
> > 
> > It is moderately hard to get that sort of interface right, but it can be done.
> > IIO does it for DMA type buffers, but not for anything where a software
> > fifo is involved.   You would need to guarantee a fixed format etc.
> >   
> > > > >
> > > > > Felipe suggested the creation of a counter_event structure so that users
> > > > > can poll on an attribute. This kind of behavior is useful for notifying
> > > > > users of interrupts and other events, but I think we should restrict the
> > > > > use of the read call on these sysfs attributes to just human-readable
> > > > > data. Instead, perhaps ioctl calls can be used to facilitate binary data
> > > > > transfers.
> > > > >
> > > > > For example, we can define a COUNTER_GET_TIMESTAMPS_IOCTL ioctl request
> > > > > that returns a counter_timestamps structure with a timestamps array
> > > > > populated:
> > > > >
> > > > >         struct counter_timestamps{
> > > > >                 size_t num_timestamps;
> > > > >         	unsigned int *timestamps;
> > > > >         }
> > > > >
> > > > > That would allow quick access to the timestamps data, while also
> > > > > restricting it to a standard representation that all userspace
> > > > > applications can follow and interpret. In addition, this won't interfer
> > > > > with polling, so users can still wait for an interrupt and then decide
> > > > > whether they want to use the slower human-readable printout (via read)
> > > > > or the faster binary data access (via ioctl).    
> > > > 
> > > > Seems like we're starting to build the need for a /dev/counter[0123...]
> > > > representation of the subsystem. If that's the case, then it may very
> > > > well be that sysfs becomes somewhat optional.  
> > 
> > Agreed.  Don't map this stuff onto sysfs where a chardev is more sensible.
> >   
> > > > 
> > > > I think is makes sense to rely more on character devices specially since
> > > > I know of devices running linux with so little memory that sysfs (and a
> > > > bunch of other features) are removed from the kernel. Having a character
> > > > device representation would allow counter subsystem to be used on such
> > > > devices.
> > > > 
> > > > cheers
> > > > 
> > > > -- 
> > > > balbi    
> > > 
> > > A character device node for a counter might be a good idea. If a
> > > performance critical application can't depend on parsing a sysfs
> > > printout for timestamps, then it probably doesn't want to do so for the
> > > other attributes either. I think you are right that certain systems
> > > would have sysfs disabled for that very reason.
> > >   
> > 
> > I am a little curious to whether people often run sysfs free any more?
> > I know it used to happen a fair bit, but a lot of the kernel is now rather
> > dependent on it.
> > 
> > Of course you can add IOCTLs to do all the configuration of a device, but
> > you can end up with a nasty and inflexible interface + add a burden
> > on simple users of them having to use a library to unwind it all rather
> > than simply poking text files. 
> >   
> > > I think latency concerns are the same reason the GPIO subsystem started
> > > providing character device nodes as well.   
> > 
> > That was part of it, but a big element was also to provide the ability
> > to set+get multiple lines in parallel.  Kind of the same as you have
> > here with a timestamp matching to a count.  For low latency gpio on an
> > SoC people often just directly write the registers from a userspace
> > mapping.
> >   
> > > We can do similar with the
> > > Counter subsystem: provide character device nodes by default, and
> > > optionally provide the human-readable sysfs interface as well. This
> > > would allow applications with latency concerns to use a standard
> > > interface for the Counter subsystem, while optionally providing a
> > > simpler sysfs interface for other users.  
> > 
> > I wouldn't necessarily go for making the sysfs interface optional.
> > It is extremely convenient for describing complex devices. It is
> > possible to do all that via complex IOCTL query interfaces (see
> > the mediabus framework for example), but it's much heavier weight.
> > 
> > The IIO approach was to use the chardev for fast path and sysfs
> > for slow.  Maybe we'd do it differently if starting now, but I'm
> > not sure we would end up different in this aspect.  Other things
> > would change though ;)
> > 
> > 
> > Jonathan
> >   
> > > 
> > > William Breathitt Gray  
> 
> I agree about the utility and convenience of sysfs; it's an interface
> that is simple enough for a human to understand and provides information
> in a universally parsable format: text. We can leave the sysfs interface
> available by default since disabling it can be left to the power users
> that have a need to do so (and flipping a Kconfig option shouldn't be
> that difficult to accomplish).
> 
> Regardless, I do see the benefits of providing a chardev interface --
> not just for timestamps evaluation but in other counter applications as
> well, such as two-dimensional positioning tables where multiple counter
> values would need to be read at relatively the same time (a situation
> where multiple sysfs reads would impede precision).

Thats one of the main reasons we have this in IIO as well.

> 
> My concern with this is similar: a growing list of IOCTLs that are added
> with new drivers trying to expose their device-specific extensions. I
> think exposing every single extension with a respective dedicated IOCTL
> is not the way to go; this would quickly lead to an unmaintainable list
> of IOCTLs that most users would never need

Yes. If you are going to do that, it needs to be very carefully controlled.
Assumption is that a new driver almost never adds a new IOCTL.

> 
> A more maintainable approach is to dedicate specialized IOCTLs only for
> those features that are used by multiple counter drivers (i.e. listed in
> Documentation/ABI/testing/sysfs-bus-counter); this will help curb
> feature creep. The other driver-specific extensions can be accessed via
> a few IOCTLs made for such interaction: perhaps one to poll for a list
> of available extensions and another to read/write.
> 
> The only change required to the Counter driver API is to allow for a
> more opaque extension structure. This will allow us to supply dedicated
> extension callbacks to handle binary data rather than text (though in an
> opaque way such as the existing counter_count_direction enum); however,
> driver-specific extensions can still be handled by text for simplicity.

I'd push back as much as possible on any opaque structures.  They don't
work for generic userspace.   This isn't an area where things are normally
complex enough that we need a userspace driver component for most devices.

> 
> The main change would be in the drivers/counter/counter.c file. This can
> be split into three file: counter-core, counter-sysfs, counter-chardev.
> The counter-core file would provide the core Counter functionality and
> handle the driver API; the counter-sysfs and counter-chardev files can
> expose the Counter subsystem to userspace in their respective formats.

That would be easy enough.  The chardev control interface will need a lot
of careful thought though.  It is very easy to end up with something
inconsistent or impossible to extend.

Good luck ;)

Jonathan


> 
> William Breathitt Gray