Message ID | 20190415160915.16324-1-jeffrey.l.hugo@gmail.com |
---|---|
Headers | show |
Series | Basic DT support for Lenovo Miix 630 | expand |
On Mon, Apr 15, 2019 at 6:11 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote: > > Following up on commit 2bafa1e96254 ("HID: quirks: Fix keyboard + touchpad > on Lenovo Miix 630"), the devicetree (DT) identifier for the combo keyboard > + touchpad device is "elan,combo400-i2c", which differs from the ACPI ID, > thus if we want the quirk to work properly when booting via DT instead of > ACPI, we need to key off the DT id as well. > > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > --- > drivers/hid/hid-quirks.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c > index 77ffba48cc73..00c08f8318b8 100644 > --- a/drivers/hid/hid-quirks.c > +++ b/drivers/hid/hid-quirks.c > @@ -997,7 +997,8 @@ bool hid_ignore(struct hid_device *hdev) > return true; > /* Same with product id 0x0400 */ > if (hdev->product == 0x0400 && > - strncmp(hdev->name, "QTEC0001", 8) != 0) > + (strncmp(hdev->name, "QTEC0001", 8) != 0 || > + strncmp(hdev->name, "elan,combo400-i2c", 17) != 0)) I think we are taking the problem the wrong way here. When I first introduced 6ccfe64, I thought 0x0400 would be reserved for the elan_i2c touchpads only. But it turns out we are deliberately disabling valid HID touchpads hoping that they would be picked up by elan_i2c when elan_i2c has its own whitelist of devices. How about we turn this into list with the matching ones from elan_i2c: if ((hdev->product == 0x0400 || hdev->product == 0x0401) && (strncmp(hdev->name, "ELAN0000", 8) == 0 || strncmp(hdev->name, "ELAN0100", 8) == 0 || ... strncmp(hdev->name, "ELAN1000", 8) == 0)) return true; So next time we need to force binding a HID touchpad to elan_i2c, we can just blacklist here and whitelist it in elan_i2c. Cheers, Benjamin > return true; > break; > } > -- > 2.17.1 >
Hi, On 18-04-19 11:34, Benjamin Tissoires wrote: > On Mon, Apr 15, 2019 at 6:11 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote: >> >> Following up on commit 2bafa1e96254 ("HID: quirks: Fix keyboard + touchpad >> on Lenovo Miix 630"), the devicetree (DT) identifier for the combo keyboard >> + touchpad device is "elan,combo400-i2c", which differs from the ACPI ID, >> thus if we want the quirk to work properly when booting via DT instead of >> ACPI, we need to key off the DT id as well. >> >> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> >> --- >> drivers/hid/hid-quirks.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c >> index 77ffba48cc73..00c08f8318b8 100644 >> --- a/drivers/hid/hid-quirks.c >> +++ b/drivers/hid/hid-quirks.c >> @@ -997,7 +997,8 @@ bool hid_ignore(struct hid_device *hdev) >> return true; >> /* Same with product id 0x0400 */ >> if (hdev->product == 0x0400 && >> - strncmp(hdev->name, "QTEC0001", 8) != 0) >> + (strncmp(hdev->name, "QTEC0001", 8) != 0 || >> + strncmp(hdev->name, "elan,combo400-i2c", 17) != 0)) > > I think we are taking the problem the wrong way here. > > When I first introduced 6ccfe64, I thought 0x0400 would be reserved > for the elan_i2c touchpads only. But it turns out we are deliberately > disabling valid HID touchpads hoping that they would be picked up by > elan_i2c when elan_i2c has its own whitelist of devices. > > How about we turn this into list with the matching ones from elan_i2c: > if ((hdev->product == 0x0400 || hdev->product == 0x0401) && > (strncmp(hdev->name, "ELAN0000", 8) == 0 || > strncmp(hdev->name, "ELAN0100", 8) == 0 || > ... > strncmp(hdev->name, "ELAN1000", 8) == 0)) > return true; > > So next time we need to force binding a HID touchpad to elan_i2c, we > can just blacklist here and whitelist it in elan_i2c. This indeed sounds like a better way forward with this. Regards, Hans
On Thu, Apr 18, 2019 at 3:51 AM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 18-04-19 11:34, Benjamin Tissoires wrote: > > On Mon, Apr 15, 2019 at 6:11 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote: > >> > >> Following up on commit 2bafa1e96254 ("HID: quirks: Fix keyboard + touchpad > >> on Lenovo Miix 630"), the devicetree (DT) identifier for the combo keyboard > >> + touchpad device is "elan,combo400-i2c", which differs from the ACPI ID, > >> thus if we want the quirk to work properly when booting via DT instead of > >> ACPI, we need to key off the DT id as well. > >> > >> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > >> --- > >> drivers/hid/hid-quirks.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/hid/hid-quirks.c b/drivers/hid/hid-quirks.c > >> index 77ffba48cc73..00c08f8318b8 100644 > >> --- a/drivers/hid/hid-quirks.c > >> +++ b/drivers/hid/hid-quirks.c > >> @@ -997,7 +997,8 @@ bool hid_ignore(struct hid_device *hdev) > >> return true; > >> /* Same with product id 0x0400 */ > >> if (hdev->product == 0x0400 && > >> - strncmp(hdev->name, "QTEC0001", 8) != 0) > >> + (strncmp(hdev->name, "QTEC0001", 8) != 0 || > >> + strncmp(hdev->name, "elan,combo400-i2c", 17) != 0)) > > > > I think we are taking the problem the wrong way here. > > > > When I first introduced 6ccfe64, I thought 0x0400 would be reserved > > for the elan_i2c touchpads only. But it turns out we are deliberately > > disabling valid HID touchpads hoping that they would be picked up by > > elan_i2c when elan_i2c has its own whitelist of devices. > > > > How about we turn this into list with the matching ones from elan_i2c: > > if ((hdev->product == 0x0400 || hdev->product == 0x0401) && > > (strncmp(hdev->name, "ELAN0000", 8) == 0 || > > strncmp(hdev->name, "ELAN0100", 8) == 0 || > > ... > > strncmp(hdev->name, "ELAN1000", 8) == 0)) > > return true; > > > > So next time we need to force binding a HID touchpad to elan_i2c, we > > can just blacklist here and whitelist it in elan_i2c. > > This indeed sounds like a better way forward with this. This sounds good to me. Let me implement it and test with the Miix 630. Thanks for the suggestion.
On Mon 15 Apr 09:11 PDT 2019, Jeffrey Hugo wrote: > diff --git a/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi b/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi [..] > + thermal-zones { > + battery-thermal { > + polling-delay-passive = <250>; > + polling-delay = <1000>; > + > + thermal-sensors = <&tsens0 0>; I guess you inherited the battery and skin thermal nodes from my MTP dts. Unfortunately after talking to Amit I think I got these wrong, and they should be &pmi8998_adc 0 and 5 instead. Can you confirm this? > + > + trips { > + battery_crit: trip0 { > + temperature = <60000>; > + hysteresis = <2000>; > + type = "critical"; > + }; > + }; > + }; Regards, Bjorn
On Fri, Apr 26, 2019 at 10:42 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Mon 15 Apr 09:11 PDT 2019, Jeffrey Hugo wrote: > > diff --git a/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi b/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi > [..] > > + thermal-zones { > > + battery-thermal { > > + polling-delay-passive = <250>; > > + polling-delay = <1000>; > > + > > + thermal-sensors = <&tsens0 0>; > > I guess you inherited the battery and skin thermal nodes from my MTP > dts. Unfortunately after talking to Amit I think I got these wrong, and > they should be &pmi8998_adc 0 and 5 instead. > > Can you confirm this? Yeah, I pulled thermal from the MTP. Someone pointed this out on the v4 series. I haven't circled back to it yet. > > > + > > + trips { > > + battery_crit: trip0 { > > + temperature = <60000>; > > + hysteresis = <2000>; > > + type = "critical"; > > + }; > > + }; > > + }; > > Regards, > Bjorn