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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <f8930b5a-e1eb-7c41-19ad-6ba539c59322@gmx.de>
Date:   Tue, 22 Dec 2020 12:28:06 +0100
From:   Heinrich Schuchardt <xypron.glpk@....de>
To:     Marek Szyprowski <m.szyprowski@...sung.com>, u-boot@...ts.denx.de,
        u-boot-amlogic@...ups.io
Cc:     Neil Armstrong <narmstrong@...libre.com>,
        Lukasz Majewski <lukma@...x.de>,
        Philippe Reynes <philippe.reynes@...tathome.com>,
        Simon Glass <sjg@...omium.org>,
        Jaehoon Chung <jh80.chung@...sung.com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Rob Herring <robh@...nel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        devicetree@...r.kernel.org,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 1/3] dt-bindings: input: adc-keys bindings
 documentation

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@...sung.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>;
>> +        };
>> +    };
>>
>