Re: [PATCH v3] surface pro 4: Add support for Surface Pro 4 Buttons

[Andy Shevchenko]

On Sun, Dec 27, 2015 at 9:21 PM, Weng Xuetian <wengxt@xxxxxxxxx> wrote:
> Surface Pro 4 buttons are managed by a device with _HID "MSHW0040"
> different from Surface Pro 3.
>
> This commit adds MSHW0040 to id list to support the Surface Pro 4, and
> renames the driver to surfacepro_button accordingly.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=109871
> Signed-off-by: Weng Xuetian <wengxt@xxxxxxxxx>

Darren, this one looks fine for me, still couple of question up to you.

First is do we really want to rename driver? (Renaming variables and
stuff like I said if fine to me)

> ---
> v3:
>  - Fix commit message grammar mistakes.
> v2:
>  - Reformat patch with -M -C
> ---
>  MAINTAINERS                                            |  4 ++--
>  drivers/platform/x86/Kconfig                           |  6 +++---
>  drivers/platform/x86/Makefile                          |  2 +-
>  .../x86/{surfacepro3_button.c => surfacepro_button.c}  | 18 ++++++++++--------
>  4 files changed, 16 insertions(+), 14 deletions(-)
>  rename drivers/platform/x86/{surfacepro3_button.c => surfacepro_button.c} (93%)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 233f834..1c07436 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7019,11 +7019,11 @@ T:      git git://git.monstr.eu/linux-2.6-microblaze.git
>  S:     Supported
>  F:     arch/microblaze/
>
> -MICROSOFT SURFACE PRO 3 BUTTON DRIVER
> +MICROSOFT SURFACE PRO SERIES BUTTON DRIVER
>  M:     Chen Yu <yu.c.chen@xxxxxxxxx>
>  L:     platform-driver-x86@xxxxxxxxxxxxxxx
>  S:     Supported
> -F:     drivers/platform/x86/surfacepro3_button.c
> +F:     drivers/platform/x86/surfacepro_button.c
>
>  MICROTEK X6 SCANNER
>  M:     Oliver Neukum <oliver@xxxxxxxxxx>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 1089eaa..3358fb0 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -939,9 +939,9 @@ config INTEL_PMC_IPC
>         The PMC is an ARC processor which defines IPC commands for communication
>         with other entities in the CPU.
>
> -config SURFACE_PRO3_BUTTON
> -       tristate "Power/home/volume buttons driver for Microsoft Surface Pro 3 tablet"
> +config SURFACE_PRO_BUTTON
> +       tristate "Power/home/volume buttons driver for Microsoft Surface Pro Series tablet"
>         depends on ACPI && INPUT
>         ---help---
> -         This driver handles the power/home/volume buttons on the Microsoft Surface Pro 3 tablet.
> +         This driver handles the power/home/volume buttons on the Microsoft Surface Pro Series tablet.
>  endif # X86_PLATFORM_DEVICES
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 3ca78a3..b4ece33 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -61,4 +61,4 @@ obj-$(CONFIG_INTEL_SMARTCONNECT)      += intel-smartconnect.o
>  obj-$(CONFIG_PVPANIC)           += pvpanic.o
>  obj-$(CONFIG_ALIENWARE_WMI)    += alienware-wmi.o
>  obj-$(CONFIG_INTEL_PMC_IPC)    += intel_pmc_ipc.o
> -obj-$(CONFIG_SURFACE_PRO3_BUTTON)      += surfacepro3_button.o
> +obj-$(CONFIG_SURFACE_PRO_BUTTON)       += surfacepro_button.o
> diff --git a/drivers/platform/x86/surfacepro3_button.c b/drivers/platform/x86/surfacepro_button.c
> similarity index 93%
> rename from drivers/platform/x86/surfacepro3_button.c
> rename to drivers/platform/x86/surfacepro_button.c
> index f7dade3..cda52b8 100644
> --- a/drivers/platform/x86/surfacepro3_button.c
> +++ b/drivers/platform/x86/surfacepro_button.c
> @@ -1,6 +1,6 @@
>  /*
>   * power/home/volume button support for
> - * Microsoft Surface Pro 3 tablet.
> + * Microsoft Surface Pro Series tablet.
>   *
>   * Copyright (c) 2015 Intel Corporation.
>   * All rights reserved.
> @@ -19,9 +19,10 @@
>  #include <linux/acpi.h>
>  #include <acpi/button.h>
>
> -#define SURFACE_BUTTON_HID             "MSHW0028"
> +#define SURFACE_PRO3_BUTTON_HID                "MSHW0028"
> +#define SURFACE_PRO4_BUTTON_HID                "MSHW0040"
>  #define SURFACE_BUTTON_OBJ_NAME                "VGBI"
> -#define SURFACE_BUTTON_DEVICE_NAME     "Surface Pro 3 Buttons"
> +#define SURFACE_BUTTON_DEVICE_NAME     "Surface Pro Series Buttons"
>
>  #define SURFACE_BUTTON_NOTIFY_PRESS_POWER      0xc6
>  #define SURFACE_BUTTON_NOTIFY_RELEASE_POWER    0xc7
> @@ -35,10 +36,10 @@
>  #define SURFACE_BUTTON_NOTIFY_PRESS_VOLUME_DOWN        0xc2
>  #define SURFACE_BUTTON_NOTIFY_RELEASE_VOLUME_DOWN      0xc3
>
> -ACPI_MODULE_NAME("surface pro 3 button");
> +ACPI_MODULE_NAME("surface pro series button");
>
>  MODULE_AUTHOR("Chen Yu");
> -MODULE_DESCRIPTION("Surface Pro3 Button Driver");
> +MODULE_DESCRIPTION("Surface Pro Series Button Driver");
>  MODULE_LICENSE("GPL v2");
>
>  /*
> @@ -54,7 +55,8 @@ MODULE_LICENSE("GPL v2");
>   * acpi_driver.
>   */
>  static const struct acpi_device_id surface_button_device_ids[] = {
> -       {SURFACE_BUTTON_HID,    0},
> +       {SURFACE_PRO3_BUTTON_HID,    0},
> +       {SURFACE_PRO4_BUTTON_HID,    0},
>         {"", 0},
>  };
>  MODULE_DEVICE_TABLE(acpi, surface_button_device_ids);
> @@ -202,8 +204,8 @@ static SIMPLE_DEV_PM_OPS(surface_button_pm,
>                 surface_button_suspend, surface_button_resume);
>
>  static struct acpi_driver surface_button_driver = {
> -       .name = "surface_pro3_button",
> -       .class = "SurfacePro3",
> +       .name = "surface_pro_button",
> +       .class = "SurfacePro",

So, beside the driver renaming I don't know the side effect of
renaming .class field here.

>         .ids = surface_button_device_ids,
>         .ops = {
>                 .add = surface_button_add,

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/