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

Andy Shevchenko
2018-01-22T13:42:25Z

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