Input: rotary-encoder - use more than two gpios

For all operations called in .probe there are devm_* variants. (input_register_device is clever enough to be devm aware on its own.) This allows to get rid of the error unwind code paths in .probe and the complete .remove function.

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

---
 drivers/input/misc/rotary_encoder.c | 74 +++++++++++--------------------------
 1 file changed, 22 insertions(+), 52 deletions(-)

drivers/input/misc/rotary_encoder.c +22 -52

index 8aee71986430..386bdb5314e6 100644
@@ -211,8 +211,8 @@ static struct rotary_encoder_platform_data *rotary_encoder_parse_dt(struct devic
 	if (!of_id || !np)
 		return NULL;
 
-	pdata = kzalloc(sizeof(struct rotary_encoder_platform_data),
-			GFP_KERNEL);
+	pdata = devm_kzalloc(dev, sizeof(struct rotary_encoder_platform_data),
+			     GFP_KERNEL);
 	if (!pdata)
 		return ERR_PTR(-ENOMEM);
 

@@ -277,11 +277,11 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 		}
 	}
 
-	encoder = kzalloc(sizeof(struct rotary_encoder), GFP_KERNEL);
-	input = input_allocate_device();
+	encoder = devm_kzalloc(dev, sizeof(struct rotary_encoder), GFP_KERNEL);
+	input = devm_input_allocate_device(&pdev->dev);
 	if (!encoder || !input) {
-		err = -ENOMEM;
-		goto exit_free_mem;
+		dev_err(dev, "unable to allocate memory\n");
+		return -ENOMEM;
 	}
 
 	encoder->input = input;

@@ -301,16 +301,18 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 	}
 
 	/* request the GPIOs */
-	err = gpio_request_one(pdata->gpio_a, GPIOF_IN, dev_name(dev));
+	err = devm_gpio_request_one(dev, pdata->gpio_a,
+				    GPIOF_IN, dev_name(dev));
 	if (err) {
 		dev_err(dev, "unable to request GPIO %d\n", pdata->gpio_a);
-		goto exit_free_mem;
+		return err;
 	}
 
-	err = gpio_request_one(pdata->gpio_b, GPIOF_IN, dev_name(dev));
+	err = devm_gpio_request_one(dev, pdata->gpio_b,
+				    GPIOF_IN, dev_name(dev));
 	if (err) {
 		dev_err(dev, "unable to request GPIO %d\n", pdata->gpio_b);
-		goto exit_free_gpio_a;
+		return err;
 	}
 
 	encoder->irq_a = gpio_to_irq(pdata->gpio_a);

@@ -331,30 +333,29 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 	default:
 		dev_err(dev, "'%d' is not a valid steps-per-period value\n",
 			pdata->steps_per_period);
-		err = -EINVAL;
-		goto exit_free_gpio_b;
+		return -EINVAL;
 	}
 
-	err = request_irq(encoder->irq_a, handler,
-			  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-			  DRV_NAME, encoder);
+	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);
-		goto exit_free_gpio_b;
+		return err;
 	}
 
-	err = request_irq(encoder->irq_b, handler,
-			  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-			  DRV_NAME, encoder);
+	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);
-		goto exit_free_irq_a;
+		return err;
 	}
 
 	err = input_register_device(input);
 	if (err) {
 		dev_err(dev, "failed to register input device\n");
-		goto exit_free_irq_b;
+		return err;
 	}
 
 	device_init_wakeup(&pdev->dev, pdata->wakeup_source);

@@ -362,42 +363,11 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, encoder);
 
 	return 0;
-
-exit_free_irq_b:
-	free_irq(encoder->irq_b, encoder);
-exit_free_irq_a:
-	free_irq(encoder->irq_a, encoder);
-exit_free_gpio_b:
-	gpio_free(pdata->gpio_b);
-exit_free_gpio_a:
-	gpio_free(pdata->gpio_a);
-exit_free_mem:
-	input_free_device(input);
-	kfree(encoder);
-	if (!dev_get_platdata(&pdev->dev))
-		kfree(pdata);
-
-	return err;
 }
 
 static int rotary_encoder_remove(struct platform_device *pdev)
 {
-	struct rotary_encoder *encoder = platform_get_drvdata(pdev);
-	const struct rotary_encoder_platform_data *pdata = encoder->pdata;
-
 	device_init_wakeup(&pdev->dev, false);
-
-	free_irq(encoder->irq_a, encoder);
-	free_irq(encoder->irq_b, encoder);
-	gpio_free(pdata->gpio_a);
-	gpio_free(pdata->gpio_b);
-
-	input_unregister_device(encoder->input);
-	kfree(encoder);
-
-	if (!dev_get_platdata(&pdev->dev))
-		kfree(pdata);
-
 	return 0;
 }
 

Quoted 4 lines · Uwe Kleine-K?nig

For all operations called in .probe there are devm_* variants. (input_register_device is clever enough to be devm aware on its own.) This allows to get rid of the error unwind code paths in .probe and the complete .remove function.

While at it, could you also convert this over to the gpiod_* API?

Quoted 7 lines · Daniel Mack

Quoted 4 lines · Uwe Kleine-K?nig

For all operations called in .probe there are devm_* variants. (input_register_device is clever enough to be devm aware on its own.) This allows to get rid of the error unwind code paths in .probe and the complete .remove function.

While at it, could you also convert this over to the gpiod_* API?

This is done in patch 3.

This is a preparation for the next patche. There is no change in behaviour intended.

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

---
 drivers/input/misc/rotary_encoder.c | 166 ++++++++++++++++++++----------------
 1 file changed, 94 insertions(+), 72 deletions(-)

drivers/input/misc/rotary_encoder.c +94 -72

index 386bdb5314e6..0582e851993f 100644
@@ -32,58 +32,65 @@
 
 struct rotary_encoder {
 	struct input_dev *input;
-	const struct rotary_encoder_platform_data *pdata;
 
+	/* configuration */
+	unsigned int steps;
 	unsigned int axis;
-	unsigned int pos;
+	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;
 
+	/* state */
+	unsigned int pos;
 	bool armed;
 	unsigned char dir;	/* 0 - clockwise, 1 - CCW */
-
 	char last_stable;
 };
 
-static int rotary_encoder_get_state(const struct rotary_encoder_platform_data *pdata)
+static int rotary_encoder_get_state(const struct rotary_encoder *encoder)
 {
-	int a = !!gpio_get_value(pdata->gpio_a);
-	int b = !!gpio_get_value(pdata->gpio_b);
+	int a = !!gpio_get_value(encoder->gpio_a);
+	int b = !!gpio_get_value(encoder->gpio_b);
 
-	a ^= pdata->inverted_a;
-	b ^= pdata->inverted_b;
+	a ^= encoder->inverted_a;
+	b ^= encoder->inverted_b;
 
 	return ((a << 1) | b);
 }
 
 static void rotary_encoder_report_event(struct rotary_encoder *encoder)
 {
-	const struct rotary_encoder_platform_data *pdata = encoder->pdata;
-
-	if (pdata->relative_axis) {
+	if (encoder->relative_axis) {
 		input_report_rel(encoder->input,
-				 pdata->axis, encoder->dir ? -1 : 1);
+				 encoder->axis, encoder->dir ? -1 : 1);
 	} else {
 		unsigned int pos = encoder->pos;
 
 		if (encoder->dir) {
 			/* turning counter-clockwise */
-			if (pdata->rollover)
-				pos += pdata->steps;
+			if (encoder->rollover)
+				pos += encoder->steps;
 			if (pos)
 				pos--;
 		} else {
 			/* turning clockwise */
-			if (pdata->rollover || pos < pdata->steps)
+			if (encoder->rollover || pos < encoder->steps)
 				pos++;
 		}
 
-		if (pdata->rollover)
-			pos %= pdata->steps;
+		if (encoder->rollover)
+			pos %= encoder->steps;
 
 		encoder->pos = pos;
-		input_report_abs(encoder->input, pdata->axis, encoder->pos);
+		input_report_abs(encoder->input, encoder->axis, encoder->pos);
 	}
 
 	input_sync(encoder->input);

@@ -94,7 +101,7 @@ static irqreturn_t rotary_encoder_irq(int irq, void *dev_id)
 	struct rotary_encoder *encoder = dev_id;
 	int state;
 
-	state = rotary_encoder_get_state(encoder->pdata);
+	state = rotary_encoder_get_state(encoder);
 
 	switch (state) {
 	case 0x0:

@@ -123,7 +130,7 @@ static irqreturn_t rotary_encoder_half_period_irq(int irq, void *dev_id)
 	struct rotary_encoder *encoder = dev_id;
 	int state;
 
-	state = rotary_encoder_get_state(encoder->pdata);
+	state = rotary_encoder_get_state(encoder);
 
 	switch (state) {
 	case 0x00:

@@ -149,7 +156,7 @@ static irqreturn_t rotary_encoder_quarter_period_irq(int irq, void *dev_id)
 	unsigned char sum;
 	int state;
 
-	state = rotary_encoder_get_state(encoder->pdata);
+	state = rotary_encoder_get_state(encoder);
 
 	/*
 	 * We encode the previous and the current state using a byte.

@@ -199,38 +206,34 @@ static const struct of_device_id rotary_encoder_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, rotary_encoder_of_match);
 
-static struct rotary_encoder_platform_data *rotary_encoder_parse_dt(struct device *dev)
+static int rotary_encoder_parse_dt(struct device *dev,
+				   struct rotary_encoder *encoder)
 {
 	const struct of_device_id *of_id =
 				of_match_device(rotary_encoder_of_match, dev);
 	struct device_node *np = dev->of_node;
-	struct rotary_encoder_platform_data *pdata;
 	enum of_gpio_flags flags;
 	int error;
 
 	if (!of_id || !np)
-		return NULL;
-
-	pdata = devm_kzalloc(dev, sizeof(struct rotary_encoder_platform_data),
-			     GFP_KERNEL);
-	if (!pdata)
-		return ERR_PTR(-ENOMEM);
+		return 1;
 
-	of_property_read_u32(np, "rotary-encoder,steps", &pdata->steps);
-	of_property_read_u32(np, "linux,axis", &pdata->axis);
+	of_property_read_u32(np, "rotary-encoder,steps", &encoder->steps);
+	of_property_read_u32(np, "linux,axis", &encoder->axis);
 
-	pdata->gpio_a = of_get_gpio_flags(np, 0, &flags);
-	pdata->inverted_a = flags & OF_GPIO_ACTIVE_LOW;
+	encoder->gpio_a = of_get_gpio_flags(np, 0, &flags);
+	encoder->inverted_a = flags & OF_GPIO_ACTIVE_LOW;
 
-	pdata->gpio_b = of_get_gpio_flags(np, 1, &flags);
-	pdata->inverted_b = flags & OF_GPIO_ACTIVE_LOW;
+	encoder->gpio_b = of_get_gpio_flags(np, 1, &flags);
+	encoder->inverted_b = flags & OF_GPIO_ACTIVE_LOW;
 
-	pdata->relative_axis =
+	encoder->relative_axis =
 		of_property_read_bool(np, "rotary-encoder,relative-axis");
-	pdata->rollover = of_property_read_bool(np, "rotary-encoder,rollover");
+	encoder->rollover =
+		of_property_read_bool(np, "rotary-encoder,rollover");
 
 	error = of_property_read_u32(np, "rotary-encoder,steps-per-period",
-				     &pdata->steps_per_period);
+				     &encoder->steps_per_period);
 	if (error) {
 		/*
 		 * The 'half-period' property has been deprecated, you must use

@@ -238,45 +241,57 @@ static struct rotary_encoder_platform_data *rotary_encoder_parse_dt(struct devic
 		 * need to parse it to maintain compatibility.
 		 */
 		if (of_property_read_bool(np, "rotary-encoder,half-period")) {
-			pdata->steps_per_period = 2;
+			encoder->steps_per_period = 2;
 		} else {
 			/* Fallback to one step per period behavior */
-			pdata->steps_per_period = 1;
+			encoder->steps_per_period = 1;
 		}
 	}
 
-	pdata->wakeup_source = of_property_read_bool(np, "wakeup-source");
+	encoder->wakeup_source = of_property_read_bool(np, "wakeup-source");
 
-	return pdata;
+	return 0;
 }
 #else
-static inline struct rotary_encoder_platform_data *
-rotary_encoder_parse_dt(struct device *dev)
+static inline int rotary_encoder_parse_dt(struct device *dev,
+					  struct rotary_encoder *encoder)
 {
-	return NULL;
+	return 1;
 }
 #endif
 
+static int rotary_encoder_parse_pdata(struct device *dev,
+				      struct rotary_encoder *encoder)
+{
+	const struct rotary_encoder_platform_data *pdata;
+
+	pdata = dev_get_platdata(dev);
+	if (!pdata) {
+		dev_err(dev, "missing platform data\n");
+		return -EINVAL;
+	}
+
+	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;
+
+	return 0;
+}
+
 static int rotary_encoder_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	const struct rotary_encoder_platform_data *pdata = dev_get_platdata(dev);
 	struct rotary_encoder *encoder;
 	struct input_dev *input;
 	irq_handler_t handler;
 	int err;
 
-	if (!pdata) {
-		pdata = rotary_encoder_parse_dt(dev);
-		if (IS_ERR(pdata))
-			return PTR_ERR(pdata);
-
-		if (!pdata) {
-			dev_err(dev, "missing platform data\n");
-			return -EINVAL;
-		}
-	}
-
 	encoder = devm_kzalloc(dev, sizeof(struct rotary_encoder), GFP_KERNEL);
 	input = devm_input_allocate_device(&pdev->dev);
 	if (!encoder || !input) {

@@ -284,55 +299,62 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	err = rotary_encoder_parse_dt(dev, encoder);
+	if (err > 0)
+		/* not instatiated by dt */
+		err = rotary_encoder_parse_pdata(dev, encoder);
+
+	if (err < 0)
+		return err;
+
 	encoder->input = input;
-	encoder->pdata = pdata;
 
 	input->name = pdev->name;
 	input->id.bustype = BUS_HOST;
 	input->dev.parent = dev;
 
-	if (pdata->relative_axis) {
+	if (encoder->relative_axis) {
 		input->evbit[0] = BIT_MASK(EV_REL);
-		input->relbit[0] = BIT_MASK(pdata->axis);
+		input->relbit[0] = BIT_MASK(encoder->axis);
 	} else {
 		input->evbit[0] = BIT_MASK(EV_ABS);
 		input_set_abs_params(encoder->input,
-				     pdata->axis, 0, pdata->steps, 0, 1);
+				     encoder->axis, 0, encoder->steps, 0, 1);
 	}
 
 	/* request the GPIOs */
-	err = devm_gpio_request_one(dev, pdata->gpio_a,
+	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", pdata->gpio_a);
+		dev_err(dev, "unable to request GPIO %d\n", encoder->gpio_a);
 		return err;
 	}
 
-	err = devm_gpio_request_one(dev, pdata->gpio_b,
+	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", pdata->gpio_b);
+		dev_err(dev, "unable to request GPIO %d\n", encoder->gpio_b);
 		return err;
 	}
 
-	encoder->irq_a = gpio_to_irq(pdata->gpio_a);
-	encoder->irq_b = gpio_to_irq(pdata->gpio_b);
+	encoder->irq_a = gpio_to_irq(encoder->gpio_a);
+	encoder->irq_b = gpio_to_irq(encoder->gpio_b);
 
-	switch (pdata->steps_per_period) {
+	switch (encoder->steps_per_period) {
 	case 4:
 		handler = &rotary_encoder_quarter_period_irq;
-		encoder->last_stable = rotary_encoder_get_state(pdata);
+		encoder->last_stable = rotary_encoder_get_state(encoder);
 		break;
 	case 2:
 		handler = &rotary_encoder_half_period_irq;
-		encoder->last_stable = rotary_encoder_get_state(pdata);
+		encoder->last_stable = rotary_encoder_get_state(encoder);
 		break;
 	case 1:
 		handler = &rotary_encoder_irq;
 		break;
 	default:
 		dev_err(dev, "'%d' is not a valid steps-per-period value\n",
-			pdata->steps_per_period);
+			encoder->steps_per_period);
 		return -EINVAL;
 	}
 

@@ -358,7 +380,7 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	device_init_wakeup(&pdev->dev, pdata->wakeup_source);
+	device_init_wakeup(&pdev->dev, encoder->wakeup_source);
 
 	platform_set_drvdata(pdev, encoder);
 

Quoted 2 lines · Uwe Kleine-K?nig

This is a preparation for the next patche. There is no change in behaviour intended.

This one looks good to me, but it clashes with Dmitry's latest patch sets (which I haven't found the time to test yet, sorry!).

Quoted 1 line

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

Acked-by Daniel Mack <[email protected]>

Hello,

Quoted 1 line · Daniel Mack

On 02/02/2016 11:24 AM, Uwe Kleine-K?nig wrote:

Quoted 5 lines

Quoted 2 lines

This is a preparation for the next patche. There is no change in behaviour intended.

This one looks good to me, but it clashes with Dmitry's latest patch sets (which I haven't found the time to test yet, sorry!).

There is nothing in next-20160203, so I assume Dmitry either fixes this up when applying my patches or tells me when I should rebase.

Quoted 10 lines · Uwe Kleine-K?nig

Hello,

Quoted 6 lines · Daniel Mack

Quoted 2 lines · Uwe Kleine-K?nig

This is a preparation for the next patche. There is no change in behaviour intended.

This one looks good to me, but it clashes with Dmitry's latest patch sets (which I haven't found the time to test yet, sorry!).

Quoted 2 lines

There is nothing in next-20160203, so I assume Dmitry either fixes this up when applying my patches or tells me when I should rebase.

Please take a look at my rotary-encoder branch. I am waiting to merge it into next until I can get some testers. If you have raumfild device that would be awesome.

Some time ago I sent a v1 of this, now after testing the changes more deeply patch 3 changed a bit. The old series started with

	Date: Wed,  2 Dec 2015 11:07:11 +0100                                                                                               
	From: Uwe Kleine-K?nig <[email protected]>                                                                             
	Subject: [PATCH RFC 0/3] input: rotary_encoder: use more than two gpios as input                                                    
	Message-Id: <[email protected]>                                                      

The two first patches are just preparation for the third patch.

There is an obvious improvement that allows detection of quick changes more reliably with >2 gpios, but I didn't implement this yet. (With 4 GPIOs you can distinguish a counter clockwise movement of three states from a clock wise movement of a single state. Still the patch is useful as it makes these devices work at all.

My test device looks as follows:

        rotary at 0 {
                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>;
        };

While Daniel Mack and Rojhalat Ibrahim agreed that this device is an absolute encoder and should be supported by a simpler logic, I still consider it worthwhile to get these patches in as a first step. Also the binding looks right, so IMHO the comments shouldn't stop this series from going in.

Quoted 26 lines · Uwe Kleine-K?nig

Some time ago I sent a v1 of this, now after testing the changes more deeply patch 3 changed a bit. The old series started with

	Date: Wed,  2 Dec 2015 11:07:11 +0100
	From: Uwe Kleine-K?nig <[email protected]>
	Subject: [PATCH RFC 0/3] input: rotary_encoder: use more than two gpios as input
	Message-Id: <[email protected]>

The two first patches are just preparation for the third patch.

There is an obvious improvement that allows detection of quick changes more reliably with >2 gpios, but I didn't implement this yet. (With 4 GPIOs you can distinguish a counter clockwise movement of three states from a clock wise movement of a single state. Still the patch is useful as it makes these devices work at all.

My test device looks as follows:

        rotary at 0 {
                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 5 lines

While Daniel Mack and Rojhalat Ibrahim agreed that this device is an absolute encoder and should be supported by a simpler logic, I still consider it worthwhile to get these patches in as a first step. Also the binding looks right, so IMHO the comments shouldn't stop this series from going in.

I still don't understand why this is implemented that way, rather than going for a much simpler logic of interpretation that also allows users to read out the absolute position.

The code to read the value would be really just as simple as reading all GPIOs and or-ing their values into the result, and skip the state machine completely. This code would be active if a new attribute (something like 'rotary-encoder,hardware-absolute') is set, or even implicitly, when more than 2 GPIOs are specified.

Is there any reason for not doing that?

Hello Daniel,

Quoted 43 lines · Daniel Mack

Quoted 26 lines · Uwe Kleine-K?nig

Some time ago I sent a v1 of this, now after testing the changes more deeply patch 3 changed a bit. The old series started with

	Date: Wed,  2 Dec 2015 11:07:11 +0100
	From: Uwe Kleine-K?nig <[email protected]>
	Subject: [PATCH RFC 0/3] input: rotary_encoder: use more than two gpios as input
	Message-Id: <[email protected]>

The two first patches are just preparation for the third patch.

There is an obvious improvement that allows detection of quick changes more reliably with >2 gpios, but I didn't implement this yet. (With 4 GPIOs you can distinguish a counter clockwise movement of three states from a clock wise movement of a single state. Still the patch is useful as it makes these devices work at all.

My test device looks as follows:

        rotary at 0 {
                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 5 lines

While Daniel Mack and Rojhalat Ibrahim agreed that this device is an absolute encoder and should be supported by a simpler logic, I still consider it worthwhile to get these patches in as a first step. Also the binding looks right, so IMHO the comments shouldn't stop this series from going in.

I still don't understand why this is implemented that way, rather than going for a much simpler logic of interpretation that also allows users to read out the absolute position.

The code to read the value would be really just as simple as reading all GPIOs and or-ing their values into the result, and skip the state machine completely. This code would be active if a new attribute (something like 'rotary-encoder,hardware-absolute') is set, or even implicitly, when more than 2 GPIOs are specified.

Quoted 1 line

Is there any reason for not doing that?

Currently the reason is lack of time. And when implementing rotary-encoder,hardware-absolute something similar would be the result for the relative reporting anyhow. So the problem is only that I don't have absolute support yet, but the patches as is would be the base for that anyhow.

Quoted 46 lines · Uwe Kleine-K?nig

Hello Daniel,

Quoted 43 lines · Daniel Mack

Quoted 26 lines · Uwe Kleine-K?nig

Some time ago I sent a v1 of this, now after testing the changes more deeply patch 3 changed a bit. The old series started with

	Date: Wed,  2 Dec 2015 11:07:11 +0100
	From: Uwe Kleine-K?nig <[email protected]>
	Subject: [PATCH RFC 0/3] input: rotary_encoder: use more than two gpios as input
	Message-Id: <[email protected]>

The two first patches are just preparation for the third patch.

There is an obvious improvement that allows detection of quick changes more reliably with >2 gpios, but I didn't implement this yet. (With 4 GPIOs you can distinguish a counter clockwise movement of three states from a clock wise movement of a single state. Still the patch is useful as it makes these devices work at all.

My test device looks as follows:

        rotary at 0 {
                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 5 lines

While Daniel Mack and Rojhalat Ibrahim agreed that this device is an absolute encoder and should be supported by a simpler logic, I still consider it worthwhile to get these patches in as a first step. Also the binding looks right, so IMHO the comments shouldn't stop this series from going in.

I still don't understand why this is implemented that way, rather than going for a much simpler logic of interpretation that also allows users to read out the absolute position.

The code to read the value would be really just as simple as reading all GPIOs and or-ing their values into the result, and skip the state machine completely. This code would be active if a new attribute (something like 'rotary-encoder,hardware-absolute') is set, or even implicitly, when more than 2 GPIOs are specified.

Quoted 7 lines

Quoted 1 line

Is there any reason for not doing that?

Currently the reason is lack of time. And when implementing rotary-encoder,hardware-absolute something similar would be the result for the relative reporting anyhow. So the problem is only that I don't have absolute support yet, but the patches as is would be the base for that anyhow.

Because you would support relative support for such 4-pin encoders as well? I would have thought that absolute encoders would report absolute values only, but I guess you have a point here. Just to make sure we're on the same page: For more than 2 GPIOs, and an absent "rotary-encoder,relative-axis", the driver would switch to a mode in which it bypasses the state machine, right?