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

[Andy Shevchenko]

On Tue, Jan 5, 2016 at 3:26 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
>                         {
>                                 (...)
>                         }
>                 }
>         }

Few nitpicks below. In any case looks fine to me.

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

Hmmâ Usually we leave this for OSVs (OS Vendors) to decide. Or, like
you mentioned below, to the user.

> +       ---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.

âhere is inconsistency with default value.

> +
> +         If you choose to compile this driver as a module the module will be
> +         called asus-wireless.
> +


> +++ b/drivers/platform/x86/asus-wireless.c
> @@ -0,0 +1,83 @@
> +/*
> + * Asus Wireless Radio Control Driver
> + *
> + * Copyright (C) 2015-2016 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.
> + */
> +
> +#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>
> +
> +struct asus_wireless_data {
> +       struct input_dev *inputdev;

I would suggest
*inputdev -> *idev

> +};
> +
> +static void asus_wireless_notify(struct acpi_device *device, u32 event)

In ACPI code and drivers we usually use struct acpi_device *adev.

> +{
> +       struct asus_wireless_data *data = acpi_driver_data(device);
> +
> +       dev_dbg(&device->dev, "event=0x%X\n", event);
> +       if (event != 0x88) {
> +               dev_notice(&device->dev, "Unknown ASHS event: 0x%X\n", event);
> +               return;

I believe %#X will work in both cases instead of 0x%X

> +       }
> +       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;
> +
> +       data = devm_kzalloc(&device->dev, sizeof(*data), GFP_KERNEL);
> +       if (!data)
> +               return -ENOMEM;
> +       device->driver_data = data;
> +
> +       data->inputdev = devm_input_allocate_device(&device->dev);
> +       if (!data->inputdev)
> +               return -ENOMEM;
> +       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;
> +       set_bit(EV_KEY, data->inputdev->evbit);
> +       set_bit(KEY_RFKILL, data->inputdev->keybit);
> +       return input_register_device(data->inputdev);
> +}
> +
> +static int asus_wireless_remove(struct acpi_device *device)
> +{
> +       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 Radio Control Driver",
> +       .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 Radio Control Driver");
> +MODULE_AUTHOR("JoÃo Paulo Rechi Vita <jprvita@xxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> --
> 2.5.0
>



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