2012/11/28 joeyli <jlee@xxxxxxxx>:
> 於 二，2012-11-27 於 17:55 +0200，Maxim Mikityanskiy 提到：
>> 2012/11/27 Corentin Chary <corentin.chary@xxxxxxxxx>:
>> > On Tue, Nov 27, 2012 at 8:14 AM, joeyli <jlee@xxxxxxxx> wrote:
>> >> Hi Maxim,
>> >>
>> >> 於 日，2012-11-25 於 00:29 +0200，Maxim Mikityanskiy 提到：
>> >>> Create a driver to support WMI found on MSI Wind laptops. This driver
>> >>> allows us to receive some additional keycodes for keys that are not
>> >>> handled without this driver.
>> >>>
>> >>> Signed-off-by: Maxim Mikityanskiy <maxtram95@xxxxxxxxx>
>> >>
>> >> I simply review this patch and looks there have many code the same with
>> >> msi-wmi.c. Does there have any reason didn't direct patch msi-wmi but
>> >> need generate a new driver?
>> >
>> > Right, if it's only a keymap and guid change, plus 1 or 2 quirks, it
>> > may makes sense to be in msi-wind.c
>>
>> Compare msi-wind-wmi.c and dell-wmi-aio.c. They look very similar,
>> even more similar than msi-wind-wmi.c and msi-wmi.c. Why do you
>> recommend to merge msi-wind-wmi features into msi-wmi, but not
>> dell-wmi-aio?
>
> Because MSI and DELL are different manufacturers. For the similar code
> between different manufacturers we should extract those similar code to
> acpi wmi or platform framework.
>
>>
>> Look at dell-wmi.c, dell-wmi-aio.c and msi-wmi.c. They have lots of
>> common code and they differ only in GUIDs, keymap tables and some
>> quirks, but they are still different modules.
>
> Yes, because different manufacturers. For those common code need extract
> to framework.
>
>>
>> msi-wind-wmi is WMI driver that only supports hotkey handling. Each
>> WMI driver providing this feature contains such code, so there is
>> nothing strange in that msi-wmi driver also contains such code.
>> msi-wmi is a driver for fully different laptop with fully different
>> WMI. It also handles backlight brightness and needs a quirk for hotkey
>> handling. MSI Wind WMI does not send brightness change events and does
>> not need that quirk.
>>
>> Do you still think I should merge msi-wind-wmi code into msi-wmi?
>
> Yes, your contribution is very good for MSI wind user, but I still think
> it's not worth to create a new driver for a small series.
>
> In Kconfig, it separate different platform driver by manufacturers, then
> the drivers of the same manufacturer are separate to different interface
> type (platform or wmi), or different hardware series like Laptop and
> AIO.
>
> Current help of MSI_WMI in Kconfig is:
> help
> Say Y here if you want to support WMI-based hotkeys on MSI laptops.
>
> Per this define, MSI wind series is undoubted a subset of MSI laptops.
>
> We can say the MSI wind is a series that should separate to different driver,
> but I don't think MSI wind series is a big and complex series that worth to do that.
Okay, okay, if you really want to preserve one vendor - one driver
logic, I can make a patch for msi-wmi, but I think it will lead to
complication of code, lots of ifs and lots of code unused by U90/U100
but loaded in RAM.
>
> Thanks a lot!
> Joey Lee
>
>>
>> >
>> >>
>> >> Thanks a lot!
>> >> Joey Lee
>> >>
>> >>> ---
>> >>> drivers/platform/x86/Kconfig | 13 +++
>> >>> drivers/platform/x86/Makefile | 1 +
>> >>> drivers/platform/x86/msi-wind-wmi.c | 169 ++++++++++++++++++++++++++++++++++++
>> >>> 3 files changed, 183 insertions(+)
>> >>> create mode 100644 drivers/platform/x86/msi-wind-wmi.c
>> >>>
>> >>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>> >>> index c86bae8..46f269e5 100644
>> >>> --- a/drivers/platform/x86/Kconfig
>> >>> +++ b/drivers/platform/x86/Kconfig
>> >>> @@ -561,6 +561,19 @@ config MSI_WMI
>> >>> To compile this driver as a module, choose M here: the module will
>> >>> be called msi-wmi.
>> >>>
>> >>> +config MSI_WIND_WMI
>> >>> + tristate "MSI Wind WMI Driver"
>> >>> + depends on ACPI_WMI
>> >>> + depends on INPUT
>> >>> + select INPUT_SPARSEKMAP
>> >>> + ---help---
>> >>> + Say Y here if you want to support WMI-based hotkeys on MSI Wind
>> >>> + laptops. MSI Wind WMI differs from WMI found on other MSI laptops, so
>> >>> + say Y here if you have MSI Wind, otherwise select "MSI WMI extras".
>> >>> +
>> >>> + To compile this driver as a module, choose M here: the module will
>> >>> + be called msi-wind-wmi.
>> >>> +
>> >>> config TOPSTAR_LAPTOP
>> >>> tristate "Topstar Laptop Extras"
>> >>> depends on ACPI
>> >>> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
>> >>> index bf7e4f9..44389c0 100644
>> >>> --- a/drivers/platform/x86/Makefile
>> >>> +++ b/drivers/platform/x86/Makefile
>> >>> @@ -30,6 +30,7 @@ obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o
>> >>> obj-$(CONFIG_ACPI_WMI) += wmi.o
>> >>> obj-$(CONFIG_MSI_WMI) += msi-wmi.o
>> >>> obj-$(CONFIG_TOPSTAR_LAPTOP) += topstar-laptop.o
>> >>> +obj-$(CONFIG_MSI_WIND_WMI) += msi-wind-wmi.o
>> >>>
>> >>> # toshiba_acpi must link after wmi to ensure that wmi devices are found
>> >>> # before toshiba_acpi initializes
>> >>> diff --git a/drivers/platform/x86/msi-wind-wmi.c b/drivers/platform/x86/msi-wind-wmi.c
>> >>> new file mode 100644
>> >>> index 0000000..4f19434
>> >>> --- /dev/null
>> >>> +++ b/drivers/platform/x86/msi-wind-wmi.c
>> >>> @@ -0,0 +1,169 @@
>> >>> +/*
>> >>> + * MSI Wind WMI hotkeys
>> >>> + *
>> >>> + * Copyright (C) 2012 Maxim Mikityanskiy <maxtram95@xxxxxxxxx>
>> >>> + *
>> >>> + * This program is free software; you can redistribute it and/or modify
>> >>> + * it under the terms of the GNU General Public License as published by
>> >>> + * the Free Software Foundation; either version 2 of the License, or
>> >>> + * (at your option) any later version.
>> >>> + *
>> >>> + * This program is distributed in the hope that it will be useful,
>> >>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> >>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> >>> + * GNU General Public License for more details.
>> >>> + *
>> >>> + * You should have received a copy of the GNU General Public License
>> >>> + * along with this program; if not, write to the Free Software
>> >>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> >>> + */
>> >>> +
>> >>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> >>> +
>> >>> +#include <linux/module.h>
>> >>> +#include <linux/input.h>
>> >>> +#include <linux/input/sparse-keymap.h>
>> >>> +#include <linux/acpi.h>
>> >>> +
>> >>> +MODULE_AUTHOR("Maxim Mikityanskiy <maxtram95@xxxxxxxxx>");
>> >>> +MODULE_DESCRIPTION("MSI Wind laptop WMI hotkeys driver");
>> >>> +MODULE_LICENSE("GPL");
>> >>> +
>> >>> +#define WMI_EVENT_GUID "5B3CC38A-40D9-7245-8AE6-1145B751BE3F"
>> >>> +
>> >>> +MODULE_ALIAS("wmi:" WMI_EVENT_GUID);
>> >>> +
>> >>> +/* Fn+F3 touchpad toggle */
>> >>> +#define WIND_KEY_TOUCHPAD 0x08
>> >>> +/* Fn+F11 Bluetooth toggle */
>> >>> +#define WIND_KEY_BLUETOOTH 0x56
>> >>> +/* Fn+F6 webcam toggle */
>> >>> +#define WIND_KEY_CAMERA 0x57
>> >>> +/* Fn+F11 Wi-Fi toggle */
>> >>> +#define WIND_KEY_WLAN 0x5f
>> >>> +/* Fn+F10 turbo mode toggle */
>> >>> +#define WIND_KEY_TURBO 0x60
>> >>> +/* Fn+F10 ECO mode toggle */
>> >>> +#define WIND_KEY_ECO 0x69
>> >>> +
>> >>> +static struct key_entry wind_keymap[] = {
>> >>> + /* These keys work without WMI. Ignore them to avoid double keycodes */
>> >>> + { KE_IGNORE, WIND_KEY_TOUCHPAD, { KEY_TOUCHPAD_TOGGLE } },
>> >>> + { KE_IGNORE, WIND_KEY_BLUETOOTH, { KEY_BLUETOOTH } },
>> >>> + { KE_IGNORE, WIND_KEY_CAMERA, { KEY_CAMERA } },
>> >>> + { KE_IGNORE, WIND_KEY_WLAN, { KEY_WLAN } },
>> >>> + /* These are keys that should be handled via WMI */
>> >>> + { KE_KEY, WIND_KEY_TURBO, { KEY_PROG1 } },
>> >>> + { KE_KEY, WIND_KEY_ECO, { KEY_PROG2 } },
>> >>> + { KE_END, 0 }
>> >>> +};
>> >>> +
>> >>> +static struct input_dev *wind_input_dev;
>> >>> +
>> >>> +static void wind_wmi_notify(u32 value, void *context)
>> >>> +{
>> >>> + struct acpi_buffer response = {
>> >>> + .length = ACPI_ALLOCATE_BUFFER,
>> >>> + .pointer = NULL
>> >>> + };
>> >>> + acpi_status status = wmi_get_event_data(value, &response);
>> >>> + union acpi_object *obj = response.pointer;
>> >>> + int code;
>> >>> +
>> >>> + if (status != AE_OK) {
>> >>> + pr_warn("Bad event status %#x\n", status);
>> >>> + return;
>> >>> + }
>> >>> +
>> >>> + if (!obj || obj->type != ACPI_TYPE_INTEGER) {
>> >>> + pr_warn("Unknown event received\n");
>> >>> + goto wind_wmi_notify_free;
>> >>> + }
>> >>> +
>> >>> + code = obj->integer.value;
>> >>> + pr_debug("Event code: %#x\n", code);
>> >>> +
>> >>> + if (code == 0x00 || code == 0x62 || code == 0x63) {
>> >>> + /* Unknown events - drop them for now */
>> >>> + goto wind_wmi_notify_free;
>> >>> + }
>> >>> +
>> >>> + if (!sparse_keymap_report_event(wind_input_dev, code, 1, true))
>> >>> + pr_warn("Unknown key %#x pressed\n", code);
>> >>> +
>> >>> +wind_wmi_notify_free:
>> >>> + kfree(response.pointer);
>> >>> +}
>> >>> +
>> >>> +static int __init wind_input_setup(void)
>> >>> +{
>> >>> + int err;
>> >>> +
>> >>> + wind_input_dev = input_allocate_device();
>> >>> + if (!wind_input_dev)
>> >>> + return -ENOMEM;
>> >>> +
>> >>> + wind_input_dev->name = "MSI WMI hotkeys";
>> >>> + wind_input_dev->phys = "wmi/input0";
>> >>> + wind_input_dev->id.bustype = BUS_HOST;
>> >>> +
>> >>> + err = sparse_keymap_setup(wind_input_dev, wind_keymap, NULL);
>> >>> + if (err)
>> >>> + goto wind_input_setup_free_dev;
>> >>> +
>> >>> + err = input_register_device(wind_input_dev);
>> >>> + if (err)
>> >>> + goto wind_input_setup_free_keymap;
>> >>> +
>> >>> + return 0;
>> >>> +
>> >>> +wind_input_setup_free_keymap:
>> >>> + sparse_keymap_free(wind_input_dev);
>> >>> +wind_input_setup_free_dev:
>> >>> + input_free_device(wind_input_dev);
>> >>> + return err;
>> >>> +}
>> >>> +
>> >>> +static int __init wind_wmi_init(void)
>> >>> +{
>> >>> + int err;
>> >>> +
>> >>> + if (!wmi_has_guid(WMI_EVENT_GUID)) {
>> >>> + pr_err("No MSI Wind WMI found\n");
>> >>> + return -ENODEV;
>> >>> + }
>> >>> +
>> >>> + err = wind_input_setup();
>> >>> + if (err) {
>> >>> + pr_err("Unable to setup input device\n");
>> >>> + return err;
>> >>> + }
>> >>> +
>> >>> + if (ACPI_FAILURE(wmi_install_notify_handler(WMI_EVENT_GUID,
>> >>> + wind_wmi_notify, NULL))) {
>> >>> + pr_err("Unable to install WMI notify handler\n");
>> >>> + err = -EIO;
>> >>> + goto wind_wmi_init_unregister_input;
>> >>> + }
>> >>> +
>> >>> + pr_debug("Event handler installed\n");
>> >>> +
>> >>> + return 0;
>> >>> +
>> >>> +wind_wmi_init_unregister_input:
>> >>> + input_unregister_device(wind_input_dev);
>> >>> + sparse_keymap_free(wind_input_dev);
>> >>> + input_free_device(wind_input_dev);
>> >>> + return err;
>> >>> +}
>> >>> +
>> >>> +static void __exit wind_wmi_exit(void)
>> >>> +{
>> >>> + wmi_remove_notify_handler(WMI_EVENT_GUID);
>> >>> + input_unregister_device(wind_input_dev);
>> >>> + sparse_keymap_free(wind_input_dev);
>> >>> + input_free_device(wind_input_dev);
>> >>> +}
>> >>> +
>> >>> +module_init(wind_wmi_init);
>> >>> +module_exit(wind_wmi_exit);
>> >>
>> >>
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
>> >> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> >
>> >
>> >
>> > --
>> > Corentin Chary
>> > http://xf.iksaif.net
>>
>
>
--
To unsubscribe from this list: send the line "unsubscribe platform-driver-x86" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
