linux-kernel - Re: [PATCH v8 1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad

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: <ZWLFOV/U90bhm4ow@nixie71>
Date:   Sat, 25 Nov 2023 22:10:33 -0600
From:   Jeff LaBundy <jeff@...undy.com>
To:     Thomas Weißschuh <linux@...ssschuh.net>
Cc:     Anshul Dalal <anshulusr@...il.com>, linux-input@...r.kernel.org,
        devicetree@...r.kernel.org, Conor Dooley <conor+dt@...nel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        linux-kernel@...r.kernel.org,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Conor Dooley <conor.dooley@...rochip.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH v8 1/2] dt-bindings: input: bindings for Adafruit Seesaw
 Gamepad

Hi Thomas and Anshul,

On Tue, Nov 14, 2023 at 08:20:46PM +0100, Thomas Weißschuh wrote:
> On 2023-11-08 06:23:35+0530, Anshul Dalal wrote:
> > Adds bindings for the Adafruit Seesaw Gamepad.
> > 
> > The gamepad functions as an i2c device with the default address of 0x50
> > and has an IRQ pin that can be enabled in the driver to allow for a rising
> > edge trigger on each button press or joystick movement.
> > 
> > Product page:
> >   https://www.adafruit.com/product/5743
> > Arduino driver:
> >   https://github.com/adafruit/Adafruit_Seesaw
> > 
> > Reviewed-by: Conor Dooley <conor.dooley@...rochip.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> > Signed-off-by: Anshul Dalal <anshulusr@...il.com>
> > ---

[...]

> > +properties:
> > +  compatible:
> > +    const: adafruit,seesaw-gamepad
> 
> I don't really have any clue about devicetree, but shouldn't the actual
> driver have an id-table for this "compatible"?
> 
> It had one up to v5 of the patchset.
> 
> Jeff had some comments about the OF aspect [0], but to me the state now
> seems incorrect.
> Maybe the DT can be dropped completely?
> 
> Jeff, could you advise?

My original comment was merely to say that this driver doesn't need an
entire binding because it is easily covered by trivial-devices.yaml. The
whole point of that binding is to save the trouble of writing a new file
such as this for trivial devices with this same set of common properties.

I don't feel strongly about _not_ adding a new binding for this device,
it just seems like unnecessary work for all involved. If the maintainers
do not mind, then I don't either :)

Taking things a step further, this driver really doesn't need to define
an of_device_id struct either, because I seem to recall that OF can still
bind to drivers with "compatible" strings specified in the i2c_device_id
struct.

At any rate, this version is most certainly broken because the compatible
string defined in this binding does not match SEESAW_DEVICE_NAME specified
in the driver's i2c_device_id struct, and there is no of_device_id struct.
So there is no way the driver would bind if this binding were followed in
earnest.

I see this has been addressed in the latest version, so I will take a look
at that one.

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +    description:
> > +      The gamepad's IRQ pin triggers a rising edge if interrupts are enabled.
> 
> Interrupts are not supported yet by the driver.
> Should the property be there already?
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        joystick@50 {
> > +            compatible = "adafruit,seesaw-gamepad";
> > +            reg = <0x50>;
> > +        };
> > +    };
> > -- 
> > 2.42.0
> 
> [0] https://lore.kernel.org/lkml/ZTWza+S+t+UZKlwu@nixie71/

Kind regards,
Jeff LaBundy