The Lenovo Miix 630 is one of three ARM based (specifically Qualcomm
MSM8998) laptops that comes with Windows, and seems to have a dedicated
following of folks intrested to get Linux up and running on it.
This series adds support for the basic functionality this is validated
towork using devicetree. Although the laptops do feed ACPI to Windows,
the existing MSM8998 support in mainline is DT based, so DT provides a
quick path to functionality while ACPI support is investigated.
The three devices are very similar, but do have differences in the set
of peripherals supported, so the idea is that the vast majority of the
support for all three can live in a common include, which should reduce
overall duplication. Adding support for the other two devices as a
follow on should involve minimal work.
The bleeding edge work for these laptops and work in progress can be
found at https://github.com/aarch64-laptops/prebuilt
v5:
-Split out elan_i2c changes into their own patch
-Use a static list of strings to match
-Fixed typo of "whitelist"
-Dropped incorrect thermal zones
-Dropped tags from Bjorn and Lee since the functional should be
identical, but the code is structured different
v4:
-Changed the hid-quirks ELAN handling around per Benjamin Tissoires
-Dropped new DT binding
v3:
-Changed "clam" to "clamshell"
-Defined a dt binding for the combo Elan keyboard + touchpad device
-Adjusted the HID quirk to be correct for dt boot
-Removed extranious comment in board dts
-Fixed board level compatible
v2:
-Changed "cls" to "clam" since feedback indicated "cls" is too opaque,
but
"clamshell" is a mouthfull. "clam" seems to be a happy medium.
Jeffrey Hugo (3):
Input: elan_i2c: Add comment about link between elan_i2c and
hid-quirks
HID: quirks: Refactor ELAN 400 and 401 handling
arm64: dts: qcom: Add Lenovo Miix 630
arch/arm64/boot/dts/qcom/Makefile | 1 +
.../boot/dts/qcom/msm8998-clamshell.dtsi | 240 ++++++++++++++++++
.../boot/dts/qcom/msm8998-lenovo-miix-630.dts | 30 +++
drivers/hid/hid-quirks.c | 78 +++++-
drivers/input/mouse/elan_i2c_core.c | 4 +
5 files changed, 342 insertions(+), 11 deletions(-)
create mode 100644 arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
create mode 100644 arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
--
2.17.1
Elan_i2c and hid-quirks work in conjunction to decide which devices each
driver will handle. Document this link in elan_i2c as a reminder that
updates to elan_i2c need to be mirrored to hid-quirks.
Signed-off-by: Jeffrey Hugo <[email protected]>
---
drivers/input/mouse/elan_i2c_core.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 65cd325eabc3..4cb49925a8b4 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -1374,6 +1374,10 @@ static const struct i2c_device_id elan_id[] = {
};
MODULE_DEVICE_TABLE(i2c, elan_id);
+/*
+ * when these whitelists get updated, the corresponding blacklist in hid-quirks
+ * needs to be updated to match.
+ */
#ifdef CONFIG_ACPI
static const struct acpi_device_id elan_acpi_id[] = {
{ "ELAN0000", 0 },
--
2.17.1
There needs to be coordination between hid-quirks and the elan_i2c driver
about which devices are handled by what drivers. Currently, both use
whitelists, which results in valid devices being unhandled by default,
when they should not be rejected by hid-quirks. This is quickly becoming
an issue.
Since elan_i2c has a maintained whitelist of what devices it will handle,
use that to implement a blacklist in hid-quirks so that only the devices
that need to be handled by elan_i2c get rejected by hid-quirks, and
everything else is handled by default. The downside is the whitelist and
blacklist need to be kept in sync.
Suggested-by: Benjamin Tissoires <[email protected]>
Signed-off-by: Jeffrey Hugo <[email protected]>
---
drivers/hid/hid-quirks.c | 78 ++++++++++++++++++++++++++++++++++------
1 file changed, 67 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
index e5ca6fe2ca57..edebd0700e3d 100644
--- a/drivers/hid/hid-quirks.c
+++ b/drivers/hid/hid-quirks.c
@@ -912,8 +912,66 @@ static const struct hid_device_id hid_mouse_ignore_list[] = {
{ }
};
+/*
+ * List of device names that elan_i2c is handling and HID should ignore. Must
+ * be kept in sync with elan_i2c
+ */
+static const char *hid_elan_i2c_ignore[] = {
+ "ELAN0000",
+ "ELAN0100",
+ "ELAN0600",
+ "ELAN0601",
+ "ELAN0602",
+ "ELAN0603",
+ "ELAN0604",
+ "ELAN0605",
+ "ELAN0606",
+ "ELAN0607",
+ "ELAN0608",
+ "ELAN0609",
+ "ELAN060B",
+ "ELAN060C",
+ "ELAN060F",
+ "ELAN0610",
+ "ELAN0611",
+ "ELAN0612",
+ "ELAN0613",
+ "ELAN0614",
+ "ELAN0615",
+ "ELAN0616",
+ "ELAN0617",
+ "ELAN0618",
+ "ELAN0619",
+ "ELAN061A",
+ "ELAN061B",
+ "ELAN061C",
+ "ELAN061D",
+ "ELAN061E",
+ "ELAN061F",
+ "ELAN0620",
+ "ELAN0621",
+ "ELAN0622",
+ "ELAN0623",
+ "ELAN0624",
+ "ELAN0625",
+ "ELAN0626",
+ "ELAN0627",
+ "ELAN0628",
+ "ELAN0629",
+ "ELAN062A",
+ "ELAN062B",
+ "ELAN062C",
+ "ELAN062D",
+ "ELAN0631",
+ "ELAN0632",
+ "ELAN1000",
+ "elan,ekth3000",
+ "" /* sentinel */
+};
bool hid_ignore(struct hid_device *hdev)
{
+ int i;
+
if (hdev->quirks & HID_QUIRK_NO_IGNORE)
return false;
if (hdev->quirks & HID_QUIRK_IGNORE)
@@ -978,18 +1036,16 @@ bool hid_ignore(struct hid_device *hdev)
break;
case USB_VENDOR_ID_ELAN:
/*
- * Many Elan devices have a product id of 0x0401 and are handled
- * by the elan_i2c input driver. But the ACPI HID ELAN0800 dev
- * is not (and cannot be) handled by that driver ->
- * Ignore all 0x0401 devs except for the ELAN0800 dev.
+ * Blacklist of everything that gets handled by the elan_i2c
+ * input driver. This should be kept in sync with the whitelist
+ * that exists in that driver. This avoids disabling valid
+ * touchpads and other ELAN devices.
*/
- if (hdev->product == 0x0401 &&
- strncmp(hdev->name, "ELAN0800", 8) != 0)
- return true;
- /* Same with product id 0x0400 */
- if (hdev->product == 0x0400 &&
- strncmp(hdev->name, "QTEC0001", 8) != 0)
- return true;
+ if ((hdev->product == 0x0401 || hdev->product == 0x0400))
+ for (i = 0; strlen(hid_elan_i2c_ignore[i]); ++i)
+ if (!strncmp(hdev->name, hid_elan_i2c_ignore[i],
+ strlen(hid_elan_i2c_ignore[i])))
+ return true;
break;
}
--
2.17.1
This adds the initial DT for the Lenovo Miix 630 laptop. Supported
functionality includes USB (host), microSD-card, keyboard, and trackpad.
Signed-off-by: Jeffrey Hugo <[email protected]>
---
arch/arm64/boot/dts/qcom/Makefile | 1 +
.../boot/dts/qcom/msm8998-clamshell.dtsi | 240 ++++++++++++++++++
.../boot/dts/qcom/msm8998-lenovo-miix-630.dts | 30 +++
3 files changed, 271 insertions(+)
create mode 100644 arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
create mode 100644 arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
index 21d548f02d39..c3e4307bcbd4 100644
--- a/arch/arm64/boot/dts/qcom/Makefile
+++ b/arch/arm64/boot/dts/qcom/Makefile
@@ -6,6 +6,7 @@ dtb-$(CONFIG_ARCH_QCOM) += msm8916-mtp.dtb
dtb-$(CONFIG_ARCH_QCOM) += msm8992-bullhead-rev-101.dtb
dtb-$(CONFIG_ARCH_QCOM) += msm8994-angler-rev-101.dtb
dtb-$(CONFIG_ARCH_QCOM) += msm8996-mtp.dtb
+dtb-$(CONFIG_ARCH_QCOM) += msm8998-lenovo-miix-630.dtb
dtb-$(CONFIG_ARCH_QCOM) += msm8998-mtp.dtb
dtb-$(CONFIG_ARCH_QCOM) += sdm845-mtp.dtb
dtb-$(CONFIG_ARCH_QCOM) += qcs404-evb-1000.dtb
diff --git a/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi b/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
new file mode 100644
index 000000000000..9682d4dd7496
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2019, Jeffrey Hugo. All rights reserved. */
+
+/*
+ * Common include for MSM8998 clamshell devices, ie the Lenovo Miix 630,
+ * Asus NovaGo TP370QL, and HP Envy x2. All three devices are basically the
+ * same, with differences in peripherals.
+ */
+
+#include "msm8998.dtsi"
+#include "pm8998.dtsi"
+#include "pm8005.dtsi"
+
+/ {
+ chosen {
+ };
+
+ vph_pwr: vph-pwr-regulator {
+ compatible = "regulator-fixed";
+ regulator-name = "vph_pwr";
+ regulator-always-on;
+ regulator-boot-on;
+ };
+};
+
+&qusb2phy {
+ status = "okay";
+
+ vdda-pll-supply = <&vreg_l12a_1p8>;
+ vdda-phy-dpdm-supply = <&vreg_l24a_3p075>;
+};
+
+&rpm_requests {
+ pm8998-regulators {
+ compatible = "qcom,rpm-pm8998-regulators";
+
+ vdd_s1-supply = <&vph_pwr>;
+ vdd_s2-supply = <&vph_pwr>;
+ vdd_s3-supply = <&vph_pwr>;
+ vdd_s4-supply = <&vph_pwr>;
+ vdd_s5-supply = <&vph_pwr>;
+ vdd_s6-supply = <&vph_pwr>;
+ vdd_s7-supply = <&vph_pwr>;
+ vdd_s8-supply = <&vph_pwr>;
+ vdd_s9-supply = <&vph_pwr>;
+ vdd_s10-supply = <&vph_pwr>;
+ vdd_s11-supply = <&vph_pwr>;
+ vdd_s12-supply = <&vph_pwr>;
+ vdd_s13-supply = <&vph_pwr>;
+ vdd_l1_l27-supply = <&vreg_s7a_1p025>;
+ vdd_l2_l8_l17-supply = <&vreg_s3a_1p35>;
+ vdd_l3_l11-supply = <&vreg_s7a_1p025>;
+ vdd_l4_l5-supply = <&vreg_s7a_1p025>;
+ vdd_l6-supply = <&vreg_s5a_2p04>;
+ vdd_l7_l12_l14_l15-supply = <&vreg_s5a_2p04>;
+ vdd_l9-supply = <&vph_pwr>;
+ vdd_l10_l23_l25-supply = <&vph_pwr>;
+ vdd_l13_l19_l21-supply = <&vph_pwr>;
+ vdd_l16_l28-supply = <&vph_pwr>;
+ vdd_l18_l22-supply = <&vph_pwr>;
+ vdd_l20_l24-supply = <&vph_pwr>;
+ vdd_l26-supply = <&vreg_s3a_1p35>;
+ vdd_lvs1_lvs2-supply = <&vreg_s4a_1p8>;
+
+ vreg_s3a_1p35: s3 {
+ regulator-min-microvolt = <1352000>;
+ regulator-max-microvolt = <1352000>;
+ };
+ vreg_s4a_1p8: s4 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ regulator-allow-set-load;
+ };
+ vreg_s5a_2p04: s5 {
+ regulator-min-microvolt = <1904000>;
+ regulator-max-microvolt = <2040000>;
+ };
+ vreg_s7a_1p025: s7 {
+ regulator-min-microvolt = <900000>;
+ regulator-max-microvolt = <1028000>;
+ };
+ vreg_l1a_0p875: l1 {
+ regulator-min-microvolt = <880000>;
+ regulator-max-microvolt = <880000>;
+ regulator-allow-set-load;
+ };
+ vreg_l2a_1p2: l2 {
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ regulator-allow-set-load;
+ };
+ vreg_l3a_1p0: l3 {
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1000000>;
+ };
+ vreg_l5a_0p8: l5 {
+ regulator-min-microvolt = <800000>;
+ regulator-max-microvolt = <800000>;
+ };
+ vreg_l6a_1p8: l6 {
+ regulator-min-microvolt = <1808000>;
+ regulator-max-microvolt = <1808000>;
+ };
+ vreg_l7a_1p8: l7 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ vreg_l8a_1p2: l8 {
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ };
+ vreg_l9a_1p8: l9 {
+ regulator-min-microvolt = <1808000>;
+ regulator-max-microvolt = <2960000>;
+ };
+ vreg_l10a_1p8: l10 {
+ regulator-min-microvolt = <1808000>;
+ regulator-max-microvolt = <2960000>;
+ };
+ vreg_l11a_1p0: l11 {
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1000000>;
+ };
+ vreg_l12a_1p8: l12 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ vreg_l13a_2p95: l13 {
+ regulator-min-microvolt = <1808000>;
+ regulator-max-microvolt = <2960000>;
+ };
+ vreg_l14a_1p88: l14 {
+ regulator-min-microvolt = <1880000>;
+ regulator-max-microvolt = <1880000>;
+ };
+ vreg_15a_1p8: l15 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+ vreg_l16a_2p7: l16 {
+ regulator-min-microvolt = <2704000>;
+ regulator-max-microvolt = <2704000>;
+ };
+ vreg_l17a_1p3: l17 {
+ regulator-min-microvolt = <1304000>;
+ regulator-max-microvolt = <1304000>;
+ };
+ vreg_l18a_2p7: l18 {
+ regulator-min-microvolt = <2704000>;
+ regulator-max-microvolt = <2704000>;
+ };
+ vreg_l19a_3p0: l19 {
+ regulator-min-microvolt = <3008000>;
+ regulator-max-microvolt = <3008000>;
+ };
+ vreg_l20a_2p95: l20 {
+ regulator-min-microvolt = <2960000>;
+ regulator-max-microvolt = <2960000>;
+ regulator-allow-set-load;
+ };
+ vreg_l21a_2p95: l21 {
+ regulator-min-microvolt = <2960000>;
+ regulator-max-microvolt = <2960000>;
+ regulator-allow-set-load;
+ regulator-system-load = <800000>;
+ };
+ vreg_l22a_2p85: l22 {
+ regulator-min-microvolt = <2864000>;
+ regulator-max-microvolt = <2864000>;
+ };
+ vreg_l23a_3p3: l23 {
+ regulator-min-microvolt = <3312000>;
+ regulator-max-microvolt = <3312000>;
+ };
+ vreg_l24a_3p075: l24 {
+ regulator-min-microvolt = <3088000>;
+ regulator-max-microvolt = <3088000>;
+ };
+ vreg_l25a_3p3: l25 {
+ regulator-min-microvolt = <3104000>;
+ regulator-max-microvolt = <3312000>;
+ };
+ vreg_l26a_1p2: l26 {
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ };
+ vreg_l28_3p0: l28 {
+ regulator-min-microvolt = <3008000>;
+ regulator-max-microvolt = <3008000>;
+ };
+
+ vreg_lvs1a_1p8: lvs1 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+
+ vreg_lvs2a_1p8: lvs2 {
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ };
+
+ };
+};
+
+&tlmm {
+ gpio-reserved-ranges = <0 4>, <81 4>;
+
+ touchpad: touchpad {
+ config {
+ pins = "gpio123";
+ bias-pull-up; /* pull up */
+ };
+ };
+};
+
+&sdhc2 {
+ status = "okay";
+
+ vmmc-supply = <&vreg_l21a_2p95>;
+ vqmmc-supply = <&vreg_l13a_2p95>;
+
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&sdc2_clk_on &sdc2_cmd_on &sdc2_data_on &sdc2_cd_on>;
+ pinctrl-1 = <&sdc2_clk_off &sdc2_cmd_off &sdc2_data_off &sdc2_cd_off>;
+};
+
+&usb3 {
+ status = "okay";
+};
+
+&usb3_dwc3 {
+ dr_mode = "host"; /* Force to host until we have Type-C hooked up */
+};
+
+&usb3phy {
+ status = "okay";
+
+ vdda-phy-supply = <&vreg_l1a_0p875>;
+ vdda-pll-supply = <&vreg_l2a_1p2>;
+};
diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
new file mode 100644
index 000000000000..407c6a32911c
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
@@ -0,0 +1,30 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2019, Jeffrey Hugo. All rights reserved. */
+
+/dts-v1/;
+
+#include "msm8998-clamshell.dtsi"
+
+/ {
+ model = "Lenovo Miix 630";
+ compatible = "lenovo,miix-630", "qcom,msm8998";
+};
+
+&blsp1_i2c6 {
+ status = "okay";
+
+ keyboard@3a {
+ compatible = "hid-over-i2c";
+ interrupt-parent = <&tlmm>;
+ interrupts = <0x79 IRQ_TYPE_LEVEL_LOW>;
+ reg = <0x3a>;
+ hid-descr-addr = <0x0001>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&touchpad>;
+ };
+};
+
+&sdhc2 {
+ cd-gpios = <&tlmm 95 GPIO_ACTIVE_HIGH>;
+};
--
2.17.1
On Thu, Jun 06, 2019 at 09:13:22AM -0700, Jeffrey Hugo wrote:
> There needs to be coordination between hid-quirks and the elan_i2c driver
> about which devices are handled by what drivers. Currently, both use
> whitelists, which results in valid devices being unhandled by default,
> when they should not be rejected by hid-quirks. This is quickly becoming
> an issue.
>
> Since elan_i2c has a maintained whitelist of what devices it will handle,
> use that to implement a blacklist in hid-quirks so that only the devices
> that need to be handled by elan_i2c get rejected by hid-quirks, and
> everything else is handled by default. The downside is the whitelist and
> blacklist need to be kept in sync.
>
> Suggested-by: Benjamin Tissoires <[email protected]>
> Signed-off-by: Jeffrey Hugo <[email protected]>
> ---
> drivers/hid/hid-quirks.c | 78 ++++++++++++++++++++++++++++++++++------
> 1 file changed, 67 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> index e5ca6fe2ca57..edebd0700e3d 100644
> --- a/drivers/hid/hid-quirks.c
> +++ b/drivers/hid/hid-quirks.c
> @@ -912,8 +912,66 @@ static const struct hid_device_id hid_mouse_ignore_list[] = {
> { }
> };
>
> +/*
> + * List of device names that elan_i2c is handling and HID should ignore. Must
> + * be kept in sync with elan_i2c
> + */
> +static const char *hid_elan_i2c_ignore[] = {
If this is a copy of elan whitelist, then, if we do not want to bother
with sharing it in object form (as a elan-i2c-ids module), can we at
least move it into include/linux/input/elan-i2c-ids.h and consume from
hid-quirks.c?
Thanks.
--
Dmitry
On 6/11/2019 6:35 PM, Dmitry Torokhov wrote:
> On Thu, Jun 06, 2019 at 09:13:22AM -0700, Jeffrey Hugo wrote:
>> There needs to be coordination between hid-quirks and the elan_i2c driver
>> about which devices are handled by what drivers. Currently, both use
>> whitelists, which results in valid devices being unhandled by default,
>> when they should not be rejected by hid-quirks. This is quickly becoming
>> an issue.
>>
>> Since elan_i2c has a maintained whitelist of what devices it will handle,
>> use that to implement a blacklist in hid-quirks so that only the devices
>> that need to be handled by elan_i2c get rejected by hid-quirks, and
>> everything else is handled by default. The downside is the whitelist and
>> blacklist need to be kept in sync.
>>
>> Suggested-by: Benjamin Tissoires <[email protected]>
>> Signed-off-by: Jeffrey Hugo <[email protected]>
>> ---
>> drivers/hid/hid-quirks.c | 78 ++++++++++++++++++++++++++++++++++------
>> 1 file changed, 67 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
>> index e5ca6fe2ca57..edebd0700e3d 100644
>> --- a/drivers/hid/hid-quirks.c
>> +++ b/drivers/hid/hid-quirks.c
>> @@ -912,8 +912,66 @@ static const struct hid_device_id hid_mouse_ignore_list[] = {
>> { }
>> };
>>
>> +/*
>> + * List of device names that elan_i2c is handling and HID should ignore. Must
>> + * be kept in sync with elan_i2c
>> + */
>> +static const char *hid_elan_i2c_ignore[] = {
>
> If this is a copy of elan whitelist, then, if we do not want to bother
> with sharing it in object form (as a elan-i2c-ids module), can we at
> least move it into include/linux/input/elan-i2c-ids.h and consume from
> hid-quirks.c?
I can put it in a shared header file, however elan-i2c and hid-quirks
would need to be updated in the same change to prevent a breakage, but
that would seem to violate a concern Benjamin brought up in v4 given
that elan-i2c is maintained in your input tree, and hid-quirks is
maintained in his hid tree.
Are you ok with the elan-i2c changes going through Benjamin's hid tree?
On Tue, 11 Jun 2019, Dmitry Torokhov wrote:
> > +static const char *hid_elan_i2c_ignore[] = {
>
> If this is a copy of elan whitelist, then, if we do not want to bother
> with sharing it in object form (as a elan-i2c-ids module), can we at
> least move it into include/linux/input/elan-i2c-ids.h and consume from
> hid-quirks.c?
Let's just not duplicate it in both objects. Why not properly export it
from hid_quirks?
Thanks,
--
Jiri Kosina
SUSE Labs
On 6/12/2019 8:37 AM, Lee Jones wrote:
> On Wed, 12 Jun 2019, Jeffrey Hugo wrote:
>
>> On 6/11/2019 6:35 PM, Dmitry Torokhov wrote:
>>> On Thu, Jun 06, 2019 at 09:13:22AM -0700, Jeffrey Hugo wrote:
>>>> There needs to be coordination between hid-quirks and the elan_i2c driver
>>>> about which devices are handled by what drivers. Currently, both use
>>>> whitelists, which results in valid devices being unhandled by default,
>>>> when they should not be rejected by hid-quirks. This is quickly becoming
>>>> an issue.
>>>>
>>>> Since elan_i2c has a maintained whitelist of what devices it will handle,
>>>> use that to implement a blacklist in hid-quirks so that only the devices
>>>> that need to be handled by elan_i2c get rejected by hid-quirks, and
>>>> everything else is handled by default. The downside is the whitelist and
>>>> blacklist need to be kept in sync.
>>>>
>>>> Suggested-by: Benjamin Tissoires <[email protected]>
>>>> Signed-off-by: Jeffrey Hugo <[email protected]>
>>>> ---
>>>> drivers/hid/hid-quirks.c | 78 ++++++++++++++++++++++++++++++++++------
>>>> 1 file changed, 67 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
>>>> index e5ca6fe2ca57..edebd0700e3d 100644
>>>> --- a/drivers/hid/hid-quirks.c
>>>> +++ b/drivers/hid/hid-quirks.c
>>>> @@ -912,8 +912,66 @@ static const struct hid_device_id hid_mouse_ignore_list[] = {
>>>> { }
>>>> };
>>>> +/*
>>>> + * List of device names that elan_i2c is handling and HID should ignore. Must
>>>> + * be kept in sync with elan_i2c
>>>> + */
>>>> +static const char *hid_elan_i2c_ignore[] = {
>>>
>>> If this is a copy of elan whitelist, then, if we do not want to bother
>>> with sharing it in object form (as a elan-i2c-ids module), can we at
>>> least move it into include/linux/input/elan-i2c-ids.h and consume from
>>> hid-quirks.c?
>>
>> I can put it in a shared header file, however elan-i2c and hid-quirks
>> would need to be updated in the same change to prevent a breakage, but
>> that would seem to violate a concern Benjamin brought up in v4 given
>> that elan-i2c is maintained in your input tree, and hid-quirks is
>> maintained in his hid tree.
>>
>> Are you ok with the elan-i2c changes going through Benjamin's hid tree?
>
> We co-ordinate cross-subsystem merges all the time. That is never a
> reason to not do the 'right thing (tm)'. If this information can be
> held in a single, central place, without the need for constant
> re-alignment, I'm all for it.
>
I'm aware coordination is a thing. I'm trying to elicit some
coordination between Dmitry and Benjamin.
On Wed, 12 Jun 2019, Jeffrey Hugo wrote:
> On 6/11/2019 6:35 PM, Dmitry Torokhov wrote:
> > On Thu, Jun 06, 2019 at 09:13:22AM -0700, Jeffrey Hugo wrote:
> > > There needs to be coordination between hid-quirks and the elan_i2c driver
> > > about which devices are handled by what drivers. Currently, both use
> > > whitelists, which results in valid devices being unhandled by default,
> > > when they should not be rejected by hid-quirks. This is quickly becoming
> > > an issue.
> > >
> > > Since elan_i2c has a maintained whitelist of what devices it will handle,
> > > use that to implement a blacklist in hid-quirks so that only the devices
> > > that need to be handled by elan_i2c get rejected by hid-quirks, and
> > > everything else is handled by default. The downside is the whitelist and
> > > blacklist need to be kept in sync.
> > >
> > > Suggested-by: Benjamin Tissoires <[email protected]>
> > > Signed-off-by: Jeffrey Hugo <[email protected]>
> > > ---
> > > drivers/hid/hid-quirks.c | 78 ++++++++++++++++++++++++++++++++++------
> > > 1 file changed, 67 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c
> > > index e5ca6fe2ca57..edebd0700e3d 100644
> > > --- a/drivers/hid/hid-quirks.c
> > > +++ b/drivers/hid/hid-quirks.c
> > > @@ -912,8 +912,66 @@ static const struct hid_device_id hid_mouse_ignore_list[] = {
> > > { }
> > > };
> > > +/*
> > > + * List of device names that elan_i2c is handling and HID should ignore. Must
> > > + * be kept in sync with elan_i2c
> > > + */
> > > +static const char *hid_elan_i2c_ignore[] = {
> >
> > If this is a copy of elan whitelist, then, if we do not want to bother
> > with sharing it in object form (as a elan-i2c-ids module), can we at
> > least move it into include/linux/input/elan-i2c-ids.h and consume from
> > hid-quirks.c?
>
> I can put it in a shared header file, however elan-i2c and hid-quirks
> would need to be updated in the same change to prevent a breakage, but
> that would seem to violate a concern Benjamin brought up in v4 given
> that elan-i2c is maintained in your input tree, and hid-quirks is
> maintained in his hid tree.
>
> Are you ok with the elan-i2c changes going through Benjamin's hid tree?
We co-ordinate cross-subsystem merges all the time. That is never a
reason to not do the 'right thing (tm)'. If this information can be
held in a single, central place, without the need for constant
re-alignment, I'm all for it.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Jun 12, 2019 at 7:45 AM Jiri Kosina <[email protected]> wrote:
>
> On Tue, 11 Jun 2019, Dmitry Torokhov wrote:
>
> > > +static const char *hid_elan_i2c_ignore[] = {
> >
> > If this is a copy of elan whitelist, then, if we do not want to bother
> > with sharing it in object form (as a elan-i2c-ids module), can we at
> > least move it into include/linux/input/elan-i2c-ids.h and consume from
> > hid-quirks.c?
>
> Let's just not duplicate it in both objects. Why not properly export it
> from hid_quirks?
Strictly speaking Elan does not depend on HID; exporting it from
quirks would mean adding this dependency. This also mean that you
can't make Elan built-in while keeping HID as a module (I think this
at least used to be config on some Chromebooks).
Thanks.
--
Dmitry
On Wed, Jun 12, 2019 at 5:14 PM Dmitry Torokhov
<[email protected]> wrote:
>
> On Wed, Jun 12, 2019 at 7:45 AM Jiri Kosina <[email protected]> wrote:
> >
> > On Tue, 11 Jun 2019, Dmitry Torokhov wrote:
> >
> > > > +static const char *hid_elan_i2c_ignore[] = {
> > >
> > > If this is a copy of elan whitelist, then, if we do not want to bother
> > > with sharing it in object form (as a elan-i2c-ids module), can we at
> > > least move it into include/linux/input/elan-i2c-ids.h and consume from
> > > hid-quirks.c?
> >
> > Let's just not duplicate it in both objects. Why not properly export it
> > from hid_quirks?
>
> Strictly speaking Elan does not depend on HID; exporting it from
> quirks would mean adding this dependency. This also mean that you
> can't make Elan built-in while keeping HID as a module (I think this
> at least used to be config on some Chromebooks).
>
I also think it would me things cleaner to have the list of devices in elan_i2c.
If we put the list of devices supported by elan_i2c in a header, and
have HID read this .h file directly, there will be no runtime
dependency.
I am sure we can work something out to remove Jeffrey's fears.
Cheers,
Benjamin
On 6/12/2019 9:21 AM, Benjamin Tissoires wrote:
> On Wed, Jun 12, 2019 at 5:14 PM Dmitry Torokhov
> <[email protected]> wrote:
>>
>> On Wed, Jun 12, 2019 at 7:45 AM Jiri Kosina <[email protected]> wrote:
>>>
>>> On Tue, 11 Jun 2019, Dmitry Torokhov wrote:
>>>
>>>>> +static const char *hid_elan_i2c_ignore[] = {
>>>>
>>>> If this is a copy of elan whitelist, then, if we do not want to bother
>>>> with sharing it in object form (as a elan-i2c-ids module), can we at
>>>> least move it into include/linux/input/elan-i2c-ids.h and consume from
>>>> hid-quirks.c?
>>>
>>> Let's just not duplicate it in both objects. Why not properly export it
>>> from hid_quirks?
>>
>> Strictly speaking Elan does not depend on HID; exporting it from
>> quirks would mean adding this dependency. This also mean that you
>> can't make Elan built-in while keeping HID as a module (I think this
>> at least used to be config on some Chromebooks).
>>
>
> I also think it would me things cleaner to have the list of devices in elan_i2c.
> If we put the list of devices supported by elan_i2c in a header, and
> have HID read this .h file directly, there will be no runtime
> dependency.
>
> I am sure we can work something out to remove Jeffrey's fears.
Works for me. I plan to have the next rev posted by the end of the week.