Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1571524 > unrolled thread
| Started by | João Paulo Rechi Vita <jprvita@gmail.com> |
|---|---|
| First post | 2017-02-01 13:30 +0100 |
| Last post | 2017-02-08 18:10 +0100 |
| Articles | 6 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID João Paulo Rechi Vita <jprvita@gmail.com> - 2017-02-01 13:30 +0100
Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-04 16:10 +0100
Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID João Paulo Rechi Vita <jprvita@gmail.com> - 2017-02-07 22:50 +0100
[PATCHv2] platform/x86: asus-wireless: Use per-HID HSWC parameters "João Paulo Rechi Vita" <jprvita@gmail.com> - 2017-02-07 23:00 +0100
Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-02-08 00:50 +0100
Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID João Paulo Rechi Vita <jprvita@gmail.com> - 2017-02-08 18:10 +0100
| From | João Paulo Rechi Vita <jprvita@gmail.com> |
|---|---|
| Date | 2017-02-01 13:30 +0100 |
| Subject | Re: [PATCH 1/1] asus-wireless: Use the correct HSWC parameter for each HID |
| Message-ID | <t62cX-5Vf-47@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-02-04 16:10 +0100 |
| Message-ID | <t7a8q-2cc-19@gated-at.bofh.it> |
| In reply to | #1571524 |
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
[toc] | [prev] | [next] | [standalone]
| From | João Paulo Rechi Vita <jprvita@gmail.com> |
|---|---|
| Date | 2017-02-07 22:50 +0100 |
| Message-ID | <t8lOb-8hG-51@gated-at.bofh.it> |
| In reply to | #1573681 |
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
[toc] | [prev] | [next] | [standalone]
| From | "João Paulo Rechi Vita" <jprvita@gmail.com> |
|---|---|
| Date | 2017-02-07 23:00 +0100 |
| Subject | [PATCHv2] platform/x86: asus-wireless: Use per-HID HSWC parameters |
| Message-ID | <t8lXP-8l3-1@gated-at.bofh.it> |
| In reply to | #1576095 |
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
[toc] | [prev] | [next] | [standalone]
| From | Andy Shevchenko <andy.shevchenko@gmail.com> |
|---|---|
| Date | 2017-02-08 00:50 +0100 |
| Message-ID | <t8nGi-ZA-21@gated-at.bofh.it> |
| In reply to | #1576095 |
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
[toc] | [prev] | [next] | [standalone]
| From | João Paulo Rechi Vita <jprvita@gmail.com> |
|---|---|
| Date | 2017-02-08 18:10 +0100 |
| Message-ID | <t8DUJ-33r-17@gated-at.bofh.it> |
| In reply to | #1576181 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web