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: <b2ce575a-470c-5a56-2cd0-9f3e7dbac1ad@redhat.com>
Date:   Wed, 24 Aug 2022 16:12:42 +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 D. 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>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20220813092753.6635-1-luke@ljones.dev>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

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