Re: [PATCH] platform/x86: Add Acer Wireless Radio Control driver

[Andy Shevchenko]

On Thu, Nov 16, 2017 at 3:44 PM, Chris Chiu <chiu@xxxxxxxxxxxx> wrote:
> New Acer laptops in 2018 will have a separate ACPI device for
> notifications from the airplane mode hotkey. The device name in
> the DSDT is SMKB and its ACPI _HID is 10251229.
>
> For these models, when the airplane mode hotkey (Fn+F3) pressed,
> a query 0x02 is started in the Embedded Controller, and all this
> query does is a notify SMKB with the value 0x80.
>
>         Scope (_SB.PCI0.LPCB.EC0)
>         {
>                 (...)
>                 Method (_Q02, 0, NotSerialized)  // _Qxx: EC Query
>                 {
>                     HKEV (0x2, One)
>                     Notify (SMKB, 0x80) // Status Change
>                 }
>         }
>
> Based on code from asus-wireless

Thanks for the patch! Overall looks good, comment below.


> +#include <linux/kernel.h>

> +#include <linux/module.h>
> +#include <linux/init.h>

Either from that, not both.

> +#include <linux/types.h>
> +#include <linux/acpi.h>
> +#include <linux/input.h>

> +#include <linux/pci_ids.h>

Why do you need this one?
Okay, you are using Vendor ID from there.

Besides above, please keep them in alphabetical order.

> +
> +struct acer_wireless_data {
> +       struct input_dev *idev;

> +       struct acpi_device *adev;

Do you need this one?
I suppose whenever you need struct device out of it you may find it
via input->parent. Right?

> +};
> +
> +static const struct acpi_device_id device_ids[] = {

acer_wireless_acpi_ids

> +       {"10251229", 0},
> +       {"", 0},
> +};
> +MODULE_DEVICE_TABLE(acpi, device_ids);
> +
> +static void acer_wireless_notify(struct acpi_device *adev, u32 event)
> +{
> +       struct acer_wireless_data *data = acpi_driver_data(adev);
> +

> +       dev_dbg(&adev->dev, "event=%#x\n", event);

This makes sense after the check, otherwise you will get two messages in a row.

> +       if (event != 0x80) {
> +               dev_notice(&adev->dev, "Unknown SMKB event: %#x\n", event);
> +               return;
> +       }

> +       input_report_key(data->idev, KEY_RFKILL, 1);
> +       input_report_key(data->idev, KEY_RFKILL, 0);


> +       data->idev = devm_input_allocate_device(&adev->dev);
> +       if (!data->idev)
> +               return -ENOMEM;
> +       data->idev->name = "Acer Wireless Radio Control";
> +       data->idev->phys = "acer-wireless/input0";
> +       data->idev->id.bustype = BUS_HOST;
> +       data->idev->id.vendor = PCI_VENDOR_ID_AI;

Where is product ID?

I suppose you can use ACPI ID (which is basically PCI one in ACPI
table) and split it.

u32 id = simple_strtoul(...ACPI ID..., 16);

vendor = id >> 16;
product = id & 0xffff;

Or just hard code product ID for now.

> +static int acer_wireless_remove(struct acpi_device *adev)
> +{
> +       return 0;
> +}
> +

No need UI suppose.

> +MODULE_LICENSE("GPL");

GPL v2 ?

-- 
With Best Regards,
Andy Shevchenko