Re: [PATCH 1/4] platform/x86: Add Asus Wireless Radio Control driver

From: Andy Shevchenko
Date: Sun Dec 27 2015 - 08:21:59 EST

On Sat, Dec 26, 2015 at 4:56 PM, JoÃo Paulo Rechi Vita
<jprvita@xxxxxxxxx> wrote:
> Some Asus notebooks like the Asus E202SA and the Asus X555UB have a
> separate ACPI device for notifications from the airplane mode hotkey.
> This device is called "Wireless Radio Control" in Asus websites and ASHS
> in the DSDT, and its ACPI _HID is ATK4002 in the two models mentioned
> above.
>
> For these models, when the airplane mode hotkey (Fn+F2) is pressed, a
> query 0x0B is started in the Embedded Controller, and all this query does
> is a notify ASHS with the value 0x88 (for acpi_osi >= "Windows 2012"):
>
> Scope (_SB.PCI0.SBRG.EC0)
> {
> (...)
> Method (_Q0B, 0, NotSerialized) // _Qxx: EC Query
> {
> If ((MSOS () >= OSW8))
> {
> Notify (ASHS, 0x88) // Device-Specific
> }
> Else
> {
> (...)
> }
> }
> }
>
> Signed-off-by: JoÃo Paulo Rechi Vita <jprvita@xxxxxxxxxxxx>
> ---
> MAINTAINERS | 6 ++
> drivers/platform/x86/Kconfig | 15 +++++
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/asus-wireless.c | 105 +++++++++++++++++++++++++++++++++++
> 4 files changed, 127 insertions(+)
> create mode 100644 drivers/platform/x86/asus-wireless.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c984136..e1860f2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1791,6 +1791,12 @@ S: Maintained
> F: drivers/platform/x86/asus*.c
> F: drivers/platform/x86/eeepc*.c
>
> +ASUS WIRELESS RADIO CONTROL DRIVER
> +M: JoÃo Paulo Rechi Vita <jprvita@xxxxxxxxx>
> +L: platform-driver-x86@xxxxxxxxxxxxxxx
> +S: Maintained
> +F: drivers/platform/x86/asus-wireless.c
> +
> ASYNCHRONOUS TRANSFERS/TRANSFORMS (IOAT) API
> R: Dan Williams <dan.j.williams@xxxxxxxxx>
> W: http://sourceforge.net/projects/xscaleiop
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index d0bfcf8..d3a088b 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -587,6 +587,21 @@ config EEEPC_WMI
> If you have an ACPI-WMI compatible Eee PC laptop (>= 1000), say Y or M
> here.
>
> +config ASUS_WIRELESS
> + tristate "Asus Wireless Radio Control Driver"
> + depends on ACPI
> + depends on INPUT
> + default m
> + ---help---
> + The Asus Wireless Radio Control handles the airplane mode hotkey
> + present on some Asus laptops.
> +
> + Say Y or M here if you have an ASUS notebook with an airplane mode
> + hotkey.
> +
> + If you choose to compile this driver as a module the module will be
> + called asus-wireless.
> +
> config ACPI_WMI
> tristate "WMI"
> depends on ACPI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 4410e91..8b8df29 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -5,6 +5,7 @@
> obj-$(CONFIG_ASUS_LAPTOP) += asus-laptop.o
> obj-$(CONFIG_ASUS_WMI) += asus-wmi.o
> obj-$(CONFIG_ASUS_NB_WMI) += asus-nb-wmi.o
> +obj-$(CONFIG_ASUS_WIRELESS) += asus-wireless.o
> obj-$(CONFIG_EEEPC_LAPTOP) += eeepc-laptop.o
> obj-$(CONFIG_EEEPC_WMI) += eeepc-wmi.o
> obj-$(CONFIG_MSI_LAPTOP) += msi-laptop.o
> diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c
> new file mode 100644
> index 0000000..ef7dba5
> --- /dev/null
> +++ b/drivers/platform/x86/asus-wireless.c
> @@ -0,0 +1,105 @@
> +/*
> + * Asus Wireless Radio Control Driver
> + *
> + * Copyright (C) 2015 Endless Mobile, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/acpi.h>
> +#include <linux/input.h>

> +#include <linux/pci_ids.h>

Didn't notice exatcly how this one is used.

> +
> +#define ASUS_WIRELESS_MODULE_NAME "Asus Wireless Radio Control Driver"
> +
> +struct asus_wireless_data {
> + struct input_dev *inputdev;
> +};
> +
> +static void asus_wireless_notify(struct acpi_device *device, u32 event)
> +{
> + struct asus_wireless_data *data = acpi_driver_data(device);
> +
> + pr_debug("event=0x%X\n", event);

dev_, please.

> + if (event != 0x88) {
> + pr_info("Unknown ASHS event: 0x%X\n", event);

Ditto. Moreover, this is apparently not an info, I can put it either
to debug or to warn depending how much those I produced in erroneous
state.

> + return;
> + }
> + input_report_key(data->inputdev, KEY_RFKILL, 1);
> + input_report_key(data->inputdev, KEY_RFKILL, 0);
> + input_sync(data->inputdev);
> +}
> +
> +static int asus_wireless_add(struct acpi_device *device)
> +{
> + struct asus_wireless_data *data;
> + int err = -ENOMEM;
> +

> + pr_info(ASUS_WIRELESS_MODULE_NAME"\n");

Kinda useless,

> + data = kzalloc(sizeof(*data), GFP_KERNEL);

devm_kzalloc();

> + if (!data)
> + return -ENOMEM;
> + device->driver_data = data;
> +
> + data->inputdev = input_allocate_device();

Use devm_*

> + if (!data->inputdev)
> + goto fail;
> +
> + data->inputdev->name = "Asus Wireless Radio Control";
> + data->inputdev->phys = "asus-wireless/input0";
> + data->inputdev->id.bustype = BUS_HOST;
> + data->inputdev->id.vendor = PCI_VENDOR_ID_ASUSTEK;
> + data->inputdev->dev.parent = &device->dev;
> + set_bit(EV_REP, data->inputdev->evbit);
> + set_bit(KEY_RFKILL, data->inputdev->keybit);
> +
> + err = input_register_device(data->inputdev);
> + if (err)
> + goto fail;
> + return 0;
> +
> +fail:
> + device->driver->ops.remove(device);

Better to show explicitly what you do here. Currently I might say it's
a strange way I've ever seen to express the error path.

> + return err;
> +}
> +
> +static int asus_wireless_remove(struct acpi_device *device)
> +{
> + struct asus_wireless_data *data = acpi_driver_data(device);
> +

> + pr_info("Removing "ASUS_WIRELESS_MODULE_NAME"\n");

Kinda useless.

> + if (data->inputdev)

How is it possible?

> + input_unregister_device(data->inputdev);

Redundant after switching to devm_

> + kfree(data);

Ditto.

> + return 0;
> +}
> +
> +static const struct acpi_device_id device_ids[] = {
> + {"ATK4002", 0},
> + {"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, device_ids);
> +
> +static struct acpi_driver asus_wireless_driver = {
> + .name = ASUS_WIRELESS_MODULE_NAME,
> + .class = "hotkey",
> + .ids = device_ids,
> + .ops = {
> + .add = asus_wireless_add,
> + .remove = asus_wireless_remove,
> + .notify = asus_wireless_notify,
> + },
> +};
> +module_acpi_driver(asus_wireless_driver);
> +
> +MODULE_DESCRIPTION(ASUS_WIRELESS_MODULE_NAME);
> +MODULE_AUTHOR("JoÃo Paulo Rechi Vita <jprvita@xxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> --
> 2.5.0
>
> --
> 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/



--
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/