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

From: Darren Hart
Date: Sat Nov 05 2016 - 15:16:27 EST


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