Hi,

On 05/19/2016 04:55 PM, Krzysztof Kozlowski wrote:
[...]

On 05/19/2016 11:04 AM, Vignesh R wrote:

quoted

diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index c7fc8d4fb080..8f60289fb6cd 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c

@@ -25,24 +25,29 @@
 #include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/property.h>
+#include <linux/input-polldev.h>
 
 #define DRV_NAME "rotary-encoder"
 
 struct rotary_encoder {
 	struct input_dev *input;
-
+#ifdef CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT

Ifdefs look dirty.

I will drop this.

quoted

+	struct input_polled_dev *poll_dev;
+#endif
 	struct mutex access_mutex;
 
 	u32 steps;
 	u32 axis;
 	bool relative_axis;
 	bool rollover;
+	bool absolute_encoder;
 
 	unsigned int pos;
 
 	struct gpio_descs *gpios;
+	struct device *dev;
 
-	unsigned int *irq;
+	int *irq;
 
 	bool armed;
 	signed char dir;	/* 1 - clockwise, -1 - CCW */

@@ -67,6 +72,21 @@ static unsigned int rotary_encoder_get_state(struct rotary_encoder *encoder)
 	return ret & 3;
 }
 
+static unsigned int rotary_encoder_get_gpios_state(struct rotary_encoder
+						   *encoder)
+{
+	int i;
+	unsigned int ret = 0;
+
+	for (i = 0; i < encoder->gpios->ndescs; ++i) {
+		int val = gpiod_get_value_cansleep(encoder->gpios->desc[i]);
+
+		ret = ret << 1 | val;
+	}
+
+	return ret;
+}
+
 static void rotary_encoder_report_event(struct rotary_encoder *encoder)
 {
 	if (encoder->relative_axis) {

@@ -178,6 +198,72 @@ out:
 	return IRQ_HANDLED;
 }
 
+static void rotary_encoder_setup_input_params(struct rotary_encoder  *encoder)
+{
+	struct input_dev *input = encoder->input;
+	struct platform_device *pdev = to_platform_device(encoder->dev);
+
+	input->name = pdev->name;
+	input->id.bustype = BUS_HOST;
+	input->dev.parent = encoder->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);
+}
+
+static irqreturn_t rotary_absolute_encoder_irq(int irq, void *dev_id)
+{
+	struct rotary_encoder *encoder = dev_id;
+	unsigned int state;
+
+	mutex_lock(&encoder->access_mutex);
+
+	state = rotary_encoder_get_gpios_state(encoder);
+	if (state != encoder->last_stable) {
+		input_report_abs(encoder->input, encoder->axis, state);
+		input_sync(encoder->input);
+		encoder->last_stable = state;
+	}
+
+	mutex_lock(&encoder->access_mutex);

1. Double mutex_lock()? Oh, that is weird. Please document it at least
in form of lockdep annotations.

2. Maybe that should be unlock? Did you test this code?

Sorry... this should have been unlock.
I dont have a h/w with rotary-encoder hooked to IRQ line. It would be
much appreciated if someone with rotary-encoder hooked up to h/w IRQ
line test this patch.

quoted

+
+	return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT

Same here and later in the code, ifdefs look dirty.

Ok, I will remove #ifdefs and directly select INPUT_POLLDEV when
INPUT_GPIO_ROTARY_ENCODER is selected.

-- 
Regards
Vignesh