[PATCH 1/3] HID: asus: Cleanup Asus T101HA keyboard-dock handling

[PATCH 1/3] HID: asus: Cleanup Asus T101HA keyboard-dock handling

[Hans de Goede]

There is no need to use a quirk and then return -ENODEV from the
asus_probe() function to avoid that hid-asus binds to the hiddev
for the USB-interface for the hid-multitouch touchpad.

The hid-multitouch hiddev has a group of HID_GROUP_MULTITOUCH_WIN_8,
so the same result can be achieved by making the hid_device_id entry
for the dock in the asus_devices[] table only match on HID_GROUP_GENERIC
instead of having it match HID_GROUP_ANY.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-asus.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 1dfe184ebf5a..b892aea673f9 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -79,10 +79,9 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define QUIRK_T100_KEYBOARD		BIT(6)
 #define QUIRK_T100CHI			BIT(7)
 #define QUIRK_G752_KEYBOARD		BIT(8)
-#define QUIRK_T101HA_DOCK		BIT(9)
-#define QUIRK_T90CHI			BIT(10)
-#define QUIRK_MEDION_E1239T		BIT(11)
-#define QUIRK_ROG_NKEY_KEYBOARD		BIT(12)
+#define QUIRK_T90CHI			BIT(9)
+#define QUIRK_MEDION_E1239T		BIT(10)
+#define QUIRK_ROG_NKEY_KEYBOARD		BIT(11)
 
 #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
 						 QUIRK_NO_INIT_REPORTS | \
@@ -1072,11 +1071,6 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
-	/* use hid-multitouch for T101HA touchpad */
-	if (id->driver_data & QUIRK_T101HA_DOCK &&
-	    hdev->collection->usage == HID_GD_MOUSE)
-		return -ENODEV;
-
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret) {
 		hid_err(hdev, "Asus hw start failed: %d\n", ret);
@@ -1227,8 +1221,6 @@ static const struct hid_device_id asus_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 		USB_DEVICE_ID_ASUSTEK_T100TAF_KEYBOARD),
 	  QUIRK_T100_KEYBOARD | QUIRK_NO_CONSUMER_USAGES },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
-		USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD), QUIRK_T101HA_DOCK },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_ASUS_AK1D) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_TURBOX, USB_DEVICE_ID_ASUS_MD_5110) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_JESS, USB_DEVICE_ID_ASUS_MD_5112) },
@@ -1236,6 +1228,12 @@ static const struct hid_device_id asus_devices[] = {
 		USB_DEVICE_ID_ASUSTEK_T100CHI_KEYBOARD), QUIRK_T100CHI },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ITE, USB_DEVICE_ID_ITE_MEDION_E1239T),
 		QUIRK_MEDION_E1239T },
+	/*
+	 * Note bind to the HID_GROUP_GENERIC group, so that we only bind to the keyboard
+	 * part, while letting hid-multitouch.c handle the touchpad.
+	 */
+	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
+		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) },
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, asus_devices);
-- 
2.30.1

[PATCH 2/3] HID: multitouch: Disable event reporting on suspend on the Asus T101HA touchpad

[Hans de Goede]

The Asus T101HA has a problem with spurious wakeups when the lid is
closed, this is caused by the screen sitting so close to the touchpad
that the touchpad ends up reporting touch events, causing these wakeups.

Add a quirk which disables event reporting on suspend when set, and
enable this quirk for the Asus T101HA touchpad fixing the spurious
wakeups, while still allowing the device to be woken by pressing a
key on the keyboard (which is part of the same USB device).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-multitouch.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 9d9f3e1bd5f4..cfb68e443ddd 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -70,6 +70,7 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_WIN8_PTP_BUTTONS	BIT(18)
 #define MT_QUIRK_SEPARATE_APP_REPORT	BIT(19)
 #define MT_QUIRK_FORCE_MULTI_INPUT	BIT(20)
+#define MT_QUIRK_DISABLE_WAKEUP		BIT(21)
 
 #define MT_INPUTMODE_TOUCHSCREEN	0x02
 #define MT_INPUTMODE_TOUCHPAD		0x03
@@ -191,6 +192,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app);
 #define MT_CLS_EXPORT_ALL_INPUTS		0x0013
 /* reserved					0x0014 */
 #define MT_CLS_WIN_8_FORCE_MULTI_INPUT		0x0015
+#define MT_CLS_WIN_8_DISABLE_WAKEUP		0x0016
 
 /* vendor specific classes */
 #define MT_CLS_3M				0x0101
@@ -283,6 +285,15 @@ static const struct mt_class mt_classes[] = {
 			MT_QUIRK_WIN8_PTP_BUTTONS |
 			MT_QUIRK_FORCE_MULTI_INPUT,
 		.export_all_inputs = true },
+	{ .name = MT_CLS_WIN_8_DISABLE_WAKEUP,
+		.quirks = MT_QUIRK_ALWAYS_VALID |
+			MT_QUIRK_IGNORE_DUPLICATES |
+			MT_QUIRK_HOVERING |
+			MT_QUIRK_CONTACT_CNT_ACCURATE |
+			MT_QUIRK_STICKY_FINGERS |
+			MT_QUIRK_WIN8_PTP_BUTTONS |
+			MT_QUIRK_DISABLE_WAKEUP,
+		.export_all_inputs = true },
 
 	/*
 	 * vendor specific classes
@@ -759,7 +770,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			return 1;
 		case HID_DG_CONFIDENCE:
 			if ((cls->name == MT_CLS_WIN_8 ||
-			     cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT) &&
+			     cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT ||
+			     cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP) &&
 				(field->application == HID_DG_TOUCHPAD ||
 				 field->application == HID_DG_TOUCHSCREEN))
 				app->quirks |= MT_QUIRK_CONFIDENCE;
@@ -1749,8 +1761,14 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 #ifdef CONFIG_PM
 static int mt_suspend(struct hid_device *hdev, pm_message_t state)
 {
+	struct mt_device *td = hid_get_drvdata(hdev);
+
 	/* High latency is desirable for power savings during S3/S0ix */
-	mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
+	if (td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP)
+		mt_set_modes(hdev, HID_LATENCY_HIGH, false, false);
+	else
+		mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
+
 	return 0;
 }
 
@@ -1809,6 +1827,12 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_ANTON,
 			USB_DEVICE_ID_ANTON_TOUCH_PAD) },
 
+	/* Asus T101HA */
+	{ .driver_data = MT_CLS_WIN_8_DISABLE_WAKEUP,
+		HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH_WIN_8,
+			   USB_VENDOR_ID_ASUSTEK,
+			   USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) },
+
 	/* Asus T304UA */
 	{ .driver_data = MT_CLS_ASUS,
 		HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH_WIN_8,
-- 
2.30.1

[PATCH 3/3] HID: multitouch: Disable event reporting on suspend when our parent is not a wakeup-source

[Hans de Goede]

Disable event reporting on suspend when our parent is not
a wakeup-source. This should help save some extra power in
this case.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/Kconfig          |  2 +-
 drivers/hid/hid-multitouch.c | 23 ++++++++++++++++++++++-
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 786b71ef7738..5cbe4adfd816 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -675,7 +675,7 @@ config HID_MONTEREY
 
 config HID_MULTITOUCH
 	tristate "HID Multitouch panels"
-	depends on HID
+	depends on USB_HID
 	help
 	  Generic support for HID multitouch panels.
 
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index cfb68e443ddd..7926295bab81 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1759,12 +1759,33 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 }
 
 #ifdef CONFIG_PM
+
+/* Check if the parent which has the power/wakeup* sysfs attributes may wake the hdev */
+static bool mt_parent_may_wake(struct hid_device *hdev)
+{
+	struct device *parent = hdev->dev.parent;
+
+	/*
+	 * USB-HID is attached to the usb_interface (our parent), the
+	 * power/wakeup* attr are part of the usb-device which is its parent.
+	 */
+	if (hid_is_using_ll_driver(hdev, &usb_hid_driver) && parent)
+		parent = parent->parent;
+
+	if (parent)
+		return device_may_wakeup(parent);
+
+	/* Huh? Play it safe and keep reporting events. */
+	return true;
+}
+
 static int mt_suspend(struct hid_device *hdev, pm_message_t state)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
 
 	/* High latency is desirable for power savings during S3/S0ix */
-	if (td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP)
+	if ((td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP) ||
+	    !mt_parent_may_wake(hdev))
 		mt_set_modes(hdev, HID_LATENCY_HIGH, false, false);
 	else
 		mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
-- 
2.30.1

Re: [PATCH 1/3] HID: asus: Cleanup Asus T101HA keyboard-dock handling

[Jiri Kosina]

On Sat, 6 Mar 2021, Hans de Goede wrote:

> There is no need to use a quirk and then return -ENODEV from the
> asus_probe() function to avoid that hid-asus binds to the hiddev
> for the USB-interface for the hid-multitouch touchpad.
> 
> The hid-multitouch hiddev has a group of HID_GROUP_MULTITOUCH_WIN_8,
> so the same result can be achieved by making the hid_device_id entry
> for the dock in the asus_devices[] table only match on HID_GROUP_GENERIC
> instead of having it match HID_GROUP_ANY.

Benjamin, could you please Ack this series, as it touches hid-multitouch, 
please?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/3] HID: asus: Cleanup Asus T101HA keyboard-dock handling

[Jiri Kosina]

On Wed, 31 Mar 2021, Jiri Kosina wrote:

> > There is no need to use a quirk and then return -ENODEV from the
> > asus_probe() function to avoid that hid-asus binds to the hiddev
> > for the USB-interface for the hid-multitouch touchpad.
> > 
> > The hid-multitouch hiddev has a group of HID_GROUP_MULTITOUCH_WIN_8,
> > so the same result can be achieved by making the hid_device_id entry
> > for the dock in the asus_devices[] table only match on HID_GROUP_GENERIC
> > instead of having it match HID_GROUP_ANY.
> 
> Benjamin, could you please Ack this series, as it touches hid-multitouch, 
> please?

Benjamin, friendly ping on this one.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/3] HID: asus: Cleanup Asus T101HA keyboard-dock handling

[Benjamin Tissoires]

On Wed, May 5, 2021 at 2:24 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Wed, 31 Mar 2021, Jiri Kosina wrote:
>
> > > There is no need to use a quirk and then return -ENODEV from the
> > > asus_probe() function to avoid that hid-asus binds to the hiddev
> > > for the USB-interface for the hid-multitouch touchpad.
> > >
> > > The hid-multitouch hiddev has a group of HID_GROUP_MULTITOUCH_WIN_8,
> > > so the same result can be achieved by making the hid_device_id entry
> > > for the dock in the asus_devices[] table only match on HID_GROUP_GENERIC
> > > instead of having it match HID_GROUP_ANY.
> >
> > Benjamin, could you please Ack this series, as it touches hid-multitouch,
> > please?
>
> Benjamin, friendly ping on this one.
>

Sorry for being such a bad co-maintainer... :(

This one completely fell through the cracks.

I have no objections for 1/3 and 2/3. I'll comment on 3/3.

Cheers,
Benjamin

Re: [PATCH 3/3] HID: multitouch: Disable event reporting on suspend when our parent is not a wakeup-source

[Benjamin Tissoires]

Hi Hans,

On Sat, Mar 6, 2021 at 2:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Disable event reporting on suspend when our parent is not
> a wakeup-source. This should help save some extra power in
> this case.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hid/Kconfig          |  2 +-
>  drivers/hid/hid-multitouch.c | 23 ++++++++++++++++++++++-
>  2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 786b71ef7738..5cbe4adfd816 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -675,7 +675,7 @@ config HID_MONTEREY
>
>  config HID_MULTITOUCH
>         tristate "HID Multitouch panels"
> -       depends on HID
> +       depends on USB_HID

I tried really hard during the past 8 years to not have a usbhid
dependency on hid-multitouch.

The code below should not break the test suite, but still I am not
that happy about the Kconfig change.

I don't see an immediate and better way of doing what you are
achieving here, but maybe you have some magic I did not think about
that would help to no pull USB_HID with HID_MULTITOUCH.

FTR, I think the use case of hid-multitouch *without* USB is rather
non-existent, but there might be some weird systems with I2C only
(edge computing?).

Cheers,
Benjamin

>         help
>           Generic support for HID multitouch panels.
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index cfb68e443ddd..7926295bab81 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -1759,12 +1759,33 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  }
>
>  #ifdef CONFIG_PM
> +
> +/* Check if the parent which has the power/wakeup* sysfs attributes may wake the hdev */
> +static bool mt_parent_may_wake(struct hid_device *hdev)
> +{
> +       struct device *parent = hdev->dev.parent;
> +
> +       /*
> +        * USB-HID is attached to the usb_interface (our parent), the
> +        * power/wakeup* attr are part of the usb-device which is its parent.
> +        */
> +       if (hid_is_using_ll_driver(hdev, &usb_hid_driver) && parent)
> +               parent = parent->parent;
> +
> +       if (parent)
> +               return device_may_wakeup(parent);
> +
> +       /* Huh? Play it safe and keep reporting events. */
> +       return true;
> +}
> +
>  static int mt_suspend(struct hid_device *hdev, pm_message_t state)
>  {
>         struct mt_device *td = hid_get_drvdata(hdev);
>
>         /* High latency is desirable for power savings during S3/S0ix */
> -       if (td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP)
> +       if ((td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP) ||
> +           !mt_parent_may_wake(hdev))
>                 mt_set_modes(hdev, HID_LATENCY_HIGH, false, false);
>         else
>                 mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
> --
> 2.30.1
>

Re: [PATCH 3/3] HID: multitouch: Disable event reporting on suspend when our parent is not a wakeup-source

[Hans de Goede]

Hi,

On 5/5/21 3:40 PM, Benjamin Tissoires wrote:
> Hi Hans,
> 
> On Sat, Mar 6, 2021 at 2:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Disable event reporting on suspend when our parent is not
>> a wakeup-source. This should help save some extra power in
>> this case.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/hid/Kconfig          |  2 +-
>>  drivers/hid/hid-multitouch.c | 23 ++++++++++++++++++++++-
>>  2 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 786b71ef7738..5cbe4adfd816 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -675,7 +675,7 @@ config HID_MONTEREY
>>
>>  config HID_MULTITOUCH
>>         tristate "HID Multitouch panels"
>> -       depends on HID
>> +       depends on USB_HID
> 
> I tried really hard during the past 8 years to not have a usbhid
> dependency on hid-multitouch.
> 
> The code below should not break the test suite, but still I am not
> that happy about the Kconfig change.
> 
> I don't see an immediate and better way of doing what you are
> achieving here, but maybe you have some magic I did not think about
> that would help to no pull USB_HID with HID_MULTITOUCH.
> 
> FTR, I think the use case of hid-multitouch *without* USB is rather
> non-existent, but there might be some weird systems with I2C only
> (edge computing?).

Interesting how you often manage to pick out the bits of patches
which I'm not 100% happy with myself either. I was thinking the
same thing myself.

We have this: "hid_is_using_ll_driver(hdev, &usb_hid_driver)" check
in various drivers under drivers/hid and so far the dependency fix
of adding a "depends on USB_HID" was not pretty but ok, because it
would be weird to enable those HID drivers on a system without
USB_HID being enabled. But I agree with you that hid-multitouch
is different. So I did try to come up with something better and
failed.

But now that I look at this with fresh eyes I think I see a
nice solution for this.

I propose to add a hid_is_usb_device() helper which is defined
in hid-core.c (1) and this helper would look like this:

bool hid_is_usb_device(struct hid_device *hid)
{
#if IS_ENABLED(CONFIG_USB_HID)
	return hid_is_using_ll_driver(hid, &usb_hid_driver);
#else
	return false;
#endif
}

And then I can use this helper function instead of directly doing
the hid_is_using_ll_driver() check in hid-multitouch.c fixing
this dependency ugliness.

1) hid-core.c is controlled by CONFIG_HID which gets selected at
the Kconfig level by CONFIG_USB_HID so there is no chance of
builtin vs module issues.

As an added bonus I can then also do a follow-up patch-set to
remove more depends on USB_HID stuff by switching to the helper
in other places too.

###

Unrelated but something else which I was wondering about while
working on this patch. 

I think that it might also be useful to change the
mt_parent_may_wake() helper introduced here into a generic
hid_parent_may_wakeup() helper in case we need a similar thing
in other places. I decided it may be best to do that once we
have a second driver needing such a check, but since we're
discussing this anyways, what is your opinion on this ?

Regards,

Hans



>>         help
>>           Generic support for HID multitouch panels.
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index cfb68e443ddd..7926295bab81 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -1759,12 +1759,33 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>  }
>>
>>  #ifdef CONFIG_PM
>> +
>> +/* Check if the parent which has the power/wakeup* sysfs attributes may wake the hdev */
>> +static bool mt_parent_may_wake(struct hid_device *hdev)
>> +{
>> +       struct device *parent = hdev->dev.parent;
>> +
>> +       /*
>> +        * USB-HID is attached to the usb_interface (our parent), the
>> +        * power/wakeup* attr are part of the usb-device which is its parent.
>> +        */
>> +       if (hid_is_using_ll_driver(hdev, &usb_hid_driver) && parent)
>> +               parent = parent->parent;
>> +
>> +       if (parent)
>> +               return device_may_wakeup(parent);
>> +
>> +       /* Huh? Play it safe and keep reporting events. */
>> +       return true;
>> +}
>> +
>>  static int mt_suspend(struct hid_device *hdev, pm_message_t state)
>>  {
>>         struct mt_device *td = hid_get_drvdata(hdev);
>>
>>         /* High latency is desirable for power savings during S3/S0ix */
>> -       if (td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP)
>> +       if ((td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP) ||
>> +           !mt_parent_may_wake(hdev))
>>                 mt_set_modes(hdev, HID_LATENCY_HIGH, false, false);
>>         else
>>                 mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
>> --
>> 2.30.1
>>
> 

Re: [PATCH 3/3] HID: multitouch: Disable event reporting on suspend when our parent is not a wakeup-source

[Benjamin Tissoires]

On Wed, May 5, 2021 at 4:00 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 5/5/21 3:40 PM, Benjamin Tissoires wrote:
> > Hi Hans,
> >
> > On Sat, Mar 6, 2021 at 2:37 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> Disable event reporting on suspend when our parent is not
> >> a wakeup-source. This should help save some extra power in
> >> this case.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/hid/Kconfig          |  2 +-
> >>  drivers/hid/hid-multitouch.c | 23 ++++++++++++++++++++++-
> >>  2 files changed, 23 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> >> index 786b71ef7738..5cbe4adfd816 100644
> >> --- a/drivers/hid/Kconfig
> >> +++ b/drivers/hid/Kconfig
> >> @@ -675,7 +675,7 @@ config HID_MONTEREY
> >>
> >>  config HID_MULTITOUCH
> >>         tristate "HID Multitouch panels"
> >> -       depends on HID
> >> +       depends on USB_HID
> >
> > I tried really hard during the past 8 years to not have a usbhid
> > dependency on hid-multitouch.
> >
> > The code below should not break the test suite, but still I am not
> > that happy about the Kconfig change.
> >
> > I don't see an immediate and better way of doing what you are
> > achieving here, but maybe you have some magic I did not think about
> > that would help to no pull USB_HID with HID_MULTITOUCH.
> >
> > FTR, I think the use case of hid-multitouch *without* USB is rather
> > non-existent, but there might be some weird systems with I2C only
> > (edge computing?).
>
> Interesting how you often manage to pick out the bits of patches
> which I'm not 100% happy with myself either. I was thinking the
> same thing myself.

:)

>
> We have this: "hid_is_using_ll_driver(hdev, &usb_hid_driver)" check
> in various drivers under drivers/hid and so far the dependency fix
> of adding a "depends on USB_HID" was not pretty but ok, because it
> would be weird to enable those HID drivers on a system without
> USB_HID being enabled. But I agree with you that hid-multitouch
> is different. So I did try to come up with something better and
> failed.
>
> But now that I look at this with fresh eyes I think I see a
> nice solution for this.
>
> I propose to add a hid_is_usb_device() helper which is defined
> in hid-core.c (1) and this helper would look like this:
>
> bool hid_is_usb_device(struct hid_device *hid)
> {
> #if IS_ENABLED(CONFIG_USB_HID)
>         return hid_is_using_ll_driver(hid, &usb_hid_driver);
> #else
>         return false;
> #endif
> }
>
> And then I can use this helper function instead of directly doing
> the hid_is_using_ll_driver() check in hid-multitouch.c fixing
> this dependency ugliness.
>
> 1) hid-core.c is controlled by CONFIG_HID which gets selected at
> the Kconfig level by CONFIG_USB_HID so there is no chance of
> builtin vs module issues.

OK, sounds good enough to me. The one thing I dislike about IS_ENABLED
is that it is not very friendly with out of the tree modules. But
here, I guess if you have a system without CONFIG_USB_HID, you will
probably never need to enable it without recompiling your tree.

So ack by me.

>
> As an added bonus I can then also do a follow-up patch-set to
> remove more depends on USB_HID stuff by switching to the helper
> in other places too.

That would be wonderful :)

>
> ###
>
> Unrelated but something else which I was wondering about while
> working on this patch.
>
> I think that it might also be useful to change the
> mt_parent_may_wake() helper introduced here into a generic
> hid_parent_may_wakeup() helper in case we need a similar thing
> in other places. I decided it may be best to do that once we
> have a second driver needing such a check, but since we're
> discussing this anyways, what is your opinion on this ?

I can definitely see the benefit of it, but OTOH, I would stick to
your first approach. If we are just needing it for one driver, we
probably want to keep it local to this one driver.

Cheers,
Benjamin

>
> Regards,
>
> Hans
>
>
>
> >>         help
> >>           Generic support for HID multitouch panels.
> >>
> >> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> >> index cfb68e443ddd..7926295bab81 100644
> >> --- a/drivers/hid/hid-multitouch.c
> >> +++ b/drivers/hid/hid-multitouch.c
> >> @@ -1759,12 +1759,33 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >>  }
> >>
> >>  #ifdef CONFIG_PM
> >> +
> >> +/* Check if the parent which has the power/wakeup* sysfs attributes may wake the hdev */
> >> +static bool mt_parent_may_wake(struct hid_device *hdev)
> >> +{
> >> +       struct device *parent = hdev->dev.parent;
> >> +
> >> +       /*
> >> +        * USB-HID is attached to the usb_interface (our parent), the
> >> +        * power/wakeup* attr are part of the usb-device which is its parent.
> >> +        */
> >> +       if (hid_is_using_ll_driver(hdev, &usb_hid_driver) && parent)
> >> +               parent = parent->parent;
> >> +
> >> +       if (parent)
> >> +               return device_may_wakeup(parent);
> >> +
> >> +       /* Huh? Play it safe and keep reporting events. */
> >> +       return true;
> >> +}
> >> +
> >>  static int mt_suspend(struct hid_device *hdev, pm_message_t state)
> >>  {
> >>         struct mt_device *td = hid_get_drvdata(hdev);
> >>
> >>         /* High latency is desirable for power savings during S3/S0ix */
> >> -       if (td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP)
> >> +       if ((td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP) ||
> >> +           !mt_parent_may_wake(hdev))
> >>                 mt_set_modes(hdev, HID_LATENCY_HIGH, false, false);
> >>         else
> >>                 mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
> >> --
> >> 2.30.1
> >>
> >
>

Re: [PATCH 1/3] HID: asus: Cleanup Asus T101HA keyboard-dock handling

[Jiri Kosina]

On Sat, 6 Mar 2021, Hans de Goede wrote:

> There is no need to use a quirk and then return -ENODEV from the
> asus_probe() function to avoid that hid-asus binds to the hiddev
> for the USB-interface for the hid-multitouch touchpad.
> 
> The hid-multitouch hiddev has a group of HID_GROUP_MULTITOUCH_WIN_8,
> so the same result can be achieved by making the hid_device_id entry
> for the dock in the asus_devices[] table only match on HID_GROUP_GENERIC
> instead of having it match HID_GROUP_ANY.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied to for-5.13/upstream-fixes.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/3] HID: multitouch: Disable event reporting on suspend on the Asus T101HA touchpad

[Jiri Kosina]

On Sat, 6 Mar 2021, Hans de Goede wrote:

> The Asus T101HA has a problem with spurious wakeups when the lid is
> closed, this is caused by the screen sitting so close to the touchpad
> that the touchpad ends up reporting touch events, causing these wakeups.
> 
> Add a quirk which disables event reporting on suspend when set, and
> enable this quirk for the Asus T101HA touchpad fixing the spurious
> wakeups, while still allowing the device to be woken by pressing a
> key on the keyboard (which is part of the same USB device).

This one also applied to for-5.13/upstream-fixes. Thanks,

-- 
Jiri Kosina
SUSE Labs