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

[Andy Shevchenko]

On Mon, Jan 22, 2018 at 2:48 PM, Marco Martin <notmart@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> CC: Matthew Garrett <mjg59@xxxxxxxxxxxxx>
> CC: "Pali RohÃr" <pali.rohar@xxxxxxxxx>
> CC: Darren Hart <dvhart@xxxxxxxxxxxxx>
> CC: Mario Limonciello <mario_limonciello@xxxxxxxx>
> CC: Andy Shevchenko <andy@xxxxxxxxxxxxx>
>

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