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

[Maxime Ripard]

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@xxxxxxxxxxx>
> ---
>  .../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@xxxxxxxxxxx>
> +
> +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

Attachment: signature.asc
Description: PGP signature