[PATCH] Support intel-vbtn based tablet mode switch

[PATCH] Support intel-vbtn based tablet mode switch

[Marco Martin]

some laptops such as Dell Inspiron 7000 series have the
tablet mode switch implemented in intel acpi,
the events to enter and exit the tablet mode are 0xCC and 0xCD

CC: platform-driver-x86@vger.kernel.org
CC: Matthew Garrett <mjg59@srcf.ucam.org>
CC: "Pali Rohár" <pali.rohar@gmail.com>
CC: Darren Hart <dvhart@infradead.org>
CC: Mario Limonciello <mario_limonciello@dell.com>
CC: Andy Shevchenko <andy@infradead.org>

Signed-off-by: Marco Martin <notmart@gmail.com>
---
 drivers/platform/x86/intel-vbtn.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 4809267..c4362df 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -26,6 +26,9 @@
 #include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 
+/* When NOT in tablet mode, VBDS has the flag 0x40 */
+#define TABLET_MODE_FLAG 0x40
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("AceLan Kao");
 
@@ -42,6 +45,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
 	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
 	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
 	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release */
+	{ KE_SW,  0xCC, { .sw = { SW_TABLET_MODE, 1 } } }, /* Tabletmode in */
+	{ KE_SW,  0xCD, { .sw = { SW_TABLET_MODE, 0 } } }, /* Tabletmode out */
 	{ KE_END },
 };
 
@@ -81,6 +86,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 			return;
 		}
 	} else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) {
+		input_sync(priv->input_dev);
 		return;
 	}
 	dev_info(&device->dev, "unknown event index 0x%x\n", event);
@@ -92,6 +98,7 @@ static int intel_vbtn_probe(struct platform_device *device)
 	struct intel_vbtn_priv *priv;
 	acpi_status status;
 	int err;
+	struct acpi_buffer vgbs_output = { ACPI_ALLOCATE_BUFFER, NULL };
 
 	status = acpi_evaluate_object(handle, "VBDL", NULL, NULL);
 	if (ACPI_FAILURE(status)) {
@@ -110,6 +117,27 @@ static int intel_vbtn_probe(struct platform_device *device)
 		return err;
 	}
 
+	status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output);
+	/* VGBS being present and returning something means
+	 * we have a tablet mode switch
+	 */
+	if (ACPI_SUCCESS(status)) {
+		union acpi_object *obj = vgbs_output.pointer;
+
+		input_set_capability(priv->input_dev, EV_SW, SW_TABLET_MODE);
+
+		if (obj && obj->type == ACPI_TYPE_INTEGER) {
+			if (obj->integer.value & TABLET_MODE_FLAG)
+				input_report_switch(priv->input_dev,
+							SW_TABLET_MODE,
+							0);
+			else
+				input_report_switch(priv->input_dev,
+							SW_TABLET_MODE,
+							1);
+		}
+	}
+
 	status = acpi_install_notify_handler(handle,
 					     ACPI_DEVICE_NOTIFY,
 					     notify_handler,
-- 
2.7.4

Re: [PATCH] Support intel-vbtn based tablet mode switch

[Andy Shevchenko]

On Mon, Jan 22, 2018 at 2:48 PM, Marco Martin <notmart@gmail.com> wrote:
> some laptops such as Dell Inspiron 7000 series have the
> tablet mode switch implemented in intel acpi,
> the events to enter and exit the tablet mode are 0xCC and 0xCD

Thanks for an update.

For the future, please consider the following:
- use -v<N> to git format-patch to get a proper version of the patch
(-v2 in this case)
- use proper English grammar, i.e. capitalize sentences in the commit
message, put periods and so on
- CC: tag will look better if capitalized as a word, i.e. Cc:

> CC: platform-driver-x86@vger.kernel.org
> CC: Matthew Garrett <mjg59@srcf.ucam.org>
> CC: "Pali Rohár" <pali.rohar@gmail.com>
> CC: Darren Hart <dvhart@infradead.org>
> CC: Mario Limonciello <mario_limonciello@dell.com>
> CC: Andy Shevchenko <andy@infradead.org>
>

Before I will go somewhere with this version, I would like to hear
from Dmitry that the approach is correct per se.

Also, some style comments below.

> +       { KE_SW,  0xCC, { .sw = { SW_TABLET_MODE, 1 } } }, /* Tabletmode in */
> +       { KE_SW,  0xCD, { .sw = { SW_TABLET_MODE, 0 } } }, /* Tabletmode out */

Tabletmode -> Tablet mode

>         struct intel_vbtn_priv *priv;
>         acpi_status status;
>         int err;
> +       struct acpi_buffer vgbs_output = { ACPI_ALLOCATE_BUFFER, NULL };

Please, locate this in a rule:
 long lines first, group of assignments even before them if it's
allowed by an execution path.

> +       status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output);
> +       /* VGBS being present and returning something means
> +        * we have a tablet mode switch
> +        */
> +       if (ACPI_SUCCESS(status)) {
> +               union acpi_object *obj = vgbs_output.pointer;
> +
> +               input_set_capability(priv->input_dev, EV_SW, SW_TABLET_MODE);
> +
> +               if (obj && obj->type == ACPI_TYPE_INTEGER) {
> +                       if (obj->integer.value & TABLET_MODE_FLAG)
> +                               input_report_switch(priv->input_dev,

> +                                                       SW_TABLET_MODE,
> +                                                       0);

One line?

> +                       else
> +                               input_report_switch(priv->input_dev,

> +                                                       SW_TABLET_MODE,
> +                                                       1);

Ditto.

Besides that, if you have more then one line in the branches, use
curly braces — if {} else {}.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Support intel-vbtn based tablet mode switch

[Marco Martin]

On Mon, Jan 22, 2018 at 2:42 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
>> +       status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output);
>> +       /* VGBS being present and returning something means
>> +        * we have a tablet mode switch
>> +        */
>> +       if (ACPI_SUCCESS(status)) {
>> +               union acpi_object *obj = vgbs_output.pointer;
>> +
>> +               input_set_capability(priv->input_dev, EV_SW, SW_TABLET_MODE);
>> +
>> +               if (obj && obj->type == ACPI_TYPE_INTEGER) {
>> +                       if (obj->integer.value & TABLET_MODE_FLAG)
>> +                               input_report_switch(priv->input_dev,
>
>> +                                                       SW_TABLET_MODE,
>> +                                                       0);
>
> One line?

checkpatch.pl was complaining about lines too long.

--
Marco Martin

Re: [PATCH] Support intel-vbtn based tablet mode switch

[Andy Shevchenko]

On Mon, Jan 22, 2018 at 3:52 PM, Marco Martin <notmart@gmail.com> wrote:
> On Mon, Jan 22, 2018 at 2:42 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:

>>> +               if (obj && obj->type == ACPI_TYPE_INTEGER) {
>>> +                       if (obj->integer.value & TABLET_MODE_FLAG)
>>> +                               input_report_switch(priv->input_dev,
>>
>>> +                                                       SW_TABLET_MODE,
>>> +                                                       0);
>>
>> One line?
>
> checkpatch.pl was complaining about lines too long.

My mailer is using true type fonts, so, I don't see clearly an
indentation here, if you already moved SW_ under priv-> that's fine to
leave, otherwise, just move one tab left the entire line.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] Support intel-vbtn based tablet mode switch

[Dmitry Torokhov]

On Mon, Jan 22, 2018 at 01:48:01PM +0100, Marco Martin wrote:
> some laptops such as Dell Inspiron 7000 series have the
> tablet mode switch implemented in intel acpi,
> the events to enter and exit the tablet mode are 0xCC and 0xCD
> 
> CC: platform-driver-x86@vger.kernel.org
> CC: Matthew Garrett <mjg59@srcf.ucam.org>
> CC: "Pali Rohár" <pali.rohar@gmail.com>
> CC: Darren Hart <dvhart@infradead.org>
> CC: Mario Limonciello <mario_limonciello@dell.com>
> CC: Andy Shevchenko <andy@infradead.org>
> 
> Signed-off-by: Marco Martin <notmart@gmail.com>
> ---
>  drivers/platform/x86/intel-vbtn.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
> index 4809267..c4362df 100644
> --- a/drivers/platform/x86/intel-vbtn.c
> +++ b/drivers/platform/x86/intel-vbtn.c
> @@ -26,6 +26,9 @@
>  #include <linux/suspend.h>
>  #include <acpi/acpi_bus.h>
>  
> +/* When NOT in tablet mode, VBDS has the flag 0x40 */
> +#define TABLET_MODE_FLAG 0x40
> +
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("AceLan Kao");
>  
> @@ -42,6 +45,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
>  	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
>  	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
>  	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release */
> +	{ KE_SW,  0xCC, { .sw = { SW_TABLET_MODE, 1 } } }, /* Tabletmode in */
> +	{ KE_SW,  0xCD, { .sw = { SW_TABLET_MODE, 0 } } }, /* Tabletmode out */
>  	{ KE_END },
>  };
>  
> @@ -81,6 +86,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  			return;
>  		}
>  	} else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) {
> +		input_sync(priv->input_dev);

I do not understand why this is needed, as sparse keymap should issue
sync for you. From sparse_keymap_report_entry():

	case KE_SW:
		value = ke->sw.value;
		/* fall through */

	case KE_VSW:
		input_report_switch(dev, ke->sw.code, value);
		input_sync(dev);

		^^^^^^^^^- here is the sync.

		break;
	}
}

>  		return;
>  	}
>  	dev_info(&device->dev, "unknown event index 0x%x\n", event);
> @@ -92,6 +98,7 @@ static int intel_vbtn_probe(struct platform_device *device)
>  	struct intel_vbtn_priv *priv;
>  	acpi_status status;
>  	int err;
> +	struct acpi_buffer vgbs_output = { ACPI_ALLOCATE_BUFFER, NULL };
>  
>  	status = acpi_evaluate_object(handle, "VBDL", NULL, NULL);
>  	if (ACPI_FAILURE(status)) {
> @@ -110,6 +117,27 @@ static int intel_vbtn_probe(struct platform_device *device)
>  		return err;
>  	}
>  
> +	status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output);
> +	/* VGBS being present and returning something means
> +	 * we have a tablet mode switch
> +	 */
> +	if (ACPI_SUCCESS(status)) {
> +		union acpi_object *obj = vgbs_output.pointer;
> +
> +		input_set_capability(priv->input_dev, EV_SW, SW_TABLET_MODE);
> +
> +		if (obj && obj->type == ACPI_TYPE_INTEGER) {

Do you want to set the capability if you do not get 'obj' or it is of
different type?

> +			if (obj->integer.value & TABLET_MODE_FLAG)
> +				input_report_switch(priv->input_dev,
> +							SW_TABLET_MODE,
> +							0);
> +			else
> +				input_report_switch(priv->input_dev,
> +							SW_TABLET_MODE,
> +							1);

Can be shorter:

			input_report_switch(priv->input_dev, SW_TABLET_MODE,
					obj->integer.value & TABLET_MODE_FLAG);

You also need:

			input_sync(priv->input_dev);

Thanks.

-- 
Dmitry

Re: [PATCH] Support intel-vbtn based tablet mode switch

[Marco Martin]

On Tue, Jan 23, 2018 at 12:46 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>> @@ -81,6 +86,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>>                       return;
>>               }
>>       } else if (sparse_keymap_report_event(priv->input_dev, event, 1, true)) {
>> +             input_sync(priv->input_dev);
>
> I do not understand why this is needed, as sparse keymap should issue
> sync for you. From sparse_keymap_report_entry():

indeed, I was testing on a 4.13 build, the sync there was pushed in
november only for 4.14, i've rebuilt it now on 4.14 and my sync is not
needed anymore

> Can be shorter:
>
>                         input_report_switch(priv->input_dev, SW_TABLET_MODE,
>                                         obj->integer.value & TABLET_MODE_FLAG);
>
> You also need:
>
>                         input_sync(priv->input_dev);

in v3 this shorter version and doesn't report the capability if it's
the wrong type

--
Marco Martin

[PATCH] Support intel-vbtn based tablet mode switch

[Marco Martin]

Some laptops such as Dell Inspiron 7000 series have the
tablet mode switch implemented in Intel ACPI,
the events to enter and exit the tablet mode are 0xCC and 0xCD

CC: platform-driver-x86@vger.kernel.org
CC: Matthew Garrett <mjg59@srcf.ucam.org>
CC: "Pali Rohár" <pali.rohar@gmail.com>
CC: Darren Hart <dvhart@infradead.org>
CC: Mario Limonciello <mario_limonciello@dell.com>
CC: Andy Shevchenko <andy@infradead.org>

Signed-off-by: Marco Martin <notmart@gmail.com>
---
 drivers/platform/x86/intel-vbtn.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 58c5ff3..64b4b34 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -26,6 +26,9 @@
 #include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 
+/* When NOT in tablet mode, VBDS has the flag 0x40 */
+#define TABLET_MODE_FLAG 0x40
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("AceLan Kao");
 
@@ -42,6 +45,8 @@ static const struct key_entry intel_vbtn_keymap[] = {
 	{ KE_IGNORE, 0xC5, { KEY_VOLUMEUP } },		/* volume-up key release */
 	{ KE_KEY, 0xC6, { KEY_VOLUMEDOWN } },		/* volume-down key press */
 	{ KE_IGNORE, 0xC7, { KEY_VOLUMEDOWN } },	/* volume-down key release */
+	{ KE_SW,  0xCC, { .sw = { SW_TABLET_MODE, 1 } } }, /* Tablet mode in */
+	{ KE_SW,  0xCD, { .sw = { SW_TABLET_MODE, 0 } } }, /* Tablet mode out */
 	{ KE_END },
 };
 
@@ -88,6 +93,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 
 static int intel_vbtn_probe(struct platform_device *device)
 {
+	struct acpi_buffer vgbs_output = { ACPI_ALLOCATE_BUFFER, NULL };
 	acpi_handle handle = ACPI_HANDLE(&device->dev);
 	struct intel_vbtn_priv *priv;
 	acpi_status status;
@@ -110,6 +116,21 @@ static int intel_vbtn_probe(struct platform_device *device)
 		return err;
 	}
 
+	status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output);
+	/* VGBS being present and returning something means
+	 * we have a tablet mode switch
+	 */
+	if (ACPI_SUCCESS(status)) {
+		union acpi_object *obj = vgbs_output.pointer;
+
+		if (obj && obj->type == ACPI_TYPE_INTEGER) {
+			input_set_capability(priv->input_dev, EV_SW, SW_TABLET_MODE);
+			input_report_switch(priv->input_dev,
+								SW_TABLET_MODE,
+								!(obj->integer.value & TABLET_MODE_FLAG));
+		}
+	}
+
 	status = acpi_install_notify_handler(handle,
 					     ACPI_DEVICE_NOTIFY,
 					     notify_handler,
-- 
2.7.4

[PATCH] Support intel-vbtn based tablet mode switch

[Marco Martin]

On some laptop like the Dell Inspiron 7000 series
tablet mode switch implemented in Intel ACPI,
the events to enter and exit the tablet mode are 0xCC and 0xCD

This initializes the tablet/laptop mode at the correct value
if the system booted in tablet mode (or the vbtn module loaded
with the device in tablet mode)

CC: platform-driver-x86@vger.kernel.org
CC: Matthew Garrett <mjg59@srcf.ucam.org>
CC: "Pali Rohár" <pali.rohar@gmail.com>
CC: Darren Hart <dvhart@infradead.org>
CC: Mario Limonciello <mario_limonciello@dell.com>
CC: Andy Shevchenko <andy@infradead.org>
CC: Stefan Brüns<stefan.bruens@rwth-aachen.de>

Signed-off-by: Marco Martin <notmart@gmail.com>

Reviewed-by: Mario Limonciello <mario.limonciello@dell.com>
Acked-by: Pali Rohár <pali.rohar@gmail.com>
---
 drivers/platform/x86/intel-vbtn.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/platform/x86/intel-vbtn.c b/drivers/platform/x86/intel-vbtn.c
index 5fc4315..019ff21 100644
--- a/drivers/platform/x86/intel-vbtn.c
+++ b/drivers/platform/x86/intel-vbtn.c
@@ -26,6 +26,9 @@
 #include <linux/suspend.h>
 #include <acpi/acpi_bus.h>
 
+/* When NOT in tablet mode, VGBS returns with the flag 0x40 */
+#define TABLET_MODE_FLAG 0x40
+
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("AceLan Kao");
 
@@ -108,6 +111,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
 
 static int intel_vbtn_probe(struct platform_device *device)
 {
+	struct acpi_buffer vgbs_output = { ACPI_ALLOCATE_BUFFER, NULL };
 	acpi_handle handle = ACPI_HANDLE(&device->dev);
 	struct intel_vbtn_priv *priv;
 	acpi_status status;
@@ -130,6 +134,22 @@ static int intel_vbtn_probe(struct platform_device *device)
 		return err;
 	}
 
+	status = acpi_evaluate_object(handle, "VGBS", NULL, &vgbs_output);
+	/* VGBS being present and returning something means
+	 * we have a tablet mode switch
+	 */
+	if (ACPI_SUCCESS(status)) {
+		union acpi_object *obj = vgbs_output.pointer;
+
+		if (obj && obj->type == ACPI_TYPE_INTEGER) {
+			input_report_switch(priv->input_dev,
+								SW_TABLET_MODE,
+								!(obj->integer.value & TABLET_MODE_FLAG));
+		}
+	}
+
+	kfree(vgbs_output.pointer);
+
 	status = acpi_install_notify_handler(handle,
 					     ACPI_DEVICE_NOTIFY,
 					     notify_handler,
-- 
2.7.4