Re: [PATCH] platform/x86: Add Acer Wireless Radio Control driver
On Mon, Nov 20, 2017 at 8:24 PM, Andy Shevchenko
wrote:
> On Mon, Nov 20, 2017 at 8:31 AM, Chris Chiu wrote:
>> On Fri, Nov 17, 2017 at 10:25 PM, Andy Shevchenko
>> wrote:
>>> On Thu, Nov 16, 2017 at 3:44 PM, Chris Chiu wrote:
>
+
+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?
>>>
>>
>> But I think it would make life easier to have the following line in
>> .notify callback
>> struct acer_wireless_data * data = acpi_driver_data(adev);
>>
>> I can get its parent acer_wireless_data and point to input_dev easier.
>
> Yes, though if I'm not mistaken, you may get rid completely of your
> custom container and use struct input_dev for accessing that via
> to_acpi_device(idev->dev.parent).
>
> It would be less code, less data structures, cleaner view.
> Right?
>
+static int acer_wireless_remove(struct acpi_device *adev)
+{
+ return 0;
+}
+
>>>
>>> No need UI suppose.
>>>
>>
>> You mean remove this acer_wireless_remove which does nothing? Will do.
>
> Correct.
>
> --
> With Best Regards,
> Andy Shevchenko
I think I get your point. Will soon have a v2 patch later.
Re: [PATCH] platform/x86: Add Acer Wireless Radio Control driver
On Mon, Nov 20, 2017 at 8:31 AM, Chris Chiu wrote:
> On Fri, Nov 17, 2017 at 10:25 PM, Andy Shevchenko
> wrote:
>> On Thu, Nov 16, 2017 at 3:44 PM, Chris Chiu wrote:
>>> +
>>> +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?
>>
>
> But I think it would make life easier to have the following line in
> .notify callback
> struct acer_wireless_data * data = acpi_driver_data(adev);
>
> I can get its parent acer_wireless_data and point to input_dev easier.
Yes, though if I'm not mistaken, you may get rid completely of your
custom container and use struct input_dev for accessing that via
to_acpi_device(idev->dev.parent).
It would be less code, less data structures, cleaner view.
Right?
>>> +static int acer_wireless_remove(struct acpi_device *adev)
>>> +{
>>> + return 0;
>>> +}
>>> +
>>
>> No need UI suppose.
>>
>
> You mean remove this acer_wireless_remove which does nothing? Will do.
Correct.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] platform/x86: Add Acer Wireless Radio Control driver
On Fri, Nov 17, 2017 at 10:25 PM, Andy Shevchenko
wrote:
> On Thu, Nov 16, 2017 at 3:44 PM, Chris Chiu 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
>
>> +#include
>> +#include
>
> Either from that, not both.
>
>> +#include
>> +#include
>> +#include
>
>> +#include
>
> 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?
>
But I think it would make life easier to have the following line in
.notify callback
struct acer_wireless_data * data = acpi_driver_data(adev);
I can get its parent acer_wireless_data and point to input_dev easier.
>> +};
>> +
>> +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 & 0x;
>
> Or just hard code product ID for now.
>
Thanks. I will hard code the product ID for now with
data->idev->id.vendor = 0x1229;
>> +static int acer_wireless_remove(struct acpi_device *adev)
>> +{
>> + return 0;
>> +}
>> +
>
> No need UI suppose.
>
You mean remove this acer_wireless_remove which does nothing? Will do.
>> +MODULE_LICENSE("GPL");
>
> GPL v2 ?
>
> --
> With Best Regards,
> Andy Shevchenko
Re: [PATCH] platform/x86: Add Acer Wireless Radio Control driver
On Thu, Nov 16, 2017 at 3:44 PM, Chris Chiu 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
> +#include
> +#include
Either from that, not both.
> +#include
> +#include
> +#include
> +#include
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 & 0x;
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
