[PATCH] Input: rotary_encoder - support binary encoding of states

[PATCH] Input: rotary_encoder - support binary encoding of states

[Uwe Kleine-König]

A plain binary encoding has some downsides compared to the usual Gray
encoding, but that doesn't stop hardware engineers to eventually use it.
So implement support for this encoding in the rotary encoder driver.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

an alternative to define this difference in the device tree is to use
something like:

	rotary-encoder,encoding = "binary";

or

	rotary-encoder,encoding = <ROTARY_ENCODER_ENCODING_BINARY>;

instead of a property

	rotary-encoder,encoding-binary;

. While the two first solutions make it obvious that there can only be
one encoding, they IMHO look ugly, so I went for the property without
value. What do you think?

Best regards
Uwe

 .../devicetree/bindings/input/rotary-encoder.txt      |  2 ++
 drivers/input/misc/rotary_encoder.c                   | 19 ++++++++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/rotary-encoder.txt b/Documentation/devicetree/bindings/input/rotary-encoder.txt
index 6c9f0c8a846c..4a96e4a32503 100644
--- a/Documentation/devicetree/bindings/input/rotary-encoder.txt
+++ b/Documentation/devicetree/bindings/input/rotary-encoder.txt
@@ -20,6 +20,8 @@ Optional properties:
   2: Half-period mode
   4: Quarter-period mode
 - wakeup-source: Boolean, rotary encoder can wake up the system.
+- rotary-encoder,encoding-binary: The device uses binary states instead of
+  gray encoding (which is the default).
 
 Deprecated properties:
 - rotary-encoder,half-period: Makes the driver work on half-period mode.
diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index 96c486de49e0..ce43a59ca41a 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -28,6 +28,11 @@
 
 #define DRV_NAME "rotary-encoder"
 
+enum rotary_encoder_encoding {
+	ROTENC_GRAY,
+	ROTENC_BINARY,
+};
+
 struct rotary_encoder {
 	struct input_dev *input;
 
@@ -37,6 +42,7 @@ struct rotary_encoder {
 	u32 axis;
 	bool relative_axis;
 	bool rollover;
+	enum rotary_encoder_encoding encoding;
 
 	unsigned int pos;
 
@@ -57,9 +63,11 @@ static unsigned rotary_encoder_get_state(struct rotary_encoder *encoder)
 
 	for (i = 0; i < encoder->gpios->ndescs; ++i) {
 		int val = gpiod_get_value_cansleep(encoder->gpios->desc[i]);
-		/* convert from gray encoding to normal */
-		if (ret & 1)
-			val = !val;
+
+		if (encoder->encoding == ROTENC_GRAY)
+			/* convert from gray encoding to normal */
+			if (ret & 1)
+				val = !val;
 
 		ret = ret << 1 | val;
 	}
@@ -213,6 +221,11 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 	encoder->rollover =
 		device_property_read_bool(dev, "rotary-encoder,rollover");
 
+	if (device_property_read_bool(dev, "rotary-encoder,encoding-binary"))
+		encoder->encoding = ROTENC_BINARY;
+	else
+		encoder->encoding = ROTENC_GRAY;
+
 	device_property_read_u32(dev, "linux,axis", &encoder->axis);
 	encoder->relative_axis =
 		device_property_read_bool(dev, "rotary-encoder,relative-axis");
-- 
2.7.0

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

Re: [PATCH] Input: rotary_encoder - support binary encoding of states

[Ezequiel Garcia]

(Adding DT people)

On 22 March 2016 at 18:08, Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> A plain binary encoding has some downsides compared to the usual Gray
> encoding, but that doesn't stop hardware engineers to eventually use it.
> So implement support for this encoding in the rotary encoder driver.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
> Hello,
>
> an alternative to define this difference in the device tree is to use
> something like:
>
>         rotary-encoder,encoding = "binary";
>
> or
>
>         rotary-encoder,encoding = <ROTARY_ENCODER_ENCODING_BINARY>;
>
> instead of a property
>
>         rotary-encoder,encoding-binary;
>
> . While the two first solutions make it obvious that there can only be
> one encoding, they IMHO look ugly, so I went for the property without
> value. What do you think?
>

Yes, picking something like:

  rotary-encoder,encoding = "binary";

emphasizing the fact that only one encoding will be used,
will work better, scaling well if we need to introduce some
"foobar" encoding.

IMHO, it's better to use boolean properties on boolean stuff only.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
--
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

Re: [PATCH] Input: rotary_encoder - support binary encoding of states

[Rob Herring]

On Tue, Mar 22, 2016 at 5:50 PM, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
> (Adding DT people)
>
> On 22 March 2016 at 18:08, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>> A plain binary encoding has some downsides compared to the usual Gray
>> encoding, but that doesn't stop hardware engineers to eventually use it.
>> So implement support for this encoding in the rotary encoder driver.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>> Hello,
>>
>> an alternative to define this difference in the device tree is to use
>> something like:
>>
>>         rotary-encoder,encoding = "binary";
>>
>> or
>>
>>         rotary-encoder,encoding = <ROTARY_ENCODER_ENCODING_BINARY>;
>>
>> instead of a property
>>
>>         rotary-encoder,encoding-binary;
>>
>> . While the two first solutions make it obvious that there can only be
>> one encoding, they IMHO look ugly, so I went for the property without
>> value. What do you think?
>>
>
> Yes, picking something like:
>
>   rotary-encoder,encoding = "binary";
>
> emphasizing the fact that only one encoding will be used,
> will work better, scaling well if we need to introduce some
> "foobar" encoding.

Fine, but "rotary-encoder" is not a vendor I've ever heard of. Just
"encoding" is sufficient.

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

Re: [PATCH] Input: rotary_encoder - support binary encoding of states

[Uwe Kleine-König]

Hello Ezequiel,

On Tue, Mar 22, 2016 at 07:47:06PM -0500, Rob Herring wrote:
> On Tue, Mar 22, 2016 at 5:50 PM, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
> > (Adding DT people)

Thanks. (I thought addressing the list was enough.)

> > On 22 March 2016 at 18:08, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> >> A plain binary encoding has some downsides compared to the usual Gray
> >> encoding, but that doesn't stop hardware engineers to eventually use it.
> >> So implement support for this encoding in the rotary encoder driver.
> >>
> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> >> ---
> >> Hello,
> >>
> >> an alternative to define this difference in the device tree is to use
> >> something like:
> >>
> >>         rotary-encoder,encoding = "binary";
> >>
> >> or
> >>
> >>         rotary-encoder,encoding = <ROTARY_ENCODER_ENCODING_BINARY>;
> >>
> >> instead of a property
> >>
> >>         rotary-encoder,encoding-binary;
> >>
> >> . While the two first solutions make it obvious that there can only be
> >> one encoding, they IMHO look ugly, so I went for the property without
> >> value. What do you think?
> >>
> >
> > Yes, picking something like:
> >
> >   rotary-encoder,encoding = "binary";
> >
> > emphasizing the fact that only one encoding will be used,
> > will work better, scaling well if we need to introduce some
> > "foobar" encoding.

OK, then I stick to strings to not have to introduce magic constants or
cpp symbols?

> Fine, but "rotary-encoder" is not a vendor I've ever heard of. Just
> "encoding" is sufficient.

I picked that to be consistent with rotary-encoder,steps and other
already existing properties documented in
Documentation/devicetree/bindings/input/rotary-encoder.txt.

Should the prefix be dropped for these, too (with compat code)?

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 linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Input: rotary_encoder - support binary encoding of states

[Rob Herring]

On Wed, Mar 23, 2016 at 1:49 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hello Ezequiel,
>
> On Tue, Mar 22, 2016 at 07:47:06PM -0500, Rob Herring wrote:
>> On Tue, Mar 22, 2016 at 5:50 PM, Ezequiel Garcia
>> <ezequiel@vanguardiasur.com.ar> wrote:
>> > (Adding DT people)
>
> Thanks. (I thought addressing the list was enough.)
>
>> > On 22 March 2016 at 18:08, Uwe Kleine-König
>> > <u.kleine-koenig@pengutronix.de> wrote:
>> >> A plain binary encoding has some downsides compared to the usual Gray
>> >> encoding, but that doesn't stop hardware engineers to eventually use it.
>> >> So implement support for this encoding in the rotary encoder driver.
>> >>
>> >> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> >> ---
>> >> Hello,
>> >>
>> >> an alternative to define this difference in the device tree is to use
>> >> something like:
>> >>
>> >>         rotary-encoder,encoding = "binary";
>> >>
>> >> or
>> >>
>> >>         rotary-encoder,encoding = <ROTARY_ENCODER_ENCODING_BINARY>;
>> >>
>> >> instead of a property
>> >>
>> >>         rotary-encoder,encoding-binary;
>> >>
>> >> . While the two first solutions make it obvious that there can only be
>> >> one encoding, they IMHO look ugly, so I went for the property without
>> >> value. What do you think?
>> >>
>> >
>> > Yes, picking something like:
>> >
>> >   rotary-encoder,encoding = "binary";
>> >
>> > emphasizing the fact that only one encoding will be used,
>> > will work better, scaling well if we need to introduce some
>> > "foobar" encoding.
>
> OK, then I stick to strings to not have to introduce magic constants or
> cpp symbols?
>
>> Fine, but "rotary-encoder" is not a vendor I've ever heard of. Just
>> "encoding" is sufficient.
>
> I picked that to be consistent with rotary-encoder,steps and other
> already existing properties documented in
> Documentation/devicetree/bindings/input/rotary-encoder.txt.

Uggg.

> Should the prefix be dropped for these, too (with compat code)?

On one hand, there's only one user of one property in the kernel, but
I'd guess there are others in the wild. Probably not worth carrying
both, so I guess just leave this for consistency.

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

Re: [PATCH] Input: rotary_encoder - support binary encoding of states

[Daniel Mack]

On 03/23/2016 01:46 PM, Rob Herring wrote:
> On Wed, Mar 23, 2016 at 1:49 AM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:

>>> Fine, but "rotary-encoder" is not a vendor I've ever heard of. Just
>>> "encoding" is sufficient.
>>
>> I picked that to be consistent with rotary-encoder,steps and other
>> already existing properties documented in
>> Documentation/devicetree/bindings/input/rotary-encoder.txt.
> 
> Uggg.

IIRC, the discussion back then concluded that there should be no
properties without prefixes except for properties that are really
generic, which wasn't an applicable argument for those of this driver.
Then again, this goes back many years, and the rules might have changed.

>> Should the prefix be dropped for these, too (with compat code)?
> 
> On one hand, there's only one user of one property in the kernel, but
> I'd guess there are others in the wild. Probably not worth carrying
> both, so I guess just leave this for consistency.

Wasn't there a general agreement to *not* provide guarantees on the
stability of TD bindings, in order to be able to refactor and unify
them? Sorry, I haven't followed the discussion around all that for a
while, so I'm just asking out of curiosity.

As far as I'm concerned, I could live with any kind of such change and
notify the users I know of personally so they can follow up. But I don't
know about others of course.


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