| 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
| ||
|
Date: Mon, 4 Jan 2016 12:35:59 -0800
From: Darren Hart <dvhart@...radead.org>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Weng Xuetian <wengxt@...il.com>, Chen Yu <yu.c.chen@...el.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v3] surface pro 4: Add support for Surface Pro 4 Buttons
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@...il.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@...il.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@...el.com>
> > L: platform-driver-x86@...r.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@...kum.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists