[PATCH 0/8] counter: ti-eqep: implement features for speed measurement

[PATCH 0/8] counter: ti-eqep: implement features for speed measurement

[David Lechner]

Now that the counter subsystem has a new chrdev for events, we can use this to
add new features to the TI eQEP driver to be able to do accurate speed
measurement.

This adds two new device-level components, a Unit Timer and an Edge Capture
Unit. I don't have much knowledge about other available counter hardware, so
I don't know if it makes sense to try to make these more generic, e.g.
counterX/timerY/* and counterX/captureY/*. For now, they are just flat
(counterX/unit_timer_* and counterX/edge_capture_unit_*) and assume there is
only one instance per counter device.

This has been tested on a BeagleBone Blue with LEGO MINDSTORMS motors.

David Lechner (8):
  counter/ti-eqep: implement over/underflow events
  counter/ti-eqep: add support for direction
  counter/ti-eqep: add support for unit timer
  docs: counter: add unit timer sysfs attributes
  counter/ti-eqep: add support for latched position
  docs: counter: add latch_mode and latched_count sysfs attributes
  counter/ti-eqep: add support for edge capture unit
  docs: counter: add edge_capture_unit_* attributes

 Documentation/ABI/testing/sysfs-bus-counter | 100 +++-
 drivers/counter/ti-eqep.c                   | 482 +++++++++++++++++++-
 include/uapi/linux/counter.h                |   4 +
 3 files changed, 575 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH 1/8] counter/ti-eqep: implement over/underflow events

[David Lechner]

This adds support to the TI eQEP counter driver for subscribing to
overflow and underflow events using the counter chrdev interface.

Since this is the first event added, this involved adding an irq
handler. Also, additional range checks had to be added to the ceiling
attribute to avoid infinite interrupts.

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 117 insertions(+), 2 deletions(-)

diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index 09817c953f9a..b7c79435e127 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -7,6 +7,7 @@
 
 #include <linux/bitops.h>
 #include <linux/counter.h>
+#include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
@@ -67,6 +68,44 @@
 #define QEPCTL_UTE		BIT(1)
 #define QEPCTL_WDE		BIT(0)
 
+#define QEINT_UTO		BIT(11)
+#define QEINT_IEL		BIT(10)
+#define QEINT_SEL		BIT(9)
+#define QEINT_PCM		BIT(8)
+#define QEINT_PCR		BIT(7)
+#define QEINT_PCO		BIT(6)
+#define QEINT_PCU		BIT(5)
+#define QEINT_WTO		BIT(4)
+#define QEINT_QDC		BIT(3)
+#define QEINT_PHE		BIT(2)
+#define QEINT_PCE		BIT(1)
+
+#define QFLG_UTO		BIT(11)
+#define QFLG_IEL		BIT(10)
+#define QFLG_SEL		BIT(9)
+#define QFLG_PCM		BIT(8)
+#define QFLG_PCR		BIT(7)
+#define QFLG_PCO		BIT(6)
+#define QFLG_PCU		BIT(5)
+#define QFLG_WTO		BIT(4)
+#define QFLG_QDC		BIT(3)
+#define QFLG_PHE		BIT(2)
+#define QFLG_PCE		BIT(1)
+#define QFLG_INT		BIT(0)
+
+#define QCLR_UTO		BIT(11)
+#define QCLR_IEL		BIT(10)
+#define QCLR_SEL		BIT(9)
+#define QCLR_PCM		BIT(8)
+#define QCLR_PCR		BIT(7)
+#define QCLR_PCO		BIT(6)
+#define QCLR_PCU		BIT(5)
+#define QCLR_WTO		BIT(4)
+#define QCLR_QDC		BIT(3)
+#define QCLR_PHE		BIT(2)
+#define QCLR_PCE		BIT(1)
+#define QCLR_INT		BIT(0)
+
 /* EQEP Inputs */
 enum {
 	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
@@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter,
 	}
 }
 
+static int ti_eqep_events_configure(struct counter_device *counter)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	struct counter_event_node *event_node;
+	u32 qeint = 0;
+
+	list_for_each_entry(event_node, &counter->events_list, l) {
+		switch (event_node->event) {
+		case COUNTER_EVENT_OVERFLOW:
+			qeint |= QEINT_PCO;
+			break;
+		case COUNTER_EVENT_UNDERFLOW:
+			qeint |= QEINT_PCU;
+			break;
+		}
+	}
+
+	return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint);
+}
+
+static int ti_eqep_watch_validate(struct counter_device *counter,
+				  const struct counter_watch *watch)
+{
+	switch (watch->event) {
+	case COUNTER_EVENT_OVERFLOW:
+	case COUNTER_EVENT_UNDERFLOW:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 static const struct counter_ops ti_eqep_counter_ops = {
 	.count_read	= ti_eqep_count_read,
 	.count_write	= ti_eqep_count_write,
 	.function_read	= ti_eqep_function_read,
 	.function_write	= ti_eqep_function_write,
 	.action_read	= ti_eqep_action_read,
+	.events_configure = ti_eqep_events_configure,
+	.watch_validate	= ti_eqep_watch_validate,
 };
 
 static int ti_eqep_position_ceiling_read(struct counter_device *counter,
@@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
 					  u64 ceiling)
 {
 	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qposmax = ceiling;
 
-	if (ceiling != (u32)ceiling)
+	/* ensure that value fits in 32-bit register */
+	if (qposmax != ceiling)
 		return -ERANGE;
 
-	regmap_write(priv->regmap32, QPOSMAX, ceiling);
+	/* protect against infinite overflow interrupts */
+	if (qposmax == 0)
+		return -EINVAL;
+
+	regmap_write(priv->regmap32, QPOSMAX, qposmax);
 
 	return 0;
 }
@@ -349,6 +428,25 @@ static struct counter_count ti_eqep_counts[] = {
 	},
 };
 
+static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
+{
+	struct ti_eqep_cnt *priv = dev_id;
+	struct counter_device *counter = &priv->counter;
+	u32 qflg;
+
+	regmap_read(priv->regmap16, QFLG, &qflg);
+
+	if (qflg & QFLG_PCO)
+		counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0);
+
+	if (qflg & QFLG_PCU)
+		counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0);
+
+	regmap_set_bits(priv->regmap16, QCLR, ~0);
+
+	return IRQ_HANDLED;
+}
+
 static const struct regmap_config ti_eqep_regmap32_config = {
 	.name = "32-bit",
 	.reg_bits = 32,
@@ -371,6 +469,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
 	struct ti_eqep_cnt *priv;
 	void __iomem *base;
 	int err;
+	int irq;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -390,6 +489,15 @@ static int ti_eqep_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->regmap16))
 		return PTR_ERR(priv->regmap16);
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	err = devm_request_threaded_irq(dev, irq, NULL, ti_eqep_irq_handler,
+					IRQF_ONESHOT, dev_name(dev), priv);
+	if (err < 0)
+		return err;
+
 	priv->counter.name = dev_name(dev);
 	priv->counter.parent = dev;
 	priv->counter.ops = &ti_eqep_counter_ops;
@@ -409,6 +517,13 @@ static int ti_eqep_probe(struct platform_device *pdev)
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
+	/*
+	 * We can end up with an interupt infinite loop (interrupts triggered
+	 * as soon as they are cleared) if we leave this at the default value
+	 * of 0 and events are enabled.
+	 */
+	regmap_write(priv->regmap32, QPOSMAX, UINT_MAX);
+
 	err = counter_register(&priv->counter);
 	if (err < 0) {
 		pm_runtime_put_sync(dev);
-- 
2.25.1

Re: [PATCH 1/8] counter/ti-eqep: implement over/underflow events

[Jonathan Cameron]

On Sat, 16 Oct 2021 20:33:36 -0500
David Lechner <david@lechnology.com> wrote:

> This adds support to the TI eQEP counter driver for subscribing to
> overflow and underflow events using the counter chrdev interface.
> 
> Since this is the first event added, this involved adding an irq
> handler. Also, additional range checks had to be added to the ceiling
> attribute to avoid infinite interrupts.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

Hi David,

A few minor things inline. I've not commented on the new counter interface
stuff though as it's still a bit vague in my head, so over to William for that :)

Jonathan

> ---
>  drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 117 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index 09817c953f9a..b7c79435e127 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/counter.h>
> +#include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> @@ -67,6 +68,44 @@
>  #define QEPCTL_UTE		BIT(1)
>  #define QEPCTL_WDE		BIT(0)
>  
> +#define QEINT_UTO		BIT(11)
> +#define QEINT_IEL		BIT(10)
> +#define QEINT_SEL		BIT(9)
> +#define QEINT_PCM		BIT(8)
> +#define QEINT_PCR		BIT(7)
> +#define QEINT_PCO		BIT(6)
> +#define QEINT_PCU		BIT(5)
> +#define QEINT_WTO		BIT(4)
> +#define QEINT_QDC		BIT(3)
> +#define QEINT_PHE		BIT(2)
> +#define QEINT_PCE		BIT(1)
> +
> +#define QFLG_UTO		BIT(11)
> +#define QFLG_IEL		BIT(10)
> +#define QFLG_SEL		BIT(9)
> +#define QFLG_PCM		BIT(8)
> +#define QFLG_PCR		BIT(7)
> +#define QFLG_PCO		BIT(6)
> +#define QFLG_PCU		BIT(5)
> +#define QFLG_WTO		BIT(4)
> +#define QFLG_QDC		BIT(3)
> +#define QFLG_PHE		BIT(2)
> +#define QFLG_PCE		BIT(1)
> +#define QFLG_INT		BIT(0)
> +
> +#define QCLR_UTO		BIT(11)
> +#define QCLR_IEL		BIT(10)
> +#define QCLR_SEL		BIT(9)
> +#define QCLR_PCM		BIT(8)
> +#define QCLR_PCR		BIT(7)
> +#define QCLR_PCO		BIT(6)
> +#define QCLR_PCU		BIT(5)
> +#define QCLR_WTO		BIT(4)
> +#define QCLR_QDC		BIT(3)
> +#define QCLR_PHE		BIT(2)
> +#define QCLR_PCE		BIT(1)
> +#define QCLR_INT		BIT(0)
> +
>  /* EQEP Inputs */
>  enum {
>  	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
> @@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter,
>  	}
>  }
>  
> +static int ti_eqep_events_configure(struct counter_device *counter)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	struct counter_event_node *event_node;
> +	u32 qeint = 0;
> +
> +	list_for_each_entry(event_node, &counter->events_list, l) {
> +		switch (event_node->event) {
> +		case COUNTER_EVENT_OVERFLOW:
> +			qeint |= QEINT_PCO;
> +			break;
> +		case COUNTER_EVENT_UNDERFLOW:
> +			qeint |= QEINT_PCU;
> +			break;
> +		}
> +	}
> +
> +	return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint);

regmap_write() given mask is all bits.


> +}
> +
> +static int ti_eqep_watch_validate(struct counter_device *counter,
> +				  const struct counter_watch *watch)
> +{
> +	switch (watch->event) {
> +	case COUNTER_EVENT_OVERFLOW:
> +	case COUNTER_EVENT_UNDERFLOW:
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static const struct counter_ops ti_eqep_counter_ops = {
>  	.count_read	= ti_eqep_count_read,
>  	.count_write	= ti_eqep_count_write,
>  	.function_read	= ti_eqep_function_read,
>  	.function_write	= ti_eqep_function_write,
>  	.action_read	= ti_eqep_action_read,
> +	.events_configure = ti_eqep_events_configure,
> +	.watch_validate	= ti_eqep_watch_validate,
>  };
>  
>  static int ti_eqep_position_ceiling_read(struct counter_device *counter,
> @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
>  					  u64 ceiling)
>  {
>  	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qposmax = ceiling;
>  
> -	if (ceiling != (u32)ceiling)
> +	/* ensure that value fits in 32-bit register */
> +	if (qposmax != ceiling)
>  		return -ERANGE;
>  
> -	regmap_write(priv->regmap32, QPOSMAX, ceiling);
> +	/* protect against infinite overflow interrupts */
> +	if (qposmax == 0)
> +		return -EINVAL;
> +
> +	regmap_write(priv->regmap32, QPOSMAX, qposmax);
>  
>  	return 0;
>  }
> @@ -349,6 +428,25 @@ static struct counter_count ti_eqep_counts[] = {
>  	},
>  };
>  
> +static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
> +{
> +	struct ti_eqep_cnt *priv = dev_id;
> +	struct counter_device *counter = &priv->counter;
> +	u32 qflg;
> +
> +	regmap_read(priv->regmap16, QFLG, &qflg);
> +
> +	if (qflg & QFLG_PCO)
> +		counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0);
> +
> +	if (qflg & QFLG_PCU)
> +		counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0);
> +
> +	regmap_set_bits(priv->regmap16, QCLR, ~0);
Generally avoid clearing bits reflecting interrupt events you haven't handled.
I'm guessing there is a potential race in here where we read qflg and additional
events then occur before we clear.  Those events we never see because they
are unconditionally cleared by this write.

We are better off interrupting again if that race happens.

> +
> +	return IRQ_HANDLED;
> +}
> +
>  static const struct regmap_config ti_eqep_regmap32_config = {
>  	.name = "32-bit",
>  	.reg_bits = 32,
> @@ -371,6 +469,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	struct ti_eqep_cnt *priv;
>  	void __iomem *base;
>  	int err;
> +	int irq;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -390,6 +489,15 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->regmap16))
>  		return PTR_ERR(priv->regmap16);
>  
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	err = devm_request_threaded_irq(dev, irq, NULL, ti_eqep_irq_handler,
> +					IRQF_ONESHOT, dev_name(dev), priv);
> +	if (err < 0)
> +		return err;
> +
>  	priv->counter.name = dev_name(dev);
>  	priv->counter.parent = dev;
>  	priv->counter.ops = &ti_eqep_counter_ops;
> @@ -409,6 +517,13 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	pm_runtime_enable(dev);
>  	pm_runtime_get_sync(dev);
>  
> +	/*
> +	 * We can end up with an interupt infinite loop (interrupts triggered
> +	 * as soon as they are cleared) if we leave this at the default value
> +	 * of 0 and events are enabled.
> +	 */
> +	regmap_write(priv->regmap32, QPOSMAX, UINT_MAX);
> +
>  	err = counter_register(&priv->counter);
>  	if (err < 0) {
>  		pm_runtime_put_sync(dev);

Re: [PATCH 1/8] counter/ti-eqep: implement over/underflow events

[William Breathitt Gray]

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

On Sat, Oct 16, 2021 at 08:33:36PM -0500, David Lechner wrote:
> This adds support to the TI eQEP counter driver for subscribing to
> overflow and underflow events using the counter chrdev interface.
> 
> Since this is the first event added, this involved adding an irq
> handler. Also, additional range checks had to be added to the ceiling
> attribute to avoid infinite interrupts.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

Hi David,

This looks functionally okay, but I have a couple minor comments inline.

> ---
>  drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 117 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index 09817c953f9a..b7c79435e127 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/counter.h>
> +#include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> @@ -67,6 +68,44 @@
>  #define QEPCTL_UTE		BIT(1)
>  #define QEPCTL_WDE		BIT(0)
>  
> +#define QEINT_UTO		BIT(11)
> +#define QEINT_IEL		BIT(10)
> +#define QEINT_SEL		BIT(9)
> +#define QEINT_PCM		BIT(8)
> +#define QEINT_PCR		BIT(7)
> +#define QEINT_PCO		BIT(6)
> +#define QEINT_PCU		BIT(5)
> +#define QEINT_WTO		BIT(4)
> +#define QEINT_QDC		BIT(3)
> +#define QEINT_PHE		BIT(2)
> +#define QEINT_PCE		BIT(1)
> +
> +#define QFLG_UTO		BIT(11)
> +#define QFLG_IEL		BIT(10)
> +#define QFLG_SEL		BIT(9)
> +#define QFLG_PCM		BIT(8)
> +#define QFLG_PCR		BIT(7)
> +#define QFLG_PCO		BIT(6)
> +#define QFLG_PCU		BIT(5)
> +#define QFLG_WTO		BIT(4)
> +#define QFLG_QDC		BIT(3)
> +#define QFLG_PHE		BIT(2)
> +#define QFLG_PCE		BIT(1)
> +#define QFLG_INT		BIT(0)
> +
> +#define QCLR_UTO		BIT(11)
> +#define QCLR_IEL		BIT(10)
> +#define QCLR_SEL		BIT(9)
> +#define QCLR_PCM		BIT(8)
> +#define QCLR_PCR		BIT(7)
> +#define QCLR_PCO		BIT(6)
> +#define QCLR_PCU		BIT(5)
> +#define QCLR_WTO		BIT(4)
> +#define QCLR_QDC		BIT(3)
> +#define QCLR_PHE		BIT(2)
> +#define QCLR_PCE		BIT(1)
> +#define QCLR_INT		BIT(0)
> +
>  /* EQEP Inputs */
>  enum {
>  	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
> @@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter,
>  	}
>  }
>  
> +static int ti_eqep_events_configure(struct counter_device *counter)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	struct counter_event_node *event_node;
> +	u32 qeint = 0;
> +
> +	list_for_each_entry(event_node, &counter->events_list, l) {
> +		switch (event_node->event) {
> +		case COUNTER_EVENT_OVERFLOW:
> +			qeint |= QEINT_PCO;
> +			break;
> +		case COUNTER_EVENT_UNDERFLOW:
> +			qeint |= QEINT_PCU;
> +			break;
> +		}
> +	}
> +
> +	return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint);
> +}
> +
> +static int ti_eqep_watch_validate(struct counter_device *counter,
> +				  const struct counter_watch *watch)
> +{
> +	switch (watch->event) {
> +	case COUNTER_EVENT_OVERFLOW:
> +	case COUNTER_EVENT_UNDERFLOW:
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static const struct counter_ops ti_eqep_counter_ops = {
>  	.count_read	= ti_eqep_count_read,
>  	.count_write	= ti_eqep_count_write,
>  	.function_read	= ti_eqep_function_read,
>  	.function_write	= ti_eqep_function_write,
>  	.action_read	= ti_eqep_action_read,
> +	.events_configure = ti_eqep_events_configure,
> +	.watch_validate	= ti_eqep_watch_validate,
>  };
>  
>  static int ti_eqep_position_ceiling_read(struct counter_device *counter,
> @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
>  					  u64 ceiling)
>  {
>  	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qposmax = ceiling;
>  
> -	if (ceiling != (u32)ceiling)
> +	/* ensure that value fits in 32-bit register */
> +	if (qposmax != ceiling)
>  		return -ERANGE;
>  
> -	regmap_write(priv->regmap32, QPOSMAX, ceiling);
> +	/* protect against infinite overflow interrupts */
> +	if (qposmax == 0)
> +		return -EINVAL;

Would you be able to explain this scenario a bit further? My expectation
would be that an overflow event would only occur if the position
increased past the ceiling (i.e. increased to greater than 0). Of
course, running the device with a ceiling of 0 effectively guarantees
overflow eventss with every movement, but I would expect a stationary
device to sit with a position of 0 and thus no overflow events.

> +
> +	regmap_write(priv->regmap32, QPOSMAX, qposmax);
>  
>  	return 0;
>  }
> @@ -349,6 +428,25 @@ static struct counter_count ti_eqep_counts[] = {
>  	},
>  };
>  
> +static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
> +{
> +	struct ti_eqep_cnt *priv = dev_id;
> +	struct counter_device *counter = &priv->counter;
> +	u32 qflg;
> +
> +	regmap_read(priv->regmap16, QFLG, &qflg);
> +
> +	if (qflg & QFLG_PCO)
> +		counter_push_event(counter, COUNTER_EVENT_OVERFLOW, 0);
> +
> +	if (qflg & QFLG_PCU)
> +		counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0);
> +
> +	regmap_set_bits(priv->regmap16, QCLR, ~0);

I agree with Jonathan here, it looks like this is unconditionally
clearing all interrupt flags but it's possible new ones could have
appear between the time after you read QFLG to here. Is it possible to
clear just the interrupt flags you've handled here instead of all?

William Breathitt Gray

> +
> +	return IRQ_HANDLED;
> +}
> +
>  static const struct regmap_config ti_eqep_regmap32_config = {
>  	.name = "32-bit",
>  	.reg_bits = 32,
> @@ -371,6 +469,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	struct ti_eqep_cnt *priv;
>  	void __iomem *base;
>  	int err;
> +	int irq;
>  
>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -390,6 +489,15 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	if (IS_ERR(priv->regmap16))
>  		return PTR_ERR(priv->regmap16);
>  
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	err = devm_request_threaded_irq(dev, irq, NULL, ti_eqep_irq_handler,
> +					IRQF_ONESHOT, dev_name(dev), priv);
> +	if (err < 0)
> +		return err;
> +
>  	priv->counter.name = dev_name(dev);
>  	priv->counter.parent = dev;
>  	priv->counter.ops = &ti_eqep_counter_ops;
> @@ -409,6 +517,13 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	pm_runtime_enable(dev);
>  	pm_runtime_get_sync(dev);
>  
> +	/*
> +	 * We can end up with an interupt infinite loop (interrupts triggered
> +	 * as soon as they are cleared) if we leave this at the default value
> +	 * of 0 and events are enabled.
> +	 */
> +	regmap_write(priv->regmap32, QPOSMAX, UINT_MAX);
> +
>  	err = counter_register(&priv->counter);
>  	if (err < 0) {
>  		pm_runtime_put_sync(dev);
> -- 
> 2.25.1
> 

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

Re: [PATCH 1/8] counter/ti-eqep: implement over/underflow events

[David Lechner]

On 10/25/21 2:13 AM, William Breathitt Gray wrote:
> On Sat, Oct 16, 2021 at 08:33:36PM -0500, David Lechner wrote:
>> This adds support to the TI eQEP counter driver for subscribing to
>> overflow and underflow events using the counter chrdev interface.
>>
>> Since this is the first event added, this involved adding an irq
>> handler. Also, additional range checks had to be added to the ceiling
>> attribute to avoid infinite interrupts.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
> 
> Hi David,
> 
> This looks functionally okay, but I have a couple minor comments inline.
> 
>> ---
>>   drivers/counter/ti-eqep.c | 119 +++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 117 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
>> index 09817c953f9a..b7c79435e127 100644
>> --- a/drivers/counter/ti-eqep.c
>> +++ b/drivers/counter/ti-eqep.c
>> @@ -7,6 +7,7 @@
>>   
>>   #include <linux/bitops.h>
>>   #include <linux/counter.h>
>> +#include <linux/interrupt.h>
>>   #include <linux/kernel.h>
>>   #include <linux/mod_devicetable.h>
>>   #include <linux/module.h>
>> @@ -67,6 +68,44 @@
>>   #define QEPCTL_UTE		BIT(1)
>>   #define QEPCTL_WDE		BIT(0)
>>   
>> +#define QEINT_UTO		BIT(11)
>> +#define QEINT_IEL		BIT(10)
>> +#define QEINT_SEL		BIT(9)
>> +#define QEINT_PCM		BIT(8)
>> +#define QEINT_PCR		BIT(7)
>> +#define QEINT_PCO		BIT(6)
>> +#define QEINT_PCU		BIT(5)
>> +#define QEINT_WTO		BIT(4)
>> +#define QEINT_QDC		BIT(3)
>> +#define QEINT_PHE		BIT(2)
>> +#define QEINT_PCE		BIT(1)
>> +
>> +#define QFLG_UTO		BIT(11)
>> +#define QFLG_IEL		BIT(10)
>> +#define QFLG_SEL		BIT(9)
>> +#define QFLG_PCM		BIT(8)
>> +#define QFLG_PCR		BIT(7)
>> +#define QFLG_PCO		BIT(6)
>> +#define QFLG_PCU		BIT(5)
>> +#define QFLG_WTO		BIT(4)
>> +#define QFLG_QDC		BIT(3)
>> +#define QFLG_PHE		BIT(2)
>> +#define QFLG_PCE		BIT(1)
>> +#define QFLG_INT		BIT(0)
>> +
>> +#define QCLR_UTO		BIT(11)
>> +#define QCLR_IEL		BIT(10)
>> +#define QCLR_SEL		BIT(9)
>> +#define QCLR_PCM		BIT(8)
>> +#define QCLR_PCR		BIT(7)
>> +#define QCLR_PCO		BIT(6)
>> +#define QCLR_PCU		BIT(5)
>> +#define QCLR_WTO		BIT(4)
>> +#define QCLR_QDC		BIT(3)
>> +#define QCLR_PHE		BIT(2)
>> +#define QCLR_PCE		BIT(1)
>> +#define QCLR_INT		BIT(0)
>> +
>>   /* EQEP Inputs */
>>   enum {
>>   	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
>> @@ -233,12 +272,46 @@ static int ti_eqep_action_read(struct counter_device *counter,
>>   	}
>>   }
>>   
>> +static int ti_eqep_events_configure(struct counter_device *counter)
>> +{
>> +	struct ti_eqep_cnt *priv = counter->priv;
>> +	struct counter_event_node *event_node;
>> +	u32 qeint = 0;
>> +
>> +	list_for_each_entry(event_node, &counter->events_list, l) {
>> +		switch (event_node->event) {
>> +		case COUNTER_EVENT_OVERFLOW:
>> +			qeint |= QEINT_PCO;
>> +			break;
>> +		case COUNTER_EVENT_UNDERFLOW:
>> +			qeint |= QEINT_PCU;
>> +			break;
>> +		}
>> +	}
>> +
>> +	return regmap_write_bits(priv->regmap16, QEINT, ~0, qeint);
>> +}
>> +
>> +static int ti_eqep_watch_validate(struct counter_device *counter,
>> +				  const struct counter_watch *watch)
>> +{
>> +	switch (watch->event) {
>> +	case COUNTER_EVENT_OVERFLOW:
>> +	case COUNTER_EVENT_UNDERFLOW:
>> +		return 0;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>>   static const struct counter_ops ti_eqep_counter_ops = {
>>   	.count_read	= ti_eqep_count_read,
>>   	.count_write	= ti_eqep_count_write,
>>   	.function_read	= ti_eqep_function_read,
>>   	.function_write	= ti_eqep_function_write,
>>   	.action_read	= ti_eqep_action_read,
>> +	.events_configure = ti_eqep_events_configure,
>> +	.watch_validate	= ti_eqep_watch_validate,
>>   };
>>   
>>   static int ti_eqep_position_ceiling_read(struct counter_device *counter,
>> @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
>>   					  u64 ceiling)
>>   {
>>   	struct ti_eqep_cnt *priv = counter->priv;
>> +	u32 qposmax = ceiling;
>>   
>> -	if (ceiling != (u32)ceiling)
>> +	/* ensure that value fits in 32-bit register */
>> +	if (qposmax != ceiling)
>>   		return -ERANGE;
>>   
>> -	regmap_write(priv->regmap32, QPOSMAX, ceiling);
>> +	/* protect against infinite overflow interrupts */
>> +	if (qposmax == 0)
>> +		return -EINVAL;
> 
> Would you be able to explain this scenario a bit further? My expectation
> would be that an overflow event would only occur if the position
> increased past the ceiling (i.e. increased to greater than 0). Of
> course, running the device with a ceiling of 0 effectively guarantees
> overflow eventss with every movement, but I would expect a stationary
> device to sit with a position of 0 and thus no overflow events.
> 

This is just the way the hardware works. I discovered this the first
time I enabled interrupts. Even if you clear the interrupt, it is
triggered again immediately when QPOSMAX == 0.

Re: [PATCH 1/8] counter/ti-eqep: implement over/underflow events

[William Breathitt Gray]

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

On Wed, Oct 27, 2021 at 10:23:13AM -0500, David Lechner wrote:
> On 10/25/21 2:13 AM, William Breathitt Gray wrote:
> > On Sat, Oct 16, 2021 at 08:33:36PM -0500, David Lechner wrote:
> >> @@ -260,11 +333,17 @@ static int ti_eqep_position_ceiling_write(struct counter_device *counter,
> >>   					  u64 ceiling)
> >>   {
> >>   	struct ti_eqep_cnt *priv = counter->priv;
> >> +	u32 qposmax = ceiling;
> >>   
> >> -	if (ceiling != (u32)ceiling)
> >> +	/* ensure that value fits in 32-bit register */
> >> +	if (qposmax != ceiling)
> >>   		return -ERANGE;
> >>   
> >> -	regmap_write(priv->regmap32, QPOSMAX, ceiling);
> >> +	/* protect against infinite overflow interrupts */
> >> +	if (qposmax == 0)
> >> +		return -EINVAL;
> > 
> > Would you be able to explain this scenario a bit further? My expectation
> > would be that an overflow event would only occur if the position
> > increased past the ceiling (i.e. increased to greater than 0). Of
> > course, running the device with a ceiling of 0 effectively guarantees
> > overflow eventss with every movement, but I would expect a stationary
> > device to sit with a position of 0 and thus no overflow events.
> > 
> 
> This is just the way the hardware works. I discovered this the first
> time I enabled interrupts. Even if you clear the interrupt, it is
> triggered again immediately when QPOSMAX == 0.

For this device, does an overflow event occur once the count value
increases to equal the ceiling value, or once the count value increases
past the ceiling value?

The Counter interface defines ceiling as an inclusive upper limit (count
value is capable of reaching ceiling) and defines COUNTER_EVENT_OVERFLOW
as occuring when the count value increases past ceiling. I want to make
sure the ceiling extension and COUNTER_EVENT_OVERFLOW events for this
driver are behaving as expected of the Counter interface.

Let's use a non-zero example to be clear. Suppose we set ceiling equal
to 10. If count is currently at 9 and increases by 1, count should
become 10 and no COUNTER_EVENT_OVERFLOW event is expected to trigger; if
count is 10 and further increases, count should _not_ become 11 (staying
at 10 or starting over at the floor) but a COUNTER_EVENT_OVERFLOW event
does trigger. Does the driver behave like this currently?

William Breathitt Gray

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

[PATCH 2/8] counter/ti-eqep: add support for direction

[David Lechner]

This adds support for direction to the TI eQEP counter driver. It adds
both a direction attribute to sysfs and a direction change event to
the chrdev. The direction change event type is new public API.

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/counter/ti-eqep.c    | 33 +++++++++++++++++++++++++++++++++
 include/uapi/linux/counter.h |  2 ++
 2 files changed, 35 insertions(+)

diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index b7c79435e127..9881e5115da6 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -106,6 +106,15 @@
 #define QCLR_PCE		BIT(1)
 #define QCLR_INT		BIT(0)
 
+#define QEPSTS_UPEVNT		BIT(7)
+#define QEPSTS_FDF		BIT(6)
+#define QEPSTS_QDF		BIT(5)
+#define QEPSTS_QDLF		BIT(4)
+#define QEPSTS_COEF		BIT(3)
+#define QEPSTS_CDEF		BIT(2)
+#define QEPSTS_FIMF		BIT(1)
+#define QEPSTS_PCEF		BIT(0)
+
 /* EQEP Inputs */
 enum {
 	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
@@ -286,6 +295,9 @@ static int ti_eqep_events_configure(struct counter_device *counter)
 		case COUNTER_EVENT_UNDERFLOW:
 			qeint |= QEINT_PCU;
 			break;
+		case COUNTER_EVENT_DIRECTION_CHANGE:
+			qeint |= QEINT_QDC;
+			break;
 		}
 	}
 
@@ -298,6 +310,7 @@ static int ti_eqep_watch_validate(struct counter_device *counter,
 	switch (watch->event) {
 	case COUNTER_EVENT_OVERFLOW:
 	case COUNTER_EVENT_UNDERFLOW:
+	case COUNTER_EVENT_DIRECTION_CHANGE:
 		return 0;
 	default:
 		return -EINVAL;
@@ -371,11 +384,27 @@ static int ti_eqep_position_enable_write(struct counter_device *counter,
 	return 0;
 }
 
+static int ti_eqep_direction_read(struct counter_device *counter,
+				  struct counter_count *count,
+				  enum counter_count_direction *direction)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qepsts;
+
+	regmap_read(priv->regmap16, QEPSTS, &qepsts);
+
+	*direction = (qepsts & QEPSTS_QDF) ? COUNTER_COUNT_DIRECTION_FORWARD
+					   : COUNTER_COUNT_DIRECTION_BACKWARD;
+
+	return 0;
+}
+
 static struct counter_comp ti_eqep_position_ext[] = {
 	COUNTER_COMP_CEILING(ti_eqep_position_ceiling_read,
 			     ti_eqep_position_ceiling_write),
 	COUNTER_COMP_ENABLE(ti_eqep_position_enable_read,
 			    ti_eqep_position_enable_write),
+	COUNTER_COMP_DIRECTION(ti_eqep_direction_read),
 };
 
 static struct counter_signal ti_eqep_signals[] = {
@@ -442,6 +471,10 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
 	if (qflg & QFLG_PCU)
 		counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0);
 
+	if (qflg & QFLG_QDC)
+		counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0);
+
+
 	regmap_set_bits(priv->regmap16, QCLR, ~0);
 
 	return IRQ_HANDLED;
diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
index d0aa95aeff7b..36dd3b474d09 100644
--- a/include/uapi/linux/counter.h
+++ b/include/uapi/linux/counter.h
@@ -61,6 +61,8 @@ enum counter_event_type {
 	COUNTER_EVENT_THRESHOLD,
 	/* Index signal detected */
 	COUNTER_EVENT_INDEX,
+	/* Direction change detected */
+	COUNTER_EVENT_DIRECTION_CHANGE,
 };
 
 /**
-- 
2.25.1

Re: [PATCH 2/8] counter/ti-eqep: add support for direction

[Jonathan Cameron]

On Sat, 16 Oct 2021 20:33:37 -0500
David Lechner <david@lechnology.com> wrote:

> This adds support for direction to the TI eQEP counter driver. It adds
> both a direction attribute to sysfs and a direction change event to
> the chrdev. The direction change event type is new public API.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

Trivial comment inline.

> ---
>  drivers/counter/ti-eqep.c    | 33 +++++++++++++++++++++++++++++++++
>  include/uapi/linux/counter.h |  2 ++
>  2 files changed, 35 insertions(+)
> 

...

>  static struct counter_signal ti_eqep_signals[] = {
> @@ -442,6 +471,10 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
>  	if (qflg & QFLG_PCU)
>  		counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0);
>  
> +	if (qflg & QFLG_QDC)
> +		counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0);
> +

Nitpick. One blank line is enough here.

> +
>  	regmap_set_bits(priv->regmap16, QCLR, ~0);
>  
>  	return IRQ_HANDLED;
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> index d0aa95aeff7b..36dd3b474d09 100644
> --- a/include/uapi/linux/counter.h
> +++ b/include/uapi/linux/counter.h
> @@ -61,6 +61,8 @@ enum counter_event_type {
>  	COUNTER_EVENT_THRESHOLD,
>  	/* Index signal detected */
>  	COUNTER_EVENT_INDEX,
> +	/* Direction change detected */
> +	COUNTER_EVENT_DIRECTION_CHANGE,
>  };
>  
>  /**

Re: [PATCH 2/8] counter/ti-eqep: add support for direction

[William Breathitt Gray]

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

On Sat, Oct 16, 2021 at 08:33:37PM -0500, David Lechner wrote:
> This adds support for direction to the TI eQEP counter driver. It adds
> both a direction attribute to sysfs and a direction change event to
> the chrdev. The direction change event type is new public API.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

Just one minor comment below regarding the IRQ handler; the rest of the
patch is fine.

> ---
>  drivers/counter/ti-eqep.c    | 33 +++++++++++++++++++++++++++++++++
>  include/uapi/linux/counter.h |  2 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index b7c79435e127..9881e5115da6 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -106,6 +106,15 @@
>  #define QCLR_PCE		BIT(1)
>  #define QCLR_INT		BIT(0)
>  
> +#define QEPSTS_UPEVNT		BIT(7)
> +#define QEPSTS_FDF		BIT(6)
> +#define QEPSTS_QDF		BIT(5)
> +#define QEPSTS_QDLF		BIT(4)
> +#define QEPSTS_COEF		BIT(3)
> +#define QEPSTS_CDEF		BIT(2)
> +#define QEPSTS_FIMF		BIT(1)
> +#define QEPSTS_PCEF		BIT(0)
> +
>  /* EQEP Inputs */
>  enum {
>  	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
> @@ -286,6 +295,9 @@ static int ti_eqep_events_configure(struct counter_device *counter)
>  		case COUNTER_EVENT_UNDERFLOW:
>  			qeint |= QEINT_PCU;
>  			break;
> +		case COUNTER_EVENT_DIRECTION_CHANGE:
> +			qeint |= QEINT_QDC;
> +			break;
>  		}
>  	}
>  
> @@ -298,6 +310,7 @@ static int ti_eqep_watch_validate(struct counter_device *counter,
>  	switch (watch->event) {
>  	case COUNTER_EVENT_OVERFLOW:
>  	case COUNTER_EVENT_UNDERFLOW:
> +	case COUNTER_EVENT_DIRECTION_CHANGE:
>  		return 0;
>  	default:
>  		return -EINVAL;
> @@ -371,11 +384,27 @@ static int ti_eqep_position_enable_write(struct counter_device *counter,
>  	return 0;
>  }
>  
> +static int ti_eqep_direction_read(struct counter_device *counter,
> +				  struct counter_count *count,
> +				  enum counter_count_direction *direction)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qepsts;
> +
> +	regmap_read(priv->regmap16, QEPSTS, &qepsts);
> +
> +	*direction = (qepsts & QEPSTS_QDF) ? COUNTER_COUNT_DIRECTION_FORWARD
> +					   : COUNTER_COUNT_DIRECTION_BACKWARD;
> +
> +	return 0;
> +}
> +
>  static struct counter_comp ti_eqep_position_ext[] = {
>  	COUNTER_COMP_CEILING(ti_eqep_position_ceiling_read,
>  			     ti_eqep_position_ceiling_write),
>  	COUNTER_COMP_ENABLE(ti_eqep_position_enable_read,
>  			    ti_eqep_position_enable_write),
> +	COUNTER_COMP_DIRECTION(ti_eqep_direction_read),
>  };
>  
>  static struct counter_signal ti_eqep_signals[] = {
> @@ -442,6 +471,10 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
>  	if (qflg & QFLG_PCU)
>  		counter_push_event(counter, COUNTER_EVENT_UNDERFLOW, 0);
>  
> +	if (qflg & QFLG_QDC)
> +		counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0);
> +
> +
>  	regmap_set_bits(priv->regmap16, QCLR, ~0);

As mentioned in the previous patch comments, you should try if possible
to clear only the interrupt flags for the events that you're actually
handling here.

William Breathitt Gray

>  
>  	return IRQ_HANDLED;
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> index d0aa95aeff7b..36dd3b474d09 100644
> --- a/include/uapi/linux/counter.h
> +++ b/include/uapi/linux/counter.h
> @@ -61,6 +61,8 @@ enum counter_event_type {
>  	COUNTER_EVENT_THRESHOLD,
>  	/* Index signal detected */
>  	COUNTER_EVENT_INDEX,
> +	/* Direction change detected */
> +	COUNTER_EVENT_DIRECTION_CHANGE,
>  };
>  
>  /**
> -- 
> 2.25.1
> 

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

[PATCH 3/8] counter/ti-eqep: add support for unit timer

[David Lechner]

This adds support to the TI eQEP counter driver for the Unit Timer.
The Unit Timer is a device-level extension that provides a timer to be
used for speed calculations. The sysfs interface for the Unit Timer is
new and will be documented in a later commit. It contains a R/W time
attribute for the current time, a R/W period attribute for the timeout
period and a R/W enable attribute to start/stop the timer. It also
implements a timeout event on the chrdev interface that is triggered
each time the period timeout is reached.

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/counter/ti-eqep.c    | 132 ++++++++++++++++++++++++++++++++++-
 include/uapi/linux/counter.h |   2 +
 2 files changed, 133 insertions(+), 1 deletion(-)

diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index 9881e5115da6..a4a5a4486329 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/clk.h>
 #include <linux/counter.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -131,6 +132,7 @@ enum ti_eqep_count_func {
 
 struct ti_eqep_cnt {
 	struct counter_device counter;
+	unsigned long sysclkout_rate;
 	struct regmap *regmap32;
 	struct regmap *regmap16;
 };
@@ -298,6 +300,9 @@ static int ti_eqep_events_configure(struct counter_device *counter)
 		case COUNTER_EVENT_DIRECTION_CHANGE:
 			qeint |= QEINT_QDC;
 			break;
+		case COUNTER_EVENT_TIMEOUT:
+			qeint |= QEINT_UTO;
+			break;
 		}
 	}
 
@@ -311,6 +316,7 @@ static int ti_eqep_watch_validate(struct counter_device *counter,
 	case COUNTER_EVENT_OVERFLOW:
 	case COUNTER_EVENT_UNDERFLOW:
 	case COUNTER_EVENT_DIRECTION_CHANGE:
+	case COUNTER_EVENT_TIMEOUT:
 		return 0;
 	default:
 		return -EINVAL;
@@ -457,6 +463,106 @@ static struct counter_count ti_eqep_counts[] = {
 	},
 };
 
+static int ti_eqep_unit_timer_time_read(struct counter_device *counter,
+				       u64 *value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qutmr;
+
+	regmap_read(priv->regmap32, QUTMR, &qutmr);
+
+	/* convert timer ticks to nanoseconds */
+	*value = mul_u64_u32_div(qutmr, NSEC_PER_SEC, priv->sysclkout_rate);
+
+	return 0;
+}
+
+static int ti_eqep_unit_timer_time_write(struct counter_device *counter,
+					u64 value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qutmr;
+
+	/* convert nanoseconds to timer ticks */
+	qutmr = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
+	if (qutmr != value)
+		return -ERANGE;
+
+	regmap_write(priv->regmap32, QUTMR, qutmr);
+
+	return 0;
+}
+
+static int ti_eqep_unit_timer_period_read(struct counter_device *counter,
+					  u64 *value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 quprd;
+
+	regmap_read(priv->regmap32, QUPRD, &quprd);
+
+	/* convert timer ticks to nanoseconds */
+	*value = mul_u64_u32_div(quprd, NSEC_PER_SEC, priv->sysclkout_rate);
+
+	return 0;
+}
+
+static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
+					   u64 value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 quprd;
+
+	/* convert nanoseconds to timer ticks */
+	quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
+	if (quprd != value)
+		return -ERANGE;
+
+	/* protect against infinite unit timeout interrupts */
+	if (quprd == 0)
+		return -EINVAL;
+
+	regmap_write(priv->regmap32, QUPRD, quprd);
+
+	return 0;
+}
+
+static int ti_eqep_unit_timer_enable_read(struct counter_device *counter,
+					  u8 *value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qepctl;
+
+	regmap_read(priv->regmap16, QEPCTL, &qepctl);
+	*value = !!(qepctl & QEPCTL_UTE);
+
+	return 0;
+}
+
+static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
+					   u8 value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+
+	if (value)
+		regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_UTE);
+	else
+		regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_UTE);
+
+	return 0;
+}
+
+static struct counter_comp ti_eqep_device_ext[] = {
+	COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,
+				ti_eqep_unit_timer_time_write),
+	COUNTER_COMP_DEVICE_U64("unit_timer_period",
+				ti_eqep_unit_timer_period_read,
+				ti_eqep_unit_timer_period_write),
+	COUNTER_COMP_DEVICE_BOOL("unit_timer_enable",
+				 ti_eqep_unit_timer_enable_read,
+				 ti_eqep_unit_timer_enable_write),
+};
+
 static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
 {
 	struct ti_eqep_cnt *priv = dev_id;
@@ -474,6 +580,8 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
 	if (qflg & QFLG_QDC)
 		counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0);
 
+	if (qflg & QFLG_UTO)
+		counter_push_event(counter, COUNTER_EVENT_TIMEOUT, 0);
 
 	regmap_set_bits(priv->regmap16, QCLR, ~0);
 
@@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct ti_eqep_cnt *priv;
+	struct clk *clk;
 	void __iomem *base;
 	int err;
 	int irq;
@@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev)
 	if (!priv)
 		return -ENOMEM;
 
+	clk = devm_clk_get(dev, "sysclkout");
+	if (IS_ERR(clk)) {
+		if (PTR_ERR(clk) != -EPROBE_DEFER)
+			dev_err(dev, "failed to get sysclkout");
+		return PTR_ERR(clk);
+	}
+
+	priv->sysclkout_rate = clk_get_rate(clk);
+	if (priv->sysclkout_rate == 0) {
+		dev_err(dev, "failed to get sysclkout rate");
+		/* prevent divide by zero */
+		priv->sysclkout_rate = 1;
+		/*
+		 * This error is not expected and the driver is mostly usable
+		 * without clock rate anyway, so don't exit here.
+		 */
+	}
+
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
@@ -536,6 +663,8 @@ static int ti_eqep_probe(struct platform_device *pdev)
 	priv->counter.ops = &ti_eqep_counter_ops;
 	priv->counter.counts = ti_eqep_counts;
 	priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts);
+	priv->counter.ext = ti_eqep_device_ext;
+	priv->counter.num_ext = ARRAY_SIZE(ti_eqep_device_ext);
 	priv->counter.signals = ti_eqep_signals;
 	priv->counter.num_signals = ARRAY_SIZE(ti_eqep_signals);
 	priv->counter.priv = priv;
@@ -552,10 +681,11 @@ static int ti_eqep_probe(struct platform_device *pdev)
 
 	/*
 	 * We can end up with an interupt infinite loop (interrupts triggered
-	 * as soon as they are cleared) if we leave this at the default value
+	 * as soon as they are cleared) if we leave these at the default value
 	 * of 0 and events are enabled.
 	 */
 	regmap_write(priv->regmap32, QPOSMAX, UINT_MAX);
+	regmap_write(priv->regmap32, QUPRD, UINT_MAX);
 
 	err = counter_register(&priv->counter);
 	if (err < 0) {
diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
index 36dd3b474d09..640d9719b88c 100644
--- a/include/uapi/linux/counter.h
+++ b/include/uapi/linux/counter.h
@@ -63,6 +63,8 @@ enum counter_event_type {
 	COUNTER_EVENT_INDEX,
 	/* Direction change detected */
 	COUNTER_EVENT_DIRECTION_CHANGE,
+	/* Timer exceeded timeout */
+	COUNTER_EVENT_TIMEOUT,
 };
 
 /**
-- 
2.25.1

Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer

[Jonathan Cameron]

On Sat, 16 Oct 2021 20:33:38 -0500
David Lechner <david@lechnology.com> wrote:

> This adds support to the TI eQEP counter driver for the Unit Timer.
> The Unit Timer is a device-level extension that provides a timer to be
> used for speed calculations. The sysfs interface for the Unit Timer is
> new and will be documented in a later commit. It contains a R/W time
> attribute for the current time, a R/W period attribute for the timeout
> period and a R/W enable attribute to start/stop the timer. It also
> implements a timeout event on the chrdev interface that is triggered
> each time the period timeout is reached.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

No comments on the interface in here as leaving that for William / later.

A few minor comments on the implementation.

Thanks,

Jonathan

> ---
>  drivers/counter/ti-eqep.c    | 132 ++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/counter.h |   2 +
>  2 files changed, 133 insertions(+), 1 deletion(-)

...

> +static int ti_eqep_unit_timer_time_write(struct counter_device *counter,
> +					u64 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qutmr;
> +
> +	/* convert nanoseconds to timer ticks */
> +	qutmr = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);

Hmm. This pattern strikes me as 'too clever' and also likely to trip up static
checkers who will moan about the truncation if they don't understand this trick.

I think I'd prefer you just put the answer in an u64 and then do a simple bounds
check before casting down.

> +	if (qutmr != value)
> +		return -ERANGE;
> +
> +	regmap_write(priv->regmap32, QUTMR, qutmr);
> +
> +	return 0;
> +}
> +

...

>  static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
>  {
>  	struct ti_eqep_cnt *priv = dev_id;
> @@ -474,6 +580,8 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
>  	if (qflg & QFLG_QDC)
>  		counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0);
>  
> +	if (qflg & QFLG_UTO)
> +		counter_push_event(counter, COUNTER_EVENT_TIMEOUT, 0);
>  
>  	regmap_set_bits(priv->regmap16, QCLR, ~0);
>  
> @@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct ti_eqep_cnt *priv;
> +	struct clk *clk;
>  	void __iomem *base;
>  	int err;
>  	int irq;
> @@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	clk = devm_clk_get(dev, "sysclkout");
> +	if (IS_ERR(clk)) {
> +		if (PTR_ERR(clk) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get sysclkout");

dev_err_probe() which both removes most of this boilerplate
and stashes the reason for the deferred probe such that it can be checked when
debugging.

> +		return PTR_ERR(clk);
> +	}

No need to enable the clock?

> +
> +	priv->sysclkout_rate = clk_get_rate(clk);
> +	if (priv->sysclkout_rate == 0) {
> +		dev_err(dev, "failed to get sysclkout rate");
> +		/* prevent divide by zero */
> +		priv->sysclkout_rate = 1;
> +		/*
> +		 * This error is not expected and the driver is mostly usable
> +		 * without clock rate anyway, so don't exit here.
> +		 */
> +	}
> +
>  
>  /**

Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer

[William Breathitt Gray]

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

On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
> This adds support to the TI eQEP counter driver for the Unit Timer.
> The Unit Timer is a device-level extension that provides a timer to be
> used for speed calculations. The sysfs interface for the Unit Timer is
> new and will be documented in a later commit. It contains a R/W time
> attribute for the current time, a R/W period attribute for the timeout
> period and a R/W enable attribute to start/stop the timer. It also
> implements a timeout event on the chrdev interface that is triggered
> each time the period timeout is reached.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

I'll comment on the sysfs interface in the respective docs patch. Some
comments regarding this patch below.

> ---
>  drivers/counter/ti-eqep.c    | 132 ++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/counter.h |   2 +
>  2 files changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index 9881e5115da6..a4a5a4486329 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/clk.h>
>  #include <linux/counter.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -131,6 +132,7 @@ enum ti_eqep_count_func {
>  
>  struct ti_eqep_cnt {
>  	struct counter_device counter;
> +	unsigned long sysclkout_rate;
>  	struct regmap *regmap32;
>  	struct regmap *regmap16;
>  };
> @@ -298,6 +300,9 @@ static int ti_eqep_events_configure(struct counter_device *counter)
>  		case COUNTER_EVENT_DIRECTION_CHANGE:
>  			qeint |= QEINT_QDC;
>  			break;
> +		case COUNTER_EVENT_TIMEOUT:
> +			qeint |= QEINT_UTO;
> +			break;
>  		}
>  	}
>  
> @@ -311,6 +316,7 @@ static int ti_eqep_watch_validate(struct counter_device *counter,
>  	case COUNTER_EVENT_OVERFLOW:
>  	case COUNTER_EVENT_UNDERFLOW:
>  	case COUNTER_EVENT_DIRECTION_CHANGE:
> +	case COUNTER_EVENT_TIMEOUT:
>  		return 0;
>  	default:
>  		return -EINVAL;
> @@ -457,6 +463,106 @@ static struct counter_count ti_eqep_counts[] = {
>  	},
>  };
>  
> +static int ti_eqep_unit_timer_time_read(struct counter_device *counter,
> +				       u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qutmr;
> +
> +	regmap_read(priv->regmap32, QUTMR, &qutmr);
> +
> +	/* convert timer ticks to nanoseconds */
> +	*value = mul_u64_u32_div(qutmr, NSEC_PER_SEC, priv->sysclkout_rate);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_unit_timer_time_write(struct counter_device *counter,
> +					u64 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qutmr;
> +
> +	/* convert nanoseconds to timer ticks */
> +	qutmr = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> +	if (qutmr != value)
> +		return -ERANGE;
> +
> +	regmap_write(priv->regmap32, QUTMR, qutmr);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_unit_timer_period_read(struct counter_device *counter,
> +					  u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 quprd;
> +
> +	regmap_read(priv->regmap32, QUPRD, &quprd);
> +
> +	/* convert timer ticks to nanoseconds */
> +	*value = mul_u64_u32_div(quprd, NSEC_PER_SEC, priv->sysclkout_rate);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
> +					   u64 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 quprd;
> +
> +	/* convert nanoseconds to timer ticks */
> +	quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> +	if (quprd != value)
> +		return -ERANGE;
> +
> +	/* protect against infinite unit timeout interrupts */
> +	if (quprd == 0)
> +		return -EINVAL;

I doubt there's any practical reason for a user to set the timer period
to 0, but perhaps we should not try to protect users from themselves
here. It's very obvious and expected that setting the timer period to 0
results in timeouts as quickly as possible, so really the user should be
left to reap the fruits of their decision regardless of how asinine that
decision is.

> +
> +	regmap_write(priv->regmap32, QUPRD, quprd);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_unit_timer_enable_read(struct counter_device *counter,
> +					  u8 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qepctl;
> +
> +	regmap_read(priv->regmap16, QEPCTL, &qepctl);
> +	*value = !!(qepctl & QEPCTL_UTE);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
> +					   u8 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	if (value)
> +		regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_UTE);
> +	else
> +		regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_UTE);
> +
> +	return 0;
> +}
> +
> +static struct counter_comp ti_eqep_device_ext[] = {
> +	COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,
> +				ti_eqep_unit_timer_time_write),
> +	COUNTER_COMP_DEVICE_U64("unit_timer_period",
> +				ti_eqep_unit_timer_period_read,
> +				ti_eqep_unit_timer_period_write),
> +	COUNTER_COMP_DEVICE_BOOL("unit_timer_enable",
> +				 ti_eqep_unit_timer_enable_read,
> +				 ti_eqep_unit_timer_enable_write),
> +};
> +
>  static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
>  {
>  	struct ti_eqep_cnt *priv = dev_id;
> @@ -474,6 +580,8 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id)
>  	if (qflg & QFLG_QDC)
>  		counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0);
>  
> +	if (qflg & QFLG_UTO)
> +		counter_push_event(counter, COUNTER_EVENT_TIMEOUT, 0);
>  
>  	regmap_set_bits(priv->regmap16, QCLR, ~0);

Same comment here as the previous patches about clearing all interrupt
flags.

>  
> @@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct ti_eqep_cnt *priv;
> +	struct clk *clk;
>  	void __iomem *base;
>  	int err;
>  	int irq;
> @@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	clk = devm_clk_get(dev, "sysclkout");
> +	if (IS_ERR(clk)) {
> +		if (PTR_ERR(clk) != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get sysclkout");
> +		return PTR_ERR(clk);
> +	}
> +
> +	priv->sysclkout_rate = clk_get_rate(clk);
> +	if (priv->sysclkout_rate == 0) {
> +		dev_err(dev, "failed to get sysclkout rate");
> +		/* prevent divide by zero */
> +		priv->sysclkout_rate = 1;
> +		/*
> +		 * This error is not expected and the driver is mostly usable
> +		 * without clock rate anyway, so don't exit here.
> +		 */

If the values for these new attributes are expected to be denominated in
nanoseconds then we must guarantee that. You should certainly error out
here if you can't guarantee such.

Alternatively, you could provide an additional set of attributes that
are denominated in units of raw timer ticks rather than nanoseconds.
That way if you can't determine the clock rate you can simply have the
nanosecond-denominated timer attributes return an EOPNOTSUPP error code
or similar while still providing users with the raw timer ticks
attributes.

William Breathitt Gray

> +	}
> +
>  	base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
> @@ -536,6 +663,8 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	priv->counter.ops = &ti_eqep_counter_ops;
>  	priv->counter.counts = ti_eqep_counts;
>  	priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts);
> +	priv->counter.ext = ti_eqep_device_ext;
> +	priv->counter.num_ext = ARRAY_SIZE(ti_eqep_device_ext);
>  	priv->counter.signals = ti_eqep_signals;
>  	priv->counter.num_signals = ARRAY_SIZE(ti_eqep_signals);
>  	priv->counter.priv = priv;
> @@ -552,10 +681,11 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  
>  	/*
>  	 * We can end up with an interupt infinite loop (interrupts triggered
> -	 * as soon as they are cleared) if we leave this at the default value
> +	 * as soon as they are cleared) if we leave these at the default value
>  	 * of 0 and events are enabled.
>  	 */
>  	regmap_write(priv->regmap32, QPOSMAX, UINT_MAX);
> +	regmap_write(priv->regmap32, QUPRD, UINT_MAX);
>  
>  	err = counter_register(&priv->counter);
>  	if (err < 0) {
> diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h
> index 36dd3b474d09..640d9719b88c 100644
> --- a/include/uapi/linux/counter.h
> +++ b/include/uapi/linux/counter.h
> @@ -63,6 +63,8 @@ enum counter_event_type {
>  	COUNTER_EVENT_INDEX,
>  	/* Direction change detected */
>  	COUNTER_EVENT_DIRECTION_CHANGE,
> +	/* Timer exceeded timeout */
> +	COUNTER_EVENT_TIMEOUT,
>  };
>  
>  /**
> -- 
> 2.25.1
> 

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

Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer

[David Lechner]

On 10/25/21 3:48 AM, William Breathitt Gray wrote:
> On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
>> This adds support to the TI eQEP counter driver for the Unit Timer.
>> The Unit Timer is a device-level extension that provides a timer to be
>> used for speed calculations. The sysfs interface for the Unit Timer is
>> new and will be documented in a later commit. It contains a R/W time
>> attribute for the current time, a R/W period attribute for the timeout
>> period and a R/W enable attribute to start/stop the timer. It also
>> implements a timeout event on the chrdev interface that is triggered
>> each time the period timeout is reached.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
> 
> I'll comment on the sysfs interface in the respective docs patch. Some
> comments regarding this patch below.
> 

...

>> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
>> +					   u64 value)
>> +{
>> +	struct ti_eqep_cnt *priv = counter->priv;
>> +	u32 quprd;
>> +
>> +	/* convert nanoseconds to timer ticks */
>> +	quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
>> +	if (quprd != value)
>> +		return -ERANGE;
>> +
>> +	/* protect against infinite unit timeout interrupts */
>> +	if (quprd == 0)
>> +		return -EINVAL;
> 
> I doubt there's any practical reason for a user to set the timer period
> to 0, but perhaps we should not try to protect users from themselves
> here. It's very obvious and expected that setting the timer period to 0
> results in timeouts as quickly as possible, so really the user should be
> left to reap the fruits of their decision regardless of how asinine that
> decision is.

Even if the operating system ceases operation because the interrupt
handler keeps running because clearing the interrupt has no effect
in this condition?

...

>> @@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>>   	struct ti_eqep_cnt *priv;
>> +	struct clk *clk;
>>   	void __iomem *base;
>>   	int err;
>>   	int irq;
>> @@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev)
>>   	if (!priv)
>>   		return -ENOMEM;
>>   
>> +	clk = devm_clk_get(dev, "sysclkout");
>> +	if (IS_ERR(clk)) {
>> +		if (PTR_ERR(clk) != -EPROBE_DEFER)
>> +			dev_err(dev, "failed to get sysclkout");
>> +		return PTR_ERR(clk);
>> +	}
>> +
>> +	priv->sysclkout_rate = clk_get_rate(clk);
>> +	if (priv->sysclkout_rate == 0) {
>> +		dev_err(dev, "failed to get sysclkout rate");
>> +		/* prevent divide by zero */
>> +		priv->sysclkout_rate = 1;
>> +		/*
>> +		 * This error is not expected and the driver is mostly usable
>> +		 * without clock rate anyway, so don't exit here.
>> +		 */
> 
> If the values for these new attributes are expected to be denominated in
> nanoseconds then we must guarantee that. You should certainly error out
> here if you can't guarantee such.
> 
> Alternatively, you could provide an additional set of attributes that
> are denominated in units of raw timer ticks rather than nanoseconds.
> That way if you can't determine the clock rate you can simply have the
> nanosecond-denominated timer attributes return an EOPNOTSUPP error code
> or similar while still providing users with the raw timer ticks
> attributes.

I think we should just fail here.

Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer

[William Breathitt Gray]

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

On Wed, Oct 27, 2021 at 10:28:59AM -0500, David Lechner wrote:
> On 10/25/21 3:48 AM, William Breathitt Gray wrote:
> > On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
> >> This adds support to the TI eQEP counter driver for the Unit Timer.
> >> The Unit Timer is a device-level extension that provides a timer to be
> >> used for speed calculations. The sysfs interface for the Unit Timer is
> >> new and will be documented in a later commit. It contains a R/W time
> >> attribute for the current time, a R/W period attribute for the timeout
> >> period and a R/W enable attribute to start/stop the timer. It also
> >> implements a timeout event on the chrdev interface that is triggered
> >> each time the period timeout is reached.
> >>
> >> Signed-off-by: David Lechner <david@lechnology.com>
> > 
> > I'll comment on the sysfs interface in the respective docs patch. Some
> > comments regarding this patch below.
> > 
> 
> ...
> 
> >> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
> >> +					   u64 value)
> >> +{
> >> +	struct ti_eqep_cnt *priv = counter->priv;
> >> +	u32 quprd;
> >> +
> >> +	/* convert nanoseconds to timer ticks */
> >> +	quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> >> +	if (quprd != value)
> >> +		return -ERANGE;
> >> +
> >> +	/* protect against infinite unit timeout interrupts */
> >> +	if (quprd == 0)
> >> +		return -EINVAL;
> > 
> > I doubt there's any practical reason for a user to set the timer period
> > to 0, but perhaps we should not try to protect users from themselves
> > here. It's very obvious and expected that setting the timer period to 0
> > results in timeouts as quickly as possible, so really the user should be
> > left to reap the fruits of their decision regardless of how asinine that
> > decision is.
> 
> Even if the operating system ceases operation because the interrupt
> handler keeps running because clearing the interrupt has no effect
> in this condition?

I don't disagree with you that configuring the device to repeatedly
timeout without pause would be a waste of system resources. However, it
is more appropriate for this protection to be located in a userspace
application rather than the driver code here.

The purpose of a driver is to expose the functionality of a device in an
understandable and consistent manner. Drivers should not dictate what a
user does with their device, but rather should help facilitate the
user's control so that the device behaves as would be expected given
such an interface.

For this particular case, the device is capable of sending an interrupt
when a timeout events occurs, and the timeout period can be adjusted;
setting the timeout period lower and lower results in less and less time
between timeout events. The behavior and result of setting the timeout
period lower is well-defined and predictable; it is intuitive that
configuring the timeout period to 0, the lowest value possible, results
in the shortest time possible between timeouts: no pause at all.

As long as the functionality of this device is exposed in such an
understandable and consistent manner, the driver succeeds in serving its
purpose. So I believe a timeout period of 0 is a valid configuration
for this driver to allow, albeit a seemingly pointless one for users to
actually choose. To that end, simply set the default value of QUPRD to
non-zero on probe() as you do already in this patch and let the user be
free to adjust if they so decide.

William Breathitt Gray

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

Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer

[David Lechner]

On 10/28/21 2:48 AM, William Breathitt Gray wrote:
> On Wed, Oct 27, 2021 at 10:28:59AM -0500, David Lechner wrote:
>> On 10/25/21 3:48 AM, William Breathitt Gray wrote:
>>> On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
>>>> This adds support to the TI eQEP counter driver for the Unit Timer.
>>>> The Unit Timer is a device-level extension that provides a timer to be
>>>> used for speed calculations. The sysfs interface for the Unit Timer is
>>>> new and will be documented in a later commit. It contains a R/W time
>>>> attribute for the current time, a R/W period attribute for the timeout
>>>> period and a R/W enable attribute to start/stop the timer. It also
>>>> implements a timeout event on the chrdev interface that is triggered
>>>> each time the period timeout is reached.
>>>>
>>>> Signed-off-by: David Lechner <david@lechnology.com>
>>>
>>> I'll comment on the sysfs interface in the respective docs patch. Some
>>> comments regarding this patch below.
>>>
>>
>> ...
>>
>>>> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
>>>> +					   u64 value)
>>>> +{
>>>> +	struct ti_eqep_cnt *priv = counter->priv;
>>>> +	u32 quprd;
>>>> +
>>>> +	/* convert nanoseconds to timer ticks */
>>>> +	quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
>>>> +	if (quprd != value)
>>>> +		return -ERANGE;
>>>> +
>>>> +	/* protect against infinite unit timeout interrupts */
>>>> +	if (quprd == 0)
>>>> +		return -EINVAL;
>>>
>>> I doubt there's any practical reason for a user to set the timer period
>>> to 0, but perhaps we should not try to protect users from themselves
>>> here. It's very obvious and expected that setting the timer period to 0
>>> results in timeouts as quickly as possible, so really the user should be
>>> left to reap the fruits of their decision regardless of how asinine that
>>> decision is.
>>
>> Even if the operating system ceases operation because the interrupt
>> handler keeps running because clearing the interrupt has no effect
>> in this condition?
> 
> I don't disagree with you that configuring the device to repeatedly
> timeout without pause would be a waste of system resources. However, it
> is more appropriate for this protection to be located in a userspace
> application rather than the driver code here.
> 
> The purpose of a driver is to expose the functionality of a device in an
> understandable and consistent manner. Drivers should not dictate what a
> user does with their device, but rather should help facilitate the
> user's control so that the device behaves as would be expected given
> such an interface.
> 
> For this particular case, the device is capable of sending an interrupt
> when a timeout events occurs, and the timeout period can be adjusted;
> setting the timeout period lower and lower results in less and less time
> between timeout events. The behavior and result of setting the timeout
> period lower is well-defined and predictable; it is intuitive that
> configuring the timeout period to 0, the lowest value possible, results
> in the shortest time possible between timeouts: no pause at all.
> 
> As long as the functionality of this device is exposed in such an
> understandable and consistent manner, the driver succeeds in serving its
> purpose. So I believe a timeout period of 0 is a valid configuration
> for this driver to allow, albeit a seemingly pointless one for users to
> actually choose. To that end, simply set the default value of QUPRD to
> non-zero on probe() as you do already in this patch and let the user be
> free to adjust if they so decide.
> 
> William Breathitt Gray
> 

I disagree. I consider this a crash. The system becomes completely
unusable and you have to pull power to turn it off, potentially
leading to data loss and disk corruption.

Re: [PATCH 3/8] counter/ti-eqep: add support for unit timer

[William Breathitt Gray]

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

On Thu, Oct 28, 2021 at 08:42:49AM -0500, David Lechner wrote:
> On 10/28/21 2:48 AM, William Breathitt Gray wrote:
> > On Wed, Oct 27, 2021 at 10:28:59AM -0500, David Lechner wrote:
> >> On 10/25/21 3:48 AM, William Breathitt Gray wrote:
> >>> On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote:
> >>>> This adds support to the TI eQEP counter driver for the Unit Timer.
> >>>> The Unit Timer is a device-level extension that provides a timer to be
> >>>> used for speed calculations. The sysfs interface for the Unit Timer is
> >>>> new and will be documented in a later commit. It contains a R/W time
> >>>> attribute for the current time, a R/W period attribute for the timeout
> >>>> period and a R/W enable attribute to start/stop the timer. It also
> >>>> implements a timeout event on the chrdev interface that is triggered
> >>>> each time the period timeout is reached.
> >>>>
> >>>> Signed-off-by: David Lechner <david@lechnology.com>
> >>>
> >>> I'll comment on the sysfs interface in the respective docs patch. Some
> >>> comments regarding this patch below.
> >>>
> >>
> >> ...
> >>
> >>>> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter,
> >>>> +					   u64 value)
> >>>> +{
> >>>> +	struct ti_eqep_cnt *priv = counter->priv;
> >>>> +	u32 quprd;
> >>>> +
> >>>> +	/* convert nanoseconds to timer ticks */
> >>>> +	quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> >>>> +	if (quprd != value)
> >>>> +		return -ERANGE;
> >>>> +
> >>>> +	/* protect against infinite unit timeout interrupts */
> >>>> +	if (quprd == 0)
> >>>> +		return -EINVAL;
> >>>
> >>> I doubt there's any practical reason for a user to set the timer period
> >>> to 0, but perhaps we should not try to protect users from themselves
> >>> here. It's very obvious and expected that setting the timer period to 0
> >>> results in timeouts as quickly as possible, so really the user should be
> >>> left to reap the fruits of their decision regardless of how asinine that
> >>> decision is.
> >>
> >> Even if the operating system ceases operation because the interrupt
> >> handler keeps running because clearing the interrupt has no effect
> >> in this condition?
> > 
> > I don't disagree with you that configuring the device to repeatedly
> > timeout without pause would be a waste of system resources. However, it
> > is more appropriate for this protection to be located in a userspace
> > application rather than the driver code here.
> > 
> > The purpose of a driver is to expose the functionality of a device in an
> > understandable and consistent manner. Drivers should not dictate what a
> > user does with their device, but rather should help facilitate the
> > user's control so that the device behaves as would be expected given
> > such an interface.
> > 
> > For this particular case, the device is capable of sending an interrupt
> > when a timeout events occurs, and the timeout period can be adjusted;
> > setting the timeout period lower and lower results in less and less time
> > between timeout events. The behavior and result of setting the timeout
> > period lower is well-defined and predictable; it is intuitive that
> > configuring the timeout period to 0, the lowest value possible, results
> > in the shortest time possible between timeouts: no pause at all.
> > 
> > As long as the functionality of this device is exposed in such an
> > understandable and consistent manner, the driver succeeds in serving its
> > purpose. So I believe a timeout period of 0 is a valid configuration
> > for this driver to allow, albeit a seemingly pointless one for users to
> > actually choose. To that end, simply set the default value of QUPRD to
> > non-zero on probe() as you do already in this patch and let the user be
> > free to adjust if they so decide.
> > 
> > William Breathitt Gray
> > 
> 
> I disagree. I consider this a crash. The system becomes completely
> unusable and you have to pull power to turn it off, potentially
> leading to data loss and disk corruption.

I hope I'm not being excessively pedantic here -- I'll concede to a
third opion on this if someone else wants to chime in -- but I want to
ensure that we are not going outside the scope of what a driver should
do. Note that any device is capable of flooding the system with
interrupts (e.g. a counter operating on a high enough frequency
overflowing a low ceiling), so I don't think that reason alone will pass
muster. Nevertheless, it is important to define when a driver should
return an error or not.

Take for example, the period range check right above. If a user requests
the device do something it cannot, such as counting down from a period
value that is too high for it to represent internally, then it is
appropriate to return an error: the device cannot perform the request
and as such the request is not valid input for the driver.

However, when we apply the same method to the zero value case -- a user
requests a timeout period of 0 -- the device is capable of performing
that request: the device is capable of waiting 0 nanoseconds and as such
the request is a valid input for the driver because it can be performed
by the device exactly as expected by the user. As long as the behavior
of a request is well-defined, we must assume the user knows what they
are doing, and thus should be permitted to request their device perform
that behavior.

A driver should not speculate on the intent of a user's actions.
Restricting what a user can do with their device is a matter of
configuration policy, and configuration policy belongs appropriately in
userspace. Rather, the scope of a driver should be limited narrowly to
exposure of a device functionality in a standard and predictable way.

William Breathitt Gray

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

[PATCH 4/8] docs: counter: add unit timer sysfs attributes

[David Lechner]

This documents new unit timer sysfs attributes for the counter
subsystem.

Signed-off-by: David Lechner <david@lechnology.com>
---
 Documentation/ABI/testing/sysfs-bus-counter | 24 +++++++++++++++++++++
 drivers/counter/ti-eqep.c                   |  2 +-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 06c2b3e27e0b..37d960a8cb1b 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -218,6 +218,9 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
+What:		/sys/bus/counter/devices/unit_timer_enable_component_id
+What:		/sys/bus/counter/devices/unit_timer_period_component_id
+What:		/sys/bus/counter/devices/unit_timer_time_component_id
 KernelVersion:	5.16
 Contact:	linux-iio@vger.kernel.org
 Description:
@@ -345,3 +348,24 @@ Description:
 			via index_polarity. The index function (as enabled via
 			preset_enable) is performed synchronously with the
 			quadrature clock on the active level of the index input.
+
+What:		/sys/bus/counter/devices/unit_timer_enable
+KernelVersion:	5.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write attribute that starts or stops the unit timer. Valid
+		values are boolean.
+
+What:		/sys/bus/counter/devices/unit_timer_period
+KernelVersion:	5.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write attribute that selects the unit timer timeout in
+		nanoseconds.
+
+What:		/sys/bus/counter/devices/unit_timer_time
+KernelVersion:	5.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write attribute that indicates the current time of the
+		unit timer in nanoseconds.
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index a4a5a4486329..1ba7f3c7cb7e 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -680,7 +680,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
 	pm_runtime_get_sync(dev);
 
 	/*
-	 * We can end up with an interupt infinite loop (interrupts triggered
+	 * We can end up with an interrupt infinite loop (interrupts triggered
 	 * as soon as they are cleared) if we leave these at the default value
 	 * of 0 and events are enabled.
 	 */
-- 
2.25.1

Re: [PATCH 4/8] docs: counter: add unit timer sysfs attributes

[Jonathan Cameron]

On Sat, 16 Oct 2021 20:33:39 -0500
David Lechner <david@lechnology.com> wrote:

> This documents new unit timer sysfs attributes for the counter
> subsystem.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

The ti-eqep.c change needs moving to patch 1 where the typo was introduced.

> ---
>  Documentation/ABI/testing/sysfs-bus-counter | 24 +++++++++++++++++++++
>  drivers/counter/ti-eqep.c                   |  2 +-
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 06c2b3e27e0b..37d960a8cb1b 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -218,6 +218,9 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
> +What:		/sys/bus/counter/devices/unit_timer_enable_component_id
> +What:		/sys/bus/counter/devices/unit_timer_period_component_id
> +What:		/sys/bus/counter/devices/unit_timer_time_component_id
>  KernelVersion:	5.16
>  Contact:	linux-iio@vger.kernel.org
>  Description:
> @@ -345,3 +348,24 @@ Description:
>  			via index_polarity. The index function (as enabled via
>  			preset_enable) is performed synchronously with the
>  			quadrature clock on the active level of the index input.
> +
> +What:		/sys/bus/counter/devices/unit_timer_enable
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write attribute that starts or stops the unit timer. Valid
> +		values are boolean.
> +
> +What:		/sys/bus/counter/devices/unit_timer_period
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write attribute that selects the unit timer timeout in
> +		nanoseconds.
> +
> +What:		/sys/bus/counter/devices/unit_timer_time
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write attribute that indicates the current time of the
> +		unit timer in nanoseconds.
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index a4a5a4486329..1ba7f3c7cb7e 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -680,7 +680,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	pm_runtime_get_sync(dev);
>  
>  	/*
> -	 * We can end up with an interupt infinite loop (interrupts triggered
> +	 * We can end up with an interrupt infinite loop (interrupts triggered

This change should be in a separate patch.

>  	 * as soon as they are cleared) if we leave these at the default value
>  	 * of 0 and events are enabled.
>  	 */

Re: [PATCH 4/8] docs: counter: add unit timer sysfs attributes

[William Breathitt Gray]

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

On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote:
> This documents new unit timer sysfs attributes for the counter
> subsystem.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

Hello David,

The unit timer is effectively a Count in its own right, so instead of
introducing new sysfs attributes you can just implement it as another
Count in the driver. Count 0 is "QPOSCNT", so set the name of this new
Count 1 as "Unit Timer" (or the datasheet naming if more apt) to
differentiate the Counts. You can then provide the "unit_timer_enable",
"unit_timer_period", and "unit_timer_time" functionalities as respective
Count 1 extensions ("enable" and "period") and Count 1 "count".

If you believe it appropriate, you can provide the raw timer ticks via
the Count 1 "count" while a nanoseconds interface is provided via a
Count 1 extension "timeout" (or something similar).

William Breathitt Gray

> ---
>  Documentation/ABI/testing/sysfs-bus-counter | 24 +++++++++++++++++++++
>  drivers/counter/ti-eqep.c                   |  2 +-
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 06c2b3e27e0b..37d960a8cb1b 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -218,6 +218,9 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
> +What:		/sys/bus/counter/devices/unit_timer_enable_component_id
> +What:		/sys/bus/counter/devices/unit_timer_period_component_id
> +What:		/sys/bus/counter/devices/unit_timer_time_component_id
>  KernelVersion:	5.16
>  Contact:	linux-iio@vger.kernel.org
>  Description:
> @@ -345,3 +348,24 @@ Description:
>  			via index_polarity. The index function (as enabled via
>  			preset_enable) is performed synchronously with the
>  			quadrature clock on the active level of the index input.
> +
> +What:		/sys/bus/counter/devices/unit_timer_enable
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write attribute that starts or stops the unit timer. Valid
> +		values are boolean.
> +
> +What:		/sys/bus/counter/devices/unit_timer_period
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write attribute that selects the unit timer timeout in
> +		nanoseconds.
> +
> +What:		/sys/bus/counter/devices/unit_timer_time
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write attribute that indicates the current time of the
> +		unit timer in nanoseconds.
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index a4a5a4486329..1ba7f3c7cb7e 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -680,7 +680,7 @@ static int ti_eqep_probe(struct platform_device *pdev)
>  	pm_runtime_get_sync(dev);
>  
>  	/*
> -	 * We can end up with an interupt infinite loop (interrupts triggered
> +	 * We can end up with an interrupt infinite loop (interrupts triggered
>  	 * as soon as they are cleared) if we leave these at the default value
>  	 * of 0 and events are enabled.
>  	 */
> -- 
> 2.25.1
> 

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

Re: [PATCH 4/8] docs: counter: add unit timer sysfs attributes

[David Lechner]

On 10/27/21 1:46 AM, William Breathitt Gray wrote:
> On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote:
>> This documents new unit timer sysfs attributes for the counter
>> subsystem.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
> 
> Hello David,
> 
> The unit timer is effectively a Count in its own right, so instead of
> introducing new sysfs attributes you can just implement it as another
> Count in the driver. Count 0 is "QPOSCNT", so set the name of this new
> Count 1 as "Unit Timer" (or the datasheet naming if more apt) to
> differentiate the Counts. You can then provide the "unit_timer_enable",
> "unit_timer_period", and "unit_timer_time" functionalities as respective
> Count 1 extensions ("enable" and "period") and Count 1 "count".
> 
> If you believe it appropriate, you can provide the raw timer ticks via
> the Count 1 "count" while a nanoseconds interface is provided via a
> Count 1 extension "timeout" (or something similar).
> 

Sounds reasonable.

Re: [PATCH 4/8] docs: counter: add unit timer sysfs attributes

[William Breathitt Gray]

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

On Wed, Oct 27, 2021 at 10:30:36AM -0500, David Lechner wrote:
> On 10/27/21 1:46 AM, William Breathitt Gray wrote:
> > On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote:
> >> This documents new unit timer sysfs attributes for the counter
> >> subsystem.
> >>
> >> Signed-off-by: David Lechner <david@lechnology.com>
> > 
> > Hello David,
> > 
> > The unit timer is effectively a Count in its own right, so instead of
> > introducing new sysfs attributes you can just implement it as another
> > Count in the driver. Count 0 is "QPOSCNT", so set the name of this new
> > Count 1 as "Unit Timer" (or the datasheet naming if more apt) to
> > differentiate the Counts. You can then provide the "unit_timer_enable",
> > "unit_timer_period", and "unit_timer_time" functionalities as respective
> > Count 1 extensions ("enable" and "period") and Count 1 "count".

Actually if the counter function here is COUNTER_FUNCTION_DECREASE, then
instead of introducing a new "period" extension, define this as a
"ceiling" extension; that's what ceiling represents in the Counter
interface: "the upper limit for the respective counter", which is the
period of a timer counting down to a timeout.

William Breathitt Gray

> > 
> > If you believe it appropriate, you can provide the raw timer ticks via
> > the Count 1 "count" while a nanoseconds interface is provided via a
> > Count 1 extension "timeout" (or something similar).
> > 
> 
> Sounds reasonable.
> 

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

Re: [PATCH 4/8] docs: counter: add unit timer sysfs attributes

[David Lechner]

On 10/28/21 2:59 AM, William Breathitt Gray wrote:
> On Wed, Oct 27, 2021 at 10:30:36AM -0500, David Lechner wrote:
>> On 10/27/21 1:46 AM, William Breathitt Gray wrote:
>>> On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote:
>>>> This documents new unit timer sysfs attributes for the counter
>>>> subsystem.
>>>>
>>>> Signed-off-by: David Lechner <david@lechnology.com>
>>>
>>> Hello David,
>>>
>>> The unit timer is effectively a Count in its own right, so instead of
>>> introducing new sysfs attributes you can just implement it as another
>>> Count in the driver. Count 0 is "QPOSCNT", so set the name of this new
>>> Count 1 as "Unit Timer" (or the datasheet naming if more apt) to
>>> differentiate the Counts. You can then provide the "unit_timer_enable",
>>> "unit_timer_period", and "unit_timer_time" functionalities as respective
>>> Count 1 extensions ("enable" and "period") and Count 1 "count".
> 
> Actually if the counter function here is COUNTER_FUNCTION_DECREASE, then

It is an increasing counter.

> instead of introducing a new "period" extension, define this as a
> "ceiling" extension; that's what ceiling represents in the Counter
> interface: "the upper limit for the respective counter", which is the
> period of a timer counting down to a timeout.

In one of the other patches, you made a comment about the semantics
of ceiling with relation to the overflow event. We can indeed treat
the timer as a counter and the period as the ceiling. However, the
unit timer event occurs when the count is equal to the period (ceiling)
whereas an overflow event occurs when the count exceeds the ceiling.
So what would this event be called in generic counter terms? "timeout"
doesn't seem right.

> 
> William Breathitt Gray
> 
>>>
>>> If you believe it appropriate, you can provide the raw timer ticks via
>>> the Count 1 "count" while a nanoseconds interface is provided via a
>>> Count 1 extension "timeout" (or something similar).
>>>

One area where this concept of treating a timer as a counter potentially
breaks down is the issue of CPU frequency scaling. By treating the unit
timer as a timer, then the kernel could take care of any changes in clock
rate internally by automatically adjusting the prescalar and period on
rate change events. But if we are just treating it as a counter, then we
should probably just have an attribute that provides the clock rate and
if we want to support CPU frequency scaling, add an event that indicates
that the clock rate changed.

Re: [PATCH 4/8] docs: counter: add unit timer sysfs attributes

[William Breathitt Gray]

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

On Sat, Oct 30, 2021 at 11:40:27AM -0500, David Lechner wrote:
> On 10/28/21 2:59 AM, William Breathitt Gray wrote:
> > On Wed, Oct 27, 2021 at 10:30:36AM -0500, David Lechner wrote:
> >> On 10/27/21 1:46 AM, William Breathitt Gray wrote:
> >>> On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote:
> >>>> This documents new unit timer sysfs attributes for the counter
> >>>> subsystem.
> >>>>
> >>>> Signed-off-by: David Lechner <david@lechnology.com>
> >>>
> >>> Hello David,
> >>>
> >>> The unit timer is effectively a Count in its own right, so instead of
> >>> introducing new sysfs attributes you can just implement it as another
> >>> Count in the driver. Count 0 is "QPOSCNT", so set the name of this new
> >>> Count 1 as "Unit Timer" (or the datasheet naming if more apt) to
> >>> differentiate the Counts. You can then provide the "unit_timer_enable",
> >>> "unit_timer_period", and "unit_timer_time" functionalities as respective
> >>> Count 1 extensions ("enable" and "period") and Count 1 "count".
> > 
> > Actually if the counter function here is COUNTER_FUNCTION_DECREASE, then
> 
> It is an increasing counter.
> 
> > instead of introducing a new "period" extension, define this as a
> > "ceiling" extension; that's what ceiling represents in the Counter
> > interface: "the upper limit for the respective counter", which is the
> > period of a timer counting down to a timeout.
> 
> In one of the other patches, you made a comment about the semantics
> of ceiling with relation to the overflow event. We can indeed treat
> the timer as a counter and the period as the ceiling. However, the
> unit timer event occurs when the count is equal to the period (ceiling)
> whereas an overflow event occurs when the count exceeds the ceiling.
> So what would this event be called in generic counter terms? "timeout"
> doesn't seem right.

Okay, so COUNTER_EVENT_THRESHOLD would be the respective Counter event
type for this behavior because the event triggers once a threshold is
reached (ceiling in this case).

But implementing the unit timer as a counter might not be the best path
forward as you've mentioned below.

> > 
> > William Breathitt Gray
> > 
> >>>
> >>> If you believe it appropriate, you can provide the raw timer ticks via
> >>> the Count 1 "count" while a nanoseconds interface is provided via a
> >>> Count 1 extension "timeout" (or something similar).
> >>>
> 
> One area where this concept of treating a timer as a counter potentially
> breaks down is the issue of CPU frequency scaling. By treating the unit
> timer as a timer, then the kernel could take care of any changes in clock
> rate internally by automatically adjusting the prescalar and period on
> rate change events. But if we are just treating it as a counter, then we
> should probably just have an attribute that provides the clock rate and
> if we want to support CPU frequency scaling, add an event that indicates
> that the clock rate changed.

You're right, treating the unit timer as a counter might not be the most
appropriate interface. Because this is a timer afterall, perhaps
exposing this via the hrtimer API is better. You then have an existing
interface available designed for timer configuration, and you can
leverage the struct hrtimer function callback to handle your timeout
interrupts.

William Breathitt Gray

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

Re: [PATCH 4/8] docs: counter: add unit timer sysfs attributes

[William Breathitt Gray]

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

On Mon, Nov 01, 2021 at 01:08:46PM +0900, William Breathitt Gray wrote:
> On Sat, Oct 30, 2021 at 11:40:27AM -0500, David Lechner wrote:
> > On 10/28/21 2:59 AM, William Breathitt Gray wrote:
> > > On Wed, Oct 27, 2021 at 10:30:36AM -0500, David Lechner wrote:
> > >> On 10/27/21 1:46 AM, William Breathitt Gray wrote:
> > >>> On Sat, Oct 16, 2021 at 08:33:39PM -0500, David Lechner wrote:
> > >>>> This documents new unit timer sysfs attributes for the counter
> > >>>> subsystem.
> > >>>>
> > >>>> Signed-off-by: David Lechner <david@lechnology.com>
> > >>>
> > >>> Hello David,
> > >>>
> > >>> The unit timer is effectively a Count in its own right, so instead of
> > >>> introducing new sysfs attributes you can just implement it as another
> > >>> Count in the driver. Count 0 is "QPOSCNT", so set the name of this new
> > >>> Count 1 as "Unit Timer" (or the datasheet naming if more apt) to
> > >>> differentiate the Counts. You can then provide the "unit_timer_enable",
> > >>> "unit_timer_period", and "unit_timer_time" functionalities as respective
> > >>> Count 1 extensions ("enable" and "period") and Count 1 "count".
> > > 
> > > Actually if the counter function here is COUNTER_FUNCTION_DECREASE, then
> > 
> > It is an increasing counter.
> > 
> > > instead of introducing a new "period" extension, define this as a
> > > "ceiling" extension; that's what ceiling represents in the Counter
> > > interface: "the upper limit for the respective counter", which is the
> > > period of a timer counting down to a timeout.
> > 
> > In one of the other patches, you made a comment about the semantics
> > of ceiling with relation to the overflow event. We can indeed treat
> > the timer as a counter and the period as the ceiling. However, the
> > unit timer event occurs when the count is equal to the period (ceiling)
> > whereas an overflow event occurs when the count exceeds the ceiling.
> > So what would this event be called in generic counter terms? "timeout"
> > doesn't seem right.
> 
> Okay, so COUNTER_EVENT_THRESHOLD would be the respective Counter event
> type for this behavior because the event triggers once a threshold is
> reached (ceiling in this case).
> 
> But implementing the unit timer as a counter might not be the best path
> forward as you've mentioned below.
> 
> > > 
> > > William Breathitt Gray
> > > 
> > >>>
> > >>> If you believe it appropriate, you can provide the raw timer ticks via
> > >>> the Count 1 "count" while a nanoseconds interface is provided via a
> > >>> Count 1 extension "timeout" (or something similar).
> > >>>
> > 
> > One area where this concept of treating a timer as a counter potentially
> > breaks down is the issue of CPU frequency scaling. By treating the unit
> > timer as a timer, then the kernel could take care of any changes in clock
> > rate internally by automatically adjusting the prescalar and period on
> > rate change events. But if we are just treating it as a counter, then we
> > should probably just have an attribute that provides the clock rate and
> > if we want to support CPU frequency scaling, add an event that indicates
> > that the clock rate changed.
> 
> You're right, treating the unit timer as a counter might not be the most
> appropriate interface. Because this is a timer afterall, perhaps
> exposing this via the hrtimer API is better. You then have an existing
> interface available designed for timer configuration, and you can
> leverage the struct hrtimer function callback to handle your timeout
> interrupts.
> 
> William Breathitt Gray

Sorry, I think I meant the clockevents framework, not hrtimers. I'm not
as familiar with timers but perhaps you know more than I do here.

William Breathitt Gray

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

[PATCH 5/8] counter/ti-eqep: add support for latched position

[David Lechner]

This adds support to the TI eQEP counter driver for a latched position.
This is a new extension that gets the counter count that was recorded
when an event was triggered. A new device-level latch_mode attribute is
added to select the trigger. Edge capture unit support will be needed
to make full use of this, but "Unit timeout" mode can already be used
to calculate high speeds.

The unit timer could also have attributes for latched_time and
latched_period that use the same trigger. However this is not a use
case at this time, so they can be added later if needed.

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/counter/ti-eqep.c | 50 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index 1ba7f3c7cb7e..ef899655ad1d 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -405,12 +405,28 @@ static int ti_eqep_direction_read(struct counter_device *counter,
 	return 0;
 }
 
+static int ti_eqep_position_latched_count_read(struct counter_device *counter,
+					       struct counter_count *count,
+					       u64 *value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qposlat;
+
+	regmap_read(priv->regmap32, QPOSLAT, &qposlat);
+
+	*value = qposlat;
+
+	return 0;
+}
+
 static struct counter_comp ti_eqep_position_ext[] = {
 	COUNTER_COMP_CEILING(ti_eqep_position_ceiling_read,
 			     ti_eqep_position_ceiling_write),
 	COUNTER_COMP_ENABLE(ti_eqep_position_enable_read,
 			    ti_eqep_position_enable_write),
 	COUNTER_COMP_DIRECTION(ti_eqep_direction_read),
+	COUNTER_COMP_COUNT_U64("latched_count",
+			       ti_eqep_position_latched_count_read, NULL),
 };
 
 static struct counter_signal ti_eqep_signals[] = {
@@ -463,6 +479,38 @@ static struct counter_count ti_eqep_counts[] = {
 	},
 };
 
+static int ti_eqep_latch_mode_read(struct counter_device *counter,
+					    u32 *value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qepctl;
+
+	regmap_read(priv->regmap16, QEPCTL, &qepctl);
+	*value = !!(qepctl & QEPCTL_QCLM);
+
+	return 0;
+}
+
+static int ti_eqep_latch_mode_write(struct counter_device *counter,
+					     u32 value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+
+	if (value)
+		regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_QCLM);
+	else
+		regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_QCLM);
+
+	return 0;
+}
+
+static const char *const ti_eqep_latch_mode_names[] = {
+	"Read count",
+	"Unit timeout",
+};
+
+static DEFINE_COUNTER_ENUM(ti_eqep_latch_modes, ti_eqep_latch_mode_names);
+
 static int ti_eqep_unit_timer_time_read(struct counter_device *counter,
 				       u64 *value)
 {
@@ -553,6 +601,8 @@ static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
 }
 
 static struct counter_comp ti_eqep_device_ext[] = {
+	COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read,
+				ti_eqep_latch_mode_write, ti_eqep_latch_modes),
 	COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,
 				ti_eqep_unit_timer_time_write),
 	COUNTER_COMP_DEVICE_U64("unit_timer_period",
-- 
2.25.1

Re: [PATCH 5/8] counter/ti-eqep: add support for latched position

[William Breathitt Gray]

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

On Sat, Oct 16, 2021 at 08:33:40PM -0500, David Lechner wrote:
> This adds support to the TI eQEP counter driver for a latched position.
> This is a new extension that gets the counter count that was recorded
> when an event was triggered. A new device-level latch_mode attribute is
> added to select the trigger. Edge capture unit support will be needed
> to make full use of this, but "Unit timeout" mode can already be used
> to calculate high speeds.
> 
> The unit timer could also have attributes for latched_time and
> latched_period that use the same trigger. However this is not a use
> case at this time, so they can be added later if needed.

I see that "latched_count" holds the captured counter count; would this
"latched_time" hold the captured unit timer time? If so, does that mean
setting the latch mode to "Unit timeout" always results in a
"latched_time" equal to 0 (assuming that's when the timeout event
triggers)?

> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  drivers/counter/ti-eqep.c | 50 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index 1ba7f3c7cb7e..ef899655ad1d 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -405,12 +405,28 @@ static int ti_eqep_direction_read(struct counter_device *counter,
>  	return 0;
>  }
>  
> +static int ti_eqep_position_latched_count_read(struct counter_device *counter,
> +					       struct counter_count *count,
> +					       u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qposlat;
> +
> +	regmap_read(priv->regmap32, QPOSLAT, &qposlat);
> +
> +	*value = qposlat;
> +
> +	return 0;
> +}
> +
>  static struct counter_comp ti_eqep_position_ext[] = {
>  	COUNTER_COMP_CEILING(ti_eqep_position_ceiling_read,
>  			     ti_eqep_position_ceiling_write),
>  	COUNTER_COMP_ENABLE(ti_eqep_position_enable_read,
>  			    ti_eqep_position_enable_write),
>  	COUNTER_COMP_DIRECTION(ti_eqep_direction_read),
> +	COUNTER_COMP_COUNT_U64("latched_count",
> +			       ti_eqep_position_latched_count_read, NULL),
>  };
>  
>  static struct counter_signal ti_eqep_signals[] = {
> @@ -463,6 +479,38 @@ static struct counter_count ti_eqep_counts[] = {
>  	},
>  };
>  
> +static int ti_eqep_latch_mode_read(struct counter_device *counter,
> +					    u32 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qepctl;
> +
> +	regmap_read(priv->regmap16, QEPCTL, &qepctl);
> +	*value = !!(qepctl & QEPCTL_QCLM);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_latch_mode_write(struct counter_device *counter,
> +					     u32 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	if (value)
> +		regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_QCLM);
> +	else
> +		regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_QCLM);
> +
> +	return 0;
> +}
> +
> +static const char *const ti_eqep_latch_mode_names[] = {
> +	"Read count",
> +	"Unit timeout",
> +};
> +
> +static DEFINE_COUNTER_ENUM(ti_eqep_latch_modes, ti_eqep_latch_mode_names);
> +
>  static int ti_eqep_unit_timer_time_read(struct counter_device *counter,
>  				       u64 *value)
>  {
> @@ -553,6 +601,8 @@ static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
>  }
>  
>  static struct counter_comp ti_eqep_device_ext[] = {
> +	COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read,
> +				ti_eqep_latch_mode_write, ti_eqep_latch_modes),

It seems more appropriate to move this alongside "latched_count" as
Count extension because this is setting the trigger mode to latch the
respective Count's count. Or does this particular extension also affect
the "latched_time" capture for the unit timer?

William Breathitt Gray

>  	COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,
>  				ti_eqep_unit_timer_time_write),
>  	COUNTER_COMP_DEVICE_U64("unit_timer_period",
> -- 
> 2.25.1
> 

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

Re: [PATCH 5/8] counter/ti-eqep: add support for latched position

[David Lechner]

On 10/27/21 2:44 AM, William Breathitt Gray wrote:
> On Sat, Oct 16, 2021 at 08:33:40PM -0500, David Lechner wrote:
>> This adds support to the TI eQEP counter driver for a latched position.
>> This is a new extension that gets the counter count that was recorded
>> when an event was triggered. A new device-level latch_mode attribute is
>> added to select the trigger. Edge capture unit support will be needed
>> to make full use of this, but "Unit timeout" mode can already be used
>> to calculate high speeds.
>>
>> The unit timer could also have attributes for latched_time and
>> latched_period that use the same trigger. However this is not a use
>> case at this time, so they can be added later if needed.
> 
> I see that "latched_count" holds the captured counter count; would this
> "latched_time" hold the captured unit timer time? If so, does that mean
> setting the latch mode to "Unit timeout" always results in a
> "latched_time" equal to 0 (assuming that's when the timeout event
> triggers)?
> 

Some `latched_*` attributes will only be useful for one `latched_mode`
selection but not the other.

These latched registers are used to measure speed. There are two ways
to do this. A) measuring the change in position over a fixed time and
B) measuring the change in time for a fixed change in position. So for
A) latched_mode would be set to trigger on timeout and we would use
the latched_position for the measurement. For B) we would set the
latched_mode to trigger on reading the count register and use the
latched_time as the measurement.

...

>>   static struct counter_comp ti_eqep_device_ext[] = {
>> +	COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read,
>> +				ti_eqep_latch_mode_write, ti_eqep_latch_modes),
> 
> It seems more appropriate to move this alongside "latched_count" as
> Count extension because this is setting the trigger mode to latch the
> respective Count's count. Or does this particular extension also affect
> the "latched_time" capture for the unit timer?
> 

In hardware, there are at least 3 registers that get latched that I
recall. They are in different subsystems (main count, unit timer,
edge capture). So as you have guessed, that is the reason for having
the trigger selection at the device level.

Re: [PATCH 5/8] counter/ti-eqep: add support for latched position

[William Breathitt Gray]

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

On Wed, Oct 27, 2021 at 10:40:15AM -0500, David Lechner wrote:
> On 10/27/21 2:44 AM, William Breathitt Gray wrote:
> > On Sat, Oct 16, 2021 at 08:33:40PM -0500, David Lechner wrote:
> >> This adds support to the TI eQEP counter driver for a latched position.
> >> This is a new extension that gets the counter count that was recorded
> >> when an event was triggered. A new device-level latch_mode attribute is
> >> added to select the trigger. Edge capture unit support will be needed
> >> to make full use of this, but "Unit timeout" mode can already be used
> >> to calculate high speeds.
> >>
> >> The unit timer could also have attributes for latched_time and
> >> latched_period that use the same trigger. However this is not a use
> >> case at this time, so they can be added later if needed.
> > 
> > I see that "latched_count" holds the captured counter count; would this
> > "latched_time" hold the captured unit timer time? If so, does that mean
> > setting the latch mode to "Unit timeout" always results in a
> > "latched_time" equal to 0 (assuming that's when the timeout event
> > triggers)?
> > 
> 
> Some `latched_*` attributes will only be useful for one `latched_mode`
> selection but not the other.
> 
> These latched registers are used to measure speed. There are two ways
> to do this. A) measuring the change in position over a fixed time and
> B) measuring the change in time for a fixed change in position. So for
> A) latched_mode would be set to trigger on timeout and we would use
> the latched_position for the measurement. For B) we would set the
> latched_mode to trigger on reading the count register and use the
> latched_time as the measurement.
> 
> ...
> 
> >>   static struct counter_comp ti_eqep_device_ext[] = {
> >> +	COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read,
> >> +				ti_eqep_latch_mode_write, ti_eqep_latch_modes),
> > 
> > It seems more appropriate to move this alongside "latched_count" as
> > Count extension because this is setting the trigger mode to latch the
> > respective Count's count. Or does this particular extension also affect
> > the "latched_time" capture for the unit timer?
> > 
> 
> In hardware, there are at least 3 registers that get latched that I
> recall. They are in different subsystems (main count, unit timer,
> edge capture). So as you have guessed, that is the reason for having
> the trigger selection at the device level.

Ah, I see what's going on now. I think supporting these latch registers
will involve some further considerations. I'll continue my reply in the
respective docs patch where you've gone more in depth about the
hardware.

William Breathitt Gray

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

[PATCH 6/8] docs: counter: add latch_mode and latched_count sysfs attributes

[David Lechner]

This documents new counterX/latch_mode* and
counterX/countY/latched_count* attributes.

The counterX/signalY/*_available are moved to the
counterX/countY/*_available section similar to the way we already have
The counterX/*_component_id and The counterX/signalY/*_component_id
grouped together so that we don't have to start a 3rd redundant section
for device-level *_available section.

Signed-off-by: David Lechner <david@lechnology.com>
---
 Documentation/ABI/testing/sysfs-bus-counter | 39 ++++++++++++++++-----
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 37d960a8cb1b..78bb1a501007 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -59,10 +59,13 @@ What:		/sys/bus/counter/devices/counterX/countY/error_noise_available
 What:		/sys/bus/counter/devices/counterX/countY/function_available
 What:		/sys/bus/counter/devices/counterX/countY/prescaler_available
 What:		/sys/bus/counter/devices/counterX/countY/signalZ_action_available
+What:		/sys/bus/counter/devices/counterX/latch_mode_available
+What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
+What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
 KernelVersion:	5.2
 Contact:	linux-iio@vger.kernel.org
 Description:
-		Discrete set of available values for the respective Count Y
+		Discrete set of available values for the respective component
 		configuration are listed in this file. Values are delimited by
 		newline characters.
 
@@ -147,6 +150,14 @@ Description:
 			updates	the respective count. Quadrature encoding
 			determines the direction.
 
+What:		/sys/bus/counter/devices/counterX/countY/latched_count
+KernelVersion:	5.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Latched count data of Count Y represented as a string. The value
+		is latched in based on the trigger selected by the
+		counterX/latch_mode attribute.
+
 What:		/sys/bus/counter/devices/counterX/countY/name
 KernelVersion:	5.2
 Contact:	linux-iio@vger.kernel.org
@@ -209,6 +220,7 @@ What:		/sys/bus/counter/devices/counterX/countY/count_mode_component_id
 What:		/sys/bus/counter/devices/counterX/countY/direction_component_id
 What:		/sys/bus/counter/devices/counterX/countY/enable_component_id
 What:		/sys/bus/counter/devices/counterX/countY/error_noise_component_id
+What:		/sys/bus/counter/devices/counterX/countY/latched_count_component_id
 What:		/sys/bus/counter/devices/counterX/countY/prescaler_component_id
 What:		/sys/bus/counter/devices/counterX/countY/preset_component_id
 What:		/sys/bus/counter/devices/counterX/countY/preset_enable_component_id
@@ -218,6 +230,7 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
+What:		/sys/bus/counter/devices/latch_mode_component_id
 What:		/sys/bus/counter/devices/unit_timer_enable_component_id
 What:		/sys/bus/counter/devices/unit_timer_period_component_id
 What:		/sys/bus/counter/devices/unit_timer_time_component_id
@@ -244,6 +257,22 @@ Description:
 		counter_event data structures. The number of elements will be
 		rounded-up to a power of 2.
 
+What:		/sys/bus/counter/devices/counterX/latch_mode
+KernelVersion:	5.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write attribute that selects the trigger for latching
+		values. Valid values are device-specific (given by
+		latch_mode_available attribute) and may include:
+
+		"Read count":
+			Reading the countY/count attribute latches values.
+
+		"Unit timeout":
+			Unit timer timeout event latches values.
+
+		The latched values can be read from latched_* attributes.
+
 What:		/sys/bus/counter/devices/counterX/name
 KernelVersion:	5.2
 Contact:	linux-iio@vger.kernel.org
@@ -298,14 +327,6 @@ Description:
 		Active level of index input Signal Y; irrelevant in
 		non-synchronous load mode.
 
-What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
-What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
-KernelVersion:	5.2
-Contact:	linux-iio@vger.kernel.org
-Description:
-		Discrete set of available values for the respective Signal Y
-		configuration are listed in this file.
-
 What:		/sys/bus/counter/devices/counterX/signalY/name
 KernelVersion:	5.2
 Contact:	linux-iio@vger.kernel.org
-- 
2.25.1

Re: [PATCH 6/8] docs: counter: add latch_mode and latched_count sysfs attributes

[Jonathan Cameron]

On Sat, 16 Oct 2021 20:33:41 -0500
David Lechner <david@lechnology.com> wrote:

> This documents new counterX/latch_mode* and
> counterX/countY/latched_count* attributes.
> 
> The counterX/signalY/*_available are moved to the
> counterX/countY/*_available section similar to the way we already have
> The counterX/*_component_id and The counterX/signalY/*_component_id
> grouped together so that we don't have to start a 3rd redundant section
> for device-level *_available section.

Two unrelated changes in the same patch. Please redo this as multiple patches.

Thanks,

Jonathan

> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-counter | 39 ++++++++++++++++-----
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 37d960a8cb1b..78bb1a501007 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -59,10 +59,13 @@ What:		/sys/bus/counter/devices/counterX/countY/error_noise_available
>  What:		/sys/bus/counter/devices/counterX/countY/function_available
>  What:		/sys/bus/counter/devices/counterX/countY/prescaler_available
>  What:		/sys/bus/counter/devices/counterX/countY/signalZ_action_available
> +What:		/sys/bus/counter/devices/counterX/latch_mode_available
> +What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
> +What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
>  KernelVersion:	5.2
>  Contact:	linux-iio@vger.kernel.org
>  Description:
> -		Discrete set of available values for the respective Count Y
> +		Discrete set of available values for the respective component
>  		configuration are listed in this file. Values are delimited by
>  		newline characters.
>  
> @@ -147,6 +150,14 @@ Description:
>  			updates	the respective count. Quadrature encoding
>  			determines the direction.
>  
> +What:		/sys/bus/counter/devices/counterX/countY/latched_count
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Latched count data of Count Y represented as a string. The value
> +		is latched in based on the trigger selected by the
> +		counterX/latch_mode attribute.
> +
>  What:		/sys/bus/counter/devices/counterX/countY/name
>  KernelVersion:	5.2
>  Contact:	linux-iio@vger.kernel.org
> @@ -209,6 +220,7 @@ What:		/sys/bus/counter/devices/counterX/countY/count_mode_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/direction_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/enable_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/error_noise_component_id
> +What:		/sys/bus/counter/devices/counterX/countY/latched_count_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/prescaler_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/preset_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/preset_enable_component_id
> @@ -218,6 +230,7 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
> +What:		/sys/bus/counter/devices/latch_mode_component_id
>  What:		/sys/bus/counter/devices/unit_timer_enable_component_id
>  What:		/sys/bus/counter/devices/unit_timer_period_component_id
>  What:		/sys/bus/counter/devices/unit_timer_time_component_id
> @@ -244,6 +257,22 @@ Description:
>  		counter_event data structures. The number of elements will be
>  		rounded-up to a power of 2.
>  
> +What:		/sys/bus/counter/devices/counterX/latch_mode
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write attribute that selects the trigger for latching
> +		values. Valid values are device-specific (given by
> +		latch_mode_available attribute) and may include:
> +
> +		"Read count":
> +			Reading the countY/count attribute latches values.
> +
> +		"Unit timeout":
> +			Unit timer timeout event latches values.
> +
> +		The latched values can be read from latched_* attributes.
> +
>  What:		/sys/bus/counter/devices/counterX/name
>  KernelVersion:	5.2
>  Contact:	linux-iio@vger.kernel.org
> @@ -298,14 +327,6 @@ Description:
>  		Active level of index input Signal Y; irrelevant in
>  		non-synchronous load mode.
>  
> -What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
> -What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
> -KernelVersion:	5.2
> -Contact:	linux-iio@vger.kernel.org
> -Description:
> -		Discrete set of available values for the respective Signal Y
> -		configuration are listed in this file.
> -
>  What:		/sys/bus/counter/devices/counterX/signalY/name
>  KernelVersion:	5.2
>  Contact:	linux-iio@vger.kernel.org

Re: [PATCH 6/8] docs: counter: add latch_mode and latched_count sysfs attributes

[William Breathitt Gray]

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

On Sat, Oct 16, 2021 at 08:33:41PM -0500, David Lechner wrote:
> This documents new counterX/latch_mode* and
> counterX/countY/latched_count* attributes.
> 
> The counterX/signalY/*_available are moved to the
> counterX/countY/*_available section similar to the way we already have
> The counterX/*_component_id and The counterX/signalY/*_component_id
> grouped together so that we don't have to start a 3rd redundant section
> for device-level *_available section.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

Please separate these two distinct changes into two distinct patches.

> ---
>  Documentation/ABI/testing/sysfs-bus-counter | 39 ++++++++++++++++-----
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 37d960a8cb1b..78bb1a501007 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -59,10 +59,13 @@ What:		/sys/bus/counter/devices/counterX/countY/error_noise_available
>  What:		/sys/bus/counter/devices/counterX/countY/function_available
>  What:		/sys/bus/counter/devices/counterX/countY/prescaler_available
>  What:		/sys/bus/counter/devices/counterX/countY/signalZ_action_available
> +What:		/sys/bus/counter/devices/counterX/latch_mode_available
> +What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
> +What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
>  KernelVersion:	5.2
>  Contact:	linux-iio@vger.kernel.org
>  Description:
> -		Discrete set of available values for the respective Count Y
> +		Discrete set of available values for the respective component
>  		configuration are listed in this file. Values are delimited by
>  		newline characters.
>  
> @@ -147,6 +150,14 @@ Description:
>  			updates	the respective count. Quadrature encoding
>  			determines the direction.
>  
> +What:		/sys/bus/counter/devices/counterX/countY/latched_count
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Latched count data of Count Y represented as a string. The value
> +		is latched in based on the trigger selected by the
> +		counterX/latch_mode attribute.
> +

Latches are pretty common components of devices, and not simply limited
to latching the count data. I wonder if it would be better to omit the
"_count" suffix in order to make this more general. Well, the name
"latched_count" is suitable for counters so you probably don't need to
change it, but it's something to think about in the future.

>  What:		/sys/bus/counter/devices/counterX/countY/name
>  KernelVersion:	5.2
>  Contact:	linux-iio@vger.kernel.org
> @@ -209,6 +220,7 @@ What:		/sys/bus/counter/devices/counterX/countY/count_mode_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/direction_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/enable_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/error_noise_component_id
> +What:		/sys/bus/counter/devices/counterX/countY/latched_count_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/prescaler_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/preset_component_id
>  What:		/sys/bus/counter/devices/counterX/countY/preset_enable_component_id
> @@ -218,6 +230,7 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
> +What:		/sys/bus/counter/devices/latch_mode_component_id
>  What:		/sys/bus/counter/devices/unit_timer_enable_component_id
>  What:		/sys/bus/counter/devices/unit_timer_period_component_id
>  What:		/sys/bus/counter/devices/unit_timer_time_component_id
> @@ -244,6 +257,22 @@ Description:
>  		counter_event data structures. The number of elements will be
>  		rounded-up to a power of 2.
>  
> +What:		/sys/bus/counter/devices/counterX/latch_mode
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write attribute that selects the trigger for latching
> +		values. Valid values are device-specific (given by
> +		latch_mode_available attribute) and may include:
> +
> +		"Read count":
> +			Reading the countY/count attribute latches values.
> +
> +		"Unit timeout":
> +			Unit timer timeout event latches values.
> +
> +		The latched values can be read from latched_* attributes.
> +

To make these modes more generic for use in future drivers, I suggest
removing the "Unit " prefix and just leaving that mode as "Timeout". In
a similar vein, rewording "Read count" to "Count read" would make this
mode easier to understand in case a future driver introduces a mode
called "Signal read" or similar.

William Breathitt Gray

>  What:		/sys/bus/counter/devices/counterX/name
>  KernelVersion:	5.2
>  Contact:	linux-iio@vger.kernel.org
> @@ -298,14 +327,6 @@ Description:
>  		Active level of index input Signal Y; irrelevant in
>  		non-synchronous load mode.
>  
> -What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
> -What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
> -KernelVersion:	5.2
> -Contact:	linux-iio@vger.kernel.org
> -Description:
> -		Discrete set of available values for the respective Signal Y
> -		configuration are listed in this file.
> -
>  What:		/sys/bus/counter/devices/counterX/signalY/name
>  KernelVersion:	5.2
>  Contact:	linux-iio@vger.kernel.org
> -- 
> 2.25.1
> 

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

Re: [PATCH 6/8] docs: counter: add latch_mode and latched_count sysfs attributes

[David Lechner]

On 10/27/21 2:54 AM, William Breathitt Gray wrote:
> On Sat, Oct 16, 2021 at 08:33:41PM -0500, David Lechner wrote:
>> This documents new counterX/latch_mode* and
>> counterX/countY/latched_count* attributes.
>>
>> The counterX/signalY/*_available are moved to the
>> counterX/countY/*_available section similar to the way we already have
>> The counterX/*_component_id and The counterX/signalY/*_component_id
>> grouped together so that we don't have to start a 3rd redundant section
>> for device-level *_available section.
>>



>> @@ -147,6 +150,14 @@ Description:
>>   			updates	the respective count. Quadrature encoding
>>   			determines the direction.
>>   
>> +What:		/sys/bus/counter/devices/counterX/countY/latched_count
>> +KernelVersion:	5.16
>> +Contact:	linux-iio@vger.kernel.org
>> +Description:
>> +		Latched count data of Count Y represented as a string. The value
>> +		is latched in based on the trigger selected by the
>> +		counterX/latch_mode attribute.
>> +
> 
> Latches are pretty common components of devices, and not simply limited
> to latching the count data. I wonder if it would be better to omit the
> "_count" suffix in order to make this more general. Well, the name
> "latched_count" is suitable for counters so you probably don't need to
> change it, but it's something to think about in the future.
> 

I chose the name counterX/countY/latched_count since we already have
counterX/countY/count to read the same (not latched) count. This
indicates that they are the same quantity, just from a different
point in time.

Also for consideration, this particular hardware actually has 3
independent latched counts. One is triggered by the selected
latched_mode. One is triggered by the index signal and one is
triggered by the strobe signal.

The latter two are not implemented in this series, but if there were a
use for those, I would probably submit attributes index_latched_count
and strobe_latched_count. These are unaffected by the latch_mode.

Similarly, the unit timer has a timer latch and a period latch. If we
change the unit timer to be a Count as suggested, then the latched
timer would basically be the same as latched_count. Both of these
are triggered by the selected latch_mode.

So, I supposed if we wanted to keep things really generic, we would
want to introduce some sort of "latch trigger" component (synapse?).
There could theoretically be multiple configurable triggers, so
the proposed latch_mode might need to be made indexed or part of
an index component/extension.



>>   What:		/sys/bus/counter/devices/counterX/countY/name
>>   KernelVersion:	5.2
>>   Contact:	linux-iio@vger.kernel.org
>> @@ -209,6 +220,7 @@ What:		/sys/bus/counter/devices/counterX/countY/count_mode_component_id
>>   What:		/sys/bus/counter/devices/counterX/countY/direction_component_id
>>   What:		/sys/bus/counter/devices/counterX/countY/enable_component_id
>>   What:		/sys/bus/counter/devices/counterX/countY/error_noise_component_id
>> +What:		/sys/bus/counter/devices/counterX/countY/latched_count_component_id
>>   What:		/sys/bus/counter/devices/counterX/countY/prescaler_component_id
>>   What:		/sys/bus/counter/devices/counterX/countY/preset_component_id
>>   What:		/sys/bus/counter/devices/counterX/countY/preset_enable_component_id
>> @@ -218,6 +230,7 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
>>   What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
>>   What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
>>   What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
>> +What:		/sys/bus/counter/devices/latch_mode_component_id
>>   What:		/sys/bus/counter/devices/unit_timer_enable_component_id
>>   What:		/sys/bus/counter/devices/unit_timer_period_component_id
>>   What:		/sys/bus/counter/devices/unit_timer_time_component_id

Just noticing here, I missed the counterX in the device-level components.

>> @@ -244,6 +257,22 @@ Description:
>>   		counter_event data structures. The number of elements will be
>>   		rounded-up to a power of 2.
>>   
>> +What:		/sys/bus/counter/devices/counterX/latch_mode
>> +KernelVersion:	5.16
>> +Contact:	linux-iio@vger.kernel.org
>> +Description:
>> +		Read/write attribute that selects the trigger for latching
>> +		values. Valid values are device-specific (given by
>> +		latch_mode_available attribute) and may include:
>> +
>> +		"Read count":
>> +			Reading the countY/count attribute latches values.
>> +
>> +		"Unit timeout":
>> +			Unit timer timeout event latches values.
>> +
>> +		The latched values can be read from latched_* attributes.
>> +
> 
> To make these modes more generic for use in future drivers, I suggest
> removing the "Unit " prefix and just leaving that mode as "Timeout". In
> a similar vein, rewording "Read count" to "Count read" would make this
> mode easier to understand in case a future driver introduces a mode
> called "Signal read" or similar.
> 

Continuing my thoughts from above and taking this suggestion into
consideration...

Maybe we need a /sys/bus/counter/devices/counterX/latchY component.
This would represent the trigger for a latch. For the TI eQEP in this
series, there are potentially 3 of these (only one implemented for
now).

latchY would have a required `trigger` attribute that would describe
what triggers the latch. If the trigger is selectable, there would be
a `triggers_available` attribute that would list the possible triggers,
otherwise the `trigger` attribute would just be read-only. Available
triggers could could be "X read" where X is a fully qualified component
name, e.g. "Count0 count read" or a fully qualified event, e.g.
"Count1 overflow event" (this is unit timer timeout in generic counter
terms). But, there may be potential triggers that don't fit either
of these patterns.

Although not currently needed, the triggers for the index and strobe
latches on the eQEP get more interesting. The `triggers_available` for
the index latch are "index rising edge", "index falling edge" and
"software" (this would require a `software_trigger` attribute that
would be written to trigger the latch). The `triggers_available` for
the strobe latch are "strobe rising edge" and "strobe rising edge if
direction is clockwise and strobe falling edge if direction is
counterclockwise".

Circling back to the beginning, to read latched registers, there
would be attributes like counterX/countY/latchY_count instead of
the proposed counterX/countY/latched_count. So for the eQEP there
would be counter0/count0/latch0_count (triggered by reading
counter0/count0/count or counter0/count1 overflow event),
counter0/count0/latch1_count (triggered by index signal),
counter0/count0/latch2_count (triggered by strobe signal),
counter0/count1/latch0_count (unit timer latched timer trigger
by same trigger as counter0/count0/latch0_count) and
counter0/count0/latch0_ceiling (unit timer latched period
triggered by same trigger as counter0/count0/latch0_count).

Re: [PATCH 6/8] docs: counter: add latch_mode and latched_count sysfs attributes

[William Breathitt Gray]

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

On Wed, Oct 27, 2021 at 12:00:24PM -0500, David Lechner wrote:
> On 10/27/21 2:54 AM, William Breathitt Gray wrote:
> > On Sat, Oct 16, 2021 at 08:33:41PM -0500, David Lechner wrote:
> >> @@ -147,6 +150,14 @@ Description:
> >>   			updates	the respective count. Quadrature encoding
> >>   			determines the direction.
> >>   
> >> +What:		/sys/bus/counter/devices/counterX/countY/latched_count
> >> +KernelVersion:	5.16
> >> +Contact:	linux-iio@vger.kernel.org
> >> +Description:
> >> +		Latched count data of Count Y represented as a string. The value
> >> +		is latched in based on the trigger selected by the
> >> +		counterX/latch_mode attribute.
> >> +
> > 
> > Latches are pretty common components of devices, and not simply limited
> > to latching the count data. I wonder if it would be better to omit the
> > "_count" suffix in order to make this more general. Well, the name
> > "latched_count" is suitable for counters so you probably don't need to
> > change it, but it's something to think about in the future.
> > 
> 
> I chose the name counterX/countY/latched_count since we already have
> counterX/countY/count to read the same (not latched) count. This
> indicates that they are the same quantity, just from a different
> point in time.
> 
> Also for consideration, this particular hardware actually has 3
> independent latched counts. One is triggered by the selected
> latched_mode. One is triggered by the index signal and one is
> triggered by the strobe signal.
> 
> The latter two are not implemented in this series, but if there were a
> use for those, I would probably submit attributes index_latched_count
> and strobe_latched_count. These are unaffected by the latch_mode.
> 
> Similarly, the unit timer has a timer latch and a period latch. If we
> change the unit timer to be a Count as suggested, then the latched
> timer would basically be the same as latched_count. Both of these
> are triggered by the selected latch_mode.
> 
> So, I supposed if we wanted to keep things really generic, we would
> want to introduce some sort of "latch trigger" component (synapse?).
> There could theoretically be multiple configurable triggers, so
> the proposed latch_mode might need to be made indexed or part of
> an index component/extension.

Aside from deriving their latched values from the current and historical
count values, these latches don't seem to be related to Counters in an
operational sense; i.e. they don't really fit into the Counter subsystem
paradigm because they aren't functionally counters, but rather just use
the count values here as source data for their own operations. As such,
I'm not sure yet if they really belong in the Counter subsystem or
somewhere else in the IIO subsystem.

> 
> >>   What:		/sys/bus/counter/devices/counterX/countY/name
> >>   KernelVersion:	5.2
> >>   Contact:	linux-iio@vger.kernel.org
> >> @@ -209,6 +220,7 @@ What:		/sys/bus/counter/devices/counterX/countY/count_mode_component_id
> >>   What:		/sys/bus/counter/devices/counterX/countY/direction_component_id
> >>   What:		/sys/bus/counter/devices/counterX/countY/enable_component_id
> >>   What:		/sys/bus/counter/devices/counterX/countY/error_noise_component_id
> >> +What:		/sys/bus/counter/devices/counterX/countY/latched_count_component_id
> >>   What:		/sys/bus/counter/devices/counterX/countY/prescaler_component_id
> >>   What:		/sys/bus/counter/devices/counterX/countY/preset_component_id
> >>   What:		/sys/bus/counter/devices/counterX/countY/preset_enable_component_id
> >> @@ -218,6 +230,7 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
> >>   What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
> >>   What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
> >>   What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
> >> +What:		/sys/bus/counter/devices/latch_mode_component_id
> >>   What:		/sys/bus/counter/devices/unit_timer_enable_component_id
> >>   What:		/sys/bus/counter/devices/unit_timer_period_component_id
> >>   What:		/sys/bus/counter/devices/unit_timer_time_component_id
> 
> Just noticing here, I missed the counterX in the device-level components.
> 
> >> @@ -244,6 +257,22 @@ Description:
> >>   		counter_event data structures. The number of elements will be
> >>   		rounded-up to a power of 2.
> >>   
> >> +What:		/sys/bus/counter/devices/counterX/latch_mode
> >> +KernelVersion:	5.16
> >> +Contact:	linux-iio@vger.kernel.org
> >> +Description:
> >> +		Read/write attribute that selects the trigger for latching
> >> +		values. Valid values are device-specific (given by
> >> +		latch_mode_available attribute) and may include:
> >> +
> >> +		"Read count":
> >> +			Reading the countY/count attribute latches values.
> >> +
> >> +		"Unit timeout":
> >> +			Unit timer timeout event latches values.
> >> +
> >> +		The latched values can be read from latched_* attributes.
> >> +
> > 
> > To make these modes more generic for use in future drivers, I suggest
> > removing the "Unit " prefix and just leaving that mode as "Timeout". In
> > a similar vein, rewording "Read count" to "Count read" would make this
> > mode easier to understand in case a future driver introduces a mode
> > called "Signal read" or similar.
> > 
> 
> Continuing my thoughts from above and taking this suggestion into
> consideration...
> 
> Maybe we need a /sys/bus/counter/devices/counterX/latchY component.
> This would represent the trigger for a latch. For the TI eQEP in this
> series, there are potentially 3 of these (only one implemented for
> now).
> 
> latchY would have a required `trigger` attribute that would describe
> what triggers the latch. If the trigger is selectable, there would be
> a `triggers_available` attribute that would list the possible triggers,
> otherwise the `trigger` attribute would just be read-only. Available
> triggers could could be "X read" where X is a fully qualified component
> name, e.g. "Count0 count read" or a fully qualified event, e.g.
> "Count1 overflow event" (this is unit timer timeout in generic counter
> terms). But, there may be potential triggers that don't fit either
> of these patterns.
> 
> Although not currently needed, the triggers for the index and strobe
> latches on the eQEP get more interesting. The `triggers_available` for
> the index latch are "index rising edge", "index falling edge" and
> "software" (this would require a `software_trigger` attribute that
> would be written to trigger the latch). The `triggers_available` for
> the strobe latch are "strobe rising edge" and "strobe rising edge if
> direction is clockwise and strobe falling edge if direction is
> counterclockwise".
> 
> Circling back to the beginning, to read latched registers, there
> would be attributes like counterX/countY/latchY_count instead of
> the proposed counterX/countY/latched_count. So for the eQEP there
> would be counter0/count0/latch0_count (triggered by reading
> counter0/count0/count or counter0/count1 overflow event),
> counter0/count0/latch1_count (triggered by index signal),
> counter0/count0/latch2_count (triggered by strobe signal),
> counter0/count1/latch0_count (unit timer latched timer trigger
> by same trigger as counter0/count0/latch0_count) and
> counter0/count0/latch0_ceiling (unit timer latched period
> triggered by same trigger as counter0/count0/latch0_count).

The complexity of configuration here is a good indication that these
latches deserve their own tree structure as you suggest. Furthermore, we
see that there at least three of these latches available for this
particular device, so just a single "latch_count" or similar will not be
sufficient -- enumeration of the form /sys/bus/../latchY or similar
would be prudent.

Jonathan, perhaps you have some insight here. From a functional aspect,
latches are not unique to counter devices, so I wonder if the IIO
subsytem has already encountered similar functionality amongst its
drivers. Essentially, a latch is just a memory buffer provided by the
device.

For the TI eQEP device here, the buffer for each latch provides a single
read-only value that is updated by the device; the update behavior can
be configured by respective control registers. However, it's not so
far-fetched to assume that there a other devices out that that have
buffers spanning multiple latched values storing historical data.

Because a latch can theoretically provide any sort of data, not
necessary count values, it seems reasonable that supporting latches
would involve their own interface independent of the Counter paradigm.
How that interface looks is the question. Should the TI eQEP latches
here be exposed through some sort of generic latches interface, or
would it be better to have a more abstract representation of what these
latches are for; e.g. if these latches are used to measure speed, then
some sort of IIO speedometer interface might be appropriate).

William Breathitt Gray

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

Re: [PATCH 6/8] docs: counter: add latch_mode and latched_count sysfs attributes

[Jonathan Cameron]

On Sat, 30 Oct 2021 10:32:57 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Wed, Oct 27, 2021 at 12:00:24PM -0500, David Lechner wrote:
> > On 10/27/21 2:54 AM, William Breathitt Gray wrote:  
> > > On Sat, Oct 16, 2021 at 08:33:41PM -0500, David Lechner wrote:  
> > >> @@ -147,6 +150,14 @@ Description:
> > >>   			updates	the respective count. Quadrature encoding
> > >>   			determines the direction.
> > >>   
> > >> +What:		/sys/bus/counter/devices/counterX/countY/latched_count
> > >> +KernelVersion:	5.16
> > >> +Contact:	linux-iio@vger.kernel.org
> > >> +Description:
> > >> +		Latched count data of Count Y represented as a string. The value
> > >> +		is latched in based on the trigger selected by the
> > >> +		counterX/latch_mode attribute.
> > >> +  
> > > 
> > > Latches are pretty common components of devices, and not simply limited
> > > to latching the count data. I wonder if it would be better to omit the
> > > "_count" suffix in order to make this more general. Well, the name
> > > "latched_count" is suitable for counters so you probably don't need to
> > > change it, but it's something to think about in the future.
> > >   
> > 
> > I chose the name counterX/countY/latched_count since we already have
> > counterX/countY/count to read the same (not latched) count. This
> > indicates that they are the same quantity, just from a different
> > point in time.
> > 
> > Also for consideration, this particular hardware actually has 3
> > independent latched counts. One is triggered by the selected
> > latched_mode. One is triggered by the index signal and one is
> > triggered by the strobe signal.
> > 
> > The latter two are not implemented in this series, but if there were a
> > use for those, I would probably submit attributes index_latched_count
> > and strobe_latched_count. These are unaffected by the latch_mode.
> > 
> > Similarly, the unit timer has a timer latch and a period latch. If we
> > change the unit timer to be a Count as suggested, then the latched
> > timer would basically be the same as latched_count. Both of these
> > are triggered by the selected latch_mode.
> > 
> > So, I supposed if we wanted to keep things really generic, we would
> > want to introduce some sort of "latch trigger" component (synapse?).
> > There could theoretically be multiple configurable triggers, so
> > the proposed latch_mode might need to be made indexed or part of
> > an index component/extension.  
> 
> Aside from deriving their latched values from the current and historical
> count values, these latches don't seem to be related to Counters in an
> operational sense; i.e. they don't really fit into the Counter subsystem
> paradigm because they aren't functionally counters, but rather just use
> the count values here as source data for their own operations. As such,
> I'm not sure yet if they really belong in the Counter subsystem or
> somewhere else in the IIO subsystem.

In this particular case I think we are talking about latching counts rather
than something else?  So one event happens and we latch the count at that
point.

The IIO equivalent is a trigger event driving data into a buffer.
There are a few examples of this though it's pretty rare.
The most general corner case is probably what we see with impact sensors.
In those cases we have data captured around an event (rather than a single
latched value).

They are rather complex beasts but the best we've managed is a special
trigger used only with that device and some control attributes to say what
is captured when the trigger fires.  Note this is a stetch in IIO because
normally triggers are one per sample...

> 
> >   
> > >>   What:		/sys/bus/counter/devices/counterX/countY/name
> > >>   KernelVersion:	5.2
> > >>   Contact:	linux-iio@vger.kernel.org
> > >> @@ -209,6 +220,7 @@ What:		/sys/bus/counter/devices/counterX/countY/count_mode_component_id
> > >>   What:		/sys/bus/counter/devices/counterX/countY/direction_component_id
> > >>   What:		/sys/bus/counter/devices/counterX/countY/enable_component_id
> > >>   What:		/sys/bus/counter/devices/counterX/countY/error_noise_component_id
> > >> +What:		/sys/bus/counter/devices/counterX/countY/latched_count_component_id
> > >>   What:		/sys/bus/counter/devices/counterX/countY/prescaler_component_id
> > >>   What:		/sys/bus/counter/devices/counterX/countY/preset_component_id
> > >>   What:		/sys/bus/counter/devices/counterX/countY/preset_enable_component_id
> > >> @@ -218,6 +230,7 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
> > >>   What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
> > >>   What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
> > >>   What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
> > >> +What:		/sys/bus/counter/devices/latch_mode_component_id
> > >>   What:		/sys/bus/counter/devices/unit_timer_enable_component_id
> > >>   What:		/sys/bus/counter/devices/unit_timer_period_component_id
> > >>   What:		/sys/bus/counter/devices/unit_timer_time_component_id  
> > 
> > Just noticing here, I missed the counterX in the device-level components.
> >   
> > >> @@ -244,6 +257,22 @@ Description:
> > >>   		counter_event data structures. The number of elements will be
> > >>   		rounded-up to a power of 2.
> > >>   
> > >> +What:		/sys/bus/counter/devices/counterX/latch_mode
> > >> +KernelVersion:	5.16
> > >> +Contact:	linux-iio@vger.kernel.org
> > >> +Description:
> > >> +		Read/write attribute that selects the trigger for latching
> > >> +		values. Valid values are device-specific (given by
> > >> +		latch_mode_available attribute) and may include:
> > >> +
> > >> +		"Read count":
> > >> +			Reading the countY/count attribute latches values.
> > >> +
> > >> +		"Unit timeout":
> > >> +			Unit timer timeout event latches values.
> > >> +
> > >> +		The latched values can be read from latched_* attributes.
> > >> +  
> > > 
> > > To make these modes more generic for use in future drivers, I suggest
> > > removing the "Unit " prefix and just leaving that mode as "Timeout". In
> > > a similar vein, rewording "Read count" to "Count read" would make this
> > > mode easier to understand in case a future driver introduces a mode
> > > called "Signal read" or similar.
> > >   
> > 
> > Continuing my thoughts from above and taking this suggestion into
> > consideration...
> > 
> > Maybe we need a /sys/bus/counter/devices/counterX/latchY component.
> > This would represent the trigger for a latch. For the TI eQEP in this
> > series, there are potentially 3 of these (only one implemented for
> > now).
> > 
> > latchY would have a required `trigger` attribute that would describe
> > what triggers the latch. If the trigger is selectable, there would be
> > a `triggers_available` attribute that would list the possible triggers,
> > otherwise the `trigger` attribute would just be read-only. Available
> > triggers could could be "X read" where X is a fully qualified component
> > name, e.g. "Count0 count read" or a fully qualified event, e.g.
> > "Count1 overflow event" (this is unit timer timeout in generic counter
> > terms). But, there may be potential triggers that don't fit either
> > of these patterns.
> > 
> > Although not currently needed, the triggers for the index and strobe
> > latches on the eQEP get more interesting. The `triggers_available` for
> > the index latch are "index rising edge", "index falling edge" and
> > "software" (this would require a `software_trigger` attribute that
> > would be written to trigger the latch). The `triggers_available` for
> > the strobe latch are "strobe rising edge" and "strobe rising edge if
> > direction is clockwise and strobe falling edge if direction is
> > counterclockwise".
> > 
> > Circling back to the beginning, to read latched registers, there
> > would be attributes like counterX/countY/latchY_count instead of
> > the proposed counterX/countY/latched_count. So for the eQEP there
> > would be counter0/count0/latch0_count (triggered by reading
> > counter0/count0/count or counter0/count1 overflow event),
> > counter0/count0/latch1_count (triggered by index signal),
> > counter0/count0/latch2_count (triggered by strobe signal),
> > counter0/count1/latch0_count (unit timer latched timer trigger
> > by same trigger as counter0/count0/latch0_count) and
> > counter0/count0/latch0_ceiling (unit timer latched period
> > triggered by same trigger as counter0/count0/latch0_count).  
> 
> The complexity of configuration here is a good indication that these
> latches deserve their own tree structure as you suggest. Furthermore, we
> see that there at least three of these latches available for this
> particular device, so just a single "latch_count" or similar will not be
> sufficient -- enumeration of the form /sys/bus/../latchY or similar
> would be prudent.
> 
> Jonathan, perhaps you have some insight here. From a functional aspect,
> latches are not unique to counter devices, so I wonder if the IIO
> subsytem has already encountered similar functionality amongst its
> drivers. Essentially, a latch is just a memory buffer provided by the
> device.

As mentioned above, they exist but are fairly rare, unless you think
of time triggers as being in this category in which case any data
ready signal is basically like this.  Ignoring that common case,
we map them onto a device specific trigger (one that has a
validate_device callback to check it's being assigned to that right device).
Another case where we do odd things like this is SoC ADCs that support touch
screen type functionality.  In those case we are grabbing data only when
the screen is touched.


> 
> For the TI eQEP device here, the buffer for each latch provides a single
> read-only value that is updated by the device; the update behavior can
> be configured by respective control registers. However, it's not so
> far-fetched to assume that there a other devices out that that have
> buffers spanning multiple latched values storing historical data.

A fifo filled on 'index' event or similar would indeed be a reasonable
bit of hardware to build.  I've world with PLC code that does this sort
of things so the requirements are there (tracking products on a conveyor
belt would be a classic case - you latch the count when a light gate is
broken).

> 
> Because a latch can theoretically provide any sort of data, not
> necessary count values, it seems reasonable that supporting latches
> would involve their own interface independent of the Counter paradigm.
> How that interface looks is the question. Should the TI eQEP latches
> here be exposed through some sort of generic latches interface, or
> would it be better to have a more abstract representation of what these
> latches are for; e.g. if these latches are used to measure speed, then
> some sort of IIO speedometer interface might be appropriate).

I'd suggest trying to avoid being too generic about this.  There might
be some reason to do it in the future but trying to define interfaces
across subsystems is a pain.  More likely we'll get some bridging
type drivers to map between different abstractions if they are needed.

Jonathan

> 
> William Breathitt Gray

Re: [PATCH 6/8] docs: counter: add latch_mode and latched_count sysfs attributes

[William Breathitt Gray]

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

On Sat, Oct 30, 2021 at 03:39:39PM +0100, Jonathan Cameron wrote:
> On Sat, 30 Oct 2021 10:32:57 +0900
> William Breathitt Gray <vilhelm.gray@gmail.com> wrote:
> 
> > On Wed, Oct 27, 2021 at 12:00:24PM -0500, David Lechner wrote:
> > > On 10/27/21 2:54 AM, William Breathitt Gray wrote:  
> > > > On Sat, Oct 16, 2021 at 08:33:41PM -0500, David Lechner wrote:  
> > > >> @@ -147,6 +150,14 @@ Description:
> > > >>   			updates	the respective count. Quadrature encoding
> > > >>   			determines the direction.
> > > >>   
> > > >> +What:		/sys/bus/counter/devices/counterX/countY/latched_count
> > > >> +KernelVersion:	5.16
> > > >> +Contact:	linux-iio@vger.kernel.org
> > > >> +Description:
> > > >> +		Latched count data of Count Y represented as a string. The value
> > > >> +		is latched in based on the trigger selected by the
> > > >> +		counterX/latch_mode attribute.
> > > >> +  
> > > > 
> > > > Latches are pretty common components of devices, and not simply limited
> > > > to latching the count data. I wonder if it would be better to omit the
> > > > "_count" suffix in order to make this more general. Well, the name
> > > > "latched_count" is suitable for counters so you probably don't need to
> > > > change it, but it's something to think about in the future.
> > > >   
> > > 
> > > I chose the name counterX/countY/latched_count since we already have
> > > counterX/countY/count to read the same (not latched) count. This
> > > indicates that they are the same quantity, just from a different
> > > point in time.
> > > 
> > > Also for consideration, this particular hardware actually has 3
> > > independent latched counts. One is triggered by the selected
> > > latched_mode. One is triggered by the index signal and one is
> > > triggered by the strobe signal.
> > > 
> > > The latter two are not implemented in this series, but if there were a
> > > use for those, I would probably submit attributes index_latched_count
> > > and strobe_latched_count. These are unaffected by the latch_mode.
> > > 
> > > Similarly, the unit timer has a timer latch and a period latch. If we
> > > change the unit timer to be a Count as suggested, then the latched
> > > timer would basically be the same as latched_count. Both of these
> > > are triggered by the selected latch_mode.
> > > 
> > > So, I supposed if we wanted to keep things really generic, we would
> > > want to introduce some sort of "latch trigger" component (synapse?).
> > > There could theoretically be multiple configurable triggers, so
> > > the proposed latch_mode might need to be made indexed or part of
> > > an index component/extension.  
> > 
> > Aside from deriving their latched values from the current and historical
> > count values, these latches don't seem to be related to Counters in an
> > operational sense; i.e. they don't really fit into the Counter subsystem
> > paradigm because they aren't functionally counters, but rather just use
> > the count values here as source data for their own operations. As such,
> > I'm not sure yet if they really belong in the Counter subsystem or
> > somewhere else in the IIO subsystem.
> 
> In this particular case I think we are talking about latching counts rather
> than something else?  So one event happens and we latch the count at that
> point.

It looks like the unit timer has latches for its time and period values
as well IIUC, but at this patch only implements support for the count
latch. I think if we were to implement support for these other latches,
we would probably expose them in a similar way and just call them
respectively latched_count, latched_period, etc.

> The IIO equivalent is a trigger event driving data into a buffer.
> There are a few examples of this though it's pretty rare.
> The most general corner case is probably what we see with impact sensors.
> In those cases we have data captured around an event (rather than a single
> latched value).
> 
> They are rather complex beasts but the best we've managed is a special
> trigger used only with that device and some control attributes to say what
> is captured when the trigger fires.  Note this is a stetch in IIO because
> normally triggers are one per sample...

Yes, complexity is something I would like to avoid. I don't want to
shoehorn functionality into the Counter subsystem if a simpler approach
is possible. However, it may be good news to hear that these kind of
complex devices are rarer, so a simple approach with the TI eQEP might
be the best option.

> > 
> > >   
> > > >>   What:		/sys/bus/counter/devices/counterX/countY/name
> > > >>   KernelVersion:	5.2
> > > >>   Contact:	linux-iio@vger.kernel.org
> > > >> @@ -209,6 +220,7 @@ What:		/sys/bus/counter/devices/counterX/countY/count_mode_component_id
> > > >>   What:		/sys/bus/counter/devices/counterX/countY/direction_component_id
> > > >>   What:		/sys/bus/counter/devices/counterX/countY/enable_component_id
> > > >>   What:		/sys/bus/counter/devices/counterX/countY/error_noise_component_id
> > > >> +What:		/sys/bus/counter/devices/counterX/countY/latched_count_component_id
> > > >>   What:		/sys/bus/counter/devices/counterX/countY/prescaler_component_id
> > > >>   What:		/sys/bus/counter/devices/counterX/countY/preset_component_id
> > > >>   What:		/sys/bus/counter/devices/counterX/countY/preset_enable_component_id
> > > >> @@ -218,6 +230,7 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
> > > >>   What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
> > > >>   What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
> > > >>   What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
> > > >> +What:		/sys/bus/counter/devices/latch_mode_component_id
> > > >>   What:		/sys/bus/counter/devices/unit_timer_enable_component_id
> > > >>   What:		/sys/bus/counter/devices/unit_timer_period_component_id
> > > >>   What:		/sys/bus/counter/devices/unit_timer_time_component_id  
> > > 
> > > Just noticing here, I missed the counterX in the device-level components.
> > >   
> > > >> @@ -244,6 +257,22 @@ Description:
> > > >>   		counter_event data structures. The number of elements will be
> > > >>   		rounded-up to a power of 2.
> > > >>   
> > > >> +What:		/sys/bus/counter/devices/counterX/latch_mode
> > > >> +KernelVersion:	5.16
> > > >> +Contact:	linux-iio@vger.kernel.org
> > > >> +Description:
> > > >> +		Read/write attribute that selects the trigger for latching
> > > >> +		values. Valid values are device-specific (given by
> > > >> +		latch_mode_available attribute) and may include:

By the way, the latch_mode_available comment here can be removed as it's
already obvious the respective *_available sysfs attribute is where the
possible values are given.

> > > >> +
> > > >> +		"Read count":
> > > >> +			Reading the countY/count attribute latches values.
> > > >> +
> > > >> +		"Unit timeout":
> > > >> +			Unit timer timeout event latches values.
> > > >> +
> > > >> +		The latched values can be read from latched_* attributes.
> > > >> +  
> > > > 
> > > > To make these modes more generic for use in future drivers, I suggest
> > > > removing the "Unit " prefix and just leaving that mode as "Timeout". In
> > > > a similar vein, rewording "Read count" to "Count read" would make this
> > > > mode easier to understand in case a future driver introduces a mode
> > > > called "Signal read" or similar.
> > > >   
> > > 
> > > Continuing my thoughts from above and taking this suggestion into
> > > consideration...
> > > 
> > > Maybe we need a /sys/bus/counter/devices/counterX/latchY component.
> > > This would represent the trigger for a latch. For the TI eQEP in this
> > > series, there are potentially 3 of these (only one implemented for
> > > now).
> > > 
> > > latchY would have a required `trigger` attribute that would describe
> > > what triggers the latch. If the trigger is selectable, there would be
> > > a `triggers_available` attribute that would list the possible triggers,
> > > otherwise the `trigger` attribute would just be read-only. Available
> > > triggers could could be "X read" where X is a fully qualified component
> > > name, e.g. "Count0 count read" or a fully qualified event, e.g.
> > > "Count1 overflow event" (this is unit timer timeout in generic counter
> > > terms). But, there may be potential triggers that don't fit either
> > > of these patterns.
> > > 
> > > Although not currently needed, the triggers for the index and strobe
> > > latches on the eQEP get more interesting. The `triggers_available` for
> > > the index latch are "index rising edge", "index falling edge" and
> > > "software" (this would require a `software_trigger` attribute that
> > > would be written to trigger the latch). The `triggers_available` for
> > > the strobe latch are "strobe rising edge" and "strobe rising edge if
> > > direction is clockwise and strobe falling edge if direction is
> > > counterclockwise".
> > > 
> > > Circling back to the beginning, to read latched registers, there
> > > would be attributes like counterX/countY/latchY_count instead of
> > > the proposed counterX/countY/latched_count. So for the eQEP there
> > > would be counter0/count0/latch0_count (triggered by reading
> > > counter0/count0/count or counter0/count1 overflow event),
> > > counter0/count0/latch1_count (triggered by index signal),
> > > counter0/count0/latch2_count (triggered by strobe signal),
> > > counter0/count1/latch0_count (unit timer latched timer trigger
> > > by same trigger as counter0/count0/latch0_count) and
> > > counter0/count0/latch0_ceiling (unit timer latched period
> > > triggered by same trigger as counter0/count0/latch0_count).  
> > 
> > The complexity of configuration here is a good indication that these
> > latches deserve their own tree structure as you suggest. Furthermore, we
> > see that there at least three of these latches available for this
> > particular device, so just a single "latch_count" or similar will not be
> > sufficient -- enumeration of the form /sys/bus/../latchY or similar
> > would be prudent.
> > 
> > Jonathan, perhaps you have some insight here. From a functional aspect,
> > latches are not unique to counter devices, so I wonder if the IIO
> > subsytem has already encountered similar functionality amongst its
> > drivers. Essentially, a latch is just a memory buffer provided by the
> > device.
> 
> As mentioned above, they exist but are fairly rare, unless you think
> of time triggers as being in this category in which case any data
> ready signal is basically like this.  Ignoring that common case,
> we map them onto a device specific trigger (one that has a
> validate_device callback to check it's being assigned to that right device).
> Another case where we do odd things like this is SoC ADCs that support touch
> screen type functionality.  In those case we are grabbing data only when
> the screen is touched.
> 
> 
> > 
> > For the TI eQEP device here, the buffer for each latch provides a single
> > read-only value that is updated by the device; the update behavior can
> > be configured by respective control registers. However, it's not so
> > far-fetched to assume that there a other devices out that that have
> > buffers spanning multiple latched values storing historical data.
> 
> A fifo filled on 'index' event or similar would indeed be a reasonable
> bit of hardware to build.  I've world with PLC code that does this sort
> of things so the requirements are there (tracking products on a conveyor
> belt would be a classic case - you latch the count when a light gate is
> broken).

I've constructed similar devices where I push the current count to a
fifo to track a part's relative position to others on a conveyor belt.
It's possible some quadrature encoding device out has a similar
functionality, but I don't want to jump the gun and start developing an
interface for this if those kinds of device are so rare as to not be
worth the effort.

> > Because a latch can theoretically provide any sort of data, not
> > necessary count values, it seems reasonable that supporting latches
> > would involve their own interface independent of the Counter paradigm.
> > How that interface looks is the question. Should the TI eQEP latches
> > here be exposed through some sort of generic latches interface, or
> > would it be better to have a more abstract representation of what these
> > latches are for; e.g. if these latches are used to measure speed, then
> > some sort of IIO speedometer interface might be appropriate).
> 
> I'd suggest trying to avoid being too generic about this.  There might
> be some reason to do it in the future but trying to define interfaces
> across subsystems is a pain.  More likely we'll get some bridging
> type drivers to map between different abstractions if they are needed.
> 
> Jonathan

I agree, after considering this some more I think it's best to keep this
specific to the use case of the TI eQEP for now. If something more
complex appears in the future then we can address the possibility of a
deadicated interface, but for the time being I think simple extensions
to the Counter subsystem will suffice.

Instead of treating each latch with its own enumerated tree structure,
let's go with the simple approach taken initially here:

	* counterX/countY/latched_count
	* counterX/latch_mode

If desired, a latched_time can be implemented as well; the edge capture
attributes can be exposed in a similar fashion (e.g. latched_period).

Whether latch_mode belongs as a device extension or Count extension is
something I'm still contemplating. If we expose the unit timer under its
own counterX/countY, then latch_mode should be a device extension
because latched_time would simply become latched_count for the unit
timer. On the other hand, if unit timer is not exposed like that, then
perhaps it makes more sense to move latch_mode to counterX/countY, and
introduce latched_time and latched_period under there as well.

William Breathitt Gray

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

[PATCH 7/8] counter/ti-eqep: add support for edge capture unit

[David Lechner]

This adds support for the Edge Capture Unit to the TI eQEP counter
driver. This just adds the minimum required features to measure speed
using the Unit Timer and the Edge Capture unit. Additional features can
be added in the future if needed.

This adds 4 new device-level attributes:
- edge_capture_unit_prescaler: selects a prescalar for the Counter count
	coming into the Edge Capture Unit
- edge_capture_unit_max_period: selects the max time period that can be
	measured by the Edge Capture Unit
- edge_capture_unit_latched_period: gets the period that was measured
	when the event selected by the latch_mode attribute is triggered
- edge_capture_unit_enable: enables or disables the Edge Capture Unit

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/counter/ti-eqep.c | 150 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 150 insertions(+)

diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
index ef899655ad1d..fb1f4d0b4cde 100644
--- a/drivers/counter/ti-eqep.c
+++ b/drivers/counter/ti-eqep.c
@@ -116,6 +116,12 @@
 #define QEPSTS_FIMF		BIT(1)
 #define QEPSTS_PCEF		BIT(0)
 
+#define QCAPCTL_CEN		BIT(15)
+#define QCAPCTL_CCPS_SHIFT	4
+#define QCAPCTL_CCPS		GENMASK(6, 4)
+#define QCAPCTL_UPPS_SHIFT	0
+#define QCAPCTL_UPPS		GENMASK(3, 0)
+
 /* EQEP Inputs */
 enum {
 	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
@@ -479,6 +485,137 @@ static struct counter_count ti_eqep_counts[] = {
 	},
 };
 
+static int ti_eqep_edge_capture_unit_enable_read(struct counter_device *counter,
+						 u8 *value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qcapctl;
+
+	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
+	*value = !!(qcapctl & QCAPCTL_CEN);
+
+	return 0;
+}
+
+static int ti_eqep_edge_capture_unit_enable_write(struct counter_device *counter,
+						  u8 value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+
+	if (value)
+		regmap_set_bits(priv->regmap16, QCAPCTL, QCAPCTL_CEN);
+	else
+		regmap_clear_bits(priv->regmap16, QCAPCTL, QCAPCTL_CEN);
+
+	return 0;
+}
+
+static int
+ti_eqep_edge_capture_unit_latched_period_read(struct counter_device *counter,
+					      u64 *value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qcprdlat, qcapctl;
+	u8 ccps;
+
+	regmap_read(priv->regmap16, QCPRDLAT, &qcprdlat);
+	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
+	ccps = (qcapctl & QCAPCTL_CCPS) >> QCAPCTL_CCPS_SHIFT;
+
+	/* convert timer ticks to nanoseconds */
+	*value = mul_u64_u32_div(qcprdlat << ccps, NSEC_PER_SEC, priv->sysclkout_rate);
+
+	return 0;
+}
+
+static int
+ti_eqep_edge_capture_unit_max_period_read(struct counter_device *counter,
+					  u64 *value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qcapctl;
+	u8 ccps;
+
+	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
+	ccps = (qcapctl & QCAPCTL_CCPS) >> QCAPCTL_CCPS_SHIFT;
+
+	/* convert timer ticks to nanoseconds */
+	*value = mul_u64_u32_div(USHRT_MAX << ccps, NSEC_PER_SEC,
+				 priv->sysclkout_rate);
+
+	return 0;
+}
+
+static int
+ti_eqep_edge_capture_unit_max_period_write(struct counter_device *counter,
+					   u64 value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 period;
+	u8 ccps;
+
+	/* convert nanoseconds to timer ticks */
+	period = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
+	if (period != value)
+		return -ERANGE;
+
+	/* find the smallest divider that will fit the requested period */
+	for (ccps = 0; ccps <= 7; ccps++)
+		if (USHRT_MAX << ccps >= period)
+			break;
+
+	if (ccps > 7)
+		return -EINVAL;
+
+	regmap_write_bits(priv->regmap16, QCAPCTL, QCAPCTL_CCPS,
+			  ccps << QCAPCTL_CCPS_SHIFT);
+
+	return 0;
+}
+
+static int
+ti_eqep_edge_capture_unit_prescaler_read(struct counter_device *counter,
+					 u32 *value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+	u32 qcapctl;
+
+	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
+	*value = (qcapctl & QCAPCTL_UPPS) >> QCAPCTL_UPPS_SHIFT;
+
+	return 0;
+}
+
+static int
+ti_eqep_edge_capture_unit_prescaler_write(struct counter_device *counter,
+					  u32 value)
+{
+	struct ti_eqep_cnt *priv = counter->priv;
+
+	regmap_write_bits(priv->regmap16, QCAPCTL, QCAPCTL_UPPS,
+			  value << QCAPCTL_UPPS_SHIFT);
+
+	return 0;
+}
+
+static const char *const ti_eqep_edge_capture_unit_prescaler_values[] = {
+	"1",
+	"2",
+	"4",
+	"8",
+	"16",
+	"32",
+	"64",
+	"128",
+	"256",
+	"512",
+	"1024",
+	"2048",
+};
+
+static DEFINE_COUNTER_ENUM(ti_eqep_edge_capture_unit_prescaler_available,
+			   ti_eqep_edge_capture_unit_prescaler_values);
+
 static int ti_eqep_latch_mode_read(struct counter_device *counter,
 					    u32 *value)
 {
@@ -601,6 +738,19 @@ static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
 }
 
 static struct counter_comp ti_eqep_device_ext[] = {
+	COUNTER_COMP_DEVICE_BOOL("edge_capture_unit_enable",
+				 ti_eqep_edge_capture_unit_enable_read,
+				 ti_eqep_edge_capture_unit_enable_write),
+	COUNTER_COMP_DEVICE_U64("edge_capture_unit_latched_period",
+				ti_eqep_edge_capture_unit_latched_period_read,
+				NULL),
+	COUNTER_COMP_DEVICE_U64("edge_capture_unit_max_period",
+				ti_eqep_edge_capture_unit_max_period_read,
+				ti_eqep_edge_capture_unit_max_period_write),
+	COUNTER_COMP_DEVICE_ENUM("edge_capture_unit_prescaler",
+				 ti_eqep_edge_capture_unit_prescaler_read,
+				 ti_eqep_edge_capture_unit_prescaler_write,
+				 ti_eqep_edge_capture_unit_prescaler_available),
 	COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read,
 				ti_eqep_latch_mode_write, ti_eqep_latch_modes),
 	COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,
-- 
2.25.1

Re: [PATCH 7/8] counter/ti-eqep: add support for edge capture unit

[Jonathan Cameron]

On Sat, 16 Oct 2021 20:33:42 -0500
David Lechner <david@lechnology.com> wrote:

> This adds support for the Edge Capture Unit to the TI eQEP counter
> driver. This just adds the minimum required features to measure speed
> using the Unit Timer and the Edge Capture unit. Additional features can
> be added in the future if needed.
> 
> This adds 4 new device-level attributes:
> - edge_capture_unit_prescaler: selects a prescalar for the Counter count
> 	coming into the Edge Capture Unit
> - edge_capture_unit_max_period: selects the max time period that can be
> 	measured by the Edge Capture Unit
> - edge_capture_unit_latched_period: gets the period that was measured
> 	when the event selected by the latch_mode attribute is triggered
> - edge_capture_unit_enable: enables or disables the Edge Capture Unit
> 
> Signed-off-by: David Lechner <david@lechnology.com>

A general comment on using FIELD_GET and FIELD_PREP to avoid having to
define shifts twice.


> ---
>  drivers/counter/ti-eqep.c | 150 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index ef899655ad1d..fb1f4d0b4cde 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -116,6 +116,12 @@
>  #define QEPSTS_FIMF		BIT(1)
>  #define QEPSTS_PCEF		BIT(0)
>  
> +#define QCAPCTL_CEN		BIT(15)
> +#define QCAPCTL_CCPS_SHIFT	4

As mentioned below, using FIELD_GET() etc can avoid need to separately
define the shifts.

> +#define QCAPCTL_CCPS		GENMASK(6, 4)
> +#define QCAPCTL_UPPS_SHIFT	0
> +#define QCAPCTL_UPPS		GENMASK(3, 0)
> +
>  /* EQEP Inputs */
>  enum {
>  	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
> @@ -479,6 +485,137 @@ static struct counter_count ti_eqep_counts[] = {
>  	},
>  };
>  
> +static int ti_eqep_edge_capture_unit_enable_read(struct counter_device *counter,
> +						 u8 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcapctl;
> +
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	*value = !!(qcapctl & QCAPCTL_CEN);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_edge_capture_unit_enable_write(struct counter_device *counter,
> +						  u8 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	if (value)
> +		regmap_set_bits(priv->regmap16, QCAPCTL, QCAPCTL_CEN);
> +	else
> +		regmap_clear_bits(priv->regmap16, QCAPCTL, QCAPCTL_CEN);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_latched_period_read(struct counter_device *counter,
> +					      u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcprdlat, qcapctl;
> +	u8 ccps;
> +
> +	regmap_read(priv->regmap16, QCPRDLAT, &qcprdlat);
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	ccps = (qcapctl & QCAPCTL_CCPS) >> QCAPCTL_CCPS_SHIFT;

FIELD_PREP / FIELD_GET and friends allow this sort of action to be expressed using
just one define of the mask rather than a mask and shift.

> +
> +	/* convert timer ticks to nanoseconds */
> +	*value = mul_u64_u32_div(qcprdlat << ccps, NSEC_PER_SEC, priv->sysclkout_rate);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_max_period_read(struct counter_device *counter,
> +					  u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcapctl;
> +	u8 ccps;
> +
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	ccps = (qcapctl & QCAPCTL_CCPS) >> QCAPCTL_CCPS_SHIFT;
> +
> +	/* convert timer ticks to nanoseconds */
> +	*value = mul_u64_u32_div(USHRT_MAX << ccps, NSEC_PER_SEC,
> +				 priv->sysclkout_rate);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_max_period_write(struct counter_device *counter,
> +					   u64 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 period;
> +	u8 ccps;
> +
> +	/* convert nanoseconds to timer ticks */
> +	period = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> +	if (period != value)
> +		return -ERANGE;
> +
> +	/* find the smallest divider that will fit the requested period */
> +	for (ccps = 0; ccps <= 7; ccps++)
> +		if (USHRT_MAX << ccps >= period)
> +			break;
> +
> +	if (ccps > 7)
> +		return -EINVAL;
> +
> +	regmap_write_bits(priv->regmap16, QCAPCTL, QCAPCTL_CCPS,
> +			  ccps << QCAPCTL_CCPS_SHIFT);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_prescaler_read(struct counter_device *counter,
> +					 u32 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcapctl;
> +
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	*value = (qcapctl & QCAPCTL_UPPS) >> QCAPCTL_UPPS_SHIFT;
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_prescaler_write(struct counter_device *counter,
> +					  u32 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	regmap_write_bits(priv->regmap16, QCAPCTL, QCAPCTL_UPPS,
> +			  value << QCAPCTL_UPPS_SHIFT);
> +
> +	return 0;
> +}
> +
> +static const char *const ti_eqep_edge_capture_unit_prescaler_values[] = {
> +	"1",
> +	"2",
> +	"4",
> +	"8",
> +	"16",
> +	"32",
> +	"64",
> +	"128",
> +	"256",
> +	"512",
> +	"1024",
> +	"2048",
> +};
> +
> +static DEFINE_COUNTER_ENUM(ti_eqep_edge_capture_unit_prescaler_available,
> +			   ti_eqep_edge_capture_unit_prescaler_values);
> +
>  static int ti_eqep_latch_mode_read(struct counter_device *counter,
>  					    u32 *value)
>  {
> @@ -601,6 +738,19 @@ static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
>  }
>  
>  static struct counter_comp ti_eqep_device_ext[] = {
> +	COUNTER_COMP_DEVICE_BOOL("edge_capture_unit_enable",
> +				 ti_eqep_edge_capture_unit_enable_read,
> +				 ti_eqep_edge_capture_unit_enable_write),
> +	COUNTER_COMP_DEVICE_U64("edge_capture_unit_latched_period",
> +				ti_eqep_edge_capture_unit_latched_period_read,
> +				NULL),
> +	COUNTER_COMP_DEVICE_U64("edge_capture_unit_max_period",
> +				ti_eqep_edge_capture_unit_max_period_read,
> +				ti_eqep_edge_capture_unit_max_period_write),
> +	COUNTER_COMP_DEVICE_ENUM("edge_capture_unit_prescaler",
> +				 ti_eqep_edge_capture_unit_prescaler_read,
> +				 ti_eqep_edge_capture_unit_prescaler_write,
> +				 ti_eqep_edge_capture_unit_prescaler_available),
>  	COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read,
>  				ti_eqep_latch_mode_write, ti_eqep_latch_modes),
>  	COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,

Re: [PATCH 7/8] counter/ti-eqep: add support for edge capture unit

[William Breathitt Gray]

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

On Sat, Oct 16, 2021 at 08:33:42PM -0500, David Lechner wrote:
> This adds support for the Edge Capture Unit to the TI eQEP counter
> driver. This just adds the minimum required features to measure speed
> using the Unit Timer and the Edge Capture unit. Additional features can
> be added in the future if needed.
> 
> This adds 4 new device-level attributes:
> - edge_capture_unit_prescaler: selects a prescalar for the Counter count
> 	coming into the Edge Capture Unit
> - edge_capture_unit_max_period: selects the max time period that can be
> 	measured by the Edge Capture Unit

What happens if a trigger occurs after the max period has elapsed; is
the latched period value invalid in that scenario?

> - edge_capture_unit_latched_period: gets the period that was measured
> 	when the event selected by the latch_mode attribute is triggered

Is this period value essentially the current latch count minus the
previous latch count?

> - edge_capture_unit_enable: enables or disables the Edge Capture Unit
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  drivers/counter/ti-eqep.c | 150 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
> 
> diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c
> index ef899655ad1d..fb1f4d0b4cde 100644
> --- a/drivers/counter/ti-eqep.c
> +++ b/drivers/counter/ti-eqep.c
> @@ -116,6 +116,12 @@
>  #define QEPSTS_FIMF		BIT(1)
>  #define QEPSTS_PCEF		BIT(0)
>  
> +#define QCAPCTL_CEN		BIT(15)
> +#define QCAPCTL_CCPS_SHIFT	4
> +#define QCAPCTL_CCPS		GENMASK(6, 4)
> +#define QCAPCTL_UPPS_SHIFT	0
> +#define QCAPCTL_UPPS		GENMASK(3, 0)
> +
>  /* EQEP Inputs */
>  enum {
>  	TI_EQEP_SIGNAL_QEPA,	/* QEPA/XCLK */
> @@ -479,6 +485,137 @@ static struct counter_count ti_eqep_counts[] = {
>  	},
>  };
>  
> +static int ti_eqep_edge_capture_unit_enable_read(struct counter_device *counter,
> +						 u8 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcapctl;
> +
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	*value = !!(qcapctl & QCAPCTL_CEN);
> +
> +	return 0;
> +}
> +
> +static int ti_eqep_edge_capture_unit_enable_write(struct counter_device *counter,
> +						  u8 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	if (value)
> +		regmap_set_bits(priv->regmap16, QCAPCTL, QCAPCTL_CEN);
> +	else
> +		regmap_clear_bits(priv->regmap16, QCAPCTL, QCAPCTL_CEN);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_latched_period_read(struct counter_device *counter,
> +					      u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcprdlat, qcapctl;
> +	u8 ccps;
> +
> +	regmap_read(priv->regmap16, QCPRDLAT, &qcprdlat);
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	ccps = (qcapctl & QCAPCTL_CCPS) >> QCAPCTL_CCPS_SHIFT;
> +
> +	/* convert timer ticks to nanoseconds */
> +	*value = mul_u64_u32_div(qcprdlat << ccps, NSEC_PER_SEC, priv->sysclkout_rate);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_max_period_read(struct counter_device *counter,
> +					  u64 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcapctl;
> +	u8 ccps;
> +
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	ccps = (qcapctl & QCAPCTL_CCPS) >> QCAPCTL_CCPS_SHIFT;
> +
> +	/* convert timer ticks to nanoseconds */
> +	*value = mul_u64_u32_div(USHRT_MAX << ccps, NSEC_PER_SEC,
> +				 priv->sysclkout_rate);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_max_period_write(struct counter_device *counter,
> +					   u64 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 period;
> +	u8 ccps;
> +
> +	/* convert nanoseconds to timer ticks */
> +	period = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC);
> +	if (period != value)
> +		return -ERANGE;
> +
> +	/* find the smallest divider that will fit the requested period */
> +	for (ccps = 0; ccps <= 7; ccps++)
> +		if (USHRT_MAX << ccps >= period)
> +			break;
> +
> +	if (ccps > 7)
> +		return -EINVAL;
> +
> +	regmap_write_bits(priv->regmap16, QCAPCTL, QCAPCTL_CCPS,
> +			  ccps << QCAPCTL_CCPS_SHIFT);
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_prescaler_read(struct counter_device *counter,
> +					 u32 *value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +	u32 qcapctl;
> +
> +	regmap_read(priv->regmap16, QCAPCTL, &qcapctl);
> +	*value = (qcapctl & QCAPCTL_UPPS) >> QCAPCTL_UPPS_SHIFT;
> +
> +	return 0;
> +}
> +
> +static int
> +ti_eqep_edge_capture_unit_prescaler_write(struct counter_device *counter,
> +					  u32 value)
> +{
> +	struct ti_eqep_cnt *priv = counter->priv;
> +
> +	regmap_write_bits(priv->regmap16, QCAPCTL, QCAPCTL_UPPS,
> +			  value << QCAPCTL_UPPS_SHIFT);
> +
> +	return 0;
> +}
> +
> +static const char *const ti_eqep_edge_capture_unit_prescaler_values[] = {
> +	"1",
> +	"2",
> +	"4",
> +	"8",
> +	"16",
> +	"32",
> +	"64",
> +	"128",
> +	"256",
> +	"512",
> +	"1024",
> +	"2048",
> +};
> +
> +static DEFINE_COUNTER_ENUM(ti_eqep_edge_capture_unit_prescaler_available,
> +			   ti_eqep_edge_capture_unit_prescaler_values);
> +
>  static int ti_eqep_latch_mode_read(struct counter_device *counter,
>  					    u32 *value)
>  {
> @@ -601,6 +738,19 @@ static int ti_eqep_unit_timer_enable_write(struct counter_device *counter,
>  }
>  
>  static struct counter_comp ti_eqep_device_ext[] = {
> +	COUNTER_COMP_DEVICE_BOOL("edge_capture_unit_enable",
> +				 ti_eqep_edge_capture_unit_enable_read,
> +				 ti_eqep_edge_capture_unit_enable_write),
> +	COUNTER_COMP_DEVICE_U64("edge_capture_unit_latched_period",
> +				ti_eqep_edge_capture_unit_latched_period_read,
> +				NULL),
> +	COUNTER_COMP_DEVICE_U64("edge_capture_unit_max_period",
> +				ti_eqep_edge_capture_unit_max_period_read,
> +				ti_eqep_edge_capture_unit_max_period_write),
> +	COUNTER_COMP_DEVICE_ENUM("edge_capture_unit_prescaler",
> +				 ti_eqep_edge_capture_unit_prescaler_read,
> +				 ti_eqep_edge_capture_unit_prescaler_write,
> +				 ti_eqep_edge_capture_unit_prescaler_available),

Would it make sense for these to be Count 0 extensions so that they're
alongside the "latched_count" extension; or do these extensions also
represent values related to "latched_time" for the unit timer?

William Breathitt Gray

>  	COUNTER_COMP_DEVICE_ENUM("latch_mode", ti_eqep_latch_mode_read,
>  				ti_eqep_latch_mode_write, ti_eqep_latch_modes),
>  	COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read,
> -- 
> 2.25.1
> 

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

Re: [PATCH 7/8] counter/ti-eqep: add support for edge capture unit

[David Lechner]

On 10/27/21 3:23 AM, William Breathitt Gray wrote:
> On Sat, Oct 16, 2021 at 08:33:42PM -0500, David Lechner wrote:
>> This adds support for the Edge Capture Unit to the TI eQEP counter
>> driver. This just adds the minimum required features to measure speed
>> using the Unit Timer and the Edge Capture unit. Additional features can
>> be added in the future if needed.
>>
>> This adds 4 new device-level attributes:
>> - edge_capture_unit_prescaler: selects a prescalar for the Counter count
>> 	coming into the Edge Capture Unit
>> - edge_capture_unit_max_period: selects the max time period that can be
>> 	measured by the Edge Capture Unit
> 
> What happens if a trigger occurs after the max period has elapsed; is
> the latched period value invalid in that scenario?


This period is essentially selecting prescalars, so if we get rid of
the time aspect and treat everything as counts this will change a bit.

But the question is still valid. I don't recall off-hand what happens
so I will have to test it the next time I am working on this. It seems
though that I was able to somehow detect when this was the case.

> 
>> - edge_capture_unit_latched_period: gets the period that was measured
>> 	when the event selected by the latch_mode attribute is triggered
> 
> Is this period value essentially the current latch count minus the
> previous latch count?
> 

No, the period is the amount of time that elapsed between counts.

>>   static struct counter_comp ti_eqep_device_ext[] = {
>> +	COUNTER_COMP_DEVICE_BOOL("edge_capture_unit_enable",
>> +				 ti_eqep_edge_capture_unit_enable_read,
>> +				 ti_eqep_edge_capture_unit_enable_write),
>> +	COUNTER_COMP_DEVICE_U64("edge_capture_unit_latched_period",
>> +				ti_eqep_edge_capture_unit_latched_period_read,
>> +				NULL),
>> +	COUNTER_COMP_DEVICE_U64("edge_capture_unit_max_period",
>> +				ti_eqep_edge_capture_unit_max_period_read,
>> +				ti_eqep_edge_capture_unit_max_period_write),
>> +	COUNTER_COMP_DEVICE_ENUM("edge_capture_unit_prescaler",
>> +				 ti_eqep_edge_capture_unit_prescaler_read,
>> +				 ti_eqep_edge_capture_unit_prescaler_write,
>> +				 ti_eqep_edge_capture_unit_prescaler_available),
> 
> Would it make sense for these to be Count 0 extensions so that they're
> alongside the "latched_count" extension; or do these extensions also
> represent values related to "latched_time" for the unit timer?
> 

They are related to both.

[PATCH 8/8] docs: counter: add edge_capture_unit_* attributes

[David Lechner]

This adds documentation for new counter subsystem edge_capture_unit_*
sysfs attributes.

Signed-off-by: David Lechner <david@lechnology.com>
---
 Documentation/ABI/testing/sysfs-bus-counter | 37 +++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
index 78bb1a501007..6c192c8c2b55 100644
--- a/Documentation/ABI/testing/sysfs-bus-counter
+++ b/Documentation/ABI/testing/sysfs-bus-counter
@@ -59,6 +59,7 @@ What:		/sys/bus/counter/devices/counterX/countY/error_noise_available
 What:		/sys/bus/counter/devices/counterX/countY/function_available
 What:		/sys/bus/counter/devices/counterX/countY/prescaler_available
 What:		/sys/bus/counter/devices/counterX/countY/signalZ_action_available
+What:		/sys/bus/counter/devices/counterX/edge_capture_unit_prescaler_available
 What:		/sys/bus/counter/devices/counterX/latch_mode_available
 What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
 What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
@@ -230,6 +231,10 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
 What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
+What:		/sys/bus/counter/devices/edge_capture_unit_enable_component_id
+What:		/sys/bus/counter/devices/edge_capture_unit_latched_period_component_id
+What:		/sys/bus/counter/devices/edge_capture_unit_max_period_component_id
+What:		/sys/bus/counter/devices/edge_capture_unit_prescaler_component_id
 What:		/sys/bus/counter/devices/latch_mode_component_id
 What:		/sys/bus/counter/devices/unit_timer_enable_component_id
 What:		/sys/bus/counter/devices/unit_timer_period_component_id
@@ -249,6 +254,38 @@ Description:
 		shorter or equal to configured value are ignored. Value 0 means
 		filter is disabled.
 
+What:		/sys/bus/counter/devices/edge_capture_unit_enable
+KernelVersion:	5.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write attribute that starts or stops the Edge Capture Unit.
+		Valid values are boolean.
+
+What:		/sys/bus/counter/devices/edge_capture_unit_latched_period
+KernelVersion:	5.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Latched period of the Edge Capture Unit represented as a string.
+		The value is latched in based on the trigger selected by the
+		counterX/latch_mode attribute. Units are nanoseconds.
+
+What:		/sys/bus/counter/devices/edge_capture_unit_max_period
+KernelVersion:	5.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write attribute that selects the maximum period that can
+		be measured by the Edge Capture Unit. Units are nanoseconds.
+
+What:		/sys/bus/counter/devices/edge_capture_unit_prescaler
+KernelVersion:	5.16
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Read/write attribute that selects the how the
+		counterX/countY/count value is scaled coming in to the Edge
+		Capture Unit. This acts like a clock divider, e.g. if a value
+		of 4 is selected, the Edge Capture Unit will measure the period
+		between every 4 counts.
+
 What:		/sys/bus/counter/devices/counterX/events_queue_size
 KernelVersion:	5.16
 Contact:	linux-iio@vger.kernel.org
-- 
2.25.1

Re: [PATCH 8/8] docs: counter: add edge_capture_unit_* attributes

[William Breathitt Gray]

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

On Sat, Oct 16, 2021 at 08:33:43PM -0500, David Lechner wrote:
> This adds documentation for new counter subsystem edge_capture_unit_*
> sysfs attributes.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-counter | 37 +++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-counter b/Documentation/ABI/testing/sysfs-bus-counter
> index 78bb1a501007..6c192c8c2b55 100644
> --- a/Documentation/ABI/testing/sysfs-bus-counter
> +++ b/Documentation/ABI/testing/sysfs-bus-counter
> @@ -59,6 +59,7 @@ What:		/sys/bus/counter/devices/counterX/countY/error_noise_available
>  What:		/sys/bus/counter/devices/counterX/countY/function_available
>  What:		/sys/bus/counter/devices/counterX/countY/prescaler_available
>  What:		/sys/bus/counter/devices/counterX/countY/signalZ_action_available
> +What:		/sys/bus/counter/devices/counterX/edge_capture_unit_prescaler_available
>  What:		/sys/bus/counter/devices/counterX/latch_mode_available
>  What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_available
>  What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_available
> @@ -230,6 +231,10 @@ What:		/sys/bus/counter/devices/counterX/signalY/cable_fault_enable_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/filter_clock_prescaler_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/index_polarity_component_id
>  What:		/sys/bus/counter/devices/counterX/signalY/synchronous_mode_component_id
> +What:		/sys/bus/counter/devices/edge_capture_unit_enable_component_id
> +What:		/sys/bus/counter/devices/edge_capture_unit_latched_period_component_id
> +What:		/sys/bus/counter/devices/edge_capture_unit_max_period_component_id
> +What:		/sys/bus/counter/devices/edge_capture_unit_prescaler_component_id
>  What:		/sys/bus/counter/devices/latch_mode_component_id
>  What:		/sys/bus/counter/devices/unit_timer_enable_component_id
>  What:		/sys/bus/counter/devices/unit_timer_period_component_id
> @@ -249,6 +254,38 @@ Description:
>  		shorter or equal to configured value are ignored. Value 0 means
>  		filter is disabled.
>  
> +What:		/sys/bus/counter/devices/edge_capture_unit_enable
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write attribute that starts or stops the Edge Capture Unit.
> +		Valid values are boolean.
> +
> +What:		/sys/bus/counter/devices/edge_capture_unit_latched_period
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Latched period of the Edge Capture Unit represented as a string.
> +		The value is latched in based on the trigger selected by the
> +		counterX/latch_mode attribute. Units are nanoseconds.
> +
> +What:		/sys/bus/counter/devices/edge_capture_unit_max_period
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write attribute that selects the maximum period that can
> +		be measured by the Edge Capture Unit. Units are nanoseconds.
> +
> +What:		/sys/bus/counter/devices/edge_capture_unit_prescaler
> +KernelVersion:	5.16
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Read/write attribute that selects the how the
> +		counterX/countY/count value is scaled coming in to the Edge
> +		Capture Unit. This acts like a clock divider, e.g. if a value
> +		of 4 is selected, the Edge Capture Unit will measure the period
> +		between every 4 counts.
> +

I'd like to see that naming for this made more generic if possible so
that other drivers can use these extensions in the future. For example,
instead of the "edge_capture_unit_*" prefix, perhaps "latched_count_*"
would be appropriate. Would this be feasible?

William Breathitt Gray

>  What:		/sys/bus/counter/devices/counterX/events_queue_size
>  KernelVersion:	5.16
>  Contact:	linux-iio@vger.kernel.org
> -- 
> 2.25.1
> 

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