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

[Darren Hart <dvhart@infradead.org>]

On Sun, Dec 27, 2015 at 10:58:06PM +0200, Andy Shevchenko wrote:
> On Sun, Dec 27, 2015 at 9:21 PM, Weng Xuetian <wengxt@gmail.com> 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@gmail.com>
> 
> 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)

Sorry, replied before seeing this.

I agree that renaming the file is probably not necessary (this works on v3+ as I
understand it, so the name isn't really a problem, and dropping the 3 suggests
it works on v1 and v2).

I'd prefer to keep the name changing to a minimum, and add some information to
the Kconfig help and driver comments making it clear that this supports 3+.

--
Darren

> 
> > ---
> > 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@intel.com>
> >  L:     platform-driver-x86@vger.kernel.org
> >  S:     Supported
> > -F:     drivers/platform/x86/surfacepro3_button.c
> > +F:     drivers/platform/x86/surfacepro_button.c
> >
> >  MICROTEK X6 SCANNER
> >  M:     Oliver Neukum <oliver@neukum.org>
> > 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
> 

-- 
Darren Hart
Intel Open Source Technology Center