* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-19 9:04 ` [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder Vignesh R
@ 2016-05-19 11:25 ` Krzysztof Kozlowski
2016-05-19 11:44 ` Vignesh R
2016-05-20 16:34 ` Dmitry Torokhov
` (2 subsequent siblings)
3 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2016-05-19 11:25 UTC (permalink / raw)
To: Vignesh R, Dmitry Torokhov, Rob Herring, Tony Lindgren
Cc: Jonathan Corbet, Johan Hovold, Sylvain Rochet, Masanari Iida,
Ezequiel Garcia, S Twiss, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Guido Martinez, Clifton Barnes,
Uwe Kleine-Konig, linux-input, devicetree, linux-kernel,
linux-doc, linux-omap, linux-arm-kernel
On 05/19/2016 11:04 AM, Vignesh R wrote:
> There are rotary-encoders where GPIO lines reflect the actual position
> of the rotary encoder dial. For example, if dial points to 9, then four
> GPIO lines connected to the rotary encoder will read HLLH(1001b = 9).
> Add support for such rotary-encoder.
> The driver relies on rotary-encoder,absolute-encoder DT property to
> detect such encoders.
> Since, GPIO IRQs are not necessary to work with
> such encoders, optional polling mode support is added using
> input_poll_dev skeleton. This is can be used by enabling
> CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT.
>
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> .../devicetree/bindings/input/rotary-encoder.txt | 4 +
> Documentation/input/rotary-encoder.txt | 9 ++
> drivers/input/misc/Kconfig | 11 ++
> drivers/input/misc/rotary_encoder.c | 165 ++++++++++++++++-----
> 4 files changed, 155 insertions(+), 34 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> index 6c9f0c8a846c..9c928dbd1500 100644
> --- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
> +++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> @@ -12,6 +12,10 @@ Optional properties:
> - rotary-encoder,relative-axis: register a relative axis rather than an
> absolute one. Relative axis will only generate +1/-1 events on the input
> device, hence no steps need to be passed.
> +- rotary-encoder,absolute-encoder: support encoders where GPIO lines
> + reflect the actual position of the rotary encoder dial. For example,
> + if dial points to 9, then four GPIO lines read HLLH(1001b = 9).
> + In this case, rotary-encoder,steps-per-period needed not be defined.
> - 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,steps-per-period: Number of steps (stable states) per period.
> diff --git a/Documentation/input/rotary-encoder.txt b/Documentation/input/rotary-encoder.txt
> index 46a74f0c551a..dc76092f5618 100644
> --- a/Documentation/input/rotary-encoder.txt
> +++ b/Documentation/input/rotary-encoder.txt
> @@ -36,6 +36,15 @@ The phase diagram of these two outputs look like this:
> |<>|
> one step (quarter-period mode)
>
> +
> +In some encoders, the value pointed by rotary encoders switch is directly
> +reflected by status of the GPIOs connecting rotary encoder to CPU. For
> +example, if rotary encoder dial points to 9, then the four GPIOs
> +conncting rotary encoder will read HLLH(1001b = 9). Such encoders are
> +supported by defining devictree binding "rotary-encoder,absolute-encoder".
> +These encoders are also supported without need for event(IRQ) generation
> +using Input subsytem's polling mode support.
> +
> For more information, please see
> https://en.wikipedia.org/wiki/Rotary_encoder
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 1f2337abcf2f..81805f9a534c 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -566,6 +566,17 @@ config INPUT_GPIO_ROTARY_ENCODER
> To compile this driver as a module, choose M here: the
> module will be called rotary_encoder.
>
> +config INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT
> + bool "Polling mode support for rotary encoder"
> + depends on INPUT_GPIO_ROTARY_ENCODER
> + select INPUT_POLLDEV
> + help
> + Say Y here to enable polling mode support for rotary encoders
> + connected to GPIO lines that do have interrupt capability and
> + report rotary encoder position as absolute value over GPIO
> + lines. Check file: Documentation/input/rotary-encoder.txt for
> + more information.
> +
> config INPUT_RB532_BUTTON
> tristate "Mikrotik Routerboard 532 button interface"
> depends on MIKROTIK_RB532
> 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.
> + 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?
> +
> + return IRQ_HANDLED;
> +}
> +
> +#ifdef CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT
Same here and later in the code, ifdefs look dirty.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-19 11:25 ` Krzysztof Kozlowski
@ 2016-05-19 11:44 ` Vignesh R
0 siblings, 0 replies; 17+ messages in thread
From: Vignesh R @ 2016-05-19 11:44 UTC (permalink / raw)
To: Krzysztof Kozlowski, Dmitry Torokhov, Rob Herring, Tony Lindgren
Cc: Jonathan Corbet, Johan Hovold, Sylvain Rochet, Masanari Iida,
Ezequiel Garcia, S Twiss, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Guido Martinez, Clifton Barnes,
Uwe Kleine-Konig, linux-input, devicetree, linux-kernel,
linux-doc, linux-omap, linux-arm-kernel
Hi,
On 05/19/2016 04:55 PM, Krzysztof Kozlowski wrote:
[...]
> On 05/19/2016 11:04 AM, Vignesh R wrote:
>> 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.
>
>> + 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.
>> +
>> + 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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-19 9:04 ` [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder Vignesh R
2016-05-19 11:25 ` Krzysztof Kozlowski
@ 2016-05-20 16:34 ` Dmitry Torokhov
2016-05-23 9:18 ` R, Vignesh
2016-05-20 21:49 ` Rob Herring
2016-05-22 10:26 ` Uwe Kleine-König
3 siblings, 1 reply; 17+ messages in thread
From: Dmitry Torokhov @ 2016-05-20 16:34 UTC (permalink / raw)
To: Vignesh R
Cc: Rob Herring, Tony Lindgren, Jonathan Corbet, Johan Hovold,
Sylvain Rochet, Masanari Iida, Ezequiel Garcia, S Twiss,
Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Guido Martinez, Clifton Barnes,
Uwe Kleine-Konig, linux-input, devicetree, linux-kernel,
linux-doc, linux-omap, linux-arm-kernel
On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
> There are rotary-encoders where GPIO lines reflect the actual position
> of the rotary encoder dial. For example, if dial points to 9, then four
> GPIO lines connected to the rotary encoder will read HLLH(1001b = 9).
> Add support for such rotary-encoder.
> The driver relies on rotary-encoder,absolute-encoder DT property to
> detect such encoders.
> Since, GPIO IRQs are not necessary to work with
> such encoders, optional polling mode support is added using
> input_poll_dev skeleton. This is can be used by enabling
> CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT.
Does this really belong to a rotary encoder and not a new driver that
simply translates gpio-encoded value into ABS* event?
>
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> .../devicetree/bindings/input/rotary-encoder.txt | 4 +
> Documentation/input/rotary-encoder.txt | 9 ++
> drivers/input/misc/Kconfig | 11 ++
> drivers/input/misc/rotary_encoder.c | 165 ++++++++++++++++-----
> 4 files changed, 155 insertions(+), 34 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> index 6c9f0c8a846c..9c928dbd1500 100644
> --- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
> +++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> @@ -12,6 +12,10 @@ Optional properties:
> - rotary-encoder,relative-axis: register a relative axis rather than an
> absolute one. Relative axis will only generate +1/-1 events on the input
> device, hence no steps need to be passed.
> +- rotary-encoder,absolute-encoder: support encoders where GPIO lines
> + reflect the actual position of the rotary encoder dial. For example,
> + if dial points to 9, then four GPIO lines read HLLH(1001b = 9).
> + In this case, rotary-encoder,steps-per-period needed not be defined.
> - 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,steps-per-period: Number of steps (stable states) per period.
> diff --git a/Documentation/input/rotary-encoder.txt b/Documentation/input/rotary-encoder.txt
> index 46a74f0c551a..dc76092f5618 100644
> --- a/Documentation/input/rotary-encoder.txt
> +++ b/Documentation/input/rotary-encoder.txt
> @@ -36,6 +36,15 @@ The phase diagram of these two outputs look like this:
> |<>|
> one step (quarter-period mode)
>
> +
> +In some encoders, the value pointed by rotary encoders switch is directly
> +reflected by status of the GPIOs connecting rotary encoder to CPU. For
> +example, if rotary encoder dial points to 9, then the four GPIOs
> +conncting rotary encoder will read HLLH(1001b = 9). Such encoders are
> +supported by defining devictree binding "rotary-encoder,absolute-encoder".
> +These encoders are also supported without need for event(IRQ) generation
> +using Input subsytem's polling mode support.
> +
> For more information, please see
> https://en.wikipedia.org/wiki/Rotary_encoder
>
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 1f2337abcf2f..81805f9a534c 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -566,6 +566,17 @@ config INPUT_GPIO_ROTARY_ENCODER
> To compile this driver as a module, choose M here: the
> module will be called rotary_encoder.
>
> +config INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT
> + bool "Polling mode support for rotary encoder"
> + depends on INPUT_GPIO_ROTARY_ENCODER
> + select INPUT_POLLDEV
> + help
> + Say Y here to enable polling mode support for rotary encoders
> + connected to GPIO lines that do have interrupt capability and
> + report rotary encoder position as absolute value over GPIO
> + lines. Check file: Documentation/input/rotary-encoder.txt for
> + more information.
> +
> config INPUT_RB532_BUTTON
> tristate "Mikrotik Routerboard 532 button interface"
> depends on MIKROTIK_RB532
> 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
> + 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);
> +
> + return IRQ_HANDLED;
> +}
> +
> +#ifdef CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT
> +static void rotary_encoder_poll_gpios(struct input_polled_dev *poll_dev)
> +{
> + struct rotary_encoder *encoder = poll_dev->private;
> + unsigned int 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;
> + }
> +}
> +
> +static int rotary_encoder_register_poll_device(struct rotary_encoder
> + *encoder)
> +{
> + struct input_polled_dev *poll_dev =
> + devm_input_allocate_polled_device(encoder->dev);
> +
> + if (!poll_dev)
> + return -ENOMEM;
> + poll_dev->private = encoder;
> + poll_dev->poll = rotary_encoder_poll_gpios;
> + encoder->input = poll_dev->input;
> + rotary_encoder_setup_input_params(encoder);
> + encoder->poll_dev = poll_dev;
> +
> + return input_register_polled_device(poll_dev);
> +}
> +#endif
> +
> static int rotary_encoder_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -227,38 +313,32 @@ static int rotary_encoder_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - input = devm_input_allocate_device(dev);
> - if (!input)
> - return -ENOMEM;
> -
> - encoder->input = input;
> -
> - input->name = pdev->name;
> - input->id.bustype = BUS_HOST;
> - input->dev.parent = 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);
> -
> - switch (steps_per_period >> (encoder->gpios->ndescs - 2)) {
> - case 4:
> - handler = &rotary_encoder_quarter_period_irq;
> - encoder->last_stable = rotary_encoder_get_state(encoder);
> - break;
> - case 2:
> - handler = &rotary_encoder_half_period_irq;
> - 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",
> - steps_per_period);
> - return -EINVAL;
> + encoder->dev = dev;
> + encoder->absolute_encoder =
> + device_property_read_bool(dev,
> + "rotary-encoder,absolute-encoder");
> + if (encoder->absolute_encoder) {
> + handler = rotary_absolute_encoder_irq;
> + } else {
> + switch (steps_per_period >> (encoder->gpios->ndescs - 2)) {
> + case 4:
> + handler = &rotary_encoder_quarter_period_irq;
> + encoder->last_stable =
> + rotary_encoder_get_state(encoder);
> + break;
> + case 2:
> + handler = &rotary_encoder_half_period_irq;
> + 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",
> + steps_per_period);
> + return -EINVAL;
> + }
> }
>
> encoder->irq =
> @@ -271,6 +351,18 @@ static int rotary_encoder_probe(struct platform_device *pdev)
> for (i = 0; i < encoder->gpios->ndescs; ++i) {
> encoder->irq[i] = gpiod_to_irq(encoder->gpios->desc[i]);
>
> +#ifdef CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT
> + if (encoder->irq[i] < 0 && encoder->absolute_encoder) {
> + dev_info(dev, "Using poll mode\n");
> + err = rotary_encoder_register_poll_device(encoder);
> + if (err) {
> + dev_err(dev, "failed to register poll dev\n");
> + return err;
> + }
> + platform_set_drvdata(pdev, encoder);
> + return 0;
> + }
> +#endif
> err = devm_request_threaded_irq(dev, encoder->irq[i],
> NULL, handler,
> IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
> @@ -283,6 +375,11 @@ static int rotary_encoder_probe(struct platform_device *pdev)
> }
> }
>
> + input = devm_input_allocate_device(dev);
> + if (!input)
> + return -ENOMEM;
> + encoder->input = input;
> + rotary_encoder_setup_input_params(encoder);
> err = input_register_device(input);
> if (err) {
> dev_err(dev, "failed to register input device\n");
> --
> 2.8.2
>
--
Dmitry
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-20 16:34 ` Dmitry Torokhov
@ 2016-05-23 9:18 ` R, Vignesh
2016-05-25 8:44 ` Vignesh R
0 siblings, 1 reply; 17+ messages in thread
From: R, Vignesh @ 2016-05-23 9:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Tony Lindgren, Jonathan Corbet, Johan Hovold,
Sylvain Rochet, Masanari Iida, Ezequiel Garcia, S Twiss,
Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Guido Martinez, Clifton Barnes,
Uwe Kleine-Konig, linux-input, devicetree, linux-kernel,
linux-doc, linux-omap, linux-arm-kernel
On 5/20/2016 10:04 PM, Dmitry Torokhov wrote:
> On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
>> There are rotary-encoders where GPIO lines reflect the actual position
>> of the rotary encoder dial. For example, if dial points to 9, then four
>> GPIO lines connected to the rotary encoder will read HLLH(1001b = 9).
>> Add support for such rotary-encoder.
>> The driver relies on rotary-encoder,absolute-encoder DT property to
>> detect such encoders.
>> Since, GPIO IRQs are not necessary to work with
>> such encoders, optional polling mode support is added using
>> input_poll_dev skeleton. This is can be used by enabling
>> CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT.
>
> Does this really belong to a rotary encoder and not a new driver that
> simply translates gpio-encoded value into ABS* event?
>
Currently rotary encoder driver only supports incremental/step counting
rotary devices. However, the device that is there on am335x-ice is an
absolute encoder but, IMO, nevertheless a kind of rotary encoder. The
only difference is that there is no need to count steps and the absolute
position value is always available as binary encoded state of connected
GPIOs.
The hardware on am335x-ice is a mechanical rotary encoder switch
connected over 4 GPIOs. It is same as binary encoder described at [1]
(except there are 4 GPIO lines), so this lead me to add support in
rotary-encoder.
[1]https://en.wikipedia.org/wiki/Rotary_encoder#Standard_binary_encoding
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-23 9:18 ` R, Vignesh
@ 2016-05-25 8:44 ` Vignesh R
2016-06-16 10:47 ` Vignesh R
0 siblings, 1 reply; 17+ messages in thread
From: Vignesh R @ 2016-05-25 8:44 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Tony Lindgren, Jonathan Corbet, Johan Hovold,
Sylvain Rochet, Masanari Iida, Ezequiel Garcia, S Twiss,
Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Guido Martinez, Clifton Barnes,
Uwe Kleine-Konig, linux-input, devicetree, linux-kernel,
linux-doc, linux-omap, linux-arm-kernel
Hi Dmitry,
On 05/23/2016 02:48 PM, R, Vignesh wrote:
>
>
> On 5/20/2016 10:04 PM, Dmitry Torokhov wrote:
>> On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
>>> There are rotary-encoders where GPIO lines reflect the actual position
>>> of the rotary encoder dial. For example, if dial points to 9, then four
>>> GPIO lines connected to the rotary encoder will read HLLH(1001b = 9).
>>> Add support for such rotary-encoder.
>>> The driver relies on rotary-encoder,absolute-encoder DT property to
>>> detect such encoders.
>>> Since, GPIO IRQs are not necessary to work with
>>> such encoders, optional polling mode support is added using
>>> input_poll_dev skeleton. This is can be used by enabling
>>> CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT.
>>
>> Does this really belong to a rotary encoder and not a new driver that
>> simply translates gpio-encoded value into ABS* event?
>>
>
> Currently rotary encoder driver only supports incremental/step counting
> rotary devices. However, the device that is there on am335x-ice is an
> absolute encoder but, IMO, nevertheless a kind of rotary encoder. The
> only difference is that there is no need to count steps and the absolute
> position value is always available as binary encoded state of connected
> GPIOs.
> The hardware on am335x-ice is a mechanical rotary encoder switch
> connected over 4 GPIOs. It is same as binary encoder described at [1]
> (except there are 4 GPIO lines), so this lead me to add support in
> rotary-encoder.
>
> [1]https://en.wikipedia.org/wiki/Rotary_encoder#Standard_binary_encoding
>
Could you please comment on how would you like to support above
described encoder: As a new driver or with existing driver with new
compatible/mode setting via DT or as suggest by Uwe in another reply?
IMHO, supporting using existing driver with new mode/compatible string
looks a better option as the hardware is a kind of rotary-encoder.
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-25 8:44 ` Vignesh R
@ 2016-06-16 10:47 ` Vignesh R
2016-07-19 13:04 ` R, Vignesh
0 siblings, 1 reply; 17+ messages in thread
From: Vignesh R @ 2016-06-16 10:47 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Tony Lindgren, Jonathan Corbet, Johan Hovold,
Sylvain Rochet, Masanari Iida, Ezequiel Garcia, S Twiss,
Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Guido Martinez, Clifton Barnes,
Uwe Kleine-Konig, linux-input, devicetree, linux-kernel,
linux-doc, linux-omap, linux-arm-kernel
Hi Dmitry,
On Wednesday 25 May 2016 02:14 PM, Vignesh R wrote:
> Hi Dmitry,
>
> On 05/23/2016 02:48 PM, R, Vignesh wrote:
>>
>>
>> On 5/20/2016 10:04 PM, Dmitry Torokhov wrote:
>>> On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
>>>> There are rotary-encoders where GPIO lines reflect the actual position
>>>> of the rotary encoder dial. For example, if dial points to 9, then four
>>>> GPIO lines connected to the rotary encoder will read HLLH(1001b = 9).
>>>> Add support for such rotary-encoder.
>>>> The driver relies on rotary-encoder,absolute-encoder DT property to
>>>> detect such encoders.
>>>> Since, GPIO IRQs are not necessary to work with
>>>> such encoders, optional polling mode support is added using
>>>> input_poll_dev skeleton. This is can be used by enabling
>>>> CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT.
>>>
>>> Does this really belong to a rotary encoder and not a new driver that
>>> simply translates gpio-encoded value into ABS* event?
>>>
>>
>> Currently rotary encoder driver only supports incremental/step counting
>> rotary devices. However, the device that is there on am335x-ice is an
>> absolute encoder but, IMO, nevertheless a kind of rotary encoder. The
>> only difference is that there is no need to count steps and the absolute
>> position value is always available as binary encoded state of connected
>> GPIOs.
>> The hardware on am335x-ice is a mechanical rotary encoder switch
>> connected over 4 GPIOs. It is same as binary encoder described at [1]
>> (except there are 4 GPIO lines), so this lead me to add support in
>> rotary-encoder.
>>
>> [1]https://en.wikipedia.org/wiki/Rotary_encoder#Standard_binary_encoding
>>
>
> Could you please comment on how would you like to support above
> described encoder: As a new driver or with existing driver with new
> compatible/mode setting via DT or as suggest by Uwe in another reply?
> IMHO, supporting using existing driver with new mode/compatible string
> looks a better option as the hardware is a kind of rotary-encoder.
>
It would be great if you could comment on how would you like to see
support for absolute rotary-encoder(the one I described above)? As a new
driver or handle it by adding new compatible to existing rotary-encoder
driver?
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-06-16 10:47 ` Vignesh R
@ 2016-07-19 13:04 ` R, Vignesh
0 siblings, 0 replies; 17+ messages in thread
From: R, Vignesh @ 2016-07-19 13:04 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring, Tony Lindgren, Jonathan Corbet, Johan Hovold,
Sylvain Rochet, Masanari Iida, Ezequiel Garcia, S Twiss,
Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Guido Martinez, Clifton Barnes,
Uwe Kleine-Konig, linux-input, devicetree, linux-kernel,
linux-doc, linux-omap, linux-arm-kernel
Dmitry,
On 6/16/2016 4:17 PM, Vignesh R wrote:
[...]
>>> On 5/20/2016 10:04 PM, Dmitry Torokhov wrote:
>>>> On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
>>>>> There are rotary-encoders where GPIO lines reflect the actual position
>>>>> of the rotary encoder dial. For example, if dial points to 9, then four
>>>>> GPIO lines connected to the rotary encoder will read HLLH(1001b = 9).
>>>>> Add support for such rotary-encoder.
>>>>> The driver relies on rotary-encoder,absolute-encoder DT property to
>>>>> detect such encoders.
>>>>> Since, GPIO IRQs are not necessary to work with
>>>>> such encoders, optional polling mode support is added using
>>>>> input_poll_dev skeleton. This is can be used by enabling
>>>>> CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT.
>>>>
>>>> Does this really belong to a rotary encoder and not a new driver that
>>>> simply translates gpio-encoded value into ABS* event?
>>>>
>>>
>>> Currently rotary encoder driver only supports incremental/step counting
>>> rotary devices. However, the device that is there on am335x-ice is an
>>> absolute encoder but, IMO, nevertheless a kind of rotary encoder. The
>>> only difference is that there is no need to count steps and the absolute
>>> position value is always available as binary encoded state of connected
>>> GPIOs.
>>> The hardware on am335x-ice is a mechanical rotary encoder switch
>>> connected over 4 GPIOs. It is same as binary encoder described at [1]
>>> (except there are 4 GPIO lines), so this lead me to add support in
>>> rotary-encoder.
>>>
>>> [1]https://en.wikipedia.org/wiki/Rotary_encoder#Standard_binary_encoding
>>>
>>
>> Could you please comment on how would you like to support above
>> described encoder: As a new driver or with existing driver with new
>> compatible/mode setting via DT or as suggest by Uwe in another reply?
>> IMHO, supporting using existing driver with new mode/compatible string
>> looks a better option as the hardware is a kind of rotary-encoder.
>>
Could you please suggest the right solution here? Do you think new
driver is need to support above described absolute encoder h/w? It would
be great to have your input now, so that I can work on the new set of
patches and submit them for v4.9 merge window as soon as v4.8 merge
window closes?
Regards
Vignesh
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-19 9:04 ` [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder Vignesh R
2016-05-19 11:25 ` Krzysztof Kozlowski
2016-05-20 16:34 ` Dmitry Torokhov
@ 2016-05-20 21:49 ` Rob Herring
2016-05-22 10:26 ` Uwe Kleine-König
3 siblings, 0 replies; 17+ messages in thread
From: Rob Herring @ 2016-05-20 21:49 UTC (permalink / raw)
To: Vignesh R
Cc: Dmitry Torokhov, Tony Lindgren, Jonathan Corbet, Johan Hovold,
Sylvain Rochet, Masanari Iida, Ezequiel Garcia, S Twiss,
Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Guido Martinez, Clifton Barnes,
Uwe Kleine-Konig, linux-input, devicetree, linux-kernel,
linux-doc, linux-omap, linux-arm-kernel
On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
> There are rotary-encoders where GPIO lines reflect the actual position
> of the rotary encoder dial. For example, if dial points to 9, then four
> GPIO lines connected to the rotary encoder will read HLLH(1001b = 9).
> Add support for such rotary-encoder.
> The driver relies on rotary-encoder,absolute-encoder DT property to
> detect such encoders.
> Since, GPIO IRQs are not necessary to work with
> such encoders, optional polling mode support is added using
> input_poll_dev skeleton. This is can be used by enabling
> CONFIG_INPUT_GPIO_ROTARY_ENCODER_POLL_MODE_SUPPORT.
>
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
> .../devicetree/bindings/input/rotary-encoder.txt | 4 +
> Documentation/input/rotary-encoder.txt | 9 ++
> drivers/input/misc/Kconfig | 11 ++
> drivers/input/misc/rotary_encoder.c | 165 ++++++++++++++++-----
> 4 files changed, 155 insertions(+), 34 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> index 6c9f0c8a846c..9c928dbd1500 100644
> --- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
> +++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> @@ -12,6 +12,10 @@ Optional properties:
> - rotary-encoder,relative-axis: register a relative axis rather than an
> absolute one. Relative axis will only generate +1/-1 events on the input
> device, hence no steps need to be passed.
> +- rotary-encoder,absolute-encoder: support encoders where GPIO lines
> + reflect the actual position of the rotary encoder dial. For example,
> + if dial points to 9, then four GPIO lines read HLLH(1001b = 9).
> + In this case, rotary-encoder,steps-per-period needed not be defined.
I think this should be done as a new compatible string rather than a
property. It seems like fundamentally different h/w.
Rob
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-19 9:04 ` [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder Vignesh R
` (2 preceding siblings ...)
2016-05-20 21:49 ` Rob Herring
@ 2016-05-22 10:26 ` Uwe Kleine-König
2016-05-23 11:18 ` R, Vignesh
3 siblings, 1 reply; 17+ messages in thread
From: Uwe Kleine-König @ 2016-05-22 10:26 UTC (permalink / raw)
To: Vignesh R
Cc: Dmitry Torokhov, Rob Herring, Tony Lindgren, Jonathan Corbet,
Johan Hovold, Sylvain Rochet, Masanari Iida, Ezequiel Garcia,
S Twiss, Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Guido Martinez, Clifton Barnes,
linux-input, devicetree, linux-kernel, linux-doc, linux-omap,
linux-arm-kernel
Hello,
On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
> There are rotary-encoders where GPIO lines reflect the actual position
> of the rotary encoder dial. For example, if dial points to 9, then four
> GPIO lines connected to the rotary encoder will read HLLH(1001b = 9).
> Add support for such rotary-encoder.
> The driver relies on rotary-encoder,absolute-encoder DT property to
> detect such encoders.
> Since, GPIO IRQs are not necessary to work with
> such encoders, optional polling mode support is added using
I don't understand this. It's necessary in the same way as with the
already supported devices. I.e. you want to trigger an irq when the
encoder is moved and then check for it's position in the handler.
> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> index 6c9f0c8a846c..9c928dbd1500 100644
> --- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
> +++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> @@ -12,6 +12,10 @@ Optional properties:
> - rotary-encoder,relative-axis: register a relative axis rather than an
> absolute one. Relative axis will only generate +1/-1 events on the input
> device, hence no steps need to be passed.
> +- rotary-encoder,absolute-encoder: support encoders where GPIO lines
> + reflect the actual position of the rotary encoder dial. For example,
> + if dial points to 9, then four GPIO lines read HLLH(1001b = 9).
> + In this case, rotary-encoder,steps-per-period needed not be defined.
IMHO this is wrong, I'd formalize this device as:
{
compatible = "rotary-encoder";
gpios = <&gpio 19 1>, <&gpio 20 0>, <...>, <...>;
rotary-encoder,encoding = "binary";
rotary-encoder,steps = <16>;
rotary-encoder,steps-per-period = <16>;
rotary-encoder,rollover;
}
and support this with a v4 of
http://thread.gmane.org/gmane.linux.kernel.input/48892
.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-22 10:26 ` Uwe Kleine-König
@ 2016-05-23 11:18 ` R, Vignesh
2016-05-23 13:18 ` Uwe Kleine-König
0 siblings, 1 reply; 17+ messages in thread
From: R, Vignesh @ 2016-05-23 11:18 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Dmitry Torokhov, Rob Herring, Tony Lindgren, Jonathan Corbet,
Johan Hovold, Sylvain Rochet, Masanari Iida, Ezequiel Garcia,
S Twiss, Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Clifton Barnes, linux-input,
devicetree, linux-kernel, linux-doc, linux-omap,
linux-arm-kernel
On 5/22/2016 3:56 PM, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
>> There are rotary-encoders where GPIO lines reflect the actual position
>> of the rotary encoder dial. For example, if dial points to 9, then four
>> GPIO lines connected to the rotary encoder will read HLLH(1001b = 9).
>> Add support for such rotary-encoder.
>> The driver relies on rotary-encoder,absolute-encoder DT property to
>> detect such encoders.
>> Since, GPIO IRQs are not necessary to work with
>> such encoders, optional polling mode support is added using
>
> I don't understand this. It's necessary in the same way as with the
> already supported devices. I.e. you want to trigger an irq when the
> encoder is moved and then check for it's position in the handler.
>
Unlike already supported device, there is no need to count steps or
determine the direction of rotation. Hence, IRQ is not a requirement,
periodically polling the gpio lines is more than sufficient. With
absolute encoder, you are able to determine the current position at any
time just by looking at the gpio inputs.
Suppose we poll device at t=0ms and see gpio values are LLLH(1), if we
again poll device at t=500ms(which is what input_poll_dev helps to do)
and see that gpio values is LLHH(3), then we know that rotary encoder
has changed to 3. This can be done w/o IRQ for absolute encoders.
>> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
>> index 6c9f0c8a846c..9c928dbd1500 100644
>> --- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
>> +++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
>> @@ -12,6 +12,10 @@ Optional properties:
>> - rotary-encoder,relative-axis: register a relative axis rather than an
>> absolute one. Relative axis will only generate +1/-1 events on the input
>> device, hence no steps need to be passed.
>> +- rotary-encoder,absolute-encoder: support encoders where GPIO lines
>> + reflect the actual position of the rotary encoder dial. For example,
>> + if dial points to 9, then four GPIO lines read HLLH(1001b = 9).
>> + In this case, rotary-encoder,steps-per-period needed not be defined.
>
> IMHO this is wrong, I'd formalize this device as:
>
> {
> compatible = "rotary-encoder";
> gpios = <&gpio 19 1>, <&gpio 20 0>, <...>, <...>;
> rotary-encoder,encoding = "binary";
> rotary-encoder,steps = <16>;
> rotary-encoder,steps-per-period = <16>;
The above bindings essential means quarter_period device. I would not
like to bother with all the logic in rotary_encoder_quarter_period_irq()
when we can know encoder->pos by directly reading state of gpio lines.
> rotary-encoder,rollover;
> }
>
> and support this with a v4 of
>
> http://thread.gmane.org/gmane.linux.kernel.input/48892
>
>
IMHO, there needs to be separate IRQ handler for absolute encoders so
that the driver logic is greatly simplified.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-23 11:18 ` R, Vignesh
@ 2016-05-23 13:18 ` Uwe Kleine-König
2016-05-24 5:09 ` Vignesh R
0 siblings, 1 reply; 17+ messages in thread
From: Uwe Kleine-König @ 2016-05-23 13:18 UTC (permalink / raw)
To: R, Vignesh
Cc: Dmitry Torokhov, Rob Herring, Tony Lindgren, Jonathan Corbet,
Johan Hovold, Sylvain Rochet, Masanari Iida, Ezequiel Garcia,
S Twiss, Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Clifton Barnes, linux-input,
devicetree, linux-kernel, linux-doc, linux-omap,
linux-arm-kernel
Hello,
On Mon, May 23, 2016 at 04:48:40PM +0530, R, Vignesh wrote:
> On 5/22/2016 3:56 PM, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
> >> There are rotary-encoders where GPIO lines reflect the actual position
> >> of the rotary encoder dial. For example, if dial points to 9, then four
> >> GPIO lines connected to the rotary encoder will read HLLH(1001b = 9).
> >> Add support for such rotary-encoder.
> >> The driver relies on rotary-encoder,absolute-encoder DT property to
> >> detect such encoders.
> >> Since, GPIO IRQs are not necessary to work with
> >> such encoders, optional polling mode support is added using
> >
> > I don't understand this. It's necessary in the same way as with the
> > already supported devices. I.e. you want to trigger an irq when the
> > encoder is moved and then check for it's position in the handler.
> >
>
> Unlike already supported device, there is no need to count steps or
> determine the direction of rotation. Hence, IRQ is not a requirement,
> periodically polling the gpio lines is more than sufficient. With
> absolute encoder, you are able to determine the current position at any
> time just by looking at the gpio inputs.
The timing might not be that critical, but there is no reason to operate
this device without irqs, is there?
> Suppose we poll device at t=0ms and see gpio values are LLLH(1), if we
> again poll device at t=500ms(which is what input_poll_dev helps to do)
> and see that gpio values is LLHH(3), then we know that rotary encoder
> has changed to 3. This can be done w/o IRQ for absolute encoders.
>
> >> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> >> index 6c9f0c8a846c..9c928dbd1500 100644
> >> --- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
> >> +++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
> >> @@ -12,6 +12,10 @@ Optional properties:
> >> - rotary-encoder,relative-axis: register a relative axis rather than an
> >> absolute one. Relative axis will only generate +1/-1 events on the input
> >> device, hence no steps need to be passed.
> >> +- rotary-encoder,absolute-encoder: support encoders where GPIO lines
> >> + reflect the actual position of the rotary encoder dial. For example,
> >> + if dial points to 9, then four GPIO lines read HLLH(1001b = 9).
> >> + In this case, rotary-encoder,steps-per-period needed not be defined.
> >
> > IMHO this is wrong, I'd formalize this device as:
> >
> > {
> > compatible = "rotary-encoder";
> > gpios = <&gpio 19 1>, <&gpio 20 0>, <...>, <...>;
> > rotary-encoder,encoding = "binary";
> > rotary-encoder,steps = <16>;
> > rotary-encoder,steps-per-period = <16>;
>
> The above bindings essential means quarter_period device. I would not
> like to bother with all the logic in rotary_encoder_quarter_period_irq()
> when we can know encoder->pos by directly reading state of gpio lines.
OK, we have code that is more complex than it needs to be for your
device. But your device is a special case of the supported devices, so
I'd say don't bother that there is more logic in the driver than you
need and be lucky.
> > rotary-encoder,rollover;
> > }
> >
> > and support this with a v4 of
> >
> > http://thread.gmane.org/gmane.linux.kernel.input/48892
> >
> >
>
> IMHO, there needs to be separate IRQ handler for absolute encoders so
> that the driver logic is greatly simplified.
As above, the added complexity can handle your case just fine. So just
use it.
One thing I see to improve is to make use of the additional GPIOs, but
this is orthogonal to the different encoding and the question if polling
is enough here.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-23 13:18 ` Uwe Kleine-König
@ 2016-05-24 5:09 ` Vignesh R
2016-05-24 8:20 ` Uwe Kleine-König
0 siblings, 1 reply; 17+ messages in thread
From: Vignesh R @ 2016-05-24 5:09 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Dmitry Torokhov, Rob Herring, Tony Lindgren, Jonathan Corbet,
Johan Hovold, Sylvain Rochet, Masanari Iida, Ezequiel Garcia,
S Twiss, Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Clifton Barnes, linux-input,
devicetree, linux-kernel, linux-doc, linux-omap,
linux-arm-kernel
Hi,
On 05/23/2016 06:48 PM, Uwe Kleine-König wrote:
> Hello,
>
> On Mon, May 23, 2016 at 04:48:40PM +0530, R, Vignesh wrote:
>> On 5/22/2016 3:56 PM, Uwe Kleine-König wrote:
>>> Hello,
>>>
>>> On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
>>>> There are rotary-encoders where GPIO lines reflect the actual position
>>>> of the rotary encoder dial. For example, if dial points to 9, then four
>>>> GPIO lines connected to the rotary encoder will read HLLH(1001b = 9).
>>>> Add support for such rotary-encoder.
>>>> The driver relies on rotary-encoder,absolute-encoder DT property to
>>>> detect such encoders.
>>>> Since, GPIO IRQs are not necessary to work with
>>>> such encoders, optional polling mode support is added using
>>>
>>> I don't understand this. It's necessary in the same way as with the
>>> already supported devices. I.e. you want to trigger an irq when the
>>> encoder is moved and then check for it's position in the handler.
>>>
>>
>> Unlike already supported device, there is no need to count steps or
>> determine the direction of rotation. Hence, IRQ is not a requirement,
>> periodically polling the gpio lines is more than sufficient. With
>> absolute encoder, you are able to determine the current position at any
>> time just by looking at the gpio inputs.
>
> The timing might not be that critical, but there is no reason to operate
> this device without irqs, is there?
No, I am not against the use of IRQs. In fact, my patches add support
for both IRQ based(default) and polling based operation(optional). If
rotary-encoder does not have interrupt capable gpio lines connected to
it (like am335x-ice) then one can make use of the polling mode support.
>
>> Suppose we poll device at t=0ms and see gpio values are LLLH(1), if we
>> again poll device at t=500ms(which is what input_poll_dev helps to do)
>> and see that gpio values is LLHH(3), then we know that rotary encoder
>> has changed to 3. This can be done w/o IRQ for absolute encoders.
>>
>>>> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
>>>> index 6c9f0c8a846c..9c928dbd1500 100644
>>>> --- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
>>>> +++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
>>>> @@ -12,6 +12,10 @@ Optional properties:
>>>> - rotary-encoder,relative-axis: register a relative axis rather than an
>>>> absolute one. Relative axis will only generate +1/-1 events on the input
>>>> device, hence no steps need to be passed.
>>>> +- rotary-encoder,absolute-encoder: support encoders where GPIO lines
>>>> + reflect the actual position of the rotary encoder dial. For example,
>>>> + if dial points to 9, then four GPIO lines read HLLH(1001b = 9).
>>>> + In this case, rotary-encoder,steps-per-period needed not be defined.
>>>
>>> IMHO this is wrong, I'd formalize this device as:
>>>
>>> {
>>> compatible = "rotary-encoder";
>>> gpios = <&gpio 19 1>, <&gpio 20 0>, <...>, <...>;
>>> rotary-encoder,encoding = "binary";
>>> rotary-encoder,steps = <16>;
>>> rotary-encoder,steps-per-period = <16>;
>>
>> The above bindings essential means quarter_period device. I would not
>> like to bother with all the logic in rotary_encoder_quarter_period_irq()
>> when we can know encoder->pos by directly reading state of gpio lines.
>
> OK, we have code that is more complex than it needs to be for your
> device. But your device is a special case of the supported devices, so
> I'd say don't bother that there is more logic in the driver than you
> need and be lucky.
>
More complexity is just a overhead. Since, encoder can be turned at a
rate faster than handling of IRQs (rotary_encoder_quarter_period_irq()
is threaded IRQ hence, priority is not close to real time), some states
can be missed. rotary_encoder_quarter_period_irq() is not robust in this
case, reading gpios directly is more suitable option. I see similar
views expressed in previously[1]
[1]http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/391196.html
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-24 5:09 ` Vignesh R
@ 2016-05-24 8:20 ` Uwe Kleine-König
2016-05-25 8:44 ` Vignesh R
0 siblings, 1 reply; 17+ messages in thread
From: Uwe Kleine-König @ 2016-05-24 8:20 UTC (permalink / raw)
To: Vignesh R
Cc: Dmitry Torokhov, Rob Herring, Tony Lindgren, Jonathan Corbet,
Johan Hovold, Sylvain Rochet, Masanari Iida, Ezequiel Garcia,
S Twiss, Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Clifton Barnes, linux-input,
devicetree, linux-kernel, linux-doc, linux-omap,
linux-arm-kernel
Hello,
On Tue, May 24, 2016 at 10:39:26AM +0530, Vignesh R wrote:
> On 05/23/2016 06:48 PM, Uwe Kleine-König wrote:
> > On Mon, May 23, 2016 at 04:48:40PM +0530, R, Vignesh wrote:
> >> On 5/22/2016 3:56 PM, Uwe Kleine-König wrote:
> >>> On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
> >>>> +- rotary-encoder,absolute-encoder: support encoders where GPIO lines
> >>>> + reflect the actual position of the rotary encoder dial. For example,
> >>>> + if dial points to 9, then four GPIO lines read HLLH(1001b = 9).
> >>>> + In this case, rotary-encoder,steps-per-period needed not be defined.
> >>>
> >>> IMHO this is wrong, I'd formalize this device as:
> >>>
> >>> {
> >>> compatible = "rotary-encoder";
> >>> gpios = <&gpio 19 1>, <&gpio 20 0>, <...>, <...>;
> >>> rotary-encoder,encoding = "binary";
> >>> rotary-encoder,steps = <16>;
> >>> rotary-encoder,steps-per-period = <16>;
> >>
> >> The above bindings essential means quarter_period device. I would not
> >> like to bother with all the logic in rotary_encoder_quarter_period_irq()
> >> when we can know encoder->pos by directly reading state of gpio lines.
> >
> > OK, we have code that is more complex than it needs to be for your
> > device. But your device is a special case of the supported devices, so
> > I'd say don't bother that there is more logic in the driver than you
> > need and be lucky.
>
> More complexity is just a overhead. Since, encoder can be turned at a
> rate faster than handling of IRQs (rotary_encoder_quarter_period_irq()
> is threaded IRQ hence, priority is not close to real time), some states
This problem isn't unique to your hardware. An "ordinary" encoder with
just two GPIOs and more than one period can be rotated faster than
1/irq_latency, too. There are two things that can be done:
- undo the conversion to threaded irqs; or
- read out the gpios in the fast handler and only delay decoding and
reporting of the event
Both approaches have their disadvantages.
> can be missed. rotary_encoder_quarter_period_irq() is not robust in this
> case, reading gpios directly is more suitable option. I see similar
> views expressed in previously[1]
>
> [1]http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/391196.html
IMHO the right thing to do is to improve
rotary_encoder_quarter_period_irq (and also the other handlers for full
and half period mode) to make use of additional GPIOs. This way all
types of devices benefit and more code is shared.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder
2016-05-24 8:20 ` Uwe Kleine-König
@ 2016-05-25 8:44 ` Vignesh R
0 siblings, 0 replies; 17+ messages in thread
From: Vignesh R @ 2016-05-25 8:44 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Dmitry Torokhov, Rob Herring, Tony Lindgren, Jonathan Corbet,
Johan Hovold, Sylvain Rochet, Masanari Iida, Ezequiel Garcia,
S Twiss, Krzysztof Kozlowski, Moritz Fischer, Arnd Bergmann,
Geert Uytterhoeven, Timo Teras, Clifton Barnes, linux-input,
devicetree, linux-kernel, linux-doc, linux-omap,
linux-arm-kernel
On 05/24/2016 01:50 PM, Uwe Kleine-König wrote:
> Hello,
>
> On Tue, May 24, 2016 at 10:39:26AM +0530, Vignesh R wrote:
>> On 05/23/2016 06:48 PM, Uwe Kleine-König wrote:
>>> On Mon, May 23, 2016 at 04:48:40PM +0530, R, Vignesh wrote:
>>>> On 5/22/2016 3:56 PM, Uwe Kleine-König wrote:
>>>>> On Thu, May 19, 2016 at 02:34:00PM +0530, Vignesh R wrote:
[...]
>>>
>>> OK, we have code that is more complex than it needs to be for your
>>> device. But your device is a special case of the supported devices, so
>>> I'd say don't bother that there is more logic in the driver than you
>>> need and be lucky.
>>
>> More complexity is just a overhead. Since, encoder can be turned at a
>> rate faster than handling of IRQs (rotary_encoder_quarter_period_irq()
>> is threaded IRQ hence, priority is not close to real time), some states
>
> This problem isn't unique to your hardware. An "ordinary" encoder with
> just two GPIOs and more than one period can be rotated faster than
> 1/irq_latency, too.
But my hardware should not be affected by this problem. The whole point
of absolute encoder is to overcome the difficulty of software keeping
track of steps, one can read the gpios state anytime and find out what
value is the encoder pointing to.
> There are two things that can be done:
>
> - undo the conversion to threaded irqs; or
> - read out the gpios in the fast handler and only delay decoding and
> reporting of the event
>
> Both approaches have their disadvantages.
>
And both cannot guarantee that an event is not missed (on a loaded
system) and all of the state logic goes for a toss. For binary encoding
multiple IRQs(thats why incremental encoders are usually gray coded)
will fire at same time and handling all of the them is a considerable
overhead, there is good chance that some events are missed.
>> can be missed. rotary_encoder_quarter_period_irq() is not robust in this
>> case, reading gpios directly is more suitable option. I see similar
>> views expressed in previously[1]
>>
>> [1]http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/391196.html
>
> IMHO the right thing to do is to improve
> rotary_encoder_quarter_period_irq (and also the other handlers for full
> and half period mode) to make use of additional GPIOs.
I doubt there exists any incremental encoder with more than 2
gpios(except for the extra reference GPIO or Z signal). So modifying
full/half period handlers maybe unnecessary.
> This way all types of devices benefit and more code is shared.
Sorry.. but IMHO, there is little code sharing and more complexity. So,
I will leave it to the maintainer to decide whats the best approach here.
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 17+ messages in thread