| Message ID | 1380392798-23345-2-git-send-email-ezequiel@vanguardiasur.com.ar (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ezequiel, On 28.09.2013 20:26, 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. > > +static irqreturn_t rotary_encoder_stepped_irq(int irq, void *dev_id) > +{ > + struct rotary_encoder *encoder = dev_id; > + int state, sum; > + > + state = rotary_encoder_get_state(encoder->pdata); > + > + /* > + * We encode the previous and the current state, > + * in the 'sum' variable and then use a table > + * to decide the direction of the turn. > + */ > + sum = (encoder->last_stable << 2) + state; > + switch (sum) { > + case 0b1101: > + case 0b0100: > + case 0b0010: > + case 0b1011: Binary constants are frowned upon, please avoid them in the kernel. checkpatch.pl should have warned you about them as well. > 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; Care to add a DT binding for this property as well? Other than that, this looks good to me, but I can't test it due to the lack of hardware. Thanks, Daniel -- 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
On 29 September 2013 07:40, Daniel Mack <zonque@gmail.com> wrote: > Hi Ezequiel, > > On 28.09.2013 20:26, 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. >> > >> +static irqreturn_t rotary_encoder_stepped_irq(int irq, void *dev_id) >> +{ >> + struct rotary_encoder *encoder = dev_id; >> + int state, sum; >> + >> + state = rotary_encoder_get_state(encoder->pdata); >> + >> + /* >> + * We encode the previous and the current state, >> + * in the 'sum' variable and then use a table >> + * to decide the direction of the turn. >> + */ >> + sum = (encoder->last_stable << 2) + state; >> + switch (sum) { >> + case 0b1101: >> + case 0b0100: >> + case 0b0010: >> + case 0b1011: > > Binary constants are frowned upon, please avoid them in the kernel. > checkpatch.pl should have warned you about them as well. > Well... despite any checkpatch.pl warnings, I think the above is much clear to the reader than any alternative. The numbers encode the previous and current gpio state, so the first two (LSB) bits have the current gpio's, while the other two have the previous. If binary values should be avoided by all means, then I would prefer to encode the previous and current in different nibbles: sum = (encoder->last_stable << 4) + state; switch (sum) { case 0x31: case 0x10: case 0x02: case 0x23: Maybe this is better? >> 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; > > Care to add a DT binding for this property as well? > Sure, I was just waiting for your ACK before I did that. > > Other than that, this looks good to me, but I can't test it due to the > lack of hardware. > OK. I'm really curious about the rotary devices you originally used with this driver. I guess those have no detents, so there's no mechanical-click on each step? Thanks,
On 29.09.2013 19:29, Ezequiel García wrote: > On 29 September 2013 07:40, Daniel Mack <zonque@gmail.com> wrote: >> On 28.09.2013 20:26, Ezequiel Garcia wrote: >>> + sum = (encoder->last_stable << 2) + state; >>> + switch (sum) { >>> + case 0b1101: >>> + case 0b0100: >>> + case 0b0010: >>> + case 0b1011: >> >> Binary constants are frowned upon, please avoid them in the kernel. >> checkpatch.pl should have warned you about them as well. >> > > Well... despite any checkpatch.pl warnings, I think the above is much clear > to the reader than any alternative. The problem is that support for that notation is a proprietary gcc extension that is AFAIK only supported from gcc 4.3 onwards or so. However, the current minimum gcc version for building the kernel is 3.2. > If binary values should be avoided by all means, then I would prefer to encode > the previous and current in different nibbles: > > sum = (encoder->last_stable << 4) + state; > switch (sum) { > case 0x31: > case 0x10: > case 0x02: > case 0x23: > > Maybe this is better? Either that, or use case BIT(3) | BIT(2) | BIT(0): ... > I'm really curious about the rotary devices you originally used with > this driver. > I guess those have no detents, so there's no mechanical-click on each step? Some models have detents, some don't. We've used one of this series which does: http://de.mouser.com/ProductDetail/Alpha-Taiwan/RE111F-20B3-20F-20P/?qs=yA6kp8fx8Y7KsyMOFz9p0A== Best regards, Daniel -- 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
diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c index 5b1aff8..8802221 100644 --- a/drivers/input/misc/rotary_encoder.c +++ b/drivers/input/misc/rotary_encoder.c @@ -117,6 +117,37 @@ 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, sum; + + state = rotary_encoder_get_state(encoder->pdata); + + /* + * We encode the previous and the current state, + * in the 'sum' variable and then use a table + * to decide the direction of the turn. + */ + sum = (encoder->last_stable << 2) + state; + switch (sum) { + case 0b1101: + case 0b0100: + case 0b0010: + case 0b1011: + encoder->dir = 0; /* clockwise */ + break; + 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; @@ -254,6 +285,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__ */
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> --- drivers/input/misc/rotary_encoder.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/rotary_encoder.h | 1 + 2 files changed, 35 insertions(+)