Hi guys,
This is an attempt to complete the branch for-3.15/ll-driver-new-callbacks:
- try to implement as much as possible ll_driver callbacks (some are still
missing, but I did not had the time to complete it)
- add inliners hid_hw_* for all the ll_driver callbacks
- remove transport dependant callbacks in struct hid_device
- remove the so called "obsolete" hidinput_input_event handler which was used
in 2/4 transport drivers
Bonus point: 14 files changed, 213 insertions(+), 272 deletions(-)
Yay!
I made sure I kept the PS3 controller working and the LEDs (on logitech-dj and
on bluetooth keyboard) working. The rest do not mostly need further testing,
the code path did not changed. But still, a review (and some tests) would be a
good idea :)
Cheers,
Benjamin
PS: obviously, this goes on top on the branch for-3.15/ll-driver-new-callbacks
Benjamin Tissoires (11):
HID: uHID: implement .raw_request
HID: i2c-hid: implement ll_driver transport-layer callbacks
HID: add inliners for ll_driver transport-layer callbacks
HID: logitech-dj: remove hidinput_input_event
HID: HIDp: remove hidp_hidinput_event
HID: remove hidinput_input_event handler
HID: HIDp: remove duplicated coded
HID: usbhid: remove duplicated code
HID: remove hid_get_raw_report in struct hid_device
HID: introduce helper to access hid_output_raw_report()
HID: move hid_output_raw_report to hid_ll_driver
drivers/hid/hid-input.c | 12 ++---
drivers/hid/hid-lg.c | 6 ++-
drivers/hid/hid-logitech-dj.c | 101 +++++++++++++---------------------
drivers/hid/hid-magicmouse.c | 2 +-
drivers/hid/hid-sony.c | 19 +++++--
drivers/hid/hid-thingm.c | 4 +-
drivers/hid/hid-wacom.c | 16 +++---
drivers/hid/hid-wiimote-core.c | 4 +-
drivers/hid/hidraw.c | 11 ++--
drivers/hid/i2c-hid/i2c-hid.c | 27 +++++++++-
drivers/hid/uhid.c | 20 ++++++-
drivers/hid/usbhid/hid-core.c | 67 +++++------------------
include/linux/hid.h | 77 ++++++++++++++++++++++----
net/bluetooth/hidp/core.c | 119 +++++------------------------------------
14 files changed, 213 insertions(+), 272 deletions(-)
--
1.8.3.1
It was missing, so adding it.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/uhid.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index f5a2b19..438c9f1 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -270,6 +270,22 @@ static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
return count;
}
+static int uhid_raw_request(struct hid_device *hid, unsigned char reportnum,
+ __u8 *buf, size_t len, unsigned char rtype,
+ int reqtype)
+{
+ switch (reqtype) {
+ case HID_REQ_GET_REPORT:
+ return uhid_hid_get_raw(hid, reportnum, buf, len, rtype);
+ case HID_REQ_SET_REPORT:
+ if (buf[0] != reportnum)
+ return -EINVAL;
+ return uhid_hid_output_raw(hid, buf, len, rtype);
+ default:
+ return -EIO;
+ }
+}
+
static struct hid_ll_driver uhid_hid_driver = {
.start = uhid_hid_start,
.stop = uhid_hid_stop,
@@ -277,6 +293,7 @@ static struct hid_ll_driver uhid_hid_driver = {
.close = uhid_hid_close,
.parse = uhid_hid_parse,
.output_report = uhid_hid_output_report,
+ .raw_request = uhid_raw_request,
};
#ifdef CONFIG_COMPAT
--
1.8.3.1
Those callbacks are not mandatory, so it's better to add inliners
to use them safely.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
include/linux/hid.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 003cc8e..dddcad0 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -680,6 +680,8 @@ struct hid_driver {
* shouldn't allocate anything to not leak memory
* @request: send report request to device (e.g. feature report)
* @wait: wait for buffered io to complete (send/recv reports)
+ * @raw_request: send raw report request to device (e.g. feature report)
+ * @output_report: send output report to device
* @idle: send idle request to device
*/
struct hid_ll_driver {
@@ -974,6 +976,49 @@ static inline void hid_hw_request(struct hid_device *hdev,
}
/**
+ * hid_hw_raw_request - send report request to device
+ *
+ * @hdev: hid device
+ * @reportnum: report ID
+ * @buf: in/out data to transfer
+ * @len: length of buf
+ * @rtype: HID report type
+ * @reqtype: HID_REQ_GET_REPORT or HID_REQ_SET_REPORT
+ *
+ * @return: count of data transfered, negative if error
+ *
+ * Same behavior as hid_hw_request, but with raw buffers instead.
+ */
+static inline int hid_hw_raw_request(struct hid_device *hdev,
+ unsigned char reportnum, __u8 *buf,
+ size_t len, unsigned char rtype, int reqtype)
+{
+ if (hdev->ll_driver->raw_request)
+ return hdev->ll_driver->raw_request(hdev, reportnum, buf, len,
+ rtype, reqtype);
+
+ return -ENOSYS;
+}
+
+/**
+ * hid_hw_output_report - send output report to device
+ *
+ * @hdev: hid device
+ * @buf: raw data to transfer
+ * @len: length of buf
+ *
+ * @return: count of data transfered, negative if error
+ */
+static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
+ size_t len)
+{
+ if (hdev->ll_driver->output_report)
+ return hdev->ll_driver->output_report(hdev, buf, len);
+
+ return -ENOSYS;
+}
+
+/**
* hid_hw_idle - send idle request to device
*
* @hdev: hid device
--
1.8.3.1
Add a helper to access hdev->hid_output_raw_report().
To convert the drivers, use the following snippets:
for i in drivers/hid/*.c
do
sed -i.bak "s/[^ \t]*->hid_output_raw_report(/hid_output_raw_report(/g" $i
done
Then manually fix for checkpatch.pl
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-input.c | 2 +-
drivers/hid/hid-lg.c | 6 ++++--
drivers/hid/hid-logitech-dj.c | 2 +-
drivers/hid/hid-magicmouse.c | 2 +-
drivers/hid/hid-sony.c | 5 +++--
drivers/hid/hid-thingm.c | 4 ++--
drivers/hid/hid-wacom.c | 16 +++++++---------
drivers/hid/hid-wiimote-core.c | 2 +-
drivers/hid/hidraw.c | 2 +-
include/linux/hid.h | 16 ++++++++++++++++
10 files changed, 37 insertions(+), 20 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 1de5997..78293c3 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1184,7 +1184,7 @@ static void hidinput_led_worker(struct work_struct *work)
hid_output_report(report, buf);
/* synchronous output report */
- hid->hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
+ hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
kfree(buf);
}
diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
index 9fe9d4a..76ed7e5 100644
--- a/drivers/hid/hid-lg.c
+++ b/drivers/hid/hid-lg.c
@@ -692,7 +692,8 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (hdev->product == USB_DEVICE_ID_LOGITECH_WII_WHEEL) {
unsigned char buf[] = { 0x00, 0xAF, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
- ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
+ ret = hid_output_raw_report(hdev, buf, sizeof(buf),
+ HID_FEATURE_REPORT);
if (ret >= 0) {
/* insert a little delay of 10 jiffies ~ 40ms */
@@ -704,7 +705,8 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id)
buf[1] = 0xB2;
get_random_bytes(&buf[2], 2);
- ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
+ ret = hid_output_raw_report(hdev, buf, sizeof(buf),
+ HID_FEATURE_REPORT);
}
}
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 61d2bbf..9347625 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -567,7 +567,7 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
out_buf[1] = djdev->device_index;
memcpy(out_buf + 2, buf, count);
- ret = djrcv_dev->hdev->hid_output_raw_report(djrcv_dev->hdev, out_buf,
+ ret = hid_output_raw_report(djrcv_dev->hdev, out_buf,
DJREPORT_SHORT_LENGTH, report_type);
kfree(out_buf);
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 3b43d1c..cb5db3a 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -538,7 +538,7 @@ static int magicmouse_probe(struct hid_device *hdev,
* but there seems to be no other way of switching the mode.
* Thus the super-ugly hacky success check below.
*/
- ret = hdev->hid_output_raw_report(hdev, feature, sizeof(feature),
+ ret = hid_output_raw_report(hdev, feature, sizeof(feature),
HID_FEATURE_REPORT);
if (ret != -EIO && ret != sizeof(feature)) {
hid_err(hdev, "unable to request touch data (%d)\n", ret);
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 3930acb..8494b8c 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -720,7 +720,8 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
static int sixaxis_set_operational_bt(struct hid_device *hdev)
{
unsigned char buf[] = { 0xf4, 0x42, 0x03, 0x00, 0x00 };
- return hdev->hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
+ return hid_output_raw_report(hdev, buf, sizeof(buf),
+ HID_FEATURE_REPORT);
}
static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
@@ -942,7 +943,7 @@ static void sixaxis_state_worker(struct work_struct *work)
buf[10] |= sc->led_state[2] << 3;
buf[10] |= sc->led_state[3] << 4;
- sc->hdev->hid_output_raw_report(sc->hdev, buf, sizeof(buf),
+ hid_output_raw_report(sc->hdev, buf, sizeof(buf),
HID_OUTPUT_REPORT);
}
diff --git a/drivers/hid/hid-thingm.c b/drivers/hid/hid-thingm.c
index 99342cf..7dd3197 100644
--- a/drivers/hid/hid-thingm.c
+++ b/drivers/hid/hid-thingm.c
@@ -48,8 +48,8 @@ static int blink1_send_command(struct blink1_data *data,
buf[0], buf[1], buf[2], buf[3], buf[4],
buf[5], buf[6], buf[7], buf[8]);
- ret = data->hdev->hid_output_raw_report(data->hdev, buf,
- BLINK1_CMD_SIZE, HID_FEATURE_REPORT);
+ ret = hid_output_raw_report(data->hdev, buf, BLINK1_CMD_SIZE,
+ HID_FEATURE_REPORT);
return ret < 0 ? ret : 0;
}
diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index 60c75dc..c720db9 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -128,8 +128,7 @@ static void wacom_set_image(struct hid_device *hdev, const char *image,
rep_data[0] = WAC_CMD_ICON_START_STOP;
rep_data[1] = 0;
- ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
- HID_FEATURE_REPORT);
+ ret = hid_output_raw_report(hdev, rep_data, 2, HID_FEATURE_REPORT);
if (ret < 0)
goto err;
@@ -143,15 +142,14 @@ static void wacom_set_image(struct hid_device *hdev, const char *image,
rep_data[j + 3] = p[(i << 6) + j];
rep_data[2] = i;
- ret = hdev->hid_output_raw_report(hdev, rep_data, 67,
+ ret = hid_output_raw_report(hdev, rep_data, 67,
HID_FEATURE_REPORT);
}
rep_data[0] = WAC_CMD_ICON_START_STOP;
rep_data[1] = 0;
- ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
- HID_FEATURE_REPORT);
+ ret = hid_output_raw_report(hdev, rep_data, 2, HID_FEATURE_REPORT);
err:
return;
@@ -183,7 +181,7 @@ static void wacom_leds_set_brightness(struct led_classdev *led_dev,
buf[3] = value;
/* use fixed brightness for OLEDs */
buf[4] = 0x08;
- hdev->hid_output_raw_report(hdev, buf, 9, HID_FEATURE_REPORT);
+ hid_output_raw_report(hdev, buf, 9, HID_FEATURE_REPORT);
kfree(buf);
}
@@ -339,7 +337,7 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
rep_data[0] = 0x03 ; rep_data[1] = 0x00;
limit = 3;
do {
- ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
+ ret = hid_output_raw_report(hdev, rep_data, 2,
HID_FEATURE_REPORT);
} while (ret < 0 && limit-- > 0);
@@ -352,7 +350,7 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
rep_data[1] = 0x00;
limit = 3;
do {
- ret = hdev->hid_output_raw_report(hdev,
+ ret = hid_output_raw_report(hdev,
rep_data, 2, HID_FEATURE_REPORT);
} while (ret < 0 && limit-- > 0);
@@ -378,7 +376,7 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
rep_data[0] = 0x03;
rep_data[1] = wdata->features;
- ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
+ ret = hid_output_raw_report(hdev, rep_data, 2,
HID_FEATURE_REPORT);
if (ret >= 0)
wdata->high_speed = speed;
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index abb20db..d7dc6c5b 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -35,7 +35,7 @@ static int wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
if (!buf)
return -ENOMEM;
- ret = hdev->hid_output_raw_report(hdev, buf, count, HID_OUTPUT_REPORT);
+ ret = hid_output_raw_report(hdev, buf, count, HID_OUTPUT_REPORT);
kfree(buf);
return ret;
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 4b2dc95..f8708c9 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -153,7 +153,7 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
goto out_free;
}
- ret = dev->hid_output_raw_report(dev, buf, count, report_type);
+ ret = hid_output_raw_report(dev, buf, count, report_type);
out_free:
kfree(buf);
out:
diff --git a/include/linux/hid.h b/include/linux/hid.h
index c56681a..a837ede 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -1012,6 +1012,22 @@ 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
--
1.8.3.1
struct hid_ll_driver is responsible for the transport communication.
Move hid_output_raw_report from hid_device to the transport layer then.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-input.c | 2 +-
drivers/hid/hid-logitech-dj.c | 2 +-
drivers/hid/hid-sony.c | 11 ++++++++++-
drivers/hid/hid-wiimote-core.c | 2 +-
drivers/hid/hidraw.c | 2 +-
drivers/hid/i2c-hid/i2c-hid.c | 2 +-
drivers/hid/uhid.c | 2 +-
drivers/hid/usbhid/hid-core.c | 2 +-
include/linux/hid.h | 11 +++++++----
net/bluetooth/hidp/core.c | 4 ++--
10 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 78293c3..3125155 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1263,7 +1263,7 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
}
input_set_drvdata(input_dev, hid);
- if (hid->ll_driver->request || hid->hid_output_raw_report)
+ if (hid->ll_driver->request || hid->ll_driver->hid_output_raw_report)
input_dev->event = hidinput_input_event;
input_dev->open = hidinput_open;
input_dev->close = hidinput_close;
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 9347625..bdfa1db 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -262,7 +262,6 @@ static void logi_dj_recv_add_djhid_device(struct dj_receiver_dev *djrcv_dev,
}
dj_hiddev->ll_driver = &logi_dj_ll_driver;
- dj_hiddev->hid_output_raw_report = logi_dj_output_hidraw_report;
dj_hiddev->dev.parent = &djrcv_hdev->dev;
dj_hiddev->bus = BUS_USB;
@@ -655,6 +654,7 @@ static struct hid_ll_driver logi_dj_ll_driver = {
.stop = logi_dj_ll_stop,
.open = logi_dj_ll_open,
.close = logi_dj_ll_close,
+ .hid_output_raw_report = logi_dj_output_hidraw_report,
};
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 8494b8c..9dd37ff 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -494,6 +494,8 @@ struct sony_sc {
unsigned long quirks;
struct work_struct state_worker;
+ struct hid_ll_driver *ll_driver;
+
#ifdef CONFIG_SONY_FF
__u8 left;
__u8 right;
@@ -1077,7 +1079,14 @@ 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;
+ sc->ll_driver = devm_kzalloc(&hdev->dev, sizeof(*sc->ll_driver),
+ GFP_KERNEL);
+ if (sc->ll_driver == NULL)
+ return -ENOMEM;
+ *sc->ll_driver = *hdev->ll_driver;
+ hdev->ll_driver = sc->ll_driver;
+ sc->ll_driver->hid_output_raw_report =
+ sixaxis_usb_output_raw_report;
ret = sixaxis_set_operational_usb(hdev);
INIT_WORK(&sc->state_worker, sixaxis_state_worker);
}
diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index d7dc6c5b..715a3ab 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -28,7 +28,7 @@ static int wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
__u8 *buf;
int ret;
- if (!hdev->hid_output_raw_report)
+ if (!hdev->ll_driver->hid_output_raw_report)
return -ENODEV;
buf = kmemdup(buffer, count, GFP_KERNEL);
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index f8708c9..c60c530 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -123,7 +123,7 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
dev = hidraw_table[minor]->hid;
- if (!dev->hid_output_raw_report) {
+ if (!dev->ll_driver->hid_output_raw_report) {
ret = -ENODEV;
goto out;
}
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index fe3b392..fabb388 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -785,6 +785,7 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
.request = i2c_hid_request,
.output_report = i2c_hid_output_report,
.raw_request = i2c_hid_raw_request,
+ .hid_output_raw_report = i2c_hid_output_raw_report,
};
static int i2c_hid_init_irq(struct i2c_client *client)
@@ -1029,7 +1030,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 7358346..8e99a5a 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -294,6 +294,7 @@ static struct hid_ll_driver uhid_hid_driver = {
.parse = uhid_hid_parse,
.output_report = uhid_hid_output_report,
.raw_request = uhid_raw_request,
+ .hid_output_raw_report = uhid_hid_output_raw,
};
#ifdef CONFIG_COMPAT
@@ -421,7 +422,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 b9a770f..9c3c244 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1260,6 +1260,7 @@ static struct hid_ll_driver usb_hid_driver = {
.raw_request = usbhid_raw_request,
.output_report = usbhid_output_report,
.idle = usbhid_idle,
+ .hid_output_raw_report = usbhid_output_raw_report,
};
static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *id)
@@ -1289,7 +1290,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 a837ede..eb588e9 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -508,9 +508,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;
@@ -678,6 +675,7 @@ struct hid_driver {
* @wait: wait for buffered io to complete (send/recv reports)
* @raw_request: send raw report request to device (e.g. feature report)
* @output_report: send output report to device
+ * @hid_output_raw_report: send report to device (e.g. feature report)
* @idle: send idle request to device
*/
struct hid_ll_driver {
@@ -702,6 +700,10 @@ struct hid_ll_driver {
int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len);
+ /* handler for raw output data, used by hidraw */
+ int (*hid_output_raw_report) (struct hid_device *, __u8 *, size_t,
+ unsigned char);
+
int (*idle)(struct hid_device *hdev, int report, int idle, int reqtype);
};
@@ -1024,7 +1026,8 @@ static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
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);
+ return hdev->ll_driver->hid_output_raw_report(hdev, buf, len,
+ report_type);
}
/**
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 77c4bad..6189b54 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -726,6 +726,8 @@ static struct hid_ll_driver hidp_hid_driver = {
.close = hidp_close,
.raw_request = hidp_raw_request,
.output_report = hidp_output_report,
+ .hid_output_raw_report = hidp_output_raw_report,
+
};
/* This function sets up the hid device. It does not add it
@@ -773,8 +775,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.3.1
dev->hid_get_raw_report(X) and hid_hw_raw_request(X, HID_REQ_GET_REPORT)
are strictly equivalent. Switch the hid subsystem to the hid_hw notation
and remove the field .hid_get_raw_report in struct hid_device.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-input.c | 6 +++---
drivers/hid/hid-sony.c | 3 ++-
drivers/hid/hidraw.c | 7 ++++---
drivers/hid/i2c-hid/i2c-hid.c | 1 -
drivers/hid/uhid.c | 1 -
drivers/hid/usbhid/hid-core.c | 1 -
include/linux/hid.h | 3 ---
net/bluetooth/hidp/core.c | 1 -
8 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 594722d..1de5997 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -350,9 +350,9 @@ static int hidinput_get_battery_property(struct power_supply *psy,
ret = -ENOMEM;
break;
}
- ret = dev->hid_get_raw_report(dev, dev->battery_report_id,
- buf, 2,
- dev->battery_report_type);
+ ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 2,
+ dev->battery_report_type,
+ HID_REQ_GET_REPORT);
if (ret != 2) {
ret = -ENODATA;
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 1235405..3930acb 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -706,7 +706,8 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
if (!buf)
return -ENOMEM;
- ret = hdev->hid_get_raw_report(hdev, 0xf2, buf, 17, HID_FEATURE_REPORT);
+ ret = hid_hw_raw_request(hdev, 0xf2, buf, 17, HID_FEATURE_REPORT,
+ HID_REQ_GET_REPORT);
if (ret < 0)
hid_err(hdev, "can't set operational mode\n");
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index cb0137b..4b2dc95 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -189,7 +189,7 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
dev = hidraw_table[minor]->hid;
- if (!dev->hid_get_raw_report) {
+ if (!dev->ll_driver->raw_request) {
ret = -ENODEV;
goto out;
}
@@ -216,14 +216,15 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
/*
* Read the first byte from the user. This is the report number,
- * which is passed to dev->hid_get_raw_report().
+ * which is passed to hid_hw_raw_request().
*/
if (copy_from_user(&report_number, buffer, 1)) {
ret = -EFAULT;
goto out_free;
}
- ret = dev->hid_get_raw_report(dev, report_number, buf, count, report_type);
+ ret = hid_hw_raw_request(dev, report_number, buf, count, report_type,
+ HID_REQ_GET_REPORT);
if (ret < 0)
goto out_free;
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 5099f1f..fe3b392 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -1029,7 +1029,6 @@ static int i2c_hid_probe(struct i2c_client *client,
hid->driver_data = client;
hid->ll_driver = &i2c_hid_ll_driver;
- hid->hid_get_raw_report = i2c_hid_get_raw_report;
hid->hid_output_raw_report = i2c_hid_output_raw_report;
hid->dev.parent = &client->dev;
ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 438c9f1..7358346 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -421,7 +421,6 @@ static int uhid_dev_create(struct uhid_device *uhid,
hid->uniq[63] = 0;
hid->ll_driver = &uhid_hid_driver;
- hid->hid_get_raw_report = uhid_hid_get_raw;
hid->hid_output_raw_report = uhid_hid_output_raw;
hid->bus = ev->u.create.bus;
hid->vendor = ev->u.create.vendor;
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 406497b..b9a770f 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1289,7 +1289,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_get_raw_report = usbhid_get_raw_report;
hid->hid_output_raw_report = usbhid_output_raw_report;
hid->ff_init = hid_pidff_init;
#ifdef CONFIG_USB_HIDDEV
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 38c307b..c56681a 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -508,9 +508,6 @@ struct hid_device { /* device report descriptor */
struct hid_usage *, __s32);
void (*hiddev_report_event) (struct hid_device *, struct hid_report *);
- /* handler for raw input (Get_Report) data, used by hidraw */
- int (*hid_get_raw_report) (struct hid_device *, unsigned char, __u8 *, size_t, unsigned char);
-
/* handler for raw output data, used by hidraw */
int (*hid_output_raw_report) (struct hid_device *, __u8 *, size_t, unsigned char);
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 02670b3..77c4bad 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -773,7 +773,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_get_raw_report = hidp_get_raw_report;
hid->hid_output_raw_report = hidp_output_raw_report;
/* True if device is blacklisted in drivers/hid/hid-core.c */
--
1.8.3.1
Well, no use to keep twice the same code.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/usbhid/hid-core.c | 64 ++++++++-----------------------------------
1 file changed, 11 insertions(+), 53 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index f8ca312..406497b 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -915,59 +915,6 @@ static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
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;
- struct usb_device *dev = hid_to_usb_dev(hid);
- struct usb_interface *intf = usbhid->intf;
- struct usb_host_interface *interface = intf->cur_altsetting;
- int ret;
-
- if (usbhid->urbout && report_type != HID_FEATURE_REPORT) {
- int actual_length;
- int skipped_report_id = 0;
-
- if (buf[0] == 0x0) {
- /* Don't send the Report ID */
- buf++;
- count--;
- skipped_report_id = 1;
- }
- ret = usb_interrupt_msg(dev, usbhid->urbout->pipe,
- buf, count, &actual_length,
- USB_CTRL_SET_TIMEOUT);
- /* return the number of bytes transferred */
- if (ret == 0) {
- ret = actual_length;
- /* count also the report id */
- if (skipped_report_id)
- ret++;
- }
- } else {
- int skipped_report_id = 0;
- int report_id = buf[0];
- if (buf[0] == 0x0) {
- /* Don't send the Report ID */
- buf++;
- count--;
- skipped_report_id = 1;
- }
- 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, if this was a numbered report. */
- if (ret > 0 && skipped_report_id)
- ret++;
- }
-
- return ret;
-}
-
static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
{
struct usbhid_device *usbhid = hid->driver_data;
@@ -998,6 +945,17 @@ 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))
--
1.8.3.1
All the different transport drivers use now the generic event handling
in hid-input. We can remove the handler definitively now.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-input.c | 4 +---
include/linux/hid.h | 4 ----
2 files changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index a713e62..594722d 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1263,9 +1263,7 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
}
input_set_drvdata(input_dev, hid);
- if (hid->ll_driver->hidinput_input_event)
- input_dev->event = hid->ll_driver->hidinput_input_event;
- else if (hid->ll_driver->request || hid->hid_output_raw_report)
+ if (hid->ll_driver->request || hid->hid_output_raw_report)
input_dev->event = hidinput_input_event;
input_dev->open = hidinput_open;
input_dev->close = hidinput_close;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index dddcad0..38c307b 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -675,7 +675,6 @@ struct hid_driver {
* @stop: called on remove
* @open: called by input layer on open
* @close: called by input layer on close
- * @hidinput_input_event: event input event (e.g. ff or leds)
* @parse: this method is called only once to parse the device data,
* shouldn't allocate anything to not leak memory
* @request: send report request to device (e.g. feature report)
@@ -693,9 +692,6 @@ struct hid_ll_driver {
int (*power)(struct hid_device *hdev, int level);
- int (*hidinput_input_event) (struct input_dev *idev, unsigned int type,
- unsigned int code, int value);
-
int (*parse)(struct hid_device *hdev);
void (*request)(struct hid_device *hdev,
--
1.8.3.1
hidp uses its own ->hidinput_input_event() instead of the generic binding
in hid-input.
Moving the handling of LEDs towards hidp_hidinput_event() allows two things:
- remove hidinput_input_event definitively from struct hid_device
- hidraw user space programs can also set the LEDs
Signed-off-by: Benjamin Tissoires <[email protected]>
---
net/bluetooth/hidp/core.c | 46 ----------------------------------------------
1 file changed, 46 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index b062cee..469e61b 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -223,51 +223,6 @@ static void hidp_input_report(struct hidp_session *session, struct sk_buff *skb)
input_sync(dev);
}
-static int hidp_send_report(struct hidp_session *session, struct hid_report *report)
-{
- unsigned char hdr;
- u8 *buf;
- int rsize, ret;
-
- buf = hid_alloc_report_buf(report, GFP_ATOMIC);
- if (!buf)
- return -EIO;
-
- hid_output_report(report, buf);
- hdr = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
-
- rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0);
- ret = hidp_send_intr_message(session, hdr, buf, rsize);
-
- kfree(buf);
- return ret;
-}
-
-static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
- unsigned int code, int value)
-{
- struct hid_device *hid = input_get_drvdata(dev);
- struct hidp_session *session = hid->driver_data;
- struct hid_field *field;
- int offset;
-
- BT_DBG("session %p type %d code %d value %d",
- session, type, code, value);
-
- if (type != EV_LED)
- return -1;
-
- offset = hidinput_find_field(hid, type, code, &field);
- if (offset == -1) {
- hid_warn(dev, "event field not found\n");
- return -1;
- }
-
- hid_set_field(field, offset, value);
-
- return hidp_send_report(session, field->report);
-}
-
static int hidp_get_raw_report(struct hid_device *hid,
unsigned char report_number,
unsigned char *data, size_t count,
@@ -817,7 +772,6 @@ static struct hid_ll_driver hidp_hid_driver = {
.close = hidp_close,
.raw_request = hidp_raw_request,
.output_report = hidp_output_report,
- .hidinput_input_event = hidp_hidinput_event,
};
/* This function sets up the hid device. It does not add it
--
1.8.3.1
Add output_report and raw_request to i2c-hid.
Hopefully, we will manage to have the same transport level between
all the transport drivers.
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/i2c-hid/i2c-hid.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index ce68a12..5099f1f 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -574,6 +574,28 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
return ret;
}
+static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
+ size_t count)
+{
+ return i2c_hid_output_raw_report(hid, buf, count, HID_OUTPUT_REPORT);
+}
+
+static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
+ __u8 *buf, size_t len, unsigned char rtype,
+ int reqtype)
+{
+ switch (reqtype) {
+ case HID_REQ_GET_REPORT:
+ return i2c_hid_get_raw_report(hid, reportnum, buf, len, rtype);
+ case HID_REQ_SET_REPORT:
+ if (buf[0] != reportnum)
+ return -EINVAL;
+ return i2c_hid_output_raw_report(hid, buf, len, rtype);
+ default:
+ return -EIO;
+ }
+}
+
static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
int reqtype)
{
@@ -761,6 +783,8 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
.close = i2c_hid_close,
.power = i2c_hid_power,
.request = i2c_hid_request,
+ .output_report = i2c_hid_output_report,
+ .raw_request = i2c_hid_raw_request,
};
static int i2c_hid_init_irq(struct i2c_client *client)
--
1.8.3.1
- Move hidp_output_report() above
- Removed duplicated code in hidp_output_raw_report()
Signed-off-by: Benjamin Tissoires <[email protected]>
---
net/bluetooth/hidp/core.c | 68 ++++++++---------------------------------------
1 file changed, 11 insertions(+), 57 deletions(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 469e61b..02670b3 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -373,62 +373,25 @@ err:
return ret;
}
-static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
- unsigned char report_type)
+static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
{
struct hidp_session *session = hid->driver_data;
- int ret;
+ return hidp_send_intr_message(session,
+ HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
+ 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) {
- report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
- return hidp_send_intr_message(session, report_type,
- data, count);
+ return hidp_output_report(hid, data, count);
} else if (report_type != HID_FEATURE_REPORT) {
return -EINVAL;
}
- if (mutex_lock_interruptible(&session->report_mutex))
- return -ERESTARTSYS;
-
- /* Set up our wait, and send the report request to the device. */
- set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
- report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
- ret = hidp_send_ctrl_message(session, report_type, data, count);
- if (ret)
- goto err;
-
- /* Wait for the ACK from the device. */
- while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags) &&
- !atomic_read(&session->terminate)) {
- int res;
-
- res = wait_event_interruptible_timeout(session->report_queue,
- !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)
- || atomic_read(&session->terminate),
- 10*HZ);
- if (res == 0) {
- /* timeout */
- ret = -EIO;
- goto err;
- }
- if (res < 0) {
- /* signal */
- ret = -ERESTARTSYS;
- goto err;
- }
- }
-
- if (!session->output_report_success) {
- ret = -EIO;
- goto err;
- }
-
- ret = count;
-
-err:
- clear_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
- mutex_unlock(&session->report_mutex);
- return ret;
+ return hidp_set_raw_report(hid, data[0], data, count, report_type);
}
static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
@@ -445,15 +408,6 @@ static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
}
}
-static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
-{
- struct hidp_session *session = hid->driver_data;
-
- return hidp_send_intr_message(session,
- HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
- data, count);
-}
-
static void hidp_idle_timeout(unsigned long arg)
{
struct hidp_session *session = (struct hidp_session *) arg;
--
1.8.3.1
hid-logitech-dj uses its own ->hidinput_input_event() instead of
the generic binding in hid-input.
Moving the handling of LEDs towards logi_dj_output_hidraw_report()
allows two things:
- remove hidinput_input_event in struct hid_device
- hidraw user space programs can also set the LEDs
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/hid/hid-logitech-dj.c | 99 +++++++++++++++----------------------------
1 file changed, 35 insertions(+), 64 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index f45279c..61d2bbf 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -44,14 +44,6 @@ static const char kbd_descriptor[] = {
0x19, 0xE0, /* USAGE_MINIMUM (Left Control) */
0x29, 0xE7, /* USAGE_MAXIMUM (Right GUI) */
0x81, 0x02, /* INPUT (Data,Var,Abs) */
- 0x95, 0x05, /* REPORT COUNT (5) */
- 0x05, 0x08, /* USAGE PAGE (LED page) */
- 0x19, 0x01, /* USAGE MINIMUM (1) */
- 0x29, 0x05, /* USAGE MAXIMUM (5) */
- 0x91, 0x02, /* OUTPUT (Data, Variable, Absolute) */
- 0x95, 0x01, /* REPORT COUNT (1) */
- 0x75, 0x03, /* REPORT SIZE (3) */
- 0x91, 0x01, /* OUTPUT (Constant) */
0x95, 0x06, /* REPORT_COUNT (6) */
0x75, 0x08, /* REPORT_SIZE (8) */
0x15, 0x00, /* LOGICAL_MINIMUM (0) */
@@ -60,6 +52,18 @@ static const char kbd_descriptor[] = {
0x19, 0x00, /* USAGE_MINIMUM (no event) */
0x2A, 0xFF, 0x00, /* USAGE_MAXIMUM (reserved) */
0x81, 0x00, /* INPUT (Data,Ary,Abs) */
+ 0x85, 0x0e, /* REPORT_ID (14) */
+ 0x05, 0x08, /* USAGE PAGE (LED page) */
+ 0x95, 0x05, /* REPORT COUNT (5) */
+ 0x75, 0x01, /* REPORT SIZE (1) */
+ 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
+ 0x25, 0x01, /* LOGICAL_MAXIMUM (1) */
+ 0x19, 0x01, /* USAGE MINIMUM (1) */
+ 0x29, 0x05, /* USAGE MAXIMUM (5) */
+ 0x91, 0x02, /* OUTPUT (Data, Variable, Absolute) */
+ 0x95, 0x01, /* REPORT COUNT (1) */
+ 0x75, 0x03, /* REPORT SIZE (3) */
+ 0x91, 0x01, /* OUTPUT (Constant) */
0xC0
};
@@ -544,10 +548,30 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
size_t count,
unsigned char report_type)
{
- /* Called by hid raw to send data */
- dbg_hid("%s\n", __func__);
+ struct dj_device *djdev = hid->driver_data;
+ struct dj_receiver_dev *djrcv_dev = djdev->dj_receiver_dev;
+ u8 *out_buf;
+ int ret;
- return 0;
+ if (buf[0] != REPORT_TYPE_LEDS)
+ return -EINVAL;
+
+ out_buf = kzalloc(DJREPORT_SHORT_LENGTH, GFP_ATOMIC);
+ if (!out_buf)
+ return -ENOMEM;
+
+ if (count < DJREPORT_SHORT_LENGTH - 2)
+ count = DJREPORT_SHORT_LENGTH - 2;
+
+ out_buf[0] = REPORT_ID_DJ_SHORT;
+ out_buf[1] = djdev->device_index;
+ memcpy(out_buf + 2, buf, count);
+
+ ret = djrcv_dev->hdev->hid_output_raw_report(djrcv_dev->hdev, out_buf,
+ DJREPORT_SHORT_LENGTH, report_type);
+
+ kfree(out_buf);
+ return ret;
}
static void rdcat(char *rdesc, unsigned int *rsize, const char *data, unsigned int size)
@@ -613,58 +637,6 @@ static int logi_dj_ll_parse(struct hid_device *hid)
return retval;
}
-static int logi_dj_ll_input_event(struct input_dev *dev, unsigned int type,
- unsigned int code, int value)
-{
- /* Sent by the input layer to handle leds and Force Feedback */
- struct hid_device *dj_hiddev = input_get_drvdata(dev);
- struct dj_device *dj_dev = dj_hiddev->driver_data;
-
- struct dj_receiver_dev *djrcv_dev =
- dev_get_drvdata(dj_hiddev->dev.parent);
- struct hid_device *dj_rcv_hiddev = djrcv_dev->hdev;
- struct hid_report_enum *output_report_enum;
-
- struct hid_field *field;
- struct hid_report *report;
- unsigned char *data;
- int offset;
-
- dbg_hid("%s: %s, type:%d | code:%d | value:%d\n",
- __func__, dev->phys, type, code, value);
-
- if (type != EV_LED)
- return -1;
-
- offset = hidinput_find_field(dj_hiddev, type, code, &field);
-
- if (offset == -1) {
- dev_warn(&dev->dev, "event field not found\n");
- return -1;
- }
- hid_set_field(field, offset, value);
-
- data = hid_alloc_report_buf(field->report, GFP_ATOMIC);
- if (!data) {
- dev_warn(&dev->dev, "failed to allocate report buf memory\n");
- return -1;
- }
-
- hid_output_report(field->report, &data[0]);
-
- output_report_enum = &dj_rcv_hiddev->report_enum[HID_OUTPUT_REPORT];
- report = output_report_enum->report_id_hash[REPORT_ID_DJ_SHORT];
- hid_set_field(report->field[0], 0, dj_dev->device_index);
- hid_set_field(report->field[0], 1, REPORT_TYPE_LEDS);
- hid_set_field(report->field[0], 2, data[1]);
-
- hid_hw_request(dj_rcv_hiddev, report, HID_REQ_SET_REPORT);
-
- kfree(data);
-
- return 0;
-}
-
static int logi_dj_ll_start(struct hid_device *hid)
{
dbg_hid("%s\n", __func__);
@@ -683,7 +655,6 @@ static struct hid_ll_driver logi_dj_ll_driver = {
.stop = logi_dj_ll_stop,
.open = logi_dj_ll_open,
.close = logi_dj_ll_close,
- .hidinput_input_event = logi_dj_ll_input_event,
};
--
1.8.3.1
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<[email protected]> wrote:
> - Move hidp_output_report() above
> - Removed duplicated code in hidp_output_raw_report()
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> net/bluetooth/hidp/core.c | 68 ++++++++---------------------------------------
> 1 file changed, 11 insertions(+), 57 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 469e61b..02670b3 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -373,62 +373,25 @@ err:
> return ret;
> }
>
> -static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
> - unsigned char report_type)
> +static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
> {
> struct hidp_session *session = hid->driver_data;
> - int ret;
>
> + return hidp_send_intr_message(session,
> + HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
> + 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) {
> - report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
> - return hidp_send_intr_message(session, report_type,
> - data, count);
> + return hidp_output_report(hid, data, count);
NAK, that does not work with BT. hidp_output_raw_report() can do both,
a synchronous SET_REPORT and an async OUTPUT_REPORT. It's currently
screwed up, as for RTYPE_OUTPUT it uses the intr-channel, for
RTYPE_FEATURE the ctrl-channel.
This function is exactly the reason why we added raw_report() and
output_report(). So I'd like to keep it as it and just remove it once
we have no more users of hidp_output_raw_report().
Thanks
David
> } else if (report_type != HID_FEATURE_REPORT) {
> return -EINVAL;
> }
>
> - if (mutex_lock_interruptible(&session->report_mutex))
> - return -ERESTARTSYS;
> -
> - /* Set up our wait, and send the report request to the device. */
> - set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
> - report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
> - ret = hidp_send_ctrl_message(session, report_type, data, count);
> - if (ret)
> - goto err;
> -
> - /* Wait for the ACK from the device. */
> - while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags) &&
> - !atomic_read(&session->terminate)) {
> - int res;
> -
> - res = wait_event_interruptible_timeout(session->report_queue,
> - !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)
> - || atomic_read(&session->terminate),
> - 10*HZ);
> - if (res == 0) {
> - /* timeout */
> - ret = -EIO;
> - goto err;
> - }
> - if (res < 0) {
> - /* signal */
> - ret = -ERESTARTSYS;
> - goto err;
> - }
> - }
> -
> - if (!session->output_report_success) {
> - ret = -EIO;
> - goto err;
> - }
> -
> - ret = count;
> -
> -err:
> - clear_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
> - mutex_unlock(&session->report_mutex);
> - return ret;
> + return hidp_set_raw_report(hid, data[0], data, count, report_type);
> }
>
> static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
> @@ -445,15 +408,6 @@ static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
> }
> }
>
> -static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
> -{
> - struct hidp_session *session = hid->driver_data;
> -
> - return hidp_send_intr_message(session,
> - HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
> - data, count);
> -}
> -
> static void hidp_idle_timeout(unsigned long arg)
> {
> struct hidp_session *session = (struct hidp_session *) arg;
> --
> 1.8.3.1
>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<[email protected]> wrote:
> hidp uses its own ->hidinput_input_event() instead of the generic binding
> in hid-input.
> Moving the handling of LEDs towards hidp_hidinput_event() allows two things:
> - remove hidinput_input_event definitively from struct hid_device
> - hidraw user space programs can also set the LEDs
This one looks good. hid-input uses output_raw_report(), which is on
INTR for BT-HID, which is equivalent to what hidp_send_report() does.
So:
Reviewed-by: David Herrmann <[email protected]>
Btw., you might have to keep hidp_send_report() if you drop earlier
patches (haven't exactly looked at the order), but feel free to keep
my r-b anyway.
Thanks
David
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> net/bluetooth/hidp/core.c | 46 ----------------------------------------------
> 1 file changed, 46 deletions(-)
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index b062cee..469e61b 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -223,51 +223,6 @@ static void hidp_input_report(struct hidp_session *session, struct sk_buff *skb)
> input_sync(dev);
> }
>
> -static int hidp_send_report(struct hidp_session *session, struct hid_report *report)
> -{
> - unsigned char hdr;
> - u8 *buf;
> - int rsize, ret;
> -
> - buf = hid_alloc_report_buf(report, GFP_ATOMIC);
> - if (!buf)
> - return -EIO;
> -
> - hid_output_report(report, buf);
> - hdr = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
> -
> - rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0);
> - ret = hidp_send_intr_message(session, hdr, buf, rsize);
> -
> - kfree(buf);
> - return ret;
> -}
> -
> -static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
> - unsigned int code, int value)
> -{
> - struct hid_device *hid = input_get_drvdata(dev);
> - struct hidp_session *session = hid->driver_data;
> - struct hid_field *field;
> - int offset;
> -
> - BT_DBG("session %p type %d code %d value %d",
> - session, type, code, value);
> -
> - if (type != EV_LED)
> - return -1;
> -
> - offset = hidinput_find_field(hid, type, code, &field);
> - if (offset == -1) {
> - hid_warn(dev, "event field not found\n");
> - return -1;
> - }
> -
> - hid_set_field(field, offset, value);
> -
> - return hidp_send_report(session, field->report);
> -}
> -
> static int hidp_get_raw_report(struct hid_device *hid,
> unsigned char report_number,
> unsigned char *data, size_t count,
> @@ -817,7 +772,6 @@ static struct hid_ll_driver hidp_hid_driver = {
> .close = hidp_close,
> .raw_request = hidp_raw_request,
> .output_report = hidp_output_report,
> - .hidinput_input_event = hidp_hidinput_event,
> };
>
> /* This function sets up the hid device. It does not add it
> --
> 1.8.3.1
>
On Mon, Feb 3, 2014 at 10:02 AM, David Herrmann <[email protected]> wrote:
> Hi
>
> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
> <[email protected]> wrote:
>> - Move hidp_output_report() above
>> - Removed duplicated code in hidp_output_raw_report()
>>
>> Signed-off-by: Benjamin Tissoires <[email protected]>
>> ---
>> net/bluetooth/hidp/core.c | 68 ++++++++---------------------------------------
>> 1 file changed, 11 insertions(+), 57 deletions(-)
>>
>> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
>> index 469e61b..02670b3 100644
>> --- a/net/bluetooth/hidp/core.c
>> +++ b/net/bluetooth/hidp/core.c
>> @@ -373,62 +373,25 @@ err:
>> return ret;
>> }
>>
>> -static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
>> - unsigned char report_type)
>> +static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
>> {
>> struct hidp_session *session = hid->driver_data;
>> - int ret;
>>
>> + return hidp_send_intr_message(session,
>> + HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
>> + 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) {
>> - report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
>> - return hidp_send_intr_message(session, report_type,
>> - data, count);
>> + return hidp_output_report(hid, data, count);
>
> NAK, that does not work with BT. hidp_output_raw_report() can do both,
> a synchronous SET_REPORT and an async OUTPUT_REPORT. It's currently
> screwed up, as for RTYPE_OUTPUT it uses the intr-channel, for
> RTYPE_FEATURE the ctrl-channel.
David, please, re-review it carefully, and see that there is _no_
change in the code path with this refactoring:
hidp_output_report() calls exactly one hidp_send_intr_message() with
the same arguments.
And for the rest, you will see that the code path is still the same also.
>
> This function is exactly the reason why we added raw_report() and
> output_report(). So I'd like to keep it as it and just remove it once
> we have no more users of hidp_output_raw_report().
I worked on that yesterday, so I can make this happening soon (not
today though).
Cheers,
Benjamin
>
> Thanks
> David
>
>> } else if (report_type != HID_FEATURE_REPORT) {
>> return -EINVAL;
>> }
>>
>> - if (mutex_lock_interruptible(&session->report_mutex))
>> - return -ERESTARTSYS;
>> -
>> - /* Set up our wait, and send the report request to the device. */
>> - set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
>> - report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
>> - ret = hidp_send_ctrl_message(session, report_type, data, count);
>> - if (ret)
>> - goto err;
>> -
>> - /* Wait for the ACK from the device. */
>> - while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags) &&
>> - !atomic_read(&session->terminate)) {
>> - int res;
>> -
>> - res = wait_event_interruptible_timeout(session->report_queue,
>> - !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)
>> - || atomic_read(&session->terminate),
>> - 10*HZ);
>> - if (res == 0) {
>> - /* timeout */
>> - ret = -EIO;
>> - goto err;
>> - }
>> - if (res < 0) {
>> - /* signal */
>> - ret = -ERESTARTSYS;
>> - goto err;
>> - }
>> - }
>> -
>> - if (!session->output_report_success) {
>> - ret = -EIO;
>> - goto err;
>> - }
>> -
>> - ret = count;
>> -
>> -err:
>> - clear_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
>> - mutex_unlock(&session->report_mutex);
>> - return ret;
>> + return hidp_set_raw_report(hid, data[0], data, count, report_type);
>> }
>>
>> static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
>> @@ -445,15 +408,6 @@ static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
>> }
>> }
>>
>> -static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
>> -{
>> - struct hidp_session *session = hid->driver_data;
>> -
>> - return hidp_send_intr_message(session,
>> - HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
>> - data, count);
>> -}
>> -
>> static void hidp_idle_timeout(unsigned long arg)
>> {
>> struct hidp_session *session = (struct hidp_session *) arg;
>> --
>> 1.8.3.1
>>
Hi
On Mon, Feb 3, 2014 at 4:11 PM, Benjamin Tissoires
<[email protected]> wrote:
> On Mon, Feb 3, 2014 at 10:02 AM, David Herrmann <[email protected]> wrote:
>> Hi
>>
>> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
>> <[email protected]> wrote:
>>> - Move hidp_output_report() above
>>> - Removed duplicated code in hidp_output_raw_report()
>>>
>>> Signed-off-by: Benjamin Tissoires <[email protected]>
>>> ---
>>> net/bluetooth/hidp/core.c | 68 ++++++++---------------------------------------
>>> 1 file changed, 11 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
>>> index 469e61b..02670b3 100644
>>> --- a/net/bluetooth/hidp/core.c
>>> +++ b/net/bluetooth/hidp/core.c
>>> @@ -373,62 +373,25 @@ err:
>>> return ret;
>>> }
>>>
>>> -static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data, size_t count,
>>> - unsigned char report_type)
>>> +static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
>>> {
>>> struct hidp_session *session = hid->driver_data;
>>> - int ret;
>>>
>>> + return hidp_send_intr_message(session,
>>> + HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
>>> + 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) {
>>> - report_type = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
>>> - return hidp_send_intr_message(session, report_type,
>>> - data, count);
>>> + return hidp_output_report(hid, data, count);
>>
>> NAK, that does not work with BT. hidp_output_raw_report() can do both,
>> a synchronous SET_REPORT and an async OUTPUT_REPORT. It's currently
>> screwed up, as for RTYPE_OUTPUT it uses the intr-channel, for
>> RTYPE_FEATURE the ctrl-channel.
>
> David, please, re-review it carefully, and see that there is _no_
> change in the code path with this refactoring:
> hidp_output_report() calls exactly one hidp_send_intr_message() with
> the same arguments.
>
> And for the rest, you will see that the code path is still the same also.
Sorry, my bad. I thought you changed hidp_output_raw_report() to call
hidp_output_report() unconditionally. You're indeed right, the
code-paths are the same and they now highlight the very horrible hack
hid_output_raw_report() has been in the past for HIDP.
Reviewed-by: David Herrmann <[email protected]>
Thanks
David
>>
>> This function is exactly the reason why we added raw_report() and
>> output_report(). So I'd like to keep it as it and just remove it once
>> we have no more users of hidp_output_raw_report().
>
> I worked on that yesterday, so I can make this happening soon (not
> today though).
>
> Cheers,
> Benjamin
>
>>
>> Thanks
>> David
>>
>>> } else if (report_type != HID_FEATURE_REPORT) {
>>> return -EINVAL;
>>> }
>>>
>>> - if (mutex_lock_interruptible(&session->report_mutex))
>>> - return -ERESTARTSYS;
>>> -
>>> - /* Set up our wait, and send the report request to the device. */
>>> - set_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
>>> - report_type = HIDP_TRANS_SET_REPORT | HIDP_DATA_RTYPE_FEATURE;
>>> - ret = hidp_send_ctrl_message(session, report_type, data, count);
>>> - if (ret)
>>> - goto err;
>>> -
>>> - /* Wait for the ACK from the device. */
>>> - while (test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags) &&
>>> - !atomic_read(&session->terminate)) {
>>> - int res;
>>> -
>>> - res = wait_event_interruptible_timeout(session->report_queue,
>>> - !test_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags)
>>> - || atomic_read(&session->terminate),
>>> - 10*HZ);
>>> - if (res == 0) {
>>> - /* timeout */
>>> - ret = -EIO;
>>> - goto err;
>>> - }
>>> - if (res < 0) {
>>> - /* signal */
>>> - ret = -ERESTARTSYS;
>>> - goto err;
>>> - }
>>> - }
>>> -
>>> - if (!session->output_report_success) {
>>> - ret = -EIO;
>>> - goto err;
>>> - }
>>> -
>>> - ret = count;
>>> -
>>> -err:
>>> - clear_bit(HIDP_WAITING_FOR_SEND_ACK, &session->flags);
>>> - mutex_unlock(&session->report_mutex);
>>> - return ret;
>>> + return hidp_set_raw_report(hid, data[0], data, count, report_type);
>>> }
>>>
>>> static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
>>> @@ -445,15 +408,6 @@ static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
>>> }
>>> }
>>>
>>> -static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
>>> -{
>>> - struct hidp_session *session = hid->driver_data;
>>> -
>>> - return hidp_send_intr_message(session,
>>> - HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT,
>>> - data, count);
>>> -}
>>> -
>>> static void hidp_idle_timeout(unsigned long arg)
>>> {
>>> struct hidp_session *session = (struct hidp_session *) arg;
>>> --
>>> 1.8.3.1
>>>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<[email protected]> wrote:
> It was missing, so adding it.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/uhid.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index f5a2b19..438c9f1 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -270,6 +270,22 @@ static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
> return count;
> }
>
> +static int uhid_raw_request(struct hid_device *hid, unsigned char reportnum,
> + __u8 *buf, size_t len, unsigned char rtype,
> + int reqtype)
> +{
> + switch (reqtype) {
> + case HID_REQ_GET_REPORT:
> + return uhid_hid_get_raw(hid, reportnum, buf, len, rtype);
> + case HID_REQ_SET_REPORT:
> + if (buf[0] != reportnum)
> + return -EINVAL;
> + return uhid_hid_output_raw(hid, buf, len, rtype);
But that one looks wrong. UHID does not have any SET_REPORT query in
the protocol, yet. You turn a SET_REPORT into an OUTPUT_REPORT here.
So if user-space gets the UHID_OUTPUT event, it doesn't know whether
to send a SET_REPORT@ctrl to the device, or an async
OUTPUT_REPORT@intr. This at least matters for low-energy BT in bluez,
which uses uhid.
So we'd have to add an UHID_SET_REPORT event. Note that currently
UHID_FEATURE is the equivalent of UHID_GET_REPORT, but just horribly
named..
Thanks
David
> + default:
> + return -EIO;
> + }
> +}
> +
> static struct hid_ll_driver uhid_hid_driver = {
> .start = uhid_hid_start,
> .stop = uhid_hid_stop,
> @@ -277,6 +293,7 @@ static struct hid_ll_driver uhid_hid_driver = {
> .close = uhid_hid_close,
> .parse = uhid_hid_parse,
> .output_report = uhid_hid_output_report,
> + .raw_request = uhid_raw_request,
> };
>
> #ifdef CONFIG_COMPAT
> --
> 1.8.3.1
>
On Mon, Feb 3, 2014 at 10:10 AM, David Herrmann <[email protected]> wrote:
> Hi
>
> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
> <[email protected]> wrote:
>> hidp uses its own ->hidinput_input_event() instead of the generic binding
>> in hid-input.
>> Moving the handling of LEDs towards hidp_hidinput_event() allows two things:
>> - remove hidinput_input_event definitively from struct hid_device
>> - hidraw user space programs can also set the LEDs
>
> This one looks good. hid-input uses output_raw_report(), which is on
> INTR for BT-HID, which is equivalent to what hidp_send_report() does.
> So:
>
> Reviewed-by: David Herrmann <[email protected]>
Thanks
>
> Btw., you might have to keep hidp_send_report() if you drop earlier
> patches (haven't exactly looked at the order), but feel free to keep
> my r-b anyway.
No need to keep it, patches 1 to 4 do not touch HIDp, so no one else
use hidp_send_report().
Cheers,
Benjamin
>
> Thanks
> David
>
>>
>> Signed-off-by: Benjamin Tissoires <[email protected]>
>> ---
>> net/bluetooth/hidp/core.c | 46 ----------------------------------------------
>> 1 file changed, 46 deletions(-)
>>
>> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
>> index b062cee..469e61b 100644
>> --- a/net/bluetooth/hidp/core.c
>> +++ b/net/bluetooth/hidp/core.c
>> @@ -223,51 +223,6 @@ static void hidp_input_report(struct hidp_session *session, struct sk_buff *skb)
>> input_sync(dev);
>> }
>>
>> -static int hidp_send_report(struct hidp_session *session, struct hid_report *report)
>> -{
>> - unsigned char hdr;
>> - u8 *buf;
>> - int rsize, ret;
>> -
>> - buf = hid_alloc_report_buf(report, GFP_ATOMIC);
>> - if (!buf)
>> - return -EIO;
>> -
>> - hid_output_report(report, buf);
>> - hdr = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
>> -
>> - rsize = ((report->size - 1) >> 3) + 1 + (report->id > 0);
>> - ret = hidp_send_intr_message(session, hdr, buf, rsize);
>> -
>> - kfree(buf);
>> - return ret;
>> -}
>> -
>> -static int hidp_hidinput_event(struct input_dev *dev, unsigned int type,
>> - unsigned int code, int value)
>> -{
>> - struct hid_device *hid = input_get_drvdata(dev);
>> - struct hidp_session *session = hid->driver_data;
>> - struct hid_field *field;
>> - int offset;
>> -
>> - BT_DBG("session %p type %d code %d value %d",
>> - session, type, code, value);
>> -
>> - if (type != EV_LED)
>> - return -1;
>> -
>> - offset = hidinput_find_field(hid, type, code, &field);
>> - if (offset == -1) {
>> - hid_warn(dev, "event field not found\n");
>> - return -1;
>> - }
>> -
>> - hid_set_field(field, offset, value);
>> -
>> - return hidp_send_report(session, field->report);
>> -}
>> -
>> static int hidp_get_raw_report(struct hid_device *hid,
>> unsigned char report_number,
>> unsigned char *data, size_t count,
>> @@ -817,7 +772,6 @@ static struct hid_ll_driver hidp_hid_driver = {
>> .close = hidp_close,
>> .raw_request = hidp_raw_request,
>> .output_report = hidp_output_report,
>> - .hidinput_input_event = hidp_hidinput_event,
>> };
>>
>> /* This function sets up the hid device. It does not add it
>> --
>> 1.8.3.1
>>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<[email protected]> wrote:
> Those callbacks are not mandatory, so it's better to add inliners
> to use them safely.
Reviewed-by: David Herrmann <[email protected]>
Thanks
David
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> include/linux/hid.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 003cc8e..dddcad0 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -680,6 +680,8 @@ struct hid_driver {
> * shouldn't allocate anything to not leak memory
> * @request: send report request to device (e.g. feature report)
> * @wait: wait for buffered io to complete (send/recv reports)
> + * @raw_request: send raw report request to device (e.g. feature report)
> + * @output_report: send output report to device
> * @idle: send idle request to device
> */
> struct hid_ll_driver {
> @@ -974,6 +976,49 @@ static inline void hid_hw_request(struct hid_device *hdev,
> }
>
> /**
> + * hid_hw_raw_request - send report request to device
> + *
> + * @hdev: hid device
> + * @reportnum: report ID
> + * @buf: in/out data to transfer
> + * @len: length of buf
> + * @rtype: HID report type
> + * @reqtype: HID_REQ_GET_REPORT or HID_REQ_SET_REPORT
> + *
> + * @return: count of data transfered, negative if error
> + *
> + * Same behavior as hid_hw_request, but with raw buffers instead.
> + */
> +static inline int hid_hw_raw_request(struct hid_device *hdev,
> + unsigned char reportnum, __u8 *buf,
> + size_t len, unsigned char rtype, int reqtype)
> +{
> + if (hdev->ll_driver->raw_request)
> + return hdev->ll_driver->raw_request(hdev, reportnum, buf, len,
> + rtype, reqtype);
> +
> + return -ENOSYS;
> +}
> +
> +/**
> + * hid_hw_output_report - send output report to device
> + *
> + * @hdev: hid device
> + * @buf: raw data to transfer
> + * @len: length of buf
> + *
> + * @return: count of data transfered, negative if error
> + */
> +static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
> + size_t len)
> +{
> + if (hdev->ll_driver->output_report)
> + return hdev->ll_driver->output_report(hdev, buf, len);
> +
> + return -ENOSYS;
> +}
> +
> +/**
> * hid_hw_idle - send idle request to device
> *
> * @hdev: hid device
> --
> 1.8.3.1
>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<[email protected]> wrote:
> Add output_report and raw_request to i2c-hid.
> Hopefully, we will manage to have the same transport level between
> all the transport drivers.
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/i2c-hid/i2c-hid.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index ce68a12..5099f1f 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -574,6 +574,28 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
> return ret;
> }
>
> +static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
> + size_t count)
> +{
> + return i2c_hid_output_raw_report(hid, buf, count, HID_OUTPUT_REPORT);
> +}
> +
> +static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
> + __u8 *buf, size_t len, unsigned char rtype,
> + int reqtype)
> +{
> + switch (reqtype) {
> + case HID_REQ_GET_REPORT:
> + return i2c_hid_get_raw_report(hid, reportnum, buf, len, rtype);
> + case HID_REQ_SET_REPORT:
> + if (buf[0] != reportnum)
> + return -EINVAL;
> + return i2c_hid_output_raw_report(hid, buf, len, rtype);
I just skimmed the I2C-HID specs and it defines three methods for
input/output reports:
1) Section 6.2:
raw async output-reports can be sent by writing the data at any time
to wOutputRegister.
This should be used as method for hid->output_report().
2) Section 7.1:
SET_REPORT can be issued by writing the right OPCODE + report-ID into
wCommandRegister and the data into wDataRegister.
This should be used as method for hid->raw_request() + HID_REQ_SET_REPORT.
3) Section 7.1:
GET_REPORT can be issued by writing the right OPCODE + report-ID into
wCommandRegister and then waiting for the device to write the data
into wDataRegister.
This should be used for hid->raw_request() + HID_REQ_GET_REPORT
The GET_REPORT implementation looks fine to me, but the
i2c_hid_set_report() seems to support both 1) and 2) depending on the
passed type and capabilities:
- it uses 2) for FEATURE_REPORT reports or if the max OUTPUT_LENGTH is 0
- it uses 1) otherwise
I'm not sure whether the i2c-hid-spec mandates this behavior, so I am
not saying it's wrong. I just wanna understand what we do here. So if
we use hid->output_report() with HID_FEATURE_REPORT, the current code
turns this into a SET_REPORT. Likewise, an hid->raw_request() with
HID_REQ_SET_REPORT but with HID_OUTPUT_REPORT turns into an
output-report.
I'd rather expect this behavior:
hid->output_report() should always do this:
args[index++] = outputRegister & 0xFF;
args[index++] = outputRegister >> 8;
hidcmd = &hid_no_cmd;
while hid->raw_request() should always do this:
args[index++] = dataRegister & 0xFF;
args[index++] = dataRegister >> 8;
The special case for maxOutputLength==0 seems fine to me, but the
"reportType == 0x03" looks weird.
Thanks
David
> + default:
> + return -EIO;
> + }
> +}
> +
> static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
> int reqtype)
> {
> @@ -761,6 +783,8 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
> .close = i2c_hid_close,
> .power = i2c_hid_power,
> .request = i2c_hid_request,
> + .output_report = i2c_hid_output_report,
> + .raw_request = i2c_hid_raw_request,
> };
>
> static int i2c_hid_init_irq(struct i2c_client *client)
> --
> 1.8.3.1
>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<[email protected]> wrote:
> hid-logitech-dj uses its own ->hidinput_input_event() instead of
> the generic binding in hid-input.
> Moving the handling of LEDs towards logi_dj_output_hidraw_report()
> allows two things:
> - remove hidinput_input_event in struct hid_device
> - hidraw user space programs can also set the LEDs
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/hid-logitech-dj.c | 99 +++++++++++++++----------------------------
> 1 file changed, 35 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index f45279c..61d2bbf 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -44,14 +44,6 @@ static const char kbd_descriptor[] = {
> 0x19, 0xE0, /* USAGE_MINIMUM (Left Control) */
> 0x29, 0xE7, /* USAGE_MAXIMUM (Right GUI) */
> 0x81, 0x02, /* INPUT (Data,Var,Abs) */
> - 0x95, 0x05, /* REPORT COUNT (5) */
> - 0x05, 0x08, /* USAGE PAGE (LED page) */
> - 0x19, 0x01, /* USAGE MINIMUM (1) */
> - 0x29, 0x05, /* USAGE MAXIMUM (5) */
> - 0x91, 0x02, /* OUTPUT (Data, Variable, Absolute) */
> - 0x95, 0x01, /* REPORT COUNT (1) */
> - 0x75, 0x03, /* REPORT SIZE (3) */
> - 0x91, 0x01, /* OUTPUT (Constant) */
> 0x95, 0x06, /* REPORT_COUNT (6) */
> 0x75, 0x08, /* REPORT_SIZE (8) */
> 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
> @@ -60,6 +52,18 @@ static const char kbd_descriptor[] = {
> 0x19, 0x00, /* USAGE_MINIMUM (no event) */
> 0x2A, 0xFF, 0x00, /* USAGE_MAXIMUM (reserved) */
> 0x81, 0x00, /* INPUT (Data,Ary,Abs) */
> + 0x85, 0x0e, /* REPORT_ID (14) */
> + 0x05, 0x08, /* USAGE PAGE (LED page) */
> + 0x95, 0x05, /* REPORT COUNT (5) */
> + 0x75, 0x01, /* REPORT SIZE (1) */
> + 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
> + 0x25, 0x01, /* LOGICAL_MAXIMUM (1) */
> + 0x19, 0x01, /* USAGE MINIMUM (1) */
> + 0x29, 0x05, /* USAGE MAXIMUM (5) */
> + 0x91, 0x02, /* OUTPUT (Data, Variable, Absolute) */
> + 0x95, 0x01, /* REPORT COUNT (1) */
> + 0x75, 0x03, /* REPORT SIZE (3) */
> + 0x91, 0x01, /* OUTPUT (Constant) */
> 0xC0
> };
>
> @@ -544,10 +548,30 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
> size_t count,
> unsigned char report_type)
> {
> - /* Called by hid raw to send data */
> - dbg_hid("%s\n", __func__);
> + struct dj_device *djdev = hid->driver_data;
> + struct dj_receiver_dev *djrcv_dev = djdev->dj_receiver_dev;
> + u8 *out_buf;
> + int ret;
>
> - return 0;
> + if (buf[0] != REPORT_TYPE_LEDS)
> + return -EINVAL;
> +
> + out_buf = kzalloc(DJREPORT_SHORT_LENGTH, GFP_ATOMIC);
> + if (!out_buf)
> + return -ENOMEM;
> +
> + if (count < DJREPORT_SHORT_LENGTH - 2)
> + count = DJREPORT_SHORT_LENGTH - 2;
> +
> + out_buf[0] = REPORT_ID_DJ_SHORT;
> + out_buf[1] = djdev->device_index;
> + memcpy(out_buf + 2, buf, count);
> +
> + ret = djrcv_dev->hdev->hid_output_raw_report(djrcv_dev->hdev, out_buf,
> + DJREPORT_SHORT_LENGTH, report_type);
Strictly speaking the code below uses HID_REQ_SET_REPORT and you
replace it with hid_output_raw_report() here (which at least for BTHID
is not equivalent). Anyhow, HID-core uses hid_output_raw_report(),
too, so this should be fine.
Thanks
David
> +
> + kfree(out_buf);
> + return ret;
> }
>
> static void rdcat(char *rdesc, unsigned int *rsize, const char *data, unsigned int size)
> @@ -613,58 +637,6 @@ static int logi_dj_ll_parse(struct hid_device *hid)
> return retval;
> }
>
> -static int logi_dj_ll_input_event(struct input_dev *dev, unsigned int type,
> - unsigned int code, int value)
> -{
> - /* Sent by the input layer to handle leds and Force Feedback */
> - struct hid_device *dj_hiddev = input_get_drvdata(dev);
> - struct dj_device *dj_dev = dj_hiddev->driver_data;
> -
> - struct dj_receiver_dev *djrcv_dev =
> - dev_get_drvdata(dj_hiddev->dev.parent);
> - struct hid_device *dj_rcv_hiddev = djrcv_dev->hdev;
> - struct hid_report_enum *output_report_enum;
> -
> - struct hid_field *field;
> - struct hid_report *report;
> - unsigned char *data;
> - int offset;
> -
> - dbg_hid("%s: %s, type:%d | code:%d | value:%d\n",
> - __func__, dev->phys, type, code, value);
> -
> - if (type != EV_LED)
> - return -1;
> -
> - offset = hidinput_find_field(dj_hiddev, type, code, &field);
> -
> - if (offset == -1) {
> - dev_warn(&dev->dev, "event field not found\n");
> - return -1;
> - }
> - hid_set_field(field, offset, value);
> -
> - data = hid_alloc_report_buf(field->report, GFP_ATOMIC);
> - if (!data) {
> - dev_warn(&dev->dev, "failed to allocate report buf memory\n");
> - return -1;
> - }
> -
> - hid_output_report(field->report, &data[0]);
> -
> - output_report_enum = &dj_rcv_hiddev->report_enum[HID_OUTPUT_REPORT];
> - report = output_report_enum->report_id_hash[REPORT_ID_DJ_SHORT];
> - hid_set_field(report->field[0], 0, dj_dev->device_index);
> - hid_set_field(report->field[0], 1, REPORT_TYPE_LEDS);
> - hid_set_field(report->field[0], 2, data[1]);
> -
> - hid_hw_request(dj_rcv_hiddev, report, HID_REQ_SET_REPORT);
> -
> - kfree(data);
> -
> - return 0;
> -}
> -
> static int logi_dj_ll_start(struct hid_device *hid)
> {
> dbg_hid("%s\n", __func__);
> @@ -683,7 +655,6 @@ static struct hid_ll_driver logi_dj_ll_driver = {
> .stop = logi_dj_ll_stop,
> .open = logi_dj_ll_open,
> .close = logi_dj_ll_close,
> - .hidinput_input_event = logi_dj_ll_input_event,
> };
>
>
> --
> 1.8.3.1
>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<[email protected]> wrote:
> Well, no use to keep twice the same code.
Yepp, I actually copied the code from that, so this is fine.
Reviewed-by: David Herrmann <[email protected]>
Thanks
David
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/usbhid/hid-core.c | 64 ++++++++-----------------------------------
> 1 file changed, 11 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index f8ca312..406497b 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -915,59 +915,6 @@ static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
> 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;
> - struct usb_device *dev = hid_to_usb_dev(hid);
> - struct usb_interface *intf = usbhid->intf;
> - struct usb_host_interface *interface = intf->cur_altsetting;
> - int ret;
> -
> - if (usbhid->urbout && report_type != HID_FEATURE_REPORT) {
> - int actual_length;
> - int skipped_report_id = 0;
> -
> - if (buf[0] == 0x0) {
> - /* Don't send the Report ID */
> - buf++;
> - count--;
> - skipped_report_id = 1;
> - }
> - ret = usb_interrupt_msg(dev, usbhid->urbout->pipe,
> - buf, count, &actual_length,
> - USB_CTRL_SET_TIMEOUT);
> - /* return the number of bytes transferred */
> - if (ret == 0) {
> - ret = actual_length;
> - /* count also the report id */
> - if (skipped_report_id)
> - ret++;
> - }
> - } else {
> - int skipped_report_id = 0;
> - int report_id = buf[0];
> - if (buf[0] == 0x0) {
> - /* Don't send the Report ID */
> - buf++;
> - count--;
> - skipped_report_id = 1;
> - }
> - 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, if this was a numbered report. */
> - if (ret > 0 && skipped_report_id)
> - ret++;
> - }
> -
> - return ret;
> -}
> -
> static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
> {
> struct usbhid_device *usbhid = hid->driver_data;
> @@ -998,6 +945,17 @@ 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))
> --
> 1.8.3.1
>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<[email protected]> wrote:
> All the different transport drivers use now the generic event handling
> in hid-input. We can remove the handler definitively now.
\o/
Reviewed-by: David Herrmann <[email protected]>
Thanks
David
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/hid-input.c | 4 +---
> include/linux/hid.h | 4 ----
> 2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index a713e62..594722d 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1263,9 +1263,7 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
> }
>
> input_set_drvdata(input_dev, hid);
> - if (hid->ll_driver->hidinput_input_event)
> - input_dev->event = hid->ll_driver->hidinput_input_event;
> - else if (hid->ll_driver->request || hid->hid_output_raw_report)
> + if (hid->ll_driver->request || hid->hid_output_raw_report)
> input_dev->event = hidinput_input_event;
> input_dev->open = hidinput_open;
> input_dev->close = hidinput_close;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index dddcad0..38c307b 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -675,7 +675,6 @@ struct hid_driver {
> * @stop: called on remove
> * @open: called by input layer on open
> * @close: called by input layer on close
> - * @hidinput_input_event: event input event (e.g. ff or leds)
> * @parse: this method is called only once to parse the device data,
> * shouldn't allocate anything to not leak memory
> * @request: send report request to device (e.g. feature report)
> @@ -693,9 +692,6 @@ struct hid_ll_driver {
>
> int (*power)(struct hid_device *hdev, int level);
>
> - int (*hidinput_input_event) (struct input_dev *idev, unsigned int type,
> - unsigned int code, int value);
> -
> int (*parse)(struct hid_device *hdev);
>
> void (*request)(struct hid_device *hdev,
> --
> 1.8.3.1
>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<[email protected]> wrote:
> dev->hid_get_raw_report(X) and hid_hw_raw_request(X, HID_REQ_GET_REPORT)
> are strictly equivalent. Switch the hid subsystem to the hid_hw notation
> and remove the field .hid_get_raw_report in struct hid_device.
Finally we can remove that beast:
Reviewed-by: David Herrmann <[email protected]>
Thanks
David
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/hid-input.c | 6 +++---
> drivers/hid/hid-sony.c | 3 ++-
> drivers/hid/hidraw.c | 7 ++++---
> drivers/hid/i2c-hid/i2c-hid.c | 1 -
> drivers/hid/uhid.c | 1 -
> drivers/hid/usbhid/hid-core.c | 1 -
> include/linux/hid.h | 3 ---
> net/bluetooth/hidp/core.c | 1 -
> 8 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 594722d..1de5997 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -350,9 +350,9 @@ static int hidinput_get_battery_property(struct power_supply *psy,
> ret = -ENOMEM;
> break;
> }
> - ret = dev->hid_get_raw_report(dev, dev->battery_report_id,
> - buf, 2,
> - dev->battery_report_type);
> + ret = hid_hw_raw_request(dev, dev->battery_report_id, buf, 2,
> + dev->battery_report_type,
> + HID_REQ_GET_REPORT);
>
> if (ret != 2) {
> ret = -ENODATA;
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 1235405..3930acb 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -706,7 +706,8 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
> if (!buf)
> return -ENOMEM;
>
> - ret = hdev->hid_get_raw_report(hdev, 0xf2, buf, 17, HID_FEATURE_REPORT);
> + ret = hid_hw_raw_request(hdev, 0xf2, buf, 17, HID_FEATURE_REPORT,
> + HID_REQ_GET_REPORT);
>
> if (ret < 0)
> hid_err(hdev, "can't set operational mode\n");
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index cb0137b..4b2dc95 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -189,7 +189,7 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
>
> dev = hidraw_table[minor]->hid;
>
> - if (!dev->hid_get_raw_report) {
> + if (!dev->ll_driver->raw_request) {
> ret = -ENODEV;
> goto out;
> }
> @@ -216,14 +216,15 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
>
> /*
> * Read the first byte from the user. This is the report number,
> - * which is passed to dev->hid_get_raw_report().
> + * which is passed to hid_hw_raw_request().
> */
> if (copy_from_user(&report_number, buffer, 1)) {
> ret = -EFAULT;
> goto out_free;
> }
>
> - ret = dev->hid_get_raw_report(dev, report_number, buf, count, report_type);
> + ret = hid_hw_raw_request(dev, report_number, buf, count, report_type,
> + HID_REQ_GET_REPORT);
>
> if (ret < 0)
> goto out_free;
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 5099f1f..fe3b392 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -1029,7 +1029,6 @@ static int i2c_hid_probe(struct i2c_client *client,
>
> hid->driver_data = client;
> hid->ll_driver = &i2c_hid_ll_driver;
> - hid->hid_get_raw_report = i2c_hid_get_raw_report;
> hid->hid_output_raw_report = i2c_hid_output_raw_report;
> hid->dev.parent = &client->dev;
> ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index 438c9f1..7358346 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -421,7 +421,6 @@ static int uhid_dev_create(struct uhid_device *uhid,
> hid->uniq[63] = 0;
>
> hid->ll_driver = &uhid_hid_driver;
> - hid->hid_get_raw_report = uhid_hid_get_raw;
> hid->hid_output_raw_report = uhid_hid_output_raw;
> hid->bus = ev->u.create.bus;
> hid->vendor = ev->u.create.vendor;
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 406497b..b9a770f 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1289,7 +1289,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_get_raw_report = usbhid_get_raw_report;
> hid->hid_output_raw_report = usbhid_output_raw_report;
> hid->ff_init = hid_pidff_init;
> #ifdef CONFIG_USB_HIDDEV
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 38c307b..c56681a 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -508,9 +508,6 @@ struct hid_device { /* device report descriptor */
> struct hid_usage *, __s32);
> void (*hiddev_report_event) (struct hid_device *, struct hid_report *);
>
> - /* handler for raw input (Get_Report) data, used by hidraw */
> - int (*hid_get_raw_report) (struct hid_device *, unsigned char, __u8 *, size_t, unsigned char);
> -
> /* handler for raw output data, used by hidraw */
> int (*hid_output_raw_report) (struct hid_device *, __u8 *, size_t, unsigned char);
>
> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index 02670b3..77c4bad 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
> @@ -773,7 +773,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_get_raw_report = hidp_get_raw_report;
> hid->hid_output_raw_report = hidp_output_raw_report;
>
> /* True if device is blacklisted in drivers/hid/hid-core.c */
> --
> 1.8.3.1
>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<[email protected]> wrote:
> Add a helper to access hdev->hid_output_raw_report().
>
> To convert the drivers, use the following snippets:
>
> for i in drivers/hid/*.c
> do
> sed -i.bak "s/[^ \t]*->hid_output_raw_report(/hid_output_raw_report(/g" $i
> done
>
> Then manually fix for checkpatch.pl
Looks all good. I guess you didn't use the hid_hw_* prefix as it's not
a hdrv but hdev callback? But we hopefully can remove this soon,
anyway. So:
Reviewed-by: David Herrmann <[email protected]>
Thanks
David
>
> Signed-off-by: Benjamin Tissoires <[email protected]>
> ---
> drivers/hid/hid-input.c | 2 +-
> drivers/hid/hid-lg.c | 6 ++++--
> drivers/hid/hid-logitech-dj.c | 2 +-
> drivers/hid/hid-magicmouse.c | 2 +-
> drivers/hid/hid-sony.c | 5 +++--
> drivers/hid/hid-thingm.c | 4 ++--
> drivers/hid/hid-wacom.c | 16 +++++++---------
> drivers/hid/hid-wiimote-core.c | 2 +-
> drivers/hid/hidraw.c | 2 +-
> include/linux/hid.h | 16 ++++++++++++++++
> 10 files changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 1de5997..78293c3 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1184,7 +1184,7 @@ static void hidinput_led_worker(struct work_struct *work)
>
> hid_output_report(report, buf);
> /* synchronous output report */
> - hid->hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
> + hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
> kfree(buf);
> }
>
> diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
> index 9fe9d4a..76ed7e5 100644
> --- a/drivers/hid/hid-lg.c
> +++ b/drivers/hid/hid-lg.c
> @@ -692,7 +692,8 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (hdev->product == USB_DEVICE_ID_LOGITECH_WII_WHEEL) {
> unsigned char buf[] = { 0x00, 0xAF, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
>
> - ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
> + ret = hid_output_raw_report(hdev, buf, sizeof(buf),
> + HID_FEATURE_REPORT);
>
> if (ret >= 0) {
> /* insert a little delay of 10 jiffies ~ 40ms */
> @@ -704,7 +705,8 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id)
> buf[1] = 0xB2;
> get_random_bytes(&buf[2], 2);
>
> - ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
> + ret = hid_output_raw_report(hdev, buf, sizeof(buf),
> + HID_FEATURE_REPORT);
> }
> }
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 61d2bbf..9347625 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -567,7 +567,7 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
> out_buf[1] = djdev->device_index;
> memcpy(out_buf + 2, buf, count);
>
> - ret = djrcv_dev->hdev->hid_output_raw_report(djrcv_dev->hdev, out_buf,
> + ret = hid_output_raw_report(djrcv_dev->hdev, out_buf,
> DJREPORT_SHORT_LENGTH, report_type);
>
> kfree(out_buf);
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index 3b43d1c..cb5db3a 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -538,7 +538,7 @@ static int magicmouse_probe(struct hid_device *hdev,
> * but there seems to be no other way of switching the mode.
> * Thus the super-ugly hacky success check below.
> */
> - ret = hdev->hid_output_raw_report(hdev, feature, sizeof(feature),
> + ret = hid_output_raw_report(hdev, feature, sizeof(feature),
> HID_FEATURE_REPORT);
> if (ret != -EIO && ret != sizeof(feature)) {
> hid_err(hdev, "unable to request touch data (%d)\n", ret);
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 3930acb..8494b8c 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -720,7 +720,8 @@ static int sixaxis_set_operational_usb(struct hid_device *hdev)
> static int sixaxis_set_operational_bt(struct hid_device *hdev)
> {
> unsigned char buf[] = { 0xf4, 0x42, 0x03, 0x00, 0x00 };
> - return hdev->hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
> + return hid_output_raw_report(hdev, buf, sizeof(buf),
> + HID_FEATURE_REPORT);
> }
>
> static void buzz_set_leds(struct hid_device *hdev, const __u8 *leds)
> @@ -942,7 +943,7 @@ static void sixaxis_state_worker(struct work_struct *work)
> buf[10] |= sc->led_state[2] << 3;
> buf[10] |= sc->led_state[3] << 4;
>
> - sc->hdev->hid_output_raw_report(sc->hdev, buf, sizeof(buf),
> + hid_output_raw_report(sc->hdev, buf, sizeof(buf),
> HID_OUTPUT_REPORT);
> }
>
> diff --git a/drivers/hid/hid-thingm.c b/drivers/hid/hid-thingm.c
> index 99342cf..7dd3197 100644
> --- a/drivers/hid/hid-thingm.c
> +++ b/drivers/hid/hid-thingm.c
> @@ -48,8 +48,8 @@ static int blink1_send_command(struct blink1_data *data,
> buf[0], buf[1], buf[2], buf[3], buf[4],
> buf[5], buf[6], buf[7], buf[8]);
>
> - ret = data->hdev->hid_output_raw_report(data->hdev, buf,
> - BLINK1_CMD_SIZE, HID_FEATURE_REPORT);
> + ret = hid_output_raw_report(data->hdev, buf, BLINK1_CMD_SIZE,
> + HID_FEATURE_REPORT);
>
> return ret < 0 ? ret : 0;
> }
> diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
> index 60c75dc..c720db9 100644
> --- a/drivers/hid/hid-wacom.c
> +++ b/drivers/hid/hid-wacom.c
> @@ -128,8 +128,7 @@ static void wacom_set_image(struct hid_device *hdev, const char *image,
>
> rep_data[0] = WAC_CMD_ICON_START_STOP;
> rep_data[1] = 0;
> - ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
> - HID_FEATURE_REPORT);
> + ret = hid_output_raw_report(hdev, rep_data, 2, HID_FEATURE_REPORT);
> if (ret < 0)
> goto err;
>
> @@ -143,15 +142,14 @@ static void wacom_set_image(struct hid_device *hdev, const char *image,
> rep_data[j + 3] = p[(i << 6) + j];
>
> rep_data[2] = i;
> - ret = hdev->hid_output_raw_report(hdev, rep_data, 67,
> + ret = hid_output_raw_report(hdev, rep_data, 67,
> HID_FEATURE_REPORT);
> }
>
> rep_data[0] = WAC_CMD_ICON_START_STOP;
> rep_data[1] = 0;
>
> - ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
> - HID_FEATURE_REPORT);
> + ret = hid_output_raw_report(hdev, rep_data, 2, HID_FEATURE_REPORT);
>
> err:
> return;
> @@ -183,7 +181,7 @@ static void wacom_leds_set_brightness(struct led_classdev *led_dev,
> buf[3] = value;
> /* use fixed brightness for OLEDs */
> buf[4] = 0x08;
> - hdev->hid_output_raw_report(hdev, buf, 9, HID_FEATURE_REPORT);
> + hid_output_raw_report(hdev, buf, 9, HID_FEATURE_REPORT);
> kfree(buf);
> }
>
> @@ -339,7 +337,7 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
> rep_data[0] = 0x03 ; rep_data[1] = 0x00;
> limit = 3;
> do {
> - ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
> + ret = hid_output_raw_report(hdev, rep_data, 2,
> HID_FEATURE_REPORT);
> } while (ret < 0 && limit-- > 0);
>
> @@ -352,7 +350,7 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
> rep_data[1] = 0x00;
> limit = 3;
> do {
> - ret = hdev->hid_output_raw_report(hdev,
> + ret = hid_output_raw_report(hdev,
> rep_data, 2, HID_FEATURE_REPORT);
> } while (ret < 0 && limit-- > 0);
>
> @@ -378,7 +376,7 @@ static void wacom_set_features(struct hid_device *hdev, u8 speed)
> rep_data[0] = 0x03;
> rep_data[1] = wdata->features;
>
> - ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
> + ret = hid_output_raw_report(hdev, rep_data, 2,
> HID_FEATURE_REPORT);
> if (ret >= 0)
> wdata->high_speed = speed;
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index abb20db..d7dc6c5b 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -35,7 +35,7 @@ static int wiimote_hid_send(struct hid_device *hdev, __u8 *buffer,
> if (!buf)
> return -ENOMEM;
>
> - ret = hdev->hid_output_raw_report(hdev, buf, count, HID_OUTPUT_REPORT);
> + ret = hid_output_raw_report(hdev, buf, count, HID_OUTPUT_REPORT);
>
> kfree(buf);
> return ret;
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 4b2dc95..f8708c9 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -153,7 +153,7 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
> goto out_free;
> }
>
> - ret = dev->hid_output_raw_report(dev, buf, count, report_type);
> + ret = hid_output_raw_report(dev, buf, count, report_type);
> out_free:
> kfree(buf);
> out:
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index c56681a..a837ede 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -1012,6 +1012,22 @@ 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
> --
> 1.8.3.1
>
On Mon, Feb 3, 2014 at 11:06 AM, David Herrmann <[email protected]> wrote:
> Hi
>
> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
> <[email protected]> wrote:
>> hid-logitech-dj uses its own ->hidinput_input_event() instead of
>> the generic binding in hid-input.
>> Moving the handling of LEDs towards logi_dj_output_hidraw_report()
>> allows two things:
>> - remove hidinput_input_event in struct hid_device
>> - hidraw user space programs can also set the LEDs
>>
>> Signed-off-by: Benjamin Tissoires <[email protected]>
>> ---
>> drivers/hid/hid-logitech-dj.c | 99 +++++++++++++++----------------------------
>> 1 file changed, 35 insertions(+), 64 deletions(-)
>>
>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>> index f45279c..61d2bbf 100644
>> --- a/drivers/hid/hid-logitech-dj.c
>> +++ b/drivers/hid/hid-logitech-dj.c
>> @@ -44,14 +44,6 @@ static const char kbd_descriptor[] = {
>> 0x19, 0xE0, /* USAGE_MINIMUM (Left Control) */
>> 0x29, 0xE7, /* USAGE_MAXIMUM (Right GUI) */
>> 0x81, 0x02, /* INPUT (Data,Var,Abs) */
>> - 0x95, 0x05, /* REPORT COUNT (5) */
>> - 0x05, 0x08, /* USAGE PAGE (LED page) */
>> - 0x19, 0x01, /* USAGE MINIMUM (1) */
>> - 0x29, 0x05, /* USAGE MAXIMUM (5) */
>> - 0x91, 0x02, /* OUTPUT (Data, Variable, Absolute) */
>> - 0x95, 0x01, /* REPORT COUNT (1) */
>> - 0x75, 0x03, /* REPORT SIZE (3) */
>> - 0x91, 0x01, /* OUTPUT (Constant) */
>> 0x95, 0x06, /* REPORT_COUNT (6) */
>> 0x75, 0x08, /* REPORT_SIZE (8) */
>> 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
>> @@ -60,6 +52,18 @@ static const char kbd_descriptor[] = {
>> 0x19, 0x00, /* USAGE_MINIMUM (no event) */
>> 0x2A, 0xFF, 0x00, /* USAGE_MAXIMUM (reserved) */
>> 0x81, 0x00, /* INPUT (Data,Ary,Abs) */
>> + 0x85, 0x0e, /* REPORT_ID (14) */
>> + 0x05, 0x08, /* USAGE PAGE (LED page) */
>> + 0x95, 0x05, /* REPORT COUNT (5) */
>> + 0x75, 0x01, /* REPORT SIZE (1) */
>> + 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
>> + 0x25, 0x01, /* LOGICAL_MAXIMUM (1) */
>> + 0x19, 0x01, /* USAGE MINIMUM (1) */
>> + 0x29, 0x05, /* USAGE MAXIMUM (5) */
>> + 0x91, 0x02, /* OUTPUT (Data, Variable, Absolute) */
>> + 0x95, 0x01, /* REPORT COUNT (1) */
>> + 0x75, 0x03, /* REPORT SIZE (3) */
>> + 0x91, 0x01, /* OUTPUT (Constant) */
>> 0xC0
>> };
>>
>> @@ -544,10 +548,30 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
>> size_t count,
>> unsigned char report_type)
>> {
>> - /* Called by hid raw to send data */
>> - dbg_hid("%s\n", __func__);
>> + struct dj_device *djdev = hid->driver_data;
>> + struct dj_receiver_dev *djrcv_dev = djdev->dj_receiver_dev;
>> + u8 *out_buf;
>> + int ret;
>>
>> - return 0;
>> + if (buf[0] != REPORT_TYPE_LEDS)
>> + return -EINVAL;
>> +
>> + out_buf = kzalloc(DJREPORT_SHORT_LENGTH, GFP_ATOMIC);
>> + if (!out_buf)
>> + return -ENOMEM;
>> +
>> + if (count < DJREPORT_SHORT_LENGTH - 2)
>> + count = DJREPORT_SHORT_LENGTH - 2;
>> +
>> + out_buf[0] = REPORT_ID_DJ_SHORT;
>> + out_buf[1] = djdev->device_index;
>> + memcpy(out_buf + 2, buf, count);
>> +
>> + ret = djrcv_dev->hdev->hid_output_raw_report(djrcv_dev->hdev, out_buf,
>> + DJREPORT_SHORT_LENGTH, report_type);
>
> Strictly speaking the code below uses HID_REQ_SET_REPORT and you
> replace it with hid_output_raw_report() here (which at least for BTHID
> is not equivalent). Anyhow, HID-core uses hid_output_raw_report(),
> too, so this should be fine.
Yes, you are right. This fires back on me yesterday when I tried to
remove the hid_output_raw_report() calls.
It occurs it works because usbhid_out_raw_report() uses set_report
when there is no urbout, which is the case with logitech receivers.
So I guess an ideal solution would be to implement a hid_hw_request call.
However, the only concern I have with hid_hw_request is that it does
not allow hidraw to play with the LEDs given that hidraw does not have
an ioctl for SET_REPORT (or GET_REPORT). I already seen this issue
regarding i2c_hid at some point, but it was "solved" in i2c_hid by
using the same ugly trick that we have in USB-hid.
Cheers,
Benjamin
>
> Thanks
> David
>
>> +
>> + kfree(out_buf);
>> + return ret;
>> }
>>
>> static void rdcat(char *rdesc, unsigned int *rsize, const char *data, unsigned int size)
>> @@ -613,58 +637,6 @@ static int logi_dj_ll_parse(struct hid_device *hid)
>> return retval;
>> }
>>
>> -static int logi_dj_ll_input_event(struct input_dev *dev, unsigned int type,
>> - unsigned int code, int value)
>> -{
>> - /* Sent by the input layer to handle leds and Force Feedback */
>> - struct hid_device *dj_hiddev = input_get_drvdata(dev);
>> - struct dj_device *dj_dev = dj_hiddev->driver_data;
>> -
>> - struct dj_receiver_dev *djrcv_dev =
>> - dev_get_drvdata(dj_hiddev->dev.parent);
>> - struct hid_device *dj_rcv_hiddev = djrcv_dev->hdev;
>> - struct hid_report_enum *output_report_enum;
>> -
>> - struct hid_field *field;
>> - struct hid_report *report;
>> - unsigned char *data;
>> - int offset;
>> -
>> - dbg_hid("%s: %s, type:%d | code:%d | value:%d\n",
>> - __func__, dev->phys, type, code, value);
>> -
>> - if (type != EV_LED)
>> - return -1;
>> -
>> - offset = hidinput_find_field(dj_hiddev, type, code, &field);
>> -
>> - if (offset == -1) {
>> - dev_warn(&dev->dev, "event field not found\n");
>> - return -1;
>> - }
>> - hid_set_field(field, offset, value);
>> -
>> - data = hid_alloc_report_buf(field->report, GFP_ATOMIC);
>> - if (!data) {
>> - dev_warn(&dev->dev, "failed to allocate report buf memory\n");
>> - return -1;
>> - }
>> -
>> - hid_output_report(field->report, &data[0]);
>> -
>> - output_report_enum = &dj_rcv_hiddev->report_enum[HID_OUTPUT_REPORT];
>> - report = output_report_enum->report_id_hash[REPORT_ID_DJ_SHORT];
>> - hid_set_field(report->field[0], 0, dj_dev->device_index);
>> - hid_set_field(report->field[0], 1, REPORT_TYPE_LEDS);
>> - hid_set_field(report->field[0], 2, data[1]);
>> -
>> - hid_hw_request(dj_rcv_hiddev, report, HID_REQ_SET_REPORT);
>> -
>> - kfree(data);
>> -
>> - return 0;
>> -}
>> -
>> static int logi_dj_ll_start(struct hid_device *hid)
>> {
>> dbg_hid("%s\n", __func__);
>> @@ -683,7 +655,6 @@ static struct hid_ll_driver logi_dj_ll_driver = {
>> .stop = logi_dj_ll_stop,
>> .open = logi_dj_ll_open,
>> .close = logi_dj_ll_close,
>> - .hidinput_input_event = logi_dj_ll_input_event,
>> };
>>
>>
>> --
>> 1.8.3.1
>>
Hi
On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
<[email protected]> wrote:
> Hi guys,
>
> This is an attempt to complete the branch for-3.15/ll-driver-new-callbacks:
> - try to implement as much as possible ll_driver callbacks (some are still
> missing, but I did not had the time to complete it)
> - add inliners hid_hw_* for all the ll_driver callbacks
> - remove transport dependant callbacks in struct hid_device
> - remove the so called "obsolete" hidinput_input_event handler which was used
> in 2/4 transport drivers
>
> Bonus point: 14 files changed, 213 insertions(+), 272 deletions(-)
> Yay!
>
> I made sure I kept the PS3 controller working and the LEDs (on logitech-dj and
> on bluetooth keyboard) working. The rest do not mostly need further testing,
> the code path did not changed. But still, a review (and some tests) would be a
> good idea :)
Thanks a lot! Looks all good except for the uhid and i2c changes. But
I have commented on those.
Cheers
David
> Cheers,
> Benjamin
>
> PS: obviously, this goes on top on the branch for-3.15/ll-driver-new-callbacks
>
> Benjamin Tissoires (11):
> HID: uHID: implement .raw_request
> HID: i2c-hid: implement ll_driver transport-layer callbacks
> HID: add inliners for ll_driver transport-layer callbacks
> HID: logitech-dj: remove hidinput_input_event
> HID: HIDp: remove hidp_hidinput_event
> HID: remove hidinput_input_event handler
> HID: HIDp: remove duplicated coded
> HID: usbhid: remove duplicated code
> HID: remove hid_get_raw_report in struct hid_device
> HID: introduce helper to access hid_output_raw_report()
> HID: move hid_output_raw_report to hid_ll_driver
>
> drivers/hid/hid-input.c | 12 ++---
> drivers/hid/hid-lg.c | 6 ++-
> drivers/hid/hid-logitech-dj.c | 101 +++++++++++++---------------------
> drivers/hid/hid-magicmouse.c | 2 +-
> drivers/hid/hid-sony.c | 19 +++++--
> drivers/hid/hid-thingm.c | 4 +-
> drivers/hid/hid-wacom.c | 16 +++---
> drivers/hid/hid-wiimote-core.c | 4 +-
> drivers/hid/hidraw.c | 11 ++--
> drivers/hid/i2c-hid/i2c-hid.c | 27 +++++++++-
> drivers/hid/uhid.c | 20 ++++++-
> drivers/hid/usbhid/hid-core.c | 67 +++++------------------
> include/linux/hid.h | 77 ++++++++++++++++++++++----
> net/bluetooth/hidp/core.c | 119 +++++------------------------------------
> 14 files changed, 213 insertions(+), 272 deletions(-)
>
> --
> 1.8.3.1
>
Hi
On Mon, Feb 3, 2014 at 5:21 PM, Benjamin Tissoires
<[email protected]> wrote:
> On Mon, Feb 3, 2014 at 11:06 AM, David Herrmann <[email protected]> wrote:
>> Hi
>>
>> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
>> <[email protected]> wrote:
>>> hid-logitech-dj uses its own ->hidinput_input_event() instead of
>>> the generic binding in hid-input.
>>> Moving the handling of LEDs towards logi_dj_output_hidraw_report()
>>> allows two things:
>>> - remove hidinput_input_event in struct hid_device
>>> - hidraw user space programs can also set the LEDs
>>>
>>> Signed-off-by: Benjamin Tissoires <[email protected]>
>>> ---
>>> drivers/hid/hid-logitech-dj.c | 99 +++++++++++++++----------------------------
>>> 1 file changed, 35 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>>> index f45279c..61d2bbf 100644
>>> --- a/drivers/hid/hid-logitech-dj.c
>>> +++ b/drivers/hid/hid-logitech-dj.c
>>> @@ -44,14 +44,6 @@ static const char kbd_descriptor[] = {
>>> 0x19, 0xE0, /* USAGE_MINIMUM (Left Control) */
>>> 0x29, 0xE7, /* USAGE_MAXIMUM (Right GUI) */
>>> 0x81, 0x02, /* INPUT (Data,Var,Abs) */
>>> - 0x95, 0x05, /* REPORT COUNT (5) */
>>> - 0x05, 0x08, /* USAGE PAGE (LED page) */
>>> - 0x19, 0x01, /* USAGE MINIMUM (1) */
>>> - 0x29, 0x05, /* USAGE MAXIMUM (5) */
>>> - 0x91, 0x02, /* OUTPUT (Data, Variable, Absolute) */
>>> - 0x95, 0x01, /* REPORT COUNT (1) */
>>> - 0x75, 0x03, /* REPORT SIZE (3) */
>>> - 0x91, 0x01, /* OUTPUT (Constant) */
>>> 0x95, 0x06, /* REPORT_COUNT (6) */
>>> 0x75, 0x08, /* REPORT_SIZE (8) */
>>> 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
>>> @@ -60,6 +52,18 @@ static const char kbd_descriptor[] = {
>>> 0x19, 0x00, /* USAGE_MINIMUM (no event) */
>>> 0x2A, 0xFF, 0x00, /* USAGE_MAXIMUM (reserved) */
>>> 0x81, 0x00, /* INPUT (Data,Ary,Abs) */
>>> + 0x85, 0x0e, /* REPORT_ID (14) */
>>> + 0x05, 0x08, /* USAGE PAGE (LED page) */
>>> + 0x95, 0x05, /* REPORT COUNT (5) */
>>> + 0x75, 0x01, /* REPORT SIZE (1) */
>>> + 0x15, 0x00, /* LOGICAL_MINIMUM (0) */
>>> + 0x25, 0x01, /* LOGICAL_MAXIMUM (1) */
>>> + 0x19, 0x01, /* USAGE MINIMUM (1) */
>>> + 0x29, 0x05, /* USAGE MAXIMUM (5) */
>>> + 0x91, 0x02, /* OUTPUT (Data, Variable, Absolute) */
>>> + 0x95, 0x01, /* REPORT COUNT (1) */
>>> + 0x75, 0x03, /* REPORT SIZE (3) */
>>> + 0x91, 0x01, /* OUTPUT (Constant) */
>>> 0xC0
>>> };
>>>
>>> @@ -544,10 +548,30 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
>>> size_t count,
>>> unsigned char report_type)
>>> {
>>> - /* Called by hid raw to send data */
>>> - dbg_hid("%s\n", __func__);
>>> + struct dj_device *djdev = hid->driver_data;
>>> + struct dj_receiver_dev *djrcv_dev = djdev->dj_receiver_dev;
>>> + u8 *out_buf;
>>> + int ret;
>>>
>>> - return 0;
>>> + if (buf[0] != REPORT_TYPE_LEDS)
>>> + return -EINVAL;
>>> +
>>> + out_buf = kzalloc(DJREPORT_SHORT_LENGTH, GFP_ATOMIC);
>>> + if (!out_buf)
>>> + return -ENOMEM;
>>> +
>>> + if (count < DJREPORT_SHORT_LENGTH - 2)
>>> + count = DJREPORT_SHORT_LENGTH - 2;
>>> +
>>> + out_buf[0] = REPORT_ID_DJ_SHORT;
>>> + out_buf[1] = djdev->device_index;
>>> + memcpy(out_buf + 2, buf, count);
>>> +
>>> + ret = djrcv_dev->hdev->hid_output_raw_report(djrcv_dev->hdev, out_buf,
>>> + DJREPORT_SHORT_LENGTH, report_type);
>>
>> Strictly speaking the code below uses HID_REQ_SET_REPORT and you
>> replace it with hid_output_raw_report() here (which at least for BTHID
>> is not equivalent). Anyhow, HID-core uses hid_output_raw_report(),
>> too, so this should be fine.
>
> Yes, you are right. This fires back on me yesterday when I tried to
> remove the hid_output_raw_report() calls.
> It occurs it works because usbhid_out_raw_report() uses set_report
> when there is no urbout, which is the case with logitech receivers.
> So I guess an ideal solution would be to implement a hid_hw_request call.
>
> However, the only concern I have with hid_hw_request is that it does
> not allow hidraw to play with the LEDs given that hidraw does not have
> an ioctl for SET_REPORT (or GET_REPORT). I already seen this issue
> regarding i2c_hid at some point, but it was "solved" in i2c_hid by
> using the same ugly trick that we have in USB-hid.
Hah! That's why it worked all the years, USB-HID checks for !urbout..
and yeah, that's what I just criticized in your i2c-hid patch.. Ok,
I'm fine if we make output_report() fall back to SET_REPORT if not
output-channel is supported. But we it gets ugly if we rely on that
from the upper stack... hmm, don't know how to make that work but
adding a hid quirk to use SET_REPORT for hidinput_input_report().
Thanks
David
On Mon, Feb 3, 2014 at 11:04 AM, David Herrmann <[email protected]> wrote:
> Hi
>
> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
> <[email protected]> wrote:
>> Add output_report and raw_request to i2c-hid.
>> Hopefully, we will manage to have the same transport level between
>> all the transport drivers.
>>
>> Signed-off-by: Benjamin Tissoires <[email protected]>
>> ---
>> drivers/hid/i2c-hid/i2c-hid.c | 24 ++++++++++++++++++++++++
>> 1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index ce68a12..5099f1f 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -574,6 +574,28 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
>> return ret;
>> }
>>
>> +static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
>> + size_t count)
>> +{
>> + return i2c_hid_output_raw_report(hid, buf, count, HID_OUTPUT_REPORT);
>> +}
>> +
>> +static int i2c_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
>> + __u8 *buf, size_t len, unsigned char rtype,
>> + int reqtype)
>> +{
>> + switch (reqtype) {
>> + case HID_REQ_GET_REPORT:
>> + return i2c_hid_get_raw_report(hid, reportnum, buf, len, rtype);
>> + case HID_REQ_SET_REPORT:
>> + if (buf[0] != reportnum)
>> + return -EINVAL;
>> + return i2c_hid_output_raw_report(hid, buf, len, rtype);
>
> I just skimmed the I2C-HID specs and it defines three methods for
> input/output reports:
>
> 1) Section 6.2:
> raw async output-reports can be sent by writing the data at any time
> to wOutputRegister.
> This should be used as method for hid->output_report().
>
> 2) Section 7.1:
> SET_REPORT can be issued by writing the right OPCODE + report-ID into
> wCommandRegister and the data into wDataRegister.
> This should be used as method for hid->raw_request() + HID_REQ_SET_REPORT.
>
> 3) Section 7.1:
> GET_REPORT can be issued by writing the right OPCODE + report-ID into
> wCommandRegister and then waiting for the device to write the data
> into wDataRegister.
> This should be used for hid->raw_request() + HID_REQ_GET_REPORT
>
>
> The GET_REPORT implementation looks fine to me, but the
> i2c_hid_set_report() seems to support both 1) and 2) depending on the
> passed type and capabilities:
> - it uses 2) for FEATURE_REPORT reports or if the max OUTPUT_LENGTH is 0
> - it uses 1) otherwise
>
> I'm not sure whether the i2c-hid-spec mandates this behavior, so I am
> not saying it's wrong. I just wanna understand what we do here. So if
> we use hid->output_report() with HID_FEATURE_REPORT, the current code
> turns this into a SET_REPORT. Likewise, an hid->raw_request() with
> HID_REQ_SET_REPORT but with HID_OUTPUT_REPORT turns into an
> output-report.
Ok, just for the record (I think that you already answered this in the
4/11 review):
this behavior has been added in commit
811adb9622de310efbb661531c3ec0ae5d2b2bc0 for Synaptics so they can
talk to their HID/I2C firmware.
The use they are making is through hdev->hid_output_raw_report(), and
at that time output_report() did not exist.
I guess that with the new API (output_report() and raw_request() ) we
can revert this and it will not bother them given that rmi-hid.c is
still not upstream.
I have no clue if anyone else is using hdev->hid_output_raw_report()
for I2C devices, but we could still try to clean this right now given
that those devices are not well spread (besides some hid-multitouch
touchscreens which do not use OUTPUT reports).
I am still a little bit concerned by hidraw not able to call
set_report in this case, but this could be fixed also by adding an
ioctl.
How does that sound?
Cheers,
Benjamin
>
> I'd rather expect this behavior:
>
> hid->output_report() should always do this:
> args[index++] = outputRegister & 0xFF;
> args[index++] = outputRegister >> 8;
> hidcmd = &hid_no_cmd;
>
> while hid->raw_request() should always do this:
> args[index++] = dataRegister & 0xFF;
> args[index++] = dataRegister >> 8;
>
> The special case for maxOutputLength==0 seems fine to me, but the
> "reportType == 0x03" looks weird.
>
>
> Thanks
> David
>
>> + default:
>> + return -EIO;
>> + }
>> +}
>> +
>> static void i2c_hid_request(struct hid_device *hid, struct hid_report *rep,
>> int reqtype)
>> {
>> @@ -761,6 +783,8 @@ static struct hid_ll_driver i2c_hid_ll_driver = {
>> .close = i2c_hid_close,
>> .power = i2c_hid_power,
>> .request = i2c_hid_request,
>> + .output_report = i2c_hid_output_report,
>> + .raw_request = i2c_hid_raw_request,
>> };
>>
>> static int i2c_hid_init_irq(struct i2c_client *client)
>> --
>> 1.8.3.1
>>
On Mon, Feb 3, 2014 at 10:26 AM, David Herrmann <[email protected]> wrote:
> Hi
>
> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
> <[email protected]> wrote:
>> It was missing, so adding it.
>>
>> Signed-off-by: Benjamin Tissoires <[email protected]>
>> ---
>> drivers/hid/uhid.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
>> index f5a2b19..438c9f1 100644
>> --- a/drivers/hid/uhid.c
>> +++ b/drivers/hid/uhid.c
>> @@ -270,6 +270,22 @@ static int uhid_hid_output_report(struct hid_device *hid, __u8 *buf,
>> return count;
>> }
>>
>> +static int uhid_raw_request(struct hid_device *hid, unsigned char reportnum,
>> + __u8 *buf, size_t len, unsigned char rtype,
>> + int reqtype)
>> +{
>> + switch (reqtype) {
>> + case HID_REQ_GET_REPORT:
>> + return uhid_hid_get_raw(hid, reportnum, buf, len, rtype);
>> + case HID_REQ_SET_REPORT:
>> + if (buf[0] != reportnum)
>> + return -EINVAL;
>> + return uhid_hid_output_raw(hid, buf, len, rtype);
>
> But that one looks wrong. UHID does not have any SET_REPORT query in
> the protocol, yet. You turn a SET_REPORT into an OUTPUT_REPORT here.
> So if user-space gets the UHID_OUTPUT event, it doesn't know whether
> to send a SET_REPORT@ctrl to the device, or an async
> OUTPUT_REPORT@intr. This at least matters for low-energy BT in bluez,
> which uses uhid.
right. So we can drop this for now.
>
> So we'd have to add an UHID_SET_REPORT event. Note that currently
> UHID_FEATURE is the equivalent of UHID_GET_REPORT, but just horribly
> named..
ouch. I think this is something which can be fixed quite easily (by
marking UHID_FEATURE obsolete and creating the two events).
However, I don't think I will have the time to make the change and do
proper testings in the next few days/weeks.
Can somebody else take this?
Cheers,
Benjamin
>
> Thanks
> David
>
>> + default:
>> + return -EIO;
>> + }
>> +}
>> +
>> static struct hid_ll_driver uhid_hid_driver = {
>> .start = uhid_hid_start,
>> .stop = uhid_hid_stop,
>> @@ -277,6 +293,7 @@ static struct hid_ll_driver uhid_hid_driver = {
>> .close = uhid_hid_close,
>> .parse = uhid_hid_parse,
>> .output_report = uhid_hid_output_report,
>> + .raw_request = uhid_raw_request,
>> };
>>
>> #ifdef CONFIG_COMPAT
>> --
>> 1.8.3.1
>>
On Mon, Feb 3, 2014 at 11:48 AM, David Herrmann <[email protected]> wrote:
> Hi
>
> On Sun, Feb 2, 2014 at 5:50 AM, Benjamin Tissoires
> <[email protected]> wrote:
>> Hi guys,
>>
>> This is an attempt to complete the branch for-3.15/ll-driver-new-callbacks:
>> - try to implement as much as possible ll_driver callbacks (some are still
>> missing, but I did not had the time to complete it)
>> - add inliners hid_hw_* for all the ll_driver callbacks
>> - remove transport dependant callbacks in struct hid_device
>> - remove the so called "obsolete" hidinput_input_event handler which was used
>> in 2/4 transport drivers
>>
>> Bonus point: 14 files changed, 213 insertions(+), 272 deletions(-)
>> Yay!
>>
>> I made sure I kept the PS3 controller working and the LEDs (on logitech-dj and
>> on bluetooth keyboard) working. The rest do not mostly need further testing,
>> the code path did not changed. But still, a review (and some tests) would be a
>> good idea :)
>
> Thanks a lot! Looks all good except for the uhid and i2c changes. But
> I have commented on those.
Thanks David for the review. Very appreciated.
Jiri, I think the simpler way to handle this is that I send out a v2
of the patches with the reviewed-by David:
I will drop 1, 2 and 11 and rework a little on 4 (hid-logitech-dj).
Then, I'll send out the second series where I will remove
hid_output_raw_report, implement a generic .request() on top of
.raw_request(), and I have some more cleanup to come (less aggressive
this time I think).
Cheers,
Benjamin
>
> Cheers
> David
>
>> Cheers,
>> Benjamin
>>
>> PS: obviously, this goes on top on the branch for-3.15/ll-driver-new-callbacks
>>
>> Benjamin Tissoires (11):
>> HID: uHID: implement .raw_request
>> HID: i2c-hid: implement ll_driver transport-layer callbacks
>> HID: add inliners for ll_driver transport-layer callbacks
>> HID: logitech-dj: remove hidinput_input_event
>> HID: HIDp: remove hidp_hidinput_event
>> HID: remove hidinput_input_event handler
>> HID: HIDp: remove duplicated coded
>> HID: usbhid: remove duplicated code
>> HID: remove hid_get_raw_report in struct hid_device
>> HID: introduce helper to access hid_output_raw_report()
>> HID: move hid_output_raw_report to hid_ll_driver
>>
>> drivers/hid/hid-input.c | 12 ++---
>> drivers/hid/hid-lg.c | 6 ++-
>> drivers/hid/hid-logitech-dj.c | 101 +++++++++++++---------------------
>> drivers/hid/hid-magicmouse.c | 2 +-
>> drivers/hid/hid-sony.c | 19 +++++--
>> drivers/hid/hid-thingm.c | 4 +-
>> drivers/hid/hid-wacom.c | 16 +++---
>> drivers/hid/hid-wiimote-core.c | 4 +-
>> drivers/hid/hidraw.c | 11 ++--
>> drivers/hid/i2c-hid/i2c-hid.c | 27 +++++++++-
>> drivers/hid/uhid.c | 20 ++++++-
>> drivers/hid/usbhid/hid-core.c | 67 +++++------------------
>> include/linux/hid.h | 77 ++++++++++++++++++++++----
>> net/bluetooth/hidp/core.c | 119 +++++------------------------------------
>> 14 files changed, 213 insertions(+), 272 deletions(-)
>>
>> --
>> 1.8.3.1
>>