| Message ID | 20231108005337.45069-1-anshulusr@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v8,1/2] dt-bindings: input: bindings for Adafruit Seesaw Gamepad | expand |
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@microchip.com> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Signed-off-by: Anshul Dalal <anshulusr@gmail.com> > --- > > Changes for v8: > - no updates > > Changes for v7: > - no updates > > Changes for v6: > - no updates > > Changes for v5: > - Added link to the datasheet > > Changes for v4: > - Fixed the URI for the id field > - Added `interrupts` property > > Changes for v3: > - Updated id field to reflect updated file name from previous version > - Added `reg` property > > Changes for v2: > - Renamed file to `adafruit,seesaw-gamepad.yaml` > - Removed quotes for `$id` and `$schema` > - Removed "Bindings for" from the description > - Changed node name to the generic name "joystick" > - Changed compatible to 'adafruit,seesaw-gamepad' instead of > 'adafruit,seesaw_gamepad' > --- > .../input/adafruit,seesaw-gamepad.yaml | 60 +++++++++++++++++++ > 1 file changed, 60 insertions(+) > create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml > > diff --git a/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml > new file mode 100644 > index 000000000000..3f0d1c5a3b9b > --- /dev/null > +++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml > @@ -0,0 +1,60 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/input/adafruit,seesaw-gamepad.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Adafruit Mini I2C Gamepad with seesaw > + > +maintainers: > + - Anshul Dalal <anshulusr@gmail.com> > + > +description: | > + Adafruit Mini I2C Gamepad > + > + +-----------------------------+ > + | ___ | > + | / \ (X) | > + | | S | __ __ (Y) (A) | > + | \___/ |ST| |SE| (B) | > + | | > + +-----------------------------+ > + > + S -> 10-bit percision bidirectional analog joystick > + ST -> Start > + SE -> Select > + X, A, B, Y -> Digital action buttons > + > + Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf > + Product page: https://www.adafruit.com/product/5743 > + Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw > + > +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? > + > + 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/
On 14/11/2023 20:20, 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@microchip.com> >> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Signed-off-by: Anshul Dalal <anshulusr@gmail.com> >> --- >> >> Changes for v8: >> - no updates >> >> Changes for v7: >> - no updates >> >> Changes for v6: >> - no updates >> >> Changes for v5: >> - Added link to the datasheet >> >> Changes for v4: >> - Fixed the URI for the id field >> - Added `interrupts` property >> >> Changes for v3: >> - Updated id field to reflect updated file name from previous version >> - Added `reg` property >> >> Changes for v2: >> - Renamed file to `adafruit,seesaw-gamepad.yaml` >> - Removed quotes for `$id` and `$schema` >> - Removed "Bindings for" from the description >> - Changed node name to the generic name "joystick" >> - Changed compatible to 'adafruit,seesaw-gamepad' instead of >> 'adafruit,seesaw_gamepad' >> --- >> .../input/adafruit,seesaw-gamepad.yaml | 60 +++++++++++++++++++ >> 1 file changed, 60 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml >> >> diff --git a/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml >> new file mode 100644 >> index 000000000000..3f0d1c5a3b9b >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml >> @@ -0,0 +1,60 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/input/adafruit,seesaw-gamepad.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Adafruit Mini I2C Gamepad with seesaw >> + >> +maintainers: >> + - Anshul Dalal <anshulusr@gmail.com> >> + >> +description: | >> + Adafruit Mini I2C Gamepad >> + >> + +-----------------------------+ >> + | ___ | >> + | / \ (X) | >> + | | S | __ __ (Y) (A) | >> + | \___/ |ST| |SE| (B) | >> + | | >> + +-----------------------------+ >> + >> + S -> 10-bit percision bidirectional analog joystick >> + ST -> Start >> + SE -> Select >> + X, A, B, Y -> Digital action buttons >> + >> + Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf >> + Product page: https://www.adafruit.com/product/5743 >> + Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw >> + >> +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 should, why was it dropped? > > 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? > >> + >> + 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? Bindings describe the hardware, no the driver. If the hardware has interrupt line, it should be described here. Best regards, Krzysztof
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@microchip.com> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > Signed-off-by: Anshul Dalal <anshulusr@gmail.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
diff --git a/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml new file mode 100644 index 000000000000..3f0d1c5a3b9b --- /dev/null +++ b/Documentation/devicetree/bindings/input/adafruit,seesaw-gamepad.yaml @@ -0,0 +1,60 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/input/adafruit,seesaw-gamepad.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Adafruit Mini I2C Gamepad with seesaw + +maintainers: + - Anshul Dalal <anshulusr@gmail.com> + +description: | + Adafruit Mini I2C Gamepad + + +-----------------------------+ + | ___ | + | / \ (X) | + | | S | __ __ (Y) (A) | + | \___/ |ST| |SE| (B) | + | | + +-----------------------------+ + + S -> 10-bit percision bidirectional analog joystick + ST -> Start + SE -> Select + X, A, B, Y -> Digital action buttons + + Datasheet: https://cdn-learn.adafruit.com/downloads/pdf/gamepad-qt.pdf + Product page: https://www.adafruit.com/product/5743 + Arduino Driver: https://github.com/adafruit/Adafruit_Seesaw + +properties: + compatible: + const: adafruit,seesaw-gamepad + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + description: + The gamepad's IRQ pin triggers a rising edge if interrupts are enabled. + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + joystick@50 { + compatible = "adafruit,seesaw-gamepad"; + reg = <0x50>; + }; + };