input: rotary_encoder: use more than two gpios as input

Signed-off-by Uwe Kleine-K?nig <[email protected]>

---
 .../devicetree/bindings/input/rotary-encoder.txt   |   2 +-

 arch/arm/mach-pxa/raumfeld.c                       |  25 +++-
 drivers/input/misc/rotary_encoder.c                | 127 +++++++++------------
 include/linux/rotary_encoder.h                     |   4 -
 4 files changed, 79 insertions(+), 79 deletions(-)

Documentation/devicetree/bindings/input/rotary-encoder.txt +1 -1

index de99cbbbf6da..6c9f0c8a846c 100644
@@ -1,7 +1,7 @@
 Rotary encoder DT bindings
 
 Required properties:
-- gpios: a spec for two GPIOs to be used
+- gpios: a spec for at least two GPIOs to be used, most significant first
 
 Optional properties:
 - linux,axis: the input subsystem axis to map to this rotary encoder.

arch/arm/mach-pxa/raumfeld.c +21 -4

index 36571a9a44fe..f47e59b0efa8 100644
@@ -21,6 +21,7 @@
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/gpio.h>
+#include <linux/gpio/machine.h>
 #include <linux/smsc911x.h>
 #include <linux/input.h>
 #include <linux/rotary_encoder.h>

@@ -370,10 +371,25 @@ static struct rotary_encoder_platform_data raumfeld_rotary_encoder_info = {
 	.steps		= 24,
 	.axis		= REL_X,
 	.relative_axis	= 1,
-	.gpio_a		= GPIO_VOLENC_A,
-	.gpio_b		= GPIO_VOLENC_B,
-	.inverted_a	= 1,
-	.inverted_b	= 0,
+};
+
+static struct gpiod_lookup_table raumfeld_rotary_encoder_gpio_lookup = {
+	.dev_id = "rotary_encoder.0",
+	.table = {
+		{
+			.chip_label = "gpio-0",
+			.chip_hwnum = GPIO_VOLENC_A,
+			.idx = 0,
+			.flags = GPIO_ACTIVE_LOW,
+		},
+		{
+			.chip_label = "gpio-0",
+			.chip_hwnum = GPIO_VOLENC_B,
+			.idx = 1,
+			.flags = GPIO_ACTIVE_HIGH,
+		},
+		{ /* sentinel */ }
+	},
 };
 
 static struct platform_device rotary_encoder_device = {

@@ -1051,6 +1067,7 @@ static void __init raumfeld_controller_init(void)
 	int ret;
 
 	pxa3xx_mfp_config(ARRAY_AND_SIZE(raumfeld_controller_pin_config));
+	gpiod_add_lookup_table(&raumfeld_rotary_encoder_gpio_lookup);
 	platform_device_register(&rotary_encoder_device);
 	spi_register_board_info(ARRAY_AND_SIZE(controller_spi_devices));
 	i2c_register_board_info(0, &raumfeld_controller_i2c_board_info, 1);

drivers/input/misc/rotary_encoder.c +57 -70

index 0582e851993f..99f19e037370 100644
@@ -25,7 +25,6 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
-#include <linux/of_gpio.h>
 #include <linux/pm.h>
 
 #define DRV_NAME "rotary-encoder"

@@ -36,45 +35,47 @@ struct rotary_encoder {
 	/* configuration */
 	unsigned int steps;
 	unsigned int axis;
-	unsigned int gpio_a;
-	unsigned int gpio_b;
-	unsigned int inverted_a;
-	unsigned int inverted_b;
 	unsigned int steps_per_period;
 	bool relative_axis;
 	bool rollover;
 	bool wakeup_source;
 
-	unsigned int irq_a;
-	unsigned int irq_b;
+	struct gpio_descs *gpios;
+	unsigned int *irq;
 
 	/* state */
 	unsigned int pos;
 	bool armed;
-	unsigned char dir;	/* 0 - clockwise, 1 - CCW */
+	signed char dir;	/* 1 - clockwise, -1 - CCW */
 	char last_stable;
 };
 
-static int rotary_encoder_get_state(const struct rotary_encoder *encoder)
+static unsigned rotary_encoder_get_state(const struct rotary_encoder *encoder)
 {
-	int a = !!gpio_get_value(encoder->gpio_a);
-	int b = !!gpio_get_value(encoder->gpio_b);
+	int i;
+	unsigned ret = 0;
 
-	a ^= encoder->inverted_a;
-	b ^= encoder->inverted_b;
+	for (i = 0; i < encoder->gpios->ndescs; ++i) {
+		int val = gpiod_get_value(encoder->gpios->desc[i]);
+		/* convert from gray encoding to normal */
+		if (ret & 1)
+			val = !val;
 
-	return ((a << 1) | b);
+		ret = ret << 1 | val;
+	}
+
+	return ret & 3;
 }
 
 static void rotary_encoder_report_event(struct rotary_encoder *encoder)
 {
 	if (encoder->relative_axis) {
 		input_report_rel(encoder->input,
-				 encoder->axis, encoder->dir ? -1 : 1);
+				 encoder->axis, encoder->dir);
 	} else {
 		unsigned int pos = encoder->pos;
 
-		if (encoder->dir) {
+		if (encoder->dir < 0) {
 			/* turning counter-clockwise */
 			if (encoder->rollover)
 				pos += encoder->steps;

@@ -112,12 +113,12 @@ static irqreturn_t rotary_encoder_irq(int irq, void *dev_id)
 		break;
 
 	case 0x1:
-	case 0x2:
+	case 0x3:
 		if (encoder->armed)
-			encoder->dir = state - 1;
+			encoder->dir = 2 - state;
 		break;
 
-	case 0x3:
+	case 0x2:
 		encoder->armed = true;
 		break;
 	}

@@ -134,7 +135,7 @@ static irqreturn_t rotary_encoder_half_period_irq(int irq, void *dev_id)
 
 	switch (state) {
 	case 0x00:
-	case 0x03:
+	case 0x02:
 		if (state != encoder->last_stable) {
 			rotary_encoder_report_event(encoder);
 			encoder->last_stable = state;

@@ -142,8 +143,8 @@ static irqreturn_t rotary_encoder_half_period_irq(int irq, void *dev_id)
 		break;
 
 	case 0x01:
-	case 0x02:
-		encoder->dir = (encoder->last_stable + state) & 0x01;
+	case 0x03:
+		encoder->dir = ((encoder->last_stable - state + 1) % 4) - 1;
 		break;
 	}
 

@@ -212,7 +213,6 @@ static int rotary_encoder_parse_dt(struct device *dev,
 	const struct of_device_id *of_id =
 				of_match_device(rotary_encoder_of_match, dev);
 	struct device_node *np = dev->of_node;
-	enum of_gpio_flags flags;
 	int error;
 
 	if (!of_id || !np)

@@ -221,12 +221,6 @@ static int rotary_encoder_parse_dt(struct device *dev,
 	of_property_read_u32(np, "rotary-encoder,steps", &encoder->steps);
 	of_property_read_u32(np, "linux,axis", &encoder->axis);
 
-	encoder->gpio_a = of_get_gpio_flags(np, 0, &flags);
-	encoder->inverted_a = flags & OF_GPIO_ACTIVE_LOW;
-
-	encoder->gpio_b = of_get_gpio_flags(np, 1, &flags);
-	encoder->inverted_b = flags & OF_GPIO_ACTIVE_LOW;
-
 	encoder->relative_axis =
 		of_property_read_bool(np, "rotary-encoder,relative-axis");
 	encoder->rollover =

@@ -273,11 +267,6 @@ static int rotary_encoder_parse_pdata(struct device *dev,
 
 	encoder->steps = pdata->steps;
 	encoder->axis = pdata->axis;
-	encoder->gpio_a = pdata->gpio_a;
-	encoder->gpio_b = pdata->gpio_b;
-	encoder->inverted_a = pdata->inverted_a;
-	encoder->inverted_b = pdata->inverted_b;
-	encoder->steps_per_period = pdata->steps_per_period;
 	encoder->relative_axis = pdata->relative_axis;
 	encoder->rollover = pdata->rollover;
 

@@ -291,6 +280,7 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 	struct input_dev *input;
 	irq_handler_t handler;
 	int err;
+	unsigned int i;
 
 	encoder = devm_kzalloc(dev, sizeof(struct rotary_encoder), GFP_KERNEL);
 	input = devm_input_allocate_device(&pdev->dev);

@@ -307,6 +297,16 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 	if (err < 0)
 		return err;
 
+	encoder->gpios = devm_gpiod_get_array(dev, NULL, GPIOD_IN);
+	if (IS_ERR(encoder->gpios)) {
+		dev_err(dev, "unable to get gpios\n");
+		return PTR_ERR(encoder->gpios);
+	}
+	if (encoder->gpios->ndescs < 2) {
+		dev_err(dev, "not enough gpios found\n");
+		return -EINVAL;
+	}
+
 	encoder->input = input;
 
 	input->name = pdev->name;

@@ -322,25 +322,7 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 				     encoder->axis, 0, encoder->steps, 0, 1);
 	}
 
-	/* request the GPIOs */
-	err = devm_gpio_request_one(dev, encoder->gpio_a,
-				    GPIOF_IN, dev_name(dev));
-	if (err) {
-		dev_err(dev, "unable to request GPIO %d\n", encoder->gpio_a);
-		return err;
-	}
-
-	err = devm_gpio_request_one(dev, encoder->gpio_b,
-				    GPIOF_IN, dev_name(dev));
-	if (err) {
-		dev_err(dev, "unable to request GPIO %d\n", encoder->gpio_b);
-		return err;
-	}
-
-	encoder->irq_a = gpio_to_irq(encoder->gpio_a);
-	encoder->irq_b = gpio_to_irq(encoder->gpio_b);
-
-	switch (encoder->steps_per_period) {
+	switch (encoder->steps_per_period >> (encoder->gpios->ndescs - 2)) {
 	case 4:
 		handler = &rotary_encoder_quarter_period_irq;
 		encoder->last_stable = rotary_encoder_get_state(encoder);

@@ -358,20 +340,23 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	err = devm_request_irq(dev, encoder->irq_a, handler,
-			       IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-			       DRV_NAME, encoder);
-	if (err) {
-		dev_err(dev, "unable to request IRQ %d\n", encoder->irq_a);
-		return err;
-	}
+	encoder->irq =
+		devm_kzalloc(dev,
+			     sizeof(*encoder->irq) * encoder->gpios->ndescs,
+			     GFP_KERNEL);
+	if (!encoder->irq)
+		return -ENOMEM;
 
-	err = devm_request_irq(dev, encoder->irq_b, handler,
-			       IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-			       DRV_NAME, encoder);
-	if (err) {
-		dev_err(dev, "unable to request IRQ %d\n", encoder->irq_b);
-		return err;
+	for (i = 0; i < encoder->gpios->ndescs; ++i) {
+		encoder->irq[i] = gpiod_to_irq(encoder->gpios->desc[i]);
+		err = devm_request_irq(dev, encoder->irq[i], handler,
+				       IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				       DRV_NAME, encoder);
+		if (err) {
+			dev_err(dev, "unable to request IRQ %d (gpio#%d)\n",
+				encoder->irq[i], i);
+			return err;
+		}
 	}
 
 	err = input_register_device(input);

@@ -399,8 +384,9 @@ static int rotary_encoder_suspend(struct device *dev)
 	struct rotary_encoder *encoder = dev_get_drvdata(dev);
 
 	if (device_may_wakeup(dev)) {
-		enable_irq_wake(encoder->irq_a);
-		enable_irq_wake(encoder->irq_b);
+		unsigned int i;
+		for (i = 0; i < encoder->gpios->ndescs; ++i)
+			enable_irq_wake(encoder->irq[i]);
 	}
 
 	return 0;

@@ -411,8 +397,9 @@ static int rotary_encoder_resume(struct device *dev)
 	struct rotary_encoder *encoder = dev_get_drvdata(dev);
 
 	if (device_may_wakeup(dev)) {
-		disable_irq_wake(encoder->irq_a);
-		disable_irq_wake(encoder->irq_b);
+		unsigned int i;
+		for (i = 0; i < encoder->gpios->ndescs; ++i)
+			disable_irq_wake(encoder->irq[i]);
 	}
 
 	return 0;

include/linux/rotary_encoder.h +0 -4

index fe3dc64e5aeb..4536c813a1e9 100644
@@ -4,10 +4,6 @@
 struct rotary_encoder_platform_data {
 	unsigned int steps;
 	unsigned int axis;
-	unsigned int gpio_a;
-	unsigned int gpio_b;
-	unsigned int inverted_a;
-	unsigned int inverted_b;
 	unsigned int steps_per_period;
 	bool relative_axis;
 	bool rollover;

Signature · 1 line

2.6.2

Hello,

some time ago I worked on the rotary encoder driver to make it support more than two input lines. This is the (only slightly tested[1]) rebase of this series on top of 4.4-rc2 (from 4.1).

It would be great to get some feedback, especially (but not only) for my change to raumfeld.c.

Before Ezequiel's patch 3a341a4c30d4 ("Input: rotary-encoder - add support for quarter-period mode") we had a dt property "rotary-encoder,half-period" defined. It's presence meant that we had 2 indents per period; it's absence that there is only 1. Ezequiel introduced rotary-encoder,steps-per-period instead when adding support for quarter-period mode (which has 4 indents per period).

Up to now (i.e. with two inputs) a period has length 4. Now with (say) four inputs a period has length 16 instead. Now how should this be modeled in dt? This series implements that I have to pass

	rotary-encoder,steps-per-period = <16>;

now for "quarter-period mode" (i.e. 4 indents per 4 state changes[2]), but that feels unnatural. I'd prefer to set a property to <1> instead, meaning the device has an indent for each state change[2]. half-period mode would be <2> and full-period mode would be <4>. But I don't have a nice name for such a property and maybe it's easier to live with steps-per-period = <16>? What do you think?

Also note that these patches are not as technically versatile as possible. With 4 (n) input lines we could detect a quick rotation where the machine's latency only allows to sample after 7 (2^(n-1)-1) state changes. Currently this is not implemented, but can be done later.

Best regards
Uwe

Quoted 32 lines · On Wednesday 02 December 2015 11:07:11 Uwe Kleine-K?nig wrote:

some time ago I worked on the rotary encoder driver to make it support more than two input lines. This is the (only slightly tested[1]) rebase of this series on top of 4.4-rc2 (from 4.1).

It would be great to get some feedback, especially (but not only) for my change to raumfeld.c.

Before Ezequiel's patch 3a341a4c30d4 ("Input: rotary-encoder - add support for quarter-period mode") we had a dt property "rotary-encoder,half-period" defined. It's presence meant that we had 2 indents per period; it's absence that there is only 1. Ezequiel introduced rotary-encoder,steps-per-period instead when adding support for quarter-period mode (which has 4 indents per period).

Up to now (i.e. with two inputs) a period has length 4. Now with (say) four inputs a period has length 16 instead. Now how should this be modeled in dt? This series implements that I have to pass

	rotary-encoder,steps-per-period = <16>;

now for "quarter-period mode" (i.e. 4 indents per 4 state changes[2]), but that feels unnatural. I'd prefer to set a property to <1> instead, meaning the device has an indent for each state change[2]. half-period mode would be <2> and full-period mode would be <4>. But I don't have a nice name for such a property and maybe it's easier to live with steps-per-period = <16>? What do you think?

Also note that these patches are not as technically versatile as possible. With 4 (n) input lines we could detect a quick rotation where the machine's latency only allows to sample after 7 (2^(n-1)-1) state changes. Currently this is not implemented, but can be done later.

Quoted 3 lines

Best regards
Uwe

AFAIUI the rotary encoder driver only handles incremental encoders not absolute encoders. (So in fact the assumed rotary encoder could also be a

linear encoder with an incremental interface.) Those encoders almost always have an interface with two outputs (A, B) with a phase shift of 90 degrees between them. So in this case we have 4 steps per period. Sometimes there is only one line for 1 or 2 steps per period. But I have never seen or

heard of a device with more than 2 lines (except for the third output which serves as a reference position index marker).

Do devices wirh more than two outputs actually exist?
Or is the purpose of supporting more than 2 lines something other than connecting an actual encoder to them?

Hello,

Quoted 37 lines · Rojhalat Ibrahim

Quoted 35 lines · On Wednesday 02 December 2015 11:07:11 Uwe Kleine-K?nig wrote:

some time ago I worked on the rotary encoder driver to make it support more than two input lines. This is the (only slightly tested[1]) rebase of this series on top of 4.4-rc2 (from 4.1).

It would be great to get some feedback, especially (but not only) for my change to raumfeld.c.

Before Ezequiel's patch 3a341a4c30d4 ("Input: rotary-encoder - add support for quarter-period mode") we had a dt property "rotary-encoder,half-period" defined. It's presence meant that we had 2 indents per period; it's absence that there is only 1. Ezequiel introduced rotary-encoder,steps-per-period instead when adding support for quarter-period mode (which has 4 indents per period).

Up to now (i.e. with two inputs) a period has length 4. Now with (say) four inputs a period has length 16 instead. Now how should this be modeled in dt? This series implements that I have to pass

	rotary-encoder,steps-per-period = <16>;

now for "quarter-period mode" (i.e. 4 indents per 4 state changes[2]), but that feels unnatural. I'd prefer to set a property to <1> instead, meaning the device has an indent for each state change[2]. half-period mode would be <2> and full-period mode would be <4>. But I don't have a nice name for such a property and maybe it's easier to live with steps-per-period = <16>? What do you think?

Also note that these patches are not as technically versatile as possible. With 4 (n) input lines we could detect a quick rotation where the machine's latency only allows to sample after 7 (2^(n-1)-1) state changes. Currently this is not implemented, but can be done later.

Best regards
Uwe

Quoted 2 lines

AFAIUI the rotary encoder driver only handles incremental encoders not absolute encoders. (So in fact the assumed rotary encoder could also be a

There is a device tree property "rotary-encoder,relative-axis" which switches between absolute and relative reporting. This is maybe badly named, but IIUC is there to differenciate between incremental and absolute encoders.

Quoted 4 lines

linear encoder with an incremental interface.) Those encoders almost always have an interface with two outputs (A, B) with a phase shift of 90 degrees between them. So in this case we have 4 steps per period. Sometimes there is only one line for 1 or 2 steps per period. But I have never seen or

I don't see how using only a single gpio would work for a rotary encoder. I suspect we use different vocabulary to describe our devices and so don't understand each other?! At least you cannot detect the direction of the movement with a single input line, do you?

Quoted 6 lines

heard of a device with more than 2 lines (except for the third output which serves as a reference position index marker).

Do devices wirh more than two outputs actually exist?
Or is the purpose of supporting more than 2 lines something other than connecting an actual encoder to them?

I have a real rotary encoder with 4 input lines and so 16 distinguishable positions. It would be described as:

	compatible = "rotary-encoder";
	gpios = <&gpio4 12 GPIO_ACTIVE_HIGH>, <&gpio4 11 GPIO_ACTIVE_HIGH>, <&gpio4 10 GPIO_ACTIVE_HIGH>, <&gpio4 9 GPIO_ACTIVE_HIGH>;
	rotary-encoder,steps = <16>;
	rotary-encoder,steps-per-period = <16>;

with the binding implemented in my patches.

The wikipedia article about rotary encoders[1] uses a device with 3 input lines to explain gray encoding. So I didn't consider "my" device as exotic up to now.

Best regards
Uwe

[1] https://en.wikipedia.org/wiki/Rotary_encoder

Quoted 8 lines · On Wednesday 02 December 2015 19:59:20 Uwe Kleine-K?nig wrote:

I have a real rotary encoder with 4 input lines and so 16 distinguishable positions. It would be described as:

	compatible = "rotary-encoder";
	gpios = <&gpio4 12 GPIO_ACTIVE_HIGH>, <&gpio4 11 GPIO_ACTIVE_HIGH>, <&gpio4 10 GPIO_ACTIVE_HIGH>, <&gpio4 9 GPIO_ACTIVE_HIGH>;
	rotary-encoder,steps = <16>;
	rotary-encoder,steps-per-period = <16>;

Quoted 6 lines

with the binding implemented in my patches.

The wikipedia article about rotary encoders[1] uses a device with 3 input lines to explain gray encoding. So I didn't consider "my" device as exotic up to now.

So you have an absolute encoder. From the state of the four input lines you can determine the absolute position value. There is no need to count steps. At least that's the original purpose of your device.

Of course it might be conceivable to use that kind of absolute encoder in an incremental way and count the steps throughout multiple revolutions. But why would you do that? The resolution of 16 steps per revolution is not very good (incremental encoders often have hundreds or thousands of steps per revolution). And the whole point of an absolute encoder is that you are able to determine the current position at any time just by looking at the inputs without having to count steps.

Quoted 21 lines · Rojhalat Ibrahim

Quoted 8 lines · On Wednesday 02 December 2015 19:59:20 Uwe Kleine-K?nig wrote:

I have a real rotary encoder with 4 input lines and so 16 distinguishable positions. It would be described as:

	compatible = "rotary-encoder";
	gpios = <&gpio4 12 GPIO_ACTIVE_HIGH>, <&gpio4 11 GPIO_ACTIVE_HIGH>, <&gpio4 10 GPIO_ACTIVE_HIGH>, <&gpio4 9 GPIO_ACTIVE_HIGH>;
	rotary-encoder,steps = <16>;
	rotary-encoder,steps-per-period = <16>;

Quoted 6 lines

with the binding implemented in my patches.

The wikipedia article about rotary encoders[1] uses a device with 3 input lines to explain gray encoding. So I didn't consider "my" device as exotic up to now.

So you have an absolute encoder. From the state of the four input lines you can determine the absolute position value. There is no need to count steps. At least that's the original purpose of your device.

Quoted 7 lines

Of course it might be conceivable to use that kind of absolute encoder in an incremental way and count the steps throughout multiple revolutions. But why would you do that? The resolution of 16 steps per revolution is not very good (incremental encoders often have hundreds or thousands of steps per revolution). And the whole point of an absolute encoder is that you are able to determine the current position at any time just by looking at the inputs without having to count steps.

Yes, you're right, that's actually a very different kind of device which should be supported with a different mode or something, so that the output of the driver reflects the absolute state of the encoder. This even simplifies things in the driver: The entire state logic would be bypassed in this mode, and the current state of the GPIO lines would be used directly to get the absolute value. Makes sense?

Thanks,
Daniel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel