Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID

[JoÃo Paulo Rechi Vita]

On 27 January 2017 at 10:26, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Thu, Jan 26, 2017 at 5:00 PM, JoÃo Paulo Rechi Vita
> <jprvita@xxxxxxxxx> wrote:
>> Some Asus machines use 0x4/0x5 as their LED on/off values, while others use
>> 0x0/0x1, as shown in the DSDT exerpts below (note "Arg0 == 0x02", used to get
>
> excerpts
>

Thanks for catching that.

>> the LED status). Luckly it seems this behavior is tied to different HIDs, after
>
> Luckily
>

Ditto.

>> looking at 44 DSDTs from different Asus models.
>
>
>
>>
>> Another small difference (not shown here) is that a few machines call GWBL
>> instead of OWGS, and SWBL instead of OWGD. That does not seem to make a
>> difference for asus-wireless, and is a reason to not try to call these methods
>> directly.
>>
>> Device (ASHS)                       | Device (ASHS)
>> {                                   | {
>>     Name (_HID, "ATK4002")          |     Name (_HID, "ATK4001")
>>     Method (HSWC, 1, Serialized)    |     Method (HSWC, 1, Serialized)
>>     {                               |     {
>>         If ((Arg0 < 0x02))          |         If ((Arg0 < 0x02))
>>         {                           |         {
>>             OWGD (Arg0)             |             OWGD (Arg0)
>>             Return (One)            |             Return (One)
>>         }                           |         }
>>         If ((Arg0 == 0x02))         |
>>         {                           |         If ((Arg0 == 0x02))
>>             Local0 = OWGS ()        |         {
>>             If (Local0)             |             Return (OWGS ())
>>             {                       |         }
>>                     Return (0x05)   |
>>             }                       |         If ((Arg0 == 0x03))
>>             Else                    |         {
>>             {                       |             Return (0xFF)
>>                     Return (0x04)   |         }
>>             }                       |
>>         }                           |         If ((Arg0 == 0x80))
>>         If ((Arg0 == 0x03))         |         {
>>         {                           |            Return (One)
>>             Return (0xFF)           |         }
>>         }                           |     }
>>         If ((Arg0 == 0x04))         |     Method (_STA, 0, NotSerialized)
>>         {                           |     {
>>             OWGD (Zero)             |         If ((MSOS () >= OSW8))
>>             Return (One)            |         {
>>         }                           |             Return (0x0F)
>>         If ((Arg0 == 0x05))         |         }
>>         {                           |         Else
>>             OWGD (One)              |         {
>>             Return (One)            |             Return (Zero)
>>         }                           |         }
>>         If ((Arg0 == 0x80))         |     }
>>         {                           | }
>>             Return (One)            |
>>         }                           |
>>     }                               |
>>     Method (_STA, 0, NotSerialized) |
>>     {                               |
>>         If ((MSOS () >= OSW8))      |
>>         {                           |
>>             Return (0x0F)           |
>>         }                           |
>>         Else                        |
>>         {                           |
>>             Return (Zero)           |
>>         }                           |
>>     }                               |
>> }                                   |
>>
>> Signed-off-by: JoÃo Paulo Rechi Vita <jprvita@xxxxxxxxxxxx>
>> ---
>>  drivers/platform/x86/asus-wireless.c | 46 ++++++++++++++++++++++++++----------
>>  1 file changed, 33 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c
>> index c9b5ac152cf1..5a238fad35fb 100644
>> --- a/drivers/platform/x86/asus-wireless.c
>> +++ b/drivers/platform/x86/asus-wireless.c
>> @@ -18,18 +18,35 @@
>>  #include <linux/leds.h>
>>
>>  #define ASUS_WIRELESS_LED_STATUS 0x2
>> -#define ASUS_WIRELESS_LED_OFF 0x4
>> -#define ASUS_WIRELESS_LED_ON 0x5
>> +
>> +struct hswc_params {
>> +       u8 on;
>> +       u8 off;
>> +};
>>
>>  struct asus_wireless_data {
>>         struct input_dev *idev;
>>         struct acpi_device *adev;
>> +       const struct hswc_params *hswc_params;
>>         struct workqueue_struct *wq;
>>         struct work_struct led_work;
>>         struct led_classdev led;
>>         int led_state;
>>  };
>>
>> +/* LED ON/OFF values for different HIDs. Please update when adding new HIDs. */
>> +static const struct hswc_params id_params[] = {
>> +       { 0x0, 0x1 },
>> +       { 0x5, 0x4 },
>> +};
>
> Add status here as well.

Not really necessary, but who knows if that may also change in the
future. I'm fixing it for the next revision.

> Split to one struct per set.
>
>> +
>> +static const struct acpi_device_id device_ids[] = {
>> +       {"ATK4001", 0},
>> +       {"ATK4002", 0},
>
> ...and use it as a parameter here.
>

I'm not exactly sure how to do that, as driver_data is a
kernel_ulong_t. Can you please elaborate a bit more?

Thanks for the review,

--
JoÃo Paulo Rechi Vita
http://about.me/jprvita