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
