From: Luke Jones <luke@ljones.dev>
To: Hans de Goede <hdegoede@redhat.com>
Cc: markgross@kernel.org, platform-driver-x86@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum
Date: Fri, 26 Aug 2022 08:50:04 +1200 [thread overview]
Message-ID: <G7V6HR.3WZF23WP4MCJ3@ljones.dev> (raw)
In-Reply-To: <b2ce575a-470c-5a56-2cd0-9f3e7dbac1ad@redhat.com>
On Wed, Aug 24 2022 at 16:12:42 +0200, Hans de Goede
<hdegoede@redhat.com> wrote:
> Hi,
>
> On 8/13/22 11:27, Luke D. Jones wrote:
>> Due to multiple types of tablet/lidflip, the existing code for
>> handling these events is refactored to use an enum for each type.
>>
>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
>
> Thank you for the patch, I've merged this but with one change:
>
>> ---
>> drivers/platform/x86/asus-nb-wmi.c | 13 +++---
>> drivers/platform/x86/asus-wmi.c | 67
>> ++++++++++++++++++++----------
>> drivers/platform/x86/asus-wmi.h | 9 +++-
>> 3 files changed, 58 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/platform/x86/asus-nb-wmi.c
>> b/drivers/platform/x86/asus-nb-wmi.c
>> index a81dc4b191b7..3a93e056c4e1 100644
>> --- a/drivers/platform/x86/asus-nb-wmi.c
>> +++ b/drivers/platform/x86/asus-nb-wmi.c
>> @@ -115,12 +115,12 @@ static struct quirk_entry quirk_asus_forceals
>> = {
>> };
>>
>> static struct quirk_entry quirk_asus_use_kbd_dock_devid = {
>> - .use_kbd_dock_devid = true,
>> + .tablet_switch_mode = asus_wmi_kbd_dock_devid,
>> };
>>
>> static struct quirk_entry quirk_asus_use_lid_flip_devid = {
>> .wmi_backlight_set_devstate = true,
>> - .use_lid_flip_devid = true,
>> + .tablet_switch_mode = asus_wmi_lid_flip_devid,
>> };
>>
>> static int dmi_matched(const struct dmi_system_id *dmi)
>> @@ -492,16 +492,13 @@ static void asus_nb_wmi_quirks(struct
>> asus_wmi_driver *driver)
>>
>> switch (tablet_mode_sw) {
>> case 0:
>> - quirks->use_kbd_dock_devid = false;
>> - quirks->use_lid_flip_devid = false;
>> + quirks->tablet_switch_mode = asus_wmi_no_tablet_switch;
>> break;
>> case 1:
>> - quirks->use_kbd_dock_devid = true;
>> - quirks->use_lid_flip_devid = false;
>> + quirks->tablet_switch_mode = asus_wmi_kbd_dock_devid;
>> break;
>> case 2:
>> - quirks->use_kbd_dock_devid = false;
>> - quirks->use_lid_flip_devid = true;
>> + quirks->tablet_switch_mode = asus_wmi_lid_flip_devid;
>> break;
>> }
>>
>> diff --git a/drivers/platform/x86/asus-wmi.c
>> b/drivers/platform/x86/asus-wmi.c
>> index 0421ffb81927..b4690ac39147 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -485,8 +485,11 @@ static bool asus_wmi_dev_is_present(struct
>> asus_wmi *asus, u32 dev_id)
>>
>> static int asus_wmi_input_init(struct asus_wmi *asus)
>> {
>> + struct device *dev;
>> int err, result;
>>
>> + dev = &asus->platform_device->dev;
>> +
>> asus->inputdev = input_allocate_device();
>> if (!asus->inputdev)
>> return -ENOMEM;
>> @@ -494,35 +497,38 @@ static int asus_wmi_input_init(struct
>> asus_wmi *asus)
>> asus->inputdev->name = asus->driver->input_name;
>> asus->inputdev->phys = asus->driver->input_phys;
>> asus->inputdev->id.bustype = BUS_HOST;
>> - asus->inputdev->dev.parent = &asus->platform_device->dev;
>> + asus->inputdev->dev.parent = dev;
>> set_bit(EV_REP, asus->inputdev->evbit);
>>
>> err = sparse_keymap_setup(asus->inputdev, asus->driver->keymap,
>> NULL);
>> if (err)
>> goto err_free_dev;
>>
>> - if (asus->driver->quirks->use_kbd_dock_devid) {
>> + switch (asus->driver->quirks->tablet_switch_mode) {
>> + case asus_wmi_no_tablet_switch:
>> + break;
>> + case asus_wmi_kbd_dock_devid:
>> result = asus_wmi_get_devstate_simple(asus,
>> ASUS_WMI_DEVID_KBD_DOCK);
>> 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("Error checking for keyboard-dock: %d\n", result);
>> + dev_err(dev, "Error checking for keyboard-dock: %d\n", result);
>> }
>> - }
>> -
>> - if (asus->driver->quirks->use_lid_flip_devid) {
>> + break;
>> + case asus_wmi_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;
>> + asus->driver->quirks->tablet_switch_mode =
>> asus_wmi_no_tablet_switch;
>> 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.");
>> + dev_err(dev, "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);
>> + dev_err(dev, "Error checking for lid-flip: %d\n", result);
>> }
>> + break;
>> }
>>
>> err = input_register_device(asus->inputdev);
>> @@ -548,8 +554,9 @@ static void asus_wmi_input_exit(struct asus_wmi
>> *asus)
>>
>> static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus)
>> {
>> - int result = asus_wmi_get_devstate_simple(asus,
>> ASUS_WMI_DEVID_LID_FLIP);
>> + int result;
>>
>> + result = asus_wmi_get_devstate_simple(asus,
>> ASUS_WMI_DEVID_LID_FLIP);
>> if (result >= 0) {
>> input_report_switch(asus->inputdev, SW_TABLET_MODE, result);
>> input_sync(asus->inputdev);
>> @@ -3044,20 +3051,26 @@ static void asus_wmi_handle_event_code(int
>> code, struct asus_wmi *asus)
>> return;
>> }
>>
>> - if (asus->driver->quirks->use_kbd_dock_devid && code ==
>> NOTIFY_KBD_DOCK_CHANGE) {
>
> Given that we need to check for both the tablet_switch_mode as well as
> different event codes, I believe that keep using something like:
>
> if (asus->driver->quirks->tablet_switch_mode ==
> asus_wmi_kbd_dock_devid &&
> code == NOTIFY_KBD_DOCK_CHANGE) {
>
> here is better then doing a switch-case with the code checkes nested
> under
I think I recall that Andy requested use of the switch/case but I might
be wrong.
Thank you for your help and clean up of this. The patch is making quite
a few people happy.
>
>> - result = asus_wmi_get_devstate_simple(asus,
>> - ASUS_WMI_DEVID_KBD_DOCK);
>> + switch (asus->driver->quirks->tablet_switch_mode) {
>> + case asus_wmi_no_tablet_switch:
>> + break;
>> + case asus_wmi_kbd_dock_devid:
>> + if (code == NOTIFY_KBD_DOCK_CHANGE) {
>> + result = asus_wmi_get_devstate_simple(asus,
>> ASUS_WMI_DEVID_KBD_DOCK);
>> if (result >= 0) {
>> input_report_switch(asus->inputdev, SW_TABLET_MODE,
>
> Note the indent of these 2 lines is wrong now, it should be indentend
> one
> extra step because it is inside the if (code ==
> NOTIFY_KBD_DOCK_CHANGE) {
> block now, sticking with a:
>
> if (asus->driver->quirks->tablet_switch_mode ==
> asus_wmi_kbd_dock_devid &&
> code == NOTIFY_KBD_DOCK_CHANGE) {
>
> is style instead of nesting avoids the need for this extra
> indentation.
>
> I have fixed this up while merging the patches.
>
> Regards,
>
> Hans
>
>
>
>
>> - !result);
>> + !result);
>> input_sync(asus->inputdev);
>> }
>> return;
>> - }
>> -
>> - if (asus->driver->quirks->use_lid_flip_devid && code ==
>> NOTIFY_LID_FLIP) {
>> - lid_flip_tablet_mode_get_state(asus);
>> - return;
>> + }
>> + break;
>> + case asus_wmi_lid_flip_devid:
>> + if (code == NOTIFY_LID_FLIP) {
>> + lid_flip_tablet_mode_get_state(asus);
>> + return;
>> + }
>> + break;
>> }
>>
>> if (asus->fan_boost_mode_available && code == NOTIFY_KBD_FBM) {
>> @@ -3700,8 +3713,14 @@ static int asus_hotk_resume(struct device
>> *device)
>> if (asus_wmi_has_fnlock_key(asus))
>> asus_wmi_fnlock_update(asus);
>>
>> - if (asus->driver->quirks->use_lid_flip_devid)
>> + switch (asus->driver->quirks->tablet_switch_mode) {
>> + case asus_wmi_no_tablet_switch:
>> + case asus_wmi_kbd_dock_devid:
>> + break;
>> + case asus_wmi_lid_flip_devid:
>> lid_flip_tablet_mode_get_state(asus);
>> + break;
>> + }
>>
>> return 0;
>> }
>> @@ -3742,8 +3761,14 @@ static int asus_hotk_restore(struct device
>> *device)
>> if (asus_wmi_has_fnlock_key(asus))
>> asus_wmi_fnlock_update(asus);
>>
>> - if (asus->driver->quirks->use_lid_flip_devid)
>> + switch (asus->driver->quirks->tablet_switch_mode) {
>> + case asus_wmi_no_tablet_switch:
>> + case asus_wmi_kbd_dock_devid:
>> + break;
>> + case asus_wmi_lid_flip_devid:
>> lid_flip_tablet_mode_get_state(asus);
>> + break;
>> + }
>>
>> return 0;
>> }
>> diff --git a/drivers/platform/x86/asus-wmi.h
>> b/drivers/platform/x86/asus-wmi.h
>> index b302415bf1d9..413920bad0c6 100644
>> --- a/drivers/platform/x86/asus-wmi.h
>> +++ b/drivers/platform/x86/asus-wmi.h
>> @@ -25,6 +25,12 @@ struct module;
>> struct key_entry;
>> struct asus_wmi;
>>
>> +enum asus_wmi_tablet_switch_mode {
>> + asus_wmi_no_tablet_switch,
>> + asus_wmi_kbd_dock_devid,
>> + asus_wmi_lid_flip_devid,
>> +};
>> +
>> struct quirk_entry {
>> bool hotplug_wireless;
>> bool scalar_panel_brightness;
>> @@ -33,8 +39,7 @@ struct quirk_entry {
>> bool wmi_backlight_native;
>> bool wmi_backlight_set_devstate;
>> bool wmi_force_als_set;
>> - bool use_kbd_dock_devid;
>> - bool use_lid_flip_devid;
>> + enum asus_wmi_tablet_switch_mode tablet_switch_mode;
>> int wapf;
>> /*
>> * For machines with AMD graphic chips, it will send out WMI event
>
next prev parent reply other threads:[~2022-08-25 20:50 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-13 9:27 [PATCH 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum Luke D. Jones
2022-08-13 9:27 ` [PATCH 2/2] asus-wmi: Add support for ROG X13 tablet mode Luke D. Jones
2022-08-24 14:12 ` [PATCH 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum Hans de Goede
2022-08-25 20:50 ` Luke Jones [this message]
2022-08-25 21:01 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=G7V6HR.3WZF23WP4MCJ3@ljones.dev \
--to=luke@ljones.dev \
--cc=hdegoede@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=markgross@kernel.org \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.