linux-kernel - Re: [PATCH v2] arm64: dts: qcom: Add Lenovo Miix 630

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190412153040.GB7481@lakrids.cambridge.arm.com>
Date:   Fri, 12 Apr 2019 16:30:41 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Jeffrey Hugo <jeffrey.l.hugo@...il.com>
Cc:     bjorn.andersson@...aro.org, agross@...nel.org,
        David Brown <david.brown@...aro.org>, robh+dt@...nel.org,
        Lee Jones <lee.jones@...aro.org>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] arm64: dts: qcom: Add Lenovo Miix 630

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.

> 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"

Thanks,
Mark.