[PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP platforms

[PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP platforms

[Kai-Heng Feng]

Wireless hotkey on HP platforms can trigger two events, if both
hp-wireless and intel-hid are supported. Two events at the same time
renders wireless hotkey useless.

HP confirmed that hp-wireless (HPQ6001) should always be the canonical
source of wireless hotkey event, so skip registering rfkill hotkey if
HPQ6001 is present.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/platform/x86/intel-hid.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c
index 9ee79b74311c..31091c8faf70 100644
--- a/drivers/platform/x86/intel-hid.c
+++ b/drivers/platform/x86/intel-hid.c
@@ -25,6 +25,8 @@ static const struct acpi_device_id intel_hid_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, intel_hid_ids);
 
+static bool hp_wireless_present;
+
 /* In theory, these are HID usages. */
 static const struct key_entry intel_hid_keymap[] = {
 	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
@@ -49,6 +51,29 @@ static const struct key_entry intel_hid_keymap[] = {
 	{ KE_END },
 };
 
+static const struct key_entry intel_hid_no_rfkill_keymap[] = {
+	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
+	/* 2: Toggle SW_ROTATE_LOCK -- easy to implement if seen in wild */
+	{ KE_KEY, 3, { KEY_NUMLOCK } },
+	{ KE_KEY, 4, { KEY_HOME } },
+	{ KE_KEY, 5, { KEY_END } },
+	{ KE_KEY, 6, { KEY_PAGEUP } },
+	{ KE_KEY, 7, { KEY_PAGEDOWN } },
+	/* 8: rfkill -- use hp-wireless instead */
+	{ KE_KEY, 9, { KEY_POWER } },
+	{ KE_KEY, 11, { KEY_SLEEP } },
+	/* 13 has two different meanings in the spec -- ignore it. */
+	{ KE_KEY, 14, { KEY_STOPCD } },
+	{ KE_KEY, 15, { KEY_PLAYPAUSE } },
+	{ KE_KEY, 16, { KEY_MUTE } },
+	{ KE_KEY, 17, { KEY_VOLUMEUP } },
+	{ KE_KEY, 18, { KEY_VOLUMEDOWN } },
+	{ KE_KEY, 19, { KEY_BRIGHTNESSUP } },
+	{ KE_KEY, 20, { KEY_BRIGHTNESSDOWN } },
+	/* 27: wake -- needs special handling */
+	{ KE_END },
+};
+
 /* 5 button array notification value. */
 static const struct key_entry intel_array_keymap[] = {
 	{ KE_KEY,    0xC2, { KEY_LEFTMETA } },                /* Press */
@@ -317,7 +342,8 @@ static int intel_hid_input_setup(struct platform_device *device)
 	if (!priv->input_dev)
 		return -ENOMEM;
 
-	ret = sparse_keymap_setup(priv->input_dev, intel_hid_keymap, NULL);
+	ret = sparse_keymap_setup(priv->input_dev, hp_wireless_present ?
+			intel_hid_no_rfkill_keymap : intel_hid_keymap, NULL);
 	if (ret)
 		return ret;
 
@@ -575,6 +601,9 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void *context, void **rv)
 			dev_info(&dev->dev,
 				 "intel-hid: created platform device\n");
 
+	if (!strcmp(acpi_device_hid(dev), "HPQ6001"))
+		hp_wireless_present = true;
+
 	return AE_OK;
 }
 
-- 
2.17.1

RE: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP platforms

[Mario.Limonciello]

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Kai-Heng Feng
> Sent: Wednesday, June 10, 2020 10:38 AM
> To: alex.hung@canonical.com
> Cc: Kai-Heng Feng; Darren Hart; Andy Shevchenko; open list:INTEL HID EVENT
> DRIVER; open list
> Subject: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP
> platforms
> 
> 
> [EXTERNAL EMAIL]
> 
> Wireless hotkey on HP platforms can trigger two events, if both
> hp-wireless and intel-hid are supported. Two events at the same time
> renders wireless hotkey useless.
> 
> HP confirmed that hp-wireless (HPQ6001) should always be the canonical
> source of wireless hotkey event, so skip registering rfkill hotkey if
> HPQ6001 is present.
> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  drivers/platform/x86/intel-hid.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-
> hid.c
> index 9ee79b74311c..31091c8faf70 100644
> --- a/drivers/platform/x86/intel-hid.c
> +++ b/drivers/platform/x86/intel-hid.c
> @@ -25,6 +25,8 @@ static const struct acpi_device_id intel_hid_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(acpi, intel_hid_ids);
> 
> +static bool hp_wireless_present;
> +
>  /* In theory, these are HID usages. */
>  static const struct key_entry intel_hid_keymap[] = {
>  	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
> @@ -49,6 +51,29 @@ static const struct key_entry intel_hid_keymap[] = {
>  	{ KE_END },
>  };
> 
> +static const struct key_entry intel_hid_no_rfkill_keymap[] = {
> +	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
> +	/* 2: Toggle SW_ROTATE_LOCK -- easy to implement if seen in wild */
> +	{ KE_KEY, 3, { KEY_NUMLOCK } },
> +	{ KE_KEY, 4, { KEY_HOME } },
> +	{ KE_KEY, 5, { KEY_END } },
> +	{ KE_KEY, 6, { KEY_PAGEUP } },
> +	{ KE_KEY, 7, { KEY_PAGEDOWN } },
> +	/* 8: rfkill -- use hp-wireless instead */
> +	{ KE_KEY, 9, { KEY_POWER } },
> +	{ KE_KEY, 11, { KEY_SLEEP } },
> +	/* 13 has two different meanings in the spec -- ignore it. */
> +	{ KE_KEY, 14, { KEY_STOPCD } },
> +	{ KE_KEY, 15, { KEY_PLAYPAUSE } },
> +	{ KE_KEY, 16, { KEY_MUTE } },
> +	{ KE_KEY, 17, { KEY_VOLUMEUP } },
> +	{ KE_KEY, 18, { KEY_VOLUMEDOWN } },
> +	{ KE_KEY, 19, { KEY_BRIGHTNESSUP } },
> +	{ KE_KEY, 20, { KEY_BRIGHTNESSDOWN } },
> +	/* 27: wake -- needs special handling */
> +	{ KE_END },
> +};
> +
>  /* 5 button array notification value. */
>  static const struct key_entry intel_array_keymap[] = {
>  	{ KE_KEY,    0xC2, { KEY_LEFTMETA } },                /* Press */
> @@ -317,7 +342,8 @@ static int intel_hid_input_setup(struct platform_device
> *device)
>  	if (!priv->input_dev)
>  		return -ENOMEM;
> 
> -	ret = sparse_keymap_setup(priv->input_dev, intel_hid_keymap, NULL);
> +	ret = sparse_keymap_setup(priv->input_dev, hp_wireless_present ?
> +			intel_hid_no_rfkill_keymap : intel_hid_keymap, NULL);
>  	if (ret)
>  		return ret;
> 
> @@ -575,6 +601,9 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void
> *context, void **rv)
>  			dev_info(&dev->dev,
>  				 "intel-hid: created platform device\n");
> 
> +	if (!strcmp(acpi_device_hid(dev), "HPQ6001"))
> +		hp_wireless_present = true;

Just having the ACPI device present doesn't actually mean that the user
has a kernel compiled with hp-wireless or that it has finished initializing.

I would think this needs a better handshake in case hp-wireless was unloaded
or not present so the event could still come through intel-hid in this
circumstance.

> +
>  	return AE_OK;
>  }
> 
> --
> 2.17.1

Re: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP platforms

[Alex Hung]

On 2020-06-10 9:49 a.m., Mario.Limonciello@dell.com wrote:
>> -----Original Message-----
>> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
>> owner@vger.kernel.org> On Behalf Of Kai-Heng Feng
>> Sent: Wednesday, June 10, 2020 10:38 AM
>> To: alex.hung@canonical.com
>> Cc: Kai-Heng Feng; Darren Hart; Andy Shevchenko; open list:INTEL HID EVENT
>> DRIVER; open list
>> Subject: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP
>> platforms
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Wireless hotkey on HP platforms can trigger two events, if both
>> hp-wireless and intel-hid are supported. Two events at the same time
>> renders wireless hotkey useless.
>>
>> HP confirmed that hp-wireless (HPQ6001) should always be the canonical
>> source of wireless hotkey event, so skip registering rfkill hotkey if
>> HPQ6001 is present.
>>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>>  drivers/platform/x86/intel-hid.c | 31 ++++++++++++++++++++++++++++++-
>>  1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-
>> hid.c
>> index 9ee79b74311c..31091c8faf70 100644
>> --- a/drivers/platform/x86/intel-hid.c
>> +++ b/drivers/platform/x86/intel-hid.c
>> @@ -25,6 +25,8 @@ static const struct acpi_device_id intel_hid_ids[] = {
>>  };
>>  MODULE_DEVICE_TABLE(acpi, intel_hid_ids);
>>
>> +static bool hp_wireless_present;
>> +
>>  /* In theory, these are HID usages. */
>>  static const struct key_entry intel_hid_keymap[] = {
>>  	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
>> @@ -49,6 +51,29 @@ static const struct key_entry intel_hid_keymap[] = {
>>  	{ KE_END },
>>  };
>>
>> +static const struct key_entry intel_hid_no_rfkill_keymap[] = {
>> +	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
>> +	/* 2: Toggle SW_ROTATE_LOCK -- easy to implement if seen in wild */
>> +	{ KE_KEY, 3, { KEY_NUMLOCK } },
>> +	{ KE_KEY, 4, { KEY_HOME } },
>> +	{ KE_KEY, 5, { KEY_END } },
>> +	{ KE_KEY, 6, { KEY_PAGEUP } },
>> +	{ KE_KEY, 7, { KEY_PAGEDOWN } },
>> +	/* 8: rfkill -- use hp-wireless instead */
>> +	{ KE_KEY, 9, { KEY_POWER } },
>> +	{ KE_KEY, 11, { KEY_SLEEP } },
>> +	/* 13 has two different meanings in the spec -- ignore it. */
>> +	{ KE_KEY, 14, { KEY_STOPCD } },
>> +	{ KE_KEY, 15, { KEY_PLAYPAUSE } },
>> +	{ KE_KEY, 16, { KEY_MUTE } },
>> +	{ KE_KEY, 17, { KEY_VOLUMEUP } },
>> +	{ KE_KEY, 18, { KEY_VOLUMEDOWN } },
>> +	{ KE_KEY, 19, { KEY_BRIGHTNESSUP } },
>> +	{ KE_KEY, 20, { KEY_BRIGHTNESSDOWN } },
>> +	/* 27: wake -- needs special handling */
>> +	{ KE_END },
>> +};
>> +
>>  /* 5 button array notification value. */
>>  static const struct key_entry intel_array_keymap[] = {
>>  	{ KE_KEY,    0xC2, { KEY_LEFTMETA } },                /* Press */
>> @@ -317,7 +342,8 @@ static int intel_hid_input_setup(struct platform_device
>> *device)
>>  	if (!priv->input_dev)
>>  		return -ENOMEM;
>>
>> -	ret = sparse_keymap_setup(priv->input_dev, intel_hid_keymap, NULL);
>> +	ret = sparse_keymap_setup(priv->input_dev, hp_wireless_present ?
>> +			intel_hid_no_rfkill_keymap : intel_hid_keymap, NULL);
>>  	if (ret)
>>  		return ret;
>>
>> @@ -575,6 +601,9 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void
>> *context, void **rv)
>>  			dev_info(&dev->dev,
>>  				 "intel-hid: created platform device\n");
>>
>> +	if (!strcmp(acpi_device_hid(dev), "HPQ6001"))
>> +		hp_wireless_present = true;

(Resend with format removed)

This can impact all HP systems that do not have this problem. How about
a DMI quirk that is limited to this particular system?


> 
> Just having the ACPI device present doesn't actually mean that the user
> has a kernel compiled with hp-wireless or that it has finished initializing.
> 
> I would think this needs a better handshake in case hp-wireless was unloaded
> or not present so the event could still come through intel-hid in this
> circumstance.
> 
>> +
>>  	return AE_OK;
>>  }
>>
>> --
>> 2.17.1
> 


-- 
Cheers,
Alex Hung

Re: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP platforms

[Kai-Heng Feng]

> On Jun 10, 2020, at 23:49, mario.limonciello@dell.com wrote:
> 
>> -----Original Message-----
>> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
>> owner@vger.kernel.org> On Behalf Of Kai-Heng Feng
>> Sent: Wednesday, June 10, 2020 10:38 AM
>> To: alex.hung@canonical.com
>> Cc: Kai-Heng Feng; Darren Hart; Andy Shevchenko; open list:INTEL HID EVENT
>> DRIVER; open list
>> Subject: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP
>> platforms
>> 
>> 
>> [EXTERNAL EMAIL]
>> 
>> Wireless hotkey on HP platforms can trigger two events, if both
>> hp-wireless and intel-hid are supported. Two events at the same time
>> renders wireless hotkey useless.
>> 
>> HP confirmed that hp-wireless (HPQ6001) should always be the canonical
>> source of wireless hotkey event, so skip registering rfkill hotkey if
>> HPQ6001 is present.
>> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>> ---
>> drivers/platform/x86/intel-hid.c | 31 ++++++++++++++++++++++++++++++-
>> 1 file changed, 30 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-
>> hid.c
>> index 9ee79b74311c..31091c8faf70 100644
>> --- a/drivers/platform/x86/intel-hid.c
>> +++ b/drivers/platform/x86/intel-hid.c
>> @@ -25,6 +25,8 @@ static const struct acpi_device_id intel_hid_ids[] = {
>> };
>> MODULE_DEVICE_TABLE(acpi, intel_hid_ids);
>> 
>> +static bool hp_wireless_present;
>> +
>> /* In theory, these are HID usages. */
>> static const struct key_entry intel_hid_keymap[] = {
>> 	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
>> @@ -49,6 +51,29 @@ static const struct key_entry intel_hid_keymap[] = {
>> 	{ KE_END },
>> };
>> 
>> +static const struct key_entry intel_hid_no_rfkill_keymap[] = {
>> +	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
>> +	/* 2: Toggle SW_ROTATE_LOCK -- easy to implement if seen in wild */
>> +	{ KE_KEY, 3, { KEY_NUMLOCK } },
>> +	{ KE_KEY, 4, { KEY_HOME } },
>> +	{ KE_KEY, 5, { KEY_END } },
>> +	{ KE_KEY, 6, { KEY_PAGEUP } },
>> +	{ KE_KEY, 7, { KEY_PAGEDOWN } },
>> +	/* 8: rfkill -- use hp-wireless instead */
>> +	{ KE_KEY, 9, { KEY_POWER } },
>> +	{ KE_KEY, 11, { KEY_SLEEP } },
>> +	/* 13 has two different meanings in the spec -- ignore it. */
>> +	{ KE_KEY, 14, { KEY_STOPCD } },
>> +	{ KE_KEY, 15, { KEY_PLAYPAUSE } },
>> +	{ KE_KEY, 16, { KEY_MUTE } },
>> +	{ KE_KEY, 17, { KEY_VOLUMEUP } },
>> +	{ KE_KEY, 18, { KEY_VOLUMEDOWN } },
>> +	{ KE_KEY, 19, { KEY_BRIGHTNESSUP } },
>> +	{ KE_KEY, 20, { KEY_BRIGHTNESSDOWN } },
>> +	/* 27: wake -- needs special handling */
>> +	{ KE_END },
>> +};
>> +
>> /* 5 button array notification value. */
>> static const struct key_entry intel_array_keymap[] = {
>> 	{ KE_KEY,    0xC2, { KEY_LEFTMETA } },                /* Press */
>> @@ -317,7 +342,8 @@ static int intel_hid_input_setup(struct platform_device
>> *device)
>> 	if (!priv->input_dev)
>> 		return -ENOMEM;
>> 
>> -	ret = sparse_keymap_setup(priv->input_dev, intel_hid_keymap, NULL);
>> +	ret = sparse_keymap_setup(priv->input_dev, hp_wireless_present ?
>> +			intel_hid_no_rfkill_keymap : intel_hid_keymap, NULL);
>> 	if (ret)
>> 		return ret;
>> 
>> @@ -575,6 +601,9 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void
>> *context, void **rv)
>> 			dev_info(&dev->dev,
>> 				 "intel-hid: created platform device\n");
>> 
>> +	if (!strcmp(acpi_device_hid(dev), "HPQ6001"))
>> +		hp_wireless_present = true;
> 
> Just having the ACPI device present doesn't actually mean that the user
> has a kernel compiled with hp-wireless or that it has finished initializing.

We can use IS_REACHEABLE() to see if it's compiled.

> 
> I would think this needs a better handshake in case hp-wireless was unloaded
> or not present so the event could still come through intel-hid in this
> circumstance.

However, it's hard to determine if the other driver has finished initialization or any form of cross module handshake.
Can you please point me to any good example if there's one?

> 
>> +
>> 	return AE_OK;
>> }
>> 
>> --
>> 2.17.1

Re: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP platforms

[Kai-Heng Feng]

> On Jun 11, 2020, at 01:41, Alex Hung <alex.hung@canonical.com> wrote:
> 
> On 2020-06-10 9:49 a.m., Mario.Limonciello@dell.com wrote:
>>> -----Original Message-----
>>> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
>>> owner@vger.kernel.org> On Behalf Of Kai-Heng Feng
>>> Sent: Wednesday, June 10, 2020 10:38 AM
>>> To: alex.hung@canonical.com
>>> Cc: Kai-Heng Feng; Darren Hart; Andy Shevchenko; open list:INTEL HID EVENT
>>> DRIVER; open list
>>> Subject: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP
>>> platforms
>>> 
>>> 
>>> [EXTERNAL EMAIL]
>>> 
>>> Wireless hotkey on HP platforms can trigger two events, if both
>>> hp-wireless and intel-hid are supported. Two events at the same time
>>> renders wireless hotkey useless.
>>> 
>>> HP confirmed that hp-wireless (HPQ6001) should always be the canonical
>>> source of wireless hotkey event, so skip registering rfkill hotkey if
>>> HPQ6001 is present.
>>> 
>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>> ---
>>> drivers/platform/x86/intel-hid.c | 31 ++++++++++++++++++++++++++++++-
>>> 1 file changed, 30 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-
>>> hid.c
>>> index 9ee79b74311c..31091c8faf70 100644
>>> --- a/drivers/platform/x86/intel-hid.c
>>> +++ b/drivers/platform/x86/intel-hid.c
>>> @@ -25,6 +25,8 @@ static const struct acpi_device_id intel_hid_ids[] = {
>>> };
>>> MODULE_DEVICE_TABLE(acpi, intel_hid_ids);
>>> 
>>> +static bool hp_wireless_present;
>>> +
>>> /* In theory, these are HID usages. */
>>> static const struct key_entry intel_hid_keymap[] = {
>>> 	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
>>> @@ -49,6 +51,29 @@ static const struct key_entry intel_hid_keymap[] = {
>>> 	{ KE_END },
>>> };
>>> 
>>> +static const struct key_entry intel_hid_no_rfkill_keymap[] = {
>>> +	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
>>> +	/* 2: Toggle SW_ROTATE_LOCK -- easy to implement if seen in wild */
>>> +	{ KE_KEY, 3, { KEY_NUMLOCK } },
>>> +	{ KE_KEY, 4, { KEY_HOME } },
>>> +	{ KE_KEY, 5, { KEY_END } },
>>> +	{ KE_KEY, 6, { KEY_PAGEUP } },
>>> +	{ KE_KEY, 7, { KEY_PAGEDOWN } },
>>> +	/* 8: rfkill -- use hp-wireless instead */
>>> +	{ KE_KEY, 9, { KEY_POWER } },
>>> +	{ KE_KEY, 11, { KEY_SLEEP } },
>>> +	/* 13 has two different meanings in the spec -- ignore it. */
>>> +	{ KE_KEY, 14, { KEY_STOPCD } },
>>> +	{ KE_KEY, 15, { KEY_PLAYPAUSE } },
>>> +	{ KE_KEY, 16, { KEY_MUTE } },
>>> +	{ KE_KEY, 17, { KEY_VOLUMEUP } },
>>> +	{ KE_KEY, 18, { KEY_VOLUMEDOWN } },
>>> +	{ KE_KEY, 19, { KEY_BRIGHTNESSUP } },
>>> +	{ KE_KEY, 20, { KEY_BRIGHTNESSDOWN } },
>>> +	/* 27: wake -- needs special handling */
>>> +	{ KE_END },
>>> +};
>>> +
>>> /* 5 button array notification value. */
>>> static const struct key_entry intel_array_keymap[] = {
>>> 	{ KE_KEY,    0xC2, { KEY_LEFTMETA } },                /* Press */
>>> @@ -317,7 +342,8 @@ static int intel_hid_input_setup(struct platform_device
>>> *device)
>>> 	if (!priv->input_dev)
>>> 		return -ENOMEM;
>>> 
>>> -	ret = sparse_keymap_setup(priv->input_dev, intel_hid_keymap, NULL);
>>> +	ret = sparse_keymap_setup(priv->input_dev, hp_wireless_present ?
>>> +			intel_hid_no_rfkill_keymap : intel_hid_keymap, NULL);
>>> 	if (ret)
>>> 		return ret;
>>> 
>>> @@ -575,6 +601,9 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void
>>> *context, void **rv)
>>> 			dev_info(&dev->dev,
>>> 				 "intel-hid: created platform device\n");
>>> 
>>> +	if (!strcmp(acpi_device_hid(dev), "HPQ6001"))
>>> +		hp_wireless_present = true;
> 
> (Resend with format removed)
> 
> This can impact all HP systems that do not have this problem.

HP is certain that HPQ6001 should always be used over INT33D5.

If this patch breaks other platform, then we should fix HPQ6001 instead.

> How about
> a DMI quirk that is limited to this particular system?

We should avoid using DMI quirk for this one, as this is to follow the HP's spec.

Kai-Heng

> 
> 
>> 
>> Just having the ACPI device present doesn't actually mean that the user
>> has a kernel compiled with hp-wireless or that it has finished initializing.
>> 
>> I would think this needs a better handshake in case hp-wireless was unloaded
>> or not present so the event could still come through intel-hid in this
>> circumstance.
>> 
>>> +
>>> 	return AE_OK;
>>> }
>>> 
>>> --
>>> 2.17.1
>> 
> 
> 
> -- 
> Cheers,
> Alex Hung

RE: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP platforms

[Mario.Limonciello]

> > Just having the ACPI device present doesn't actually mean that the user
> > has a kernel compiled with hp-wireless or that it has finished
> initializing.
> 
> We can use IS_REACHEABLE() to see if it's compiled.

Yes that was going to be my first suggestion.

> 
> >
> > I would think this needs a better handshake in case hp-wireless was
> unloaded
> > or not present so the event could still come through intel-hid in this
> > circumstance.
> 
> However, it's hard to determine if the other driver has finished
> initialization or any form of cross module handshake.
> Can you please point me to any good example if there's one?

Take a look at how drivers that use dell-wmi-descriptor work.  They can rely
upon looking at -EPROBE_DEFER return code when trying to access the functionality
and do a handoff.

> 
> >
> >> +
> >> 	return AE_OK;
> >> }
> >>
> >> --
> >> 2.17.1

Re: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP platforms

[Alex Hung]

On 2020-06-11 1:24 a.m., Kai-Heng Feng wrote:
> 
> 
>> On Jun 11, 2020, at 01:41, Alex Hung <alex.hung@canonical.com> wrote:
>>
>> On 2020-06-10 9:49 a.m., Mario.Limonciello@dell.com wrote:
>>>> -----Original Message-----
>>>> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
>>>> owner@vger.kernel.org> On Behalf Of Kai-Heng Feng
>>>> Sent: Wednesday, June 10, 2020 10:38 AM
>>>> To: alex.hung@canonical.com
>>>> Cc: Kai-Heng Feng; Darren Hart; Andy Shevchenko; open list:INTEL HID EVENT
>>>> DRIVER; open list
>>>> Subject: [PATCH] platform/x86: intel-hid: Use hp-wireless for rfkill on HP
>>>> platforms
>>>>
>>>>
>>>> [EXTERNAL EMAIL]
>>>>
>>>> Wireless hotkey on HP platforms can trigger two events, if both
>>>> hp-wireless and intel-hid are supported. Two events at the same time
>>>> renders wireless hotkey useless.
>>>>
>>>> HP confirmed that hp-wireless (HPQ6001) should always be the canonical
>>>> source of wireless hotkey event, so skip registering rfkill hotkey if
>>>> HPQ6001 is present.
>>>>
>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>>>> ---
>>>> drivers/platform/x86/intel-hid.c | 31 ++++++++++++++++++++++++++++++-
>>>> 1 file changed, 30 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-
>>>> hid.c
>>>> index 9ee79b74311c..31091c8faf70 100644
>>>> --- a/drivers/platform/x86/intel-hid.c
>>>> +++ b/drivers/platform/x86/intel-hid.c
>>>> @@ -25,6 +25,8 @@ static const struct acpi_device_id intel_hid_ids[] = {
>>>> };
>>>> MODULE_DEVICE_TABLE(acpi, intel_hid_ids);
>>>>
>>>> +static bool hp_wireless_present;
>>>> +
>>>> /* In theory, these are HID usages. */
>>>> static const struct key_entry intel_hid_keymap[] = {
>>>> 	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
>>>> @@ -49,6 +51,29 @@ static const struct key_entry intel_hid_keymap[] = {
>>>> 	{ KE_END },
>>>> };
>>>>
>>>> +static const struct key_entry intel_hid_no_rfkill_keymap[] = {
>>>> +	/* 1: LSuper (Page 0x07, usage 0xE3) -- unclear what to do */
>>>> +	/* 2: Toggle SW_ROTATE_LOCK -- easy to implement if seen in wild */
>>>> +	{ KE_KEY, 3, { KEY_NUMLOCK } },
>>>> +	{ KE_KEY, 4, { KEY_HOME } },
>>>> +	{ KE_KEY, 5, { KEY_END } },
>>>> +	{ KE_KEY, 6, { KEY_PAGEUP } },
>>>> +	{ KE_KEY, 7, { KEY_PAGEDOWN } },
>>>> +	/* 8: rfkill -- use hp-wireless instead */
>>>> +	{ KE_KEY, 9, { KEY_POWER } },
>>>> +	{ KE_KEY, 11, { KEY_SLEEP } },
>>>> +	/* 13 has two different meanings in the spec -- ignore it. */
>>>> +	{ KE_KEY, 14, { KEY_STOPCD } },
>>>> +	{ KE_KEY, 15, { KEY_PLAYPAUSE } },
>>>> +	{ KE_KEY, 16, { KEY_MUTE } },
>>>> +	{ KE_KEY, 17, { KEY_VOLUMEUP } },
>>>> +	{ KE_KEY, 18, { KEY_VOLUMEDOWN } },
>>>> +	{ KE_KEY, 19, { KEY_BRIGHTNESSUP } },
>>>> +	{ KE_KEY, 20, { KEY_BRIGHTNESSDOWN } },
>>>> +	/* 27: wake -- needs special handling */
>>>> +	{ KE_END },
>>>> +};
>>>> +
>>>> /* 5 button array notification value. */
>>>> static const struct key_entry intel_array_keymap[] = {
>>>> 	{ KE_KEY,    0xC2, { KEY_LEFTMETA } },                /* Press */
>>>> @@ -317,7 +342,8 @@ static int intel_hid_input_setup(struct platform_device
>>>> *device)
>>>> 	if (!priv->input_dev)
>>>> 		return -ENOMEM;
>>>>
>>>> -	ret = sparse_keymap_setup(priv->input_dev, intel_hid_keymap, NULL);
>>>> +	ret = sparse_keymap_setup(priv->input_dev, hp_wireless_present ?
>>>> +			intel_hid_no_rfkill_keymap : intel_hid_keymap, NULL);
>>>> 	if (ret)
>>>> 		return ret;
>>>>
>>>> @@ -575,6 +601,9 @@ check_acpi_dev(acpi_handle handle, u32 lvl, void
>>>> *context, void **rv)
>>>> 			dev_info(&dev->dev,
>>>> 				 "intel-hid: created platform device\n");
>>>>
>>>> +	if (!strcmp(acpi_device_hid(dev), "HPQ6001"))
>>>> +		hp_wireless_present = true;
>>
>> (Resend with format removed)
>>
>> This can impact all HP systems that do not have this problem.
> 
> HP is certain that HPQ6001 should always be used over INT33D5.

and OEMs can change all the time.

> 
> If this patch breaks other platform, then we should fix HPQ6001 instead.

Looks like a firmware bug, and this is a workaround.

In this case, both HP6001 and INT33D5 receive wireless toggle events -
which is an incorrect firmware behaviour.

In scenario that both HP6001 and INT33D5 are present and wireless toggle
events are only sent to INT33D5 (a correct firmware behaviour), this
patch will break and this is concerning.

> 
>> How about
>> a DMI quirk that is limited to this particular system?
> 
> We should avoid using DMI quirk for this one, as this is to follow the HP's spec.

INT33D5 is defined by Intel, and HP spec says an event from HP6001, and
nothing about interactions with INT33D5.

Workarounds are unavoidable. It doesn't have to be DMI quirks but it has
to be done without affecting good systems.

> 
> Kai-Heng
> 
>>
>>
>>>
>>> Just having the ACPI device present doesn't actually mean that the user
>>> has a kernel compiled with hp-wireless or that it has finished initializing.
>>>
>>> I would think this needs a better handshake in case hp-wireless was unloaded
>>> or not present so the event could still come through intel-hid in this
>>> circumstance.
>>>
>>>> +
>>>> 	return AE_OK;
>>>> }
>>>>
>>>> --
>>>> 2.17.1
>>>
>>
>>
>> -- 
>> Cheers,
>> Alex Hung
> 


-- 
Cheers,
Alex Hung