Re: [PATCH] HID: rmi: Add support for the touchpad in the Razer Blade 14 laptop
From: Andrew Duggan
Date: Wed Dec 10 2014 - 20:02:36 EST
Hi Benjamin,
Thanks for reviewing my patch. Comments and questions below.
On 12/10/2014 12:51 PM, Benjamin Tissoires wrote:
On Mon, Dec 8, 2014 at 7:16 PM, Andrew Duggan <aduggan@xxxxxxxxxxxxx> wrote:
On 11/24/2014 04:20 PM, Andrew Duggan wrote:
The Razer Blade 14 has a Synaptic's TouchPad on one of the interfaces of
a composite USB device. This patch allows the hid-rmi driver to bind
to that interface. It also adds support for the external click buttons
on the Razer's touchpad. External buttons are reported using generic
mouse reports instead of through the F30 like it is on ClickPads.
Signed-off-by: Andrew Duggan <aduggan@xxxxxxxxxxxxx>
---
This patch depends on the "HID: rmi: Scan the report descriptor to
determine if the device is
suitable for the hid-rmi driver" I submitted earlier today to correctly
bind to the touchpad HID
device in the composite USB device.
Any comments on this patch?
Again, sorry for the lag on this series. I think now that I re-read
this one I understood why I did not put too many efforts to properly
review the series. This one is a little bit worrisome IMO.
I wasn't sure about this patch either. But, after waiting a while and
not coming with with anything better I figured I would post it to at
least get some feedback.
Thanks,
Andrew
drivers/hid/hid-core.c | 4 +++-
drivers/hid/hid-ids.h | 3 +++
drivers/hid/hid-rmi.c | 15 ++++++++++++++-
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index ba9dc59..d69ea16 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -792,7 +792,9 @@ static int hid_scan_report(struct hid_device *hid)
/*
* Vendor specific handlings
*/
- if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS) &&
+ if ((hid->vendor == USB_VENDOR_ID_SYNAPTICS
+ || (hid->vendor == USB_VENDOR_ID_RAZER
+ && hid->product == USB_DEVICE_ID_RAZER_BLADE_14)) &&
I don't like this. We already have a blacklist, an ignore list, and
there, we will have a new blacklist...
I understood why you put this here, the current have_special_driver
list will not fit 100%. Still, I find it not good.
It works, but I think we should still head for an entry in
have_special_driver, and a specific behavior in hid-rmi which would
rely on hid-input to handle the keyboard/mouse buttons and the rest.
Are you suggesting that we have hid-rmi bind to all of the hid devices
on this USB device and then have hid-rmi decide what reports it should
process and let the remaining events continue to hid-input? I guess once
hid-rmi loads it could look at the hid report ids and determine if it is
one of our devices and if it should put the device into rmi mode. If not
simply act as a pass through.
(hid->group == HID_GROUP_GENERIC)) {
if ((parser->scan_flags & HID_SCAN_FLAG_VENDOR_SPECIFIC)
&& (parser->scan_flags &
HID_SCAN_FLAG_GENDESK_POINTER))
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 25cd674..c677aad 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -751,6 +751,9 @@
#define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3001 0x3001
#define USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3008 0x3008
+#define USB_VENDOR_ID_RAZER 0x1532
+#define USB_DEVICE_ID_RAZER_BLADE_14 0x011D
+
#define USB_VENDOR_ID_REALTEK 0x0bda
#define USB_DEVICE_ID_REALTEK_READER 0x0152
diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index 3cccff7..1f131df 100644
--- a/drivers/hid/hid-rmi.c
+++ b/drivers/hid/hid-rmi.c
@@ -453,7 +453,15 @@ static int rmi_raw_event(struct hid_device *hdev,
case RMI_ATTN_REPORT_ID:
return rmi_input_event(hdev, data, size);
case RMI_MOUSE_REPORT_ID:
- rmi_schedule_reset(hdev);
+ /*
+ * touchpads with physical mouse buttons will report those
+ * buttons in mouse reports even in RMI mode. Only reset
+ * the device if we see reports which contain X or Y data.
+ */
+ if (data[2] != 0 || data[3] != 0)
this seems a little bit magical. There may not be an other solution,
but I would prefer that we look at alternatives before using this
magical numbers (will the touchpad always use the same report
descriptor on the mouse interface?)
I did just look at the reports and see where X and Y where reported.
Since mouse mode is really only there for compatibility when no custom
driver is present I think it is unlikely that it will change in the
future. However, there is no guarantee that it won't change. I will see
if I can come up with is less of a hack. Maybe, I can use the event
callback instead of raw_event.
+ rmi_schedule_reset(hdev);
+ else
+ return 1;
break;
}
@@ -871,6 +879,11 @@ static int rmi_input_mapping(struct hid_device
*hdev,
struct hid_input *hi, struct hid_field *field,
struct hid_usage *usage, unsigned long **bit, int *max)
{
+ if (field->application == HID_GD_POINTER
+ && (usage->hid & HID_USAGE_PAGE) == HID_UP_BUTTON)
+ /* Pass mouse button reports to generic code for
processing */
+ return 0;
+
/* we want to make HID ignore the advertised HID collection */
return -1;
}
Cheers,
Benjamin
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/