Hi
On Fri, Jan 17, 2014 at 3:42 AM, Frank Praznik <frank.praznik@xxxxxxxxx> 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.
> Signed-off-by: Frank Praznik <frank.praznik@xxxxxxxxx>
>
> ---
>
> 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@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
