Re: [PATCH v2 0/3] Input: rotary-encoder - use more than two gpios

Hello Daniel,

On Tue, Feb 02, 2016 at 02:14:49PM +0100, Daniel Mack wrote:

On 02/02/2016 01:56 PM, Uwe Kleine-König wrote:

quoted

Hello Daniel,

On Tue, Feb 02, 2016 at 01:08:07PM +0100, Daniel Mack wrote:

quoted

On 02/02/2016 11:24 AM, Uwe Kleine-König wrote:

quoted

Some time ago I sent a v1 of this, now after testing the changes more
deeply patch 3 changed a bit. The old series started with

	Date: Wed,  2 Dec 2015 11:07:11 +0100
	From: Uwe Kleine-König [off-list ref]
	Subject: [PATCH RFC 0/3] input: rotary_encoder: use more than two gpios as input
	Message-Id: [ref]

The two first patches are just preparation for the third patch.

There is an obvious improvement that allows detection of quick changes
more reliably with >2 gpios, but I didn't implement this yet. (With 4
GPIOs you can distinguish a counter clockwise movement of three states
from a clock wise movement of a single state. Still the patch is useful
as it makes these devices work at all.

My test device looks as follows:

        rotary@0 {
                compatible = "rotary-encoder";
                gpios = <&gpio4 12 GPIO_ACTIVE_HIGH>, <&gpio4 11 GPIO_ACTIVE_HIGH>, <&gpio4 10 GPIO_ACTIVE_HIGH>, <&gpio4 9 GPIO_ACTIVE_HIGH>;

                rotary-encoder,steps = <16>;
		rotary-encoder,steps-per-period = <16>;
        };

While Daniel Mack and Rojhalat Ibrahim agreed that this device is an
absolute encoder and should be supported by a simpler logic, I still
consider it worthwhile to get these patches in as a first step. Also the
binding looks right, so IMHO the comments shouldn't stop this series
from going in.

I still don't understand why this is implemented that way, rather than
going for a much simpler logic of interpretation that also allows users
to read out the absolute position.

The code to read the value would be really just as simple as reading all
GPIOs and or-ing their values into the result, and skip the state
machine completely. This code would be active if a new attribute
(something like 'rotary-encoder,hardware-absolute') is set, or even
implicitly, when more than 2 GPIOs are specified.

Is there any reason for not doing that?

Currently the reason is lack of time. And when implementing
rotary-encoder,hardware-absolute something similar would be the result
for the relative reporting anyhow. So the problem is only that I don't
have absolute support yet, but the patches as is would be the base for
that anyhow.

Because you would support relative support for such 4-pin encoders as
well? I would have thought that absolute encoders would report absolute
values only, but I guess you have a point here. Just to make sure we're
on the same page: For more than 2 GPIOs, and an absent
"rotary-encoder,relative-axis", the driver would switch to a mode in
which it bypasses the state machine, right?

Not sure about how this will be represented in the device tree, but yes,
that's more or less how I imagine to implement this. (And in the end
having support for a 4-line relative device is just a side effect of
having support for relative devices and for 4-line devices.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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