Re: [PATCH v4 1/3] dt-bindings: input: adc-keys bindings documentation

[Heinrich Schuchardt]

On 12/22/20 11:12 AM, Heinrich Schuchardt wrote:
> On 12/22/20 9:56 AM, Marek Szyprowski wrote:
> > Dump adc-keys bindings documentation from Linux kernel source tree v5.10.
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> > ---
> >   doc/device-tree-bindings/input/adc-keys.txt | 49 +++++++++++++++++++++
> >   1 file changed, 49 insertions(+)
> >   create mode 100644 doc/device-tree-bindings/input/adc-keys.txt
> >
> > diff --git a/doc/device-tree-bindings/input/adc-keys.txt
> > b/doc/device-tree-bindings/input/adc-keys.txt
> > new file mode 100644
> > index 0000000000..e551814629
> > --- /dev/null
> > +++ b/doc/device-tree-bindings/input/adc-keys.txt
> > @@ -0,0 +1,49 @@
> > +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 at 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 ADC input when this key is
> > pressed.
>
> https://www.merriam-webster.com/dictionary/threshold
> defines threshold as "a level, point, or value above which something is
> true or will take place and below which it is not or will not"
>
> "when this key is pressed" leaves it completely open if a key is
> considered pressed below or above the threshold. Please, replace the
> word 'when' by either 'above which' or 'below which'.
>
> In the example keyup-threshold-microvolt is larger than
> keyup-threshold-microvolt all values of press-threshold-microvolt. So
> one might assume that 'above' is the intended meaning and the
> interpretation of the example might be:
>
> 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
>
> Both directions 'above' and 'below' make sense. So maybe if
> keyup-threshold-microvolt is lower than all press-threshold-microvolt
> you want to invert the logic?
>
> The binding lacks a hysteresis which is needed for a reliable function.
>
> If you look into drivers/input/keyboard/adc-keys.c in the Linux source,
> you can see that it is not using the threshold value as a threshold at
> all. Instead is uses abs(st->map[i].voltage - value) to find the nearest
> "threshold" voltage level.
>
> Could you, please, try to bring this text into a form that cannot be
> misinterpreted and reconcile with upstream. This should include
>
> * a results table for the example
> * a definition if keyup-threshold-microvolt must be higher than all
>    press-threshold-microvolt or may be lower than all
>    press-threshold-microvolt
> * a sentence forbidding keyup-threshold-microvolt to be between two
>    press-threshold-microvolt
> * a definition if or when any of the thresholds is a lower or upper
>    boundary

Cf. [PATCH 1/1] dt-bindings: adc-keys.txt: clarify description
https://lore.kernel.org/linux-input/20201222110815.24121-1-xypron.glpk@xxxxxx/T/#u

Best regards

Heinrich

> Best regards
>
> Heinrich
>
> > +
> > +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>;
> > +        };
> > +    };