[PATCH 0/3] Introduce a counter inkernel API

[PATCH 0/3] Introduce a counter inkernel API

[Kamel Bouhara]

Hello everyone,

This series introduce a basic inkernel API for the counter subsystem and
add a new rotary encoder driver that use a counter interface instead of
the GPIO existing one.

See commit log in 0001-counter-add-an-inkernel-API.patch for further
details.

Kamel Bouhara (3):
  counter: add an inkernel API
  Input: rotary-encoder-counter: add DT bindings
  Input: add a rotary encoders based on counter devices

 .../input/rotary-encoder-counter.yaml         |  67 ++++++
 drivers/counter/counter.c                     | 213 ++++++++++++++++++
 drivers/input/misc/Kconfig                    |   9 +
 drivers/input/misc/Makefile                   |   1 +
 drivers/input/misc/rotary_encoder_counter.c   | 152 +++++++++++++
 include/linux/counter.h                       |  27 +++
 6 files changed, 469 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
 create mode 100644 drivers/input/misc/rotary_encoder_counter.c

--
2.25.0

[PATCH 1/3] counter: add an inkernel API

[Kamel Bouhara]

Currently, counter devices are exposed directly to user-space through a
sysfs interface. However, there are situations where it might makes
sense for another kernel driver to use a counter handled by the counter
subsystem.

An example is the implementation of an input driver for a rotary
encoder that is connected to hardware counters handled by the counter
subsystem.

This is very much like what happens with IIO subsystem, where ADC
channels are directly accessible by user-space through sysfs, or can be
used through the IIO in-kernel API by other kernel drivers, such as the
adc-keys input keyboard driver.

The proposed API is fairly simple:

 - devm_counter_get() allows a driver to get a reference to a counter
   device. From a Device Tree point of view, a counter-phandle property
   allows to indicate which counter device should be used.
   devm_counter_get() returns a reference to the counter_device
   structure.

 - counter_count_get() and counter_count_set() allow respectively to
   read and set a count value.

 - counter_function_get() and counter_function_set() allow to read and
   set the current count function.

 - counter_action_get() and counter_action_set allow to read and set the
   current count action/synapse.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 drivers/counter/counter.c | 213 ++++++++++++++++++++++++++++++++++++++
 include/linux/counter.h   |  27 +++++
 2 files changed, 240 insertions(+)

diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
index 6a683d086008..f81d2d1dbca7 100644
--- a/drivers/counter/counter.c
+++ b/drivers/counter/counter.c
@@ -19,6 +19,7 @@
 #include <linux/string.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
+#include <linux/of.h>
 
 const char *const counter_count_direction_str[2] = {
 	[COUNTER_COUNT_DIRECTION_FORWARD] = "forward",
@@ -1460,6 +1461,218 @@ static int devm_counter_match(struct device *dev, void *res, void *data)
 	return *r == data;
 }
 
+static int match(struct device *dev, const void *data)
+{
+	return (dev->of_node == data) &&
+		(dev->type == &counter_device_type);
+}
+
+/* Internal counter request function */
+static struct counter_device *counter_get(struct device *dev)
+{
+	struct device_node *np;
+	struct device *counter_dev;
+	struct counter_device_state *counter_ds;
+	struct counter_device *counter;
+
+	/* Try DT lookup */
+	np = of_parse_phandle(dev->of_node, "counter", 0);
+	if (!np) {
+		dev_dbg(dev,
+			"Looking up from device tree %pOF failed\n", np);
+		return ERR_PTR(-ENODEV);
+	}
+
+	counter_dev = bus_find_device(&counter_bus_type, NULL, np, match);
+	of_node_put(np);
+	if (!counter_dev)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	counter_ds = to_counter_device_state(counter_dev);
+
+	counter = dev_get_drvdata(&counter_ds->dev);
+	if (!counter)
+		return ERR_PTR(-ENODEV);
+
+	/* Check if device is already owned otherwise set the busy bit */
+	if (test_and_set_bit(COUNTER_BUSY_BIT_POS, &counter->flags)) {
+		pr_info("counter is busy\n");
+		return ERR_PTR(-EBUSY);
+	}
+
+	pr_info("counter acquired\n");
+	get_device(&counter_ds->dev);
+
+	return counter;
+}
+
+static void devm_counter_release(struct device *dev, void *res)
+{
+	struct counter_device *counter = *(struct counter_device **)res;
+
+	if (!counter)
+		return;
+	clear_bit(COUNTER_BUSY_BIT_POS, &counter->flags);
+	put_device(&counter->device_state->dev);
+}
+
+/*
+ * devm_counter_get - Obtain an exclusive access to a counter.
+ * @dev: device for counter "consumer"
+ *
+ * Returns a struct counter_device matching the counter producer, or
+ * IS_ERR() condition containing errno.
+ *
+ */
+struct counter_device *devm_counter_get(struct device *dev)
+{
+	struct counter_device **ptr, *counter;
+
+	ptr = devres_alloc(devm_counter_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	counter = counter_get(dev);
+	if (IS_ERR(counter)) {
+		devres_free(ptr);
+		return counter;
+	}
+
+	*ptr = counter;
+	devres_add(dev, ptr);
+
+	return counter;
+}
+EXPORT_SYMBOL_GPL(devm_counter_get);
+
+/*
+ * counter_action_get - get counter synapse mode
+ * @counter: counter device to operate with
+ * @action: pointer to store the current counter synapse mode
+ * returns:
+ *	0 on success, error code on failure.
+ */
+int counter_action_get(struct counter_device *counter, int *mode)
+{
+	struct counter_synapse *synapse = counter->counts->synapses;
+	size_t action_index;
+	int err;
+
+	err = counter->ops->action_get(counter, counter->counts, synapse,
+				       &action_index);
+	if (err)
+		return err;
+
+	*mode = synapse->actions_list[action_index];
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(counter_action_get);
+
+/*
+ * counter_action_set - set counter device synapse
+ * @counter: counter device to operate with
+ * @action: enum of the synapse mode
+ * returns:
+ *	0 on success, error code on failure.
+ */
+int counter_action_set(struct counter_device *counter,
+		       enum counter_synapse_action action)
+{
+	struct counter_synapse *synapse = counter->counts->synapses;
+	const size_t num_actions = synapse->num_actions;
+	size_t action_index;
+
+	/* Find requested action mode */
+	for (action_index = 0; action_index < num_actions; action_index++) {
+		if (action == synapse->actions_list[action_index])
+			break;
+	}
+
+	if (action_index >= num_actions)
+		return -EINVAL;
+
+	return counter->ops->action_set(counter, counter->counts, synapse,
+					action_index);
+}
+EXPORT_SYMBOL_GPL(counter_action_set);
+
+/*
+ * counter_function_get - get the count function
+ * @counter: pointer to counter device to operate with
+ * @mode: pointer to store the current counter function mode
+ * returns:
+ *	0 on success, error code on failure.
+ */
+int counter_function_get(struct counter_device *counter, int *mode)
+{
+	size_t func_index;
+	int err;
+
+	err = counter->ops->function_get(counter, counter->counts,
+					 &func_index);
+	if (err)
+		return err;
+
+	*mode = counter->counts->functions_list[func_index];
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(counter_function_get);
+
+/*
+ * counter_function_set - set a count function
+ * @counter: pointer to a counter device to operate with
+ * @function: enum of the function mode
+ * returns:
+ *	0 on success, error code on failure.
+ */
+int counter_function_set(struct counter_device *counter,
+			  enum counter_count_function function)
+{
+	const size_t num_functions = counter->counts->num_functions;
+	size_t func_index;
+
+	for (func_index = 0; func_index < num_functions; func_index++) {
+		if (function == counter->counts->functions_list[func_index])
+			break;
+	}
+
+	if (func_index >= num_functions)
+		return -EINVAL;
+
+	return counter->ops->function_set(counter, counter->counts, func_index);
+}
+EXPORT_SYMBOL_GPL(counter_function_set);
+
+/*
+ * counter_count_set - set a count value
+ * @counter: pointer to the counter device to operate with
+ * @val: count value to write into the counter
+ * @len: length of the value written to the counter
+ * returns:
+ *	bytes length of the value on success, error code on failure.
+ */
+size_t counter_count_set(struct counter_device *counter,
+			 unsigned long val, size_t len)
+{
+	return counter->ops->count_write(counter, counter->counts, val);
+}
+EXPORT_SYMBOL_GPL(counter_count_set);
+
+/*
+ * counter_count_get - read the count value
+ * @counter: pointer to the counter device to operate with
+ * @val: pointer to store the count value
+ * returns:
+ *  0 on success, error code on failure.
+ */
+int counter_count_get(struct counter_device *counter, unsigned long *val)
+{
+	return counter->ops->count_read(counter, counter->counts, val);
+}
+EXPORT_SYMBOL_GPL(counter_count_get);
+
 /**
  * devm_counter_unregister - Resource-managed counter_unregister
  * @dev:	device this counter_device belongs to
diff --git a/include/linux/counter.h b/include/linux/counter.h
index 9dbd5df4cd34..81f63479be55 100644
--- a/include/linux/counter.h
+++ b/include/linux/counter.h
@@ -10,6 +10,8 @@
 #include <linux/device.h>
 #include <linux/types.h>
 
+#define COUNTER_BUSY_BIT_POS 1
+
 enum counter_count_direction {
 	COUNTER_COUNT_DIRECTION_FORWARD = 0,
 	COUNTER_COUNT_DIRECTION_BACKWARD
@@ -290,6 +292,15 @@ struct counter_device_state {
 	const struct attribute_group **groups;
 };
 
+/**
+ * to_counter_device_state - Returns a &struct counter_device_state
+ * from the &struct device embedded in it.
+ *
+ * @d:pointer to &struct device
+ */
+#define to_counter_device_state(d) \
+	container_of(d, struct counter_device_state, dev)
+
 enum counter_signal_value {
 	COUNTER_SIGNAL_LOW = 0,
 	COUNTER_SIGNAL_HIGH
@@ -424,6 +435,7 @@ struct counter_device_enum_ext {
  * @num_counts:		number of Counts specified in @counts
  * @ext:		optional array of Counter device extensions
  * @num_ext:		number of Counter device extensions specified in @ext
+ * @flags:		internal device flags including a busy flag.
  * @priv:		optional private data supplied by driver
  */
 struct counter_device {
@@ -440,6 +452,7 @@ struct counter_device {
 
 	const struct counter_device_ext *ext;
 	size_t num_ext;
+	unsigned long flags;
 
 	void *priv;
 };
@@ -450,5 +463,19 @@ int devm_counter_register(struct device *dev,
 			  struct counter_device *const counter);
 void devm_counter_unregister(struct device *dev,
 			     struct counter_device *const counter);
+/* inkernel interface */
+int __must_check counter_count_get(struct counter_device *counter,
+				   unsigned long *val);
+size_t __must_check counter_count_set(struct counter_device *counter,
+				      unsigned long val, size_t len);
+int __must_check counter_function_get(struct counter_device *counter,
+				      int *mode);
+int __must_check counter_function_set(struct counter_device *counter,
+				      enum counter_count_function function);
+int __must_check counter_action_get(struct counter_device *counter,
+				    int *mode);
+int __must_check counter_action_set(struct counter_device *counter,
+				    enum counter_synapse_action action);
+struct counter_device *__must_check devm_counter_get(struct device *dev);
 
 #endif /* _COUNTER_H_ */
-- 
2.25.0

Re: [PATCH 1/3] counter: add an inkernel API

[Randy Dunlap]

Hi--

On 4/6/20 8:58 AM, Kamel Bouhara wrote:
> ---
>  drivers/counter/counter.c | 213 ++++++++++++++++++++++++++++++++++++++
>  include/linux/counter.h   |  27 +++++
>  2 files changed, 240 insertions(+)
> 
> diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
> index 6a683d086008..f81d2d1dbca7 100644
> --- a/drivers/counter/counter.c
> +++ b/drivers/counter/counter.c

[snip]

Please use
/**
on these functions so that kernel-doc will process them.

> +
> +/*
> + * devm_counter_get - Obtain an exclusive access to a counter.
> + * @dev: device for counter "consumer"
> + *
> + * Returns a struct counter_device matching the counter producer, or
> + * IS_ERR() condition containing errno.
> + *
> + */
> +struct counter_device *devm_counter_get(struct device *dev)
> +{
> +	struct counter_device **ptr, *counter;
> +
> +	ptr = devres_alloc(devm_counter_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	counter = counter_get(dev);
> +	if (IS_ERR(counter)) {
> +		devres_free(ptr);
> +		return counter;
> +	}
> +
> +	*ptr = counter;
> +	devres_add(dev, ptr);
> +
> +	return counter;
> +}
> +EXPORT_SYMBOL_GPL(devm_counter_get);
> +
> +/*
> + * counter_action_get - get counter synapse mode
> + * @counter: counter device to operate with
> + * @action: pointer to store the current counter synapse mode

should be @mode: ^^^^^

> + * returns:
> + *	0 on success, error code on failure.
> + */
> +int counter_action_get(struct counter_device *counter, int *mode)
> +{
> +	struct counter_synapse *synapse = counter->counts->synapses;
> +	size_t action_index;
> +	int err;
> +
> +	err = counter->ops->action_get(counter, counter->counts, synapse,
> +				       &action_index);
> +	if (err)
> +		return err;
> +
> +	*mode = synapse->actions_list[action_index];
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(counter_action_get);
> +
> +/*
> + * counter_action_set - set counter device synapse
> + * @counter: counter device to operate with
> + * @action: enum of the synapse mode
> + * returns:
> + *	0 on success, error code on failure.
> + */
> +int counter_action_set(struct counter_device *counter,
> +		       enum counter_synapse_action action)
> +{
> +	struct counter_synapse *synapse = counter->counts->synapses;
> +	const size_t num_actions = synapse->num_actions;
> +	size_t action_index;
> +
> +	/* Find requested action mode */
> +	for (action_index = 0; action_index < num_actions; action_index++) {
> +		if (action == synapse->actions_list[action_index])
> +			break;
> +	}
> +
> +	if (action_index >= num_actions)
> +		return -EINVAL;
> +
> +	return counter->ops->action_set(counter, counter->counts, synapse,
> +					action_index);
> +}
> +EXPORT_SYMBOL_GPL(counter_action_set);
> +
> +/*
> + * counter_function_get - get the count function
> + * @counter: pointer to counter device to operate with
> + * @mode: pointer to store the current counter function mode
> + * returns:
> + *	0 on success, error code on failure.
> + */
> +int counter_function_get(struct counter_device *counter, int *mode)
> +{
> +	size_t func_index;
> +	int err;
> +
> +	err = counter->ops->function_get(counter, counter->counts,
> +					 &func_index);
> +	if (err)
> +		return err;
> +
> +	*mode = counter->counts->functions_list[func_index];
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(counter_function_get);
> +
> +/*
> + * counter_function_set - set a count function
> + * @counter: pointer to a counter device to operate with
> + * @function: enum of the function mode
> + * returns:
> + *	0 on success, error code on failure.
> + */
> +int counter_function_set(struct counter_device *counter,
> +			  enum counter_count_function function)
> +{
> +	const size_t num_functions = counter->counts->num_functions;
> +	size_t func_index;
> +
> +	for (func_index = 0; func_index < num_functions; func_index++) {
> +		if (function == counter->counts->functions_list[func_index])
> +			break;
> +	}
> +
> +	if (func_index >= num_functions)
> +		return -EINVAL;
> +
> +	return counter->ops->function_set(counter, counter->counts, func_index);
> +}
> +EXPORT_SYMBOL_GPL(counter_function_set);
> +
> +/*
> + * counter_count_set - set a count value
> + * @counter: pointer to the counter device to operate with
> + * @val: count value to write into the counter
> + * @len: length of the value written to the counter
> + * returns:
> + *	bytes length of the value on success, error code on failure.
> + */
> +size_t counter_count_set(struct counter_device *counter,
> +			 unsigned long val, size_t len)
> +{
> +	return counter->ops->count_write(counter, counter->counts, val);
> +}
> +EXPORT_SYMBOL_GPL(counter_count_set);
> +
> +/*
> + * counter_count_get - read the count value
> + * @counter: pointer to the counter device to operate with
> + * @val: pointer to store the count value
> + * returns:
> + *  0 on success, error code on failure.
> + */
> +int counter_count_get(struct counter_device *counter, unsigned long *val)
> +{
> +	return counter->ops->count_read(counter, counter->counts, val);
> +}
> +EXPORT_SYMBOL_GPL(counter_count_get);
> +
>  /**
>   * devm_counter_unregister - Resource-managed counter_unregister
>   * @dev:	device this counter_device belongs to


thanks.
-- 
~Randy

Re: [PATCH 1/3] counter: add an inkernel API

[Kamel Bouhara]

On Fri, Apr 10, 2020 at 10:34:34AM -0700, Randy Dunlap wrote:
> Hi--
>

Hello,

> On 4/6/20 8:58 AM, Kamel Bouhara wrote:
> > ---
> >  drivers/counter/counter.c | 213 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/counter.h   |  27 +++++
> >  2 files changed, 240 insertions(+)
> >
> > diff --git a/drivers/counter/counter.c b/drivers/counter/counter.c
> > index 6a683d086008..f81d2d1dbca7 100644
> > --- a/drivers/counter/counter.c
> > +++ b/drivers/counter/counter.c
>
> [snip]
>
> Please use
> /**
> on these functions so that kernel-doc will process them.
>

OK, fixed.

Thanks.

> > +
> > +/*
> > + * devm_counter_get - Obtain an exclusive access to a counter.
> > + * @dev: device for counter "consumer"
> > + *
> > + * Returns a struct counter_device matching the counter producer, or
> > + * IS_ERR() condition containing errno.
> > + *
> > + */
> > +struct counter_device *devm_counter_get(struct device *dev)
> > +{
> > +	struct counter_device **ptr, *counter;
> > +
> > +	ptr = devres_alloc(devm_counter_release, sizeof(*ptr), GFP_KERNEL);
> > +	if (!ptr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	counter = counter_get(dev);
> > +	if (IS_ERR(counter)) {
> > +		devres_free(ptr);
> > +		return counter;
> > +	}
> > +
> > +	*ptr = counter;
> > +	devres_add(dev, ptr);
> > +
> > +	return counter;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_counter_get);
> > +
> > +/*
> > + * counter_action_get - get counter synapse mode
> > + * @counter: counter device to operate with
> > + * @action: pointer to store the current counter synapse mode
>
> should be @mode: ^^^^^
>

Fixed.

Thanks.

Kamel

> > + * returns:
> > + *	0 on success, error code on failure.
> > + */
> > +int counter_action_get(struct counter_device *counter, int *mode)
> > +{
> > +	struct counter_synapse *synapse = counter->counts->synapses;
> > +	size_t action_index;
> > +	int err;
> > +
> > +	err = counter->ops->action_get(counter, counter->counts, synapse,
> > +				       &action_index);
> > +	if (err)
> > +		return err;
> > +
> > +	*mode = synapse->actions_list[action_index];
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(counter_action_get);
> > +
> > +/*
> > + * counter_action_set - set counter device synapse
> > + * @counter: counter device to operate with
> > + * @action: enum of the synapse mode
> > + * returns:
> > + *	0 on success, error code on failure.
> > + */
> > +int counter_action_set(struct counter_device *counter,
> > +		       enum counter_synapse_action action)
> > +{
> > +	struct counter_synapse *synapse = counter->counts->synapses;
> > +	const size_t num_actions = synapse->num_actions;
> > +	size_t action_index;
> > +
> > +	/* Find requested action mode */
> > +	for (action_index = 0; action_index < num_actions; action_index++) {
> > +		if (action == synapse->actions_list[action_index])
> > +			break;
> > +	}
> > +
> > +	if (action_index >= num_actions)
> > +		return -EINVAL;
> > +
> > +	return counter->ops->action_set(counter, counter->counts, synapse,
> > +					action_index);
> > +}
> > +EXPORT_SYMBOL_GPL(counter_action_set);
> > +
> > +/*
> > + * counter_function_get - get the count function
> > + * @counter: pointer to counter device to operate with
> > + * @mode: pointer to store the current counter function mode
> > + * returns:
> > + *	0 on success, error code on failure.
> > + */
> > +int counter_function_get(struct counter_device *counter, int *mode)
> > +{
> > +	size_t func_index;
> > +	int err;
> > +
> > +	err = counter->ops->function_get(counter, counter->counts,
> > +					 &func_index);
> > +	if (err)
> > +		return err;
> > +
> > +	*mode = counter->counts->functions_list[func_index];
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(counter_function_get);
> > +
> > +/*
> > + * counter_function_set - set a count function
> > + * @counter: pointer to a counter device to operate with
> > + * @function: enum of the function mode
> > + * returns:
> > + *	0 on success, error code on failure.
> > + */
> > +int counter_function_set(struct counter_device *counter,
> > +			  enum counter_count_function function)
> > +{
> > +	const size_t num_functions = counter->counts->num_functions;
> > +	size_t func_index;
> > +
> > +	for (func_index = 0; func_index < num_functions; func_index++) {
> > +		if (function == counter->counts->functions_list[func_index])
> > +			break;
> > +	}
> > +
> > +	if (func_index >= num_functions)
> > +		return -EINVAL;
> > +
> > +	return counter->ops->function_set(counter, counter->counts, func_index);
> > +}
> > +EXPORT_SYMBOL_GPL(counter_function_set);
> > +
> > +/*
> > + * counter_count_set - set a count value
> > + * @counter: pointer to the counter device to operate with
> > + * @val: count value to write into the counter
> > + * @len: length of the value written to the counter
> > + * returns:
> > + *	bytes length of the value on success, error code on failure.
> > + */
> > +size_t counter_count_set(struct counter_device *counter,
> > +			 unsigned long val, size_t len)
> > +{
> > +	return counter->ops->count_write(counter, counter->counts, val);
> > +}
> > +EXPORT_SYMBOL_GPL(counter_count_set);
> > +
> > +/*
> > + * counter_count_get - read the count value
> > + * @counter: pointer to the counter device to operate with
> > + * @val: pointer to store the count value
> > + * returns:
> > + *  0 on success, error code on failure.
> > + */
> > +int counter_count_get(struct counter_device *counter, unsigned long *val)
> > +{
> > +	return counter->ops->count_read(counter, counter->counts, val);
> > +}
> > +EXPORT_SYMBOL_GPL(counter_count_get);
> > +
> >  /**
> >   * devm_counter_unregister - Resource-managed counter_unregister
> >   * @dev:	device this counter_device belongs to
>
>
> thanks.
> --
> ~Randy
>

--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH 2/3] Input: rotary-encoder-counter: add DT bindings

[Kamel Bouhara]

Add dt binding for the counter variant of the rotary encoder driver.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml

diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
new file mode 100644
index 000000000000..a59f7c1faf0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rotary Encoder Counter
+
+maintainers:
+  - Kamel Bouhara <kamel.bouhara@bootlin.com>
+
+description:
+  Registers a Rotary encoder connected through a counter device.
+
+properties:
+  compatible:
+    const: rotary-encoder-counter
+
+  counter:
+    description: Phandle for the counter device providing rotary position.
+
+  linux-axis:
+    description: The input subsystem axis to map to this rotary encoder.
+    type: boolean
+
+  qdec-mode:
+    description: |
+      Quadrature decoder function to set in the counter device.
+      3: x1-PHA
+      4: x1-PHB
+      5: x2-PHA
+      6: x2-PHB
+      7: x4-PHA and PHB
+
+  steps:
+    description: Number of steps in a full turnaround of the encoder.
+      Only relevant for absolute axis. Defaults to 24 which is a typical
+      value for such devices.
+
+  relative-axis:
+    description: Register a relative axis rather than an absolute one.
+    type: boolean
+
+  rollover:
+    description: Automatic rollover when the rotary value becomes greater
+      than the specified steps or smaller than 0. For absolute axis only.
+    type: boolean
+
+  poll-interval:
+    description: Poll interval at which the position is read from the counter
+      device (default 500ms).
+
+required:
+  - compatible
+  - counter
+  - qdec-mode
+
+examples:
+  - |
+    rotary@0 {
+        compatible = "rotary-encoder-counter";
+
+        counter =<&qdec>;
+        qdec-mode = <7>;
+        relative-axis;
+    };
+...
-- 
2.25.0

Re: [PATCH 2/3] Input: rotary-encoder-counter: add DT bindings

[Maxime Ripard]

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

Hi Kamel,

The prefix for device tree bindings is usually dt-bindings:
$framework: $title

So a title like "dt-bindings: input: Add a counter-based rotary
encoder binding" would be better.

On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> Add dt binding for the counter variant of the rotary encoder driver.
>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
>  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> new file mode 100644
> index 000000000000..a59f7c1faf0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: GPL-2.0

Bindings are usually used by other OS's, so you should consider
putting it under a more permissive license, usually that would be GPL2
and the BSD-2-Clause

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rotary Encoder Counter
> +
> +maintainers:
> +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> +
> +description:
> +  Registers a Rotary encoder connected through a counter device.

You shouldn't really describe the action here, but more what the
binding is about. The registration will not depend on the presence of
the node following that binding, but rather on whether or not the OS
that uses it has support for it.

> +properties:
> +  compatible:
> +    const: rotary-encoder-counter
> +
> +  counter:
> +    description: Phandle for the counter device providing rotary position.

This should have a type

> +  linux-axis:
> +    description: The input subsystem axis to map to this rotary encoder.
> +    type: boolean
> +
> +  qdec-mode:
> +    description: |
> +      Quadrature decoder function to set in the counter device.
> +      3: x1-PHA
> +      4: x1-PHB
> +      5: x2-PHA
> +      6: x2-PHB
> +      7: x4-PHA and PHB

That range (even though it's a bit odd) should be expressed through an
enum so that you can check that the values are actually within that
range.

> +  steps:
> +    description: Number of steps in a full turnaround of the encoder.

Muli-line strings should have either quotes around them, or a | or >
like you did for the description. | will keep the \n, > will make that
a single string.

This should also have a type

> +      Only relevant for absolute axis.

This should be expressed through a if / then clause, or a dependencies one

>                                         Defaults to 24 which is a typical
> +      value for such devices.

This should be expressed through a default property.

> +  relative-axis:
> +    description: Register a relative axis rather than an absolute one.
> +    type: boolean
> +
> +  rollover:
> +    description: Automatic rollover when the rotary value becomes greater
> +      than the specified steps or smaller than 0. For absolute axis only.
> +    type: boolean

Same story than steps for the dependency. Also, what is is the
behaviour when this property isn't set?

> +  poll-interval:
> +    description: Poll interval at which the position is read from the counter
> +      device (default 500ms).

It should have a type too, and a default property

> +
> +required:
> +  - compatible
> +  - counter
> +  - qdec-mode
> +
> +examples:
> +  - |
> +    rotary@0 {
> +        compatible = "rotary-encoder-counter";

A unit-address (the part after @) only makes sense for a node if
there's a matching reg property in the node. This will trigger a DTC
warning, so you should remove the @0

Maxime

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

Re: [PATCH 2/3] Input: rotary-encoder-counter: add DT bindings

[Kamel Bouhara]

On Tue, Apr 07, 2020 at 11:41:59AM +0200, Maxime Ripard wrote:
> Hi Kamel,
>

Hi Maxime,

> The prefix for device tree bindings is usually dt-bindings:
> $framework: $title
>
> So a title like "dt-bindings: input: Add a counter-based rotary
> encoder binding" would be better.
>

OK, to be fixed then.

> On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > Add dt binding for the counter variant of the rotary encoder driver.
> >
> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > ---
> >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > new file mode 100644
> > index 000000000000..a59f7c1faf0c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > @@ -0,0 +1,67 @@
> > +# SPDX-License-Identifier: GPL-2.0
>
> Bindings are usually used by other OS's, so you should consider
> putting it under a more permissive license, usually that would be GPL2
> and the BSD-2-Clause
>

Well to be honest I just looked into an existing binding and I guess
the wrong one :).

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rotary Encoder Counter
> > +
> > +maintainers:
> > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > +
> > +description:
> > +  Registers a Rotary encoder connected through a counter device.
>
> You shouldn't really describe the action here, but more what the
> binding is about. The registration will not depend on the presence of
> the node following that binding, but rather on whether or not the OS
> that uses it has support for it.
>

Then shall it be better with just :
"A rotary encoder device using a generic counter interface." ?

> > +properties:
> > +  compatible:
> > +    const: rotary-encoder-counter
> > +
> > +  counter:
> > +    description: Phandle for the counter device providing rotary position.
>
> This should have a type
>
> > +  linux-axis:
> > +    description: The input subsystem axis to map to this rotary encoder.
> > +    type: boolean
> > +
> > +  qdec-mode:
> > +    description: |
> > +      Quadrature decoder function to set in the counter device.
> > +      3: x1-PHA
> > +      4: x1-PHB
> > +      5: x2-PHA
> > +      6: x2-PHB
> > +      7: x4-PHA and PHB
>
> That range (even though it's a bit odd) should be expressed through an
> enum so that you can check that the values are actually within that
> range.
>

Indeed, that make sens to check it from the binding.

Will fix it in v2.

> > +  steps:
> > +    description: Number of steps in a full turnaround of the encoder.
>
> Muli-line strings should have either quotes around them, or a | or >
> like you did for the description. | will keep the \n, > will make that
> a single string.
>
> This should also have a type
>
> > +      Only relevant for absolute axis.
>
> This should be expressed through a if / then clause, or a dependencies one
>
> >                                         Defaults to 24 which is a typical
> > +      value for such devices.
>
> This should be expressed through a default property.
>

The devil is in the details and yet quite lot of them to fix.

Thanks.

> > +  relative-axis:
> > +    description: Register a relative axis rather than an absolute one.
> > +    type: boolean
> > +
> > +  rollover:
> > +    description: Automatic rollover when the rotary value becomes greater
> > +      than the specified steps or smaller than 0. For absolute axis only.
> > +    type: boolean
>
> Same story than steps for the dependency. Also, what is is the
> behaviour when this property isn't set?
>

OK, if rollover isn't set then the count is unbounded, of course this
shall be described here.

> > +  poll-interval:
> > +    description: Poll interval at which the position is read from the counter
> > +      device (default 500ms).
>
> It should have a type too, and a default property
>
> > +
> > +required:
> > +  - compatible
> > +  - counter
> > +  - qdec-mode
> > +
> > +examples:
> > +  - |
> > +    rotary@0 {
> > +        compatible = "rotary-encoder-counter";
>
> A unit-address (the part after @) only makes sense for a node if
> there's a matching reg property in the node. This will trigger a DTC
> warning, so you should remove the @0
>

Ok I'll fix it then.

Thanks again.

> Maxime



--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 2/3] Input: rotary-encoder-counter: add DT bindings

[Maxime Ripard]

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

On Tue, Apr 07, 2020 at 01:03:39PM +0200, Kamel Bouhara wrote:
> > On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > > Add dt binding for the counter variant of the rotary encoder driver.
> > >
> > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > ---
> > >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> > >  1 file changed, 67 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > new file mode 100644
> > > index 000000000000..a59f7c1faf0c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > @@ -0,0 +1,67 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> >
> > Bindings are usually used by other OS's, so you should consider
> > putting it under a more permissive license, usually that would be GPL2
> > and the BSD-2-Clause
>
> Well to be honest I just looked into an existing binding and I guess
> the wrong one :).

Not the wrong ones, but the old ones :)

It's painful to change a license on existing files, whereas it's
pretty easy to mention it during review.

> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Rotary Encoder Counter
> > > +
> > > +maintainers:
> > > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > +
> > > +description:
> > > +  Registers a Rotary encoder connected through a counter device.
> >
> > You shouldn't really describe the action here, but more what the
> > binding is about. The registration will not depend on the presence of
> > the node following that binding, but rather on whether or not the OS
> > that uses it has support for it.
> >
>
> Then shall it be better with just :
> "A rotary encoder device using a generic counter interface." ?

The generic counter interface is a Linux-only stuff though, some other
OS might want to implement something else. Something like "based on a
counter"?

Maxime

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

Re: [PATCH 2/3] Input: rotary-encoder-counter: add DT bindings

[Kamel Bouhara]

On Tue, Apr 07, 2020 at 04:22:38PM +0200, Maxime Ripard wrote:
> On Tue, Apr 07, 2020 at 01:03:39PM +0200, Kamel Bouhara wrote:
> > > On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > > > Add dt binding for the counter variant of the rotary encoder driver.
> > > >
> > > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > > ---
> > > >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> > > >  1 file changed, 67 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > > new file mode 100644
> > > > index 000000000000..a59f7c1faf0c
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > > @@ -0,0 +1,67 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > >
> > > Bindings are usually used by other OS's, so you should consider
> > > putting it under a more permissive license, usually that would be GPL2
> > > and the BSD-2-Clause
> >
> > Well to be honest I just looked into an existing binding and I guess
> > the wrong one :).
>
> Not the wrong ones, but the old ones :)
>
> It's painful to change a license on existing files, whereas it's
> pretty easy to mention it during review.
>

Alright.

> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Rotary Encoder Counter
> > > > +
> > > > +maintainers:
> > > > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > > +
> > > > +description:
> > > > +  Registers a Rotary encoder connected through a counter device.
> > >
> > > You shouldn't really describe the action here, but more what the
> > > binding is about. The registration will not depend on the presence of
> > > the node following that binding, but rather on whether or not the OS
> > > that uses it has support for it.
> > >
> >
> > Then shall it be better with just :
> > "A rotary encoder device using a generic counter interface." ?
>
> The generic counter interface is a Linux-only stuff though, some other
> OS might want to implement something else. Something like "based on a
> counter"?
>

Indeed, that's fair enough.

Thanks.

> Maxime



--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH 2/3] Input: rotary-encoder-counter: add DT bindings

[Dmitry Torokhov]

On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> Add dt binding for the counter variant of the rotary encoder driver.
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
>  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> new file mode 100644
> index 000000000000..a59f7c1faf0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rotary Encoder Counter
> +
> +maintainers:
> +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> +
> +description:
> +  Registers a Rotary encoder connected through a counter device.
> +
> +properties:
> +  compatible:
> +    const: rotary-encoder-counter

I wonder if a separate driver is really needed. The original driver be
taught to use counter device when available?

> +
> +  counter:
> +    description: Phandle for the counter device providing rotary position.
> +
> +  linux-axis:
> +    description: The input subsystem axis to map to this rotary encoder.
> +    type: boolean
> +
> +  qdec-mode:
> +    description: |
> +      Quadrature decoder function to set in the counter device.
> +      3: x1-PHA
> +      4: x1-PHB
> +      5: x2-PHA
> +      6: x2-PHB
> +      7: x4-PHA and PHB

Is it really property of the rotary encoder itself or property of the
counter device?

> +
> +  steps:
> +    description: Number of steps in a full turnaround of the encoder.
> +      Only relevant for absolute axis. Defaults to 24 which is a typical
> +      value for such devices.
> +
> +  relative-axis:
> +    description: Register a relative axis rather than an absolute one.
> +    type: boolean
> +
> +  rollover:
> +    description: Automatic rollover when the rotary value becomes greater
> +      than the specified steps or smaller than 0. For absolute axis only.
> +    type: boolean
> +
> +  poll-interval:
> +    description: Poll interval at which the position is read from the counter
> +      device (default 500ms).

Is there a way found counters to signal an interrupt?

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: rotary-encoder-counter: add DT bindings

[Alexandre Belloni]

Hi Dmitry,

On 09/04/2020 15:21:15-0700, Dmitry Torokhov wrote:
> On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > Add dt binding for the counter variant of the rotary encoder driver.
> > 
> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > ---
> >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > new file mode 100644
> > index 000000000000..a59f7c1faf0c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > @@ -0,0 +1,67 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rotary Encoder Counter
> > +
> > +maintainers:
> > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > +
> > +description:
> > +  Registers a Rotary encoder connected through a counter device.
> > +
> > +properties:
> > +  compatible:
> > +    const: rotary-encoder-counter
> 
> I wonder if a separate driver is really needed. The original driver be
> taught to use counter device when available?
> 

By the original driver, do you mean drivers/input/misc/rotary_encoder.c
that is using gpios ?

> > +
> > +  counter:
> > +    description: Phandle for the counter device providing rotary position.
> > +
> > +  linux-axis:
> > +    description: The input subsystem axis to map to this rotary encoder.
> > +    type: boolean
> > +
> > +  qdec-mode:
> > +    description: |
> > +      Quadrature decoder function to set in the counter device.
> > +      3: x1-PHA
> > +      4: x1-PHB
> > +      5: x2-PHA
> > +      6: x2-PHB
> > +      7: x4-PHA and PHB
> 
> Is it really property of the rotary encoder itself or property of the
> counter device?
> 

The mode the quadrature decoder has to be put in depends on both the
rotary encoder and the qdec.

> > +
> > +  steps:
> > +    description: Number of steps in a full turnaround of the encoder.
> > +      Only relevant for absolute axis. Defaults to 24 which is a typical
> > +      value for such devices.
> > +
> > +  relative-axis:
> > +    description: Register a relative axis rather than an absolute one.
> > +    type: boolean
> > +
> > +  rollover:
> > +    description: Automatic rollover when the rotary value becomes greater
> > +      than the specified steps or smaller than 0. For absolute axis only.
> > +    type: boolean
> > +
> > +  poll-interval:
> > +    description: Poll interval at which the position is read from the counter
> > +      device (default 500ms).
> 
> Is there a way found counters to signal an interrupt?
> 

For some counters, there are interrupts available, this is not trivial
with the counter that is the target of this work but this is on the TODO
list. Of course, this will also require adding a bit more to the
in-kernel counter API to allow registering a callback that would be
called when an interrupt happens.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 2/3] Input: rotary-encoder-counter: add DT bindings

[Dmitry Torokhov]

On Fri, Apr 10, 2020 at 12:39:07AM +0200, Alexandre Belloni wrote:
> Hi Dmitry,
> 
> On 09/04/2020 15:21:15-0700, Dmitry Torokhov wrote:
> > On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > > Add dt binding for the counter variant of the rotary encoder driver.
> > > 
> > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > ---
> > >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> > >  1 file changed, 67 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > new file mode 100644
> > > index 000000000000..a59f7c1faf0c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > @@ -0,0 +1,67 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Rotary Encoder Counter
> > > +
> > > +maintainers:
> > > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > +
> > > +description:
> > > +  Registers a Rotary encoder connected through a counter device.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: rotary-encoder-counter
> > 
> > I wonder if a separate driver is really needed. The original driver be
> > taught to use counter device when available?
> > 
> 
> By the original driver, do you mean drivers/input/misc/rotary_encoder.c
> that is using gpios ?

Yes.

> 
> > > +
> > > +  counter:
> > > +    description: Phandle for the counter device providing rotary position.
> > > +
> > > +  linux-axis:
> > > +    description: The input subsystem axis to map to this rotary encoder.
> > > +    type: boolean
> > > +
> > > +  qdec-mode:
> > > +    description: |
> > > +      Quadrature decoder function to set in the counter device.
> > > +      3: x1-PHA
> > > +      4: x1-PHB
> > > +      5: x2-PHA
> > > +      6: x2-PHB
> > > +      7: x4-PHA and PHB
> > 
> > Is it really property of the rotary encoder itself or property of the
> > counter device?
> > 
> 
> The mode the quadrature decoder has to be put in depends on both the
> rotary encoder and the qdec.

OK.

> 
> > > +
> > > +  steps:
> > > +    description: Number of steps in a full turnaround of the encoder.
> > > +      Only relevant for absolute axis. Defaults to 24 which is a typical
> > > +      value for such devices.
> > > +
> > > +  relative-axis:
> > > +    description: Register a relative axis rather than an absolute one.
> > > +    type: boolean
> > > +
> > > +  rollover:
> > > +    description: Automatic rollover when the rotary value becomes greater
> > > +      than the specified steps or smaller than 0. For absolute axis only.
> > > +    type: boolean
> > > +
> > > +  poll-interval:
> > > +    description: Poll interval at which the position is read from the counter
> > > +      device (default 500ms).
> > 
> > Is there a way found counters to signal an interrupt?
> > 
> 
> For some counters, there are interrupts available, this is not trivial
> with the counter that is the target of this work but this is on the TODO
> list. Of course, this will also require adding a bit more to the
> in-kernel counter API to allow registering a callback that would be
> called when an interrupt happens.

Should it be a callback, or can counter create an irqchip so that users
do not need to know how exactly it is wired up?

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: rotary-encoder-counter: add DT bindings

[Kamel Bouhara]

On Thu, Apr 09, 2020 at 04:46:23PM -0700, Dmitry Torokhov wrote:
> On Fri, Apr 10, 2020 at 12:39:07AM +0200, Alexandre Belloni wrote:
> > Hi Dmitry,
> >
> > On 09/04/2020 15:21:15-0700, Dmitry Torokhov wrote:
> > > On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > > > Add dt binding for the counter variant of the rotary encoder driver.
> > > >
> > > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > > ---
> > > >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> > > >  1 file changed, 67 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > > new file mode 100644
> > > > index 000000000000..a59f7c1faf0c
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > > @@ -0,0 +1,67 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Rotary Encoder Counter
> > > > +
> > > > +maintainers:
> > > > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > > +
> > > > +description:
> > > > +  Registers a Rotary encoder connected through a counter device.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: rotary-encoder-counter
> > >
> > > I wonder if a separate driver is really needed. The original driver be
> > > taught to use counter device when available?
> > >
> >
> > By the original driver, do you mean drivers/input/misc/rotary_encoder.c
> > that is using gpios ?
>
> Yes.
>

Well, it could be the case if the counter device could provide such a
way to signal interrupts.

> >
> > > > +
> > > > +  counter:
> > > > +    description: Phandle for the counter device providing rotary position.
> > > > +
> > > > +  linux-axis:
> > > > +    description: The input subsystem axis to map to this rotary encoder.
> > > > +    type: boolean
> > > > +
> > > > +  qdec-mode:
> > > > +    description: |
> > > > +      Quadrature decoder function to set in the counter device.
> > > > +      3: x1-PHA
> > > > +      4: x1-PHB
> > > > +      5: x2-PHA
> > > > +      6: x2-PHB
> > > > +      7: x4-PHA and PHB
> > >
> > > Is it really property of the rotary encoder itself or property of the
> > > counter device?
> > >
> >
> > The mode the quadrature decoder has to be put in depends on both the
> > rotary encoder and the qdec.
>
> OK.
>
> >
> > > > +
> > > > +  steps:
> > > > +    description: Number of steps in a full turnaround of the encoder.
> > > > +      Only relevant for absolute axis. Defaults to 24 which is a typical
> > > > +      value for such devices.
> > > > +
> > > > +  relative-axis:
> > > > +    description: Register a relative axis rather than an absolute one.
> > > > +    type: boolean
> > > > +
> > > > +  rollover:
> > > > +    description: Automatic rollover when the rotary value becomes greater
> > > > +      than the specified steps or smaller than 0. For absolute axis only.
> > > > +    type: boolean
> > > > +
> > > > +  poll-interval:
> > > > +    description: Poll interval at which the position is read from the counter
> > > > +      device (default 500ms).
> > >
> > > Is there a way found counters to signal an interrupt?
> > >
> >
> > For some counters, there are interrupts available, this is not trivial
> > with the counter that is the target of this work but this is on the TODO
> > list. Of course, this will also require adding a bit more to the
> > in-kernel counter API to allow registering a callback that would be
> > called when an interrupt happens.
>
> Should it be a callback, or can counter create an irqchip so that users
> do not need to know how exactly it is wired up?
>

Maybe for some of them yes but for others the polling is still required.

> Thanks.
>
> --
> Dmitry

--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[PATCH 3/3] Input: add a rotary encoders based on counter devices

[Kamel Bouhara]

This add support for rotary encoders that use the counter subsystem to
expose an input device.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 drivers/input/misc/Kconfig                  |   9 ++
 drivers/input/misc/Makefile                 |   1 +
 drivers/input/misc/rotary_encoder_counter.c | 152 ++++++++++++++++++++
 3 files changed, 162 insertions(+)
 create mode 100644 drivers/input/misc/rotary_encoder_counter.c

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 7e2e658d551c..b91b4257e337 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -619,6 +619,15 @@ config INPUT_GPIO_ROTARY_ENCODER
 	  To compile this driver as a module, choose M here: the
 	  module will be called rotary_encoder.
 
+config INPUT_COUNTER_ROTARY_ENCODER
+       tristate "Rotary encoders connected to counter devices"
+       depends on COUNTER || COMPILE_TEST
+       help
+         Say Y here to add support for rotary encoders connected to counter devices.
+
+         To compile this driver as a module, choose M here: the
+         module will be called rotary_encoder_counter.
+
 config INPUT_RB532_BUTTON
 	tristate "Mikrotik Routerboard 532 button interface"
 	depends on MIKROTIK_RB532
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 8fd187f314bd..74bbe6d400a3 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -68,6 +68,7 @@ obj-$(CONFIG_INPUT_REGULATOR_HAPTIC)	+= regulator-haptic.o
 obj-$(CONFIG_INPUT_RETU_PWRBUTTON)	+= retu-pwrbutton.o
 obj-$(CONFIG_INPUT_AXP20X_PEK)		+= axp20x-pek.o
 obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER)	+= rotary_encoder.o
+obj-$(CONFIG_INPUT_COUNTER_ROTARY_ENCODER) += rotary_encoder_counter.o
 obj-$(CONFIG_INPUT_RK805_PWRKEY)	+= rk805-pwrkey.o
 obj-$(CONFIG_INPUT_SC27XX_VIBRA)	+= sc27xx-vibra.o
 obj-$(CONFIG_INPUT_SGI_BTNS)		+= sgi_btns.o
diff --git a/drivers/input/misc/rotary_encoder_counter.c b/drivers/input/misc/rotary_encoder_counter.c
new file mode 100644
index 000000000000..20017308f4f3
--- /dev/null
+++ b/drivers/input/misc/rotary_encoder_counter.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * A rotary encoder driver using the generic counter interface.
+ *
+ * Author: Kamel Bouhara <kamel.bouhara@bootlin.com>
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/counter.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/property.h>
+
+#define MAX_STEPS	24
+
+struct rotary_encoder {
+	struct input_dev *input;
+	u32 steps;
+	u32 axis;
+	bool relative_axis;
+	bool rollover;
+	long last_pos;
+	struct counter_device *counter;
+};
+
+static void rotary_encoder_poll(struct input_dev *input)
+{
+	struct rotary_encoder *encoder = input_get_drvdata(input);
+	long rotary_pos;
+	int ret;
+
+	ret = counter_count_get(encoder->counter, &rotary_pos);
+	if (ret)
+		return;
+
+	if (encoder->relative_axis) {
+		input_report_rel(encoder->input, encoder->axis,
+				 rotary_pos - encoder->last_pos);
+	} else {
+		if (encoder->rollover)
+			rotary_pos %= encoder->steps;
+		input_report_abs(encoder->input, encoder->axis, rotary_pos);
+	}
+
+	encoder->last_pos = rotary_pos;
+	input_sync(encoder->input);
+}
+
+static int rotary_encoder_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct counter_device *counter;
+	struct rotary_encoder *encoder;
+	struct input_dev *input;
+	int qdec_mode;
+	u32 poll_interval;
+	int err;
+
+	encoder = devm_kzalloc(dev, sizeof(struct rotary_encoder), GFP_KERNEL);
+	if (!encoder)
+		return -ENOMEM;
+
+	encoder->rollover =
+		device_property_read_bool(dev, "rollover");
+
+	device_property_read_u32(dev, "steps", &encoder->steps);
+
+	device_property_read_u32(dev, "linux-axis", &encoder->axis);
+
+	encoder->relative_axis =
+		device_property_read_bool(dev, "relative-axis");
+
+	counter = devm_counter_get(dev);
+	if (IS_ERR(counter))
+		return PTR_ERR(counter);
+
+	if (device_property_read_u32(dev, "qdec-mode", &qdec_mode)) {
+		dev_err(dev, "Invalid or missing quadrature mode\n");
+		return -EINVAL;
+	}
+
+	err = counter_function_set(counter, qdec_mode);
+	if (err) {
+		dev_err(dev, "Failed to set quadrature mode %d\n",
+			qdec_mode);
+		return err;
+	}
+
+	input = devm_input_allocate_device(dev);
+	if (!input)
+		return -ENOMEM;
+
+	input_set_drvdata(input, encoder);
+	encoder->input = input;
+	encoder->counter = counter;
+	encoder->steps = (!encoder->steps) ? MAX_STEPS : encoder->steps;
+	input->name = pdev->name;
+	input->id.bustype = BUS_HOST;
+	input->dev.parent = dev;
+
+	if (encoder->relative_axis)
+		input_set_capability(input, EV_REL, encoder->axis);
+	else
+		input_set_abs_params(input, encoder->axis, 0,
+				     encoder->steps, 0, 1);
+
+	err = input_setup_polling(input, rotary_encoder_poll);
+	if (err)
+		return err;
+
+	if (!device_property_read_u32(dev, "poll-interval",
+				      &poll_interval))
+		input_set_poll_interval(input, poll_interval);
+
+	err = input_register_device(input);
+	if (err) {
+		dev_err(dev, "failed to register device, err=%d\n", err);
+		return err;
+	}
+
+	platform_set_drvdata(pdev, encoder);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id rotary_encoder_of_match[] = {
+	{ .compatible = "rotary-encoder-counter", },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, rotary_encoder_of_match);
+#endif
+
+static struct platform_driver rotary_encoder_driver = {
+	.probe		= rotary_encoder_probe,
+	.driver		= {
+		.name	= "rotary-encoder-counter",
+		.of_match_table = of_match_ptr(rotary_encoder_of_match),
+	}
+};
+module_platform_driver(rotary_encoder_driver);
+
+MODULE_DESCRIPTION("Counter rotary encoder driver");
+MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.25.0

Re: [PATCH 0/3] Introduce a counter inkernel API

[William Breathitt Gray]

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

On Mon, Apr 06, 2020 at 05:58:03PM +0200, Kamel Bouhara wrote:
> Hello everyone,
> 
> This series introduce a basic inkernel API for the counter subsystem and
> add a new rotary encoder driver that use a counter interface instead of
> the GPIO existing one.
> 
> See commit log in 0001-counter-add-an-inkernel-API.patch for further
> details.
> 
> Kamel Bouhara (3):
>   counter: add an inkernel API
>   Input: rotary-encoder-counter: add DT bindings
>   Input: add a rotary encoders based on counter devices
> 
>  .../input/rotary-encoder-counter.yaml         |  67 ++++++
>  drivers/counter/counter.c                     | 213 ++++++++++++++++++
>  drivers/input/misc/Kconfig                    |   9 +
>  drivers/input/misc/Makefile                   |   1 +
>  drivers/input/misc/rotary_encoder_counter.c   | 152 +++++++++++++
>  include/linux/counter.h                       |  27 +++
>  6 files changed, 469 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
>  create mode 100644 drivers/input/misc/rotary_encoder_counter.c
> 
> --
> 2.25.0

Hello Kamel,

I'm not inherently opposed to adding an in-kernel API for the Counter
subsystem, but I'm not sure yet if it's necessary for this particular
situation.

Is the purpose of this driver to allow users to poll on the rotary
encoder position value? If so, perhaps instead of an in-kernel API, the
polling functionality should be added as part of the Counter subsystem;
I can see this being a useful feature for many counter devices, and
it'll keep the code contained to a single subsystem.

By the way, I'm going to be submitting a major update to the Counter
subsystem code in the next couple weeks that isolates the sysfs code
from the rest of the subsystem -- it'll likely affect the interface and
code here -- so I'll probably wait to decide for certain until that
patch lands; I anticipate it making things easier for you here after
it's merged.

For now, I want to get a better high-level understanding about how users
would interact with this driver to use the device (input_setup_polling
is a new call for me). That should help me understand whether an
in-kernel API is the best choice here.

William Breathitt Gray

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

Re: [PATCH 0/3] Introduce a counter inkernel API

[Alexandre Belloni]

Hi,

On 11/04/2020 13:22:59-0400, William Breathitt Gray wrote:
> I'm not inherently opposed to adding an in-kernel API for the Counter
> subsystem, but I'm not sure yet if it's necessary for this particular
> situation.
> 
> Is the purpose of this driver to allow users to poll on the rotary
> encoder position value? If so, perhaps instead of an in-kernel API, the
> polling functionality should be added as part of the Counter subsystem;
> I can see this being a useful feature for many counter devices, and
> it'll keep the code contained to a single subsystem.
> 
> By the way, I'm going to be submitting a major update to the Counter
> subsystem code in the next couple weeks that isolates the sysfs code
> from the rest of the subsystem -- it'll likely affect the interface and
> code here -- so I'll probably wait to decide for certain until that
> patch lands; I anticipate it making things easier for you here after
> it's merged.
> 
> For now, I want to get a better high-level understanding about how users
> would interact with this driver to use the device (input_setup_polling
> is a new call for me). That should help me understand whether an
> in-kernel API is the best choice here.
> 

Well, the goal is not really polling the counters but mainly exposing
the correct userspace interface for the rotary encoders that are
connected to quadrature decoders.

The input driver is using polling because this reduces the complexity of
the patches but the ultimate goal is to also have interrupts working.

I'm pretty sure the in-kernel interface can also have other usages like
for example iio triggers. I could envision having to read an ADC after x
turns of a motor to check for the torque.

I also think that having the sysfs code separate would help as it could
be considered as one of the in-kernel interface user.

BTW, do you have plans to add a character device interface?

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/3] Introduce a counter inkernel API

[William Breathitt Gray]

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

On Sun, Apr 12, 2020 at 01:31:45AM +0200, Alexandre Belloni wrote:
> Hi,
> 
> On 11/04/2020 13:22:59-0400, William Breathitt Gray wrote:
> > I'm not inherently opposed to adding an in-kernel API for the Counter
> > subsystem, but I'm not sure yet if it's necessary for this particular
> > situation.
> > 
> > Is the purpose of this driver to allow users to poll on the rotary
> > encoder position value? If so, perhaps instead of an in-kernel API, the
> > polling functionality should be added as part of the Counter subsystem;
> > I can see this being a useful feature for many counter devices, and
> > it'll keep the code contained to a single subsystem.
> > 
> > By the way, I'm going to be submitting a major update to the Counter
> > subsystem code in the next couple weeks that isolates the sysfs code
> > from the rest of the subsystem -- it'll likely affect the interface and
> > code here -- so I'll probably wait to decide for certain until that
> > patch lands; I anticipate it making things easier for you here after
> > it's merged.
> > 
> > For now, I want to get a better high-level understanding about how users
> > would interact with this driver to use the device (input_setup_polling
> > is a new call for me). That should help me understand whether an
> > in-kernel API is the best choice here.
> > 
> 
> Well, the goal is not really polling the counters but mainly exposing
> the correct userspace interface for the rotary encoders that are
> connected to quadrature decoders.
> 
> The input driver is using polling because this reduces the complexity of
> the patches but the ultimate goal is to also have interrupts working.

Okay, I think understand now. Interrupt support is another feature I
want to get working for counters too, so that development will probably
overlap with this driver as well. Hopefully with interrupts working
you'll be able to signal to the input driver whenever data is ready,
rather than just polling periodically to check.

> I'm pretty sure the in-kernel interface can also have other usages like
> for example iio triggers. I could envision having to read an ADC after x
> turns of a motor to check for the torque.

That's an interesting use case. I can see how an in-kernel interface
would be helpful here.

> I also think that having the sysfs code separate would help as it could
> be considered as one of the in-kernel interface user.
> 
> BTW, do you have plans to add a character device interface?

Yes, actually a character device interface (and the timestamp feature)
is the primary motivation for this refactoring: sysfs code is separated
so that it can share a common core of functionality with the character
device code.

Implementing an in-kernel API should be trivial after these changes
since it will be just a matter of codifying the shared code that forms
the new core of the Counter subsystem. So perhaps this patchset should
wait until the Counter subsystem is updated, since it may be easier to
interact with counter devices once that is complete.

If you're curious about this patch, it's available on my personal iio
tree in the counter_chardev branch:
https://gitlab.com/vilhelmgray/iio/-/commits/counter_chardev/

I still need to add the character device code and respective userspace
API, but I expect to have it completed in the next couple weeks,
assuming no major issues or delays arise.

William Breathitt Gray

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