| Message ID | 1458680914-4533-1-git-send-email-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
(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. IMHO, it's better to use boolean properties on boolean stuff only.
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
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
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
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
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");
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(-)