[PATCH 0/4] HID: ll transport cleanup: final round

[PATCH 0/4] HID: ll transport cleanup: final round

[Benjamin Tissoires]

Hi guys,

by shuffling around the hid files, I noticed that I introduced a regression
in commit 3a75b24949a8 (HID: hidraw: replace hid_output_raw_report() calls...).
We removed the hid_output_raw_report() calls in hidraw, which means that
the special sixasis handling was broken from the hidraw callers (I don't know
if there are any however).

So I took some time to finish this work. I am not particularly proud of patches
2/4 and 3/4:

For the patch 2/4, we need to get some feedbacks from David Barksdale,
because the spec seems a little bit nebulous to me and I'd prefer having real
tests to confirm that it is the right choice. I hope that David B. will send
his feedbacks soon. If not, I still would like to see this in 3.15 with the
rest of the cleanup to avoid having new drivers which use the old API.

Regarding 3/4, well, I added 2 quirks which are not so trivials, but I am not
sure I could do something else.

Anyway, happy reviewing.

Cheers,
Benjamin

Benjamin Tissoires (4):
  HID: cp2112: remove various hid_out_raw_report calls
  HID: cp2112: remove the last hid_output_raw_report() call
  HID: sony: do not rely on hid_out_raw_report
  HID: remove hid_output_raw_report transport implementations

 drivers/hid/hid-cp2112.c      | 28 +++++++++++++++-----
 drivers/hid/hid-sony.c        | 59 ++++++++++---------------------------------
 drivers/hid/hidraw.c          |  3 ++-
 drivers/hid/i2c-hid/i2c-hid.c | 14 ----------
 drivers/hid/uhid.c            |  1 -
 drivers/hid/usbhid/hid-core.c | 19 +++++---------
 include/linux/hid.h           | 21 ++-------------
 net/bluetooth/hidp/core.c     | 14 ----------
 8 files changed, 45 insertions(+), 114 deletions(-)

-- 
1.8.5.3

[PATCH 1/4] HID: cp2112: remove various hid_out_raw_report calls

[Benjamin Tissoires]

hid_out_raw_report is going to be obsoleted as it is not part of the
unified HID low level transport documentation
(Documentation/hid/hid-transport.txt)

  hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
is strictly equivalent to:
  hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
		HID_FEATURE_REPORT, HID_REQ_SET_REPORT);

So use the new api.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-cp2112.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 1025982..860db694 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -185,8 +185,8 @@ static int cp2112_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 	buf[1] &= ~(1 << offset);
 	buf[2] = gpio_push_pull;
 
-	ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf),
-					  HID_FEATURE_REPORT);
+	ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	if (ret < 0) {
 		hid_err(hdev, "error setting GPIO config: %d\n", ret);
 		return ret;
@@ -207,8 +207,8 @@ static void cp2112_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	buf[1] = value ? 0xff : 0;
 	buf[2] = 1 << offset;
 
-	ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf),
-					  HID_FEATURE_REPORT);
+	ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	if (ret < 0)
 		hid_err(hdev, "error setting GPIO values: %d\n", ret);
 }
@@ -253,8 +253,8 @@ static int cp2112_gpio_direction_output(struct gpio_chip *chip,
 	buf[1] |= 1 << offset;
 	buf[2] = gpio_push_pull;
 
-	ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf),
-					  HID_FEATURE_REPORT);
+	ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	if (ret < 0) {
 		hid_err(hdev, "error setting GPIO config: %d\n", ret);
 		return ret;
-- 
1.8.5.3

[PATCH 2/4] HID: cp2112: remove the last hid_output_raw_report() call

[Benjamin Tissoires]

I don't have access to the device, so I copied/pasted the code
from hidraw.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-cp2112.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 860db694..c4f87bd 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -290,7 +290,21 @@ static int cp2112_hid_output(struct hid_device *hdev, u8 *data, size_t count,
 	if (!buf)
 		return -ENOMEM;
 
-	ret = hdev->hid_output_raw_report(hdev, buf, count, report_type);
+	/* Fixme: test which function is actually called for output reports */
+	if (report_type == HID_OUTPUT_REPORT) {
+		ret = hid_hw_output_report(hdev, buf, count);
+		/*
+		 * compatibility with old implementation of USB-HID:
+		 * if the device does not support receiving output reports,
+		 * on an interrupt endpoint, fallback to SET_REPORT HID command.
+		 */
+		if (ret != -ENOSYS)
+			goto out_free;
+	}
+
+	ret = hid_hw_raw_request(hdev, buf[0], buf, count, report_type,
+				HID_REQ_SET_REPORT);
+out_free:
 	kfree(buf);
 	return ret;
 }
-- 
1.8.5.3

[PATCH 3/4] HID: sony: do not rely on hid_output_raw_report

[Benjamin Tissoires]

hid_out_raw_report is going to be obsoleted as it is not part of the
unified HID low level transport documentation
(Documentation/hid/hid-transport.txt)

To do so, we need to introduce two new quirks:
* HID_QUIRK_NO_OUTPUT_REPORTS: this quirks prevents the transport
  driver to use the interrupt channel to send output report (and thus
  force to use HID_REQ_SET_REPORT command)
* HID_QUIRK_SKIP_OUTPUT_REPORT_ID: this one forces usbhid to not
  include the report ID in the buffer it sends to the device through
  HID_REQ_SET_REPORT in case of an output report

This also fixes a regression introduced in commit 3a75b24949a8
(HID: hidraw: replace hid_output_raw_report() calls by appropriates ones).
The hidraw API was not able to communicate with the PS3 SixAxis
controllers in USB mode.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-sony.c        | 59 ++++++++++---------------------------------
 drivers/hid/hidraw.c          |  3 ++-
 drivers/hid/usbhid/hid-core.c |  7 ++++-
 include/linux/hid.h           |  2 ++
 4 files changed, 24 insertions(+), 47 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index b5fe65e..08eac71 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1007,45 +1007,6 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
 }
 
 /*
- * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
- * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
- * so we need to override that forcing HID Output Reports on the Control EP.
- *
- * There is also another issue about HID Output Reports via USB, the Sixaxis
- * does not want the report_id as part of the data packet, so we have to
- * discard buf[0] when sending the actual control message, even for numbered
- * reports, humpf!
- */
-static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
-		size_t count, unsigned char report_type)
-{
-	struct usb_interface *intf = to_usb_interface(hid->dev.parent);
-	struct usb_device *dev = interface_to_usbdev(intf);
-	struct usb_host_interface *interface = intf->cur_altsetting;
-	int report_id = buf[0];
-	int ret;
-
-	if (report_type == HID_OUTPUT_REPORT) {
-		/* Don't send the Report ID */
-		buf++;
-		count--;
-	}
-
-	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
-		HID_REQ_SET_REPORT,
-		USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-		((report_type + 1) << 8) | report_id,
-		interface->desc.bInterfaceNumber, buf, count,
-		USB_CTRL_SET_TIMEOUT);
-
-	/* Count also the Report ID, in case of an Output report. */
-	if (ret > 0 && report_type == HID_OUTPUT_REPORT)
-		ret++;
-
-	return ret;
-}
-
-/*
  * Sending HID_REQ_GET_REPORT changes the operation mode of the ps3 controller
  * to "operational".  Without this, the ps3 controller will not report any
  * events.
@@ -1305,11 +1266,8 @@ static void sixaxis_state_worker(struct work_struct *work)
 	buf[10] |= sc->led_state[2] << 3;
 	buf[10] |= sc->led_state[3] << 4;
 
-	if (sc->quirks & SIXAXIS_CONTROLLER_USB)
-		hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
-	else
-		hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf),
-				HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
+	hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf), HID_OUTPUT_REPORT,
+			HID_REQ_SET_REPORT);
 }
 
 static void dualshock4_state_worker(struct work_struct *work)
@@ -1659,7 +1617,18 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 
 	if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
-		hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
+		/*
+		 * The Sony Sixaxis does not handle HID Output Reports on the
+		 * Interrupt EP like it could, so we need to forcing HID Output
+		 * Reports to use HID_REQ_SET_REPORT on the Control EP.
+		 *
+		 * There is also another issue about HID Output Reports via USB,
+		 * the Sixaxis does not want the report_id as part of the data
+		 * packet, so we have to discard buf[0] when sending the actual
+		 * control message, even for numbered reports, humpf!
+		 */
+		hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS;
+		hdev->quirks |= HID_QUIRK_SKIP_OUTPUT_REPORT_ID;
 		ret = sixaxis_set_operational_usb(hdev);
 		sc->worker_initialized = 1;
 		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 2cc484c..6537e58 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -149,7 +149,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
 		goto out_free;
 	}
 
-	if (report_type == HID_OUTPUT_REPORT) {
+	if ((report_type == HID_OUTPUT_REPORT) &&
+	    !(dev->quirks & HID_QUIRK_NO_OUTPUT_REPORTS)) {
 		ret = hid_hw_output_report(dev, buf, count);
 		/*
 		 * compatibility with old implementation of USB-HID and I2C-HID:
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 0d1d875..3bc7cad 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -894,7 +894,12 @@ static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
 	int ret, skipped_report_id = 0;
 
 	/* Byte 0 is the report number. Report data starts at byte 1.*/
-	buf[0] = reportnum;
+	if ((rtype == HID_OUTPUT_REPORT) &&
+	    (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORT_ID))
+		buf[0] = 0;
+	else
+		buf[0] = reportnum;
+
 	if (buf[0] == 0x0) {
 		/* Don't send the Report ID */
 		buf++;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 5eb282e..2baf834 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -287,6 +287,8 @@ struct hid_item {
 #define HID_QUIRK_NO_EMPTY_INPUT		0x00000100
 #define HID_QUIRK_NO_INIT_INPUT_REPORTS		0x00000200
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
+#define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		0x00020000
+#define HID_QUIRK_NO_OUTPUT_REPORTS		0x00040000
 #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
 #define HID_QUIRK_NO_INIT_REPORTS		0x20000000
 #define HID_QUIRK_NO_IGNORE			0x40000000
-- 
1.8.5.3

[PATCH 4/4] HID: remove hid_output_raw_report transport implementations

[Benjamin Tissoires]

Nobody calls hid_output_raw_report anymore, and nobody should.
We can now remove the various implementation in the different
transport drivers and the declarations.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 14 --------------
 drivers/hid/uhid.c            |  1 -
 drivers/hid/usbhid/hid-core.c | 12 ------------
 include/linux/hid.h           | 19 -------------------
 net/bluetooth/hidp/core.c     | 14 --------------
 5 files changed, 60 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index cbd44a7..8c52a07 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -596,19 +596,6 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
 	return ret;
 }
 
-static int __i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
-		size_t count, unsigned char report_type)
-{
-	struct i2c_client *client = hid->driver_data;
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
-	bool data = true; /* SET_REPORT */
-
-	if (report_type == HID_OUTPUT_REPORT)
-		data = le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0;
-
-	return i2c_hid_output_raw_report(hid, buf, count, report_type, data);
-}
-
 static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
 		size_t count)
 {
@@ -1037,7 +1024,6 @@ static int i2c_hid_probe(struct i2c_client *client,
 
 	hid->driver_data = client;
 	hid->ll_driver = &i2c_hid_ll_driver;
-	hid->hid_output_raw_report = __i2c_hid_output_raw_report;
 	hid->dev.parent = &client->dev;
 	ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
 	hid->bus = BUS_I2C;
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 60acee4..7ed79be 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -400,7 +400,6 @@ static int uhid_dev_create(struct uhid_device *uhid,
 	hid->uniq[63] = 0;
 
 	hid->ll_driver = &uhid_hid_driver;
-	hid->hid_output_raw_report = uhid_hid_output_raw;
 	hid->bus = ev->u.create.bus;
 	hid->vendor = ev->u.create.vendor;
 	hid->product = ev->u.create.product;
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 3bc7cad..7b88f4c 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -950,17 +950,6 @@ static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
 	return ret;
 }
 
-static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf,
-		size_t count, unsigned char report_type)
-{
-	struct usbhid_device *usbhid = hid->driver_data;
-
-	if (usbhid->urbout && report_type != HID_FEATURE_REPORT)
-		return usbhid_output_report(hid, buf, count);
-
-	return usbhid_set_raw_report(hid, buf[0], buf, count, report_type);
-}
-
 static void usbhid_restart_queues(struct usbhid_device *usbhid)
 {
 	if (usbhid->urbout && !test_bit(HID_OUT_RUNNING, &usbhid->iofl))
@@ -1294,7 +1283,6 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
 
 	usb_set_intfdata(intf, hid);
 	hid->ll_driver = &usb_hid_driver;
-	hid->hid_output_raw_report = usbhid_output_raw_report;
 	hid->ff_init = hid_pidff_init;
 #ifdef CONFIG_USB_HIDDEV
 	hid->hiddev_connect = hiddev_connect;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 2baf834..622eb4a 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -510,9 +510,6 @@ struct hid_device {							/* device report descriptor */
 				  struct hid_usage *, __s32);
 	void (*hiddev_report_event) (struct hid_device *, struct hid_report *);
 
-	/* handler for raw output data, used by hidraw */
-	int (*hid_output_raw_report) (struct hid_device *, __u8 *, size_t, unsigned char);
-
 	/* debugging support via debugfs */
 	unsigned short debug;
 	struct dentry *debug_dir;
@@ -1020,22 +1017,6 @@ static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
 }
 
 /**
- * hid_output_raw_report - send an output or a feature report to the device
- *
- * @hdev: hid device
- * @buf: raw data to transfer
- * @len: length of buf
- * @report_type: HID_FEATURE_REPORT or HID_OUTPUT_REPORT
- *
- * @return: count of data transfered, negative if error
- */
-static inline int hid_output_raw_report(struct hid_device *hdev, __u8 *buf,
-					size_t len, unsigned char report_type)
-{
-	return hdev->hid_output_raw_report(hdev, buf, len, report_type);
-}
-
-/**
  * hid_hw_idle - send idle request to device
  *
  * @hdev: hid device
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 98e4840..514ddb5 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -382,18 +382,6 @@ static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
 				      data, count);
 }
 
-static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data,
-		size_t count, unsigned char report_type)
-{
-	if (report_type == HID_OUTPUT_REPORT) {
-		return hidp_output_report(hid, data, count);
-	} else if (report_type != HID_FEATURE_REPORT) {
-		return -EINVAL;
-	}
-
-	return hidp_set_raw_report(hid, data[0], data, count, report_type);
-}
-
 static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
 			    __u8 *buf, size_t len, unsigned char rtype,
 			    int reqtype)
@@ -776,8 +764,6 @@ static int hidp_setup_hid(struct hidp_session *session,
 	hid->dev.parent = &session->conn->hcon->dev;
 	hid->ll_driver = &hidp_hid_driver;
 
-	hid->hid_output_raw_report = hidp_output_raw_report;
-
 	/* True if device is blacklisted in drivers/hid/hid-core.c */
 	if (hid_ignore(hid)) {
 		hid_destroy_device(session->hid);
-- 
1.8.5.3

Re: [PATCH 3/4] HID: sony: do not rely on hid_output_raw_report

[Antonio Ospite]

Hi Benjamin,

On Fri, 28 Feb 2014 19:20:36 -0500
Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:

> hid_out_raw_report is going to be obsoleted as it is not part of the
> unified HID low level transport documentation
> (Documentation/hid/hid-transport.txt)
> 
> To do so, we need to introduce two new quirks:
> * HID_QUIRK_NO_OUTPUT_REPORTS: this quirks prevents the transport
>   driver to use the interrupt channel to send output report (and thus
>   force to use HID_REQ_SET_REPORT command)

Maybe a little more informative name? Like:
	HID_QUIRK_NON_STD_OUTPUT_REPORTS
or
	HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP
or expansions of the above.

The rationale being that, from outside, these are still output reports
(kind of), it's the channel they are sent over to which changes.

Another comment about a typo is inlined below.

Thanks,
   Antonio

> * HID_QUIRK_SKIP_OUTPUT_REPORT_ID: this one forces usbhid to not
>   include the report ID in the buffer it sends to the device through
>   HID_REQ_SET_REPORT in case of an output report
> 
> This also fixes a regression introduced in commit 3a75b24949a8
> (HID: hidraw: replace hid_output_raw_report() calls by appropriates ones).
> The hidraw API was not able to communicate with the PS3 SixAxis
> controllers in USB mode.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-sony.c        | 59 ++++++++++---------------------------------
>  drivers/hid/hidraw.c          |  3 ++-
>  drivers/hid/usbhid/hid-core.c |  7 ++++-
>  include/linux/hid.h           |  2 ++
>  4 files changed, 24 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index b5fe65e..08eac71 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -1007,45 +1007,6 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
>  }
>  
>  /*
> - * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
> - * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
> - * so we need to override that forcing HID Output Reports on the Control EP.
> - *
> - * There is also another issue about HID Output Reports via USB, the Sixaxis
> - * does not want the report_id as part of the data packet, so we have to
> - * discard buf[0] when sending the actual control message, even for numbered
> - * reports, humpf!
> - */
> -static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
> -		size_t count, unsigned char report_type)
> -{
> -	struct usb_interface *intf = to_usb_interface(hid->dev.parent);
> -	struct usb_device *dev = interface_to_usbdev(intf);
> -	struct usb_host_interface *interface = intf->cur_altsetting;
> -	int report_id = buf[0];
> -	int ret;
> -
> -	if (report_type == HID_OUTPUT_REPORT) {
> -		/* Don't send the Report ID */
> -		buf++;
> -		count--;
> -	}
> -
> -	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
> -		HID_REQ_SET_REPORT,
> -		USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> -		((report_type + 1) << 8) | report_id,
> -		interface->desc.bInterfaceNumber, buf, count,
> -		USB_CTRL_SET_TIMEOUT);
> -
> -	/* Count also the Report ID, in case of an Output report. */
> -	if (ret > 0 && report_type == HID_OUTPUT_REPORT)
> -		ret++;
> -
> -	return ret;
> -}
> -
> -/*
>   * Sending HID_REQ_GET_REPORT changes the operation mode of the ps3 controller
>   * to "operational".  Without this, the ps3 controller will not report any
>   * events.
> @@ -1305,11 +1266,8 @@ static void sixaxis_state_worker(struct work_struct *work)
>  	buf[10] |= sc->led_state[2] << 3;
>  	buf[10] |= sc->led_state[3] << 4;
>  
> -	if (sc->quirks & SIXAXIS_CONTROLLER_USB)
> -		hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
> -	else
> -		hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf),
> -				HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
> +	hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf), HID_OUTPUT_REPORT,
> +			HID_REQ_SET_REPORT);
>  }
>  
>  static void dualshock4_state_worker(struct work_struct *work)
> @@ -1659,7 +1617,18 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	}
>  
>  	if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
> -		hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
> +		/*
> +		 * The Sony Sixaxis does not handle HID Output Reports on the
> +		 * Interrupt EP like it could, so we need to forcing HID Output
                                                             ^^^^
typo: "we need to force".

> +		 * Reports to use HID_REQ_SET_REPORT on the Control EP.
> +		 *
> +		 * There is also another issue about HID Output Reports via USB,
> +		 * the Sixaxis does not want the report_id as part of the data
> +		 * packet, so we have to discard buf[0] when sending the actual
> +		 * control message, even for numbered reports, humpf!
> +		 */
> +		hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS;
> +		hdev->quirks |= HID_QUIRK_SKIP_OUTPUT_REPORT_ID;
>  		ret = sixaxis_set_operational_usb(hdev);
>  		sc->worker_initialized = 1;
>  		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 2cc484c..6537e58 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -149,7 +149,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>  		goto out_free;
>  	}
>  
> -	if (report_type == HID_OUTPUT_REPORT) {
> +	if ((report_type == HID_OUTPUT_REPORT) &&
> +	    !(dev->quirks & HID_QUIRK_NO_OUTPUT_REPORTS)) {
>  		ret = hid_hw_output_report(dev, buf, count);
>  		/*
>  		 * compatibility with old implementation of USB-HID and I2C-HID:
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 0d1d875..3bc7cad 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -894,7 +894,12 @@ static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
>  	int ret, skipped_report_id = 0;
>  
>  	/* Byte 0 is the report number. Report data starts at byte 1.*/
> -	buf[0] = reportnum;
> +	if ((rtype == HID_OUTPUT_REPORT) &&
> +	    (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORT_ID))
> +		buf[0] = 0;
> +	else
> +		buf[0] = reportnum;
> +
>  	if (buf[0] == 0x0) {
>  		/* Don't send the Report ID */
>  		buf++;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 5eb282e..2baf834 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -287,6 +287,8 @@ struct hid_item {
>  #define HID_QUIRK_NO_EMPTY_INPUT		0x00000100
>  #define HID_QUIRK_NO_INIT_INPUT_REPORTS		0x00000200
>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
> +#define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		0x00020000
> +#define HID_QUIRK_NO_OUTPUT_REPORTS		0x00040000
>  #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
>  #define HID_QUIRK_NO_INIT_REPORTS		0x20000000
>  #define HID_QUIRK_NO_IGNORE			0x40000000
> -- 
> 1.8.5.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
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?

Re: [PATCH 3/4] HID: sony: do not rely on hid_output_raw_report

[Benjamin Tissoires]

On Sat, Mar 1, 2014 at 7:16 AM, Antonio Ospite <ao2@ao2.it> wrote:
> Hi Benjamin,
>
> On Fri, 28 Feb 2014 19:20:36 -0500
> Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>
>> hid_out_raw_report is going to be obsoleted as it is not part of the
>> unified HID low level transport documentation
>> (Documentation/hid/hid-transport.txt)
>>
>> To do so, we need to introduce two new quirks:
>> * HID_QUIRK_NO_OUTPUT_REPORTS: this quirks prevents the transport
>>   driver to use the interrupt channel to send output report (and thus
>>   force to use HID_REQ_SET_REPORT command)
>
> Maybe a little more informative name? Like:
>         HID_QUIRK_NON_STD_OUTPUT_REPORTS
> or
>         HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP
> or expansions of the above.
>
> The rationale being that, from outside, these are still output reports
> (kind of), it's the channel they are sent over to which changes.

Sorry, putting names on things has always been my weakness. I like
more the second proposition, so I will pick it in the v2 unless
someone else complains.

>
> Another comment about a typo is inlined below.
>
> Thanks,
>    Antonio
>
>> * HID_QUIRK_SKIP_OUTPUT_REPORT_ID: this one forces usbhid to not
>>   include the report ID in the buffer it sends to the device through
>>   HID_REQ_SET_REPORT in case of an output report
>>
>> This also fixes a regression introduced in commit 3a75b24949a8
>> (HID: hidraw: replace hid_output_raw_report() calls by appropriates ones).
>> The hidraw API was not able to communicate with the PS3 SixAxis
>> controllers in USB mode.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>  drivers/hid/hid-sony.c        | 59 ++++++++++---------------------------------
>>  drivers/hid/hidraw.c          |  3 ++-
>>  drivers/hid/usbhid/hid-core.c |  7 ++++-
>>  include/linux/hid.h           |  2 ++
>>  4 files changed, 24 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index b5fe65e..08eac71 100644
>> --- a/drivers/hid/hid-sony.c
>> +++ b/drivers/hid/hid-sony.c
>> @@ -1007,45 +1007,6 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
>>  }
>>
>>  /*
>> - * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
>> - * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
>> - * so we need to override that forcing HID Output Reports on the Control EP.
>> - *
>> - * There is also another issue about HID Output Reports via USB, the Sixaxis
>> - * does not want the report_id as part of the data packet, so we have to
>> - * discard buf[0] when sending the actual control message, even for numbered
>> - * reports, humpf!
>> - */
>> -static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
>> -             size_t count, unsigned char report_type)
>> -{
>> -     struct usb_interface *intf = to_usb_interface(hid->dev.parent);
>> -     struct usb_device *dev = interface_to_usbdev(intf);
>> -     struct usb_host_interface *interface = intf->cur_altsetting;
>> -     int report_id = buf[0];
>> -     int ret;
>> -
>> -     if (report_type == HID_OUTPUT_REPORT) {
>> -             /* Don't send the Report ID */
>> -             buf++;
>> -             count--;
>> -     }
>> -
>> -     ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
>> -             HID_REQ_SET_REPORT,
>> -             USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
>> -             ((report_type + 1) << 8) | report_id,
>> -             interface->desc.bInterfaceNumber, buf, count,
>> -             USB_CTRL_SET_TIMEOUT);
>> -
>> -     /* Count also the Report ID, in case of an Output report. */
>> -     if (ret > 0 && report_type == HID_OUTPUT_REPORT)
>> -             ret++;
>> -
>> -     return ret;
>> -}
>> -
>> -/*
>>   * Sending HID_REQ_GET_REPORT changes the operation mode of the ps3 controller
>>   * to "operational".  Without this, the ps3 controller will not report any
>>   * events.
>> @@ -1305,11 +1266,8 @@ static void sixaxis_state_worker(struct work_struct *work)
>>       buf[10] |= sc->led_state[2] << 3;
>>       buf[10] |= sc->led_state[3] << 4;
>>
>> -     if (sc->quirks & SIXAXIS_CONTROLLER_USB)
>> -             hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
>> -     else
>> -             hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf),
>> -                             HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
>> +     hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf), HID_OUTPUT_REPORT,
>> +                     HID_REQ_SET_REPORT);
>>  }
>>
>>  static void dualshock4_state_worker(struct work_struct *work)
>> @@ -1659,7 +1617,18 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>       }
>>
>>       if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
>> -             hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
>> +             /*
>> +              * The Sony Sixaxis does not handle HID Output Reports on the
>> +              * Interrupt EP like it could, so we need to forcing HID Output
>                                                              ^^^^
> typo: "we need to force".
>

k, will fix in v2.

Thanks,
Benjamin

>> +              * Reports to use HID_REQ_SET_REPORT on the Control EP.
>> +              *
>> +              * There is also another issue about HID Output Reports via USB,
>> +              * the Sixaxis does not want the report_id as part of the data
>> +              * packet, so we have to discard buf[0] when sending the actual
>> +              * control message, even for numbered reports, humpf!
>> +              */
>> +             hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS;
>> +             hdev->quirks |= HID_QUIRK_SKIP_OUTPUT_REPORT_ID;
>>               ret = sixaxis_set_operational_usb(hdev);
>>               sc->worker_initialized = 1;
>>               INIT_WORK(&sc->state_worker, sixaxis_state_worker);
>> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
>> index 2cc484c..6537e58 100644
>> --- a/drivers/hid/hidraw.c
>> +++ b/drivers/hid/hidraw.c
>> @@ -149,7 +149,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>>               goto out_free;
>>       }
>>
>> -     if (report_type == HID_OUTPUT_REPORT) {
>> +     if ((report_type == HID_OUTPUT_REPORT) &&
>> +         !(dev->quirks & HID_QUIRK_NO_OUTPUT_REPORTS)) {
>>               ret = hid_hw_output_report(dev, buf, count);
>>               /*
>>                * compatibility with old implementation of USB-HID and I2C-HID:
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 0d1d875..3bc7cad 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -894,7 +894,12 @@ static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
>>       int ret, skipped_report_id = 0;
>>
>>       /* Byte 0 is the report number. Report data starts at byte 1.*/
>> -     buf[0] = reportnum;
>> +     if ((rtype == HID_OUTPUT_REPORT) &&
>> +         (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORT_ID))
>> +             buf[0] = 0;
>> +     else
>> +             buf[0] = reportnum;
>> +
>>       if (buf[0] == 0x0) {
>>               /* Don't send the Report ID */
>>               buf++;
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 5eb282e..2baf834 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -287,6 +287,8 @@ struct hid_item {
>>  #define HID_QUIRK_NO_EMPTY_INPUT             0x00000100
>>  #define HID_QUIRK_NO_INIT_INPUT_REPORTS              0x00000200
>>  #define HID_QUIRK_SKIP_OUTPUT_REPORTS                0x00010000
>> +#define HID_QUIRK_SKIP_OUTPUT_REPORT_ID              0x00020000
>> +#define HID_QUIRK_NO_OUTPUT_REPORTS          0x00040000
>>  #define HID_QUIRK_FULLSPEED_INTERVAL         0x10000000
>>  #define HID_QUIRK_NO_INIT_REPORTS            0x20000000
>>  #define HID_QUIRK_NO_IGNORE                  0x40000000
>> --
>> 1.8.5.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> --
> 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?

Re: [PATCH 2/4] HID: cp2112: remove the last hid_output_raw_report() call

[Jiri Kosina]

On Fri, 28 Feb 2014, Benjamin Tissoires wrote:

> I don't have access to the device, so I copied/pasted the code
> from hidraw.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/hid/hid-cp2112.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index 860db694..c4f87bd 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -290,7 +290,21 @@ static int cp2112_hid_output(struct hid_device *hdev, u8 *data, size_t count,
>  	if (!buf)
>  		return -ENOMEM;
>  
> -	ret = hdev->hid_output_raw_report(hdev, buf, count, report_type);
> +	/* Fixme: test which function is actually called for output reports */

I don't completely understand this Fixme (oh, and please spell it as 
'FIXME:' so that we are consistent with all the other instances), could 
you please elaborate?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/4] HID: cp2112: remove the last hid_output_raw_report() call

[Benjamin Tissoires]

On Mar 04 2014 or thereabouts, Jiri Kosina wrote:
> On Fri, 28 Feb 2014, Benjamin Tissoires wrote:
> 
> > I don't have access to the device, so I copied/pasted the code
> > from hidraw.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  drivers/hid/hid-cp2112.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> > index 860db694..c4f87bd 100644
> > --- a/drivers/hid/hid-cp2112.c
> > +++ b/drivers/hid/hid-cp2112.c
> > @@ -290,7 +290,21 @@ static int cp2112_hid_output(struct hid_device *hdev, u8 *data, size_t count,
> >  	if (!buf)
> >  		return -ENOMEM;
> >  
> > -	ret = hdev->hid_output_raw_report(hdev, buf, count, report_type);
> > +	/* Fixme: test which function is actually called for output reports */
> 
> I don't completely understand this Fixme (oh, and please spell it as 
> 'FIXME:' so that we are consistent with all the other instances), could 
> you please elaborate?

Well, sorry:
As I said, this part is a copy/paste of what is in hidraw. However, this
just reflect that we don't know how the device actually behave, which is
not very elegant. I have currently no clues of which function will be
actually called for output reports: hid_hw_output_report() or
hid_hw_raw_request(). Once we got the confirmation of which function is
called, we could make the path more straightforward.

I bought one of these (it may help debugging some Synaptics devices),
and I'll receive it by the end of the week. So by next week, we should
get the actual code path and remove this FIXME.

I need to send a v2 of hid-sony in any cases, so I guess you should not
pull these 4 patches right away. If you prefer having this in linux-next,
the sooner, I can also send the v2 right away, and we will fix this
cp2112 driver next week.

Cheers,
Benjamin

> 
> Thanks,
> 
> -- 
> Jiri Kosina
> SUSE Labs

Re: [PATCH 2/4] HID: cp2112: remove the last hid_output_raw_report() call

[Jiri Kosina]

On Tue, 4 Mar 2014, Benjamin Tissoires wrote:

> I need to send a v2 of hid-sony in any cases, so I guess you should not
> pull these 4 patches right away. If you prefer having this in linux-next,
> the sooner, I can also send the v2 right away, and we will fix this
> cp2112 driver next week.

That's fine, I'll wait for the whole v2 respin. Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 2/4] HID: cp2112: remove the last hid_output_raw_report() call

[Benjamin Tissoires]

On Wed, Mar 5, 2014 at 1:11 PM, David Barksdale <dbarksdale@uplogix.com> wrote:
> Sorry for the delay, hid_hw_output_report() is the correct function.

No worries and thanks for the tests.

This saves me some time!

Sending the v2 right away.

Cheers,
Benjamin

>
>
> On March 4, 2014 8:18:38 AM CST, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>>
>> On Mar 04 2014 or thereabouts, Jiri Kosina wrote:
>>>
>>>  On Fri, 28 Feb 2014, Benjamin Tissoires wrote:
>>>
>>>>
>>>>  I don't have access to the device, so I copied/pasted the code
>>>>  from hidraw.
>>>>
>>>>  Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>>>  ---
>>>>   drivers/hid/hid-cp2112.c | 16 +++++++++++++++-
>>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>>  diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
>>>>  index 860db694..c4f87bd 100644
>>>>  --- a/drivers/hid/hid-cp2112.c
>>>>  +++ b/drivers/hid/hid-cp2112.c
>>>>  @@ -290,7 +290,21 @@ static int cp2112_hid_output(struct hid_device
>>>> *hdev, u8 *data, size_t count,
>>>>    if (!buf)
>>>>     return -ENOMEM;
>>>>
>>>>  - ret = hdev->hid_output_raw_report(hdev, buf, count, report_type);
>>>>  + /* Fixme: test which function is actually called for output reports
>>>> */
>>>
>>>
>>>  I don't completely understand this Fixme (oh, and please spell it as
>>>  'FIXME:' so that we are consistent with all the other instances), could
>>>  you please elaborate?
>>
>>
>> Well, sorry:
>> As I said, this part is a copy/paste of what is in hidraw. However, this
>> just reflect that we don't know how the device actually behave, which is
>> not very elegant. I have currently no clues of which function will be
>> actually called for output reports: hid_hw_output_report() or
>> hid_hw_raw_request(). Once we got the confirmation of which function is
>> called, we could make the path more straightforward.
>>
>> I bought one of these (it may help debugging some Synaptics devices),
>> and I'll receive it by the end of the week. So by next week, we should
>> get the actual code path and remove this FIXME.
>>
>> I need to send a v2 of hid-sony in any cases, so I guess you should not
>> pull these 4 patches right away. If you prefer having this in linux-next,
>> the sooner, I can also send the v2 right away, and we will fix this
>> cp2112 driver next week.
>>
>> Cheers,
>> Benjamin
>>
>>>
>>>  Thanks,
>>>
>>>  --
>>>  Jiri Kosina
>>>  SUSE Labs
>
>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.