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: [email protected]
CC: Matthew Garrett <[email protected]>
CC: "Pali Rohár" <[email protected]>
CC: Darren Hart <[email protected]>
CC: Mario Limonciello <[email protected]>
CC: Andy Shevchenko <[email protected]>
Signed-off-by: Marco Martin <[email protected]>
---
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
On Mon, Jan 22, 2018 at 2:48 PM, Marco Martin <[email protected]> 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: [email protected]
> CC: Matthew Garrett <[email protected]>
> CC: "Pali Rohár" <[email protected]>
> CC: Darren Hart <[email protected]>
> CC: Mario Limonciello <[email protected]>
> CC: Andy Shevchenko <[email protected]>
>
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
On Mon, Jan 22, 2018 at 2:42 PM, Andy Shevchenko
<[email protected]> 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
On Mon, Jan 22, 2018 at 3:52 PM, Marco Martin <[email protected]> wrote:
> On Mon, Jan 22, 2018 at 2:42 PM, Andy Shevchenko
> <[email protected]> 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
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: [email protected]
> CC: Matthew Garrett <[email protected]>
> CC: "Pali Roh?r" <[email protected]>
> CC: Darren Hart <[email protected]>
> CC: Mario Limonciello <[email protected]>
> CC: Andy Shevchenko <[email protected]>
>
> Signed-off-by: Marco Martin <[email protected]>
> ---
> 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
On Tue, Jan 23, 2018 at 12:46 AM, Dmitry Torokhov
<[email protected]> 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