linux-kernel - Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute encoder

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 23 May 2016 15:18:08 +0200
From:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>
To:	"R, Vignesh" <vigneshr@...com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Tony Lindgren <tony@...mide.com>,
	Jonathan Corbet <corbet@....net>,
	Johan Hovold <johan@...nel.org>,
	Sylvain Rochet <sylvain.rochet@...secur.com>,
	Masanari Iida <standby24x7@...il.com>,
	Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
	S Twiss <stwiss.opensource@...semi.com>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Moritz Fischer <moritz.fischer@...us.com>,
	Arnd Bergmann <arnd@...db.de>,
	Geert Uytterhoeven <geert@...ux-m68k.org>,
	Timo Teras <timo.teras@....fi>,
	Clifton Barnes <clifton.a.barnes@...il.com>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC PATCH 1/2] Input: rotary-encoder- Add support for absolute
 encoder

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/  |