Thread (46 messages) 46 messages, 3 authors, 2012-11-06

Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path

From: Benjamin Tissoires <hidden>
Date: 2012-10-30 11:04:45
Also in: lkml

Hi Henrik,

On Mon, Oct 29, 2012 at 11:57 PM, Henrik Rydberg [off-list ref] wrote:
Hi Benjamin,
quoted
This patch factorizes the hid set_feature command by using
hid_device->hid_output_raw_report instead of direclty relying on
usbhid. This makes the driver usb independant.

However I still can't remove the 2 usb related headers because the
function mt_resume has a specific patch for usb devices.

Signed-off-by: Benjamin Tissoires <redacted>
---
 drivers/hid/hid-multitouch.c | 63 ++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 26 deletions(-)
In my drawer, I have a patchset that aims to remove all usbhid
dependence, from all the drivers. Perhaps the attached patch is
something to consider here?
yep, removing usbhid dependencies is a good thing.
See my review below :)
quoted hunk ↗ jump to hunk
quoted
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 21a120b..33038c5 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -670,47 +670,58 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
      return 1;
 }

-static void mt_set_input_mode(struct hid_device *hdev)
+static void mt_set_feature(struct hid_device *hdev, __s8 feature_id,
+     u8 value, size_t index)
 {
-     struct mt_device *td = hid_get_drvdata(hdev);
      struct hid_report *r;
      struct hid_report_enum *re;
+     u8 *data;
+     u8 max_value;
+     int len;
+
+     if (feature_id < 0 || !hdev->hid_output_raw_report)
+             return;
+
+     re = &hdev->report_enum[HID_FEATURE_REPORT];
+     r = re->report_id_hash[feature_id];
+     if (!r)
+             return;
+
+     len = ((r->size - 1) >> 3) + 1 + (r->id > 0);
+     max_value = r->field[0]->logical_maximum;
+     value = min(value, max_value);

-     if (td->inputmode < 0)
+     if (r->field[0]->value[index] == value || len < 2 || index + 1 >= len)
              return;

-     re = &(hdev->report_enum[HID_FEATURE_REPORT]);
-     r = re->report_id_hash[td->inputmode];
-     if (r) {
-             r->field[0]->value[td->inputmode_index] = 0x02;
-             usbhid_submit_report(hdev, r, USB_DIR_OUT);
+     data = kzalloc(len, GFP_ATOMIC);
+     if (!data) {
+             hid_warn(hdev, "output queueing failed\n");
+             return;
      }
+
+     data[0] = r->id;
+     data[index + 1] = value;
+     hdev->hid_output_raw_report(hdev, data, len, HID_FEATURE_REPORT);
+     kfree(data);
 }

-static void mt_set_maxcontacts(struct hid_device *hdev)
+static void mt_set_input_mode(struct hid_device *hdev)
 {
      struct mt_device *td = hid_get_drvdata(hdev);
-     struct hid_report *r;
-     struct hid_report_enum *re;
-     int fieldmax, max;

-     if (td->maxcontact_report_id < 0)
-             return;
+     mt_set_feature(hdev, td->inputmode, 0x02, td->inputmode_index);
+}

-     if (!td->mtclass.maxcontacts)
+static void mt_set_maxcontacts(struct hid_device *hdev)
+{
+     struct mt_device *td = hid_get_drvdata(hdev);
+     int max = td->mtclass.maxcontacts;
+
+     if (!max)
              return;

-     re = &hdev->report_enum[HID_FEATURE_REPORT];
-     r = re->report_id_hash[td->maxcontact_report_id];
-     if (r) {
-             max = td->mtclass.maxcontacts;
-             fieldmax = r->field[0]->logical_maximum;
-             max = min(fieldmax, max);
-             if (r->field[0]->value[0] != max) {
-                     r->field[0]->value[0] = max;
-                     usbhid_submit_report(hdev, r, USB_DIR_OUT);
-             }
-     }
+     mt_set_feature(hdev, td->maxcontact_report_id, max, 0);
 }

 static void mt_post_parse_default_settings(struct mt_device *td)
--
1.7.11.7
Thanks,
Henrik

---

From 5d9a791e0a9e41bcea0fcb286e2849b403675f37 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg <redacted>
Date: Mon, 2 Jul 2012 20:38:21 +0200
Subject: [PATCH 3/7] hid: extend interface with report requests

---
 drivers/hid/usbhid/hid-core.c | 14 ++++++++++++++
 include/linux/hid.h           | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index de1f9ac..3c618da 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1251,6 +1251,18 @@ static int usbhid_power(struct hid_device *hid, int lvl)
        return r;
 }

+static void usbhid_send(struct hid_device *hid, struct hid_report *rep, int req)
+{
+       switch (req) {
+       case HID_REQ_GET_REPORT:
+               usbhid_submit_report(hid, rep, USB_DIR_IN);
+               break;
+       case HID_REQ_SET_REPORT:
+               usbhid_submit_report(hid, rep, USB_DIR_OUT);
+               break;
+       }
+}
+
 static struct hid_ll_driver usb_hid_driver = {
        .parse = usbhid_parse,
        .start = usbhid_start,
@@ -1259,6 +1271,8 @@ static struct hid_ll_driver usb_hid_driver = {
        .close = usbhid_close,
        .power = usbhid_power,
        .hidinput_input_event = usb_hidinput_input_event,
+       .send = usbhid_send,
the name here is a little bit misleading. You are using "send" to also
"get" reports...
Maybe "hid_request" is a better name. This will allows us to add the
missing function:
Get_Descriptor, Set_Descriptor, Get_Report Request, Set_Report
Request, Get_Idle, Set_Idle, Get_Protocol, Set_Protocol and maybe
others - even WAIT for instance :)
+       .wait = usbhid_wait_io,
is it really necessary to wait for IO? (I think it will not be one
line for hid over i2c...).

Cheers,
Benjamin
quoted hunk ↗ jump to hunk
 };

 static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *id)
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 241eb40..5e169c1 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -686,6 +686,8 @@ struct hid_driver {
  * @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
+ * @send: send report request to device (e.g. feature report)
+ * @wait: wait for buffered io to complete (send/recv reports)
  */
 struct hid_ll_driver {
        int (*start)(struct hid_device *hdev);
@@ -700,6 +702,11 @@ struct hid_ll_driver {
                        unsigned int code, int value);

        int (*parse)(struct hid_device *hdev);
+
+       void (*send)(struct hid_device *hdev,
+                    struct hid_report *report, int req);
+       int (*wait)(struct hid_device *hdev);
+
 };

 #define        PM_HINT_FULLON  1<<5
@@ -892,6 +899,32 @@ static inline int hid_hw_power(struct hid_device *hdev, int level)
        return hdev->ll_driver->power ? hdev->ll_driver->power(hdev, level) : 0;
 }

+
+/**
+ * hid_hw_send - send report request to device
+ *
+ * @hdev: hid device
+ * @report: report to send
+ * @req: hid request type
+ */
+static inline void hid_hw_send(struct hid_device *hdev,
+                              struct hid_report *report, int req)
+{
+       if (hdev->ll_driver->send)
+               hdev->ll_driver->send(hdev, report, req);
+}
+
+/**
+ * hid_hw_wait - wait for buffered io to complete
+ *
+ * @hdev: hid device
+ */
+static inline void hid_hw_wait(struct hid_device *hdev)
+{
+       if (hdev->ll_driver->wait)
+               hdev->ll_driver->wait(hdev);
+}
+
 int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
                int interrupt);

--
1.7.11.1
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help