Message-ID: <403412d0-18b8-8d2f-e044-0e27b06a2d12@linaro.org>
Date:   Mon, 1 May 2023 08:37:49 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     8a790966-d985-c0fc-498e-c17e69a6622e@...aro.org
Cc:     devicetree@...r.kernel.org, dmitry.torokhov@...il.com,
        jiriv@...s.com, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org, robh+dt@...nel.org,
        u.kleine-koenig@...gutronix.de
Subject: Re: Fwd: [PATCH 1/2] dt-bindings: input: microchip,cap11xx: add
 advanced sensitivity settings

On 28/04/2023 19:09, Jiri Valek - 2N wrote:
> Hi Krzysztof,
> and thanks for the review
> 
> On 4/15/23 11:10, Krzysztof Kozlowski wrote:
>> On 15/04/2023 01:38, Jiri Valek - 2N wrote:
>>> Add support for advanced sensitivity settings and signal guard feature.
>>>
>>> Signed-off-by: Jiri Valek - 2N <jiriv@...s.com>
>>> ---
>>>   .../bindings/input/microchip,cap11xx.yaml     | 64 ++++++++++++++++++-
>>>   1 file changed, 61 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
>>> index 5fa625b5c5fb..08e28226a164 100644
>>> --- a/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
>>> +++ b/Documentation/devicetree/bindings/input/microchip,cap11xx.yaml
>>> @@ -45,13 +45,13 @@ properties:
>>>         Enables the Linux input system's autorepeat feature on the input device.
>>>   
>>>     linux,keycodes:
>>> -    minItems: 6
>>> -    maxItems: 6
>>> +    minItems: 3
>>> +    maxItems: 8
>>>       description: |
>>>         Specifies an array of numeric keycode values to
>>>         be used for the channels. If this property is
>>>         omitted, KEY_A, KEY_B, etc are used as defaults.
>>> -      The array must have exactly six entries.
>>> +      The number of entries must correspond to the number of channels.
>>>   
>>>     microchip,sensor-gain:
>>>       $ref: /schemas/types.yaml#/definitions/uint32
>>> @@ -70,6 +70,58 @@ properties:
>>>         open drain. This property allows using the active
>>>         high push-pull output.
>>>   
>>> +  microchip,sensitivity-delta-sense:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    default: 32
>>> +    enum: [1, 2, 4, 8, 16, 32, 64, 128]
>>> +    description: |
>>
>> Do not need '|' unless you need to preserve formatting.
> 
> OK. Will remove them.
> 
>>
>>> +      Optional parameter. Controls the sensitivity multiplier of a touch detection.
>>> +      At the more sensitive settings, touches are detected for a smaller delta
>>> +      capacitance corresponding to a “lighter” touch.
>>> +
>>> +  microchip,sensitivity-base-shift:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    default: 256
>>> +    enum: [1, 2, 4, 8, 16, 32, 64, 128, 256]
>>> +    description: |
>>> +      Optional parameter. Controls data scaling factor.
>>> +      The higher the value of these bits, the larger the range and the lower
>>> +      the resolution of the data presented. These settings will not affect
>>> +      touch detection or sensitivity.
>>> +
>>> +  microchip,signal-guard:
>>> +    minItems: 3
>>> +    maxItems: 8
>>> +    enum: [0, 1]
>>> +    default: 0
>>
>> This was not really tested. Missing ref, mixing scalar and array
>> properties. You want items with enum. And drop default.
> 
> Ack. I will fix it.
> 
>>
>>
>>> +    description: |
>>> +      Optional parameter supported only for CAP129x.
>>
>> Then disallow it for others (allOf:if:then: ...
>> microchip,signal-guard:false)
> 
> Ack. I will fix it.
> 
>>> +      The signal guard isolates the signal from virtual grounds.
>>> +      If enabled then the behavior of the channel is changed to signal guard.
>>> +      The number of entries must correspond to the number of channels.
>>> +
>>> +  microchip,input-treshold:
>>> +    minItems: 3
>>> +    maxItems: 8
>>> +    minimum: 0
>>> +    maximum: 127
>>> +    default: 64
>>> +    description: |
>>> +      Optional parameter. Specifies the delta threshold that is used to
>>> +      determine if a touch has been detected.
>>> +      The number of entries must correspond to the number of channels.
>>> +
>>> +  microchip,calib-sensitivity:
>>> +    minItems: 3
>>> +    maxItems: 8
>>> +    enum: [1, 2, 4]
>>> +    default: 1
>>> +    description: |
>>> +      Optional parameter supported only for CAP129x. Specifies an array of
>>> +      numeric values that controls the gain used by the calibration routine to
>>> +      enable sensor inputs to be more sensitive for proximity detection.
>>> +      The number of entries must correspond to the number of channels.
>>
>> Most of these properties do not look like hardware properties. Policies
>> and runtime configuration should not be put into DT. Explain please why
>> these are board-specific thus suitable for DT.
> 
> All these parameters are intended to set HW properties of touch buttons. 

I know, but some HW properties are software policies. Consider the
simplest example - audio volume of a speaker. It's a hardware property,
but it is not for DT. Software should choose audio volume based on
user's decisions.

> Each button can have different PCB layout and when you start without 
> setting these parameters in DT, then touches won't be detected or you 
> will get false positive readings.
> E.g. 'signal-guard' change property of analog input from button to some 
> type of shield.
> I made all of them optional for backward compatibility.
> Maybe 'sensitivity-base-shift' is really not necessary to have in DT.
> I will remove it if you agree.

Keep only ones which are not policies but depend on physical/electrical
characteristic of boards.

Best regards,
Krzysztof

Powered by blists - more mailing lists