On Fri, Oct 30, 2020 at 08:12:06PM +0100, Benjamin Tissoires wrote: > On Fri, Oct 30, 2020 at 7:00 PM Rob Herring <robh@xxxxxxxxxx> wrote: > > > > On Fri, Oct 30, 2020 at 11:51:53AM +0100, Benjamin Tissoires wrote: > > > Hi Doug, > > > > > > Foreword: I was about to say "yeah, whatever" to please Rob for once. > > > > Read my other reply first... I think we mostly agree. > > > > > 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. > > > > It's not really any different than PCI and USB VID/PIDs. > > Well, it is, because in the USB (HID) world, there is a specification > that provides all of the entry points a device needs. In the i2c-hid > case, the only entry point a device needs, in the ACPI world is one > register address, and this is provided by ACPI itself. So in the ACPI > world, for i2c-hid devices, we don't need to recompile the driver to > support any current or new devices. > > > > > > 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. > > > > Or does something outside of what the spec covers. > > This is solved in the ACPI case by running ACPI callbacks, and I am > more and more thinking we should mimic that for DT devices. So this is the root of the problem. I2CHID spec was done for ACPI-based systems, with very limited interface between hardware and the kernel and all "unplesantness" such as powering up and down devices properly tucked safely away into firmware. So there is still a lot of custom code, we just do not see it and can pretend it does not exist. So even in case of "standard" I2C one can not say they do not need to recompile to use a new device, they just need to recompile different thing (driver vs firmware). I am still unsure if we want a flexible way of describing power up sequence, or simply hard-code based on a given model. Given that here are many I2C-HID compatible devices a flexible scheme would be nice IMO. Thanks. -- Dmitry
