Without this, my "Gasia Co.,Ltd PS(R) Gamepad" would not send
any events. Now everything works including the leds.
Based on work by Andrew Haines and Antonio Ospite.
cc: Antonio Ospite <[email protected]>
cc: Andrew Haines <[email protected]>
Signed-off-by: Lauri Kasanen <[email protected]>
---
drivers/hid/hid-sony.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
Antonio's original approach was not enough; it enabled the events,
but only for a few seconds, then the controller timed out and sent
no more. Andrew's did more than was necessary. This is a combination
of the two, against Linus' git.
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 31e9d25..de93386 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -36,6 +36,7 @@
#include <linux/list.h>
#include <linux/idr.h>
#include <linux/input/mt.h>
+#include <linux/usb/input.h>
#include "hid-ids.h"
@@ -1130,8 +1131,12 @@ static void sony_input_configured(struct hid_device *hdev,
*/
static int sixaxis_set_operational_usb(struct hid_device *hdev)
{
+ struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
+ struct usb_device *dev = interface_to_usbdev(intf);
int ret;
- char *buf = kmalloc(18, GFP_KERNEL);
+ char *buf = kmalloc(65, GFP_KERNEL);
+ unsigned char buf2[] = { 0x00 };
+ int transfered;
if (!buf)
return -ENOMEM;
@@ -1140,7 +1145,24 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
HID_REQ_GET_REPORT);
if (ret < 0)
- hid_err(hdev, "can't set operational mode\n");
+ hid_err(hdev, "can't set operational mode on the control EP\n");
+
+ /*
+ * Some compatible controllers like the Speedlink Strike FX and
+ * Gasia need another query plus an USB interrupt to get operational.
+ */
+ ret = hid_hw_raw_request(hdev, 0xf5, buf, 64, HID_FEATURE_REPORT,
+ HID_REQ_GET_REPORT);
+
+ if (ret < 0)
+ hid_err(hdev, "can't set operational mode on the interrupt EP\n");
+
+ ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02),
+ buf2, sizeof(buf2),
+ &transfered, USB_CTRL_SET_TIMEOUT);
+
+ if (ret < 0)
+ hid_err(hdev, "can't set operational mode on the interrupt EP\n");
kfree(buf);
--
1.8.3.1
Hi Lauri,
On Sat, Feb 7, 2015 at 8:48 AM, Lauri Kasanen <[email protected]> wrote:
> Without this, my "Gasia Co.,Ltd PS(R) Gamepad" would not send
> any events. Now everything works including the leds.
>
> Based on work by Andrew Haines and Antonio Ospite.
>
> cc: Antonio Ospite <[email protected]>
> cc: Andrew Haines <[email protected]>
> Signed-off-by: Lauri Kasanen <[email protected]>
> ---
> drivers/hid/hid-sony.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> Antonio's original approach was not enough; it enabled the events,
> but only for a few seconds, then the controller timed out and sent
> no more. Andrew's did more than was necessary. This is a combination
> of the two, against Linus' git.
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 31e9d25..de93386 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -36,6 +36,7 @@
> #include <linux/list.h>
> #include <linux/idr.h>
> #include <linux/input/mt.h>
> +#include <linux/usb/input.h>
Please don't.
HID should be transport agnostic, so please refrain from directly call usb.
>
> #include "hid-ids.h"
>
> @@ -1130,8 +1131,12 @@ static void sony_input_configured(struct hid_device *hdev,
> */
> static int sixaxis_set_operational_usb(struct hid_device *hdev)
> {
> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> + struct usb_device *dev = interface_to_usbdev(intf);
> int ret;
> - char *buf = kmalloc(18, GFP_KERNEL);
> + char *buf = kmalloc(65, GFP_KERNEL);
18 or 65 should be retrieved by the description of the device, not hardcoded.
> + unsigned char buf2[] = { 0x00 };
> + int transfered;
>
> if (!buf)
> return -ENOMEM;
> @@ -1140,7 +1145,24 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
> HID_REQ_GET_REPORT);
>
> if (ret < 0)
> - hid_err(hdev, "can't set operational mode\n");
> + hid_err(hdev, "can't set operational mode on the control EP\n");
> +
> + /*
> + * Some compatible controllers like the Speedlink Strike FX and
> + * Gasia need another query plus an USB interrupt to get operational.
> + */
> + ret = hid_hw_raw_request(hdev, 0xf5, buf, 64, HID_FEATURE_REPORT,
> + HID_REQ_GET_REPORT);
0xf5 should not be hardcoded. You have to retrieve it from the
description of the device or at least put a special case for your
specific game controller.
> +
> + if (ret < 0)
> + hid_err(hdev, "can't set operational mode on the interrupt EP\n");
> +
> + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02),
> + buf2, sizeof(buf2),
> + &transfered, USB_CTRL_SET_TIMEOUT);
Can't you simply use a hid_hw_output_report request instead of hard
coding the device specific endpoint?
And I'd also prefer it to be guarded against your specific controller.
Cheers,
Benjamin
> +
> + if (ret < 0)
> + hid_err(hdev, "can't set operational mode on the interrupt EP\n");
>
> kfree(buf);
>
> --
> 1.8.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 7 Feb 2015 10:56:49 -0500
Benjamin Tissoires <[email protected]> wrote:
> Hi Lauri,
>
> On Sat, Feb 7, 2015 at 8:48 AM, Lauri Kasanen <[email protected]> wrote:
> > Without this, my "Gasia Co.,Ltd PS(R) Gamepad" would not send
> > any events. Now everything works including the leds.
> >
> > Based on work by Andrew Haines and Antonio Ospite.
> >
> > cc: Antonio Ospite <[email protected]>
> > cc: Andrew Haines <[email protected]>
> > Signed-off-by: Lauri Kasanen <[email protected]>
> > ---
> > drivers/hid/hid-sony.c | 26 ++++++++++++++++++++++++--
> > 1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > Antonio's original approach was not enough; it enabled the events,
> > but only for a few seconds, then the controller timed out and sent
> > no more. Andrew's did more than was necessary. This is a combination
> > of the two, against Linus' git.
> >
> > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> > index 31e9d25..de93386 100644
> > --- a/drivers/hid/hid-sony.c
> > +++ b/drivers/hid/hid-sony.c
> > @@ -36,6 +36,7 @@
> > #include <linux/list.h>
> > #include <linux/idr.h>
> > #include <linux/input/mt.h>
> > +#include <linux/usb/input.h>
>
> Please don't.
> HID should be transport agnostic, so please refrain from directly call usb.
>
I agree with Benjamin here.
> >
> > #include "hid-ids.h"
> >
> > @@ -1130,8 +1131,12 @@ static void sony_input_configured(struct hid_device *hdev,
> > */
> > static int sixaxis_set_operational_usb(struct hid_device *hdev)
> > {
> > + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> > + struct usb_device *dev = interface_to_usbdev(intf);
> > int ret;
> > - char *buf = kmalloc(18, GFP_KERNEL);
> > + char *buf = kmalloc(65, GFP_KERNEL);
>
> 18 or 65 should be retrieved by the description of the device, not hardcoded.
>
However I don't mind about hardcoding these values here.
The device report descriptor does not describe all the feature reports,
it never has and it's not really worth doing that IMHO.
> > + unsigned char buf2[] = { 0x00 };
> > + int transfered;
> >
> > if (!buf)
> > return -ENOMEM;
> > @@ -1140,7 +1145,24 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
> > HID_REQ_GET_REPORT);
> >
> > if (ret < 0)
> > - hid_err(hdev, "can't set operational mode\n");
> > + hid_err(hdev, "can't set operational mode on the control EP\n");
> > +
> > + /*
> > + * Some compatible controllers like the Speedlink Strike FX and
> > + * Gasia need another query plus an USB interrupt to get operational.
> > + */
> > + ret = hid_hw_raw_request(hdev, 0xf5, buf, 64, HID_FEATURE_REPORT,
> > + HID_REQ_GET_REPORT);
>
> 0xf5 should not be hardcoded. You have to retrieve it from the
> description of the device or at least put a special case for your
> specific game controller.
>
I find this acceptable, more so considering that the current code does
exactly the same for feature report 0xf2.
The operations are just ping-of-life tricks, nothing we want to expose.
> > +
> > + if (ret < 0)
> > + hid_err(hdev, "can't set operational mode on the interrupt EP\n");
The error message is not accurate in this case.
You could use something like "can't set operational mode: step 2" to
differentiate between the errors and call "step 1" the previous one.
> > +
> > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02),
> > + buf2, sizeof(buf2),
> > + &transfered, USB_CTRL_SET_TIMEOUT);
>
> Can't you simply use a hid_hw_output_report request instead of hard
> coding the device specific endpoint?
> And I'd also prefer it to be guarded against your specific controller.
>
usb_interrupt_msg() is called in usbhid_output_report() indeed, so it
should be possible to use the generic HID interface.
> > +
> > + if (ret < 0)
> > + hid_err(hdev, "can't set operational mode on the interrupt EP\n");
And adjust this message accordingly. Call it "step 3" perhaps?
> >
> > kfree(buf);
> >
Thanks,
Antonio
--
Antonio Ospite
http://ao2.it
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?
Hi,
On Sat, 7 Feb 2015 17:31:33 +0100
Antonio Ospite <[email protected]> wrote:
> > > +#include <linux/usb/input.h>
> >
> > Please don't.
> > HID should be transport agnostic, so please refrain from directly call usb.
> >
>
> I agree with Benjamin here.
>
> > > +
> > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02),
> > > + buf2, sizeof(buf2),
> > > + &transfered, USB_CTRL_SET_TIMEOUT);
> >
> > Can't you simply use a hid_hw_output_report request instead of hard
> > coding the device specific endpoint?
> > And I'd also prefer it to be guarded against your specific controller.
> >
>
> usb_interrupt_msg() is called in usbhid_output_report() indeed, so it
> should be possible to use the generic HID interface.
Regarding the USB. This is a comment from Andrew's patch:
// doesn't work. gets sent as a SET_REPORT Request intstead of a
// URB_INTERRUPT out. I guess usbhid->urbout is null
//if ( hdev->hid_output_raw_report(hdev, buf2, sizeof(buf2),
I took his word for it, and did not attempt it. Do you think it would
work with the current kernel? If so, I can test later. Had quite an
enjoyable evening playing some FF7 once I had the controller working ;)
Antonio, others, can you test with official Sony controllers that the
current patch did not break them? I only have the Gasia.
> > > + if (ret < 0)
> > > + hid_err(hdev, "can't set operational mode on the interrupt EP\n");
>
> And adjust this message accordingly. Call it "step 3" perhaps?
Sure, will edit the error messages.
- Lauri
On Sat, 2015-02-07 at 15:48 +0200, Lauri Kasanen wrote:
> Without this, my "Gasia Co.,Ltd PS(R) Gamepad" would not send
> any events. Now everything works including the leds.
>
> Based on work by Andrew Haines and Antonio Ospite.
>
> cc: Antonio Ospite <[email protected]>
> cc: Andrew Haines <[email protected]>
> Signed-off-by: Lauri Kasanen <[email protected]>
> ---
> drivers/hid/hid-sony.c | 26 ++++++++++++++++++++++++--
> 1 file changed, 24 insertions(+), 2 deletions(-)
>
> Antonio's original approach was not enough; it enabled the events,
> but only for a few seconds, then the controller timed out and sent
> no more. Andrew's did more than was necessary. This is a combination
> of the two, against Linus' git.
>
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 31e9d25..de93386 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -36,6 +36,7 @@
> #include <linux/list.h>
> #include <linux/idr.h>
> #include <linux/input/mt.h>
> +#include <linux/usb/input.h>
>
> #include "hid-ids.h"
>
> @@ -1130,8 +1131,12 @@ static void sony_input_configured(struct hid_device *hdev,
> */
> static int sixaxis_set_operational_usb(struct hid_device *hdev)
> {
> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> + struct usb_device *dev = interface_to_usbdev(intf);
> int ret;
> - char *buf = kmalloc(18, GFP_KERNEL);
> + char *buf = kmalloc(65, GFP_KERNEL);
> + unsigned char buf2[] = { 0x00 };
> + int transfered;
>
> if (!buf)
> return -ENOMEM;
> @@ -1140,7 +1145,24 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
> HID_REQ_GET_REPORT);
>
> if (ret < 0)
> - hid_err(hdev, "can't set operational mode\n");
> + hid_err(hdev, "can't set operational mode on the control EP\n");
> +
> + /*
> + * Some compatible controllers like the Speedlink Strike FX and
> + * Gasia need another query plus an USB interrupt to get operational.
> + */
> + ret = hid_hw_raw_request(hdev, 0xf5, buf, 64, HID_FEATURE_REPORT,
> + HID_REQ_GET_REPORT);
> +
> + if (ret < 0)
> + hid_err(hdev, "can't set operational mode on the interrupt EP\n");
> +
> + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02),
> + buf2, sizeof(buf2),
> + &transfered, USB_CTRL_SET_TIMEOUT);
You cannot do this. Even for a single byte DMA on the stack is
wrong. Not on all architectures it works at all and you violate
the DMA constrainsts. You must use kmalloc().
Regards
Oliver
On Mon, 09 Feb 2015 11:08:01 +0100
Oliver Neukum <[email protected]> wrote:
> > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02),
> > + buf2, sizeof(buf2),
> > + &transfered, USB_CTRL_SET_TIMEOUT);
>
> You cannot do this. Even for a single byte DMA on the stack is
> wrong. Not on all architectures it works at all and you violate
> the DMA constrainsts. You must use kmalloc().
Hi Oliver,
Does this still apply when using hid_hw_output_report?
- Lauri
On Mon, 2015-02-09 at 20:44 +0200, Lauri Kasanen wrote:
> On Mon, 09 Feb 2015 11:08:01 +0100
> Oliver Neukum <[email protected]> wrote:
>
> > > + ret = usb_interrupt_msg(dev, usb_sndintpipe(dev, 0x02),
> > > + buf2, sizeof(buf2),
> > > + &transfered, USB_CTRL_SET_TIMEOUT);
> >
> > You cannot do this. Even for a single byte DMA on the stack is
> > wrong. Not on all architectures it works at all and you violate
> > the DMA constrainsts. You must use kmalloc().
>
> Hi Oliver,
>
> Does this still apply when using hid_hw_output_report?
Yes. For USB devices hid_hw_output_report() goes to
usbhid_output_report(). That goes to usb_interrupt_msg(),
which passes the buffer pointer. It will then be mapped
for DMA. You must not do that on the stack.
HTH
Oliver