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

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

[Benjamin Tissoires]

Alright, this is the re-spin of the last round of transport cleanup.

Some minor but important modifications are here, but nothing very enthousiastic.

Thanks for the reviews and the tests so far.

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_output_raw_report
  HID: remove hid_output_raw_report transport implementations

 drivers/hid/hid-cp2112.c      | 19 +++++++++-----
 drivers/hid/hid-sony.c        | 60 ++++++++++---------------------------------
 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, 36 insertions(+), 115 deletions(-)

-- 
1.8.5.3

[PATCH v2 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>
---

no changes since v1

 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 v2 2/4] HID: cp2112: remove the last hid_output_raw_report() call

[Benjamin Tissoires]

tests have shown that output reports use hid_hw_output_report().

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---

changes since v1:
- removed FIXME
- actually use the proper calls and do not guess

 drivers/hid/hid-cp2112.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 860db694..a4fff699 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -290,7 +290,12 @@ 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);
+	if (report_type == HID_OUTPUT_REPORT)
+		ret = hid_hw_output_report(hdev, buf, count);
+	else
+		ret = hid_hw_raw_request(hdev, buf[0], buf, count, report_type,
+				HID_REQ_SET_REPORT);
+
 	kfree(buf);
 	return ret;
 }
-- 
1.8.5.3

[PATCH v2 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_ON_INTR_EP: 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>
---

changes since v1:
- removed usb.h include
- renamed HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP
- fix typo

 drivers/hid/hid-sony.c        | 60 ++++++++++---------------------------------
 drivers/hid/hidraw.c          |  3 ++-
 drivers/hid/usbhid/hid-core.c |  7 ++++-
 include/linux/hid.h           |  2 ++
 4 files changed, 24 insertions(+), 48 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index b5fe65e..4884bb5 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -29,7 +29,6 @@
 #include <linux/hid.h>
 #include <linux/module.h>
 #include <linux/slab.h>
-#include <linux/usb.h>
 #include <linux/leds.h>
 #include <linux/power_supply.h>
 #include <linux/spinlock.h>
@@ -1007,45 +1006,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 +1265,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 +1616,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 force 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_ON_INTR_EP;
+		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..ffa648c 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_ON_INTR_EP)) {
 		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 e224516..2cd7174 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -290,6 +290,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_ON_INTR_EP	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 v2 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>
---

no changes since v1 (what did you expected?)

 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 2cd7174..720e3a1 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -513,9 +513,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;
@@ -1023,22 +1020,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 v2 1/4] HID: cp2112: remove various hid_out_raw_report calls

[David Herrmann]

Hi

On Wed, Mar 5, 2014 at 10:18 PM, 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)
>
>   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);

This time you might be right that feature-reports always put the
report-id into the first byte, but I'd still prefer if we avoid using
this. Besides, the code is much nicer imho if we pass the ID directly,
see below..

>
> So use the new api.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>
> no changes since v1
>
>  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);

buf[0] => CP2112_GPIO_CONFIG

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

Here buf[0] seems fine as it is set explicitly just 3 lines above to
CP2112_GPIO_SET.

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

Here an explicit CP2112_GPIO_CONFIG seems nicer, imho.

Thanks
David

>         if (ret < 0) {
>                 hid_err(hdev, "error setting GPIO config: %d\n", ret);
>                 return ret;
> --
> 1.8.5.3
>

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

[David Herrmann]

Hi

On Wed, Mar 5, 2014 at 10:18 PM, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
> Alright, this is the re-spin of the last round of transport cleanup.
>
> Some minor but important modifications are here, but nothing very enthousiastic.
>
> Thanks for the reviews and the tests so far.

Apart from some minor comments on #1 and #2, this is:

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> 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_output_raw_report
>   HID: remove hid_output_raw_report transport implementations
>
>  drivers/hid/hid-cp2112.c      | 19 +++++++++-----
>  drivers/hid/hid-sony.c        | 60 ++++++++++---------------------------------
>  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, 36 insertions(+), 115 deletions(-)
>
> --
> 1.8.5.3

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

[Benjamin Tissoires]

On Fri, Mar 7, 2014 at 4:47 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Mar 5, 2014 at 10:18 PM, 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)
>>
>>   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);
>
> This time you might be right that feature-reports always put the
> report-id into the first byte, but I'd still prefer if we avoid using
> this. Besides, the code is much nicer imho if we pass the ID directly,
> see below..

Yes you are completely right.

Will send a v3 ASAP.

Cheers,
Benjamin

>
>>
>> So use the new api.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> ---
>>
>> no changes since v1
>>
>>  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);
>
> buf[0] => CP2112_GPIO_CONFIG
>
>>         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);
>
> Here buf[0] seems fine as it is set explicitly just 3 lines above to
> CP2112_GPIO_SET.
>
>>         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);
>
> Here an explicit CP2112_GPIO_CONFIG seems nicer, imho.
>
> Thanks
> David
>
>>         if (ret < 0) {
>>                 hid_err(hdev, "error setting GPIO config: %d\n", ret);
>>                 return ret;
>> --
>> 1.8.5.3
>>

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

[Benjamin Tissoires]

On Fri, Mar 7, 2014 at 4:52 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Mar 5, 2014 at 10:18 PM, Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
>> Alright, this is the re-spin of the last round of transport cleanup.
>>
>> Some minor but important modifications are here, but nothing very enthousiastic.
>>
>> Thanks for the reviews and the tests so far.
>
> Apart from some minor comments on #1 and #2, this is:

Hmm, neither my mailboxes nor patchwork gives me your comments on #2...

By looking at the code, (assuming you mean the same comment that you
made on #1), this is not so obvious.
What we can do is add an arg to cp2112_hid_output() with the report
number (and pass it to hid_hw_raw_request() ). However, this function
is also used by cp2112_xfer(), and there is chances that I introduce a
bug if I am not careful enough.
I'd rather split cp2112_hid_output() in two, one for output reports,
and one for features (with the reportID as an arg). I'll send this one
after this series because I would like to do some more tests.

>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>

Thanks a lot!

Cheers,
Benjamin

> Thanks
> David
>
>> 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_output_raw_report
>>   HID: remove hid_output_raw_report transport implementations
>>
>>  drivers/hid/hid-cp2112.c      | 19 +++++++++-----
>>  drivers/hid/hid-sony.c        | 60 ++++++++++---------------------------------
>>  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, 36 insertions(+), 115 deletions(-)
>>
>> --
>> 1.8.5.3