Re: [PATCH 2/4] HID: sony: Use standard output reports instead of raw reports to send data to the Dualshock 4.
From: David Herrmann <hidden>
Date: 2014-01-17 11:09:57
Hi On Fri, Jan 17, 2014 at 3:42 AM, Frank Praznik [off-list ref] wrote:
Use regular HID output reports instead of raw reports in the dualshock4_state_worker function. (Thanks Simon Mungewell)
This description is actually wrong. hid_output_raw_report() is used for regular HID output reports. What you do, is using SET_REPORT to synchronously set output-reports. Anyhow, see below for comments.
quoted hunk ↗ jump to hunk
Signed-off-by: Frank Praznik <redacted> --- Apply against jikos/hid.git/for-3.14/sony drivers/hid/hid-sony.c | 45 ++++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 17 deletions(-)diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c index 2f992e1..e623131 100644 --- a/drivers/hid/hid-sony.c +++ b/drivers/hid/hid-sony.c@@ -664,28 +664,39 @@ static void sixaxis_state_worker(struct work_struct *work) static void dualshock4_state_worker(struct work_struct *work) { struct sony_sc *sc = container_of(work, struct sony_sc, state_worker); - unsigned char buf[] = { - 0x05, - 0x03, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, - }; + struct hid_device *hdev = sc->hdev; + struct list_head *head, *list; + struct hid_report *report; + __s32 *value; + + list = &hdev->report_enum[HID_OUTPUT_REPORT].report_list; + + list_for_each(head, list) { + report = list_entry(head, struct hid_report, list); + + /* Report 5 is used to send data to the controller via USB */ + if ((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && report->id == 5) + break; + } + + if (head == list) { + hid_err(hdev, "Dualshock 4 output report not found\n");
Are you sure you want to print this error on *every* invokation? I'd rather like to see this "list_for_each()" in ->probe() and then print the error once. In ->probe() after you called hid_parse(), the report-descriptor will not get changed again so it's safe to cache the report pointer there. At least I don't know any code-path where we'd change it again. Maybe @Jiri can confirm that. I'd also like to see a safety check that the given report is long enough. Otherwise, you might write out of buffer bounds below. But you can do all this in ->probe(). Thanks David
+ return; + } + + value = report->field[0]->value; + value[0] = 0x03; #ifdef CONFIG_SONY_FF - buf[4] = sc->right; - buf[5] = sc->left; + value[3] = sc->right; + value[4] = sc->left; #endif - buf[6] = sc->led_state[0]; - buf[7] = sc->led_state[1]; - buf[8] = sc->led_state[2]; + value[5] = sc->led_state[0]; + value[6] = sc->led_state[1]; + value[7] = sc->led_state[2]; - sc->hdev->hid_output_raw_report(sc->hdev, buf, sizeof(buf), - HID_OUTPUT_REPORT); + hid_hw_request(hdev, report, HID_REQ_SET_REPORT); } #ifdef CONFIG_SONY_FF -- 1.8.3.2 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html