diff mbox series

HID: sony: Fix SHANWAN PS3 GamePad rumbling on USB again

Message ID CAPsNqc427=u2B02fPpi3Dtw-K7dtSGfphq3qi-PeBNTRRmDVTg@mail.gmail.com (mailing list archive)
State Superseded
Delegated to: Jiri Kosina
Headers show
Series HID: sony: Fix SHANWAN PS3 GamePad rumbling on USB again | expand

Commit Message

Hongye Yuan Dec. 28, 2018, 2:26 a.m. UTC 
From 6323ffb83e23eab9a08208063f9dba27c6a0d228 Mon Sep 17 00:00:00 2001
From: Hongye Yuan <outmatch@gmail.com>
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@gmail.com>
---
 drivers/hid/hid-sony.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

             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);
 }

 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");

Comments

Antonio Ospite Dec. 28, 2018, 10:33 a.m. UTC | #1 
On Fri, 28 Dec 2018 10:26:14 +0800
liquid <outmatch@gmail.com> wrote:

> >From 6323ffb83e23eab9a08208063f9dba27c6a0d228 Mon Sep 17 00:00:00 2001
> From: Hongye Yuan <outmatch@gmail.com>
> 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@gmail.com>

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
Hongye Yuan Dec. 28, 2018, 12:21 p.m. UTC | #2 
Hi. I do test this patch on my controller before submitting. It's very
interesting that even the logic is unexpected inverted, the controller
is still working fine.
Maybe because sixaxis_send_output_report() used hid_hw_output_report
now, it accidentally fixed this controller up by supplied motor
setting values.
So it's regardless if sixaxis_set_operational_usb() had skipped step 3
or not. Anyway, I will fix this inverted logic in patch v2 for maximum
hardware compatibility tomorrow.

I don't think hdev->quirks has anything to do with hid kernel
interface other than hidraw. The only mention of
HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP
in kernel is in drivers/hid/hidraw.c. But sixaxis_send_output_report()
didn't use hidraw device file interface at all. So it's better to
force sixaxis_send_output_report() sending output reports via
interrupt EP by hid_hw_output_report() there.

Antonio Ospite <ao2@ao2.it> 于2018年12月28日周五 下午6:33写道：
>
> On Fri, 28 Dec 2018 10:26:14 +0800
> liquid <outmatch@gmail.com> wrote:
>
> > >From 6323ffb83e23eab9a08208063f9dba27c6a0d228 Mon Sep 17 00:00:00 2001
> > From: Hongye Yuan <outmatch@gmail.com>
> > 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@gmail.com>
>
> 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?
diff mbox series

Patch

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) {
         ret = hid_hw_output_report(hdev, buf, 1);
         if (ret < 0) {