On Fri, Oct 30, 2020 at 08:12:06PM +0100, Benjamin Tissoires wrote:

On Fri, Oct 30, 2020 at 7:00 PM Rob Herring [off-list ref] wrote:

quoted

On Fri, Oct 30, 2020 at 11:51:53AM +0100, Benjamin Tissoires wrote:

quoted

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.

quoted

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 [off-list ref] wrote:

quoted

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.

quoted

quoted

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