* [PATCH 0/1] asus-wireless: Use the correct HSWC parameter for each HID @ 2017-01-26 15:00 João Paulo Rechi Vita 2017-01-26 15:00 ` [PATCH 1/1] " João Paulo Rechi Vita 0 siblings, 1 reply; 10+ messages in thread From: João Paulo Rechi Vita @ 2017-01-26 15:00 UTC (permalink / raw) To: Darren Hart Cc: platform-driver-x86, acpi4asus-user, linux-kernel, linux, João Paulo Rechi Vita As stated in the commit message, different machines can use different values to represent LED ON/OFF, and this commit makes sure we use the correct value pair for each _HID. I don't know if there is a better way to store these pairs than in a structure matching the device_ids[] indexing that would avoid looping through device_ids[] in asus_wireless_add(). I'm happy to try suggestions to improve that. João Paulo Rechi Vita (1): asus-wireless: Use the correct HSWC parameter for each HID drivers/platform/x86/asus-wireless.c | 46 ++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 13 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID 2017-01-26 15:00 [PATCH 0/1] asus-wireless: Use the correct HSWC parameter for each HID João Paulo Rechi Vita @ 2017-01-26 15:00 ` João Paulo Rechi Vita 2017-01-27 15:26 ` Andy Shevchenko 0 siblings, 1 reply; 10+ messages in thread From: João Paulo Rechi Vita @ 2017-01-26 15:00 UTC (permalink / raw) To: Darren Hart Cc: platform-driver-x86, acpi4asus-user, linux-kernel, linux, João Paulo Rechi Vita 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 the LED status). Luckly it seems this behavior is tied to different HIDs, after 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@endlessm.com> --- 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 }, +}; + +static const struct acpi_device_id device_ids[] = { + {"ATK4001", 0}, + {"ATK4002", 0}, + {"", 0}, +}; +MODULE_DEVICE_TABLE(acpi, device_ids); + static u64 asus_wireless_method(acpi_handle handle, const char *method, int param) { @@ -62,7 +79,7 @@ static enum led_brightness led_state_get(struct led_classdev *led) data = container_of(led, struct asus_wireless_data, led); s = asus_wireless_method(acpi_device_handle(data->adev), "HSWC", ASUS_WIRELESS_LED_STATUS); - if (s == ASUS_WIRELESS_LED_ON) + if (s == data->hswc_params->on) return LED_FULL; return LED_OFF; } @@ -82,8 +99,8 @@ static void led_state_set(struct led_classdev *led, struct asus_wireless_data *data; data = container_of(led, struct asus_wireless_data, led); - data->led_state = value == LED_OFF ? ASUS_WIRELESS_LED_OFF : - ASUS_WIRELESS_LED_ON; + data->led_state = value == LED_OFF ? data->hswc_params->off : + data->hswc_params->on; queue_work(data->wq, &data->led_work); } @@ -104,13 +121,22 @@ static void asus_wireless_notify(struct acpi_device *adev, u32 event) static int asus_wireless_add(struct acpi_device *adev) { struct asus_wireless_data *data; - int err; + const char *hid; + int err, i; data = devm_kzalloc(&adev->dev, sizeof(*data), GFP_KERNEL); if (!data) return -ENOMEM; adev->driver_data = data; + hid = acpi_device_hid(adev); + for (i = 0; strcmp(device_ids[i].id, ""); i++) { + if (!strcmp(device_ids[i].id, hid)) { + data->hswc_params = &id_params[i]; + break; + } + } + data->idev = devm_input_allocate_device(&adev->dev); if (!data->idev) return -ENOMEM; @@ -137,6 +163,7 @@ static int asus_wireless_add(struct acpi_device *adev) err = devm_led_classdev_register(&adev->dev, &data->led); if (err) destroy_workqueue(data->wq); + return err; } @@ -149,13 +176,6 @@ static int asus_wireless_remove(struct acpi_device *adev) return 0; } -static const struct acpi_device_id device_ids[] = { - {"ATK4001", 0}, - {"ATK4002", 0}, - {"", 0}, -}; -MODULE_DEVICE_TABLE(acpi, device_ids); - static struct acpi_driver asus_wireless_driver = { .name = "Asus Wireless Radio Control Driver", .class = "hotkey", -- 2.11.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID 2017-01-26 15:00 ` [PATCH 1/1] " João Paulo Rechi Vita @ 2017-01-27 15:26 ` Andy Shevchenko 2017-02-01 12:20 ` João Paulo Rechi Vita 0 siblings, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2017-01-27 15:26 UTC (permalink / raw) To: João Paulo Rechi Vita Cc: Darren Hart, Platform Driver, acpi4asus-user, linux-kernel, linux, João Paulo Rechi Vita On Thu, Jan 26, 2017 at 5:00 PM, João Paulo Rechi Vita <jprvita@gmail.com> 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 > the LED status). Luckly it seems this behavior is tied to different HIDs, after Luckily > 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@endlessm.com> > --- > 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. 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. > + {"", 0}, > +}; > +MODULE_DEVICE_TABLE(acpi, device_ids); > + > static u64 asus_wireless_method(acpi_handle handle, const char *method, > int param) > { > @@ -62,7 +79,7 @@ static enum led_brightness led_state_get(struct led_classdev *led) > data = container_of(led, struct asus_wireless_data, led); > s = asus_wireless_method(acpi_device_handle(data->adev), "HSWC", > ASUS_WIRELESS_LED_STATUS); > - if (s == ASUS_WIRELESS_LED_ON) > + if (s == data->hswc_params->on) > return LED_FULL; > return LED_OFF; > } > @@ -82,8 +99,8 @@ static void led_state_set(struct led_classdev *led, > struct asus_wireless_data *data; > > data = container_of(led, struct asus_wireless_data, led); > - data->led_state = value == LED_OFF ? ASUS_WIRELESS_LED_OFF : > - ASUS_WIRELESS_LED_ON; > + data->led_state = value == LED_OFF ? data->hswc_params->off : > + data->hswc_params->on; > queue_work(data->wq, &data->led_work); > } > > @@ -104,13 +121,22 @@ static void asus_wireless_notify(struct acpi_device *adev, u32 event) > static int asus_wireless_add(struct acpi_device *adev) > { > struct asus_wireless_data *data; > - int err; > + const char *hid; > + int err, i; > > data = devm_kzalloc(&adev->dev, sizeof(*data), GFP_KERNEL); > if (!data) > return -ENOMEM; > adev->driver_data = data; > > + hid = acpi_device_hid(adev); > + for (i = 0; strcmp(device_ids[i].id, ""); i++) { > + if (!strcmp(device_ids[i].id, hid)) { > + data->hswc_params = &id_params[i]; > + break; > + } > + } > + No, see above. > data->idev = devm_input_allocate_device(&adev->dev); > if (!data->idev) > return -ENOMEM; > @@ -137,6 +163,7 @@ static int asus_wireless_add(struct acpi_device *adev) > err = devm_led_classdev_register(&adev->dev, &data->led); > if (err) > destroy_workqueue(data->wq); > + > return err; > } > > @@ -149,13 +176,6 @@ static int asus_wireless_remove(struct acpi_device *adev) > return 0; > } > > -static const struct acpi_device_id device_ids[] = { > - {"ATK4001", 0}, > - {"ATK4002", 0}, > - {"", 0}, > -}; > -MODULE_DEVICE_TABLE(acpi, device_ids); > - > static struct acpi_driver asus_wireless_driver = { > .name = "Asus Wireless Radio Control Driver", > .class = "hotkey", > -- > 2.11.0 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID 2017-01-27 15:26 ` Andy Shevchenko @ 2017-02-01 12:20 ` João Paulo Rechi Vita 2017-02-04 15:02 ` Andy Shevchenko 0 siblings, 1 reply; 10+ messages in thread From: João Paulo Rechi Vita @ 2017-02-01 12:20 UTC (permalink / raw) To: Andy Shevchenko Cc: Darren Hart, Platform Driver, acpi4asus-user, linux-kernel, linux, João Paulo Rechi Vita On 27 January 2017 at 10:26, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Thu, Jan 26, 2017 at 5:00 PM, João Paulo Rechi Vita > <jprvita@gmail.com> 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@endlessm.com> >> --- >> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID 2017-02-01 12:20 ` João Paulo Rechi Vita @ 2017-02-04 15:02 ` Andy Shevchenko 2017-02-07 21:44 ` João Paulo Rechi Vita 0 siblings, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2017-02-04 15:02 UTC (permalink / raw) To: João Paulo Rechi Vita Cc: Darren Hart, Platform Driver, acpi4asus-user, linux-kernel, linux, João Paulo Rechi Vita On Wed, Feb 1, 2017 at 2:20 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote: > On 27 January 2017 at 10:26, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Thu, Jan 26, 2017 at 5:00 PM, João Paulo Rechi Vita >> <jprvita@gmail.com> wrote: >>> +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? {"ATK4001", (kernel_ulong_t)atk4001_id_params}, Similar for the other one. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID 2017-02-04 15:02 ` Andy Shevchenko @ 2017-02-07 21:44 ` João Paulo Rechi Vita 2017-02-07 21:45 ` [PATCHv2] platform/x86: asus-wireless: Use per-HID HSWC parameters João Paulo Rechi Vita 2017-02-07 23:37 ` [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID Andy Shevchenko 0 siblings, 2 replies; 10+ messages in thread From: João Paulo Rechi Vita @ 2017-02-07 21:44 UTC (permalink / raw) To: Andy Shevchenko Cc: Darren Hart, Platform Driver, acpi4asus-user, linux-kernel, linux, João Paulo Rechi Vita On 4 February 2017 at 10:02, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Feb 1, 2017 at 2:20 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote: >> On 27 January 2017 at 10:26, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> On Thu, Jan 26, 2017 at 5:00 PM, João Paulo Rechi Vita >>> <jprvita@gmail.com> wrote: > >>>> +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? > > {"ATK4001", (kernel_ulong_t)atk4001_id_params}, > The code you suggested: static const struct hswc_params atk4001_id_params = {0x0, 0x1, 0x2}; static const struct acpi_device_id device_ids[] = { {"ATK4001", (kernel_ulong_t)atk4001_id_params}, {"", 0}, }; does not compile: "drivers/platform/x86/asus-wireless.c:44:2: error: aggregate value used where an integer was expected", so I guess you meant the address of that struct (&atk4001_id_params), right? I don't see any way how we could have the actual data in .driver_data. Even after moving the parameters in the driver_data field of device_ids[], I'm not able retrieve them with acpi_match_device(), because the "struct device" from the "struct acpi_device" that comes as an input parameter in asus_wireless_add() does not have a acpi companion device associated with it, so acpi_match_device() returns NULL. I still need to manually loop through device_ids[] in order to retrieve the .driver_data associated with the HID, and I see the same pattern in the i2c-scmi driver. I'm sending the next revision for feedback, please advise if I'm missing any detail, or if you want this implemented differently. -- João Paulo Rechi Vita http://about.me/jprvita ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv2] platform/x86: asus-wireless: Use per-HID HSWC parameters 2017-02-07 21:44 ` João Paulo Rechi Vita @ 2017-02-07 21:45 ` João Paulo Rechi Vita 2017-02-13 22:09 ` Andy Shevchenko 2017-02-07 23:37 ` [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID Andy Shevchenko 1 sibling, 1 reply; 10+ messages in thread From: João Paulo Rechi Vita @ 2017-02-07 21:45 UTC (permalink / raw) To: Darren Hart, Andy Shevchenko Cc: platform-driver-x86, acpi4asus-user, linux-kernel, linux, João Paulo Rechi Vita Some Asus machines use 0x4/0x5 as their LED on/off values, while others use 0x0/0x1, as shown in the DSDT excerpts below. Luckily it seems this behavior is tied to different HIDs, after looking at 44 DSDTs from different Asus models. Another small difference is that a few of them call GWBL instead of OWGS, and SWBL instead of OWGD. That does not seem to make a difference for asus-wireless, and is additional reasoning 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@endlessm.com> --- drivers/platform/x86/asus-wireless.c | 57 ++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c index a7ee18383146..f3796164329e 100644 --- a/drivers/platform/x86/asus-wireless.c +++ b/drivers/platform/x86/asus-wireless.c @@ -17,19 +17,41 @@ #include <linux/pci_ids.h> #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; + u8 status; +}; 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; }; +static const struct hswc_params atk4001_id_params = { + .on = 0x0, + .off = 0x1, + .status = 0x2, +}; + +static const struct hswc_params atk4002_id_params = { + .on = 0x5, + .off = 0x4, + .status = 0x2, +}; + +static const struct acpi_device_id device_ids[] = { + {"ATK4001", (kernel_ulong_t)&atk4001_id_params}, + {"ATK4002", (kernel_ulong_t)&atk4002_id_params}, + {"", 0}, +}; +MODULE_DEVICE_TABLE(acpi, device_ids); + static u64 asus_wireless_method(acpi_handle handle, const char *method, int param) { @@ -61,8 +83,8 @@ static enum led_brightness led_state_get(struct led_classdev *led) data = container_of(led, struct asus_wireless_data, led); s = asus_wireless_method(acpi_device_handle(data->adev), "HSWC", - ASUS_WIRELESS_LED_STATUS); - if (s == ASUS_WIRELESS_LED_ON) + data->hswc_params->status); + if (s == data->hswc_params->on) return LED_FULL; return LED_OFF; } @@ -81,8 +103,8 @@ static void led_state_set(struct led_classdev *led, enum led_brightness value) struct asus_wireless_data *data; data = container_of(led, struct asus_wireless_data, led); - data->led_state = value == LED_OFF ? ASUS_WIRELESS_LED_OFF : - ASUS_WIRELESS_LED_ON; + data->led_state = value == LED_OFF ? data->hswc_params->off : + data->hswc_params->on; queue_work(data->wq, &data->led_work); } @@ -103,12 +125,14 @@ static void asus_wireless_notify(struct acpi_device *adev, u32 event) static int asus_wireless_add(struct acpi_device *adev) { struct asus_wireless_data *data; + const struct acpi_device_id *id; int err; data = devm_kzalloc(&adev->dev, sizeof(*data), GFP_KERNEL); if (!data) return -ENOMEM; adev->driver_data = data; + data->adev = adev; data->idev = devm_input_allocate_device(&adev->dev); if (!data->idev) @@ -123,7 +147,16 @@ static int asus_wireless_add(struct acpi_device *adev) if (err) return err; - data->adev = adev; + for (id = device_ids; id->id[0]; id++) { + if (!strcmp((char *) id->id, acpi_device_hid(adev))) { + data->hswc_params = + (const struct hswc_params *)id->driver_data; + break; + } + } + if (!data->hswc_params) + return 0; + data->wq = create_singlethread_workqueue("asus_wireless_workqueue"); if (!data->wq) return -ENOMEM; @@ -136,6 +169,7 @@ static int asus_wireless_add(struct acpi_device *adev) err = devm_led_classdev_register(&adev->dev, &data->led); if (err) destroy_workqueue(data->wq); + return err; } @@ -148,13 +182,6 @@ static int asus_wireless_remove(struct acpi_device *adev) return 0; } -static const struct acpi_device_id device_ids[] = { - {"ATK4001", 0}, - {"ATK4002", 0}, - {"", 0}, -}; -MODULE_DEVICE_TABLE(acpi, device_ids); - static struct acpi_driver asus_wireless_driver = { .name = "Asus Wireless Radio Control Driver", .class = "hotkey", -- 2.11.0 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] platform/x86: asus-wireless: Use per-HID HSWC parameters 2017-02-07 21:45 ` [PATCHv2] platform/x86: asus-wireless: Use per-HID HSWC parameters João Paulo Rechi Vita @ 2017-02-13 22:09 ` Andy Shevchenko 0 siblings, 0 replies; 10+ messages in thread From: Andy Shevchenko @ 2017-02-13 22:09 UTC (permalink / raw) To: João Paulo Rechi Vita Cc: Darren Hart, Platform Driver, acpi4asus-user, linux-kernel, linux, João Paulo Rechi Vita On Tue, Feb 7, 2017 at 11:45 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote: > Some Asus machines use 0x4/0x5 as their LED on/off values, while others > use 0x0/0x1, as shown in the DSDT excerpts below. Luckily it seems this > behavior is tied to different HIDs, after looking at 44 DSDTs from > different Asus models. > > Another small difference is that a few of them call GWBL instead of > OWGS, and SWBL instead of OWGD. That does not seem to make a difference > for asus-wireless, and is additional reasoning 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) | > } | > } | > } | > Thanks, applied to testing. > Signed-off-by: João Paulo Rechi Vita <jprvita@endlessm.com> > --- > drivers/platform/x86/asus-wireless.c | 57 ++++++++++++++++++++++++++---------- > 1 file changed, 42 insertions(+), 15 deletions(-) > > diff --git a/drivers/platform/x86/asus-wireless.c b/drivers/platform/x86/asus-wireless.c > index a7ee18383146..f3796164329e 100644 > --- a/drivers/platform/x86/asus-wireless.c > +++ b/drivers/platform/x86/asus-wireless.c > @@ -17,19 +17,41 @@ > #include <linux/pci_ids.h> > #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; > + u8 status; > +}; > > 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; > }; > > +static const struct hswc_params atk4001_id_params = { > + .on = 0x0, > + .off = 0x1, > + .status = 0x2, > +}; > + > +static const struct hswc_params atk4002_id_params = { > + .on = 0x5, > + .off = 0x4, > + .status = 0x2, > +}; > + > +static const struct acpi_device_id device_ids[] = { > + {"ATK4001", (kernel_ulong_t)&atk4001_id_params}, > + {"ATK4002", (kernel_ulong_t)&atk4002_id_params}, > + {"", 0}, > +}; > +MODULE_DEVICE_TABLE(acpi, device_ids); > + > static u64 asus_wireless_method(acpi_handle handle, const char *method, > int param) > { > @@ -61,8 +83,8 @@ static enum led_brightness led_state_get(struct led_classdev *led) > > data = container_of(led, struct asus_wireless_data, led); > s = asus_wireless_method(acpi_device_handle(data->adev), "HSWC", > - ASUS_WIRELESS_LED_STATUS); > - if (s == ASUS_WIRELESS_LED_ON) > + data->hswc_params->status); > + if (s == data->hswc_params->on) > return LED_FULL; > return LED_OFF; > } > @@ -81,8 +103,8 @@ static void led_state_set(struct led_classdev *led, enum led_brightness value) > struct asus_wireless_data *data; > > data = container_of(led, struct asus_wireless_data, led); > - data->led_state = value == LED_OFF ? ASUS_WIRELESS_LED_OFF : > - ASUS_WIRELESS_LED_ON; > + data->led_state = value == LED_OFF ? data->hswc_params->off : > + data->hswc_params->on; > queue_work(data->wq, &data->led_work); > } > > @@ -103,12 +125,14 @@ static void asus_wireless_notify(struct acpi_device *adev, u32 event) > static int asus_wireless_add(struct acpi_device *adev) > { > struct asus_wireless_data *data; > + const struct acpi_device_id *id; > int err; > > data = devm_kzalloc(&adev->dev, sizeof(*data), GFP_KERNEL); > if (!data) > return -ENOMEM; > adev->driver_data = data; > + data->adev = adev; > > data->idev = devm_input_allocate_device(&adev->dev); > if (!data->idev) > @@ -123,7 +147,16 @@ static int asus_wireless_add(struct acpi_device *adev) > if (err) > return err; > > - data->adev = adev; > + for (id = device_ids; id->id[0]; id++) { > + if (!strcmp((char *) id->id, acpi_device_hid(adev))) { > + data->hswc_params = > + (const struct hswc_params *)id->driver_data; > + break; > + } > + } > + if (!data->hswc_params) > + return 0; > + > data->wq = create_singlethread_workqueue("asus_wireless_workqueue"); > if (!data->wq) > return -ENOMEM; > @@ -136,6 +169,7 @@ static int asus_wireless_add(struct acpi_device *adev) > err = devm_led_classdev_register(&adev->dev, &data->led); > if (err) > destroy_workqueue(data->wq); > + > return err; > } > > @@ -148,13 +182,6 @@ static int asus_wireless_remove(struct acpi_device *adev) > return 0; > } > > -static const struct acpi_device_id device_ids[] = { > - {"ATK4001", 0}, > - {"ATK4002", 0}, > - {"", 0}, > -}; > -MODULE_DEVICE_TABLE(acpi, device_ids); > - > static struct acpi_driver asus_wireless_driver = { > .name = "Asus Wireless Radio Control Driver", > .class = "hotkey", > -- > 2.11.0 > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID 2017-02-07 21:44 ` João Paulo Rechi Vita 2017-02-07 21:45 ` [PATCHv2] platform/x86: asus-wireless: Use per-HID HSWC parameters João Paulo Rechi Vita @ 2017-02-07 23:37 ` Andy Shevchenko 2017-02-08 17:04 ` João Paulo Rechi Vita 1 sibling, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2017-02-07 23:37 UTC (permalink / raw) To: João Paulo Rechi Vita Cc: Darren Hart, Platform Driver, acpi4asus-user, linux-kernel, linux, João Paulo Rechi Vita On Tue, Feb 7, 2017 at 11:44 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote: > On 4 February 2017 at 10:02, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >> On Wed, Feb 1, 2017 at 2:20 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote: >>> On 27 January 2017 at 10:26, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>>> On Thu, Jan 26, 2017 at 5:00 PM, João Paulo Rechi Vita >>>> <jprvita@gmail.com> wrote: >> >>>>> +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? >> >> {"ATK4001", (kernel_ulong_t)atk4001_id_params}, >> > > The code you suggested: > > static const struct hswc_params atk4001_id_params = {0x0, 0x1, 0x2}; > static const struct acpi_device_id device_ids[] = { > {"ATK4001", (kernel_ulong_t)atk4001_id_params}, > {"", 0}, > }; > > does not compile: "drivers/platform/x86/asus-wireless.c:44:2: error: > aggregate value used where an integer was expected", so I guess you > meant the address of that struct (&atk4001_id_params), right? I don't > see any way how we could have the actual data in .driver_data. Yes, you are right. I kept in mind array when was suggesting this. > Even after moving the parameters in the driver_data field of > device_ids[], I'm not able retrieve them with acpi_match_device(), > because the "struct device" from the "struct acpi_device" that comes > as an input parameter in asus_wireless_add() does not have a acpi > companion device associated with it, so acpi_match_device() returns > NULL. I still need to manually loop through device_ids[] in order to > retrieve the .driver_data associated with the HID, and I see the same > pattern in the i2c-scmi driver. And what prevents us to set companion device? > > I'm sending the next revision for feedback, please advise if I'm > missing any detail, or if you want this implemented differently. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID 2017-02-07 23:37 ` [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID Andy Shevchenko @ 2017-02-08 17:04 ` João Paulo Rechi Vita 0 siblings, 0 replies; 10+ messages in thread From: João Paulo Rechi Vita @ 2017-02-08 17:04 UTC (permalink / raw) To: Andy Shevchenko Cc: Darren Hart, Platform Driver, acpi4asus-user, linux-kernel, linux, João Paulo Rechi Vita On 7 February 2017 at 18:37, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Feb 7, 2017 at 11:44 PM, João Paulo Rechi Vita > <jprvita@gmail.com> wrote: >> On 4 February 2017 at 10:02, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>> On Wed, Feb 1, 2017 at 2:20 PM, João Paulo Rechi Vita <jprvita@gmail.com> wrote: >>>> On 27 January 2017 at 10:26, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: >>>>> On Thu, Jan 26, 2017 at 5:00 PM, João Paulo Rechi Vita >>>>> <jprvita@gmail.com> wrote: >>> >>>>>> +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? >>> >>> {"ATK4001", (kernel_ulong_t)atk4001_id_params}, >>> >> >> The code you suggested: >> >> static const struct hswc_params atk4001_id_params = {0x0, 0x1, 0x2}; >> static const struct acpi_device_id device_ids[] = { >> {"ATK4001", (kernel_ulong_t)atk4001_id_params}, >> {"", 0}, >> }; >> >> does not compile: "drivers/platform/x86/asus-wireless.c:44:2: error: >> aggregate value used where an integer was expected", so I guess you >> meant the address of that struct (&atk4001_id_params), right? I don't >> see any way how we could have the actual data in .driver_data. > > Yes, you are right. I kept in mind array when was suggesting this. > >> Even after moving the parameters in the driver_data field of >> device_ids[], I'm not able retrieve them with acpi_match_device(), >> because the "struct device" from the "struct acpi_device" that comes >> as an input parameter in asus_wireless_add() does not have a acpi >> companion device associated with it, so acpi_match_device() returns >> NULL. I still need to manually loop through device_ids[] in order to >> retrieve the .driver_data associated with the HID, and I see the same >> pattern in the i2c-scmi driver. > > And what prevents us to set companion device? > Assuming this in the scope of a platform driver (genuine question, as I don't see anything under drivers/platform/ doing so), nothing prevents us to set it. But when doing so, acpi_match_device() still fails, because acpi_get_first_physical_node() returns a different "struct dev *" in acpi_primary_dev_companion(). -- João Paulo Rechi Vita http://about.me/jprvita ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-02-13 22:09 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-26 15:00 [PATCH 0/1] asus-wireless: Use the correct HSWC parameter for each HID João Paulo Rechi Vita 2017-01-26 15:00 ` [PATCH 1/1] " João Paulo Rechi Vita 2017-01-27 15:26 ` Andy Shevchenko 2017-02-01 12:20 ` João Paulo Rechi Vita 2017-02-04 15:02 ` Andy Shevchenko 2017-02-07 21:44 ` João Paulo Rechi Vita 2017-02-07 21:45 ` [PATCHv2] platform/x86: asus-wireless: Use per-HID HSWC parameters João Paulo Rechi Vita 2017-02-13 22:09 ` Andy Shevchenko 2017-02-07 23:37 ` [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID Andy Shevchenko 2017-02-08 17:04 ` João Paulo Rechi Vita
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®