Messages in this thread | | | Subject | Re: [PATCH v3] platform/x86: asus-wmi: Add support for SW_TABLET_MODE on UX360 | From | Hans de Goede <> | Date | Wed, 28 Oct 2020 12:44:32 +0100 |
| |
Hi,
On 10/21/20 12:29 AM, Samuel Čavoj wrote: > Hi, > > naturally I notice this right after I send the patch, but my whitespace > is wrong. Time to set a pre-commit hook up. I guess that means a v4, > unless you would fix it on your end? It's just a single line. > > Sorry about all the noise, > Sam > > On 21.10.2020 00:09, Samuel Čavoj wrote: >> @@ -375,6 +376,20 @@ static int asus_wmi_input_init(struct asus_wmi *asus) >> } >> } >> >> + if (asus->driver->quirks->use_lid_flip_devid) { >> + result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP); >> + if (result < 0) > > Right ^here.
No worries I will fix this when merging it.
> >> + asus->driver->quirks->use_lid_flip_devid = 0; >> + if (result >= 0) { >> + input_set_capability(asus->inputdev, EV_SW, SW_TABLET_MODE); >> + input_report_switch(asus->inputdev, SW_TABLET_MODE, result); >> + } else if (result == -ENODEV) { >> + pr_err("This device has lid_flip quirk but got ENODEV checking it. This is a bug."); >> + } else { >> + pr_err("Error checking for lid-flip: %d\n", result); >> + } >> + } >
>> + if (asus->driver->quirks->use_lid_flip_devid) { >> + result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_LID_FLIP); >> + if (result < 0) >> + asus->driver->quirks->use_lid_flip_devid = 0; > > I had the idea of doing this, not sure if it is actually a good idea > though. The idea is that it would prevent querying the device later > during runtime, if the first get_devstate fails. Also I assume doing a > input_report_switch without a corresponding input_set_capability is not > great either, this would prevent that, if for some reason the > get_devstate fails in the beginning but later fixes itself. > > However, I can also imagine that writing to the quirks structure is > frowned upon, please tell if that is the case. Thanks.
The quirk_entry structs are not defined const; and they are already written to in other places (e.g. the wapf module param handling). So this is fine.
Note FWIW, that calling input_report_switch() with a switch which has not been declared before through input_set_capability() before is allowed and is simply treated as a no-op. But just skipping the handling of the event alltogether is a bit cleaner, so your approach is fine.
Regards,
Hans
| |