Message-ID: <8c9e5e74-966b-4969-9776-7655863fd197@david-bauer.net>
Date: Tue, 6 May 2025 12:05:43 +0200
From: David Bauer <mail@...id-bauer.net>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>,
 Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>
Cc: linux-input@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: input: add Semtech SX951x binding

Hi Krzysztof,

thanks for the review.

On 5/6/25 08:21, Krzysztof Kozlowski wrote:
> On 05/05/2025 22:38, David Bauer wrote:
>> Add device-tree binding for the Semtech SX9512/SX9513 family of touch
>> controllers with integrated LED driver.
>>
>> Signed-off-by: David Bauer <mail@...id-bauer.net>
> 
> You CC-ed an address, which suggests you do not work on mainline kernel
> or you do not use get_maintainers.pl/b4/patman. Please rebase and always
> work on mainline or start using mentioned tools, so correct addresses
> will be used.
I'm a bit unsure what you are referring to - maybe I've set the options
for get_maintainer.pl wrong, but i use

get_maintainer.pl --nogit --nogit-fallback --norolestats --nol

to determine TO recipients and

get_maintainer.pl --nogit --nogit-fallback --norolestats --nom

for CC destinations.

Granted, my tree was a bit out of date but it was from mainline
and after rebase both commands returned consistent results.

Hope you can provide me with some guidance there.

> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC (and consider --no-git-fallback argument, so you will
> not CC people just because they made one commit years ago). It might
> happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux
> kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline) or work on fork of kernel
> (don't, instead use mainline). Just use b4 and everything should be
> fine, although remember about `b4 prep --auto-to-cc` if you added new
> patches to the patchset.
> 
> 
>> ---
>>   .../bindings/input/semtech,sx951x.yaml        | 180 ++++++++++++++++++
>>   1 file changed, 180 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/input/semtech,sx951x.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/input/semtech,sx951x.yaml b/Documentation/devicetree/bindings/input/semtech,sx951x.yaml
>> new file mode 100644
>> index 000000000000..e4f938decd86
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/semtech,sx951x.yaml
>> @@ -0,0 +1,180 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/input/semtech,sx951x.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Semtech SX9512/SX9513 based capacitive touch sensors
>> +
>> +description: |
> 
> Do not need '|' unless you need to preserve formatting.
> 
>> +  The Semtech SX9512/SX9513 Family of capacitive touch controllers
>> +  with integrated LED drivers. The device communication is using I2C only.
>> +
>> +maintainers:
>> +  - David Bauer <mail@...id-bauer.net>
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - semtech,sx9512
>> +      - semtech,sx9513
> 
> Devices are not compatible? What are the differences?

The SX9513 is a cost-reduced version which does not
support proximity sensing. With the current support
of the driver they work identical. Should i add this
information as a comment?

> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +  poll-interval:
>> +    default: 100
>> +    description: |
> 
> Do not need '|' unless you need to preserve formatting. Same comment
> everywhere.
> 
>> +      The polling interval for touch events in milliseconds.
> 
> Missing -ms property unit suffix... unless you are using existing
> property from common schema, but I do not see any reference (and thus
> unevaluatedProperties at the end).
> 
>> +
>> +patternProperties:
>> +  "^channel@[0-7]$":
>> +    $ref: input.yaml#
>> +    type: object
>> +    description: |
>> +      Each node represents a channel of the touch controller.
>> +      Each channel provides a capacitive touch sensor input and
>> +      an LED driver output.
>> +
>> +    properties:
>> +      reg:
>> +        enum: [0, 1, 2, 3, 4, 5, 6, 7]
>> +
>> +      linux,keycodes:
>> +        maxItems: 1
>> +        description: |
>> +          Specifies an array of numeric keycode values to
>> +          be used for the channels. If this property is
>> +          omitted, the channel is not used as a key.
>> +
>> +      semtech,cin-delta:
> 
> Use proper unit suffix and express it in pF.

To represent 2.3 and 3.8 pF, would it be better to represent in
femtofarad?

> 
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        minimum: 0
>> +        maximum: 3
>> +        default: 3
>> +        description: |
>> +          The capacitance delta which is used to detect a touch
>> +          or release event. The property value is mapped to a
>> +          farad range between 7pF and 2.3pF internally. The delta
>> +          becomes smaller the higher the value is.
>> +
>> +      semtech,sense-threshold:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        minimum: 0
>> +        maximum: 255
>> +        default: 4
>> +        description: |
>> +          The threshold value after which the channel detects a touch.
>> +          Refer to the datasheet for the internal calculation of the
>> +          resulting touch sensitivity.
>> +
>> +      led:
> 
> I think subnode is here not needed. This should be part of the channel,
> probably.

Just to be sure - you mean to have a property "led" upon which instructs
the channel to be used to drive an LED and include the LED specific
properties in the node of the channel?

> 
>> +        $ref: /schemas/leds/common.yaml#
>> +        type: object
>> +        unevaluatedProperties: false
>> +        description: |
>> +          Presence of this property indicates the channel
>> +          is used as an LED driver.
>> +
>> +    required:
>> +      - reg
>> +
>> +    additionalProperties: false
> 
> unevaluatedProperties instead
> 
>> +
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/input/input.h>
>> +    #include <dt-bindings/leds/common.h>
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      touch@2b {
>> +        compatible = "semtech,sx9512";
>> +
> 
> Drop blank line
> 
>> +        reg = <0x2b>;
>> +
> 
> 
> Best regards,
> Krzysztof

Powered by blists - more mailing lists