[PATCH v2 0/2] rotary-encoder: Add new interruption handler

[PATCH v2 0/2] rotary-encoder: Add new interruption handler

[Ezequiel Garcia]

Some rotary-encoder devices (such as those with detents) are capable
of producing a stable event on each step. This simple patch adds support
for this case, by implementing a new interruption handler.

The handler needs only detect the direction of the turn and generate
an event according to this detection.

We encode the previous and the current state, and then use the sum of them
to decide on the direction of the turn, according to the following simple
table:

Previous state + currrent state	| Movement
==========================================
	0b1101			| clockwise
	0b0100			|   ..
	0b0010			|   ..
	0b1011			|   ..
==========================================
	0b1110			| counter-clockwise
	0b0111			|   ..
	0b0001			|   ..
	0b1000			|   ..

(The other sumed values are considered illegal)

This calculation is based on some previous work found at this blog:

  http://bildr.org/2012/08/rotary-encoder-arduino/

The result is a seemingly very robust behavior, with a truly simple
implementation, that produces an event on each turn of the device.

Thanks!

Ezequiel Garcia (2):
  Input: rotary-encoder: Add 'stepped' irq handler
  input: rotary-encoder: Add 'on-each-step' to binding documentation

 .../devicetree/bindings/input/rotary-encoder.txt   |  1 +
 drivers/input/misc/rotary_encoder.c                | 41 ++++++++++++++++++++++
 include/linux/rotary_encoder.h                     |  1 +
 3 files changed, 43 insertions(+)

-- 
1.8.1.5

[PATCH v2 1/2] Input: rotary-encoder: Add 'stepped' irq handler

[Ezequiel Garcia]

Some rotary-encoder devices (such as those with detents) are capable
of producing a stable event on each step. This commit adds support
for this case, by implementing a new interruption handler.

The handler needs only detect the direction of the turn and generate
an event according to this detection.

Cc: Daniel Mack <zonque@gmail.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
v1->v2:
  * Add DT property handling
  * Replace binary representation by hexa-decimal in the sum encoding

 drivers/input/misc/rotary_encoder.c | 41 +++++++++++++++++++++++++++++++++++++
 include/linux/rotary_encoder.h      |  1 +
 2 files changed, 42 insertions(+)

diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index 5b1aff8..6c7a554 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -117,6 +117,42 @@ static irqreturn_t rotary_encoder_irq(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t rotary_encoder_stepped_irq(int irq, void *dev_id)
+{
+	struct rotary_encoder *encoder = dev_id;
+	int state;
+	unsigned char sum;
+
+	state = rotary_encoder_get_state(encoder->pdata);
+
+	/*
+	 * We encode the previous and the current state using a byte.
+	 * The previous state in the MSB nibble, the current state in the LSB
+	 * nibble. Then use a table to decide the direction of the turn.
+	 */
+	sum = (encoder->last_stable << 4) + state;
+	switch (sum) {
+	case 0x31:
+	case 0x10:
+	case 0x02:
+	case 0x23:
+		encoder->dir = 0; /* clockwise */
+		break;
+	/*
+	 * TODO: Add the other case, and the illegal values should
+	 * say a WARN (it's a BUG, but we won't stop the kernel for it :)
+	 */
+	default:
+		encoder->dir = 1;
+	}
+
+	rotary_encoder_report_event(encoder);
+
+	encoder->last_stable = state;
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t rotary_encoder_half_period_irq(int irq, void *dev_id)
 {
 	struct rotary_encoder *encoder = dev_id;
@@ -180,6 +216,8 @@ static struct rotary_encoder_platform_data *rotary_encoder_parse_dt(struct devic
 					"rotary-encoder,rollover", NULL);
 	pdata->half_period = !!of_get_property(np,
 					"rotary-encoder,half-period", NULL);
+	pdata->on_each_step = !!of_get_property(np,
+					"rotary-encoder,on-each-step", NULL);
 
 	return pdata;
 }
@@ -254,6 +292,9 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 	if (pdata->half_period) {
 		handler = &rotary_encoder_half_period_irq;
 		encoder->last_stable = rotary_encoder_get_state(pdata);
+	} else if (pdata->on_each_step) {
+		handler = &rotary_encoder_stepped_irq;
+		encoder->last_stable = rotary_encoder_get_state(pdata);
 	} else {
 		handler = &rotary_encoder_irq;
 	}
diff --git a/include/linux/rotary_encoder.h b/include/linux/rotary_encoder.h
index 3f594dc..499f6f7 100644
--- a/include/linux/rotary_encoder.h
+++ b/include/linux/rotary_encoder.h
@@ -11,6 +11,7 @@ struct rotary_encoder_platform_data {
 	bool relative_axis;
 	bool rollover;
 	bool half_period;
+	bool on_each_step;
 };
 
 #endif /* __ROTARY_ENCODER_H__ */
-- 
1.8.1.5

Re: [PATCH v2 1/2] Input: rotary-encoder: Add 'stepped' irq handler

[Daniel Mack]

On 04.10.2013 14:53, Ezequiel Garcia wrote:
> Some rotary-encoder devices (such as those with detents) are capable
> of producing a stable event on each step. This commit adds support
> for this case, by implementing a new interruption handler.
> 
> The handler needs only detect the direction of the turn and generate
> an event according to this detection.

I think you can squash patch 2/2 into this one. It doesn't make much
sense to have a one-liner patch to just update the documenation separately.

Other than that, the code looks fine to me.

  Acked-by: Daniel Mack <zonque@gmail.com>


Thanks,
Daniel


> 
> Cc: Daniel Mack <zonque@gmail.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
> v1->v2:
>   * Add DT property handling
>   * Replace binary representation by hexa-decimal in the sum encoding
> 
>  drivers/input/misc/rotary_encoder.c | 41 +++++++++++++++++++++++++++++++++++++
>  include/linux/rotary_encoder.h      |  1 +
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
> index 5b1aff8..6c7a554 100644
> --- a/drivers/input/misc/rotary_encoder.c
> +++ b/drivers/input/misc/rotary_encoder.c
> @@ -117,6 +117,42 @@ static irqreturn_t rotary_encoder_irq(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static irqreturn_t rotary_encoder_stepped_irq(int irq, void *dev_id)
> +{
> +	struct rotary_encoder *encoder = dev_id;
> +	int state;
> +	unsigned char sum;
> +
> +	state = rotary_encoder_get_state(encoder->pdata);
> +
> +	/*
> +	 * We encode the previous and the current state using a byte.
> +	 * The previous state in the MSB nibble, the current state in the LSB
> +	 * nibble. Then use a table to decide the direction of the turn.
> +	 */
> +	sum = (encoder->last_stable << 4) + state;
> +	switch (sum) {
> +	case 0x31:
> +	case 0x10:
> +	case 0x02:
> +	case 0x23:
> +		encoder->dir = 0; /* clockwise */
> +		break;
> +	/*
> +	 * TODO: Add the other case, and the illegal values should
> +	 * say a WARN (it's a BUG, but we won't stop the kernel for it :)
> +	 */
> +	default:
> +		encoder->dir = 1;
> +	}
> +
> +	rotary_encoder_report_event(encoder);
> +
> +	encoder->last_stable = state;
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t rotary_encoder_half_period_irq(int irq, void *dev_id)
>  {
>  	struct rotary_encoder *encoder = dev_id;
> @@ -180,6 +216,8 @@ static struct rotary_encoder_platform_data *rotary_encoder_parse_dt(struct devic
>  					"rotary-encoder,rollover", NULL);
>  	pdata->half_period = !!of_get_property(np,
>  					"rotary-encoder,half-period", NULL);
> +	pdata->on_each_step = !!of_get_property(np,
> +					"rotary-encoder,on-each-step", NULL);
>  
>  	return pdata;
>  }
> @@ -254,6 +292,9 @@ static int rotary_encoder_probe(struct platform_device *pdev)
>  	if (pdata->half_period) {
>  		handler = &rotary_encoder_half_period_irq;
>  		encoder->last_stable = rotary_encoder_get_state(pdata);
> +	} else if (pdata->on_each_step) {
> +		handler = &rotary_encoder_stepped_irq;
> +		encoder->last_stable = rotary_encoder_get_state(pdata);
>  	} else {
>  		handler = &rotary_encoder_irq;
>  	}
> diff --git a/include/linux/rotary_encoder.h b/include/linux/rotary_encoder.h
> index 3f594dc..499f6f7 100644
> --- a/include/linux/rotary_encoder.h
> +++ b/include/linux/rotary_encoder.h
> @@ -11,6 +11,7 @@ struct rotary_encoder_platform_data {
>  	bool relative_axis;
>  	bool rollover;
>  	bool half_period;
> +	bool on_each_step;
>  };
>  
>  #endif /* __ROTARY_ENCODER_H__ */
> 

Re: [PATCH v2 1/2] Input: rotary-encoder: Add 'stepped' irq handler

[Ezequiel García]

Dmitry,

On 4 October 2013 10:18, Daniel Mack <zonque@gmail.com> wrote:
> On 04.10.2013 14:53, Ezequiel Garcia wrote:
>> Some rotary-encoder devices (such as those with detents) are capable
>> of producing a stable event on each step. This commit adds support
>> for this case, by implementing a new interruption handler.
>>
>> The handler needs only detect the direction of the turn and generate
>> an event according to this detection.
>
> I think you can squash patch 2/2 into this one. It doesn't make much
> sense to have a one-liner patch to just update the documenation separately.
>
> Other than that, the code looks fine to me.
>
>   Acked-by: Daniel Mack <zonque@gmail.com>
>

Could you merge these two patches?

It's already acked by Daniel, and there wasn't any other feedback
so I think we're ready to get them in and support more devices :-)
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/2] input: rotary-encoder: Add 'on-each-step' to binding documentation

[Ezequiel Garcia]

The driver now supports a new mode to handle the interruptions generated
by the device: on this new mode an input event is generated on each step
(i.e. on each IRQ). Therefore, add a new DT property, to select the
mode: 'rotary-encoder,on-each-step'.

Cc: Daniel Mack <zonque-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Dmitry Torokhov <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Ezequiel Garcia <ezequiel-30ULvvUtt6G51wMPkGsGjgyUoB5FGQPZ@public.gmane.org>
---
I'm not at all happy with this DT binding as it's way to customized
for the current driver. For instance, if we want to support mapping
key events (or better arbitrary linux-input event types) it seems
there's no easy way to fix the binding.

Maybe a better way of handling the different 'modes' is through
compatible strings?

I'm not really sure, so I hope the DT guys have some comment on this.

 Documentation/devicetree/bindings/input/rotary-encoder.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
index 3315495..b89e38d 100644
--- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
+++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
@@ -15,6 +15,7 @@ Optional properties:
 - rotary-encoder,rollover: Automatic rollove when the rotary value becomes
   greater than the specified steps or smaller than 0. For absolute axis only.
 - rotary-encoder,half-period: Makes the driver work on half-period mode.
+- rotary-encoder,on-each-step: Makes the driver send an event on each step.
 
 See Documentation/input/rotary-encoder.txt for more information.
 
-- 
1.8.1.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] input: rotary-encoder: Add 'on-each-step' to binding documentation

[Mark Rutland]

On Fri, Oct 04, 2013 at 01:53:23PM +0100, Ezequiel Garcia wrote:
> The driver now supports a new mode to handle the interruptions generated
> by the device: on this new mode an input event is generated on each step
> (i.e. on each IRQ). Therefore, add a new DT property, to select the
> mode: 'rotary-encoder,on-each-step'.
> 
> Cc: Daniel Mack <zonque@gmail.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
> I'm not at all happy with this DT binding as it's way to customized
> for the current driver. For instance, if we want to support mapping
> key events (or better arbitrary linux-input event types) it seems
> there's no easy way to fix the binding.
> 
> Maybe a better way of handling the different 'modes' is through
> compatible strings?

I'd prefer not to have more pseudo-devices in DT, and would prefer not
to have compatible strings that boil down to driver options. We end up
just embedding a tonne of Linux-specific driver configuration in the DT
rather than describing hardware.

That said, I'm not sure what the best solution is here.

> 
> I'm not really sure, so I hope the DT guys have some comment on this.
> 
>  Documentation/devicetree/bindings/input/rotary-encoder.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> index 3315495..b89e38d 100644
> --- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
> +++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> @@ -15,6 +15,7 @@ Optional properties:
>  - rotary-encoder,rollover: Automatic rollove when the rotary value becomes
>    greater than the specified steps or smaller than 0. For absolute axis only.
>  - rotary-encoder,half-period: Makes the driver work on half-period mode.
> +- rotary-encoder,on-each-step: Makes the driver send an event on each step.

Could this not be something requested at runtime?

Could you explain what you want to achieve with this? -- what events do
you want to occur when, to be handled in what way?

Cheers,
Mark.

Re: [PATCH v2 2/2] input: rotary-encoder: Add 'on-each-step' to binding documentation

[Ezequiel Garcia]

On Fri, Oct 04, 2013 at 02:19:56PM +0100, Mark Rutland wrote:
> On Fri, Oct 04, 2013 at 01:53:23PM +0100, Ezequiel Garcia wrote:
> > The driver now supports a new mode to handle the interruptions generated
> > by the device: on this new mode an input event is generated on each step
> > (i.e. on each IRQ). Therefore, add a new DT property, to select the
> > mode: 'rotary-encoder,on-each-step'.
> > 
> > Cc: Daniel Mack <zonque@gmail.com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > ---
> > I'm not at all happy with this DT binding as it's way to customized
> > for the current driver. For instance, if we want to support mapping
> > key events (or better arbitrary linux-input event types) it seems
> > there's no easy way to fix the binding.
> > 
> > Maybe a better way of handling the different 'modes' is through
> > compatible strings?
> 
> I'd prefer not to have more pseudo-devices in DT, and would prefer not
> to have compatible strings that boil down to driver options. We end up
> just embedding a tonne of Linux-specific driver configuration in the DT
> rather than describing hardware.
> 
> That said, I'm not sure what the best solution is here.
> 
> > 
> > I'm not really sure, so I hope the DT guys have some comment on this.
> > 
> >  Documentation/devicetree/bindings/input/rotary-encoder.txt | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> > index 3315495..b89e38d 100644
> > --- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
> > +++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> > @@ -15,6 +15,7 @@ Optional properties:
> >  - rotary-encoder,rollover: Automatic rollove when the rotary value becomes
> >    greater than the specified steps or smaller than 0. For absolute axis only.
> >  - rotary-encoder,half-period: Makes the driver work on half-period mode.
> > +- rotary-encoder,on-each-step: Makes the driver send an event on each step.
> 
> Could this not be something requested at runtime?
> 

Sure. The different modes:

* default (no option)
* rotary-encoder,half-period
* rotary-encoder,on-each-step

Just map to different interruption handlers. I don't have any other
rotary-encoder device, so I'm not at all sure what's the use of the
other two cases.
My particular device is detented, and produces a 'stable' event on each
step (i.e on each IRQ).

Regarding the runtime specification: you mean as a module parameter?
That should be trivial to add, no?

> Could you explain what you want to achieve with this? -- what events do
> you want to occur when, to be handled in what way?
> 

Hm.. maybe I should have added the binding to the 1/2 patch and CCed
everybody involved for better context.

Anyway, I hope the above is clearer, I'm not really sure how to specify
the details in the DT binding, since it's a specific interruption handler
for this class of encoder devices (stable on each step).

That said, I really hope I'm crafting a generic solution and not some
tailor-made implementation that just happens to match my use case.

The input maintainer's opinion on this would be valuable.
-- 
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] input: rotary-encoder: Add 'on-each-step' to binding documentation

[Ezequiel García]

On 4 October 2013 11:09, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> On Fri, Oct 04, 2013 at 02:19:56PM +0100, Mark Rutland wrote:
>> On Fri, Oct 04, 2013 at 01:53:23PM +0100, Ezequiel Garcia wrote:
>> > The driver now supports a new mode to handle the interruptions generated
>> > by the device: on this new mode an input event is generated on each step
>> > (i.e. on each IRQ). Therefore, add a new DT property, to select the
>> > mode: 'rotary-encoder,on-each-step'.
>> >
>> > Cc: Daniel Mack <zonque@gmail.com>
>> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> > Cc: Rob Herring <rob.herring@calxeda.com>
>> > Cc: devicetree@vger.kernel.org
>> > Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> > ---
>> > I'm not at all happy with this DT binding as it's way to customized
>> > for the current driver. For instance, if we want to support mapping
>> > key events (or better arbitrary linux-input event types) it seems
>> > there's no easy way to fix the binding.
>> >
>> > Maybe a better way of handling the different 'modes' is through
>> > compatible strings?
>>
>> I'd prefer not to have more pseudo-devices in DT, and would prefer not
>> to have compatible strings that boil down to driver options. We end up
>> just embedding a tonne of Linux-specific driver configuration in the DT
>> rather than describing hardware.
>>
>> That said, I'm not sure what the best solution is here.
>>
>> >
>> > I'm not really sure, so I hope the DT guys have some comment on this.
>> >
>> >  Documentation/devicetree/bindings/input/rotary-encoder.txt | 1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
>> > index 3315495..b89e38d 100644
>> > --- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
>> > +++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
>> > @@ -15,6 +15,7 @@ Optional properties:
>> >  - rotary-encoder,rollover: Automatic rollove when the rotary value becomes
>> >    greater than the specified steps or smaller than 0. For absolute axis only.
>> >  - rotary-encoder,half-period: Makes the driver work on half-period mode.
>> > +- rotary-encoder,on-each-step: Makes the driver send an event on each step.
>>
>> Could this not be something requested at runtime?
>>
>
> Sure. The different modes:
>
> * default (no option)
> * rotary-encoder,half-period
> * rotary-encoder,on-each-step
>
> Just map to different interruption handlers. I don't have any other
> rotary-encoder device, so I'm not at all sure what's the use of the
> other two cases.
> My particular device is detented, and produces a 'stable' event on each
> step (i.e on each IRQ).
>
> Regarding the runtime specification: you mean as a module parameter?
> That should be trivial to add, no?
>
>> Could you explain what you want to achieve with this? -- what events do
>> you want to occur when, to be handled in what way?
>>
>
> Hm.. maybe I should have added the binding to the 1/2 patch and CCed
> everybody involved for better context.
>
> Anyway, I hope the above is clearer, I'm not really sure how to specify
> the details in the DT binding, since it's a specific interruption handler
> for this class of encoder devices (stable on each step).
>
> That said, I really hope I'm crafting a generic solution and not some
> tailor-made implementation that just happens to match my use case.
>
> The input maintainer's opinion on this would be valuable.

Any insights on this binding?

If nobody objects, then I'd like to get this change accepted,
together with the driver change.

Thanks!
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html