linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] new driver for TI eQEP
@ 2019-07-22 15:45 David Lechner
  2019-07-22 15:45 ` [PATCH 1/4] dt-bindings: counter: new bindings " David Lechner
                   ` (4 more replies)
  0 siblings, 5 replies; 23+ messages in thread
From: David Lechner @ 2019-07-22 15:45 UTC (permalink / raw)
  To: linux-iio, linux-omap, devicetree
  Cc: David Lechner, Rob Herring, Mark Rutland, Benoît Cousson,
	Tony Lindgren, William Breathitt Gray, Thierry Reding,
	linux-kernel, linux-pwm

This series adds device tree bindings and a new counter driver for the Texas
Instruments Enhanced Quadrature Encoder Pulse (eQEP).

As mentioned in one of the commit messages, to start with, the driver only
supports reading the current counter value and setting the min/max values.
Other features can be added on an as-needed basis.

The only other feature I am interested in is adding is getting time data in
order to calculate the rotational speed of a motor. However, there probably
needs to be a higher level discussion of how this can fit into the counter
subsystem in general first.

This series has been tested on a BeagleBone Blue with the following script:

#!/usr/bin/env python3

from os import path
from time import sleep

COUNTER_PATH = '/sys/bus/counter/devices'
COUNTERS = ['counter0', 'counter1', 'counter2']
COUNT0 = 'count0'
COUNT = 'count'
CEILING = 'ceiling'
FLOOR = 'floor'
ENABLE = 'enable'

cnts = []

for c in COUNTERS:
    enable_path = path.join(COUNTER_PATH, c, COUNT0, ENABLE)
    with open(enable_path, 'w') as f:
        f.write('1')
    ceiling_path = path.join(COUNTER_PATH, c, COUNT0, CEILING)
    with open(ceiling_path, 'w') as f:
        f.write(str(0xffffffff))

    cnt_path = path.join(COUNTER_PATH, c, COUNT0, COUNT)
    cnts.append(open(cnt_path, 'r'))

while True:
    for c in cnts:
        c.seek(0)
        val = int(c.read())
        if val >= 0x80000000:
            val -= 0x100000000
        print(val, end=' ')
    print()
    sleep(1)

David Lechner (4):
  dt-bindings: counter: new bindings for TI eQEP
  counter: new TI eQEP driver
  ARM: dts: am33xx: Add nodes for eQEP
  ARM: dts: am335x-boneblue: Enable eQEP

 .../devicetree/bindings/counter/ti-eqep.txt   |  18 +
 MAINTAINERS                                   |   6 +
 arch/arm/boot/dts/am335x-boneblue.dts         |  54 +++
 arch/arm/boot/dts/am33xx-l4.dtsi              |  27 ++
 drivers/counter/Kconfig                       |  12 +
 drivers/counter/Makefile                      |   1 +
 drivers/counter/ti-eqep.c                     | 381 ++++++++++++++++++
 drivers/pwm/Kconfig                           |   2 +-
 8 files changed, 500 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/counter/ti-eqep.txt
 create mode 100644 drivers/counter/ti-eqep.c

-- 
2.17.1


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

* [PATCH 1/4] dt-bindings: counter: new bindings for TI eQEP
  2019-07-22 15:45 [PATCH 0/4] new driver for TI eQEP David Lechner
@ 2019-07-22 15:45 ` David Lechner
  2019-07-27 19:48   ` Jonathan Cameron
  2019-07-22 15:45 ` [PATCH 2/4] counter: new TI eQEP driver David Lechner
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 23+ messages in thread
From: David Lechner @ 2019-07-22 15:45 UTC (permalink / raw)
  To: linux-iio, linux-omap, devicetree
  Cc: David Lechner, Rob Herring, Mark Rutland, Benoît Cousson,
	Tony Lindgren, William Breathitt Gray, Thierry Reding,
	linux-kernel, linux-pwm

This documents device tree binding for the Texas Instruments Enhanced
Quadrature Encoder Pulse (eQEP) Module found in various TI SoCs.

Signed-off-by: David Lechner <david@lechnology.com>
---
 .../devicetree/bindings/counter/ti-eqep.txt    | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/counter/ti-eqep.txt

diff --git a/Documentation/devicetree/bindings/counter/ti-eqep.txt b/Documentation/devicetree/bindings/counter/ti-eqep.txt
new file mode 100644
index 000000000000..fbcebc2c2cc2
--- /dev/null
+++ b/Documentation/devicetree/bindings/counter/ti-eqep.txt
@@ -0,0 +1,18 @@
+Texas Instruments Enhanced Quadrature Encoder Pulse (eQEP) Module
+
+Required properties:
+- compatible:		Must be "ti,am3352-eqep".
+- reg:			Physical base address and size of the registers map.
+- clocks:		Handle to the PWM's functional clock.
+- clock-names:		Must be "fck".
+- interrupts:		Handle to the eQEP event interrupt
+
+Example:
+
+	eqep0: eqep@180 {
+		compatible = "ti,am3352-eqep";
+		reg = <0x180 0x80>;
+		clocks = <&l4ls_gclk>;
+		clock-names = "fck";
+		interrupts = <79>;
+	};
-- 
2.17.1


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

* [PATCH 2/4] counter: new TI eQEP driver
  2019-07-22 15:45 [PATCH 0/4] new driver for TI eQEP David Lechner
  2019-07-22 15:45 ` [PATCH 1/4] dt-bindings: counter: new bindings " David Lechner
@ 2019-07-22 15:45 ` David Lechner
  2019-07-30 12:35   ` Uwe Kleine-König
  2019-08-02  9:27   ` William Breathitt Gray
  2019-07-22 15:45 ` [PATCH 3/4] ARM: dts: am33xx: Add nodes for eQEP David Lechner
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 23+ messages in thread
From: David Lechner @ 2019-07-22 15:45 UTC (permalink / raw)
  To: linux-iio, linux-omap, devicetree
  Cc: David Lechner, Rob Herring, Mark Rutland, Benoît Cousson,
	Tony Lindgren, William Breathitt Gray, Thierry Reding,
	linux-kernel, linux-pwm

This adds a new counter driver for the Texas Instruments Enhanced
Quadrature Encoder Pulse (eQEP) module.

Only very basic functionality is currently implemented - only enough to
be able to read the position. The actual device has many more features
which can be added to the driver on an as-needed basis.

Signed-off-by: David Lechner <david@lechnology.com>
---
 MAINTAINERS               |   6 +
 drivers/counter/Kconfig   |  12 ++
 drivers/counter/Makefile  |   1 +
 drivers/counter/ti-eqep.c | 381 ++++++++++++++++++++++++++++++++++++++
 drivers/pwm/Kconfig       |   2 +-
 5 files changed, 401 insertions(+), 1 deletion(-)
 create mode 100644 drivers/counter/ti-eqep.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 783569e3c4b4..f3b5e275732b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16014,6 +16014,12 @@ S:	Maintained
 F:	drivers/media/platform/davinci/
 F:	include/media/davinci/
 
+TI ENHANCED QUADRATURE ENCODER PULSE (eQEP) DRIVER
+R:	David Lechner <david@lechnology.com>
+L:	linux-iio@vger.kernel.org
+F:	Documentation/devicetree/bindings/counter/ti-eqep.txt
+F:	drivers/counter/ti-eqep.c
+
 TI ETHERNET SWITCH DRIVER (CPSW)
 R:	Grygorii Strashko <grygorii.strashko@ti.com>
 L:	linux-omap@vger.kernel.org
diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
index 2967d0a9ff91..7eeb310f0cda 100644
--- a/drivers/counter/Kconfig
+++ b/drivers/counter/Kconfig
@@ -49,6 +49,18 @@ config STM32_LPTIMER_CNT
 	  To compile this driver as a module, choose M here: the
 	  module will be called stm32-lptimer-cnt.
 
+config TI_EQEP
+	tristate "TI eQEP counter driver"
+	depends on (SOC_AM33XX || COMPILE_TEST)
+	select PWM
+	select REGMAP_MMIO
+	help
+	  Select this option to enable the Texas Instruments Enhanced Quadrature
+	  Encoder Pulse (eQEP) counter driver.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ti-eqep.
+
 config FTM_QUADDEC
 	tristate "Flex Timer Module Quadrature decoder driver"
 	depends on HAS_IOMEM && OF
diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
index 40d35522937d..55142d1f4c43 100644
--- a/drivers/counter/Makefile
+++ b/drivers/counter/Makefile
@@ -8,4 +8,5 @@ obj-$(CONFIG_COUNTER) += counter.o
 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_TI_EQEP)		+= ti-eqep.o
 obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
new file mode 100644
index 000000000000..7aaa4abbc9c5
--- /dev/null
+++ b/drivers/counter/ti-eqep.c
@@ -0,0 +1,381 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2019 David Lechner <david@lechnology.com>
+ *
+ * Counter driver for Texas Instruments Enhanced Quadrature Encoder Pulse (eQEP)
+ */
+
+#include <linux/bitops.h>
+#include <linux/counter.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+
+/* 32-bit registers */
+#define QPOSCNT		0x0
+#define QPOSINIT	0x4
+#define QPOSMAX		0x8
+#define QPOSCMP		0xc
+#define QPOSILAT	0x10
+#define QPOSSLAT	0x14
+#define QPOSLAT		0x18
+#define QUTMR		0x1c
+#define QUPRD		0x20
+
+/* 16-bit registers */
+#define QWDTMR		0x0	/* 0x24 */
+#define QWDPRD		0x2	/* 0x26 */
+#define QDECCTL		0x4	/* 0x28 */
+#define QEPCTL		0x6	/* 0x2a */
+#define QCAPCTL		0x8	/* 0x2c */
+#define QPOSCTL		0xa	/* 0x2e */
+#define QEINT		0xc	/* 0x30 */
+#define QFLG		0xe	/* 0x32 */
+#define QCLR		0x10	/* 0x34 */
+#define QFRC		0x12	/* 0x36 */
+#define QEPSTS		0x14	/* 0x38 */
+#define QCTMR		0x16	/* 0x3a */
+#define QCPRD		0x18	/* 0x3c */
+#define QCTMRLAT	0x1a	/* 0x3e */
+#define QCPRDLAT	0x1c	/* 0x40 */
+
+#define QDECCTL_QSRC_SHIFT	14
+#define QDECCTL_QSRC		GENMASK(15, 14)
+#define QDECCTL_SOEN		BIT(13)
+#define QDECCTL_SPSEL		BIT(12)
+#define QDECCTL_XCR		BIT(11)
+#define QDECCTL_SWAP		BIT(10)
+#define QDECCTL_IGATE		BIT(9)
+#define QDECCTL_QAP		BIT(8)
+#define QDECCTL_QBP		BIT(7)
+#define QDECCTL_QIP		BIT(6)
+#define QDECCTL_QSP		BIT(5)
+
+#define QEPCTL_FREE_SOFT	GENMASK(15, 14)
+#define QEPCTL_PCRM		GENMASK(13, 12)
+#define QEPCTL_SEI		GENMASK(11, 10)
+#define QEPCTL_IEI		GENMASK(9, 8)
+#define QEPCTL_SWI		BIT(7)
+#define QEPCTL_SEL		BIT(6)
+#define QEPCTL_IEL		GENMASK(5, 4)
+#define QEPCTL_PHEN		BIT(3)
+#define QEPCTL_QCLM		BIT(2)
+#define QEPCTL_UTE		BIT(1)
+#define QEPCTL_WDE		BIT(0)
+
+/* EQEP Inputs */
+enum {
+	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
+	TI_EQEP_SIGNAL_QEPB,	/* QEPB/XDIR */
+	TI_EQEP_SIGNAL_QEPI,	/* Index */
+	TI_EQEP_SIGNAL_QEPS,	/* Strobe */
+};
+
+/* Position Counter Input Modes */
+enum {
+	TI_EQEP_COUNT_FUNC_QUAD_COUNT,
+	TI_EQEP_COUNT_FUNC_DIR_COUNT,
+	TI_EQEP_COUNT_FUNC_UP_COUNT,
+	TI_EQEP_COUNT_FUNC_DOWN_COUNT,
+};
+
+struct ti_eqep_cnt {
+	struct counter_device counter;
+	struct regmap *regmap32;
+	struct regmap *regmap16;
+};
+
+static int ti_eqep_count_read(struct counter_device *counter,
+			      struct counter_count *count,
+			      struct counter_count_read_value *val)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 cnt;
+
+	regmap_read(priv->regmap32, QPOSCNT, &cnt);
+	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cnt);
+
+	return 0;
+}
+
+static int ti_eqep_count_write(struct counter_device *counter,
+			       struct counter_count *count,
+			       struct counter_count_write_value *val)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 cnt, max;
+	int err;
+
+	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
+	if (err)
+		return err;
+
+	regmap_read(priv->regmap32, QPOSMAX, &max);
+	if (cnt > max)
+		return -EINVAL;
+
+	return regmap_write(priv->regmap32, QPOSCNT, cnt);
+}
+
+static int ti_eqep_function_get(struct counter_device *counter,
+				struct counter_count *count, size_t *function)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qdecctl;
+
+	regmap_read(priv->regmap16, QDECCTL, &qdecctl);
+	*function = (qdecctl & QDECCTL_QSRC) >> QDECCTL_QSRC_SHIFT;
+
+	return 0;
+}
+
+static int ti_eqep_function_set(struct counter_device *counter,
+				struct counter_count *count, size_t function)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+
+	return regmap_write_bits(priv->regmap16, QDECCTL, QDECCTL_QSRC,
+				 function << QDECCTL_QSRC_SHIFT);
+}
+
+static ssize_t ti_eqep_position_ceiling_read(struct counter_device *counter,
+					     struct counter_count *count,
+					     void *ext_priv, char *buf)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qposmax;
+
+	regmap_read(priv->regmap32, QPOSMAX, &qposmax);
+
+	return sprintf(buf, "%u\n", qposmax);
+}
+
+static ssize_t ti_eqep_position_ceiling_write(struct counter_device *counter,
+					      struct counter_count *count,
+					      void *ext_priv, const char *buf,
+					      size_t len)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	int err;
+	u32 res;
+
+	err = kstrtouint(buf, 10, &res);
+	if (err < 0)
+		return err;
+
+	regmap_write(priv->regmap32, QPOSMAX, res);
+
+	return len;
+}
+
+static ssize_t ti_eqep_position_floor_read(struct counter_device *counter,
+					   struct counter_count *count,
+					   void *ext_priv, char *buf)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qposinit;
+
+	regmap_read(priv->regmap32, QPOSINIT, &qposinit);
+
+	return sprintf(buf, "%u\n", qposinit);
+}
+
+static ssize_t ti_eqep_position_floor_write(struct counter_device *counter,
+					    struct counter_count *count,
+					    void *ext_priv, const char *buf,
+					    size_t len)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	int err;
+	u32 res;
+
+	err = kstrtouint(buf, 10, &res);
+	if (err < 0)
+		return err;
+
+	regmap_write(priv->regmap32, QPOSINIT, res);
+
+	return len;
+}
+
+static ssize_t ti_eqep_position_enable_read(struct counter_device *counter,
+					    struct counter_count *count,
+					    void *ext_priv, char *buf)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qepctl;
+
+	regmap_read(priv->regmap16, QEPCTL, &qepctl);
+
+	return sprintf(buf, "%u\n", !!(qepctl & QEPCTL_PHEN));
+}
+
+static ssize_t ti_eqep_position_enable_write(struct counter_device *counter,
+					     struct counter_count *count,
+					     void *ext_priv, const char *buf,
+					     size_t len)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	int err;
+	bool res;
+
+	err = kstrtobool(buf, &res);
+	if (err < 0)
+		return err;
+
+	regmap_write_bits(priv->regmap16, QEPCTL, QEPCTL_PHEN, res ? -1 : 0);
+
+	return len;
+}
+
+static const struct regmap_config ti_eqep_regmap32_config = {
+	.name = "32-bit",
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = 0x24,
+};
+
+static const struct regmap_config ti_eqep_regmap16_config = {
+	.name = "16-bit",
+	.reg_bits = 16,
+	.val_bits = 16,
+	.reg_stride = 2,
+	.max_register = 0x1e,
+};
+
+static const struct counter_ops ti_eqep_counter_ops = {
+	.count_read	= ti_eqep_count_read,
+	.count_write	= ti_eqep_count_write,
+	.function_get	= ti_eqep_function_get,
+	.function_set	= ti_eqep_function_set,
+};
+
+static const enum counter_count_function ti_eqep_position_functions[] = {
+	[TI_EQEP_COUNT_FUNC_QUAD_COUNT]	= COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
+	[TI_EQEP_COUNT_FUNC_DIR_COUNT]	= COUNTER_COUNT_FUNCTION_PULSE_DIRECTION,
+	[TI_EQEP_COUNT_FUNC_UP_COUNT]	= COUNTER_COUNT_FUNCTION_INCREASE,
+	[TI_EQEP_COUNT_FUNC_DOWN_COUNT]	= COUNTER_COUNT_FUNCTION_DECREASE,
+};
+
+static struct counter_signal ti_eqep_signals[] = {
+	[TI_EQEP_SIGNAL_QEPA] = {
+		.id = TI_EQEP_SIGNAL_QEPA,
+		.name = "QEPA"
+	},
+	[TI_EQEP_SIGNAL_QEPB] = {
+		.id = TI_EQEP_SIGNAL_QEPB,
+		.name = "QEPB"
+	},
+	[TI_EQEP_SIGNAL_QEPI] = {
+		.id = TI_EQEP_SIGNAL_QEPI,
+		.name = "QEPI"
+	},
+	[TI_EQEP_SIGNAL_QEPS] = {
+		.id = TI_EQEP_SIGNAL_QEPS,
+		.name = "QEPS"
+	},
+};
+
+static struct counter_count_ext ti_eqep_position_ext[] = {
+	{
+		.name	= "ceiling",
+		.read	= ti_eqep_position_ceiling_read,
+		.write	= ti_eqep_position_ceiling_write,
+	},
+	{
+		.name	= "floor",
+		.read	= ti_eqep_position_floor_read,
+		.write	= ti_eqep_position_floor_write,
+	},
+	{
+		.name	= "enable",
+		.read	= ti_eqep_position_enable_read,
+		.write	= ti_eqep_position_enable_write,
+	},
+};
+
+static struct counter_count ti_eqep_counts[] = {
+	{
+		.id		= 0,
+		.name		= "QPOSCNT",
+		.functions_list	= ti_eqep_position_functions,
+		.num_functions	= ARRAY_SIZE(ti_eqep_position_functions),
+		.ext		= ti_eqep_position_ext,
+		.num_ext	= ARRAY_SIZE(ti_eqep_position_ext),
+	},
+};
+
+static int ti_eqep_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ti_eqep_cnt *priv;
+	struct resource *res;
+	void __iomem *base;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	priv->regmap32 = devm_regmap_init_mmio(dev, base,
+					       &ti_eqep_regmap32_config);
+	if (IS_ERR(priv->regmap32))
+		return PTR_ERR(priv->regmap32);
+
+	priv->regmap16 = devm_regmap_init_mmio(dev, base + 0x24,
+					       &ti_eqep_regmap16_config);
+	if (IS_ERR(priv->regmap16))
+		return PTR_ERR(priv->regmap16);
+
+	priv->counter.name = dev_name(dev);
+	priv->counter.parent = dev;
+	priv->counter.ops = &ti_eqep_counter_ops;
+	priv->counter.counts = ti_eqep_counts;
+	priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts);
+	priv->counter.signals = ti_eqep_signals;
+	priv->counter.num_signals = ARRAY_SIZE(ti_eqep_signals);
+	priv->counter.priv = priv;
+
+	pm_runtime_enable(dev);
+	pm_runtime_get(dev);
+
+	return devm_counter_register(dev, &priv->counter);
+}
+
+static int ti_eqep_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	pm_runtime_put(dev),
+	pm_runtime_disable(dev);
+
+	return 0;
+}
+
+static const struct of_device_id ti_eqep_of_match[] = {
+	{ .compatible = "ti,am3352-eqep", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ti_eqep_of_match);
+
+static struct platform_driver ti_eqep_driver = {
+	.probe = ti_eqep_probe,
+	.remove = ti_eqep_remove,
+	.driver = {
+		.name = "ti-eqep-cnt",
+		.of_match_table = ti_eqep_of_match,
+	},
+};
+module_platform_driver(ti_eqep_driver);
+
+MODULE_AUTHOR("David Lechner <david@lechnology.com>");
+MODULE_DESCRIPTION("TI eQEP counter driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a7e57516959e..ddcbb8573894 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -499,7 +499,7 @@ config  PWM_TIEHRPWM
 
 config  PWM_TIPWMSS
 	bool
-	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM)
+	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM || TI_EQEP)
 	help
 	  PWM Subsystem driver support for AM33xx SOC.
 
-- 
2.17.1


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

* [PATCH 3/4] ARM: dts: am33xx: Add nodes for eQEP
  2019-07-22 15:45 [PATCH 0/4] new driver for TI eQEP David Lechner
  2019-07-22 15:45 ` [PATCH 1/4] dt-bindings: counter: new bindings " David Lechner
  2019-07-22 15:45 ` [PATCH 2/4] counter: new TI eQEP driver David Lechner
@ 2019-07-22 15:45 ` David Lechner
  2019-07-23  8:42   ` Tony Lindgren
  2019-07-22 15:45 ` [PATCH 4/4] ARM: dts: am335x-boneblue: Enable eQEP David Lechner
  2019-07-25 12:40 ` [PATCH 0/4] new driver for TI eQEP William Breathitt Gray
  4 siblings, 1 reply; 23+ messages in thread
From: David Lechner @ 2019-07-22 15:45 UTC (permalink / raw)
  To: linux-iio, linux-omap, devicetree
  Cc: David Lechner, Rob Herring, Mark Rutland, Benoît Cousson,
	Tony Lindgren, William Breathitt Gray, Thierry Reding,
	linux-kernel, linux-pwm

This adds new nodes for the Texas Instruments Enhanced Quadrature
Encoder Pulse (eQEP) module in the PWM subsystem on AM33XX.

Signed-off-by: David Lechner <david@lechnology.com>
---
 arch/arm/boot/dts/am33xx-l4.dtsi | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
index 3b1fb2ba4dff..7fdc2f61c553 100644
--- a/arch/arm/boot/dts/am33xx-l4.dtsi
+++ b/arch/arm/boot/dts/am33xx-l4.dtsi
@@ -1908,6 +1908,15 @@
 					status = "disabled";
 				};
 
+				eqep0: eqep@180 {
+					compatible = "ti,am3352-eqep";
+					reg = <0x180 0x80>;
+					clocks = <&l4ls_gclk>;
+					clock-names = "fck";
+					interrupts = <79>;
+					status = "disabled";
+				};
+
 				ehrpwm0: pwm@200 {
 					compatible = "ti,am3352-ehrpwm",
 						     "ti,am33xx-ehrpwm";
@@ -1961,6 +1970,15 @@
 					status = "disabled";
 				};
 
+				eqep1: eqep@180 {
+					compatible = "ti,am3352-eqep";
+					reg = <0x180 0x80>;
+					clocks = <&l4ls_gclk>;
+					clock-names = "fck";
+					interrupts = <88>;
+					status = "disabled";
+				};
+
 				ehrpwm1: pwm@200 {
 					compatible = "ti,am3352-ehrpwm",
 						     "ti,am33xx-ehrpwm";
@@ -2014,6 +2032,15 @@
 					status = "disabled";
 				};
 
+				eqep2: eqep@180 {
+					compatible = "ti,am3352-eqep";
+					reg = <0x180 0x80>;
+					clocks = <&l4ls_gclk>;
+					clock-names = "fck";
+					interrupts = <89>;
+					status = "disabled";
+				};
+
 				ehrpwm2: pwm@200 {
 					compatible = "ti,am3352-ehrpwm",
 						     "ti,am33xx-ehrpwm";
-- 
2.17.1


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

* [PATCH 4/4] ARM: dts: am335x-boneblue: Enable eQEP
  2019-07-22 15:45 [PATCH 0/4] new driver for TI eQEP David Lechner
                   ` (2 preceding siblings ...)
  2019-07-22 15:45 ` [PATCH 3/4] ARM: dts: am33xx: Add nodes for eQEP David Lechner
@ 2019-07-22 15:45 ` David Lechner
  2019-07-25 12:40 ` [PATCH 0/4] new driver for TI eQEP William Breathitt Gray
  4 siblings, 0 replies; 23+ messages in thread
From: David Lechner @ 2019-07-22 15:45 UTC (permalink / raw)
  To: linux-iio, linux-omap, devicetree
  Cc: David Lechner, Rob Herring, Mark Rutland, Benoît Cousson,
	Tony Lindgren, William Breathitt Gray, Thierry Reding,
	linux-kernel, linux-pwm

This enables the Enhanced Quadrature Encoder Pulse (eQEP) module for
connectors E1, E2 and E3 on BeagleBone Blue.

Signed-off-by: David Lechner <david@lechnology.com>
---
 arch/arm/boot/dts/am335x-boneblue.dts | 54 +++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/arch/arm/boot/dts/am335x-boneblue.dts b/arch/arm/boot/dts/am335x-boneblue.dts
index 0257576d5d16..df3978ce061c 100644
--- a/arch/arm/boot/dts/am335x-boneblue.dts
+++ b/arch/arm/boot/dts/am335x-boneblue.dts
@@ -258,6 +258,30 @@
 			AM33XX_PADCONF(AM335X_PIN_MII1_RXD0, PIN_OUTPUT, MUX_MODE7)		/* (M16) gmii1_rxd0.gpio2[21] */
 		>;
 	};
+
+	/* E1 */
+	eqep0_pins: pinmux_eqep0_pins {
+		pinctrl-single,pins = <
+			AM33XX_PADCONF(AM335X_PIN_MCASP0_AXR0, PIN_INPUT, MUX_MODE1)		/* (B12) mcasp0_aclkr.eQEP0A_in */
+			AM33XX_PADCONF(AM335X_PIN_MCASP0_FSR, PIN_INPUT, MUX_MODE1)		/* (C13) mcasp0_fsr.eQEP0B_in */
+		>;
+	};
+
+	/* E2 */
+	eqep1_pins: pinmux_eqep1_pins {
+		pinctrl-single,pins = <
+			AM33XX_PADCONF(AM335X_PIN_LCD_DATA12, PIN_INPUT, MUX_MODE2)		/* (V2) lcd_data12.eQEP1A_in */
+			AM33XX_PADCONF(AM335X_PIN_LCD_DATA13, PIN_INPUT, MUX_MODE2)		/* (V3) lcd_data13.eQEP1B_in */
+		>;
+	};
+
+	/* E3 */
+	eqep2_pins: pinmux_eqep2_pins {
+		pinctrl-single,pins = <
+			AM33XX_PADCONF(AM335X_PIN_GPMC_AD12, PIN_INPUT, MUX_MODE4)		/* (T12) gpmc_ad12.eQEP2A_in */
+			AM33XX_PADCONF(AM335X_PIN_GPMC_AD13, PIN_INPUT, MUX_MODE4)		/* (R12) gpmc_ad13.eQEP2B_in */
+		>;
+	};
 };
 
 &uart0 {
@@ -530,3 +554,33 @@
 		line-name = "LS_BUF_EN";
 	};
 };
+
+&epwmss0 {
+	status = "okay";
+};
+
+&eqep0 {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&eqep0_pins>;
+};
+
+&epwmss1 {
+	status = "okay";
+};
+
+&eqep1 {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&eqep1_pins>;
+};
+
+&epwmss2 {
+	status = "okay";
+};
+
+&eqep2 {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&eqep2_pins>;
+};
-- 
2.17.1


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

* Re: [PATCH 3/4] ARM: dts: am33xx: Add nodes for eQEP
  2019-07-22 15:45 ` [PATCH 3/4] ARM: dts: am33xx: Add nodes for eQEP David Lechner
@ 2019-07-23  8:42   ` Tony Lindgren
  2019-07-23 14:45     ` David Lechner
  0 siblings, 1 reply; 23+ messages in thread
From: Tony Lindgren @ 2019-07-23  8:42 UTC (permalink / raw)
  To: David Lechner
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, William Breathitt Gray, Thierry Reding,
	linux-kernel, linux-pwm

* David Lechner <david@lechnology.com> [190722 15:46]:
> This adds new nodes for the Texas Instruments Enhanced Quadrature
> Encoder Pulse (eQEP) module in the PWM subsystem on AM33XX.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  arch/arm/boot/dts/am33xx-l4.dtsi | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
> index 3b1fb2ba4dff..7fdc2f61c553 100644
> --- a/arch/arm/boot/dts/am33xx-l4.dtsi
> +++ b/arch/arm/boot/dts/am33xx-l4.dtsi
> @@ -1908,6 +1908,15 @@
>  					status = "disabled";
>  				};
>  
> +				eqep0: eqep@180 {
> +					compatible = "ti,am3352-eqep";
> +					reg = <0x180 0x80>;
> +					clocks = <&l4ls_gclk>;
> +					clock-names = "fck";
> +					interrupts = <79>;
> +					status = "disabled";
> +				};
> +

You probably no longer need to map any clocks here as this
is now a child of the interconnect target module managed
by ti-sysc driver. I have not checked but probably l4ls_gclk
is same as clocks = <&l4ls_clkctrl AM3_L4LS_EPWMSS0_CLKCTRL 0>
already managed by ti-sysc. If so, then just using runtime PM
calls in any of the child device drivers will keep it enabled.

If l4ls_gclk is a separate functional clock, then it still
needs to be managed by the child device driver directly.

Regards,

Tony

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

* Re: [PATCH 3/4] ARM: dts: am33xx: Add nodes for eQEP
  2019-07-23  8:42   ` Tony Lindgren
@ 2019-07-23 14:45     ` David Lechner
  2019-07-23 14:51       ` Tony Lindgren
  0 siblings, 1 reply; 23+ messages in thread
From: David Lechner @ 2019-07-23 14:45 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, William Breathitt Gray, Thierry Reding,
	linux-kernel, linux-pwm

On 7/23/19 3:42 AM, Tony Lindgren wrote:
> * David Lechner <david@lechnology.com> [190722 15:46]:
>> This adds new nodes for the Texas Instruments Enhanced Quadrature
>> Encoder Pulse (eQEP) module in the PWM subsystem on AM33XX.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>>   arch/arm/boot/dts/am33xx-l4.dtsi | 27 +++++++++++++++++++++++++++
>>   1 file changed, 27 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
>> index 3b1fb2ba4dff..7fdc2f61c553 100644
>> --- a/arch/arm/boot/dts/am33xx-l4.dtsi
>> +++ b/arch/arm/boot/dts/am33xx-l4.dtsi
>> @@ -1908,6 +1908,15 @@
>>   					status = "disabled";
>>   				};
>>   
>> +				eqep0: eqep@180 {
>> +					compatible = "ti,am3352-eqep";
>> +					reg = <0x180 0x80>;
>> +					clocks = <&l4ls_gclk>;
>> +					clock-names = "fck";
>> +					interrupts = <79>;
>> +					status = "disabled";
>> +				};
>> +
> 
> You probably no longer need to map any clocks here as this> is now a child of the interconnect target module managed
> by ti-sysc driver. I have not checked but probably l4ls_gclk
> is same as clocks = <&l4ls_clkctrl AM3_L4LS_EPWMSS0_CLKCTRL 0>
> already managed by ti-sysc. If so, then just using runtime PM
> calls in any of the child device drivers will keep it enabled.
> 
> If l4ls_gclk is a separate functional clock, then it still
> needs to be managed by the child device driver directly.

The clock is included so that we can get the clock rate for
the timing aspects of the eQEP, not for power management.

I chose to use the "fck" name to be consistent with the
sibling EHRPWM and ECAP nodes that already have the same
bindings for the same clock.

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

* Re: [PATCH 3/4] ARM: dts: am33xx: Add nodes for eQEP
  2019-07-23 14:45     ` David Lechner
@ 2019-07-23 14:51       ` Tony Lindgren
  0 siblings, 0 replies; 23+ messages in thread
From: Tony Lindgren @ 2019-07-23 14:51 UTC (permalink / raw)
  To: David Lechner
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, William Breathitt Gray, Thierry Reding,
	linux-kernel, linux-pwm

* David Lechner <david@lechnology.com> [190723 14:46]:
> On 7/23/19 3:42 AM, Tony Lindgren wrote:
> > * David Lechner <david@lechnology.com> [190722 15:46]:
> > > This adds new nodes for the Texas Instruments Enhanced Quadrature
> > > Encoder Pulse (eQEP) module in the PWM subsystem on AM33XX.
> > > 
> > > Signed-off-by: David Lechner <david@lechnology.com>
> > > ---
> > >   arch/arm/boot/dts/am33xx-l4.dtsi | 27 +++++++++++++++++++++++++++
> > >   1 file changed, 27 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
> > > index 3b1fb2ba4dff..7fdc2f61c553 100644
> > > --- a/arch/arm/boot/dts/am33xx-l4.dtsi
> > > +++ b/arch/arm/boot/dts/am33xx-l4.dtsi
> > > @@ -1908,6 +1908,15 @@
> > >   					status = "disabled";
> > >   				};
> > > +				eqep0: eqep@180 {
> > > +					compatible = "ti,am3352-eqep";
> > > +					reg = <0x180 0x80>;
> > > +					clocks = <&l4ls_gclk>;
> > > +					clock-names = "fck";
> > > +					interrupts = <79>;
> > > +					status = "disabled";
> > > +				};
> > > +
> > 
> > You probably no longer need to map any clocks here as this> is now a child of the interconnect target module managed
> > by ti-sysc driver. I have not checked but probably l4ls_gclk
> > is same as clocks = <&l4ls_clkctrl AM3_L4LS_EPWMSS0_CLKCTRL 0>
> > already managed by ti-sysc. If so, then just using runtime PM
> > calls in any of the child device drivers will keep it enabled.
> > 
> > If l4ls_gclk is a separate functional clock, then it still
> > needs to be managed by the child device driver directly.
> 
> The clock is included so that we can get the clock rate for
> the timing aspects of the eQEP, not for power management.
> 
> I chose to use the "fck" name to be consistent with the
> sibling EHRPWM and ECAP nodes that already have the same
> bindings for the same clock.

OK makes sense to me thanks.

Tony

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

* Re: [PATCH 0/4] new driver for TI eQEP
  2019-07-22 15:45 [PATCH 0/4] new driver for TI eQEP David Lechner
                   ` (3 preceding siblings ...)
  2019-07-22 15:45 ` [PATCH 4/4] ARM: dts: am335x-boneblue: Enable eQEP David Lechner
@ 2019-07-25 12:40 ` William Breathitt Gray
  2019-07-25 22:52   ` David Lechner
  4 siblings, 1 reply; 23+ messages in thread
From: William Breathitt Gray @ 2019-07-25 12:40 UTC (permalink / raw)
  To: David Lechner
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, Tony Lindgren, Thierry Reding, linux-kernel,
	linux-pwm

On Mon, Jul 22, 2019 at 10:45:34AM -0500, David Lechner wrote:
> This series adds device tree bindings and a new counter driver for the Texas
> Instruments Enhanced Quadrature Encoder Pulse (eQEP).
> 
> As mentioned in one of the commit messages, to start with, the driver only
> supports reading the current counter value and setting the min/max values.
> Other features can be added on an as-needed basis.
> 
> The only other feature I am interested in is adding is getting time data in
> order to calculate the rotational speed of a motor. However, there probably
> needs to be a higher level discussion of how this can fit into the counter
> subsystem in general first.

I believe exposing some sort of time data has merit. Quadrature counter
devices in particular are commonly used for position tracking of
automation systems, and such systems would benefit from velocity/speed
information. So let's try to introduce that sort of functionality in this
driver if possible.

First, let's discuss your specific use case and requirements, and hopefully we
can generalize it enough to be of use for future drivers. From your description,
it sounds like you're attaching some sort of rotary encoder to the eQEP device.
Is that correct? What sort of time data are you hoping to use; does the eQEP
device provide a clock value, or would you be grabbing a timestamp from the
system?

I'm not sure yet if it would make sense to expose rotational speed directly as
an attribute. If we were to expose just the count value and timestamp since the
last read, that should be enough for a user to compute the delta and derive
speed. I'll think more about this since some devices may simplify that case if
the hardware is able to compute the speed for us.

William Breathitt Gray

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

* Re: [PATCH 0/4] new driver for TI eQEP
  2019-07-25 12:40 ` [PATCH 0/4] new driver for TI eQEP William Breathitt Gray
@ 2019-07-25 22:52   ` David Lechner
  2019-07-27 19:45     ` Jonathan Cameron
  2019-07-30  4:45     ` William Breathitt Gray
  0 siblings, 2 replies; 23+ messages in thread
From: David Lechner @ 2019-07-25 22:52 UTC (permalink / raw)
  To: William Breathitt Gray
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, Tony Lindgren, Thierry Reding, linux-kernel,
	linux-pwm

On 7/25/19 7:40 AM, William Breathitt Gray wrote:
> On Mon, Jul 22, 2019 at 10:45:34AM -0500, David Lechner wrote:
>> This series adds device tree bindings and a new counter driver for the Texas
>> Instruments Enhanced Quadrature Encoder Pulse (eQEP).
>>
>> As mentioned in one of the commit messages, to start with, the driver only
>> supports reading the current counter value and setting the min/max values.
>> Other features can be added on an as-needed basis.
>>
>> The only other feature I am interested in is adding is getting time data in
>> order to calculate the rotational speed of a motor. However, there probably
>> needs to be a higher level discussion of how this can fit into the counter
>> subsystem in general first.
> 
> I believe exposing some sort of time data has merit. Quadrature counter
> devices in particular are commonly used for position tracking of
> automation systems, and such systems would benefit from velocity/speed
> information. So let's try to introduce that sort of functionality in this
> driver if possible.
> 
> First, let's discuss your specific use case and requirements, and hopefully we
> can generalize it enough to be of use for future drivers. From your description,
> it sounds like you're attaching some sort of rotary encoder to the eQEP device.
> Is that correct? What sort of time data are you hoping to use; does the eQEP
> device provide a clock value, or would you be grabbing a timestamp from the
> system?

My use case is robotics using LEGO MINDSTORMS. More specifically, I am using
motors that have a cheap optical rotary encoder (plastic wheel and infrared
LED/detectors) that give 360 counts per 1 rotation of the motor shaft. One count
is defined as the rising edge or falling edge of the A signal. We are looking at
anywhere from 0 to around 2000 counts per second. We use the speed as feedback in
a control algorithm to drive the motor at a constant speed. The control loop
updates on the order of 1 to 10 ms.

Because the encoder resolution and speeds are relatively low, we are currently
logging a timestamp for each count. If no count occurs for 50ms, then we log the
same count again with a new timestamp (otherwise we would never see 0 speed). To
get the actual speed, we find the first timestamp > 20 ms before the current
timestamp then compute the speed as the change in position divided by the change
in time between these two samples. This give a fairly accurate speed across most
of the range, but does get a bit noisy once we get below 100 counts per second.
It also means that we need a ring buffer that holds about 50 samples.

The timestamp itself comes from the eQEP, not the system. There are latching
registers to ensure that the timestamp read is from exactly the moment when
the count register was read.

  
> I'm not sure yet if it would make sense to expose rotational speed directly as
> an attribute. If we were to expose just the count value and timestamp since the
> last read, that should be enough for a user to compute the delta and derive
> speed. I'll think more about this since some devices may simplify that case if
> the hardware is able to compute the speed for us.
> 

I agree that it probably doesn't make sense to expect drivers to compute the
speed. There isn't really a general way to do that works for an arbitrary
speed. For example at high speeds, it is better to just look at the change
in counts over a fixed interval rather than triggering a timestamp based on
a certain number of counts.

I also don't think having a timestamp sysfs attribute would be very useful.
To make it work at all, I think it would have to be implemented such that
it returns the timestamp for the count that was most recently read via sysfs.
And it would require 4 syscalls (2 seeks and 2 reads) to get a single count/
timestamp pair in a control loop. On a 300MHz ARM9 processor, this is not
a negligible amount of time.

I noticed that several of the other counter drivers also register an IIO
device. So this got me thinking that perhaps the counter subsystem should
just be for configuring a counter device an then the IIO subsystem should
be used for triggers and ring buffers.

For the general case a counter device could have two possible triggers.
One that triggers an interrupt after X counts and another that triggers
with a period of T nanoseconds (or microseconds). Both triggers would add
a count/timestamp pair to an IIO ring buffer.

To fully reproduce our current methodology the first trigger would actually
need two configurable settings, the count X that triggers every X counts and
a watchdog time setting (using terminology from eQEP docs) that will also
trigger if and only if the count does not change before the time has elapsed.
Note, this is different from the other proposed time based trigger which
would cause a trigger interrupt at a fixed period regardless of whether
the count changed or not.

---

Thinking more generally though, I think what I would propose is adding a new
component to the existing list of Count, Signal and Synapse. The new component
could be called Event. Event would be more general than the trigger conditions
I have just discussed. In addition to those two, it could be any event
generated by the hardware, such as an error condition or a change in direction.

Drivers could register an arbitrary number of events for each Count, so we
would have /sys/bus/counter/devices/counterX/eventY/*. There should be a few
standard attributes, like "name" and "enable". Configurable events would need
ext attributes to allow configuration.

However, I see that there are already preset and error_noise "events" for
count objects, so maybe we don't do the eventY thing and keep it flatter (or
is the counter subsystem still considered in "staging" where breaking ABI
changes could be made?).

When thinking about what events would actually do when enabled though, it
seems like we should be using IIO events and triggers (we have found reading
sysfs attributes to be insufficient performance-wise). It seems like unnecessary
work to reproduce all of this in the counter subsystem. Which makes me wonder if
it would be better to have counter devices just be a different device type (i.e.
different struct device_type for dev->type) in the IIO subsystem instead of
creating a completely new subsystem.

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

* Re: [PATCH 0/4] new driver for TI eQEP
  2019-07-25 22:52   ` David Lechner
@ 2019-07-27 19:45     ` Jonathan Cameron
  2019-07-30  4:45     ` William Breathitt Gray
  1 sibling, 0 replies; 23+ messages in thread
From: Jonathan Cameron @ 2019-07-27 19:45 UTC (permalink / raw)
  To: David Lechner
  Cc: William Breathitt Gray, linux-iio, linux-omap, devicetree,
	Rob Herring, Mark Rutland, Benoît Cousson, Tony Lindgren,
	Thierry Reding, linux-kernel, linux-pwm

On Thu, 25 Jul 2019 17:52:21 -0500
David Lechner <david@lechnology.com> wrote:

> On 7/25/19 7:40 AM, William Breathitt Gray wrote:
> > On Mon, Jul 22, 2019 at 10:45:34AM -0500, David Lechner wrote:  
> >> This series adds device tree bindings and a new counter driver for the Texas
> >> Instruments Enhanced Quadrature Encoder Pulse (eQEP).
> >>
> >> As mentioned in one of the commit messages, to start with, the driver only
> >> supports reading the current counter value and setting the min/max values.
> >> Other features can be added on an as-needed basis.
> >>
> >> The only other feature I am interested in is adding is getting time data in
> >> order to calculate the rotational speed of a motor. However, there probably
> >> needs to be a higher level discussion of how this can fit into the counter
> >> subsystem in general first.  
> > 
> > I believe exposing some sort of time data has merit. Quadrature counter
> > devices in particular are commonly used for position tracking of
> > automation systems, and such systems would benefit from velocity/speed
> > information. So let's try to introduce that sort of functionality in this
> > driver if possible.
> > 
> > First, let's discuss your specific use case and requirements, and hopefully we
> > can generalize it enough to be of use for future drivers. From your description,
> > it sounds like you're attaching some sort of rotary encoder to the eQEP device.
> > Is that correct? What sort of time data are you hoping to use; does the eQEP
> > device provide a clock value, or would you be grabbing a timestamp from the
> > system?  
> 
> My use case is robotics using LEGO MINDSTORMS. More specifically, I am using
> motors that have a cheap optical rotary encoder (plastic wheel and infrared
> LED/detectors) that give 360 counts per 1 rotation of the motor shaft. One count
> is defined as the rising edge or falling edge of the A signal. We are looking at
> anywhere from 0 to around 2000 counts per second. We use the speed as feedback in
> a control algorithm to drive the motor at a constant speed. The control loop
> updates on the order of 1 to 10 ms.
> 
> Because the encoder resolution and speeds are relatively low, we are currently
> logging a timestamp for each count. If no count occurs for 50ms, then we log the
> same count again with a new timestamp (otherwise we would never see 0 speed). To
> get the actual speed, we find the first timestamp > 20 ms before the current
> timestamp then compute the speed as the change in position divided by the change
> in time between these two samples. This give a fairly accurate speed across most
> of the range, but does get a bit noisy once we get below 100 counts per second.
> It also means that we need a ring buffer that holds about 50 samples.
> 
> The timestamp itself comes from the eQEP, not the system. There are latching
> registers to ensure that the timestamp read is from exactly the moment when
> the count register was read.
> 
>   
> > I'm not sure yet if it would make sense to expose rotational speed directly as
> > an attribute. If we were to expose just the count value and timestamp since the
> > last read, that should be enough for a user to compute the delta and derive
> > speed. I'll think more about this since some devices may simplify that case if
> > the hardware is able to compute the speed for us.
> >   
> 
> I agree that it probably doesn't make sense to expect drivers to compute the
> speed. There isn't really a general way to do that works for an arbitrary
> speed. For example at high speeds, it is better to just look at the change
> in counts over a fixed interval rather than triggering a timestamp based on
> a certain number of counts.

Worth noting perhaps that userspace has the same problem with knowing the
right approach...

> 
> I also don't think having a timestamp sysfs attribute would be very useful.
> To make it work at all, I think it would have to be implemented such that
> it returns the timestamp for the count that was most recently read via sysfs.
> And it would require 4 syscalls (2 seeks and 2 reads) to get a single count/
> timestamp pair in a control loop. On a 300MHz ARM9 processor, this is not
> a negligible amount of time.
> 
> I noticed that several of the other counter drivers also register an IIO
> device. So this got me thinking that perhaps the counter subsystem should
> just be for configuring a counter device an then the IIO subsystem should
> be used for triggers and ring buffers.
> 
> For the general case a counter device could have two possible triggers.
> One that triggers an interrupt after X counts and another that triggers
> with a period of T nanoseconds (or microseconds). Both triggers would add
> a count/timestamp pair to an IIO ring buffer.
> 
> To fully reproduce our current methodology the first trigger would actually
> need two configurable settings, the count X that triggers every X counts and
> a watchdog time setting (using terminology from eQEP docs) that will also
> trigger if and only if the count does not change before the time has elapsed.
> Note, this is different from the other proposed time based trigger which
> would cause a trigger interrupt at a fixed period regardless of whether
> the count changed or not.
> 
> ---
> 
> Thinking more generally though, I think what I would propose is adding a new
> component to the existing list of Count, Signal and Synapse. The new component
> could be called Event. Event would be more general than the trigger conditions
> I have just discussed. In addition to those two, it could be any event
> generated by the hardware, such as an error condition or a change in direction.
> 
> Drivers could register an arbitrary number of events for each Count, so we
> would have /sys/bus/counter/devices/counterX/eventY/*. There should be a few
> standard attributes, like "name" and "enable". Configurable events would need
> ext attributes to allow configuration.
> 
> However, I see that there are already preset and error_noise "events" for
> count objects, so maybe we don't do the eventY thing and keep it flatter (or
> is the counter subsystem still considered in "staging" where breaking ABI
> changes could be made?).

No.  I would say it can be extended but compatibility needs to be maintained.

> 
> When thinking about what events would actually do when enabled though, it
> seems like we should be using IIO events and triggers (we have found reading
> sysfs attributes to be insufficient performance-wise). It seems like unnecessary
> work to reproduce all of this in the counter subsystem. Which makes me wonder if
> it would be better to have counter devices just be a different device type (i.e.
> different struct device_type for dev->type) in the IIO subsystem instead of
> creating a completely new subsystem.

Hmm. That might have sort of worked, rather than the full split we went with
when it became clear counters really didn't fit the IIO model,
but it may not have come out cleanly and wouldn't have shared all that much code
(I think...) In particular the buffered paths in IIO present a large and
complex interface which would need to be replace given the different
fundamental structures in the counter abstraction.

It may be that the best thing to do here is to use very similar concepts
and implement some sort of kfifo interface for counters.  There may
well even be code to share, but I don't think we want to go back to 
trying to handle these as devices within IIO.

Tricky to know how well this would work without a prototype though...

Jonathan

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

* Re: [PATCH 1/4] dt-bindings: counter: new bindings for TI eQEP
  2019-07-22 15:45 ` [PATCH 1/4] dt-bindings: counter: new bindings " David Lechner
@ 2019-07-27 19:48   ` Jonathan Cameron
  2019-08-02  7:25     ` William Breathitt Gray
  0 siblings, 1 reply; 23+ messages in thread
From: Jonathan Cameron @ 2019-07-27 19:48 UTC (permalink / raw)
  To: David Lechner
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, Tony Lindgren, William Breathitt Gray,
	Thierry Reding, linux-kernel, linux-pwm

On Mon, 22 Jul 2019 10:45:35 -0500
David Lechner <david@lechnology.com> wrote:

> This documents device tree binding for the Texas Instruments Enhanced
> Quadrature Encoder Pulse (eQEP) Module found in various TI SoCs.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

Up to William given it is a counter binding, (unless Rob overrules)
but new bindings are generally preferred as yaml.

Content looks fine to me.

Thanks,

Jonathan

> ---
>  .../devicetree/bindings/counter/ti-eqep.txt    | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/counter/ti-eqep.txt
> 
> diff --git a/Documentation/devicetree/bindings/counter/ti-eqep.txt b/Documentation/devicetree/bindings/counter/ti-eqep.txt
> new file mode 100644
> index 000000000000..fbcebc2c2cc2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/counter/ti-eqep.txt
> @@ -0,0 +1,18 @@
> +Texas Instruments Enhanced Quadrature Encoder Pulse (eQEP) Module
> +
> +Required properties:
> +- compatible:		Must be "ti,am3352-eqep".
> +- reg:			Physical base address and size of the registers map.
> +- clocks:		Handle to the PWM's functional clock.
> +- clock-names:		Must be "fck".
> +- interrupts:		Handle to the eQEP event interrupt
> +
> +Example:
> +
> +	eqep0: eqep@180 {
> +		compatible = "ti,am3352-eqep";
> +		reg = <0x180 0x80>;
> +		clocks = <&l4ls_gclk>;
> +		clock-names = "fck";
> +		interrupts = <79>;
> +	};


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

* Re: [PATCH 0/4] new driver for TI eQEP
  2019-07-25 22:52   ` David Lechner
  2019-07-27 19:45     ` Jonathan Cameron
@ 2019-07-30  4:45     ` William Breathitt Gray
  2019-08-01 17:37       ` David Lechner
  1 sibling, 1 reply; 23+ messages in thread
From: William Breathitt Gray @ 2019-07-30  4:45 UTC (permalink / raw)
  To: David Lechner
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, Tony Lindgren, Thierry Reding, linux-kernel,
	linux-pwm

On Thu, Jul 25, 2019 at 05:52:21PM -0500, David Lechner wrote:
> On 7/25/19 7:40 AM, William Breathitt Gray wrote:
> > On Mon, Jul 22, 2019 at 10:45:34AM -0500, David Lechner wrote:
> >> This series adds device tree bindings and a new counter driver for the Texas
> >> Instruments Enhanced Quadrature Encoder Pulse (eQEP).
> >>
> >> As mentioned in one of the commit messages, to start with, the driver only
> >> supports reading the current counter value and setting the min/max values.
> >> Other features can be added on an as-needed basis.
> >>
> >> The only other feature I am interested in is adding is getting time data in
> >> order to calculate the rotational speed of a motor. However, there probably
> >> needs to be a higher level discussion of how this can fit into the counter
> >> subsystem in general first.
> > 
> > I believe exposing some sort of time data has merit. Quadrature counter
> > devices in particular are commonly used for position tracking of
> > automation systems, and such systems would benefit from velocity/speed
> > information. So let's try to introduce that sort of functionality in this
> > driver if possible.
> > 
> > First, let's discuss your specific use case and requirements, and hopefully we
> > can generalize it enough to be of use for future drivers. From your description,
> > it sounds like you're attaching some sort of rotary encoder to the eQEP device.
> > Is that correct? What sort of time data are you hoping to use; does the eQEP
> > device provide a clock value, or would you be grabbing a timestamp from the
> > system?
> 
> My use case is robotics using LEGO MINDSTORMS. More specifically, I am using
> motors that have a cheap optical rotary encoder (plastic wheel and infrared
> LED/detectors) that give 360 counts per 1 rotation of the motor shaft. One count
> is defined as the rising edge or falling edge of the A signal. We are looking at
> anywhere from 0 to around 2000 counts per second. We use the speed as feedback in
> a control algorithm to drive the motor at a constant speed. The control loop
> updates on the order of 1 to 10 ms.
> 
> Because the encoder resolution and speeds are relatively low, we are currently
> logging a timestamp for each count. If no count occurs for 50ms, then we log the
> same count again with a new timestamp (otherwise we would never see 0 speed). To
> get the actual speed, we find the first timestamp > 20 ms before the current
> timestamp then compute the speed as the change in position divided by the change
> in time between these two samples. This give a fairly accurate speed across most
> of the range, but does get a bit noisy once we get below 100 counts per second.
> It also means that we need a ring buffer that holds about 50 samples.
> 
> The timestamp itself comes from the eQEP, not the system. There are latching
> registers to ensure that the timestamp read is from exactly the moment when
> the count register was read.

So if I understand correctly, there are two registers you're reading: a
count register and a timestamp register. The count register is updated
by the rotation of the motor shaft, while the timestamp register is
updated by reading the count register (thus logging the time associated
with the read count value).

> > I'm not sure yet if it would make sense to expose rotational speed directly as
> > an attribute. If we were to expose just the count value and timestamp since the
> > last read, that should be enough for a user to compute the delta and derive
> > speed. I'll think more about this since some devices may simplify that case if
> > the hardware is able to compute the speed for us.
> > 
> 
> I agree that it probably doesn't make sense to expect drivers to compute the
> speed. There isn't really a general way to do that works for an arbitrary
> speed. For example at high speeds, it is better to just look at the change
> in counts over a fixed interval rather than triggering a timestamp based on
> a certain number of counts.

This is a good point. Depending on the resolution the user cares about,
they may be more interested in the speed over a short time interval
versus a long time interval. It doesn't seem practical to have the driver
try to handle all possible speed calculations when the user can decide
themselves how best to use the data.

> I also don't think having a timestamp sysfs attribute would be very useful.
> To make it work at all, I think it would have to be implemented such that
> it returns the timestamp for the count that was most recently read via sysfs.
> And it would require 4 syscalls (2 seeks and 2 reads) to get a single count/
> timestamp pair in a control loop. On a 300MHz ARM9 processor, this is not
> a negligible amount of time.

This is a concern I've had as well. The sysfs interface is useful in
that it provides an intuitive and human-friendly way to expose data
about devices. But as you note, there is considerable overhead in the
amount of syscalls we have to make to interact with multiple attributes.

One solution that may work is providing a character device interface in
addition to the sysfs interface. I believe that should reduce the
syscall overhead since a user can pass in a data structure with a
configuration defining what data/actions they want, and receive back
all data in a single syscall.

I think concern over latency was one of the reasons the GPIO subsystem
gained a character device interface as well. It's an addition to the
Counter subsystem that is worth considering, but the possible downsides
to such an interface should also be investigated.
 
> I noticed that several of the other counter drivers also register an IIO
> device. So this got me thinking that perhaps the counter subsystem should
> just be for configuring a counter device an then the IIO subsystem should
> be used for triggers and ring buffers.
> 
> For the general case a counter device could have two possible triggers.
> One that triggers an interrupt after X counts and another that triggers
> with a period of T nanoseconds (or microseconds). Both triggers would add
> a count/timestamp pair to an IIO ring buffer.
> 
> To fully reproduce our current methodology the first trigger would actually
> need two configurable settings, the count X that triggers every X counts and
> a watchdog time setting (using terminology from eQEP docs) that will also
> trigger if and only if the count does not change before the time has elapsed.
> Note, this is different from the other proposed time based trigger which
> would cause a trigger interrupt at a fixed period regardless of whether
> the count changed or not.

The counter drivers in the kernel right now are registering IIO devices
in order to keep the preexisting (but now deprecated) IIO Counter
interface working for these devices -- some users may be using this
older interface so we don't want to remove it cold turkey. Regardless,
there's nothing the prevents incorporating the IIO interface with your
Counter drivers; in fact, in some circumstances it's better that you do
just that.

The key idea to recognize is how the Counter subsystem differs from the
IIO subsystem on a conceptual level: the IIO subsystem provides an
interface for your device by describing it on a hardware level, whereas
the Counter subsystem provides an interface for your device by
describing it on a more abstract level.

What I mean is that every interface interaction in the Counter subsystem
relates to the abstract concept of an ideal "counter device" (Counts,
Synapses, Signals); if a device functionality or data does not relate
directly to those ideal counter device components, then the Counter
subsystem isn't that right interface for it.

For example, it makes sense to have an "enable" attribute or "present"
attribute, because these functionalities/data are directly related to
the Count, Synapse, and Signal components conceptually. However, in the
Counter subsystem you will likely not see something like the IIO
"in_voltageY_supply_raw" attribute -- not because that data is not
useful to know about for the operation of the counter device hardware,
but because it is outside the scope of the Counter subsystem paradigm
(i.e. it does not directly related to Counts, Synapses, or Signals).
As such, this would be a case where the counter driver should register
both a Counter device and IIO device, one to handle the counter device
on an abstract level while the other provides an interface for control
of the more specific hardware details.

> ---
> 
> Thinking more generally though, I think what I would propose is adding a new
> component to the existing list of Count, Signal and Synapse. The new component
> could be called Event. Event would be more general than the trigger conditions
> I have just discussed. In addition to those two, it could be any event
> generated by the hardware, such as an error condition or a change in direction.
> 
> Drivers could register an arbitrary number of events for each Count, so we
> would have /sys/bus/counter/devices/counterX/eventY/*. There should be a few
> standard attributes, like "name" and "enable". Configurable events would need
> ext attributes to allow configuration.
> 
> However, I see that there are already preset and error_noise "events" for
> count objects, so maybe we don't do the eventY thing and keep it flatter (or
> is the counter subsystem still considered in "staging" where breaking ABI
> changes could be made?).

The components for handling events already exist in the Counter
interface paradigm: Signals and Synapses. Although, the Counter
subsystem is currently lacking the implementation (I still need to code
in support for interrupts and such), the paradigm itself supports the
concept of events and triggers.

Recall that the Counter subsystem represents hardware via the
abstraction of an idealized "counter device". This is important to
understand because it means that Signals are not necessarily limited to
the physical wires of the hardware. To summarize the Counter interface
paradigm:

    * A Signal is a stream of data to be evaluated.
    * A Synapse is a trigger condition based on the evaluation of the
      data streams (i.e. the Signals).
    * A Count is the accumulation of the effects of Synapses (i.e. the
      triggers).

As such, in order to represent an event, you would add in a Signal to
represent the stream of device events, and a Synapse defining the
specific event that will trigger the action. I'll give an example in
order to demonstrate what I mean.

A simple clock can be conceptualize as a proper counter device: an
oscillation is a Signal, a rising edge from that oscillation line can be
the Synapse, and the current clock value is the Count.

                Count                Synapse          Signal
                -----                -------          ------
        +---------------------+
        | Data: Clock ticks   |    Rising Edge     _____________
        | Function: Increase  |  <-------------   / Oscillation \
        |                     |                  _________________
        +---------------------+

Now, in order to represent your timestamping clock we need two Signals:
a simple clock and an event stream. The simple clock is the source of
the current clock ticks we will store, while the event stream provides
the rotation count register read notification that will trigger the
timestamp.

                   Count                       Synapse      Signal
                   -----                       -------      ------
        +-------------------------------+
        | Data: Timestamp               |       None        _______
        | Function: Current clock ticks |  <------------   / Clock \
        |                               |                 ___________
        |                               |
        |                               |    Read event     ________
        |                               |  <------------   / Events \
        |                               |                 ____________
        +-------------------------------+

Note that in this case both Signals either do not exist in or are not
exposed by the hardware (maybe the simple clock is exposed, but it's not
necessary to be) -- they are meant to be abstract representations of the
components of the timestamp clock as an idealized "counter device".

By organizing the timestamp clock in this way, we can control and
configure the components using the standard Counter interface: common
attributes such as "name", "preset", "enable", etc. can now be exposed
to users like every other counter device component.

In theory we can sleep on the timestamp count attribute read (or
character device equivalent if we go down that route), and be woken when
an event triggers updating the timestamp value. However, the current
Counter subsystem implementation is lacking the code for this so it
needs to be added to the core functionality first.

> When thinking about what events would actually do when enabled though, it
> seems like we should be using IIO events and triggers (we have found reading
> sysfs attributes to be insufficient performance-wise). It seems like unnecessary
> work to reproduce all of this in the counter subsystem. Which makes me wonder if
> it would be better to have counter devices just be a different device type (i.e.
> different struct device_type for dev->type) in the IIO subsystem instead of
> creating a completely new subsystem.

I plan on adding interrupt support for the 104-QUAD-8 counter driver
since this device has some useful interrupts on configured threshold
conditions and such, so having the ability to handle an event rather
than constantly read and loop is something I want to have in the Counter
subsystem.

It's possible that I can reuse some code from the IIO subsystem, as
Jonathan pointed out, but overall I believe these should be separate
subsystems. From the reasons described above, the IIO subsystem and
Counter subsystem have different goals and thus different
implementations. I don't think that's a bad thing, and we can share code
in the few cases where the two may overlap.

Regarding whether to use IIO events and triggers within the TI eQEP
counter driver, I think we should wait for a proper Counter subsystem
implementation to be added first. My fear is that we'll have a similar
situation as what happened with IIO_COUNT, where we'll have to keep a
IIO interface present with a newer Counter interface. If adding in event
support to the Counter subsystem will take too long, we can add this TI
eQEP driver as-is now and later add in the timestamp support.

William Breathitt Gray

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

* Re: [PATCH 2/4] counter: new TI eQEP driver
  2019-07-22 15:45 ` [PATCH 2/4] counter: new TI eQEP driver David Lechner
@ 2019-07-30 12:35   ` Uwe Kleine-König
  2019-07-30 15:28     ` David Lechner
  2019-08-02  9:27   ` William Breathitt Gray
  1 sibling, 1 reply; 23+ messages in thread
From: Uwe Kleine-König @ 2019-07-30 12:35 UTC (permalink / raw)
  To: David Lechner
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, Tony Lindgren, William Breathitt Gray,
	Thierry Reding, linux-kernel, linux-pwm

On Mon, Jul 22, 2019 at 10:45:36AM -0500, David Lechner wrote:
> This adds a new counter driver for the Texas Instruments Enhanced
> Quadrature Encoder Pulse (eQEP) module.
> 
> Only very basic functionality is currently implemented - only enough to
> be able to read the position. The actual device has many more features
> which can be added to the driver on an as-needed basis.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  MAINTAINERS               |   6 +
>  drivers/counter/Kconfig   |  12 ++
>  drivers/counter/Makefile  |   1 +
>  drivers/counter/ti-eqep.c | 381 ++++++++++++++++++++++++++++++++++++++
>  drivers/pwm/Kconfig       |   2 +-

It's not obvious why the change to drivers/pwm/Kconfig is needed. Can
you please motivate that in the change log?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

* Re: [PATCH 2/4] counter: new TI eQEP driver
  2019-07-30 12:35   ` Uwe Kleine-König
@ 2019-07-30 15:28     ` David Lechner
  0 siblings, 0 replies; 23+ messages in thread
From: David Lechner @ 2019-07-30 15:28 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, Tony Lindgren, William Breathitt Gray,
	Thierry Reding, linux-kernel, linux-pwm

On 7/30/19 7:35 AM, Uwe Kleine-König wrote:
> On Mon, Jul 22, 2019 at 10:45:36AM -0500, David Lechner wrote:
>> This adds a new counter driver for the Texas Instruments Enhanced
>> Quadrature Encoder Pulse (eQEP) module.
>>
>> Only very basic functionality is currently implemented - only enough to
>> be able to read the position. The actual device has many more features
>> which can be added to the driver on an as-needed basis.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>>   MAINTAINERS               |   6 +
>>   drivers/counter/Kconfig   |  12 ++
>>   drivers/counter/Makefile  |   1 +
>>   drivers/counter/ti-eqep.c | 381 ++++++++++++++++++++++++++++++++++++++
>>   drivers/pwm/Kconfig       |   2 +-
> 
> It's not obvious why the change to drivers/pwm/Kconfig is needed. Can
> you please motivate that in the change log?

Will do. The short version is that it is needed for power management.

> 
> Best regards
> Uwe
> 


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

* Re: [PATCH 0/4] new driver for TI eQEP
  2019-07-30  4:45     ` William Breathitt Gray
@ 2019-08-01 17:37       ` David Lechner
  0 siblings, 0 replies; 23+ messages in thread
From: David Lechner @ 2019-08-01 17:37 UTC (permalink / raw)
  To: William Breathitt Gray
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, Tony Lindgren, Thierry Reding, linux-kernel,
	linux-pwm

On 7/29/19 11:45 PM, William Breathitt Gray wrote:
> On Thu, Jul 25, 2019 at 05:52:21PM -0500, David Lechner wrote:
>> On 7/25/19 7:40 AM, William Breathitt Gray wrote:
>>> On Mon, Jul 22, 2019 at 10:45:34AM -0500, David Lechner wrote:
>>>> This series adds device tree bindings and a new counter driver for the Texas
>>>> Instruments Enhanced Quadrature Encoder Pulse (eQEP).
>>>>
>>>> As mentioned in one of the commit messages, to start with, the driver only
>>>> supports reading the current counter value and setting the min/max values.
>>>> Other features can be added on an as-needed basis.
>>>>
>>>> The only other feature I am interested in is adding is getting time data in
>>>> order to calculate the rotational speed of a motor. However, there probably
>>>> needs to be a higher level discussion of how this can fit into the counter
>>>> subsystem in general first.
>>>
>>> I believe exposing some sort of time data has merit. Quadrature counter
>>> devices in particular are commonly used for position tracking of
>>> automation systems, and such systems would benefit from velocity/speed
>>> information. So let's try to introduce that sort of functionality in this
>>> driver if possible.
>>>
>>> First, let's discuss your specific use case and requirements, and hopefully we
>>> can generalize it enough to be of use for future drivers. From your description,
>>> it sounds like you're attaching some sort of rotary encoder to the eQEP device.
>>> Is that correct? What sort of time data are you hoping to use; does the eQEP
>>> device provide a clock value, or would you be grabbing a timestamp from the
>>> system?
>>
>> My use case is robotics using LEGO MINDSTORMS. More specifically, I am using
>> motors that have a cheap optical rotary encoder (plastic wheel and infrared
>> LED/detectors) that give 360 counts per 1 rotation of the motor shaft. One count
>> is defined as the rising edge or falling edge of the A signal. We are looking at
>> anywhere from 0 to around 2000 counts per second. We use the speed as feedback in
>> a control algorithm to drive the motor at a constant speed. The control loop
>> updates on the order of 1 to 10 ms.
>>
>> Because the encoder resolution and speeds are relatively low, we are currently
>> logging a timestamp for each count. If no count occurs for 50ms, then we log the
>> same count again with a new timestamp (otherwise we would never see 0 speed). To
>> get the actual speed, we find the first timestamp > 20 ms before the current
>> timestamp then compute the speed as the change in position divided by the change
>> in time between these two samples. This give a fairly accurate speed across most
>> of the range, but does get a bit noisy once we get below 100 counts per second.
>> It also means that we need a ring buffer that holds about 50 samples.
>>
>> The timestamp itself comes from the eQEP, not the system. There are latching
>> registers to ensure that the timestamp read is from exactly the moment when
>> the count register was read.
> 
> So if I understand correctly, there are two registers you're reading: a
> count register and a timestamp register. The count register is updated
> by the rotation of the motor shaft, while the timestamp register is
> updated by reading the count register (thus logging the time associated
> with the read count value).

That is correct.

> 
>>> I'm not sure yet if it would make sense to expose rotational speed directly as
>>> an attribute. If we were to expose just the count value and timestamp since the
>>> last read, that should be enough for a user to compute the delta and derive
>>> speed. I'll think more about this since some devices may simplify that case if
>>> the hardware is able to compute the speed for us.
>>>
>>
>> I agree that it probably doesn't make sense to expect drivers to compute the
>> speed. There isn't really a general way to do that works for an arbitrary
>> speed. For example at high speeds, it is better to just look at the change
>> in counts over a fixed interval rather than triggering a timestamp based on
>> a certain number of counts.
> 
> This is a good point. Depending on the resolution the user cares about,
> they may be more interested in the speed over a short time interval
> versus a long time interval. It doesn't seem practical to have the driver
> try to handle all possible speed calculations when the user can decide
> themselves how best to use the data.
> 
>> I also don't think having a timestamp sysfs attribute would be very useful.
>> To make it work at all, I think it would have to be implemented such that
>> it returns the timestamp for the count that was most recently read via sysfs.
>> And it would require 4 syscalls (2 seeks and 2 reads) to get a single count/
>> timestamp pair in a control loop. On a 300MHz ARM9 processor, this is not
>> a negligible amount of time.
> 
> This is a concern I've had as well. The sysfs interface is useful in
> that it provides an intuitive and human-friendly way to expose data
> about devices. But as you note, there is considerable overhead in the
> amount of syscalls we have to make to interact with multiple attributes.
> 
> One solution that may work is providing a character device interface in
> addition to the sysfs interface. I believe that should reduce the
> syscall overhead since a user can pass in a data structure with a
> configuration defining what data/actions they want, and receive back
> all data in a single syscall.

Just toying with the idea here, but I've been thinking that it might
work well to be able to mmap a char device to access a ring buffer.
Then there should basically be no overhead at all from getting information
from the kernel to userspace.

> 
> I think concern over latency was one of the reasons the GPIO subsystem
> gained a character device interface as well. It's an addition to the
> Counter subsystem that is worth considering, but the possible downsides
> to such an interface should also be investigated.
>   
>> I noticed that several of the other counter drivers also register an IIO
>> device. So this got me thinking that perhaps the counter subsystem should
>> just be for configuring a counter device an then the IIO subsystem should
>> be used for triggers and ring buffers.
>>
>> For the general case a counter device could have two possible triggers.
>> One that triggers an interrupt after X counts and another that triggers
>> with a period of T nanoseconds (or microseconds). Both triggers would add
>> a count/timestamp pair to an IIO ring buffer.
>>
>> To fully reproduce our current methodology the first trigger would actually
>> need two configurable settings, the count X that triggers every X counts and
>> a watchdog time setting (using terminology from eQEP docs) that will also
>> trigger if and only if the count does not change before the time has elapsed.
>> Note, this is different from the other proposed time based trigger which
>> would cause a trigger interrupt at a fixed period regardless of whether
>> the count changed or not.
> 
> The counter drivers in the kernel right now are registering IIO devices
> in order to keep the preexisting (but now deprecated) IIO Counter
> interface working for these devices -- some users may be using this
> older interface so we don't want to remove it cold turkey. Regardless,
> there's nothing the prevents incorporating the IIO interface with your
> Counter drivers; in fact, in some circumstances it's better that you do
> just that.
> 
> The key idea to recognize is how the Counter subsystem differs from the
> IIO subsystem on a conceptual level: the IIO subsystem provides an
> interface for your device by describing it on a hardware level, whereas
> the Counter subsystem provides an interface for your device by
> describing it on a more abstract level.
> 
> What I mean is that every interface interaction in the Counter subsystem
> relates to the abstract concept of an ideal "counter device" (Counts,
> Synapses, Signals); if a device functionality or data does not relate
> directly to those ideal counter device components, then the Counter
> subsystem isn't that right interface for it.
> 
> For example, it makes sense to have an "enable" attribute or "present"
> attribute, because these functionalities/data are directly related to
> the Count, Synapse, and Signal components conceptually. However, in the
> Counter subsystem you will likely not see something like the IIO
> "in_voltageY_supply_raw" attribute -- not because that data is not
> useful to know about for the operation of the counter device hardware,
> but because it is outside the scope of the Counter subsystem paradigm
> (i.e. it does not directly related to Counts, Synapses, or Signals).
> As such, this would be a case where the counter driver should register
> both a Counter device and IIO device, one to handle the counter device
> on an abstract level while the other provides an interface for control
> of the more specific hardware details.

Makes sense. I that case, I don't see a need for an IIO device for the
eQEP.

> 
>> ---
>>
>> Thinking more generally though, I think what I would propose is adding a new
>> component to the existing list of Count, Signal and Synapse. The new component
>> could be called Event. Event would be more general than the trigger conditions
>> I have just discussed. In addition to those two, it could be any event
>> generated by the hardware, such as an error condition or a change in direction.
>>
>> Drivers could register an arbitrary number of events for each Count, so we
>> would have /sys/bus/counter/devices/counterX/eventY/*. There should be a few
>> standard attributes, like "name" and "enable". Configurable events would need
>> ext attributes to allow configuration.
>>
>> However, I see that there are already preset and error_noise "events" for
>> count objects, so maybe we don't do the eventY thing and keep it flatter (or
>> is the counter subsystem still considered in "staging" where breaking ABI
>> changes could be made?).
> 
> The components for handling events already exist in the Counter
> interface paradigm: Signals and Synapses. Although, the Counter
> subsystem is currently lacking the implementation (I still need to code
> in support for interrupts and such), the paradigm itself supports the
> concept of events and triggers.
> 
> Recall that the Counter subsystem represents hardware via the
> abstraction of an idealized "counter device". This is important to
> understand because it means that Signals are not necessarily limited to
> the physical wires of the hardware. To summarize the Counter interface
> paradigm:
> 
>      * A Signal is a stream of data to be evaluated.
>      * A Synapse is a trigger condition based on the evaluation of the
>        data streams (i.e. the Signals).
>      * A Count is the accumulation of the effects of Synapses (i.e. the
>        triggers).
> 
> As such, in order to represent an event, you would add in a Signal to
> represent the stream of device events, and a Synapse defining the
> specific event that will trigger the action. I'll give an example in
> order to demonstrate what I mean.
> 
> A simple clock can be conceptualize as a proper counter device: an
> oscillation is a Signal, a rising edge from that oscillation line can be
> the Synapse, and the current clock value is the Count.
> 
>                  Count                Synapse          Signal
>                  -----                -------          ------
>          +---------------------+
>          | Data: Clock ticks   |    Rising Edge     _____________
>          | Function: Increase  |  <-------------   / Oscillation \
>          |                     |                  _________________
>          +---------------------+
> 
> Now, in order to represent your timestamping clock we need two Signals:
> a simple clock and an event stream. The simple clock is the source of
> the current clock ticks we will store, while the event stream provides
> the rotation count register read notification that will trigger the
> timestamp.
> 
>                     Count                       Synapse      Signal
>                     -----                       -------      ------
>          +-------------------------------+
>          | Data: Timestamp               |       None        _______
>          | Function: Current clock ticks |  <------------   / Clock \
>          |                               |                 ___________
>          |                               |
>          |                               |    Read event     ________
>          |                               |  <------------   / Events \
>          |                               |                 ____________
>          +-------------------------------+
> 
> Note that in this case both Signals either do not exist in or are not
> exposed by the hardware (maybe the simple clock is exposed, but it's not
> necessary to be) -- they are meant to be abstract representations of the
> components of the timestamp clock as an idealized "counter device".
> 
> By organizing the timestamp clock in this way, we can control and
> configure the components using the standard Counter interface: common
> attributes such as "name", "preset", "enable", etc. can now be exposed
> to users like every other counter device component.

This way of looking at things makes very much sense. Thanks for the
detailed explanation.

> 
> In theory we can sleep on the timestamp count attribute read (or
> character device equivalent if we go down that route), and be woken when
> an event triggers updating the timestamp value. However, the current
> Counter subsystem implementation is lacking the code for this so it
> needs to be added to the core functionality first.
> 
>> When thinking about what events would actually do when enabled though, it
>> seems like we should be using IIO events and triggers (we have found reading
>> sysfs attributes to be insufficient performance-wise). It seems like unnecessary
>> work to reproduce all of this in the counter subsystem. Which makes me wonder if
>> it would be better to have counter devices just be a different device type (i.e.
>> different struct device_type for dev->type) in the IIO subsystem instead of
>> creating a completely new subsystem.
> 
> I plan on adding interrupt support for the 104-QUAD-8 counter driver
> since this device has some useful interrupts on configured threshold
> conditions and such, so having the ability to handle an event rather
> than constantly read and loop is something I want to have in the Counter
> subsystem.
> 
> It's possible that I can reuse some code from the IIO subsystem, as
> Jonathan pointed out, but overall I believe these should be separate
> subsystems. From the reasons described above, the IIO subsystem and
> Counter subsystem have different goals and thus different
> implementations. I don't think that's a bad thing, and we can share code
> in the few cases where the two may overlap.
> 
> Regarding whether to use IIO events and triggers within the TI eQEP
> counter driver, I think we should wait for a proper Counter subsystem
> implementation to be added first. My fear is that we'll have a similar
> situation as what happened with IIO_COUNT, where we'll have to keep a
> IIO interface present with a newer Counter interface. If adding in event
> support to the Counter subsystem will take too long, we can add this TI
> eQEP driver as-is now and later add in the timestamp support.

I don't think we need triggers anymore since I now better understand
what a synapse does.

---

In summary, this has been a very helpful discussion. Back the the patch
series I have submitted, I think it still makes sense to merge it now
as-is (barring any serious issues) and the additional functionality we
have been discussing can be added in the future as the framework for it
is developed.




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

* Re: [PATCH 1/4] dt-bindings: counter: new bindings for TI eQEP
  2019-07-27 19:48   ` Jonathan Cameron
@ 2019-08-02  7:25     ` William Breathitt Gray
  2019-08-02 13:34       ` Rob Herring
  0 siblings, 1 reply; 23+ messages in thread
From: William Breathitt Gray @ 2019-08-02  7:25 UTC (permalink / raw)
  To: Rob Herring
  Cc: David Lechner, linux-iio, linux-omap, devicetree, Mark Rutland,
	Jonathan Cameron, Benoît Cousson, Tony Lindgren,
	Thierry Reding, linux-kernel, linux-pwm

On Sat, Jul 27, 2019 at 08:48:36PM +0100, Jonathan Cameron wrote:
> On Mon, 22 Jul 2019 10:45:35 -0500
> David Lechner <david@lechnology.com> wrote:
> 
> > This documents device tree binding for the Texas Instruments Enhanced
> > Quadrature Encoder Pulse (eQEP) Module found in various TI SoCs.
> > 
> > Signed-off-by: David Lechner <david@lechnology.com>
> 
> Up to William given it is a counter binding, (unless Rob overrules)
> but new bindings are generally preferred as yaml.
> 
> Content looks fine to me.
> 
> Thanks,
> 
> Jonathan

Rob,

Would you prefer these bindings as yaml, or shall I accept them as they
are now?

William Breathitt Gray

> 
> > ---
> >  .../devicetree/bindings/counter/ti-eqep.txt    | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/counter/ti-eqep.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/counter/ti-eqep.txt b/Documentation/devicetree/bindings/counter/ti-eqep.txt
> > new file mode 100644
> > index 000000000000..fbcebc2c2cc2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/counter/ti-eqep.txt
> > @@ -0,0 +1,18 @@
> > +Texas Instruments Enhanced Quadrature Encoder Pulse (eQEP) Module
> > +
> > +Required properties:
> > +- compatible:		Must be "ti,am3352-eqep".
> > +- reg:			Physical base address and size of the registers map.
> > +- clocks:		Handle to the PWM's functional clock.
> > +- clock-names:		Must be "fck".
> > +- interrupts:		Handle to the eQEP event interrupt
> > +
> > +Example:
> > +
> > +	eqep0: eqep@180 {
> > +		compatible = "ti,am3352-eqep";
> > +		reg = <0x180 0x80>;
> > +		clocks = <&l4ls_gclk>;
> > +		clock-names = "fck";
> > +		interrupts = <79>;
> > +	};
> 

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

* Re: [PATCH 2/4] counter: new TI eQEP driver
  2019-07-22 15:45 ` [PATCH 2/4] counter: new TI eQEP driver David Lechner
  2019-07-30 12:35   ` Uwe Kleine-König
@ 2019-08-02  9:27   ` William Breathitt Gray
  2019-08-02 16:09     ` David Lechner
  2019-08-02 16:17     ` David Lechner
  1 sibling, 2 replies; 23+ messages in thread
From: William Breathitt Gray @ 2019-08-02  9:27 UTC (permalink / raw)
  To: David Lechner
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, Tony Lindgren, Thierry Reding, linux-kernel,
	linux-pwm

On Mon, Jul 22, 2019 at 10:45:36AM -0500, David Lechner wrote:
> This adds a new counter driver for the Texas Instruments Enhanced
> Quadrature Encoder Pulse (eQEP) module.
> 
> Only very basic functionality is currently implemented - only enough to
> be able to read the position. The actual device has many more features
> which can be added to the driver on an as-needed basis.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

Some changes suggested below, but most of the important stuff is there
and correct so good job.

William Breathitt Gray

> ---
>  MAINTAINERS               |   6 +
>  drivers/counter/Kconfig   |  12 ++
>  drivers/counter/Makefile  |   1 +
>  drivers/counter/ti-eqep.c | 381 ++++++++++++++++++++++++++++++++++++++
>  drivers/pwm/Kconfig       |   2 +-
>  5 files changed, 401 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/counter/ti-eqep.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 783569e3c4b4..f3b5e275732b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16014,6 +16014,12 @@ S:	Maintained
>  F:	drivers/media/platform/davinci/
>  F:	include/media/davinci/
>  
> +TI ENHANCED QUADRATURE ENCODER PULSE (eQEP) DRIVER
> +R:	David Lechner <david@lechnology.com>
> +L:	linux-iio@vger.kernel.org
> +F:	Documentation/devicetree/bindings/counter/ti-eqep.txt
> +F:	drivers/counter/ti-eqep.c
> +
>  TI ETHERNET SWITCH DRIVER (CPSW)
>  R:	Grygorii Strashko <grygorii.strashko@ti.com>
>  L:	linux-omap@vger.kernel.org
> diff --git a/drivers/counter/Kconfig b/drivers/counter/Kconfig
> index 2967d0a9ff91..7eeb310f0cda 100644
> --- a/drivers/counter/Kconfig
> +++ b/drivers/counter/Kconfig
> @@ -49,6 +49,18 @@ config STM32_LPTIMER_CNT
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called stm32-lptimer-cnt.
>  
> +config TI_EQEP
> +	tristate "TI eQEP counter driver"
> +	depends on (SOC_AM33XX || COMPILE_TEST)
> +	select PWM
> +	select REGMAP_MMIO
> +	help
> +	  Select this option to enable the Texas Instruments Enhanced Quadrature
> +	  Encoder Pulse (eQEP) counter driver.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ti-eqep.
> +
>  config FTM_QUADDEC
>  	tristate "Flex Timer Module Quadrature decoder driver"
>  	depends on HAS_IOMEM && OF
> diff --git a/drivers/counter/Makefile b/drivers/counter/Makefile
> index 40d35522937d..55142d1f4c43 100644
> --- a/drivers/counter/Makefile
> +++ b/drivers/counter/Makefile
> @@ -8,4 +8,5 @@ obj-$(CONFIG_COUNTER) += counter.o
>  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_TI_EQEP)		+= ti-eqep.o
>  obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> new file mode 100644
> index 000000000000..7aaa4abbc9c5
> --- /dev/null
> +++ b/drivers/counter/ti-eqep.c
> @@ -0,0 +1,381 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2019 David Lechner <david@lechnology.com>
> + *
> + * Counter driver for Texas Instruments Enhanced Quadrature Encoder Pulse (eQEP)
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/counter.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +/* 32-bit registers */
> +#define QPOSCNT		0x0
> +#define QPOSINIT	0x4
> +#define QPOSMAX		0x8
> +#define QPOSCMP		0xc
> +#define QPOSILAT	0x10
> +#define QPOSSLAT	0x14
> +#define QPOSLAT		0x18
> +#define QUTMR		0x1c
> +#define QUPRD		0x20
> +
> +/* 16-bit registers */
> +#define QWDTMR		0x0	/* 0x24 */
> +#define QWDPRD		0x2	/* 0x26 */
> +#define QDECCTL		0x4	/* 0x28 */
> +#define QEPCTL		0x6	/* 0x2a */
> +#define QCAPCTL		0x8	/* 0x2c */
> +#define QPOSCTL		0xa	/* 0x2e */
> +#define QEINT		0xc	/* 0x30 */
> +#define QFLG		0xe	/* 0x32 */
> +#define QCLR		0x10	/* 0x34 */
> +#define QFRC		0x12	/* 0x36 */
> +#define QEPSTS		0x14	/* 0x38 */
> +#define QCTMR		0x16	/* 0x3a */
> +#define QCPRD		0x18	/* 0x3c */
> +#define QCTMRLAT	0x1a	/* 0x3e */
> +#define QCPRDLAT	0x1c	/* 0x40 */
> +
> +#define QDECCTL_QSRC_SHIFT	14
> +#define QDECCTL_QSRC		GENMASK(15, 14)
> +#define QDECCTL_SOEN		BIT(13)
> +#define QDECCTL_SPSEL		BIT(12)
> +#define QDECCTL_XCR		BIT(11)
> +#define QDECCTL_SWAP		BIT(10)
> +#define QDECCTL_IGATE		BIT(9)
> +#define QDECCTL_QAP		BIT(8)
> +#define QDECCTL_QBP		BIT(7)
> +#define QDECCTL_QIP		BIT(6)
> +#define QDECCTL_QSP		BIT(5)
> +
> +#define QEPCTL_FREE_SOFT	GENMASK(15, 14)
> +#define QEPCTL_PCRM		GENMASK(13, 12)
> +#define QEPCTL_SEI		GENMASK(11, 10)
> +#define QEPCTL_IEI		GENMASK(9, 8)
> +#define QEPCTL_SWI		BIT(7)
> +#define QEPCTL_SEL		BIT(6)
> +#define QEPCTL_IEL		GENMASK(5, 4)
> +#define QEPCTL_PHEN		BIT(3)
> +#define QEPCTL_QCLM		BIT(2)
> +#define QEPCTL_UTE		BIT(1)
> +#define QEPCTL_WDE		BIT(0)
> +
> +/* EQEP Inputs */
> +enum {
> +	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
> +	TI_EQEP_SIGNAL_QEPB,	/* QEPB/XDIR */
> +	TI_EQEP_SIGNAL_QEPI,	/* Index */
> +	TI_EQEP_SIGNAL_QEPS,	/* Strobe */
> +};
> +
> +/* Position Counter Input Modes */
> +enum {
> +	TI_EQEP_COUNT_FUNC_QUAD_COUNT,
> +	TI_EQEP_COUNT_FUNC_DIR_COUNT,
> +	TI_EQEP_COUNT_FUNC_UP_COUNT,
> +	TI_EQEP_COUNT_FUNC_DOWN_COUNT,
> +};
> +
> +struct ti_eqep_cnt {
> +	struct counter_device counter;
> +	struct regmap *regmap32;
> +	struct regmap *regmap16;
> +};
> +
> +static int ti_eqep_count_read(struct counter_device *counter,
> +			      struct counter_count *count,
> +			      struct counter_count_read_value *val)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 cnt;
> +
> +	regmap_read(priv->regmap32, QPOSCNT, &cnt);
> +	counter_count_read_value_set(val, COUNTER_COUNT_POSITION, &cnt);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_count_write(struct counter_device *counter,
> +			       struct counter_count *count,
> +			       struct counter_count_write_value *val)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 cnt, max;
> +	int err;
> +
> +	err = counter_count_write_value_get(&cnt, COUNTER_COUNT_POSITION, val);
> +	if (err)
> +		return err;
> +
> +	regmap_read(priv->regmap32, QPOSMAX, &max);
> +	if (cnt > max)
> +		return -EINVAL;
> +
> +	return regmap_write(priv->regmap32, QPOSCNT, cnt);
> +}
> +
> +static int ti_eqep_function_get(struct counter_device *counter,
> +				struct counter_count *count, size_t *function)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qdecctl;
> +
> +	regmap_read(priv->regmap16, QDECCTL, &qdecctl);
> +	*function = (qdecctl & QDECCTL_QSRC) >> QDECCTL_QSRC_SHIFT;
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_function_set(struct counter_device *counter,
> +				struct counter_count *count, size_t function)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	return regmap_write_bits(priv->regmap16, QDECCTL, QDECCTL_QSRC,
> +				 function << QDECCTL_QSRC_SHIFT);
> +}
> +
> +static ssize_t ti_eqep_position_ceiling_read(struct counter_device *counter,
> +					     struct counter_count *count,
> +					     void *ext_priv, char *buf)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qposmax;
> +
> +	regmap_read(priv->regmap32, QPOSMAX, &qposmax);
> +
> +	return sprintf(buf, "%u\n", qposmax);
> +}
> +
> +static ssize_t ti_eqep_position_ceiling_write(struct counter_device *counter,
> +					      struct counter_count *count,
> +					      void *ext_priv, const char *buf,
> +					      size_t len)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	int err;
> +	u32 res;
> +
> +	err = kstrtouint(buf, 10, &res);

In Documentation/ABI/testing/sysfs-bus-counter, a base is not specified
for the ceiling attribute so the expectation is for the base of the
string to be automatically detected with the conventional semantics.

That means you should pass 0 here to kstrtouint instead of 10.

> +	if (err < 0)
> +		return err;
> +
> +	regmap_write(priv->regmap32, QPOSMAX, res);
> +
> +	return len;
> +}
> +
> +static ssize_t ti_eqep_position_floor_read(struct counter_device *counter,
> +					   struct counter_count *count,
> +					   void *ext_priv, char *buf)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qposinit;
> +
> +	regmap_read(priv->regmap32, QPOSINIT, &qposinit);
> +
> +	return sprintf(buf, "%u\n", qposinit);
> +}
> +
> +static ssize_t ti_eqep_position_floor_write(struct counter_device *counter,
> +					    struct counter_count *count,
> +					    void *ext_priv, const char *buf,
> +					    size_t len)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	int err;
> +	u32 res;
> +
> +	err = kstrtouint(buf, 10, &res);

For the same reason as the ceiling attribute, you should pass 0 here to
kstrtouint instead of 10.

> +	if (err < 0)
> +		return err;
> +
> +	regmap_write(priv->regmap32, QPOSINIT, res);
> +
> +	return len;
> +}
> +
> +static ssize_t ti_eqep_position_enable_read(struct counter_device *counter,
> +					    struct counter_count *count,
> +					    void *ext_priv, char *buf)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qepctl;
> +
> +	regmap_read(priv->regmap16, QEPCTL, &qepctl);
> +
> +	return sprintf(buf, "%u\n", !!(qepctl & QEPCTL_PHEN));
> +}
> +
> +static ssize_t ti_eqep_position_enable_write(struct counter_device *counter,
> +					     struct counter_count *count,
> +					     void *ext_priv, const char *buf,
> +					     size_t len)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	int err;
> +	bool res;
> +
> +	err = kstrtobool(buf, &res);
> +	if (err < 0)
> +		return err;
> +
> +	regmap_write_bits(priv->regmap16, QEPCTL, QEPCTL_PHEN, res ? -1 : 0);
> +
> +	return len;
> +}
> +
> +static const struct regmap_config ti_eqep_regmap32_config = {
> +	.name = "32-bit",
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = 0x24,
> +};
> +
> +static const struct regmap_config ti_eqep_regmap16_config = {
> +	.name = "16-bit",
> +	.reg_bits = 16,
> +	.val_bits = 16,
> +	.reg_stride = 2,
> +	.max_register = 0x1e,
> +};
> +
> +static const struct counter_ops ti_eqep_counter_ops = {
> +	.count_read	= ti_eqep_count_read,
> +	.count_write	= ti_eqep_count_write,
> +	.function_get	= ti_eqep_function_get,
> +	.function_set	= ti_eqep_function_set,
> +};

Are you able to provide a signal_read function, or are the Signals not
exposed to the user by this device? Sometimes quadrature encoder devices
provide an instanteous read of the signal lines to tell whether they are
high or low, so I figured I'd ask.

You should define an action_get function as well along with Synapses
corresponding to each Signal. This will allow users to know whether the
Synapse fires on a rising edge, falling edge, no edge, or both edges.

For example, consider the drivers/counter/104-quad-8.c file. Each count
register has three associated signal lines: Quadrature A, Quadrature B,
and Index.

Quadrature A and B are your typical quadrature encoder lines and
depending on the function mode selected (quadrature x4, pulse-direction,
etc.) could have a Synapse action mode of none, rising edge, falling
edge, or both edges; see the quad8_synapse_actions_list array.

In contrast, the Index signal line only has two Synapse action modes:
rising edge (in the case preset functionality is enabled) or none.

For the TI eQEP driver, there will be four Synapses corresponding to the
four Signals: QEPA, QEPB, QEPI, and QEPS. See if you are able to
implement the Synapses and action_get function by using the 104-quad-8.c
file as a reference. That file does have a lot of extra functionality
tossed in compared to yours, so if you have trouble groking it, just let
me know and I'll try to help.

> +
> +static const enum counter_count_function ti_eqep_position_functions[] = {
> +	[TI_EQEP_COUNT_FUNC_QUAD_COUNT]	= COUNTER_COUNT_FUNCTION_QUADRATURE_X4,
> +	[TI_EQEP_COUNT_FUNC_DIR_COUNT]	= COUNTER_COUNT_FUNCTION_PULSE_DIRECTION,
> +	[TI_EQEP_COUNT_FUNC_UP_COUNT]	= COUNTER_COUNT_FUNCTION_INCREASE,
> +	[TI_EQEP_COUNT_FUNC_DOWN_COUNT]	= COUNTER_COUNT_FUNCTION_DECREASE,
> +};
> +
> +static struct counter_signal ti_eqep_signals[] = {
> +	[TI_EQEP_SIGNAL_QEPA] = {
> +		.id = TI_EQEP_SIGNAL_QEPA,
> +		.name = "QEPA"
> +	},
> +	[TI_EQEP_SIGNAL_QEPB] = {
> +		.id = TI_EQEP_SIGNAL_QEPB,
> +		.name = "QEPB"
> +	},
> +	[TI_EQEP_SIGNAL_QEPI] = {
> +		.id = TI_EQEP_SIGNAL_QEPI,
> +		.name = "QEPI"
> +	},
> +	[TI_EQEP_SIGNAL_QEPS] = {
> +		.id = TI_EQEP_SIGNAL_QEPS,
> +		.name = "QEPS"
> +	},
> +};
> +
> +static struct counter_count_ext ti_eqep_position_ext[] = {
> +	{
> +		.name	= "ceiling",
> +		.read	= ti_eqep_position_ceiling_read,
> +		.write	= ti_eqep_position_ceiling_write,
> +	},
> +	{
> +		.name	= "floor",
> +		.read	= ti_eqep_position_floor_read,
> +		.write	= ti_eqep_position_floor_write,
> +	},
> +	{
> +		.name	= "enable",
> +		.read	= ti_eqep_position_enable_read,
> +		.write	= ti_eqep_position_enable_write,
> +	},
> +};
> +
> +static struct counter_count ti_eqep_counts[] = {
> +	{
> +		.id		= 0,
> +		.name		= "QPOSCNT",
> +		.functions_list	= ti_eqep_position_functions,
> +		.num_functions	= ARRAY_SIZE(ti_eqep_position_functions),
> +		.ext		= ti_eqep_position_ext,
> +		.num_ext	= ARRAY_SIZE(ti_eqep_position_ext),

When you have your Synapses defined, pass them here to your Count via
the synapses and num_synapses members.

> +	},
> +};
> +
> +static int ti_eqep_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ti_eqep_cnt *priv;
> +	struct resource *res;
> +	void __iomem *base;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	priv->regmap32 = devm_regmap_init_mmio(dev, base,
> +					       &ti_eqep_regmap32_config);
> +	if (IS_ERR(priv->regmap32))
> +		return PTR_ERR(priv->regmap32);
> +
> +	priv->regmap16 = devm_regmap_init_mmio(dev, base + 0x24,
> +					       &ti_eqep_regmap16_config);
> +	if (IS_ERR(priv->regmap16))
> +		return PTR_ERR(priv->regmap16);
> +
> +	priv->counter.name = dev_name(dev);
> +	priv->counter.parent = dev;
> +	priv->counter.ops = &ti_eqep_counter_ops;
> +	priv->counter.counts = ti_eqep_counts;
> +	priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts);
> +	priv->counter.signals = ti_eqep_signals;
> +	priv->counter.num_signals = ARRAY_SIZE(ti_eqep_signals);
> +	priv->counter.priv = priv;
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get(dev);
> +
> +	return devm_counter_register(dev, &priv->counter);
> +}
> +
> +static int ti_eqep_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	pm_runtime_put(dev),
> +	pm_runtime_disable(dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ti_eqep_of_match[] = {
> +	{ .compatible = "ti,am3352-eqep", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ti_eqep_of_match);
> +
> +static struct platform_driver ti_eqep_driver = {
> +	.probe = ti_eqep_probe,
> +	.remove = ti_eqep_remove,
> +	.driver = {
> +		.name = "ti-eqep-cnt",
> +		.of_match_table = ti_eqep_of_match,
> +	},
> +};
> +module_platform_driver(ti_eqep_driver);
> +
> +MODULE_AUTHOR("David Lechner <david@lechnology.com>");
> +MODULE_DESCRIPTION("TI eQEP counter driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a7e57516959e..ddcbb8573894 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -499,7 +499,7 @@ config  PWM_TIEHRPWM
>  
>  config  PWM_TIPWMSS
>  	bool
> -	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM)
> +	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM || TI_EQEP)
>  	help
>  	  PWM Subsystem driver support for AM33xx SOC.

I was surprised to see this pwm Kconfig change in this patch. Is
PWM_TIPWMSS required for TI_EQEP to work? If not required, then this
could be a separate patch; otherwise, put in a mention about why in the
commit message so that the purpose of this change is clearer.

>  
> -- 
> 2.17.1
> 

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

* Re: [PATCH 1/4] dt-bindings: counter: new bindings for TI eQEP
  2019-08-02  7:25     ` William Breathitt Gray
@ 2019-08-02 13:34       ` Rob Herring
  2019-08-02 13:58         ` William Breathitt Gray
  0 siblings, 1 reply; 23+ messages in thread
From: Rob Herring @ 2019-08-02 13:34 UTC (permalink / raw)
  To: William Breathitt Gray
  Cc: David Lechner, open list:IIO SUBSYSTEM AND DRIVERS, linux-omap,
	devicetree, Mark Rutland, Jonathan Cameron, Benoît Cousson,
	Tony Lindgren, Thierry Reding, linux-kernel, Linux PWM List

On Fri, Aug 2, 2019 at 1:25 AM William Breathitt Gray
<vilhelm.gray@gmail.com> wrote:
>
> On Sat, Jul 27, 2019 at 08:48:36PM +0100, Jonathan Cameron wrote:
> > On Mon, 22 Jul 2019 10:45:35 -0500
> > David Lechner <david@lechnology.com> wrote:
> >
> > > This documents device tree binding for the Texas Instruments Enhanced
> > > Quadrature Encoder Pulse (eQEP) Module found in various TI SoCs.
> > >
> > > Signed-off-by: David Lechner <david@lechnology.com>
> >
> > Up to William given it is a counter binding, (unless Rob overrules)
> > but new bindings are generally preferred as yaml.
> >
> > Content looks fine to me.
> >
> > Thanks,
> >
> > Jonathan
>
> Rob,
>
> Would you prefer these bindings as yaml, or shall I accept them as they
> are now?

Still up to you at this point, but I certainly prefer them to be DT schema.

Rob

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

* Re: [PATCH 1/4] dt-bindings: counter: new bindings for TI eQEP
  2019-08-02 13:34       ` Rob Herring
@ 2019-08-02 13:58         ` William Breathitt Gray
  0 siblings, 0 replies; 23+ messages in thread
From: William Breathitt Gray @ 2019-08-02 13:58 UTC (permalink / raw)
  To: David Lechner
  Cc: Rob Herring, open list:IIO SUBSYSTEM AND DRIVERS, linux-omap,
	devicetree, Mark Rutland, Jonathan Cameron, Benoît Cousson,
	Tony Lindgren, Thierry Reding, linux-kernel, Linux PWM List

On Fri, Aug 02, 2019 at 07:34:42AM -0600, Rob Herring wrote:
> On Fri, Aug 2, 2019 at 1:25 AM William Breathitt Gray
> <vilhelm.gray@gmail.com> wrote:
> >
> > On Sat, Jul 27, 2019 at 08:48:36PM +0100, Jonathan Cameron wrote:
> > > On Mon, 22 Jul 2019 10:45:35 -0500
> > > David Lechner <david@lechnology.com> wrote:
> > >
> > > > This documents device tree binding for the Texas Instruments Enhanced
> > > > Quadrature Encoder Pulse (eQEP) Module found in various TI SoCs.
> > > >
> > > > Signed-off-by: David Lechner <david@lechnology.com>
> > >
> > > Up to William given it is a counter binding, (unless Rob overrules)
> > > but new bindings are generally preferred as yaml.
> > >
> > > Content looks fine to me.
> > >
> > > Thanks,
> > >
> > > Jonathan
> >
> > Rob,
> >
> > Would you prefer these bindings as yaml, or shall I accept them as they
> > are now?
> 
> Still up to you at this point, but I certainly prefer them to be DT schema.
> 
> Rob

I think YAML would be a good idea, it's simple and expressive enough
while still being human-readable, and if new bindings in other
subsystems are switching to yaml then we may as well.

David, resubmit this as yaml in version 2 and I'll pick it up.

Thanks,

William Breathitt Gray

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

* Re: [PATCH 2/4] counter: new TI eQEP driver
  2019-08-02  9:27   ` William Breathitt Gray
@ 2019-08-02 16:09     ` David Lechner
  2019-08-02 16:17     ` David Lechner
  1 sibling, 0 replies; 23+ messages in thread
From: David Lechner @ 2019-08-02 16:09 UTC (permalink / raw)
  To: William Breathitt Gray
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, Tony Lindgren, Thierry Reding, linux-kernel,
	linux-pwm, Uwe Kleine-König

On 8/2/19 4:27 AM, William Breathitt Gray wrote:
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index a7e57516959e..ddcbb8573894 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -499,7 +499,7 @@ config  PWM_TIEHRPWM
>>   
>>   config  PWM_TIPWMSS
>>   	bool
>> -	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM)
>> +	default y if (ARCH_OMAP2PLUS) && (PWM_TIECAP || PWM_TIEHRPWM || TI_EQEP)
>>   	help
>>   	  PWM Subsystem driver support for AM33xx SOC.
> I was surprised to see this pwm Kconfig change in this patch. Is
> PWM_TIPWMSS required for TI_EQEP to work? If not required, then this
> could be a separate patch; otherwise, put in a mention about why in the
> commit message so that the purpose of this change is clearer.
> 

This enables the parent bus for power management. Since this is the second
comment about this, I wonder if it would make sense to move this out of the
PWM subsystem and into drivers/bus/ since it is no longer exclusive to PWM
devices.

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

* Re: [PATCH 2/4] counter: new TI eQEP driver
  2019-08-02  9:27   ` William Breathitt Gray
  2019-08-02 16:09     ` David Lechner
@ 2019-08-02 16:17     ` David Lechner
  2019-08-02 16:40       ` William Breathitt Gray
  1 sibling, 1 reply; 23+ messages in thread
From: David Lechner @ 2019-08-02 16:17 UTC (permalink / raw)
  To: William Breathitt Gray
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, Tony Lindgren, Thierry Reding, linux-kernel,
	linux-pwm

On 8/2/19 4:27 AM, William Breathitt Gray wrote:
>> +static const struct counter_ops ti_eqep_counter_ops = {
>> +	.count_read	= ti_eqep_count_read,
>> +	.count_write	= ti_eqep_count_write,
>> +	.function_get	= ti_eqep_function_get,
>> +	.function_set	= ti_eqep_function_set,
>> +};
> Are you able to provide a signal_read function, or are the Signals not
> exposed to the user by this device? Sometimes quadrature encoder devices
> provide an instanteous read of the signal lines to tell whether they are
> high or low, so I figured I'd ask.

No, it does not look like these signals can be read directly.

> 
> You should define an action_get function as well along with Synapses
> corresponding to each Signal. This will allow users to know whether the
> Synapse fires on a rising edge, falling edge, no edge, or both edges.
> 
> For example, consider the drivers/counter/104-quad-8.c file. Each count
> register has three associated signal lines: Quadrature A, Quadrature B,
> and Index.
> 
> Quadrature A and B are your typical quadrature encoder lines and
> depending on the function mode selected (quadrature x4, pulse-direction,
> etc.) could have a Synapse action mode of none, rising edge, falling
> edge, or both edges; see the quad8_synapse_actions_list array.
> 
> In contrast, the Index signal line only has two Synapse action modes:
> rising edge (in the case preset functionality is enabled) or none.

The encoders I have don't use the index or strobe signals, so I was
thinking maybe I should omit those two signals from the driver for the
time being since I don't have a way of testing.

> 
> For the TI eQEP driver, there will be four Synapses corresponding to the
> four Signals: QEPA, QEPB, QEPI, and QEPS. See if you are able to
> implement the Synapses and action_get function by using the 104-quad-8.c
> file as a reference. That file does have a lot of extra functionality
> tossed in compared to yours, so if you have trouble groking it, just let
> me know and I'll try to help.
> 


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

* Re: [PATCH 2/4] counter: new TI eQEP driver
  2019-08-02 16:17     ` David Lechner
@ 2019-08-02 16:40       ` William Breathitt Gray
  0 siblings, 0 replies; 23+ messages in thread
From: William Breathitt Gray @ 2019-08-02 16:40 UTC (permalink / raw)
  To: David Lechner
  Cc: linux-iio, linux-omap, devicetree, Rob Herring, Mark Rutland,
	Benoît Cousson, Tony Lindgren, Thierry Reding, linux-kernel,
	linux-pwm

On Fri, Aug 02, 2019 at 11:17:11AM -0500, David Lechner wrote:
> On 8/2/19 4:27 AM, William Breathitt Gray wrote:
> >> +static const struct counter_ops ti_eqep_counter_ops = {
> >> +	.count_read	= ti_eqep_count_read,
> >> +	.count_write	= ti_eqep_count_write,
> >> +	.function_get	= ti_eqep_function_get,
> >> +	.function_set	= ti_eqep_function_set,
> >> +};
> > Are you able to provide a signal_read function, or are the Signals not
> > exposed to the user by this device? Sometimes quadrature encoder devices
> > provide an instanteous read of the signal lines to tell whether they are
> > high or low, so I figured I'd ask.
> 
> No, it does not look like these signals can be read directly.

All right, in that case you can ignore signal_read.

> > 
> > You should define an action_get function as well along with Synapses
> > corresponding to each Signal. This will allow users to know whether the
> > Synapse fires on a rising edge, falling edge, no edge, or both edges.
> > 
> > For example, consider the drivers/counter/104-quad-8.c file. Each count
> > register has three associated signal lines: Quadrature A, Quadrature B,
> > and Index.
> > 
> > Quadrature A and B are your typical quadrature encoder lines and
> > depending on the function mode selected (quadrature x4, pulse-direction,
> > etc.) could have a Synapse action mode of none, rising edge, falling
> > edge, or both edges; see the quad8_synapse_actions_list array.
> > 
> > In contrast, the Index signal line only has two Synapse action modes:
> > rising edge (in the case preset functionality is enabled) or none.
> 
> The encoders I have don't use the index or strobe signals, so I was
> thinking maybe I should omit those two signals from the driver for the
> time being since I don't have a way of testing.

That should be fine for now. We can add them in a later patch down the
road and keep this introduction patch simple.

William Breathitt Gray

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

end of thread, other threads:[~2019-08-02 16:41 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-22 15:45 [PATCH 0/4] new driver for TI eQEP David Lechner
2019-07-22 15:45 ` [PATCH 1/4] dt-bindings: counter: new bindings " David Lechner
2019-07-27 19:48   ` Jonathan Cameron
2019-08-02  7:25     ` William Breathitt Gray
2019-08-02 13:34       ` Rob Herring
2019-08-02 13:58         ` William Breathitt Gray
2019-07-22 15:45 ` [PATCH 2/4] counter: new TI eQEP driver David Lechner
2019-07-30 12:35   ` Uwe Kleine-König
2019-07-30 15:28     ` David Lechner
2019-08-02  9:27   ` William Breathitt Gray
2019-08-02 16:09     ` David Lechner
2019-08-02 16:17     ` David Lechner
2019-08-02 16:40       ` William Breathitt Gray
2019-07-22 15:45 ` [PATCH 3/4] ARM: dts: am33xx: Add nodes for eQEP David Lechner
2019-07-23  8:42   ` Tony Lindgren
2019-07-23 14:45     ` David Lechner
2019-07-23 14:51       ` Tony Lindgren
2019-07-22 15:45 ` [PATCH 4/4] ARM: dts: am335x-boneblue: Enable eQEP David Lechner
2019-07-25 12:40 ` [PATCH 0/4] new driver for TI eQEP William Breathitt Gray
2019-07-25 22:52   ` David Lechner
2019-07-27 19:45     ` Jonathan Cameron
2019-07-30  4:45     ` William Breathitt Gray
2019-08-01 17:37       ` David Lechner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).