| Message ID | 20201203212148.36039-1-eliadevito@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2,1/2] intel-hid: add support for SW_TABLET_MODE | expand |
Hi 2020. december 3., csütörtök 22:21 keltezéssel, Elia Devito írta: > [...] > diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c > index 86261970bd8f..fed24d4f28b8 100644 > --- a/drivers/platform/x86/intel-hid.c > +++ b/drivers/platform/x86/intel-hid.c > @@ -15,6 +15,9 @@ > #include <linux/platform_device.h> > #include <linux/suspend.h> > > +/* When NOT in tablet mode, VGBS returns with the flag 0x40 */ > +#define TABLET_MODE_FLAG 0x40 I think `BIT(6)` would be better (linux/bits.h). > + > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Alex Hung"); > > @@ -89,9 +92,26 @@ static const struct dmi_system_id button_array_table[] = { > { } > }; > [...] > +static void detect_tablet_mode(struct platform_device *device) I believe `report_tablet_mode_state()` or something similar would be a more apt name. > +{ > + struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); > + acpi_handle handle = ACPI_HANDLE(&device->dev); > + unsigned long long vgbs; > + int m; > + > + if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_VGBS_FN, &vgbs)) > + return; > + > + m = !(vgbs & TABLET_MODE_FLAG); > + input_report_switch(priv->switches, SW_TABLET_MODE, m); > + input_sync(priv->switches); > +} > + > static void notify_handler(acpi_handle handle, u32 event, void *context) > { > struct platform_device *device = context; > @@ -363,6 +415,13 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) > if (event == 0xce) > goto wakeup; > > + /* > + * Switch events will wake the device and report the new switch > + * position to the input subsystem. > + */ > + if (priv->switches && (event == 0xcc || event == 0xcd)) > + goto wakeup; > + > /* Wake up on 5-button array events only. */ > if (event == 0xc0 || !priv->array) > return; > @@ -374,6 +433,21 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) > > wakeup: > pm_wakeup_hard_event(&device->dev); > + > + if (priv->switches) { > + if (event == 0xcc) { > + input_report_switch(priv->switches, SW_TABLET_MODE, 1); > + input_sync(priv->switches); > + return; > + } > + > + if (event == 0xcd) { > + input_report_switch(priv->switches, SW_TABLET_MODE, 0); > + input_sync(priv->switches); > + return; > + } > + } > + > return; > } > > @@ -398,6 +472,20 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) > } > } > > + if (priv->switches) { > + if (event == 0xcc) { > + input_report_switch(priv->switches, SW_TABLET_MODE, 1); > + input_sync(priv->switches); > + return; > + } > + > + if (event == 0xcd) { > + input_report_switch(priv->switches, SW_TABLET_MODE, 0); > + input_sync(priv->switches); > + return; > + } > + } Wouldn't be better to create a new function `bool report_tablet_mode_event()` which would basically contain the above `if` or better, a `switch`, and then you could use it here and in the wake-up path like the following: ``` if (report_tablet_mode_event(priv->switches, event)) return; ``` (or similarly) > + > /* 0xC0 is for HID events, other values are for 5 button array */ > if (event != 0xc0) { > if (!priv->array || > @@ -485,6 +573,16 @@ static int intel_hid_probe(struct platform_device *device) > pr_err("Failed to setup Intel 5 button array hotkeys\n"); > } > > + /* Setup switches for devices that we know VGBS return correctly */ > + if (dmi_check_system(dmi_vgbs_allow_list)) { > + dev_info(&device->dev, "platform supports switches\n"); > + err = intel_hid_switches_setup(device); > + if (err) > + pr_err("Failed to setup Intel HID switches\n"); > + else > + detect_tablet_mode(device); > + } > + > status = acpi_install_notify_handler(handle, > ACPI_DEVICE_NOTIFY, > notify_handler, > -- > 2.28.0 Regards, Barnabás Pőcze
2020. december 4., péntek 0:45 keltezéssel, Barnabás Pőcze írta: > Hi > > [...] Oh well, I replied to the wrong email, apologies. :-(
Hi Barnabás In data venerdì 4 dicembre 2020 00:45:10 CET, Barnabás Pőcze ha scritto: > Hi > > 2020. december 3., csütörtök 22:21 keltezéssel, Elia Devito írta: > > [...] > > diff --git a/drivers/platform/x86/intel-hid.c > > b/drivers/platform/x86/intel-hid.c index 86261970bd8f..fed24d4f28b8 > > 100644 > > --- a/drivers/platform/x86/intel-hid.c > > +++ b/drivers/platform/x86/intel-hid.c > > @@ -15,6 +15,9 @@ > > > > #include <linux/platform_device.h> > > #include <linux/suspend.h> > > > > +/* When NOT in tablet mode, VGBS returns with the flag 0x40 */ > > +#define TABLET_MODE_FLAG 0x40 > > I think `BIT(6)` would be better (linux/bits.h). > Okay, I will change it > > + > > > > MODULE_LICENSE("GPL"); > > MODULE_AUTHOR("Alex Hung"); > > > > @@ -89,9 +92,26 @@ static const struct dmi_system_id button_array_table[] > > = {> > > { } > > > > }; > > > > [...] > > +static void detect_tablet_mode(struct platform_device *device) > > I believe `report_tablet_mode_state()` or something similar would be a more > apt name. Sound good, I will rename it. > > +{ > > + struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); > > + acpi_handle handle = ACPI_HANDLE(&device->dev); > > + unsigned long long vgbs; > > + int m; > > + > > + if (!intel_hid_evaluate_method(handle, INTEL_HID_DSM_VGBS_FN, &vgbs)) > > + return; > > + > > + m = !(vgbs & TABLET_MODE_FLAG); > > + input_report_switch(priv->switches, SW_TABLET_MODE, m); > > + input_sync(priv->switches); > > +} > > + > > > > static void notify_handler(acpi_handle handle, u32 event, void *context) > > { > > > > struct platform_device *device = context; > > > > @@ -363,6 +415,13 @@ static void notify_handler(acpi_handle handle, u32 > > event, void *context)> > > if (event == 0xce) > > > > goto wakeup; > > > > + /* > > + * Switch events will wake the device and report the new switch > > + * position to the input subsystem. > > + */ > > + if (priv->switches && (event == 0xcc || event == 0xcd)) > > + goto wakeup; > > + > > > > /* Wake up on 5-button array events only. */ > > if (event == 0xc0 || !priv->array) > > > > return; > > > > @@ -374,6 +433,21 @@ static void notify_handler(acpi_handle handle, u32 > > event, void *context)> > > wakeup: > > pm_wakeup_hard_event(&device->dev); > > > > + > > + if (priv->switches) { > > + if (event == 0xcc) { > > + input_report_switch(priv- >switches, SW_TABLET_MODE, 1); > > + input_sync(priv->switches); > > + return; > > + } > > + > > + if (event == 0xcd) { > > + input_report_switch(priv- >switches, SW_TABLET_MODE, 0); > > + input_sync(priv->switches); > > + return; > > + } > > + } > > + > > > > return; > > > > } > > > > @@ -398,6 +472,20 @@ static void notify_handler(acpi_handle handle, u32 > > event, void *context)> > > } > > > > } > > > > + if (priv->switches) { > > + if (event == 0xcc) { > > + input_report_switch(priv->switches, SW_TABLET_MODE, 1); > > + input_sync(priv->switches); > > + return; > > + } > > + > > + if (event == 0xcd) { > > + input_report_switch(priv->switches, SW_TABLET_MODE, 0); > > + input_sync(priv->switches); > > + return; > > + } > > + } > > Wouldn't be better to create a new function `bool > report_tablet_mode_event()` which would basically contain the above `if` or > better, a `switch`, and then you could use it here and in the wake-up path > like the following: > > ``` > if (report_tablet_mode_event(priv->switches, event)) > return; > ``` > (or similarly) > Looks more clean, I will do. > > + > > > > /* 0xC0 is for HID events, other values are for 5 button array */ > > if (event != 0xc0) { > > > > if (!priv->array || > > > > @@ -485,6 +573,16 @@ static int intel_hid_probe(struct platform_device > > *device)> > > pr_err("Failed to setup Intel 5 button array hotkeys\n"); > > > > } > > > > + /* Setup switches for devices that we know VGBS return correctly */ > > + if (dmi_check_system(dmi_vgbs_allow_list)) { > > + dev_info(&device->dev, "platform supports switches\n"); > > + err = intel_hid_switches_setup(device); > > + if (err) > > + pr_err("Failed to setup Intel HID switches\n"); > > + else > > + detect_tablet_mode(device); > > + } > > + > > > > status = acpi_install_notify_handler(handle, > > > > ACPI_DEVICE_NOTIFY, > > notify_handler, > > > > -- > > 2.28.0 > > Regards, > Barnabás Pőcze Thank you for the review Regards Elia
diff --git a/drivers/platform/x86/intel-hid.c b/drivers/platform/x86/intel-hid.c index fed24d4f28b8..3cc8f8ac48b2 100644 --- a/drivers/platform/x86/intel-hid.c +++ b/drivers/platform/x86/intel-hid.c @@ -404,6 +404,19 @@ static void notify_handler(acpi_handle handle, u32 event, void *context) struct platform_device *device = context; struct intel_hid_priv *priv = dev_get_drvdata(&device->dev); unsigned long long ev_index; + int err; + + /* + * Some convertible have unreliable VGBS return which could cause incorrect + * SW_TABLET_MODE report, in these cases we enable support when receiving + * the first event instead of during driver setup. + */ + if (!priv->switches && (event == 0xcc || event == 0xcd)) { + dev_info(&device->dev, "switch event received, enable switches supports\n"); + err = intel_hid_switches_setup(device); + if (err) + pr_err("Failed to setup Intel HID switches\n"); + } if (priv->wakeup_mode) { /*
Some convertible have unreliable VGBS return, in these cases we enable support when receiving the first event. Signed-off-by: Elia Devito <eliadevito@gmail.com> --- drivers/platform/x86/intel-hid.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)