Re: [PATCH 1/3] thinkpad_acpi: Move tablet detection into separate function

[Darren Hart]

On Mon, Oct 31, 2016 at 06:55:32PM -0400, Lyude wrote:
> Suggested by Daniel Martin <consume.noise@xxxxxxxxx>

Down below with the other -by: tags please.

> 
> Lenovo's having a bit of fun randomly changing what hotkey events and

Please avoid contractions in documentation in general.

> acpi handles they use for reporting tablet mode, so unfortunately this
> means we're definitely going to need to probe for multiple types of
> tablet mode support. Since the hotkey_init() is already a lot larger
> then it should be, let's split up this detection into it's own function

s/then/than/ (nit, but since I'm commenting anyway...)
s/it's/its/

> to make things a little easier to read.
> 
> As well, since we're going to have multiple types of tablet modes, make
> hotkey_tablet into an enum so we can also use it to indicate the type of
> tablet mode reporting the machine supports.
> 
> Signed-off-by: Lyude <lyude@xxxxxxxxxx>
> Cc: Daniel Martin <consume.noise@xxxxxxxxx>
> ---
>  drivers/platform/x86/thinkpad_acpi.c | 50 +++++++++++++++++++++++++++---------
>  1 file changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
> index b65ce75..369b483 100644
> --- a/drivers/platform/x86/thinkpad_acpi.c
> +++ b/drivers/platform/x86/thinkpad_acpi.c
> @@ -302,7 +302,10 @@ static struct {
>  	u32 hotkey:1;
>  	u32 hotkey_mask:1;
>  	u32 hotkey_wlsw:1;
> -	u32 hotkey_tablet:1;
> +	enum {
> +		TP_HOTKEY_TABLET_NONE = 0,
> +		TP_HOTKEY_TABLET_USES_MHKG = 1,

1 will follow automatically, no need to specify explicitly.

enum usage in this driver is not very consistent, but I don't particularly care
to have it defined within the struct, but I'll leave that for Henrique to
decide.

> +	} hotkey_tablet;
>  	u32 kbdlight:1;
>  	u32 light:1;
>  	u32 light_status:1;
> @@ -3117,6 +3120,34 @@ static const struct tpacpi_quirk tpacpi_hotkey_qtable[] __initconst = {
>  typedef u16 tpacpi_keymap_entry_t;
>  typedef tpacpi_keymap_entry_t tpacpi_keymap_t[TPACPI_HOTKEY_MAP_LEN];
>  
> +static int
> +hotkey_init_tablet_mode(void)
> +{
> +	int res;
> +	char *type;
> +	bool in_tablet_mode;

Declare variables in reverse line length order please:

	bool in_tablet_mode;
	char *type;
	int res;

> +
> +	/* For X41t, X60t, X61t Tablets... */
> +	if (acpi_evalf(hkey_handle, &res, "MHKG", "qd")) {
> +		tp_features.hotkey_tablet = TP_HOTKEY_TABLET_USES_MHKG;
> +		in_tablet_mode = !!(res & TP_HOTKEY_TABLET_MASK);
> +		type = "MHKG";
> +	}
> +
> +	if (!tp_features.hotkey_tablet)
> +		return 0;
> +
> +	pr_info("Tablet mode switch found (type: %s), currently in %s mode\n",
> +		type, in_tablet_mode ? "tablet" : "laptop");
> +
> +	res = add_to_attr_set(hotkey_dev_attributes,
> +			      &dev_attr_hotkey_tablet_mode.attr);
> +	if (res)
> +		return -1;
> +
> +	return in_tablet_mode;
> +}
> +
>  static int __init hotkey_init(struct ibm_init_struct *iibm)
>  {
>  	/* Requirements for changing the default keymaps:
> @@ -3464,17 +3495,6 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>  		res = add_to_attr_set(hotkey_dev_attributes,
>  				&dev_attr_hotkey_radio_sw.attr);
>  
> -	/* For X41t, X60t, X61t Tablets... */
> -	if (!res && acpi_evalf(hkey_handle, &status, "MHKG", "qd")) {
> -		tp_features.hotkey_tablet = 1;
> -		tabletsw_state = !!(status & TP_HOTKEY_TABLET_MASK);
> -		pr_info("possible tablet mode switch found; "
> -			"ThinkPad in %s mode\n",
> -			(tabletsw_state) ? "tablet" : "laptop");
> -		res = add_to_attr_set(hotkey_dev_attributes,
> -				&dev_attr_hotkey_tablet_mode.attr);
> -	}
> -
>  	if (!res)
>  		res = register_attr_set_with_sysfs(
>  				hotkey_dev_attributes,
> @@ -3482,6 +3502,12 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
>  	if (res)
>  		goto err_exit;
>  
> +	res = hotkey_init_tablet_mode();
> +	if (res < 0)
> +		goto err_exit;
> +
> +	tabletsw_state = res;
> +
>  	/* Set up key map */
>  	hotkey_keycode_map = kmalloc(TPACPI_HOTKEY_MAP_SIZE,
>  					GFP_KERNEL);
> -- 
> 2.7.4
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center