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

[Jeff LaBundy]

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@xxxxxxxxxxxxx>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> > Signed-off-by: Anshul Dalal <anshulusr@xxxxxxxxx>
> > ---

[...]

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