Re: [PATCH] dt-bindings: input: Convert adc-keys to DT schema

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

On 6/6/22 20:42, Rob Herring wrote:
> Convert the adc-keys binding to DT schema format.
>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>   .../devicetree/bindings/input/adc-keys.txt    |  67 ------------
>   .../devicetree/bindings/input/adc-keys.yaml   | 103 ++++++++++++++++++
>   2 files changed, 103 insertions(+), 67 deletions(-)
>   delete mode 100644 Documentation/devicetree/bindings/input/adc-keys.txt
>   create mode 100644 Documentation/devicetree/bindings/input/adc-keys.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/adc-keys.txt b/Documentation/devicetree/bindings/input/adc-keys.txt
> deleted file mode 100644
> index 6c8be6a9ace2..000000000000
> --- a/Documentation/devicetree/bindings/input/adc-keys.txt
> +++ /dev/null
> @@ -1,67 +0,0 @@
> -ADC attached resistor ladder buttons
> -------------------------------------
> -
> -Required properties:
> - - compatible: "adc-keys"
> - - io-channels: Phandle to an ADC channel
> - - io-channel-names = "buttons";
> - - keyup-threshold-microvolt: Voltage above or equal to which all the keys are
> -			      considered up.
> -
> -Optional properties:
> -	- poll-interval: Poll interval time in milliseconds
> -	- autorepeat: Boolean, Enable auto repeat feature of Linux input
> -	  subsystem.
> -
> -Each button (key) is represented as a sub-node of "adc-keys":
> -
> -Required subnode-properties:
> -	- label: Descriptive name of the key.
> -	- linux,code: Keycode to emit.
> -	- press-threshold-microvolt: voltage above or equal to which this key is
> -				     considered pressed.
> -
> -No two values of press-threshold-microvolt may be the same.
> -All values of press-threshold-microvolt must be less than
> -keyup-threshold-microvolt.
> -
> -Example:
> -
> -#include <dt-bindings/input/input.h>
> -
> -	adc-keys {
> -		compatible = "adc-keys";
> -		io-channels = <&lradc 0>;
> -		io-channel-names = "buttons";
> -		keyup-threshold-microvolt = <2000000>;
> -
> -		button-up {
> -			label = "Volume Up";
> -			linux,code = <KEY_VOLUMEUP>;
> -			press-threshold-microvolt = <1500000>;
> -		};
> -
> -		button-down {
> -			label = "Volume Down";
> -			linux,code = <KEY_VOLUMEDOWN>;
> -			press-threshold-microvolt = <1000000>;
> -		};
> -
> -		button-enter {
> -			label = "Enter";
> -			linux,code = <KEY_ENTER>;
> -			press-threshold-microvolt = <500000>;
> -		};
> -	};
> -
> -+--------------------------------+------------------------+
> -| 2.000.000 <= value             | no key pressed         |
> -+--------------------------------+------------------------+
> -| 1.500.000 <= value < 2.000.000 | KEY_VOLUMEUP pressed   |
> -+--------------------------------+------------------------+
> -| 1.000.000 <= value < 1.500.000 | KEY_VOLUMEDOWN pressed |
> -+--------------------------------+------------------------+
> -|   500.000 <= value < 1.000.000 | KEY_ENTER pressed      |
> -+--------------------------------+------------------------+
> -|              value <   500.000 | no key pressed         |
> -+--------------------------------+------------------------+
> diff --git a/Documentation/devicetree/bindings/input/adc-keys.yaml b/Documentation/devicetree/bindings/input/adc-keys.yaml
> new file mode 100644
> index 000000000000..a3a1af9550bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/adc-keys.yaml
> @@ -0,0 +1,103 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/adc-keys.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADC attached resistor ladder buttons
> +
> +maintainers:
> +  - Heinrich Schuchardt <xypron.glpk@gmx.de>

Thanks for converting to yaml.

I only contributed a single patch. I am not a maintainer. Please, remove
that line.

scripts/get_maintainer.pl
Documentation/devicetree/bindings/input/adc-keys.txt yields
Dmitry Torokhov <dmitry.torokhov@gmail.com> (maintainer:INPUT (KEYBOARD,
MOUSE, JOYSTICK, TOUCHSCREEN)...)

It would be preferable to have a single reference point for
maintainership: file /MAINTAINERS.


> +  - Alexandre Belloni <alexandre.belloni@bootlin.com>
> +
> +allOf:
> +  - $ref: /schemas/input/input.yaml#
> +
> +properties:
> +  compatible:
> +    const: adc-keys
> +
> +  io-channels:
> +    maxItems: 1

Please, add a description to each property.

> +
> +  io-channel-names:
> +    const: buttons
> +
> +  keyup-threshold-microvolt:
> +    description:
> +      Voltage above or equal to which all the keys are considered up.
> +
> +patternProperties:
> +  '^button-':
> +    type: object
> +    additionalProperties: false
> +    description:
> +      Each button (key) is represented as a sub-node.
> +
> +    properties:
> +      label: true

Please, add a description.

> +
> +      linux,code:
> +        description: Keycode to emit.
> +        $ref: /schemas/types.yaml#/definitions/uint32

Unfortunately usable values are only defined in
include/uapi/linux/input-event-codes.h up to now. Please, consider
adding that link to the description.

It is unclear to me if using values above KEY_MAX (=0x2ff) could make
sense of should be forbidden by this yaml file.

For interoperability of device-trees with other operating systems we
should have a yaml file defining an enum with used values and their meaning.

> +
> +      press-threshold-microvolt:
> +        description:
> +          Voltage above or equal to which this key is considered pressed. No
> +          two values of press-threshold-microvolt may be the same. All values
> +          of press-threshold-microvolt must be less than
> +          keyup-threshold-microvolt.
> +
> +    required:
> +      - label

Property label is not used in our the driver code. It only exists for
human readability. Why is it marked as required? Stripping the labels
would reduce the DT size.

Best regards

Heinrich

> +      - linux,code
> +      - press-threshold-microvolt
> +
> +required:
> +  - compatible
> +  - io-channels
> +  - io-channel-names
> +  - keyup-threshold-microvolt
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/input/input.h>
> +    // +--------------------------------+------------------------+
> +    // | 2.000.000 <= value             | no key pressed         |
> +    // +--------------------------------+------------------------+
> +    // | 1.500.000 <= value < 2.000.000 | KEY_VOLUMEUP pressed   |
> +    // +--------------------------------+------------------------+
> +    // | 1.000.000 <= value < 1.500.000 | KEY_VOLUMEDOWN pressed |
> +    // +--------------------------------+------------------------+
> +    // |   500.000 <= value < 1.000.000 | KEY_ENTER pressed      |
> +    // +--------------------------------+------------------------+
> +    // |              value <   500.000 | no key pressed         |
> +    // +--------------------------------+------------------------+
> +
> +    adc-keys {
> +        compatible = "adc-keys";
> +        io-channels = <&lradc 0>;
> +        io-channel-names = "buttons";
> +        keyup-threshold-microvolt = <2000000>;
> +
> +        button-up {
> +            label = "Volume Up";
> +            linux,code = <KEY_VOLUMEUP>;
> +            press-threshold-microvolt = <1500000>;
> +        };
> +
> +        button-down {
> +            label = "Volume Down";
> +            linux,code = <KEY_VOLUMEDOWN>;
> +            press-threshold-microvolt = <1000000>;
> +        };
> +
> +        button-enter {
> +            label = "Enter";
> +            linux,code = <KEY_ENTER>;
> +            press-threshold-microvolt = <500000>;
> +        };
> +    };
> +...