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

[Jeff LaBundy]

Hi Anshul,

Thank you for this additional information.

On Mon, Oct 23, 2023 at 05:28:10PM +0530, Anshul Dalal wrote:
> Hello Jeff,
> 
> On 10/23/23 05:17, Jeff LaBundy wrote:
> > Hi Anshul,
> > 
> > On Tue, Oct 17, 2023 at 09:13:44AM +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>
> > 
> > Perhaps this ship has sailed, but is there any reason this simple device
> > cannot be added to Documentation/devicetree/bindings/trivial-devices.yaml
> > as opposed to having its own binding?
> > 
> > It has no vendor-specific properties, and the only properties are the
> > standard properties already understood by the I2C core. In case I have
> > misunderstood, please let me know.
> > 
> 
> The driver currently implements only a subset of the functionality in
> the Adafruit Seesaw specification. I eventually plan on adding adding
> full support for the Seesaw framework in the form of a driver for the
> atsamd09 seesaw breakout board:
> https://learn.adafruit.com/adafruit-seesaw-atsamd09-breakout
> 
> Then I think it would be better for this driver to use the newly exposed
> seesaw APIs by the atsamd09 driver instead of relying on kernel's i2c APIs.

The underlying functions used to implement I2C communication are orthogonal
to the binding. Whether you use the kernel's core I2C support, regmap, or
your own wrappers built on top of either have no bearing on whether or not
a binding is necessary.

The binding is used to define device tree properties that describe the
hardware and its constraints. Classic examples are things such as clock
frequency, regulator voltage, etc. Drivers often translate device tree
properties into register settings.

In the case of this device, the only thing the driver needs to know about
the hardware are its compatible string and I2C client address, both of
which are already supported in the common trivial devices binding [1].

> I would also like to add support for the provided interrupt pin later
> down the line which is documented in the binding along with description
> of the non-standard action button layout.

The trivial devices binding includes interrupts as well; please see [1].
My opinion is that the device's own documentation is responsible for
describing the product and anything unique about its physical layout.

> Above were my reasons for going for a standalone binding, please let me
> know if you disagree.

I don't see any need for a binding for this device because it has no vendor-
specific properties, and the only properties it does have are covered by
existing infrastructure.

My feedback is that this patch can be replaced with at most a two-line patch
to [1]. This is just my $.02; it is ultimately up to the maintainers. The
existing binding, albeit unnecessary, is nicely written either way :)

Kind regards,
Jeff LaBundy

[1] Documentation/devicetree/bindings/trivial-devices.yaml