Message-ID: <20250512195020.GA3882546-robh@kernel.org>
Date: Mon, 12 May 2025 14:50:20 -0500
From: Rob Herring <robh@...nel.org>
To: David Bauer <mail@...id-bauer.net>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>, 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

On Tue, May 06, 2025 at 12:05:43PM +0200, David Bauer wrote:
> 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.

Probably that I don't use 'robh+dt' for a while now. Just 'robh'.

> > 
> > 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?

In the 'description' above, but not the driver support part.

> > > +
> > > +  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]

maximum: 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?

Yes.


> > > +        $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?

Actually, I think it is fine as-is if the LED driver works 
simultaneously with the touch input and isn't mutually exclusive. The 
'led' node is for the LED. The parent node is the driver/controller.

If they are mutually exclusive, then I'd say you want channel@[0-7] and 
led@[0-7] nodes at the same level.

Rob

Powered by blists - more mailing lists