Re: [PATCH v2 1/3] dt-bindings: HID: i2c-hid: Label this binding as deprecated

[Benjamin Tissoires]

Hi Doug,

Foreword: I was about to say "yeah, whatever" to please Rob for once.
But after re-reading this and more specifically patch 3 of the series,
that won't do. More comments inlined.

On Sat, Oct 24, 2020 at 1:23 AM Douglas Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> As pointed out by Rob Herring [1], we should have a device-specific
> compatible string.  This means people shouldn't be using the
> "i2c-over-hid" compatible string anymore, or at least not without a
> more specific compatible string before it.  Specifically:
>
> 1. For newly added devices we should just have the device-specific
>    device string (no "hid-over-i2c" fallback) and infer the timings
>    and hid-descr-addr from there.

And that's a big NACK from a maintainer point of view. I know in the
device tree world these strings are important so that people can just
say "I have a device compatible with X", and go on, but in the HID
world that means we will have to implement one compatible struct per
vendor/device, which is not something I want to do.

You can think of it as if you are suddenly saying that because it
would be easier for a few particular USB devices that need a quirk,
you "just" need to add the list of *all* USB HID devices that are
around. i2c-hid should be a driver that doesn't change unless 2 things
happen:
- there is a change in the spec
- there is a specific quirk required for a device that doesn't follow the spec.

So if having device tree support for these means we suddenly need to
add every single device around in the compatible table, I would be
tempted to just drop the support for those new devices.

Again, you (or anyone else) have to understand that the descriptor
address is just a parameter which is known at the manufacturing time,
but that can vary with different vendors and or products. In the ACPI
world, this parameter is provided in the DSDT, and there is no reason
for it to not be provided in the DT.

The last thing I want to see is people using device tree having to
recompile i2c-hid to register their own device.

If this part of the Device Tree binding is so important for the DT
world, then we should split up the DT bindings from i2c-hid, and have
some platform driver that would handle a conversion between devicetree
and platform data. But this driver won't be maintained by me.

I agree adding the various sleep parameters in the platform data is
not good, but I prefer that over having to maintain an endless table
of parameters for every single i2c-hid device out there.

Cheers,
Benjamin


>
> 2. If there's a need for a device tree to be backward compatible, we
>    should list the device-specific compatible string and add the
>    "hid-over-i2c" fallback and the various timings.
>
> [1] https://lore.kernel.org/r/20201019211036.GA3595039@bogus
>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
>
> Changes in v2:
> - ("dt-bindings: HID: i2c-hid: Label this binding as deprecated") new in v2.
>
>  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> index c76bafaf98d2..733a5f053280 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> @@ -1,5 +1,8 @@
>  * HID over I2C Device-Tree bindings
>
> +WARNING: this binding is deprecated.  Instead of using this, create specific
> +bindings for each hid-over-i2c device.
> +
>  HID over I2C provides support for various Human Interface Devices over the
>  I2C bus. These devices can be for example touchpads, keyboards, touch screens
>  or sensors.
> --
> 2.29.0.rc1.297.gfa9743e501-goog
>