Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <737e34f6-298d-fcf9-ef81-cd81fb0279e3@redhat.com>
Date:   Thu, 25 Aug 2022 23:01:41 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [PATCH 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum
Content-Language: en-US
To:     Luke Jones <luke@ljones.dev>
Cc:     markgross@kernel.org, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220813092753.6635-1-luke@ljones.dev>
 <b2ce575a-470c-5a56-2cd0-9f3e7dbac1ad@redhat.com>
 <G7V6HR.3WZF23WP4MCJ3@ljones.dev>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <G7V6HR.3WZF23WP4MCJ3@ljones.dev>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 8/25/22 22:50, Luke Jones wrote:
> 
> 
> 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.

Yeah, or maybe it was me even who asked that :)  The code is not really
ideal in either form.

So as you've probably seen from the Cc I've done some further cleanup
unifying the handling of all 3 tablet-mode-switch types.

Those extra cleanup patches are also in my review-hans branch now.

Regards,

Hans


> 
>>
>>>  -        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
>>
> 
>