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

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

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@samsung.com>
>> ---
>>   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@gmx.de/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>;
>> +        };
>> +    };
>>
>