[PATCH 1/8] include/fsl: add common FlexTimer #defines in a separate header.

[PATCH 1/8] include/fsl: add common FlexTimer #defines in a separate header.

[Patrick Havelange]

Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
Reviewed-by: Esben Haabendal <esben@haabendal.dk>
---
 include/linux/fsl/ftm.h | 88 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100644 include/linux/fsl/ftm.h

diff --git a/include/linux/fsl/ftm.h b/include/linux/fsl/ftm.h
new file mode 100644
index 000000000000..d59011acf66c
--- /dev/null
+++ b/include/linux/fsl/ftm.h
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0
+#ifndef __FSL_FTM_H__
+#define __FSL_FTM_H__
+
+#define FTM_SC       0x0 /* Status And Control */
+#define FTM_CNT      0x4 /* Counter */
+#define FTM_MOD      0x8 /* Modulo */
+
+#define FTM_CNTIN    0x4C /* Counter Initial Value */
+#define FTM_STATUS   0x50 /* Capture And Compare Status */
+#define FTM_MODE     0x54 /* Features Mode Selection */
+#define FTM_SYNC     0x58 /* Synchronization */
+#define FTM_OUTINIT  0x5C /* Initial State For Channels Output */
+#define FTM_OUTMASK  0x60 /* Output Mask */
+#define FTM_COMBINE  0x64 /* Function For Linked Channels */
+#define FTM_DEADTIME 0x68 /* Deadtime Insertion Control */
+#define FTM_EXTTRIG  0x6C /* FTM External Trigger */
+#define FTM_POL      0x70 /* Channels Polarity */
+#define FTM_FMS      0x74 /* Fault Mode Status */
+#define FTM_FILTER   0x78 /* Input Capture Filter Control */
+#define FTM_FLTCTRL  0x7C /* Fault Control */
+#define FTM_QDCTRL   0x80 /* Quadrature Decoder Control And Status */
+#define FTM_CONF     0x84 /* Configuration */
+#define FTM_FLTPOL   0x88 /* FTM Fault Input Polarity */
+#define FTM_SYNCONF  0x8C /* Synchronization Configuration */
+#define FTM_INVCTRL  0x90 /* FTM Inverting Control */
+#define FTM_SWOCTRL  0x94 /* FTM Software Output Control */
+#define FTM_PWMLOAD  0x98 /* FTM PWM Load */
+
+#define FTM_SC_CLK_MASK_SHIFT	3
+#define FTM_SC_CLK_MASK		(3 << FTM_SC_CLK_MASK_SHIFT)
+#define FTM_SC_TOF		0x80
+#define FTM_SC_TOIE		0x40
+#define FTM_SC_CPWMS		0x20
+#define FTM_SC_CLKS		0x18
+#define FTM_SC_PS_1		0x0
+#define FTM_SC_PS_2		0x1
+#define FTM_SC_PS_4		0x2
+#define FTM_SC_PS_8		0x3
+#define FTM_SC_PS_16		0x4
+#define FTM_SC_PS_32		0x5
+#define FTM_SC_PS_64		0x6
+#define FTM_SC_PS_128		0x7
+#define FTM_SC_PS_MASK		0x7
+
+#define FTM_MODE_FAULTIE	0x80
+#define FTM_MODE_FAULTM		0x60
+#define FTM_MODE_CAPTEST	0x10
+#define FTM_MODE_PWMSYNC	0x8
+#define FTM_MODE_WPDIS		0x4
+#define FTM_MODE_INIT		0x2
+#define FTM_MODE_FTMEN		0x1
+
+/* NXP Errata: The PHAFLTREN and PHBFLTREN bits are tide to zero internally
+ * and these bits cannot be set. Flextimer cannot use Filter in
+ * Quadrature Decoder Mode.
+ * https://community.nxp.com/thread/467648#comment-1010319
+ */
+#define FTM_QDCTRL_PHAFLTREN	0x80
+#define FTM_QDCTRL_PHBFLTREN	0x40
+#define FTM_QDCTRL_PHAPOL	0x20
+#define FTM_QDCTRL_PHBPOL	0x10
+#define FTM_QDCTRL_QUADMODE	0x8
+#define FTM_QDCTRL_QUADDIR	0x4
+#define FTM_QDCTRL_TOFDIR	0x2
+#define FTM_QDCTRL_QUADEN	0x1
+
+#define FTM_FMS_FAULTF		0x80
+#define FTM_FMS_WPEN		0x40
+#define FTM_FMS_FAULTIN		0x10
+#define FTM_FMS_FAULTF3		0x8
+#define FTM_FMS_FAULTF2		0x4
+#define FTM_FMS_FAULTF1		0x2
+#define FTM_FMS_FAULTF0		0x1
+
+#define FTM_CSC_BASE		0xC
+#define FTM_CSC_MSB		0x20
+#define FTM_CSC_MSA		0x10
+#define FTM_CSC_ELSB		0x8
+#define FTM_CSC_ELSA		0x4
+#define FTM_CSC(_channel)	(FTM_CSC_BASE + ((_channel) * 8))
+
+#define FTM_CV_BASE		0x10
+#define FTM_CV(_channel)	(FTM_CV_BASE + ((_channel) * 8))
+
+#define FTM_PS_MAX		7
+
+#endif
-- 
2.17.1

[PATCH 2/8] drivers/pwm: pwm-fsl-ftm: use common header for FlexTimer #defines

[Patrick Havelange]

This also fixes the wrong value for the previously defined
FTM_MODE_INIT macro (it was not used).

Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
Reviewed-by: Esben Haabendal <esben@haabendal.dk>
---
 drivers/pwm/pwm-fsl-ftm.c | 44 +--------------------------------------
 1 file changed, 1 insertion(+), 43 deletions(-)

diff --git a/drivers/pwm/pwm-fsl-ftm.c b/drivers/pwm/pwm-fsl-ftm.c
index 883378d055c6..f21ea1b97116 100644
--- a/drivers/pwm/pwm-fsl-ftm.c
+++ b/drivers/pwm/pwm-fsl-ftm.c
@@ -22,51 +22,9 @@
 #include <linux/pwm.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/fsl/ftm.h>
 
-#define FTM_SC		0x00
-#define FTM_SC_CLK_MASK_SHIFT	3
-#define FTM_SC_CLK_MASK	(3 << FTM_SC_CLK_MASK_SHIFT)
 #define FTM_SC_CLK(c)	(((c) + 1) << FTM_SC_CLK_MASK_SHIFT)
-#define FTM_SC_PS_MASK	0x7
-
-#define FTM_CNT		0x04
-#define FTM_MOD		0x08
-
-#define FTM_CSC_BASE	0x0C
-#define FTM_CSC_MSB	BIT(5)
-#define FTM_CSC_MSA	BIT(4)
-#define FTM_CSC_ELSB	BIT(3)
-#define FTM_CSC_ELSA	BIT(2)
-#define FTM_CSC(_channel)	(FTM_CSC_BASE + ((_channel) * 8))
-
-#define FTM_CV_BASE	0x10
-#define FTM_CV(_channel)	(FTM_CV_BASE + ((_channel) * 8))
-
-#define FTM_CNTIN	0x4C
-#define FTM_STATUS	0x50
-
-#define FTM_MODE	0x54
-#define FTM_MODE_FTMEN	BIT(0)
-#define FTM_MODE_INIT	BIT(2)
-#define FTM_MODE_PWMSYNC	BIT(3)
-
-#define FTM_SYNC	0x58
-#define FTM_OUTINIT	0x5C
-#define FTM_OUTMASK	0x60
-#define FTM_COMBINE	0x64
-#define FTM_DEADTIME	0x68
-#define FTM_EXTTRIG	0x6C
-#define FTM_POL		0x70
-#define FTM_FMS		0x74
-#define FTM_FILTER	0x78
-#define FTM_FLTCTRL	0x7C
-#define FTM_QDCTRL	0x80
-#define FTM_CONF	0x84
-#define FTM_FLTPOL	0x88
-#define FTM_SYNCONF	0x8C
-#define FTM_INVCTRL	0x90
-#define FTM_SWOCTRL	0x94
-#define FTM_PWMLOAD	0x98
 
 enum fsl_pwm_clk {
 	FSL_PWM_CLK_SYS,
-- 
2.17.1

[PATCH 3/8] drivers/clocksource: timer-fsl-ftm: use common header for FlexTimer #defines

[Patrick Havelange]

Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
Reviewed-by: Esben Haabendal <esben@haabendal.dk>
---
 drivers/clocksource/timer-fsl-ftm.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/clocksource/timer-fsl-ftm.c b/drivers/clocksource/timer-fsl-ftm.c
index 846d18daf893..e1c34b2f53a5 100644
--- a/drivers/clocksource/timer-fsl-ftm.c
+++ b/drivers/clocksource/timer-fsl-ftm.c
@@ -19,20 +19,9 @@
 #include <linux/of_irq.h>
 #include <linux/sched_clock.h>
 #include <linux/slab.h>
+#include <linux/fsl/ftm.h>
 
-#define FTM_SC		0x00
-#define FTM_SC_CLK_SHIFT	3
-#define FTM_SC_CLK_MASK	(0x3 << FTM_SC_CLK_SHIFT)
-#define FTM_SC_CLK(c)	((c) << FTM_SC_CLK_SHIFT)
-#define FTM_SC_PS_MASK	0x7
-#define FTM_SC_TOIE	BIT(6)
-#define FTM_SC_TOF	BIT(7)
-
-#define FTM_CNT		0x04
-#define FTM_MOD		0x08
-#define FTM_CNTIN	0x4C
-
-#define FTM_PS_MAX	7
+#define FTM_SC_CLK(c)	((c) << FTM_SC_CLK_MASK_SHIFT)
 
 struct ftm_clock_device {
 	void __iomem *clksrc_base;
-- 
2.17.1

[PATCH 4/8] dt-bindings: iio/counter: ftm-quaddec

[Patrick Havelange]

FlexTimer quadrature decoder driver.

Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
Reviewed-by: Esben Haabendal <esben@haabendal.dk>
---
 .../bindings/iio/counter/ftm-quaddec.txt       | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/counter/ftm-quaddec.txt

diff --git a/Documentation/devicetree/bindings/iio/counter/ftm-quaddec.txt b/Documentation/devicetree/bindings/iio/counter/ftm-quaddec.txt
new file mode 100644
index 000000000000..4d18cd722074
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/counter/ftm-quaddec.txt
@@ -0,0 +1,18 @@
+FlexTimer Quadrature decoder counter
+
+This driver exposes a simple counter for the quadrature decoder mode.
+
+Required properties:
+- compatible:		Must be "fsl,ftm-quaddec".
+- reg:			Must be set to the memory region of the flextimer.
+
+Optional property:
+- big-endian:		Access the device registers in big-endian mode.
+
+Example:
+		counter0: counter@29d0000 {
+			compatible = "fsl,ftm-quaddec";
+			reg = <0x0 0x29d0000 0x0 0x10000>;
+			big-endian;
+			status = "disabled";
+		};
-- 
2.17.1

[PATCH 5/8] iio/counter: add FlexTimer Module Quadrature decoder counter driver

[Patrick Havelange]

This driver exposes the counter for the quadrature decoder of the
FlexTimer Module, present in the LS1021A soc.

Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
Reviewed-by: Esben Haabendal <esben@haabendal.dk>
---
 drivers/iio/counter/Kconfig       |  10 +
 drivers/iio/counter/Makefile      |   1 +
 drivers/iio/counter/ftm-quaddec.c | 294 ++++++++++++++++++++++++++++++
 3 files changed, 305 insertions(+)
 create mode 100644 drivers/iio/counter/ftm-quaddec.c

diff --git a/drivers/iio/counter/Kconfig b/drivers/iio/counter/Kconfig
index bf1e559ad7cd..4641cb2e752a 100644
--- a/drivers/iio/counter/Kconfig
+++ b/drivers/iio/counter/Kconfig
@@ -31,4 +31,14 @@ config STM32_LPTIMER_CNT
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called stm32-lptimer-cnt.
+
+config FTM_QUADDEC
+	tristate "Flex Timer Module Quadrature decoder driver"
+	help
+	  Select this option to enable the Flex Timer Quadrature decoder
+	  driver.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ftm-quaddec.
+
 endmenu
diff --git a/drivers/iio/counter/Makefile b/drivers/iio/counter/Makefile
index 1b9a896eb488..757c1f4196af 100644
--- a/drivers/iio/counter/Makefile
+++ b/drivers/iio/counter/Makefile
@@ -6,3 +6,4 @@
 
 obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
 obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
+obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
diff --git a/drivers/iio/counter/ftm-quaddec.c b/drivers/iio/counter/ftm-quaddec.c
new file mode 100644
index 000000000000..ca7e55a9ab3f
--- /dev/null
+++ b/drivers/iio/counter/ftm-quaddec.c
@@ -0,0 +1,294 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Flex Timer Module Quadrature decoder
+ *
+ * This module implements a driver for decoding the FTM quadrature
+ * of ex. a LS1021A
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/workqueue.h>
+#include <linux/swait.h>
+#include <linux/sched.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/mutex.h>
+#include <linux/fsl/ftm.h>
+
+struct ftm_quaddec {
+	struct platform_device *pdev;
+	void __iomem *ftm_base;
+	bool big_endian;
+	struct mutex ftm_quaddec_mutex;
+};
+
+#define HASFLAGS(flag, bits) ((flag & bits) ? 1 : 0)
+
+#define DEFAULT_POLL_INTERVAL    100 /* in msec */
+
+static void ftm_read(struct ftm_quaddec *ftm, uint32_t offset, uint32_t *data)
+{
+	if (ftm->big_endian)
+		*data = ioread32be(ftm->ftm_base + offset);
+	else
+		*data = ioread32(ftm->ftm_base + offset);
+}
+
+static void ftm_write(struct ftm_quaddec *ftm, uint32_t offset, uint32_t data)
+{
+	if (ftm->big_endian)
+		iowrite32be(data, ftm->ftm_base + offset);
+	else
+		iowrite32(data, ftm->ftm_base + offset);
+}
+
+/* take mutex
+ * call ftm_clear_write_protection
+ * update settings
+ * call ftm_set_write_protection
+ * release mutex
+ */
+static void ftm_clear_write_protection(struct ftm_quaddec *ftm)
+{
+	uint32_t flag;
+
+	/* First see if it is enabled */
+	ftm_read(ftm, FTM_FMS, &flag);
+
+	if (flag & FTM_FMS_WPEN) {
+		ftm_read(ftm, FTM_MODE, &flag);
+		ftm_write(ftm, FTM_MODE, flag | FTM_MODE_WPDIS);
+	}
+}
+
+static void ftm_set_write_protection(struct ftm_quaddec *ftm)
+{
+	ftm_write(ftm, FTM_FMS, FTM_FMS_WPEN);
+}
+
+static void ftm_reset_counter(struct ftm_quaddec *ftm)
+{
+	/* Reset hardware counter to CNTIN */
+	ftm_write(ftm, FTM_CNT, 0x0);
+}
+
+static void ftm_quaddec_init(struct ftm_quaddec *ftm)
+{
+	ftm_clear_write_protection(ftm);
+
+	/* Do not write in the region from the CNTIN register through the
+	 * PWMLOAD register when FTMEN = 0.
+	 */
+	ftm_write(ftm, FTM_MODE, FTM_MODE_FTMEN); /* enable FTM */
+	ftm_write(ftm, FTM_CNTIN, 0x0000);         /* zero init value */
+	ftm_write(ftm, FTM_MOD, 0xffff);        /* max overflow value */
+	ftm_write(ftm, FTM_CNT, 0x0);           /* reset counter value */
+	ftm_write(ftm, FTM_SC, FTM_SC_PS_1);    /* prescale with x1 */
+	/* Select quad mode */
+	ftm_write(ftm, FTM_QDCTRL, FTM_QDCTRL_QUADEN);
+
+	/* Unused features and reset to default section */
+	ftm_write(ftm, FTM_POL, 0x0);     /* polarity is active high */
+	ftm_write(ftm, FTM_FLTCTRL, 0x0); /* all faults disabled */
+	ftm_write(ftm, FTM_SYNCONF, 0x0); /* disable all sync */
+	ftm_write(ftm, FTM_SYNC, 0xffff);
+
+	/* Lock the FTM */
+	ftm_set_write_protection(ftm);
+}
+
+static int ftm_quaddec_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct ftm_quaddec *ftm = iio_priv(indio_dev);
+	uint32_t counter;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ftm_read(ftm, FTM_CNT, &counter);
+		*val = counter;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
+				uintptr_t private,
+				struct iio_chan_spec const *chan,
+				const char *buf, size_t len)
+{
+	struct ftm_quaddec *ftm = iio_priv(indio_dev);
+
+	/* Only "counter reset" is supported for now */
+	if (!sysfs_streq(buf, "0")) {
+		dev_warn(&ftm->pdev->dev, "Reset only accepts '0'\n");
+		return -EINVAL;
+	}
+
+	ftm_reset_counter(ftm);
+
+	return len;
+}
+
+static int ftm_quaddec_get_prescaler(struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan)
+{
+	struct ftm_quaddec *ftm = iio_priv(indio_dev);
+	uint32_t scflags;
+
+	ftm_read(ftm, FTM_SC, &scflags);
+
+	return scflags & FTM_SC_PS_MASK;
+}
+
+static int ftm_quaddec_set_prescaler(struct iio_dev *indio_dev,
+					const struct iio_chan_spec *chan,
+					unsigned int type)
+{
+	struct ftm_quaddec *ftm = iio_priv(indio_dev);
+
+	uint32_t scflags;
+
+	mutex_lock(&ftm->ftm_quaddec_mutex);
+
+	ftm_read(ftm, FTM_SC, &scflags);
+
+	scflags &= ~FTM_SC_PS_MASK;
+	type &= FTM_SC_PS_MASK; /*just to be 100% sure*/
+
+	scflags |= type;
+
+	/* Write */
+	ftm_clear_write_protection(ftm);
+	ftm_write(ftm, FTM_SC, scflags);
+	ftm_set_write_protection(ftm);
+
+	/* Also resets the counter as it is undefined anyway now */
+	ftm_reset_counter(ftm);
+
+	mutex_unlock(&ftm->ftm_quaddec_mutex);
+	return 0;
+}
+
+static const char * const ftm_quaddec_prescaler[] = {
+	"1", "2", "4", "8", "16", "32", "64", "128"
+};
+
+static const struct iio_enum ftm_quaddec_prescaler_en = {
+	.items = ftm_quaddec_prescaler,
+	.num_items = ARRAY_SIZE(ftm_quaddec_prescaler),
+	.get = ftm_quaddec_get_prescaler,
+	.set = ftm_quaddec_set_prescaler,
+};
+
+static const struct iio_chan_spec_ext_info ftm_quaddec_ext_info[] = {
+	{
+		.name = "reset",
+		.shared = IIO_SEPARATE,
+		.write = ftm_write_reset,
+	},
+	IIO_ENUM("prescaler", IIO_SEPARATE, &ftm_quaddec_prescaler_en),
+	IIO_ENUM_AVAILABLE("prescaler", &ftm_quaddec_prescaler_en),
+	{}
+};
+
+static const struct iio_chan_spec ftm_quaddec_channels = {
+	.type = IIO_COUNT,
+	.channel = 0,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	.ext_info = ftm_quaddec_ext_info,
+	.indexed = 1,
+};
+
+static const struct iio_info ftm_quaddec_iio_info = {
+	.read_raw = ftm_quaddec_read_raw,
+};
+
+static int ftm_quaddec_probe(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev;
+	struct ftm_quaddec *ftm;
+	int ret;
+
+	struct device_node *node = pdev->dev.of_node;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*ftm));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	ftm = iio_priv(indio_dev);
+
+	platform_set_drvdata(pdev, ftm);
+
+	ftm->pdev = pdev;
+	ftm->big_endian = of_property_read_bool(node, "big-endian");
+	ftm->ftm_base = of_iomap(node, 0);
+	if (!ftm->ftm_base)
+		return -EINVAL;
+
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->info = &ftm_quaddec_iio_info;
+	indio_dev->num_channels = 1;
+	indio_dev->channels = &ftm_quaddec_channels;
+
+	ftm_quaddec_init(ftm);
+
+	mutex_init(&ftm->ftm_quaddec_mutex);
+
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	if (ret) {
+		mutex_destroy(&ftm->ftm_quaddec_mutex);
+		iounmap(ftm->ftm_base);
+	}
+	return ret;
+}
+
+static int ftm_quaddec_remove(struct platform_device *pdev)
+{
+	struct ftm_quaddec *ftm;
+	struct iio_dev *indio_dev;
+
+	ftm = (struct ftm_quaddec *)platform_get_drvdata(pdev);
+	indio_dev = iio_priv_to_dev(ftm);
+	/* This is needed to remove sysfs entries */
+	devm_iio_device_unregister(&pdev->dev, indio_dev);
+
+	ftm_write(ftm, FTM_MODE, 0);
+
+	iounmap(ftm->ftm_base);
+	mutex_destroy(&ftm->ftm_quaddec_mutex);
+
+	return 0;
+}
+
+static const struct of_device_id ftm_quaddec_match[] = {
+	{ .compatible = "fsl,ftm-quaddec" },
+	{},
+};
+
+static struct platform_driver ftm_quaddec_driver = {
+	.driver = {
+		.name = "ftm-quaddec",
+		.owner = THIS_MODULE,
+		.of_match_table = ftm_quaddec_match,
+	},
+	.probe = ftm_quaddec_probe,
+	.remove = ftm_quaddec_remove,
+};
+
+module_platform_driver(ftm_quaddec_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Kjeld Flarup <kfa@deif.com");
+MODULE_AUTHOR("Patrick Havelange <patrick.havelange@essensium.com");
-- 
2.17.1

[PATCH 6/8] LS1021A: dtsi: add ftm quad decoder entries

[Patrick Havelange]

Add the 4 Quadrature counters for this board.

Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
Reviewed-by: Esben Haabendal <esben@haabendal.dk>
---
 arch/arm/boot/dts/ls1021a.dtsi | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index ed0941292172..0168fb62590a 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -433,6 +433,34 @@
 			status = "disabled";
 		};
 
+		counter0: counter@29d0000 {
+			compatible = "fsl,ftm-quaddec";
+			reg = <0x0 0x29d0000 0x0 0x10000>;
+			big-endian;
+			status = "disabled";
+		};
+
+		counter1: counter@29e0000 {
+			compatible = "fsl,ftm-quaddec";
+			reg = <0x0 0x29e0000 0x0 0x10000>;
+			big-endian;
+			status = "disabled";
+		};
+
+		counter2: counter@29f0000 {
+			compatible = "fsl,ftm-quaddec";
+			reg = <0x0 0x29f0000 0x0 0x10000>;
+			big-endian;
+			status = "disabled";
+		};
+
+		counter3: counter@2a00000 {
+			compatible = "fsl,ftm-quaddec";
+			reg = <0x0 0x2a00000 0x0 0x10000>;
+			big-endian;
+			status = "disabled";
+		};
+
 		gpio0: gpio@2300000 {
 			compatible = "fsl,ls1021a-gpio", "fsl,qoriq-gpio";
 			reg = <0x0 0x2300000 0x0 0x10000>;
-- 
2.17.1

[PATCH 7/8] dt-bindings: iio/counter: ftm-quaddec: add poll-interval parameter

[Patrick Havelange]

New optional parameter supported by updated driver.

Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
Reviewed-by: Esben Haabendal <esben@haabendal.dk>
---
 .../devicetree/bindings/iio/counter/ftm-quaddec.txt       | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/counter/ftm-quaddec.txt b/Documentation/devicetree/bindings/iio/counter/ftm-quaddec.txt
index 4d18cd722074..60554e6c4367 100644
--- a/Documentation/devicetree/bindings/iio/counter/ftm-quaddec.txt
+++ b/Documentation/devicetree/bindings/iio/counter/ftm-quaddec.txt
@@ -6,8 +6,14 @@ Required properties:
 - compatible:		Must be "fsl,ftm-quaddec".
 - reg:			Must be set to the memory region of the flextimer.
 
-Optional property:
+Optional properties:
 - big-endian:		Access the device registers in big-endian mode.
+- poll-interval		Poll interval time in milliseconds for detecting
+			the under/overflow of the counter. Default value
+			is 100.
+			A value of 0 disables polling. This value can also
+			be set at runtime, but not to less than this initial
+			value (except 0 for disabling).
 
 Example:
 		counter0: counter@29d0000 {
-- 
2.17.1

[PATCH 8/8] iio/counter/ftm-quaddec: add handling of under/overflow of the counter.

[Patrick Havelange]

This is implemented by polling the counter value. A new parameter
"poll-interval" can be set in the device tree, or can be changed
at runtime. The reason for the polling is to avoid interrupts flooding.
If the quadrature input is going up and down around the overflow value
(or around 0), the interrupt will be triggering all the time. Thus,
polling is an easy way to handle overflow in a consistent way.
Polling can still be disabled by setting poll-interval to 0.

Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
Reviewed-by: Esben Haabendal <esben@haabendal.dk>
---
 drivers/iio/counter/ftm-quaddec.c | 199 +++++++++++++++++++++++++++++-
 1 file changed, 193 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/counter/ftm-quaddec.c b/drivers/iio/counter/ftm-quaddec.c
index ca7e55a9ab3f..3a0395c3ef33 100644
--- a/drivers/iio/counter/ftm-quaddec.c
+++ b/drivers/iio/counter/ftm-quaddec.c
@@ -25,11 +25,33 @@
 
 struct ftm_quaddec {
 	struct platform_device *pdev;
+	struct delayed_work delayedcounterwork;
 	void __iomem *ftm_base;
 	bool big_endian;
+
+	/* Offset added to the counter to adjust for overflows of the
+	 * 16 bit HW counter. Only the 16 MSB are set.
+	 */
+	uint32_t counteroffset;
+
+	/* Store the counter on each read, this is used to detect
+	 * if the counter readout if we over or underflow
+	 */
+	uint8_t lastregion;
+
+	/* Poll-interval, in ms before delayed work must poll counter */
+	uint16_t poll_interval;
+
 	struct mutex ftm_quaddec_mutex;
 };
 
+struct counter_result {
+	/* 16 MSB are from the counteroffset
+	 * 16 LSB are from the hardware counter
+	 */
+	uint32_t value;
+};
+
 #define HASFLAGS(flag, bits) ((flag & bits) ? 1 : 0)
 
 #define DEFAULT_POLL_INTERVAL    100 /* in msec */
@@ -74,8 +96,75 @@ static void ftm_set_write_protection(struct ftm_quaddec *ftm)
 	ftm_write(ftm, FTM_FMS, FTM_FMS_WPEN);
 }
 
+/* must be called with mutex locked */
+static void ftm_work_reschedule(struct ftm_quaddec *ftm)
+{
+	cancel_delayed_work(&ftm->delayedcounterwork);
+	if (ftm->poll_interval > 0)
+		schedule_delayed_work(&ftm->delayedcounterwork,
+				   msecs_to_jiffies(ftm->poll_interval));
+}
+
+/* Reports the hardware counter added the offset counter.
+ *
+ * The quadrature decodes does not use interrupts, because it cannot be
+ * guaranteed that the counter won't flip between 0xFFFF and 0x0000 at a high
+ * rate, causing Real Time performance degration. Instead the counter must be
+ * read frequently enough - the assumption is 150 KHz input can be handled with
+ * 100 ms read cycles.
+ */
+static void ftm_work_counter(struct ftm_quaddec *ftm,
+			     struct counter_result *returndata)
+{
+	/* only 16bits filled in*/
+	uint32_t hwcounter;
+	uint8_t currentregion;
+
+	mutex_lock(&ftm->ftm_quaddec_mutex);
+
+	ftm_read(ftm, FTM_CNT, &hwcounter);
+
+	/* Divide the counter in four regions:
+	 *   0x0000-0x4000-0x8000-0xC000-0xFFFF
+	 * When the hwcounter changes between region 0 and 3 there is an
+	 * over/underflow
+	 */
+	currentregion = hwcounter / 0x4000;
+
+	if (ftm->lastregion == 3 && currentregion == 0)
+		ftm->counteroffset += 0x10000;
+
+	if (ftm->lastregion == 0 && currentregion == 3)
+		ftm->counteroffset -= 0x10000;
+
+	ftm->lastregion = currentregion;
+
+	if (returndata)
+		returndata->value = ftm->counteroffset + hwcounter;
+
+	ftm_work_reschedule(ftm);
+
+	mutex_unlock(&ftm->ftm_quaddec_mutex);
+}
+
+/* wrapper around the real function */
+static void ftm_work_counter_delay(struct work_struct *workptr)
+{
+	struct delayed_work *work;
+	struct ftm_quaddec *ftm;
+
+	work = container_of(workptr, struct delayed_work, work);
+	ftm = container_of(work, struct ftm_quaddec, delayedcounterwork);
+
+	ftm_work_counter(ftm, NULL);
+}
+
+/* must be called with mutex locked */
 static void ftm_reset_counter(struct ftm_quaddec *ftm)
 {
+	ftm->counteroffset = 0;
+	ftm->lastregion = 0;
+
 	/* Reset hardware counter to CNTIN */
 	ftm_write(ftm, FTM_CNT, 0x0);
 }
@@ -110,18 +199,91 @@ static int ftm_quaddec_read_raw(struct iio_dev *indio_dev,
 				int *val, int *val2, long mask)
 {
 	struct ftm_quaddec *ftm = iio_priv(indio_dev);
-	uint32_t counter;
+	struct counter_result counter;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		ftm_read(ftm, FTM_CNT, &counter);
-		*val = counter;
+	case IIO_CHAN_INFO_PROCESSED:
+		ftm_work_counter(ftm, &counter);
+		if (mask == IIO_CHAN_INFO_RAW)
+			counter.value &= 0xffff;
+
+		*val = counter.value;
+
 		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
 	}
 }
 
+static uint32_t ftm_get_default_poll_interval(const struct ftm_quaddec *ftm)
+{
+	/* Read values from device tree */
+	uint32_t val;
+	const struct device_node *node = ftm->pdev->dev.of_node;
+
+	if (of_property_read_u32(node, "poll-interval", &val))
+		val = DEFAULT_POLL_INTERVAL;
+
+	return val;
+}
+
+static ssize_t ftm_read_default_poll_interval(struct iio_dev *indio_dev,
+					uintptr_t private,
+					struct iio_chan_spec const *chan,
+					char *buf)
+{
+	const struct ftm_quaddec *ftm = iio_priv(indio_dev);
+	uint32_t val = ftm_get_default_poll_interval(ftm);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", val);
+}
+
+static ssize_t ftm_read_poll_interval(struct iio_dev *indio_dev,
+				uintptr_t private,
+				struct iio_chan_spec const *chan,
+				char *buf)
+{
+	struct ftm_quaddec *ftm = iio_priv(indio_dev);
+
+	uint32_t poll_interval = READ_ONCE(ftm->poll_interval);
+
+	return snprintf(buf, PAGE_SIZE, "%u\n", poll_interval);
+}
+
+static ssize_t ftm_write_poll_interval(struct iio_dev *indio_dev,
+				uintptr_t private,
+				struct iio_chan_spec const *chan,
+				const char *buf, size_t len)
+{
+	struct ftm_quaddec *ftm = iio_priv(indio_dev);
+	uint32_t newpoll_interval;
+	uint32_t default_interval;
+
+	if (kstrtouint(buf, 10, &newpoll_interval) != 0) {
+		dev_err(&ftm->pdev->dev, "poll_interval not a number: '%s'\n",
+			buf);
+		return -EINVAL;
+	}
+
+	/* Don't accept polling times below the default value to protect the
+	 * system.
+	 */
+	default_interval = ftm_get_default_poll_interval(ftm);
+
+	if (newpoll_interval < default_interval && newpoll_interval != 0)
+		newpoll_interval = default_interval;
+
+	mutex_lock(&ftm->ftm_quaddec_mutex);
+
+	WRITE_ONCE(ftm->poll_interval, newpoll_interval);
+	ftm_work_reschedule(ftm);
+
+	mutex_unlock(&ftm->ftm_quaddec_mutex);
+
+	return len;
+}
+
 static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
 				uintptr_t private,
 				struct iio_chan_spec const *chan,
@@ -135,8 +297,11 @@ static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
 		return -EINVAL;
 	}
 
+	mutex_lock(&ftm->ftm_quaddec_mutex);
+
 	ftm_reset_counter(ftm);
 
+	mutex_unlock(&ftm->ftm_quaddec_mutex);
 	return len;
 }
 
@@ -192,6 +357,17 @@ static const struct iio_enum ftm_quaddec_prescaler_en = {
 };
 
 static const struct iio_chan_spec_ext_info ftm_quaddec_ext_info[] = {
+	{
+		.name = "default_poll_interval",
+		.shared = IIO_SHARED_BY_TYPE,
+		.read = ftm_read_default_poll_interval,
+	},
+	{
+		.name = "poll_interval",
+		.shared = IIO_SHARED_BY_TYPE,
+		.read = ftm_read_poll_interval,
+		.write = ftm_write_poll_interval,
+	},
 	{
 		.name = "reset",
 		.shared = IIO_SEPARATE,
@@ -205,7 +381,8 @@ static const struct iio_chan_spec_ext_info ftm_quaddec_ext_info[] = {
 static const struct iio_chan_spec ftm_quaddec_channels = {
 	.type = IIO_COUNT,
 	.channel = 0,
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			      BIT(IIO_CHAN_INFO_PROCESSED),
 	.ext_info = ftm_quaddec_ext_info,
 	.indexed = 1,
 };
@@ -232,10 +409,14 @@ static int ftm_quaddec_probe(struct platform_device *pdev)
 
 	ftm->pdev = pdev;
 	ftm->big_endian = of_property_read_bool(node, "big-endian");
+	ftm->counteroffset = 0;
+	ftm->lastregion = 0;
 	ftm->ftm_base = of_iomap(node, 0);
 	if (!ftm->ftm_base)
 		return -EINVAL;
 
+	ftm->poll_interval = ftm_get_default_poll_interval(ftm);
+
 	indio_dev->name = dev_name(&pdev->dev);
 	indio_dev->dev.parent = &pdev->dev;
 	indio_dev->info = &ftm_quaddec_iio_info;
@@ -245,9 +426,13 @@ static int ftm_quaddec_probe(struct platform_device *pdev)
 	ftm_quaddec_init(ftm);
 
 	mutex_init(&ftm->ftm_quaddec_mutex);
+	INIT_DELAYED_WORK(&ftm->delayedcounterwork, ftm_work_counter_delay);
+
+	ftm_work_reschedule(ftm);
 
 	ret = devm_iio_device_register(&pdev->dev, indio_dev);
 	if (ret) {
+		cancel_delayed_work_sync(&ftm->delayedcounterwork);
 		mutex_destroy(&ftm->ftm_quaddec_mutex);
 		iounmap(ftm->ftm_base);
 	}
@@ -261,13 +446,15 @@ static int ftm_quaddec_remove(struct platform_device *pdev)
 
 	ftm = (struct ftm_quaddec *)platform_get_drvdata(pdev);
 	indio_dev = iio_priv_to_dev(ftm);
-	/* This is needed to remove sysfs entries */
+	/* Make sure no concurrent attribute reads happen*/
 	devm_iio_device_unregister(&pdev->dev, indio_dev);
 
+	cancel_delayed_work_sync(&ftm->delayedcounterwork);
+
 	ftm_write(ftm, FTM_MODE, 0);
 
-	iounmap(ftm->ftm_base);
 	mutex_destroy(&ftm->ftm_quaddec_mutex);
+	iounmap(ftm->ftm_base);
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH 3/8] drivers/clocksource: timer-fsl-ftm: use common header for FlexTimer #defines

[Daniel Lezcano]

On 18/02/2019 15:03, Patrick Havelange wrote:

Changelog please

> Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> Reviewed-by: Esben Haabendal <esben@haabendal.dk>
> ---
>  drivers/clocksource/timer-fsl-ftm.c | 15 ++-------------
>  1 file changed, 2 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-fsl-ftm.c b/drivers/clocksource/timer-fsl-ftm.c
> index 846d18daf893..e1c34b2f53a5 100644
> --- a/drivers/clocksource/timer-fsl-ftm.c
> +++ b/drivers/clocksource/timer-fsl-ftm.c
> @@ -19,20 +19,9 @@
>  #include <linux/of_irq.h>
>  #include <linux/sched_clock.h>
>  #include <linux/slab.h>
> +#include <linux/fsl/ftm.h>
>  
> -#define FTM_SC		0x00
> -#define FTM_SC_CLK_SHIFT	3
> -#define FTM_SC_CLK_MASK	(0x3 << FTM_SC_CLK_SHIFT)
> -#define FTM_SC_CLK(c)	((c) << FTM_SC_CLK_SHIFT)
> -#define FTM_SC_PS_MASK	0x7
> -#define FTM_SC_TOIE	BIT(6)
> -#define FTM_SC_TOF	BIT(7)
> -
> -#define FTM_CNT		0x04
> -#define FTM_MOD		0x08
> -#define FTM_CNTIN	0x4C
> -
> -#define FTM_PS_MAX	7
> +#define FTM_SC_CLK(c)	((c) << FTM_SC_CLK_MASK_SHIFT)
>  
>  struct ftm_clock_device {
>  	void __iomem *clksrc_base;
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 5/8] iio/counter: add FlexTimer Module Quadrature decoder counter driver

[Jonathan Cameron]

On Mon, 18 Feb 2019 15:03:18 +0100
Patrick Havelange <patrick.havelange@essensium.com> wrote:

> This driver exposes the counter for the quadrature decoder of the
> FlexTimer Module, present in the LS1021A soc.
> 
> Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> Reviewed-by: Esben Haabendal <esben@haabendal.dk>
Given you cc'd William, I'm guessing you know about the counter
subsystem effort.  I would really rather not take any drivers
into IIO if we have any hope of getting that upstreamed soon
(which I personally think we do and should!).  The reason is
we end up having to maintain old ABI just because someone might be using
it and it makes the drivers very messy.

I'll review as is though as may be there are some elements that will
cross over.

Comments inline.  William: Looks like a straight forward conversion if
it makes sense to get this lined up as part of your initial submission?
You have quite a few drivers so I wouldn't have said it needs to be there
at the start, but good to have it soon after.

Jonathan

> ---
>  drivers/iio/counter/Kconfig       |  10 +
>  drivers/iio/counter/Makefile      |   1 +
>  drivers/iio/counter/ftm-quaddec.c | 294 ++++++++++++++++++++++++++++++
>  3 files changed, 305 insertions(+)
>  create mode 100644 drivers/iio/counter/ftm-quaddec.c
> 
> diff --git a/drivers/iio/counter/Kconfig b/drivers/iio/counter/Kconfig
> index bf1e559ad7cd..4641cb2e752a 100644
> --- a/drivers/iio/counter/Kconfig
> +++ b/drivers/iio/counter/Kconfig
> @@ -31,4 +31,14 @@ config STM32_LPTIMER_CNT
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called stm32-lptimer-cnt.
> +
> +config FTM_QUADDEC
> +	tristate "Flex Timer Module Quadrature decoder driver"
> +	help
> +	  Select this option to enable the Flex Timer Quadrature decoder
> +	  driver.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ftm-quaddec.
> +
>  endmenu
> diff --git a/drivers/iio/counter/Makefile b/drivers/iio/counter/Makefile
> index 1b9a896eb488..757c1f4196af 100644
> --- a/drivers/iio/counter/Makefile
> +++ b/drivers/iio/counter/Makefile
> @@ -6,3 +6,4 @@
>  
>  obj-$(CONFIG_104_QUAD_8)	+= 104-quad-8.o
>  obj-$(CONFIG_STM32_LPTIMER_CNT)	+= stm32-lptimer-cnt.o
> +obj-$(CONFIG_FTM_QUADDEC)	+= ftm-quaddec.o
> diff --git a/drivers/iio/counter/ftm-quaddec.c b/drivers/iio/counter/ftm-quaddec.c
> new file mode 100644
> index 000000000000..ca7e55a9ab3f
> --- /dev/null
> +++ b/drivers/iio/counter/ftm-quaddec.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Flex Timer Module Quadrature decoder
> + *
> + * This module implements a driver for decoding the FTM quadrature
> + * of ex. a LS1021A
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/workqueue.h>
Tidy these up. Not all are used.
> +#include <linux/swait.h>
> +#include <linux/sched.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mutex.h>
> +#include <linux/fsl/ftm.h>
> +
> +struct ftm_quaddec {
> +	struct platform_device *pdev;
> +	void __iomem *ftm_base;
> +	bool big_endian;

I'm curious. What is the benefit of running in big endian mode?

> +	struct mutex ftm_quaddec_mutex;
> +};
> +
> +#define HASFLAGS(flag, bits) ((flag & bits) ? 1 : 0)
Not used.


> +
> +#define DEFAULT_POLL_INTERVAL    100 /* in msec */
> +
> +static void ftm_read(struct ftm_quaddec *ftm, uint32_t offset, uint32_t *data)
> +{
> +	if (ftm->big_endian)
> +		*data = ioread32be(ftm->ftm_base + offset);
> +	else
> +		*data = ioread32(ftm->ftm_base + offset);
> +}
> +
> +static void ftm_write(struct ftm_quaddec *ftm, uint32_t offset, uint32_t data)
> +{
> +	if (ftm->big_endian)
> +		iowrite32be(data, ftm->ftm_base + offset);
> +	else
> +		iowrite32(data, ftm->ftm_base + offset);
> +}
> +
> +/* take mutex

Tidy this comment up.  I  would have said the flow as fairly
obvious and the only thing needed here is to document that
the mutex must be held?

> + * call ftm_clear_write_protection
> + * update settings
> + * call ftm_set_write_protection
> + * release mutex
> + */
> +static void ftm_clear_write_protection(struct ftm_quaddec *ftm)
> +{
> +	uint32_t flag;
> +
> +	/* First see if it is enabled */
> +	ftm_read(ftm, FTM_FMS, &flag);
> +
> +	if (flag & FTM_FMS_WPEN) {
> +		ftm_read(ftm, FTM_MODE, &flag);
> +		ftm_write(ftm, FTM_MODE, flag | FTM_MODE_WPDIS);
> +	}
> +}
> +
> +static void ftm_set_write_protection(struct ftm_quaddec *ftm)
> +{
> +	ftm_write(ftm, FTM_FMS, FTM_FMS_WPEN);
> +}
> +
> +static void ftm_reset_counter(struct ftm_quaddec *ftm)
> +{
> +	/* Reset hardware counter to CNTIN */
> +	ftm_write(ftm, FTM_CNT, 0x0);
> +}
> +
> +static void ftm_quaddec_init(struct ftm_quaddec *ftm)
> +{
> +	ftm_clear_write_protection(ftm);
> +
> +	/* Do not write in the region from the CNTIN register through the
IIO multiline syntax is
/*
 * Do not write
 * PWM..
 */
> +	 * PWMLOAD register when FTMEN = 0.
> +	 */
> +	ftm_write(ftm, FTM_MODE, FTM_MODE_FTMEN); /* enable FTM */

Drop any comments that are self explanatory.

> +	ftm_write(ftm, FTM_CNTIN, 0x0000);         /* zero init value */
> +	ftm_write(ftm, FTM_MOD, 0xffff);        /* max overflow value */
> +	ftm_write(ftm, FTM_CNT, 0x0);           /* reset counter value */
> +	ftm_write(ftm, FTM_SC, FTM_SC_PS_1);    /* prescale with x1 */
> +	/* Select quad mode */
> +	ftm_write(ftm, FTM_QDCTRL, FTM_QDCTRL_QUADEN);
> +
> +	/* Unused features and reset to default section */
> +	ftm_write(ftm, FTM_POL, 0x0);     /* polarity is active high */
> +	ftm_write(ftm, FTM_FLTCTRL, 0x0); /* all faults disabled */
> +	ftm_write(ftm, FTM_SYNCONF, 0x0); /* disable all sync */
> +	ftm_write(ftm, FTM_SYNC, 0xffff);
> +
> +	/* Lock the FTM */
> +	ftm_set_write_protection(ftm);
> +}
> +
> +static int ftm_quaddec_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val, int *val2, long mask)
> +{
> +	struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +	uint32_t counter;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ftm_read(ftm, FTM_CNT, &counter);
> +		*val = counter;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
> +				uintptr_t private,
> +				struct iio_chan_spec const *chan,
> +				const char *buf, size_t len)
> +{
> +	struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +
> +	/* Only "counter reset" is supported for now */
> +	if (!sysfs_streq(buf, "0")) {
> +		dev_warn(&ftm->pdev->dev, "Reset only accepts '0'\n");
> +		return -EINVAL;

Why not just make the channel attribute itself writeable given we are
setting it to 0?

> +	}
> +
> +	ftm_reset_counter(ftm);
> +
> +	return len;
> +}
> +
> +static int ftm_quaddec_get_prescaler(struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan)
> +{
> +	struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +	uint32_t scflags;
> +
> +	ftm_read(ftm, FTM_SC, &scflags);
> +
> +	return scflags & FTM_SC_PS_MASK;
> +}
> +
> +static int ftm_quaddec_set_prescaler(struct iio_dev *indio_dev,
> +					const struct iio_chan_spec *chan,
> +					unsigned int type)
> +{
> +	struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +
> +	uint32_t scflags;
> +
> +	mutex_lock(&ftm->ftm_quaddec_mutex);
> +
> +	ftm_read(ftm, FTM_SC, &scflags);
> +
> +	scflags &= ~FTM_SC_PS_MASK;
> +	type &= FTM_SC_PS_MASK; /*just to be 100% sure*/
> +
> +	scflags |= type;
> +
> +	/* Write */
> +	ftm_clear_write_protection(ftm);
> +	ftm_write(ftm, FTM_SC, scflags);
> +	ftm_set_write_protection(ftm);
> +
> +	/* Also resets the counter as it is undefined anyway now */
> +	ftm_reset_counter(ftm);
> +
> +	mutex_unlock(&ftm->ftm_quaddec_mutex);
> +	return 0;
> +}
> +
> +static const char * const ftm_quaddec_prescaler[] = {
> +	"1", "2", "4", "8", "16", "32", "64", "128"
> +};
> +
> +static const struct iio_enum ftm_quaddec_prescaler_en = {
> +	.items = ftm_quaddec_prescaler,
> +	.num_items = ARRAY_SIZE(ftm_quaddec_prescaler),
> +	.get = ftm_quaddec_get_prescaler,
> +	.set = ftm_quaddec_set_prescaler,
> +};
> +
> +static const struct iio_chan_spec_ext_info ftm_quaddec_ext_info[] = {
> +	{
> +		.name = "reset",
> +		.shared = IIO_SEPARATE,
> +		.write = ftm_write_reset,
> +	},
> +	IIO_ENUM("prescaler", IIO_SEPARATE, &ftm_quaddec_prescaler_en),

This looks like non standard ABI.  Needs documentation in
Documentation/ABI/testing/sysfs-bus-iio-*

> +	IIO_ENUM_AVAILABLE("prescaler", &ftm_quaddec_prescaler_en),
> +	{}
> +};
> +
> +static const struct iio_chan_spec ftm_quaddec_channels = {
> +	.type = IIO_COUNT,
> +	.channel = 0,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	.ext_info = ftm_quaddec_ext_info,
> +	.indexed = 1,
> +};
> +
> +static const struct iio_info ftm_quaddec_iio_info = {
> +	.read_raw = ftm_quaddec_read_raw,
> +};
> +
> +static int ftm_quaddec_probe(struct platform_device *pdev)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ftm_quaddec *ftm;
> +	int ret;
> +
> +	struct device_node *node = pdev->dev.of_node;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*ftm));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	ftm = iio_priv(indio_dev);
> +
> +	platform_set_drvdata(pdev, ftm);
> +
> +	ftm->pdev = pdev;
> +	ftm->big_endian = of_property_read_bool(node, "big-endian");
> +	ftm->ftm_base = of_iomap(node, 0);
> +	if (!ftm->ftm_base)
> +		return -EINVAL;
> +
> +	indio_dev->name = dev_name(&pdev->dev);

This should be a nice part number like string.  What does
it evaluate to here?

> +	indio_dev->dev.parent = &pdev->dev;
> +	indio_dev->info = &ftm_quaddec_iio_info;
> +	indio_dev->num_channels = 1;
> +	indio_dev->channels = &ftm_quaddec_channels;
> +
> +	ftm_quaddec_init(ftm);
> +
> +	mutex_init(&ftm->ftm_quaddec_mutex);
> +
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret) {
> +		mutex_destroy(&ftm->ftm_quaddec_mutex);

Don't have managed devm_ calls after anything unmanaged.
I opens you up to races and is hard to review.

> +		iounmap(ftm->ftm_base);
I would suggest not using of_iomap, then you can
use the devm_ioremap form handle this for you.

> +	}
> +	return ret;
> +}
> +
> +static int ftm_quaddec_remove(struct platform_device *pdev)
> +{
> +	struct ftm_quaddec *ftm;
> +	struct iio_dev *indio_dev;
> +
> +	ftm = (struct ftm_quaddec *)platform_get_drvdata(pdev);

platform_get_drvdata returns a void *.

C spec says you can always cast implicitly from a void * to any
other point type.  Hence you don't need the cast.
(I'm too lazy to find the reference now as it's been a long
day of reviewing but google will find it for you if interested)

> +	indio_dev = iio_priv_to_dev(ftm);
> +	/* This is needed to remove sysfs entries */

It does a lot more than that, so please remove the comment.
Actually this worries me.  You should not need to manually
call devm_iio_device_register, but you do because of the ordering
and the fact you want to remove the interfaces before turning the
device off.  In that case, do not use the devm_ form but
instead iio_device_register and do the error handling paths
manually.  An alternative is to use
devm_add_action_or_reset to call unwind functions automatically
for the few device specific calls you have.


> +	devm_iio_device_unregister(&pdev->dev, indio_dev);
> +
> +	ftm_write(ftm, FTM_MODE, 0);
> +
> +	iounmap(ftm->ftm_base);
> +	mutex_destroy(&ftm->ftm_quaddec_mutex);

mutex destroy is only really used in debug paths and is often
skipped in remove functions to simplify things.
(particularly when managed cleanup is going on).

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ftm_quaddec_match[] = {
> +	{ .compatible = "fsl,ftm-quaddec" },
> +	{},
> +};
> +
> +static struct platform_driver ftm_quaddec_driver = {
> +	.driver = {
> +		.name = "ftm-quaddec",
> +		.owner = THIS_MODULE,
> +		.of_match_table = ftm_quaddec_match,
> +	},
> +	.probe = ftm_quaddec_probe,
> +	.remove = ftm_quaddec_remove,
> +};
> +
> +module_platform_driver(ftm_quaddec_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Kjeld Flarup <kfa@deif.com");
> +MODULE_AUTHOR("Patrick Havelange <patrick.havelange@essensium.com");

Re: [PATCH 8/8] iio/counter/ftm-quaddec: add handling of under/overflow of the counter.

[Jonathan Cameron]

On Mon, 18 Feb 2019 15:03:21 +0100
Patrick Havelange <patrick.havelange@essensium.com> wrote:

> This is implemented by polling the counter value. A new parameter
> "poll-interval" can be set in the device tree, or can be changed
> at runtime. The reason for the polling is to avoid interrupts flooding.
> If the quadrature input is going up and down around the overflow value
> (or around 0), the interrupt will be triggering all the time. Thus,
> polling is an easy way to handle overflow in a consistent way.
> Polling can still be disabled by setting poll-interval to 0.
> 
> Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> Reviewed-by: Esben Haabendal <esben@haabendal.dk>
Comments inline.

Jonathan

> ---
>  drivers/iio/counter/ftm-quaddec.c | 199 +++++++++++++++++++++++++++++-
>  1 file changed, 193 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/counter/ftm-quaddec.c b/drivers/iio/counter/ftm-quaddec.c
> index ca7e55a9ab3f..3a0395c3ef33 100644
> --- a/drivers/iio/counter/ftm-quaddec.c
> +++ b/drivers/iio/counter/ftm-quaddec.c
> @@ -25,11 +25,33 @@
>  
>  struct ftm_quaddec {
>  	struct platform_device *pdev;
> +	struct delayed_work delayedcounterwork;
>  	void __iomem *ftm_base;
>  	bool big_endian;
> +
> +	/* Offset added to the counter to adjust for overflows of the
> +	 * 16 bit HW counter. Only the 16 MSB are set.
Comment syntax.
> +	 */
> +	uint32_t counteroffset;
> +
> +	/* Store the counter on each read, this is used to detect
> +	 * if the counter readout if we over or underflow
> +	 */
> +	uint8_t lastregion;
> +
> +	/* Poll-interval, in ms before delayed work must poll counter */
> +	uint16_t poll_interval;
> +
>  	struct mutex ftm_quaddec_mutex;
>  };
>  
> +struct counter_result {
> +	/* 16 MSB are from the counteroffset
> +	 * 16 LSB are from the hardware counter
> +	 */
> +	uint32_t value;
Why the structure?

> +};
> +
>  #define HASFLAGS(flag, bits) ((flag & bits) ? 1 : 0)
>  
>  #define DEFAULT_POLL_INTERVAL    100 /* in msec */
> @@ -74,8 +96,75 @@ static void ftm_set_write_protection(struct ftm_quaddec *ftm)
>  	ftm_write(ftm, FTM_FMS, FTM_FMS_WPEN);
>  }
>  
> +/* must be called with mutex locked */
> +static void ftm_work_reschedule(struct ftm_quaddec *ftm)
> +{
> +	cancel_delayed_work(&ftm->delayedcounterwork);
> +	if (ftm->poll_interval > 0)
> +		schedule_delayed_work(&ftm->delayedcounterwork,
> +				   msecs_to_jiffies(ftm->poll_interval));
> +}
> +
> +/* Reports the hardware counter added the offset counter.
> + *
> + * The quadrature decodes does not use interrupts, because it cannot be
> + * guaranteed that the counter won't flip between 0xFFFF and 0x0000 at a high
> + * rate, causing Real Time performance degration. Instead the counter must be
> + * read frequently enough - the assumption is 150 KHz input can be handled with
> + * 100 ms read cycles.
> + */
> +static void ftm_work_counter(struct ftm_quaddec *ftm,
> +			     struct counter_result *returndata)
> +{
> +	/* only 16bits filled in*/
> +	uint32_t hwcounter;
> +	uint8_t currentregion;
> +
> +	mutex_lock(&ftm->ftm_quaddec_mutex);
> +
> +	ftm_read(ftm, FTM_CNT, &hwcounter);
> +
> +	/* Divide the counter in four regions:
> +	 *   0x0000-0x4000-0x8000-0xC000-0xFFFF
> +	 * When the hwcounter changes between region 0 and 3 there is an
> +	 * over/underflow
> +	 */
> +	currentregion = hwcounter / 0x4000;
> +
> +	if (ftm->lastregion == 3 && currentregion == 0)
> +		ftm->counteroffset += 0x10000;
> +
> +	if (ftm->lastregion == 0 && currentregion == 3)
> +		ftm->counteroffset -= 0x10000;
> +
> +	ftm->lastregion = currentregion;
> +
> +	if (returndata)
> +		returndata->value = ftm->counteroffset + hwcounter;
> +
> +	ftm_work_reschedule(ftm);
> +
> +	mutex_unlock(&ftm->ftm_quaddec_mutex);
> +}
> +
> +/* wrapper around the real function */
> +static void ftm_work_counter_delay(struct work_struct *workptr)
> +{
> +	struct delayed_work *work;
> +	struct ftm_quaddec *ftm;
> +
> +	work = container_of(workptr, struct delayed_work, work);
> +	ftm = container_of(work, struct ftm_quaddec, delayedcounterwork);
> +
> +	ftm_work_counter(ftm, NULL);
> +}
> +
> +/* must be called with mutex locked */
>  static void ftm_reset_counter(struct ftm_quaddec *ftm)
>  {
> +	ftm->counteroffset = 0;
> +	ftm->lastregion = 0;
> +
>  	/* Reset hardware counter to CNTIN */
>  	ftm_write(ftm, FTM_CNT, 0x0);
>  }
> @@ -110,18 +199,91 @@ static int ftm_quaddec_read_raw(struct iio_dev *indio_dev,
>  				int *val, int *val2, long mask)
>  {
>  	struct ftm_quaddec *ftm = iio_priv(indio_dev);
> -	uint32_t counter;
> +	struct counter_result counter;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		ftm_read(ftm, FTM_CNT, &counter);
> -		*val = counter;
> +	case IIO_CHAN_INFO_PROCESSED:

> +		ftm_work_counter(ftm, &counter);
> +		if (mask == IIO_CHAN_INFO_RAW)
> +			counter.value &= 0xffff;
> +
> +		*val = counter.value;
> +
>  		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
>  	}
>  }
>  
> +static uint32_t ftm_get_default_poll_interval(const struct ftm_quaddec *ftm)
> +{
> +	/* Read values from device tree */
> +	uint32_t val;
> +	const struct device_node *node = ftm->pdev->dev.of_node;
> +
> +	if (of_property_read_u32(node, "poll-interval", &val))
> +		val = DEFAULT_POLL_INTERVAL;
> +
> +	return val;
> +}
> +
> +static ssize_t ftm_read_default_poll_interval(struct iio_dev *indio_dev,
> +					uintptr_t private,
> +					struct iio_chan_spec const *chan,
> +					char *buf)
> +{
> +	const struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +	uint32_t val = ftm_get_default_poll_interval(ftm);
> +
> +	return snprintf(buf, PAGE_SIZE, "%u\n", val);
> +}
> +
> +static ssize_t ftm_read_poll_interval(struct iio_dev *indio_dev,
> +				uintptr_t private,
> +				struct iio_chan_spec const *chan,
> +				char *buf)
> +{
> +	struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +
> +	uint32_t poll_interval = READ_ONCE(ftm->poll_interval);
Why bother with the local variable? I'm not awake enough to see
why the READ_ONCE is necessary here.
If worried about it, just take the lock, we are far from high
performance in this path.

> +
> +	return snprintf(buf, PAGE_SIZE, "%u\n", poll_interval);
> +}
> +
> +static ssize_t ftm_write_poll_interval(struct iio_dev *indio_dev,
> +				uintptr_t private,
> +				struct iio_chan_spec const *chan,
> +				const char *buf, size_t len)
> +{
> +	struct ftm_quaddec *ftm = iio_priv(indio_dev);
> +	uint32_t newpoll_interval;
> +	uint32_t default_interval;
> +
> +	if (kstrtouint(buf, 10, &newpoll_interval) != 0) {
> +		dev_err(&ftm->pdev->dev, "poll_interval not a number: '%s'\n",
> +			buf);
> +		return -EINVAL;
> +	}
> +
> +	/* Don't accept polling times below the default value to protect the
> +	 * system.
> +	 */
> +	default_interval = ftm_get_default_poll_interval(ftm);
> +
> +	if (newpoll_interval < default_interval && newpoll_interval != 0)
> +		newpoll_interval = default_interval;
> +
> +	mutex_lock(&ftm->ftm_quaddec_mutex);
> +
> +	WRITE_ONCE(ftm->poll_interval, newpoll_interval);
> +	ftm_work_reschedule(ftm);
> +
> +	mutex_unlock(&ftm->ftm_quaddec_mutex);
> +
> +	return len;
> +}
> +
>  static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
>  				uintptr_t private,
>  				struct iio_chan_spec const *chan,
> @@ -135,8 +297,11 @@ static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
>  		return -EINVAL;
>  	}
>  
> +	mutex_lock(&ftm->ftm_quaddec_mutex);
> +
>  	ftm_reset_counter(ftm);
>  
> +	mutex_unlock(&ftm->ftm_quaddec_mutex);
>  	return len;
>  }
>  
> @@ -192,6 +357,17 @@ static const struct iio_enum ftm_quaddec_prescaler_en = {
>  };
>  
>  static const struct iio_chan_spec_ext_info ftm_quaddec_ext_info[] = {
> +	{
> +		.name = "default_poll_interval",
> +		.shared = IIO_SHARED_BY_TYPE,
> +		.read = ftm_read_default_poll_interval,
Why is this relevant if the value is set to something else?
> +	},
> +	{
> +		.name = "poll_interval",
> +		.shared = IIO_SHARED_BY_TYPE,
> +		.read = ftm_read_poll_interval,
> +		.write = ftm_write_poll_interval,
Hmm.  New ABI that needs documenting. I'm not sure how we should
handle this.  Perhaps William has a good suggestion for how to do it.
> +	},
>  	{
>  		.name = "reset",
>  		.shared = IIO_SEPARATE,
> @@ -205,7 +381,8 @@ static const struct iio_chan_spec_ext_info ftm_quaddec_ext_info[] = {
>  static const struct iio_chan_spec ftm_quaddec_channels = {
>  	.type = IIO_COUNT,
>  	.channel = 0,
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			      BIT(IIO_CHAN_INFO_PROCESSED),
Why?  As a general rule, we don't allow bother RAW and processed for
a particular channel.  Note the raw value doesn't actually 'have'
to be the value off a sensor - it is just expected to not have
linear scaling and offset applied (which are encode dependent
here so can't be applied).  So just use raw for your non overflowing
version.

>  	.ext_info = ftm_quaddec_ext_info,
>  	.indexed = 1,
>  };
> @@ -232,10 +409,14 @@ static int ftm_quaddec_probe(struct platform_device *pdev)
>  
>  	ftm->pdev = pdev;
>  	ftm->big_endian = of_property_read_bool(node, "big-endian");
> +	ftm->counteroffset = 0;
> +	ftm->lastregion = 0;
>  	ftm->ftm_base = of_iomap(node, 0);
>  	if (!ftm->ftm_base)
>  		return -EINVAL;
>  
> +	ftm->poll_interval = ftm_get_default_poll_interval(ftm);
> +
>  	indio_dev->name = dev_name(&pdev->dev);
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->info = &ftm_quaddec_iio_info;
> @@ -245,9 +426,13 @@ static int ftm_quaddec_probe(struct platform_device *pdev)
>  	ftm_quaddec_init(ftm);
>  
>  	mutex_init(&ftm->ftm_quaddec_mutex);
> +	INIT_DELAYED_WORK(&ftm->delayedcounterwork, ftm_work_counter_delay);
> +
> +	ftm_work_reschedule(ftm);
>  
>  	ret = devm_iio_device_register(&pdev->dev, indio_dev);
>  	if (ret) {
> +		cancel_delayed_work_sync(&ftm->delayedcounterwork);
>  		mutex_destroy(&ftm->ftm_quaddec_mutex);
>  		iounmap(ftm->ftm_base);
>  	}
> @@ -261,13 +446,15 @@ static int ftm_quaddec_remove(struct platform_device *pdev)
>  
>  	ftm = (struct ftm_quaddec *)platform_get_drvdata(pdev);
>  	indio_dev = iio_priv_to_dev(ftm);
> -	/* This is needed to remove sysfs entries */
> +	/* Make sure no concurrent attribute reads happen*/
>  	devm_iio_device_unregister(&pdev->dev, indio_dev);
>  
> +	cancel_delayed_work_sync(&ftm->delayedcounterwork);
> +
>  	ftm_write(ftm, FTM_MODE, 0);
>  
> -	iounmap(ftm->ftm_base);
>  	mutex_destroy(&ftm->ftm_quaddec_mutex);
> +	iounmap(ftm->ftm_base);
Why the reorder?  If it was wrong in the first place, fix the
earlier patch not this one.
>  
>  	return 0;
>  }

Re: [PATCH 5/8] iio/counter: add FlexTimer Module Quadrature decoder counter driver

[William Breathitt Gray]

On Wed, Feb 20, 2019 at 04:41:54PM +0000, Jonathan Cameron wrote:
> On Mon, 18 Feb 2019 15:03:18 +0100
> Patrick Havelange <patrick.havelange@essensium.com> wrote:
> 
> > This driver exposes the counter for the quadrature decoder of the
> > FlexTimer Module, present in the LS1021A soc.
> > 
> > Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> > Reviewed-by: Esben Haabendal <esben@haabendal.dk>
> Given you cc'd William, I'm guessing you know about the counter
> subsystem effort.  I would really rather not take any drivers
> into IIO if we have any hope of getting that upstreamed soon
> (which I personally think we do and should!).  The reason is
> we end up having to maintain old ABI just because someone might be using
> it and it makes the drivers very messy.
> 
> I'll review as is though as may be there are some elements that will
> cross over.
> 
> Comments inline.  William: Looks like a straight forward conversion if
> it makes sense to get this lined up as part of your initial submission?
> You have quite a few drivers so I wouldn't have said it needs to be there
> at the start, but good to have it soon after.
> 
> Jonathan

I agree, we should try to merge this as part of Counter subsystem
introduction rather than as another IIO Counter driver. As we determined
when adding support for the STM32 timers, the existing IIO Counter API
is fundamentally unsuitable for representing counter devices. So
regardless of how a new Counter API is merged, the existing IIO Counter
API must be deprecated.

Patrick, I apologize for the confusion this has caused. Would you be
able to convert this driver to use the proposed Counter subsystem API
from this patchset that I believe you encountered before:
https://marc.info/?l=linux-arm-kernel&m=153229982404051

Although it was last updated in October, I believe you should be able to
rebase that Counter subsystem introduction patchset cleanly on top of
the IIO tree (if there are any merge conflicts send me an email). Take a
look at the generic-counter.rst file under the Documentation/driver-api/
directory for an overview of the API; the counter drivers under the
drivers/counter/ directory also make good references.

If you have any difficulties understanding the API, or any other
troubles, don't hesitate to ask. Hopefully, I've made the documentation
clear enough to make the conversion of this driver quick and easy -- and
if not, then it's something I need to fix, so let me know. :-)

William Breathitt Gray

Re: [PATCH 5/8] iio/counter: add FlexTimer Module Quadrature decoder counter driver

[William Breathitt Gray]

On Thu, Feb 21, 2019 at 10:09:54AM +0900, William Breathitt Gray wrote:
> On Wed, Feb 20, 2019 at 04:41:54PM +0000, Jonathan Cameron wrote:
> > On Mon, 18 Feb 2019 15:03:18 +0100
> > Patrick Havelange <patrick.havelange@essensium.com> wrote:
> > 
> > > This driver exposes the counter for the quadrature decoder of the
> > > FlexTimer Module, present in the LS1021A soc.
> > > 
> > > Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> > > Reviewed-by: Esben Haabendal <esben@haabendal.dk>
> > Given you cc'd William, I'm guessing you know about the counter
> > subsystem effort.  I would really rather not take any drivers
> > into IIO if we have any hope of getting that upstreamed soon
> > (which I personally think we do and should!).  The reason is
> > we end up having to maintain old ABI just because someone might be using
> > it and it makes the drivers very messy.
> > 
> > I'll review as is though as may be there are some elements that will
> > cross over.
> > 
> > Comments inline.  William: Looks like a straight forward conversion if
> > it makes sense to get this lined up as part of your initial submission?
> > You have quite a few drivers so I wouldn't have said it needs to be there
> > at the start, but good to have it soon after.
> > 
> > Jonathan
> 
> I agree, we should try to merge this as part of Counter subsystem
> introduction rather than as another IIO Counter driver. As we determined
> when adding support for the STM32 timers, the existing IIO Counter API
> is fundamentally unsuitable for representing counter devices. So
> regardless of how a new Counter API is merged, the existing IIO Counter
> API must be deprecated.
> 
> Patrick, I apologize for the confusion this has caused. Would you be
> able to convert this driver to use the proposed Counter subsystem API
> from this patchset that I believe you encountered before:
> https://marc.info/?l=linux-arm-kernel&m=153229982404051
> 
> Although it was last updated in October, I believe you should be able to
> rebase that Counter subsystem introduction patchset cleanly on top of
> the IIO tree (if there are any merge conflicts send me an email). Take a
> look at the generic-counter.rst file under the Documentation/driver-api/
> directory for an overview of the API; the counter drivers under the
> drivers/counter/ directory also make good references.
> 
> If you have any difficulties understanding the API, or any other
> troubles, don't hesitate to ask. Hopefully, I've made the documentation
> clear enough to make the conversion of this driver quick and easy -- and
> if not, then it's something I need to fix, so let me know. :-)
> 
> William Breathitt Gray

Patrick,

It looks like there were some minor conflicts with the v9 patchset, so
I've rebased it on top of the latest iio tree testing branch and
resolved the conflicts in my personal repository. Please pull from my
personal repository at https://gitlab.com/vilhelmgray/iio.git and base
your patches on top of the generic_counter_v10 branch.

William Breathitt Gray

Re: [PATCH 5/8] iio/counter: add FlexTimer Module Quadrature decoder counter driver

[Patrick Havelange]

Hi Jonathan,

Thanks for your comments, I'll make a new version of the patch based
on your input.

William, I'll rebase the next version on top of your branch.

I'm glad the counter subsystem effort is progressing :)


Patrick Havelange.

On Thu, Feb 21, 2019 at 9:27 AM William Breathitt Gray
<vilhelm.gray@gmail.com> wrote:
>
> On Thu, Feb 21, 2019 at 10:09:54AM +0900, William Breathitt Gray wrote:
> > On Wed, Feb 20, 2019 at 04:41:54PM +0000, Jonathan Cameron wrote:
> > > On Mon, 18 Feb 2019 15:03:18 +0100
> > > Patrick Havelange <patrick.havelange@essensium.com> wrote:
> > >
> > > > This driver exposes the counter for the quadrature decoder of the
> > > > FlexTimer Module, present in the LS1021A soc.
> > > >
> > > > Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> > > > Reviewed-by: Esben Haabendal <esben@haabendal.dk>
> > > Given you cc'd William, I'm guessing you know about the counter
> > > subsystem effort.  I would really rather not take any drivers
> > > into IIO if we have any hope of getting that upstreamed soon
> > > (which I personally think we do and should!).  The reason is
> > > we end up having to maintain old ABI just because someone might be using
> > > it and it makes the drivers very messy.
> > >
> > > I'll review as is though as may be there are some elements that will
> > > cross over.
> > >
> > > Comments inline.  William: Looks like a straight forward conversion if
> > > it makes sense to get this lined up as part of your initial submission?
> > > You have quite a few drivers so I wouldn't have said it needs to be there
> > > at the start, but good to have it soon after.
> > >
> > > Jonathan
> >
> > I agree, we should try to merge this as part of Counter subsystem
> > introduction rather than as another IIO Counter driver. As we determined
> > when adding support for the STM32 timers, the existing IIO Counter API
> > is fundamentally unsuitable for representing counter devices. So
> > regardless of how a new Counter API is merged, the existing IIO Counter
> > API must be deprecated.
> >
> > Patrick, I apologize for the confusion this has caused. Would you be
> > able to convert this driver to use the proposed Counter subsystem API
> > from this patchset that I believe you encountered before:
> > https://marc.info/?l=linux-arm-kernel&m=153229982404051
> >
> > Although it was last updated in October, I believe you should be able to
> > rebase that Counter subsystem introduction patchset cleanly on top of
> > the IIO tree (if there are any merge conflicts send me an email). Take a
> > look at the generic-counter.rst file under the Documentation/driver-api/
> > directory for an overview of the API; the counter drivers under the
> > drivers/counter/ directory also make good references.
> >
> > If you have any difficulties understanding the API, or any other
> > troubles, don't hesitate to ask. Hopefully, I've made the documentation
> > clear enough to make the conversion of this driver quick and easy -- and
> > if not, then it's something I need to fix, so let me know. :-)
> >
> > William Breathitt Gray
>
> Patrick,
>
> It looks like there were some minor conflicts with the v9 patchset, so
> I've rebased it on top of the latest iio tree testing branch and
> resolved the conflicts in my personal repository. Please pull from my
> personal repository at https://gitlab.com/vilhelmgray/iio.git and base
> your patches on top of the generic_counter_v10 branch.
>
> William Breathitt Gray

Re: [PATCH 7/8] dt-bindings: iio/counter: ftm-quaddec: add poll-interval parameter

[Rob Herring]

On Mon, 18 Feb 2019 15:03:20 +0100, Patrick Havelange wrote:
> New optional parameter supported by updated driver.
> 
> Signed-off-by: Patrick Havelange <patrick.havelange@essensium.com>
> Reviewed-by: Esben Haabendal <esben@haabendal.dk>
> ---
>  .../devicetree/bindings/iio/counter/ftm-quaddec.txt       | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 5/8] iio/counter: add FlexTimer Module Quadrature decoder counter driver

[Patrick Havelange]

On Wed, Feb 20, 2019 at 5:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
[skipped]
> > +
> > +struct ftm_quaddec {
> > +     struct platform_device *pdev;
> > +     void __iomem *ftm_base;
> > +     bool big_endian;
>
> I'm curious. What is the benefit of running in big endian mode?

It is based on the same behaviour as in drivers/clocksource/timer-fsl-ftm.c
The FlexTimer itself on the board I'm testing it with is working in
big endian mode, so this mode is required.

> > +static ssize_t ftm_write_reset(struct iio_dev *indio_dev,
> > +                             uintptr_t private,
> > +                             struct iio_chan_spec const *chan,
> > +                             const char *buf, size_t len)
> > +{
> > +     struct ftm_quaddec *ftm = iio_priv(indio_dev);
> > +
> > +     /* Only "counter reset" is supported for now */
> > +     if (!sysfs_streq(buf, "0")) {
> > +             dev_warn(&ftm->pdev->dev, "Reset only accepts '0'\n");
> > +             return -EINVAL;
>
> Why not just make the channel attribute itself writeable given we are
> setting it to 0?

Good idea, I'll see if this can be applied in the new subsystem.

[skipped]

All other comments are Acked.