Re: Re: [PATCH] HID: sony: Add force feedback support for Dualshock3 USB

Commit Message

Sven Eckelmann Nov. 17, 2013, 6:08 p.m. UTC

On Sunday 17 November 2013 17:30:51 David Herrmann wrote:
> Yeah, the input-ff callbacks cannot be handled inline. You also get
> deadlocks with the input-spinlock. In the wiimote driver I simply
> dispatch the ff-events to a workqueue. You can have a look at
> drivers/hid/hid-wiimote-modules.c. You can get some ordering-problems
> then, but these can usually be ignored as they just collapse events.
> 
> The related commit was:
> 
> commit f50f9aabf32db7414551ffdfdccc71be5f3d6e7d
> Author: David Herrmann <[e-mail]>
> Date:   Wed Oct 2 13:47:28 2013 +0200
> 
>     HID: wiimote: fix FF deadlock

Yes, I've played around with the linux kernel usb message api and came to the 
same conclusion (for now). I've only tested it with a small proof of concept 
patch and it didn't hang anymore with testhaptic.

Maybe Simon Wood can test his devices because I am unsure whether PS3 
dualshock clones will work with the interrupt or control urbs. For example the 
script from Simon didn't work at all for me.

Kind regards,
	Sven

Comments

[e-mail] Nov. 17, 2013, 7:11 p.m. UTC | #1

> Maybe Simon Wood can test his devices because I am unsure whether PS3
> dualshock clones will work with the interrupt or control urbs. For example
> the script from Simon didn't work at all for me.

With the proof of concept both my devices work OK with 'testhaptic' and
'testrumble' - the controllers rumble and the computer does not lock up.

My script is from a couple of years ago, and it didn't work for me with
the dualshock.

Simon.

PS. We know that some of the bits of that data frame are used to control
the LEDs. Anyone interested on getting them working via the LED class?

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index da551d1..d509447 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -224,6 +224,10 @@  static const unsigned int buzz_keymap[] = {
 
 struct sony_sc {
 	unsigned long quirks;
+	struct work_struct rumble_worker;
+	struct hid_device *hdev;
+	__u8 left;
+	__u8 right;
 
 	void *extra;
 };
@@ -614,35 +618,53 @@  static void buzz_remove(struct hid_device *hdev)
 	drv_data->extra = NULL;
 }
 
-#ifdef CONFIG_SONY_FF
-static int sony_play_effect(struct input_dev *dev, void *data,
-			    struct ff_effect *effect)
+static void sony_rumble_worker(struct work_struct *work)
 {
+	struct sony_sc *sc = container_of(work, struct sony_sc, rumble_worker);
 	unsigned char buf[] = {
 		0x01,
 		0x00, 0xff, 0x00, 0xff, 0x00,
-		0x00, 0x00, 0x00, 0x00, 0x03,
+		0x00, 0x00, 0x00, 0x00, 0x02,
 		0xff, 0x27, 0x10, 0x00, 0x32,
 		0xff, 0x27, 0x10, 0x00, 0x32,
 		0xff, 0x27, 0x10, 0x00, 0x32,
 		0xff, 0x27, 0x10, 0x00, 0x32,
-		0x00, 0x00, 0x00, 0x00, 0x00
+		0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00, 0x00, 0x00,
+		0x00, 0x00, 0x00
 	};
-	__u8 left;
-	__u8 right;
+	struct usb_interface *intf = to_usb_interface(sc->hdev->dev.parent);
+	struct usb_device *usbdev = interface_to_usbdev(intf);
+
+	buf[3] = sc->right;
+	buf[5] = sc->left;
+
+	if (sc->right || sc->left)
+		usb_interrupt_msg(usbdev, usb_sndctrlpipe(usbdev, 2), buf,
+				  sizeof(buf), NULL, USB_CTRL_SET_TIMEOUT);
+
+	buf[10] = 0x1e;
+	usb_interrupt_msg(usbdev, usb_sndctrlpipe(usbdev, 2), buf, sizeof(buf),
+			  NULL, USB_CTRL_SET_TIMEOUT);
+}
+
+#ifdef CONFIG_SONY_FF
+static int sony_play_effect(struct input_dev *dev, void *data,
+			    struct ff_effect *effect)
+{
 	struct hid_device *hid = input_get_drvdata(dev);
+	struct sony_sc *sc = hid_get_drvdata(hid);
 
 	if (effect->type != FF_RUMBLE)
 		return 0;
 
-	left = effect->u.rumble.strong_magnitude / 256;
-	right = effect->u.rumble.weak_magnitude ? 1 : 0;
+	sc->left = effect->u.rumble.strong_magnitude / 256;
+	sc->right = effect->u.rumble.weak_magnitude ? 1 : 0;
 
-	buf[3] = right;
-	buf[5] = left;
 
-	return hid->hid_output_raw_report(hid, buf, sizeof(buf),
-					  HID_OUTPUT_REPORT);
+	schedule_work(&sc->rumble_worker);
+	return 0;
 }
 
 static int sony_init_ff(struct hid_device *hdev)
@@ -650,6 +672,9 @@  static int sony_init_ff(struct hid_device *hdev)
 	struct hid_input *hidinput = list_entry(hdev->inputs.next,
 						struct hid_input, list);
 	struct input_dev *input_dev = hidinput->input;
+	struct sony_sc *sc = hid_get_drvdata(hdev);
+
+	INIT_WORK(&sc->rumble_worker, sony_rumble_worker);
 
 	input_set_capability(input_dev, EV_FF, FF_RUMBLE);
 	return input_ff_create_memless(input_dev, NULL, sony_play_effect);
@@ -711,6 +736,7 @@  static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (ret < 0)
 		goto err_stop;
 
+	sc->hdev = hdev;
 	ret = sony_init_ff(hdev);
 	if (ret < 0)
 		goto err_stop;
@@ -728,6 +754,8 @@  static void sony_remove(struct hid_device *hdev)
 	if (sc->quirks & BUZZ_CONTROLLER)
 		buzz_remove(hdev);
 
+	cancel_work_sync(&sc->rumble_worker);
+
 	hid_hw_stop(hdev);
 }