diff mbox series

[2/3] Input: rotary-encoder-counter: add DT bindings

Message ID 20200406155806.1295169-3-kamel.bouhara@bootlin.com
State New
Headers show
Series Introduce a counter inkernel API | expand

Commit Message

Kamel Bouhara April 6, 2020, 3:58 p.m. UTC
Add dt binding for the counter variant of the rotary encoder driver.

Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
---
 .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml

Comments

Maxime Ripard April 7, 2020, 9:41 a.m. UTC | #1
Hi Kamel,

The prefix for device tree bindings is usually dt-bindings:
$framework: $title

So a title like "dt-bindings: input: Add a counter-based rotary
encoder binding" would be better.

On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> Add dt binding for the counter variant of the rotary encoder driver.
>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
>  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> new file mode 100644
> index 000000000000..a59f7c1faf0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: GPL-2.0

Bindings are usually used by other OS's, so you should consider
putting it under a more permissive license, usually that would be GPL2
and the BSD-2-Clause

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rotary Encoder Counter
> +
> +maintainers:
> +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> +
> +description:
> +  Registers a Rotary encoder connected through a counter device.

You shouldn't really describe the action here, but more what the
binding is about. The registration will not depend on the presence of
the node following that binding, but rather on whether or not the OS
that uses it has support for it.

> +properties:
> +  compatible:
> +    const: rotary-encoder-counter
> +
> +  counter:
> +    description: Phandle for the counter device providing rotary position.

This should have a type

> +  linux-axis:
> +    description: The input subsystem axis to map to this rotary encoder.
> +    type: boolean
> +
> +  qdec-mode:
> +    description: |
> +      Quadrature decoder function to set in the counter device.
> +      3: x1-PHA
> +      4: x1-PHB
> +      5: x2-PHA
> +      6: x2-PHB
> +      7: x4-PHA and PHB

That range (even though it's a bit odd) should be expressed through an
enum so that you can check that the values are actually within that
range.

> +  steps:
> +    description: Number of steps in a full turnaround of the encoder.

Muli-line strings should have either quotes around them, or a | or >
like you did for the description. | will keep the \n, > will make that
a single string.

This should also have a type

> +      Only relevant for absolute axis.

This should be expressed through a if / then clause, or a dependencies one

>                                         Defaults to 24 which is a typical
> +      value for such devices.

This should be expressed through a default property.

> +  relative-axis:
> +    description: Register a relative axis rather than an absolute one.
> +    type: boolean
> +
> +  rollover:
> +    description: Automatic rollover when the rotary value becomes greater
> +      than the specified steps or smaller than 0. For absolute axis only.
> +    type: boolean

Same story than steps for the dependency. Also, what is is the
behaviour when this property isn't set?

> +  poll-interval:
> +    description: Poll interval at which the position is read from the counter
> +      device (default 500ms).

It should have a type too, and a default property

> +
> +required:
> +  - compatible
> +  - counter
> +  - qdec-mode
> +
> +examples:
> +  - |
> +    rotary@0 {
> +        compatible = "rotary-encoder-counter";

A unit-address (the part after @) only makes sense for a node if
there's a matching reg property in the node. This will trigger a DTC
warning, so you should remove the @0

Maxime
Kamel Bouhara April 7, 2020, 11:03 a.m. UTC | #2
On Tue, Apr 07, 2020 at 11:41:59AM +0200, Maxime Ripard wrote:
> Hi Kamel,
>

Hi Maxime,

> The prefix for device tree bindings is usually dt-bindings:
> $framework: $title
>
> So a title like "dt-bindings: input: Add a counter-based rotary
> encoder binding" would be better.
>

OK, to be fixed then.

> On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > Add dt binding for the counter variant of the rotary encoder driver.
> >
> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > ---
> >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > new file mode 100644
> > index 000000000000..a59f7c1faf0c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > @@ -0,0 +1,67 @@
> > +# SPDX-License-Identifier: GPL-2.0
>
> Bindings are usually used by other OS's, so you should consider
> putting it under a more permissive license, usually that would be GPL2
> and the BSD-2-Clause
>

Well to be honest I just looked into an existing binding and I guess
the wrong one :).

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rotary Encoder Counter
> > +
> > +maintainers:
> > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > +
> > +description:
> > +  Registers a Rotary encoder connected through a counter device.
>
> You shouldn't really describe the action here, but more what the
> binding is about. The registration will not depend on the presence of
> the node following that binding, but rather on whether or not the OS
> that uses it has support for it.
>

Then shall it be better with just :
"A rotary encoder device using a generic counter interface." ?

> > +properties:
> > +  compatible:
> > +    const: rotary-encoder-counter
> > +
> > +  counter:
> > +    description: Phandle for the counter device providing rotary position.
>
> This should have a type
>
> > +  linux-axis:
> > +    description: The input subsystem axis to map to this rotary encoder.
> > +    type: boolean
> > +
> > +  qdec-mode:
> > +    description: |
> > +      Quadrature decoder function to set in the counter device.
> > +      3: x1-PHA
> > +      4: x1-PHB
> > +      5: x2-PHA
> > +      6: x2-PHB
> > +      7: x4-PHA and PHB
>
> That range (even though it's a bit odd) should be expressed through an
> enum so that you can check that the values are actually within that
> range.
>

Indeed, that make sens to check it from the binding.

Will fix it in v2.

> > +  steps:
> > +    description: Number of steps in a full turnaround of the encoder.
>
> Muli-line strings should have either quotes around them, or a | or >
> like you did for the description. | will keep the \n, > will make that
> a single string.
>
> This should also have a type
>
> > +      Only relevant for absolute axis.
>
> This should be expressed through a if / then clause, or a dependencies one
>
> >                                         Defaults to 24 which is a typical
> > +      value for such devices.
>
> This should be expressed through a default property.
>

The devil is in the details and yet quite lot of them to fix.

Thanks.

> > +  relative-axis:
> > +    description: Register a relative axis rather than an absolute one.
> > +    type: boolean
> > +
> > +  rollover:
> > +    description: Automatic rollover when the rotary value becomes greater
> > +      than the specified steps or smaller than 0. For absolute axis only.
> > +    type: boolean
>
> Same story than steps for the dependency. Also, what is is the
> behaviour when this property isn't set?
>

OK, if rollover isn't set then the count is unbounded, of course this
shall be described here.

> > +  poll-interval:
> > +    description: Poll interval at which the position is read from the counter
> > +      device (default 500ms).
>
> It should have a type too, and a default property
>
> > +
> > +required:
> > +  - compatible
> > +  - counter
> > +  - qdec-mode
> > +
> > +examples:
> > +  - |
> > +    rotary@0 {
> > +        compatible = "rotary-encoder-counter";
>
> A unit-address (the part after @) only makes sense for a node if
> there's a matching reg property in the node. This will trigger a DTC
> warning, so you should remove the @0
>

Ok I'll fix it then.

Thanks again.

> Maxime



--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Maxime Ripard April 7, 2020, 2:22 p.m. UTC | #3
On Tue, Apr 07, 2020 at 01:03:39PM +0200, Kamel Bouhara wrote:
> > On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > > Add dt binding for the counter variant of the rotary encoder driver.
> > >
> > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > ---
> > >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> > >  1 file changed, 67 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > new file mode 100644
> > > index 000000000000..a59f7c1faf0c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > @@ -0,0 +1,67 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> >
> > Bindings are usually used by other OS's, so you should consider
> > putting it under a more permissive license, usually that would be GPL2
> > and the BSD-2-Clause
>
> Well to be honest I just looked into an existing binding and I guess
> the wrong one :).

Not the wrong ones, but the old ones :)

It's painful to change a license on existing files, whereas it's
pretty easy to mention it during review.

> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Rotary Encoder Counter
> > > +
> > > +maintainers:
> > > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > +
> > > +description:
> > > +  Registers a Rotary encoder connected through a counter device.
> >
> > You shouldn't really describe the action here, but more what the
> > binding is about. The registration will not depend on the presence of
> > the node following that binding, but rather on whether or not the OS
> > that uses it has support for it.
> >
>
> Then shall it be better with just :
> "A rotary encoder device using a generic counter interface." ?

The generic counter interface is a Linux-only stuff though, some other
OS might want to implement something else. Something like "based on a
counter"?

Maxime
Kamel Bouhara April 7, 2020, 2:55 p.m. UTC | #4
On Tue, Apr 07, 2020 at 04:22:38PM +0200, Maxime Ripard wrote:
> On Tue, Apr 07, 2020 at 01:03:39PM +0200, Kamel Bouhara wrote:
> > > On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > > > Add dt binding for the counter variant of the rotary encoder driver.
> > > >
> > > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > > ---
> > > >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> > > >  1 file changed, 67 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > > new file mode 100644
> > > > index 000000000000..a59f7c1faf0c
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > > @@ -0,0 +1,67 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > >
> > > Bindings are usually used by other OS's, so you should consider
> > > putting it under a more permissive license, usually that would be GPL2
> > > and the BSD-2-Clause
> >
> > Well to be honest I just looked into an existing binding and I guess
> > the wrong one :).
>
> Not the wrong ones, but the old ones :)
>
> It's painful to change a license on existing files, whereas it's
> pretty easy to mention it during review.
>

Alright.

> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Rotary Encoder Counter
> > > > +
> > > > +maintainers:
> > > > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > > +
> > > > +description:
> > > > +  Registers a Rotary encoder connected through a counter device.
> > >
> > > You shouldn't really describe the action here, but more what the
> > > binding is about. The registration will not depend on the presence of
> > > the node following that binding, but rather on whether or not the OS
> > > that uses it has support for it.
> > >
> >
> > Then shall it be better with just :
> > "A rotary encoder device using a generic counter interface." ?
>
> The generic counter interface is a Linux-only stuff though, some other
> OS might want to implement something else. Something like "based on a
> counter"?
>

Indeed, that's fair enough.

Thanks.

> Maxime



--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
Dmitry Torokhov April 9, 2020, 10:21 p.m. UTC | #5
On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> Add dt binding for the counter variant of the rotary encoder driver.
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
>  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> new file mode 100644
> index 000000000000..a59f7c1faf0c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rotary Encoder Counter
> +
> +maintainers:
> +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> +
> +description:
> +  Registers a Rotary encoder connected through a counter device.
> +
> +properties:
> +  compatible:
> +    const: rotary-encoder-counter

I wonder if a separate driver is really needed. The original driver be
taught to use counter device when available?

> +
> +  counter:
> +    description: Phandle for the counter device providing rotary position.
> +
> +  linux-axis:
> +    description: The input subsystem axis to map to this rotary encoder.
> +    type: boolean
> +
> +  qdec-mode:
> +    description: |
> +      Quadrature decoder function to set in the counter device.
> +      3: x1-PHA
> +      4: x1-PHB
> +      5: x2-PHA
> +      6: x2-PHB
> +      7: x4-PHA and PHB

Is it really property of the rotary encoder itself or property of the
counter device?

> +
> +  steps:
> +    description: Number of steps in a full turnaround of the encoder.
> +      Only relevant for absolute axis. Defaults to 24 which is a typical
> +      value for such devices.
> +
> +  relative-axis:
> +    description: Register a relative axis rather than an absolute one.
> +    type: boolean
> +
> +  rollover:
> +    description: Automatic rollover when the rotary value becomes greater
> +      than the specified steps or smaller than 0. For absolute axis only.
> +    type: boolean
> +
> +  poll-interval:
> +    description: Poll interval at which the position is read from the counter
> +      device (default 500ms).

Is there a way found counters to signal an interrupt?

Thanks.
Alexandre Belloni April 9, 2020, 10:39 p.m. UTC | #6
Hi Dmitry,

On 09/04/2020 15:21:15-0700, Dmitry Torokhov wrote:
> On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > Add dt binding for the counter variant of the rotary encoder driver.
> > 
> > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > ---
> >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > new file mode 100644
> > index 000000000000..a59f7c1faf0c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > @@ -0,0 +1,67 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rotary Encoder Counter
> > +
> > +maintainers:
> > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > +
> > +description:
> > +  Registers a Rotary encoder connected through a counter device.
> > +
> > +properties:
> > +  compatible:
> > +    const: rotary-encoder-counter
> 
> I wonder if a separate driver is really needed. The original driver be
> taught to use counter device when available?
> 

By the original driver, do you mean drivers/input/misc/rotary_encoder.c
that is using gpios ?

> > +
> > +  counter:
> > +    description: Phandle for the counter device providing rotary position.
> > +
> > +  linux-axis:
> > +    description: The input subsystem axis to map to this rotary encoder.
> > +    type: boolean
> > +
> > +  qdec-mode:
> > +    description: |
> > +      Quadrature decoder function to set in the counter device.
> > +      3: x1-PHA
> > +      4: x1-PHB
> > +      5: x2-PHA
> > +      6: x2-PHB
> > +      7: x4-PHA and PHB
> 
> Is it really property of the rotary encoder itself or property of the
> counter device?
> 

The mode the quadrature decoder has to be put in depends on both the
rotary encoder and the qdec.

> > +
> > +  steps:
> > +    description: Number of steps in a full turnaround of the encoder.
> > +      Only relevant for absolute axis. Defaults to 24 which is a typical
> > +      value for such devices.
> > +
> > +  relative-axis:
> > +    description: Register a relative axis rather than an absolute one.
> > +    type: boolean
> > +
> > +  rollover:
> > +    description: Automatic rollover when the rotary value becomes greater
> > +      than the specified steps or smaller than 0. For absolute axis only.
> > +    type: boolean
> > +
> > +  poll-interval:
> > +    description: Poll interval at which the position is read from the counter
> > +      device (default 500ms).
> 
> Is there a way found counters to signal an interrupt?
> 

For some counters, there are interrupts available, this is not trivial
with the counter that is the target of this work but this is on the TODO
list. Of course, this will also require adding a bit more to the
in-kernel counter API to allow registering a callback that would be
called when an interrupt happens.
Dmitry Torokhov April 9, 2020, 11:46 p.m. UTC | #7
On Fri, Apr 10, 2020 at 12:39:07AM +0200, Alexandre Belloni wrote:
> Hi Dmitry,
> 
> On 09/04/2020 15:21:15-0700, Dmitry Torokhov wrote:
> > On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > > Add dt binding for the counter variant of the rotary encoder driver.
> > > 
> > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > ---
> > >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> > >  1 file changed, 67 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > new file mode 100644
> > > index 000000000000..a59f7c1faf0c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > @@ -0,0 +1,67 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Rotary Encoder Counter
> > > +
> > > +maintainers:
> > > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > +
> > > +description:
> > > +  Registers a Rotary encoder connected through a counter device.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: rotary-encoder-counter
> > 
> > I wonder if a separate driver is really needed. The original driver be
> > taught to use counter device when available?
> > 
> 
> By the original driver, do you mean drivers/input/misc/rotary_encoder.c
> that is using gpios ?

Yes.

> 
> > > +
> > > +  counter:
> > > +    description: Phandle for the counter device providing rotary position.
> > > +
> > > +  linux-axis:
> > > +    description: The input subsystem axis to map to this rotary encoder.
> > > +    type: boolean
> > > +
> > > +  qdec-mode:
> > > +    description: |
> > > +      Quadrature decoder function to set in the counter device.
> > > +      3: x1-PHA
> > > +      4: x1-PHB
> > > +      5: x2-PHA
> > > +      6: x2-PHB
> > > +      7: x4-PHA and PHB
> > 
> > Is it really property of the rotary encoder itself or property of the
> > counter device?
> > 
> 
> The mode the quadrature decoder has to be put in depends on both the
> rotary encoder and the qdec.

OK.

> 
> > > +
> > > +  steps:
> > > +    description: Number of steps in a full turnaround of the encoder.
> > > +      Only relevant for absolute axis. Defaults to 24 which is a typical
> > > +      value for such devices.
> > > +
> > > +  relative-axis:
> > > +    description: Register a relative axis rather than an absolute one.
> > > +    type: boolean
> > > +
> > > +  rollover:
> > > +    description: Automatic rollover when the rotary value becomes greater
> > > +      than the specified steps or smaller than 0. For absolute axis only.
> > > +    type: boolean
> > > +
> > > +  poll-interval:
> > > +    description: Poll interval at which the position is read from the counter
> > > +      device (default 500ms).
> > 
> > Is there a way found counters to signal an interrupt?
> > 
> 
> For some counters, there are interrupts available, this is not trivial
> with the counter that is the target of this work but this is on the TODO
> list. Of course, this will also require adding a bit more to the
> in-kernel counter API to allow registering a callback that would be
> called when an interrupt happens.

Should it be a callback, or can counter create an irqchip so that users
do not need to know how exactly it is wired up?

Thanks.
Kamel Bouhara April 11, 2020, 10:43 a.m. UTC | #8
On Thu, Apr 09, 2020 at 04:46:23PM -0700, Dmitry Torokhov wrote:
> On Fri, Apr 10, 2020 at 12:39:07AM +0200, Alexandre Belloni wrote:
> > Hi Dmitry,
> >
> > On 09/04/2020 15:21:15-0700, Dmitry Torokhov wrote:
> > > On Mon, Apr 06, 2020 at 05:58:05PM +0200, Kamel Bouhara wrote:
> > > > Add dt binding for the counter variant of the rotary encoder driver.
> > > >
> > > > Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > > ---
> > > >  .../input/rotary-encoder-counter.yaml         | 67 +++++++++++++++++++
> > > >  1 file changed, 67 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > > new file mode 100644
> > > > index 000000000000..a59f7c1faf0c
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
> > > > @@ -0,0 +1,67 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Rotary Encoder Counter
> > > > +
> > > > +maintainers:
> > > > +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> > > > +
> > > > +description:
> > > > +  Registers a Rotary encoder connected through a counter device.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    const: rotary-encoder-counter
> > >
> > > I wonder if a separate driver is really needed. The original driver be
> > > taught to use counter device when available?
> > >
> >
> > By the original driver, do you mean drivers/input/misc/rotary_encoder.c
> > that is using gpios ?
>
> Yes.
>

Well, it could be the case if the counter device could provide such a
way to signal interrupts.

> >
> > > > +
> > > > +  counter:
> > > > +    description: Phandle for the counter device providing rotary position.
> > > > +
> > > > +  linux-axis:
> > > > +    description: The input subsystem axis to map to this rotary encoder.
> > > > +    type: boolean
> > > > +
> > > > +  qdec-mode:
> > > > +    description: |
> > > > +      Quadrature decoder function to set in the counter device.
> > > > +      3: x1-PHA
> > > > +      4: x1-PHB
> > > > +      5: x2-PHA
> > > > +      6: x2-PHB
> > > > +      7: x4-PHA and PHB
> > >
> > > Is it really property of the rotary encoder itself or property of the
> > > counter device?
> > >
> >
> > The mode the quadrature decoder has to be put in depends on both the
> > rotary encoder and the qdec.
>
> OK.
>
> >
> > > > +
> > > > +  steps:
> > > > +    description: Number of steps in a full turnaround of the encoder.
> > > > +      Only relevant for absolute axis. Defaults to 24 which is a typical
> > > > +      value for such devices.
> > > > +
> > > > +  relative-axis:
> > > > +    description: Register a relative axis rather than an absolute one.
> > > > +    type: boolean
> > > > +
> > > > +  rollover:
> > > > +    description: Automatic rollover when the rotary value becomes greater
> > > > +      than the specified steps or smaller than 0. For absolute axis only.
> > > > +    type: boolean
> > > > +
> > > > +  poll-interval:
> > > > +    description: Poll interval at which the position is read from the counter
> > > > +      device (default 500ms).
> > >
> > > Is there a way found counters to signal an interrupt?
> > >
> >
> > For some counters, there are interrupts available, this is not trivial
> > with the counter that is the target of this work but this is on the TODO
> > list. Of course, this will also require adding a bit more to the
> > in-kernel counter API to allow registering a callback that would be
> > called when an interrupt happens.
>
> Should it be a callback, or can counter create an irqchip so that users
> do not need to know how exactly it is wired up?
>

Maybe for some of them yes but for others the polling is still required.

> Thanks.
>
> --
> Dmitry

--
Kamel Bouhara, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
new file mode 100644
index 000000000000..a59f7c1faf0c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/rotary-encoder-counter.yaml
@@ -0,0 +1,67 @@ 
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/rotary-encoder-counter.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rotary Encoder Counter
+
+maintainers:
+  - Kamel Bouhara <kamel.bouhara@bootlin.com>
+
+description:
+  Registers a Rotary encoder connected through a counter device.
+
+properties:
+  compatible:
+    const: rotary-encoder-counter
+
+  counter:
+    description: Phandle for the counter device providing rotary position.
+
+  linux-axis:
+    description: The input subsystem axis to map to this rotary encoder.
+    type: boolean
+
+  qdec-mode:
+    description: |
+      Quadrature decoder function to set in the counter device.
+      3: x1-PHA
+      4: x1-PHB
+      5: x2-PHA
+      6: x2-PHB
+      7: x4-PHA and PHB
+
+  steps:
+    description: Number of steps in a full turnaround of the encoder.
+      Only relevant for absolute axis. Defaults to 24 which is a typical
+      value for such devices.
+
+  relative-axis:
+    description: Register a relative axis rather than an absolute one.
+    type: boolean
+
+  rollover:
+    description: Automatic rollover when the rotary value becomes greater
+      than the specified steps or smaller than 0. For absolute axis only.
+    type: boolean
+
+  poll-interval:
+    description: Poll interval at which the position is read from the counter
+      device (default 500ms).
+
+required:
+  - compatible
+  - counter
+  - qdec-mode
+
+examples:
+  - |
+    rotary@0 {
+        compatible = "rotary-encoder-counter";
+
+        counter =<&qdec>;
+        qdec-mode = <7>;
+        relative-axis;
+    };
+...