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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 22 Jan 2018 15:42:22 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Marco Martin <notmart@...il.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Matthew Garrett <mjg59@...f.ucam.org>,
        Pali Rohár <pali.rohar@...il.com>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Bhushan Shah <bhush94@...il.com>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Mario Limonciello <mario.limonciello@...l.com>,
        Mario Limonciello <mario_limonciello@...l.com>
Subject: Re: [PATCH] Support intel-vbtn based tablet mode switch

On Mon, Jan 22, 2018 at 2:48 PM, Marco Martin <notmart@...il.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@...r.kernel.org
> CC: Matthew Garrett <mjg59@...f.ucam.org>
> CC: "Pali Rohár" <pali.rohar@...il.com>
> CC: Darren Hart <dvhart@...radead.org>
> CC: Mario Limonciello <mario_limonciello@...l.com>
> CC: Andy Shevchenko <andy@...radead.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