On Fri, Apr 12, 2019 at 9:30 AM Mark Rutland <mark.rutland@xxxxxxx> wrote:
>
> On Fri, Apr 12, 2019 at 09:19:18AM -0600, Jeffrey Hugo wrote:
> > > > + keyboard@3a {
> > > > + /* QTEC0001 is the ACPI HID, which could be used for quirks */
> > > > + compatible = "QTEC0001", "hid-over-i2c";
> > >
> > > As mentioned last time, please drop the ACPI HID, and allocate a real
> > > compatible string.
> >
> > So, I'm in a quandary with this device. As far as I can tell, its an
> > off the shelf component, the device adheres to the "PNP0C50" spec (HID
> > over I2C), and can be driven by the full "hid-over-i2c" driver (which
> > is just a DT shim over the PNP0C50 ACPI driver). However, the device
> > itself identifies itself as an ELAN 400 device, which is an ID that is
> > also used for standalone touchpad devices. Per my understanding of
> > the Linux drivers, there is a separate ELAN driver for the standalone
> > touchpad devices as its been discovered though trial and error that
> > the Linux PNP0C50 driver cannot drive those devices. To handle this,
> > there is a quirk in hid-quirks which rejects ELAN 400 devices, except
> > those which are "QTEC0001".
> >
> > We need that quirk bypass for this device because the ELAN driver
> > cannot handle this device.
>
> This is useful context; thanks for writing this up!
>
> > I'd much rather have a single identifier to quirk on, rather than
> > having one for DT and one for ACPI, and its not looking feasible to
> > get the vendor to update the ACPI, so it seems like using the ACPI
> > identifier is just simpler.
> >
> > So, if you want a different compatible string, I'll need to go put DT
> > in a driver that is primarily ACPI. I'm not sure what the HID folks
> > will think of that.
>
> My objection is that an ACPI HID is _not_ a DT compatible string, and
> the two should be treated separately. Munging the two together opens the
> door for other pain.
>
> The driver in question has a DT probe function, i2c_hid_of_probe, so
> there's certainly a place to wire up that quirk.
Except that isn't where HID quirks are handled. They are handled in
drivers/hid/hid-quirks.c
There is a table of vendor/device IDs of HIDs which need quirking. In
hid_ignore(), there is provision to detect based on vendor/device and
the device name, whether the device can be driven by the HID
framework, or needs to be handled by a standalone driver. All the I2C
HID DT driver is doing is filling in the vendor/device IDs (read from
the hardware) into the HID framework struct. The device name seems to
come from standard mechanisms.
>
> > I'll propose it, but what do you view is a
> > "proper" compatible string? "ELAN0400-msm8998-clamshell"?
>
> A proper compatible string has a vendor-prefix, and is documented
> somewhere in Documentation/devicetree/bindings.
>
> e.g. you could allocate something like:
>
> "qcom,msm8998-clamshell-hid-over-i2c"
Ok, let me create a simple bindings file, propose to amend the
hid-quirks to accept the DT string, and then have a v3 of the miix 630
dts all as one series to hopefully get everyone on the same page and
happy with things.
