On Fri, 28 Dec 2018 10:26:14 +0800
liquid <outmatch@xxxxxxxxx> wrote:
> >From 6323ffb83e23eab9a08208063f9dba27c6a0d228 Mon Sep 17 00:00:00 2001
> From: Hongye Yuan <outmatch@xxxxxxxxx>
> Date: Thu, 27 Dec 2018 09:41:23 +0800
> Subject: [PATCH] HID: sony: Fix SHANWAN PS3 GamePad rumbling on USB again
>
> - The SHANWAN DS3 clone joystick requires HID Output Reports via Interrupt EP.
> - Added a quirk for SHANWAN PS3 GamePad.
>
> Signed-off-by: Hongye Yuan <outmatch@xxxxxxxxx>
Hi Hongye,
I am CCing Roderick Colenbrander who is the de-facto hid-sony
maintainer. Maybe Roderick should be mentioned in MAINTAINERS.
I guess you can skip linux-usb and Greg on the next iteration to spare
them some traffic.
Please consider using "git send-email" to send patches created with "git
format-patch" to avoid problems with formatting, for instance I don't
see any TABs in the patch below, all TABs seem to have been converted to
spaces and the patch does not apply.
Make sure to use ./scripts/checkpatch.pl to spot other problems.
Some other minor comments are inlined below.
> ---
> drivers/hid/hid-sony.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 9671a4bad643..842f370cfec2 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -58,6 +58,7 @@
> #define FUTUREMAX_DANCE_MAT BIT(13)
> #define NSG_MR5U_REMOTE_BT BIT(14)
> #define NSG_MR7U_REMOTE_BT BIT(15)
> +#define SHANWAN_GAMEPAD BIT(16)
>
> #define SIXAXIS_CONTROLLER (SIXAXIS_CONTROLLER_USB | SIXAXIS_CONTROLLER_BT)
> #define MOTION_CONTROLLER (MOTION_CONTROLLER_USB | MOTION_CONTROLLER_BT)
> @@ -1490,6 +1491,7 @@ static int sony_register_sensors(struct sony_sc *sc)
> */
> static int sixaxis_set_operational_usb(struct hid_device *hdev)
> {
> + struct sony_sc *sc = hid_get_drvdata(hdev);
> const int buf_size =
> max(SIXAXIS_REPORT_0xF2_SIZE, SIXAXIS_REPORT_0xF5_SIZE);
> u8 *buf;
> @@ -1521,7 +1523,7 @@ static int sixaxis_set_operational_usb(struct
> hid_device *hdev)
> * But the USB interrupt would cause SHANWAN controllers to
> * start rumbling non-stop.
> */
> - if (strcmp(hdev->name, "SHANWAN PS3 GamePad")) {
> + if (sc->quirks & SHANWAN_GAMEPAD) {
I think you are inverting the logic here, the original test meant to
check that the controller was NOT a "SHANWAN PS3 GamePad" before doing
step 3.
Maybe this can be made more explicit with the change below (ideally in
a preliminary patch):
-----------------------------------------------------------------------
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 9671a4bad643..949305cfa199 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1519,14 +1519,15 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
/*
* But the USB interrupt would cause SHANWAN controllers to
- * start rumbling non-stop.
+ * start rumbling non-stop, so skip step 3 for these controllers.
*/
- if (strcmp(hdev->name, "SHANWAN PS3 GamePad")) {
- ret = hid_hw_output_report(hdev, buf, 1);
- if (ret < 0) {
- hid_info(hdev, "can't set operational mode: step 3, ignoring\n");
- ret = 0;
- }
+ if (!strcmp(hdev->name, "SHANWAN PS3 GamePad"))
+ goto out;
+
+ ret = hid_hw_output_report(hdev, buf, 1);
+ if (ret < 0) {
+ hid_info(hdev, "can't set operational mode: step 3, ignoring\n");
+ ret = 0;
}
out:
-----------------------------------------------------------------------
Unfortunately I cannot test it.
After that change you can check for (sc->quirks & SHANWAN_GAMEPAD) in
your patch.
> ret = hid_hw_output_report(hdev, buf, 1);
> if (ret < 0) {
> hid_info(hdev, "can't set operational mode: step 3, ignoring\n");
> @@ -2097,9 +2099,13 @@ static void sixaxis_send_output_report(struct
> sony_sc *sc)
> }
> }
>
> - hid_hw_raw_request(sc->hdev, report->report_id, (u8 *)report,
> - sizeof(struct sixaxis_output_report),
> - HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> + /* SHANWAN controllers require sending output reports via intr channel */
> + if (sc->quirks & SHANWAN_GAMEPAD)
> + hid_hw_output_report(sc->hdev, (u8 *)report,
> + sizeof(struct sixaxis_output_report));
> + else hid_hw_raw_request(sc->hdev, report->report_id, (u8 *)report,
> + sizeof(struct sixaxis_output_report),
> + HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
The body of the else condition needs to go on a new line, with an extra
indentation level.
Also, this makes me wonder if the problem might be about HID quirks, can
you try playing with hdev->quirks settings in sony_input_configured()?
Ciao,
Antonio
> }
>
> static void dualshock4_send_output_report(struct sony_sc *sc)
> @@ -2811,6 +2817,9 @@ static int sony_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
> if (!strcmp(hdev->name, "FutureMax Dance Mat"))
> quirks |= FUTUREMAX_DANCE_MAT;
>
> + if (!strcmp(hdev->name, "SHANWAN PS3 GamePad"))
> + quirks |= SHANWAN_GAMEPAD;
> +
> sc = devm_kzalloc(&hdev->dev, sizeof(*sc), GFP_KERNEL);
> if (sc == NULL) {
> hid_err(hdev, "can't alloc sony descriptor\n");
> --
> 2.20.1
--
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
