From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754581AbcESLZ2 (ORCPT ); Thu, 19 May 2016 07:25:28 -0400 Received: from mailout1.w1.samsung.com ([210.118.77.11]:44563 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754560AbcESLZV (ORCPT ); Thu, 19 May 2016 07:25:21 -0400 X-AuditID: cbfec7f5-f792a6d000001302-04-573da29d71f8 Subject: Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder To: Vignesh R , Dmitry Torokhov , Rob Herring , Tony Lindgren References: <1463648641-6931-1-git-send-email-vigneshr@ti.com> <1463648641-6931-2-git-send-email-vigneshr@ti.com> 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@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org From: Krzysztof Kozlowski X-Enigmail-Draft-Status: N1110 Message-id: <573DA29A.1080609@samsung.com> Date: Thu, 19 May 2016 13:25:14 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-version: 1.0 In-reply-to: <1463648641-6931-2-git-send-email-vigneshr@ti.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02Sa0hTcRjG+e+c7ZyZo+PS+megMVLCyjv0J1z1KU73okjti049eEmnbDrc vqRJy3RqTsK5jJzOMhmJlzAFM7ap5A1vUzMvkSIzEW3LC5qZc0R+e973+T3v8+UlMf4i7kkm itMZiViULOC44D1/uixnXlYKwwN1KgxtqzsJVNssRHOfngD0ytzPRqZKK0D1VjuO5ifaWCgr Z5JAj8ZVbLRoDUINs6NspFPqcfTFtsZGw63lHPRCX4Sj3qwI9LjNTKAhzRiBLFt1u8S7AhzV K++g9pEr6GtxCUA7fSvERUhvbaoBvbaqxukaWwBd1t1E0BuTpYBu0U4RtGlVh9MNtU85tNGU C+hG/UO6qrCETRdtB9KFTbWA7hpvZtF55jEObW/wukXddwmLY5ITZYwk4Hy0S4JFPYel5Z7N nP72m8gCxtN5gEtCKhTO/KrDnPowHJiu4+QBF5JPVQPYkqshnMM8gG2f8wkHdYi6BzuWZ/cM d0oN4PDS5m6c3KWkcEbp6dhj1Hc2XFCZ985yqBDY+EbPcVZ4wYJ8Ne7QPMoPTtjtLIfGKR9Y 97YCOLQHFQG1zRssJ+MGN0qm93gudWGXKcAdXRjlD2cG/RxrjPKGjYYl7Blw0+5LaP9T2n1U BcBqgQeTEZsmjYlPCfaXilKkGeJ4/9jUlAbgfIHVD6C685wRUCQQuPI0P8PC+WyRTCpPMQJI YgJ33kudMJzPixPJFYwkNUqSkcxIjeAYiQuO8Mpal+/yqXhROvOAYdIYyT+XRXI9s4Cv4iBo N0amZt9+ThpWqkIMwddz2j3YvkcXxxQ6Qjx2lTjRPfTa0neqJ/940U5kf+eB5eJe+ZZcMSWr GLB2r1eP6Gt8or0vj56sr04yhOltpd6+H7MnxJMLKoMs6r360rUYW4fmpl9ruTI0ad20M0KK WUGuGTcGf9Rwu22ZQgEuTRAF+WESqegv0FCf+v4CAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > .../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 > #include > #include > +#include > > #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