Messages in this thread | | | From | Andy Shevchenko <> | Date | Mon, 22 Jan 2018 15:42:22 +0200 | Subject | Re: [PATCH] Support intel-vbtn based tablet mode switch |
| |
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
| |