linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] HID: asus: Add i2c touchpad support
@ 2016-11-29  7:59 Brendan McGrath
  2016-11-29  9:13 ` Benjamin Tissoires
  0 siblings, 1 reply; 6+ messages in thread
From: Brendan McGrath @ 2016-11-29  7:59 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, linux-input,
	linux-kernel
  Cc: Brendan McGrath, Victor Vlasenko, Frederik Wenigwieser

Update the hid-asus module to add multitouch support for the Asus i2c touchpad.

Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
Signed-off-by: Victor Vlasenko <victor.vlasenko@sysgears.com>
Signed-off-by: Frederik Wenigwieser <frederik.wenigwieser@gmail.com>
---
This patch aims to resolve the issue raised here:
https://bugzilla.kernel.org/show_bug.cgi?id=120181

The issue is in relation to an Asus touchpad device which currently does not have
multitouch support.

The device currently falls through to the hid-generic driver which
treats the device as a mouse.

This patch aims to add the multitouch support.

A DKMS driver has been developed to test this and can found here:
https://github.com/vlasenko/hid-asus-dkms

GitHub statistics show that this repository has had over 100 unique cloners.

We have also been able to resolve over 20 raised issues with each user confirming
the driver is working well for them. There are currently no outstanding issues
directly related to this driver.


 drivers/hid/Kconfig    |   2 +-
 drivers/hid/hid-asus.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/hid/hid-core.c |   1 +
 drivers/hid/hid-ids.h  |   1 +
 4 files changed, 296 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 3156517..9abd726 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -138,7 +138,7 @@ config HID_ASUS
 	tristate "Asus"
 	depends on I2C_HID
 	---help---
-	Support for Asus notebook built-in keyboard via i2c.
+	Support for Asus notebook built-in keyboard and touchpad via i2c.
 
 	Supported devices:
 	- EeeBook X205TA
diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 7a811ec..96179b2 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -11,6 +11,12 @@
  *  This module based on hid-ortek by
  *  Copyright (c) 2010 Johnathon Harris <jmharris@gmail.com>
  *  Copyright (c) 2011 Jiri Kosina
+ *
+ *  This module has been updated to add support for Asus i2c touchpad.
+ *
+ *  Copyright (c) 2016 Brendan McGrath <redmcg@redmandi.dyndns.org>
+ *  Copyright (c) 2016 Victor Vlasenko <victor.vlasenko@sysgears.com>
+ *  Copyright (c) 2016 Frederik Wenigwieser <frederik.wenigwieser@gmail.com>
  */
 
 /*
@@ -20,16 +26,287 @@
  * any later version.
  */
 
-#include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/module.h>
+#include <linux/input/mt.h>
 
 #include "hid-ids.h"
 
+MODULE_AUTHOR("Yusuke Fujimaki <usk.fujimaki@gmail.com>");
+MODULE_AUTHOR("Brendan McGrath <redmcg@redmandi.dyndns.org>");
+MODULE_AUTHOR("Victor Vlasenko <victor.vlasenko@sysgears.com>");
+MODULE_AUTHOR("Frederik Wenigwieser <frederik.wenigwieser@gmail.com>");
+MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
+
+#define FEATURE_REPORT_ID 0x0d
+#define INPUT_REPORT_ID 0x5d
+
+#define INPUT_REPORT_SIZE 28
+
+#define MAX_CONTACTS 5
+
+#define MAX_X 2794
+#define MAX_Y 1758
+#define MAX_TOUCH_MAJOR 8
+#define MAX_PRESSURE 128
+
+#define CONTACT_DATA_SIZE 5
+
+#define BTN_LEFT_MASK 0x01
+#define CONTACT_TOOL_TYPE_MASK 0x80
+#define CONTACT_X_MSB_MASK 0xf0
+#define CONTACT_Y_MSB_MASK 0x0f
+#define CONTACT_TOUCH_MAJOR_MASK 0x07
+#define CONTACT_PRESSURE_MASK 0x7f
+
+#define QUIRK_FIX_NOTEBOOK_REPORT	BIT(0)
+#define QUIRK_NO_INIT_REPORTS		BIT(1)
+#define QUIRK_SKIP_INPUT_MAPPING	BIT(2)
+#define QUIRK_IS_MULTITOUCH		BIT(3)
+
+#define NOTEBOOK_QUIRKS			QUIRK_FIX_NOTEBOOK_REPORT
+#define TOUCHPAD_QUIRKS			(QUIRK_NO_INIT_REPORTS | \
+						 QUIRK_SKIP_INPUT_MAPPING | \
+						 QUIRK_IS_MULTITOUCH)
+
+#define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
+
+struct asus_drvdata {
+	unsigned long quirks;
+	struct input_dev *input;
+};
+
+static void asus_report_contact_down(struct input_dev *input,
+		int toolType, u8 *data)
+{
+	int touch_major, pressure;
+	int x = (data[0] & CONTACT_X_MSB_MASK) << 4 | data[1];
+	int y = MAX_Y - ((data[0] & CONTACT_Y_MSB_MASK) << 8 | data[2]);
+
+	if (toolType == MT_TOOL_PALM) {
+		touch_major = MAX_TOUCH_MAJOR;
+		pressure = MAX_PRESSURE;
+	} else {
+		touch_major = (data[3] >> 4) & CONTACT_TOUCH_MAJOR_MASK;
+		pressure = data[4] & CONTACT_PRESSURE_MASK;
+	}
+
+	input_report_abs(input, ABS_MT_POSITION_X, x);
+	input_report_abs(input, ABS_MT_POSITION_Y, y);
+	input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major);
+	input_report_abs(input, ABS_MT_PRESSURE, pressure);
+}
+
+/* Required for Synaptics Palm Detection */
+static void asus_report_tool_width(struct input_dev *input)
+{
+	struct input_mt *mt = input->mt;
+	struct input_mt_slot *oldest;
+	int oldid, count, i;
+
+	oldest = NULL;
+	oldid = mt->trkid;
+	count = 0;
+
+	for (i = 0; i < mt->num_slots; ++i) {
+		struct input_mt_slot *ps = &mt->slots[i];
+		int id = input_mt_get_value(ps, ABS_MT_TRACKING_ID);
+
+		if (id < 0)
+			continue;
+		if ((id - oldid) & TRKID_SGN) {
+			oldest = ps;
+			oldid = id;
+		}
+		count++;
+	}
+
+	if (oldest) {
+		input_report_abs(input, ABS_TOOL_WIDTH,
+			input_mt_get_value(oldest, ABS_MT_TOUCH_MAJOR));
+	}
+}
+
+static void asus_report_input(struct input_dev *input, u8 *data)
+{
+	int i;
+	u8 *contactData = data + 2;
+
+	for (i = 0; i < MAX_CONTACTS; i++) {
+		bool down = !!(data[1] & BIT(i+3));
+		int toolType = contactData[3] & CONTACT_TOOL_TYPE_MASK ?
+						MT_TOOL_PALM : MT_TOOL_FINGER;
+
+		input_mt_slot(input, i);
+		input_mt_report_slot_state(input, toolType, down);
+
+		if (down) {
+			asus_report_contact_down(input, toolType, contactData);
+			contactData += CONTACT_DATA_SIZE;
+		}
+	}
+
+	input_report_key(input, BTN_LEFT, data[1] & BTN_LEFT_MASK);
+	asus_report_tool_width(input);
+
+	input_mt_sync_frame(input);
+	input_sync(input);
+}
+
+static int asus_raw_event(struct hid_device *hdev,
+		struct hid_report *report, u8 *data, int size)
+{
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+
+	if (drvdata->quirks & QUIRK_IS_MULTITOUCH &&
+					 data[0] == INPUT_REPORT_ID &&
+						size == INPUT_REPORT_SIZE) {
+		asus_report_input(drvdata->input, data);
+		return 1;
+	}
+
+	return 0;
+}
+
+static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
+{
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+
+	if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
+		int ret;
+		struct input_dev *input = hi->input;
+
+		input_set_abs_params(input, ABS_MT_POSITION_X, 0, MAX_X, 0, 0);
+		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, MAX_Y, 0, 0);
+		input_set_abs_params(input, ABS_TOOL_WIDTH, 0, MAX_TOUCH_MAJOR, 0, 0);
+		input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, MAX_TOUCH_MAJOR, 0, 0);
+		input_set_abs_params(input, ABS_MT_PRESSURE, 0, MAX_PRESSURE, 0, 0);
+
+		__set_bit(BTN_LEFT, input->keybit);
+		__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
+
+		ret = input_mt_init_slots(input, MAX_CONTACTS, INPUT_MT_POINTER);
+
+		if (ret) {
+			hid_err(hdev, "Asus input mt init slots failed: %d\n", ret);
+			return ret;
+		}
+
+		drvdata->input = input;
+	}
+
+	return 0;
+}
+
+static int asus_input_mapping(struct hid_device *hdev,
+		struct hid_input *hi, struct hid_field *field,
+		struct hid_usage *usage, unsigned long **bit,
+		int *max)
+{
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+
+	if (drvdata->quirks & QUIRK_SKIP_INPUT_MAPPING) {
+		/* Don't map anything from the HID report.
+		 * We do it all manually in asus_input_configured
+		 */
+		return -1;
+	}
+
+	return 0;
+}
+
+static int asus_start_multitouch(struct hid_device *hdev)
+{
+	int ret;
+	const unsigned char buf[] = { FEATURE_REPORT_ID, 0x00, 0x03, 0x01, 0x00 };
+	unsigned char *dmabuf = kmemdup(buf, sizeof(buf), GFP_KERNEL);
+
+	if (!dmabuf) {
+		ret = -ENOMEM;
+		hid_err(hdev, "Asus failed to alloc dma buf: %d\n", ret);
+		return ret;
+	}
+
+	ret = hid_hw_raw_request(hdev, dmabuf[0], dmabuf, sizeof(buf),
+					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
+
+	kfree(dmabuf);
+
+	if (ret != sizeof(buf)) {
+		hid_err(hdev, "Asus failed to start multitouch: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
+{
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+
+	if (drvdata->quirks & QUIRK_IS_MULTITOUCH)
+		return asus_start_multitouch(hdev);
+
+	return 0;
+}
+
+static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+	int ret;
+	struct asus_drvdata *drvdata;
+
+	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
+	if (drvdata == NULL) {
+		hid_err(hdev, "Can't alloc Asus descriptor\n");
+		return -ENOMEM;
+	}
+
+	hid_set_drvdata(hdev, drvdata);
+
+	drvdata->quirks = id->driver_data;
+
+	if (drvdata->quirks & QUIRK_NO_INIT_REPORTS)
+		hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "Asus hid parse failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+	if (ret) {
+		hid_err(hdev, "Asus hw start failed: %d\n", ret);
+		return ret;
+	}
+
+	if (!drvdata->input) {
+		hid_err(hdev, "Asus input not registered\n");
+		ret = -ENOMEM;
+		goto err_stop_hw;
+	}
+
+	drvdata->input->name = "Asus TouchPad";
+
+	if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
+		ret = asus_start_multitouch(hdev);
+		if (ret)
+			goto err_stop_hw;
+	}
+
+	return 0;
+err_stop_hw:
+	hid_hw_stop(hdev);
+	return ret;
+}
+
 static __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
-	if (*rsize >= 56 && rdesc[54] == 0x25 && rdesc[55] == 0x65) {
+	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
+
+	if (drvdata->quirks & QUIRK_FIX_NOTEBOOK_REPORT &&
+			*rsize >= 56 && rdesc[54] == 0x25 && rdesc[55] == 0x65) {
 		hid_info(hdev, "Fixing up Asus notebook report descriptor\n");
 		rdesc[55] = 0xdd;
 	}
@@ -37,15 +314,25 @@ static __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 }
 
 static const struct hid_device_id asus_devices[] = {
-	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD) },
+	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
+		 USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD), NOTEBOOK_QUIRKS},
+	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
+			 USB_DEVICE_ID_ASUSTEK_TOUCHPAD), TOUCHPAD_QUIRKS },
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, asus_devices);
 
 static struct hid_driver asus_driver = {
-	.name = "asus",
-	.id_table = asus_devices,
-	.report_fixup = asus_report_fixup
+	.name			= "hid-asus",
+	.id_table		= asus_devices,
+	.report_fixup		= asus_report_fixup,
+	.probe                  = asus_probe,
+	.input_mapping          = asus_input_mapping,
+	.input_configured       = asus_input_configured,
+#ifdef CONFIG_PM
+	.reset_resume           = asus_reset_resume,
+#endif
+	.raw_event		= asus_raw_event
 };
 module_hid_driver(asus_driver);
 
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b43df2d..5d1f61d 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1858,6 +1858,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
 	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD) },
+	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_TOUCHPAD) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_AUREAL, USB_DEVICE_ID_AUREAL_W01RN) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_BETOP_2185BFM, 0x2208) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 222b966..9806587 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -171,6 +171,7 @@
 #define USB_DEVICE_ID_ASUSTEK_LCM	0x1726
 #define USB_DEVICE_ID_ASUSTEK_LCM2	0x175b
 #define USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD	0x8585
+#define USB_DEVICE_ID_ASUSTEK_TOUCHPAD	0x0101
 
 #define USB_VENDOR_ID_ATEN		0x0557
 #define USB_DEVICE_ID_ATEN_UC100KM	0x2004
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] HID: asus: Add i2c touchpad support
  2016-11-29  7:59 [PATCH] HID: asus: Add i2c touchpad support Brendan McGrath
@ 2016-11-29  9:13 ` Benjamin Tissoires
  2016-11-29 15:15   ` Jiri Kosina
  2016-12-10 10:20   ` [PATCH] HID: asus: Fix keyboard support Brendan McGrath
  0 siblings, 2 replies; 6+ messages in thread
From: Benjamin Tissoires @ 2016-11-29  9:13 UTC (permalink / raw)
  To: Brendan McGrath
  Cc: Jiri Kosina, Henrik Rydberg, linux-input, linux-kernel,
	Victor Vlasenko, Frederik Wenigwieser

On Nov 29 2016 or thereabouts, Brendan McGrath wrote:
> Update the hid-asus module to add multitouch support for the Asus i2c touchpad.
> 
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> Signed-off-by: Victor Vlasenko <victor.vlasenko@sysgears.com>
> Signed-off-by: Frederik Wenigwieser <frederik.wenigwieser@gmail.com>
> ---
> This patch aims to resolve the issue raised here:
> https://bugzilla.kernel.org/show_bug.cgi?id=120181
> 
> The issue is in relation to an Asus touchpad device which currently does not have
> multitouch support.
> 
> The device currently falls through to the hid-generic driver which
> treats the device as a mouse.
> 
> This patch aims to add the multitouch support.

The comments above should be in the commit message.

Besides that and only one nitpick below, I don't have much to add.
I have helped a little bit the development on github which explains why
I can't find much to say ( I already complained :-P ).

There is at least an other device that would require to be added later,
which is the bluetooth cover of some Asus tablet IIRC. The protocol is
slightly different and we will need to keep both the keyboard and the
touchpad around, but it could be added later in an other patch.

For me, the patch is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

> 
> A DKMS driver has been developed to test this and can found here:
> https://github.com/vlasenko/hid-asus-dkms
> 
> GitHub statistics show that this repository has had over 100 unique cloners.
> 
> We have also been able to resolve over 20 raised issues with each user confirming
> the driver is working well for them. There are currently no outstanding issues
> directly related to this driver.
> 
> 
>  drivers/hid/Kconfig    |   2 +-
>  drivers/hid/hid-asus.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/hid/hid-core.c |   1 +
>  drivers/hid/hid-ids.h  |   1 +
>  4 files changed, 296 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 3156517..9abd726 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -138,7 +138,7 @@ config HID_ASUS
>  	tristate "Asus"
>  	depends on I2C_HID
>  	---help---
> -	Support for Asus notebook built-in keyboard via i2c.
> +	Support for Asus notebook built-in keyboard and touchpad via i2c.
>  
>  	Supported devices:
>  	- EeeBook X205TA
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 7a811ec..96179b2 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -11,6 +11,12 @@
>   *  This module based on hid-ortek by
>   *  Copyright (c) 2010 Johnathon Harris <jmharris@gmail.com>
>   *  Copyright (c) 2011 Jiri Kosina
> + *
> + *  This module has been updated to add support for Asus i2c touchpad.
> + *
> + *  Copyright (c) 2016 Brendan McGrath <redmcg@redmandi.dyndns.org>
> + *  Copyright (c) 2016 Victor Vlasenko <victor.vlasenko@sysgears.com>
> + *  Copyright (c) 2016 Frederik Wenigwieser <frederik.wenigwieser@gmail.com>
>   */
>  
>  /*
> @@ -20,16 +26,287 @@
>   * any later version.
>   */
>  
> -#include <linux/device.h>
>  #include <linux/hid.h>
>  #include <linux/module.h>
> +#include <linux/input/mt.h>
>  
>  #include "hid-ids.h"
>  
> +MODULE_AUTHOR("Yusuke Fujimaki <usk.fujimaki@gmail.com>");
> +MODULE_AUTHOR("Brendan McGrath <redmcg@redmandi.dyndns.org>");
> +MODULE_AUTHOR("Victor Vlasenko <victor.vlasenko@sysgears.com>");
> +MODULE_AUTHOR("Frederik Wenigwieser <frederik.wenigwieser@gmail.com>");
> +MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> +
> +#define FEATURE_REPORT_ID 0x0d
> +#define INPUT_REPORT_ID 0x5d
> +
> +#define INPUT_REPORT_SIZE 28
> +
> +#define MAX_CONTACTS 5
> +
> +#define MAX_X 2794
> +#define MAX_Y 1758
> +#define MAX_TOUCH_MAJOR 8
> +#define MAX_PRESSURE 128
> +
> +#define CONTACT_DATA_SIZE 5
> +
> +#define BTN_LEFT_MASK 0x01
> +#define CONTACT_TOOL_TYPE_MASK 0x80
> +#define CONTACT_X_MSB_MASK 0xf0
> +#define CONTACT_Y_MSB_MASK 0x0f
> +#define CONTACT_TOUCH_MAJOR_MASK 0x07
> +#define CONTACT_PRESSURE_MASK 0x7f
> +
> +#define QUIRK_FIX_NOTEBOOK_REPORT	BIT(0)
> +#define QUIRK_NO_INIT_REPORTS		BIT(1)
> +#define QUIRK_SKIP_INPUT_MAPPING	BIT(2)
> +#define QUIRK_IS_MULTITOUCH		BIT(3)
> +
> +#define NOTEBOOK_QUIRKS			QUIRK_FIX_NOTEBOOK_REPORT
> +#define TOUCHPAD_QUIRKS			(QUIRK_NO_INIT_REPORTS | \
> +						 QUIRK_SKIP_INPUT_MAPPING | \
> +						 QUIRK_IS_MULTITOUCH)
> +
> +#define TRKID_SGN       ((TRKID_MAX + 1) >> 1)
> +
> +struct asus_drvdata {
> +	unsigned long quirks;
> +	struct input_dev *input;
> +};
> +
> +static void asus_report_contact_down(struct input_dev *input,
> +		int toolType, u8 *data)
> +{
> +	int touch_major, pressure;
> +	int x = (data[0] & CONTACT_X_MSB_MASK) << 4 | data[1];
> +	int y = MAX_Y - ((data[0] & CONTACT_Y_MSB_MASK) << 8 | data[2]);
> +
> +	if (toolType == MT_TOOL_PALM) {
> +		touch_major = MAX_TOUCH_MAJOR;
> +		pressure = MAX_PRESSURE;
> +	} else {
> +		touch_major = (data[3] >> 4) & CONTACT_TOUCH_MAJOR_MASK;
> +		pressure = data[4] & CONTACT_PRESSURE_MASK;
> +	}
> +
> +	input_report_abs(input, ABS_MT_POSITION_X, x);
> +	input_report_abs(input, ABS_MT_POSITION_Y, y);
> +	input_report_abs(input, ABS_MT_TOUCH_MAJOR, touch_major);
> +	input_report_abs(input, ABS_MT_PRESSURE, pressure);
> +}
> +
> +/* Required for Synaptics Palm Detection */
> +static void asus_report_tool_width(struct input_dev *input)
> +{
> +	struct input_mt *mt = input->mt;
> +	struct input_mt_slot *oldest;
> +	int oldid, count, i;
> +
> +	oldest = NULL;
> +	oldid = mt->trkid;
> +	count = 0;
> +
> +	for (i = 0; i < mt->num_slots; ++i) {
> +		struct input_mt_slot *ps = &mt->slots[i];
> +		int id = input_mt_get_value(ps, ABS_MT_TRACKING_ID);
> +
> +		if (id < 0)
> +			continue;
> +		if ((id - oldid) & TRKID_SGN) {
> +			oldest = ps;
> +			oldid = id;
> +		}
> +		count++;
> +	}
> +
> +	if (oldest) {
> +		input_report_abs(input, ABS_TOOL_WIDTH,
> +			input_mt_get_value(oldest, ABS_MT_TOUCH_MAJOR));
> +	}
> +}
> +
> +static void asus_report_input(struct input_dev *input, u8 *data)
> +{
> +	int i;
> +	u8 *contactData = data + 2;
> +
> +	for (i = 0; i < MAX_CONTACTS; i++) {
> +		bool down = !!(data[1] & BIT(i+3));
> +		int toolType = contactData[3] & CONTACT_TOOL_TYPE_MASK ?
> +						MT_TOOL_PALM : MT_TOOL_FINGER;
> +
> +		input_mt_slot(input, i);
> +		input_mt_report_slot_state(input, toolType, down);
> +
> +		if (down) {
> +			asus_report_contact_down(input, toolType, contactData);
> +			contactData += CONTACT_DATA_SIZE;
> +		}
> +	}
> +
> +	input_report_key(input, BTN_LEFT, data[1] & BTN_LEFT_MASK);
> +	asus_report_tool_width(input);
> +
> +	input_mt_sync_frame(input);
> +	input_sync(input);
> +}
> +
> +static int asus_raw_event(struct hid_device *hdev,
> +		struct hid_report *report, u8 *data, int size)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (drvdata->quirks & QUIRK_IS_MULTITOUCH &&
> +					 data[0] == INPUT_REPORT_ID &&
> +						size == INPUT_REPORT_SIZE) {
> +		asus_report_input(drvdata->input, data);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
> +		int ret;
> +		struct input_dev *input = hi->input;
> +
> +		input_set_abs_params(input, ABS_MT_POSITION_X, 0, MAX_X, 0, 0);
> +		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, MAX_Y, 0, 0);
> +		input_set_abs_params(input, ABS_TOOL_WIDTH, 0, MAX_TOUCH_MAJOR, 0, 0);
> +		input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, MAX_TOUCH_MAJOR, 0, 0);
> +		input_set_abs_params(input, ABS_MT_PRESSURE, 0, MAX_PRESSURE, 0, 0);
> +
> +		__set_bit(BTN_LEFT, input->keybit);
> +		__set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
> +
> +		ret = input_mt_init_slots(input, MAX_CONTACTS, INPUT_MT_POINTER);
> +
> +		if (ret) {
> +			hid_err(hdev, "Asus input mt init slots failed: %d\n", ret);
> +			return ret;
> +		}
> +
> +		drvdata->input = input;
> +	}
> +
> +	return 0;
> +}
> +
> +static int asus_input_mapping(struct hid_device *hdev,
> +		struct hid_input *hi, struct hid_field *field,
> +		struct hid_usage *usage, unsigned long **bit,
> +		int *max)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (drvdata->quirks & QUIRK_SKIP_INPUT_MAPPING) {
> +		/* Don't map anything from the HID report.
> +		 * We do it all manually in asus_input_configured
> +		 */
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int asus_start_multitouch(struct hid_device *hdev)
> +{
> +	int ret;
> +	const unsigned char buf[] = { FEATURE_REPORT_ID, 0x00, 0x03, 0x01, 0x00 };
> +	unsigned char *dmabuf = kmemdup(buf, sizeof(buf), GFP_KERNEL);
> +
> +	if (!dmabuf) {
> +		ret = -ENOMEM;
> +		hid_err(hdev, "Asus failed to alloc dma buf: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = hid_hw_raw_request(hdev, dmabuf[0], dmabuf, sizeof(buf),
> +					HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
> +
> +	kfree(dmabuf);
> +
> +	if (ret != sizeof(buf)) {
> +		hid_err(hdev, "Asus failed to start multitouch: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
> +{
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (drvdata->quirks & QUIRK_IS_MULTITOUCH)
> +		return asus_start_multitouch(hdev);
> +
> +	return 0;
> +}
> +
> +static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> +	int ret;
> +	struct asus_drvdata *drvdata;
> +
> +	drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (drvdata == NULL) {
> +		hid_err(hdev, "Can't alloc Asus descriptor\n");
> +		return -ENOMEM;
> +	}
> +
> +	hid_set_drvdata(hdev, drvdata);
> +
> +	drvdata->quirks = id->driver_data;
> +
> +	if (drvdata->quirks & QUIRK_NO_INIT_REPORTS)
> +		hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
> +
> +	ret = hid_parse(hdev);
> +	if (ret) {
> +		hid_err(hdev, "Asus hid parse failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> +	if (ret) {
> +		hid_err(hdev, "Asus hw start failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!drvdata->input) {
> +		hid_err(hdev, "Asus input not registered\n");
> +		ret = -ENOMEM;
> +		goto err_stop_hw;
> +	}
> +
> +	drvdata->input->name = "Asus TouchPad";
> +
> +	if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
> +		ret = asus_start_multitouch(hdev);
> +		if (ret)
> +			goto err_stop_hw;
> +	}
> +
> +	return 0;
> +err_stop_hw:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> +
>  static __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>  		unsigned int *rsize)
>  {
> -	if (*rsize >= 56 && rdesc[54] == 0x25 && rdesc[55] == 0x65) {
> +	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> +
> +	if (drvdata->quirks & QUIRK_FIX_NOTEBOOK_REPORT &&
> +			*rsize >= 56 && rdesc[54] == 0x25 && rdesc[55] == 0x65) {
>  		hid_info(hdev, "Fixing up Asus notebook report descriptor\n");
>  		rdesc[55] = 0xdd;
>  	}
> @@ -37,15 +314,25 @@ static __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>  }
>  
>  static const struct hid_device_id asus_devices[] = {
> -	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD) },
> +	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
> +		 USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD), NOTEBOOK_QUIRKS},
> +	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
> +			 USB_DEVICE_ID_ASUSTEK_TOUCHPAD), TOUCHPAD_QUIRKS },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(hid, asus_devices);
>  
>  static struct hid_driver asus_driver = {
> -	.name = "asus",
> -	.id_table = asus_devices,
> -	.report_fixup = asus_report_fixup
> +	.name			= "hid-asus",

Not sure there is a need to change the name.

> +	.id_table		= asus_devices,
> +	.report_fixup		= asus_report_fixup,
> +	.probe                  = asus_probe,
> +	.input_mapping          = asus_input_mapping,
> +	.input_configured       = asus_input_configured,
> +#ifdef CONFIG_PM
> +	.reset_resume           = asus_reset_resume,
> +#endif
> +	.raw_event		= asus_raw_event
>  };
>  module_hid_driver(asus_driver);
>  
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index b43df2d..5d1f61d 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1858,6 +1858,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
>  	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD) },
> +	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_TOUCHPAD) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_AUREAL, USB_DEVICE_ID_AUREAL_W01RN) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_BELKIN, USB_DEVICE_ID_FLIP_KVM) },
>  	{ HID_USB_DEVICE(USB_VENDOR_ID_BETOP_2185BFM, 0x2208) },
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 222b966..9806587 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -171,6 +171,7 @@
>  #define USB_DEVICE_ID_ASUSTEK_LCM	0x1726
>  #define USB_DEVICE_ID_ASUSTEK_LCM2	0x175b
>  #define USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD	0x8585
> +#define USB_DEVICE_ID_ASUSTEK_TOUCHPAD	0x0101
>  
>  #define USB_VENDOR_ID_ATEN		0x0557
>  #define USB_DEVICE_ID_ATEN_UC100KM	0x2004
> -- 
> 2.7.4
> 

Cheers,
Benjamin

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] HID: asus: Add i2c touchpad support
  2016-11-29  9:13 ` Benjamin Tissoires
@ 2016-11-29 15:15   ` Jiri Kosina
  2016-12-10 10:20   ` [PATCH] HID: asus: Fix keyboard support Brendan McGrath
  1 sibling, 0 replies; 6+ messages in thread
From: Jiri Kosina @ 2016-11-29 15:15 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Brendan McGrath, Henrik Rydberg, linux-input, linux-kernel,
	Victor Vlasenko, Frederik Wenigwieser

On Tue, 29 Nov 2016, Benjamin Tissoires wrote:

> On Nov 29 2016 or thereabouts, Brendan McGrath wrote:
> > Update the hid-asus module to add multitouch support for the Asus i2c touchpad.
> > 
> > Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> > Signed-off-by: Victor Vlasenko <victor.vlasenko@sysgears.com>
> > Signed-off-by: Frederik Wenigwieser <frederik.wenigwieser@gmail.com>
> > ---
> > This patch aims to resolve the issue raised here:
> > https://bugzilla.kernel.org/show_bug.cgi?id=120181
> > 
> > The issue is in relation to an Asus touchpad device which currently does not have
> > multitouch support.
> > 
> > The device currently falls through to the hid-generic driver which
> > treats the device as a mouse.
> > 
> > This patch aims to add the multitouch support.
> 
> The comments above should be in the commit message.

I've fixed that up.

> Besides that and only one nitpick below, I don't have much to add.
> I have helped a little bit the development on github which explains why
> I can't find much to say ( I already complained :-P ).
> 
> There is at least an other device that would require to be added later,
> which is the bluetooth cover of some Asus tablet IIRC. The protocol is
> slightly different and we will need to keep both the keyboard and the
> touchpad around, but it could be added later in an other patch.
> 
> For me, the patch is:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Thanks!

[ ... snip ... ]
> >  static struct hid_driver asus_driver = {
> > -	.name = "asus",
> > -	.id_table = asus_devices,
> > -	.report_fixup = asus_report_fixup
> > +	.name			= "hid-asus",
> 
> Not sure there is a need to change the name.

Good point, that's not for any good.

I've dropped this change, ammended the changelog (also indicating the 
changes I've done on top of the patch), and applied to for-4.10/asus.

Thanks,

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] HID: asus: Fix keyboard support
  2016-11-29  9:13 ` Benjamin Tissoires
  2016-11-29 15:15   ` Jiri Kosina
@ 2016-12-10 10:20   ` Brendan McGrath
  2016-12-10 21:17     ` Benjamin Tissoires
  1 sibling, 1 reply; 6+ messages in thread
From: Brendan McGrath @ 2016-12-10 10:20 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Henrik Rydberg, linux-input,
	linux-kernel
  Cc: Victor Vlasenko, Brendan McGrath

The previous submission which added Touchpad support broke the
Keyboard support of this driver. This patch:
1. fixes the Keyboard support (by assigning drvdata->input);
2. renames NOTEBOOK_QUIRKS to KEYBOARD_QUIRKS;
3. adds the NO_INIT_REPORT quirk to the KEYBOARD_QUIRKS; and
4. sets the input->name to 'Asus Keyboard' for the keyboard 

Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
---
 drivers/hid/hid-asus.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 96179b2..34a703c 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c
@@ -64,7 +64,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define QUIRK_SKIP_INPUT_MAPPING	BIT(2)
 #define QUIRK_IS_MULTITOUCH		BIT(3)
 
-#define NOTEBOOK_QUIRKS			QUIRK_FIX_NOTEBOOK_REPORT
+#define KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
+						 QUIRK_NO_INIT_REPORTS)
 #define TOUCHPAD_QUIRKS			(QUIRK_NO_INIT_REPORTS | \
 						 QUIRK_SKIP_INPUT_MAPPING | \
 						 QUIRK_IS_MULTITOUCH)
@@ -170,11 +171,11 @@ static int asus_raw_event(struct hid_device *hdev,
 
 static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
 {
+	struct input_dev *input = hi->input;
 	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
 
 	if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
 		int ret;
-		struct input_dev *input = hi->input;
 
 		input_set_abs_params(input, ABS_MT_POSITION_X, 0, MAX_X, 0, 0);
 		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, MAX_Y, 0, 0);
@@ -191,10 +192,10 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
 			hid_err(hdev, "Asus input mt init slots failed: %d\n", ret);
 			return ret;
 		}
-
-		drvdata->input = input;
 	}
 
+	drvdata->input = input;
+
 	return 0;
 }
 
@@ -286,7 +287,11 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err_stop_hw;
 	}
 
-	drvdata->input->name = "Asus TouchPad";
+	if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
+		drvdata->input->name = "Asus TouchPad";
+	} else {
+		drvdata->input->name = "Asus Keyboard";
+	}
 
 	if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
 		ret = asus_start_multitouch(hdev);
@@ -315,7 +320,7 @@ static __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
 
 static const struct hid_device_id asus_devices[] = {
 	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
-		 USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD), NOTEBOOK_QUIRKS},
+		 USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD), KEYBOARD_QUIRKS},
 	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
 			 USB_DEVICE_ID_ASUSTEK_TOUCHPAD), TOUCHPAD_QUIRKS },
 	{ }
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] HID: asus: Fix keyboard support
  2016-12-10 10:20   ` [PATCH] HID: asus: Fix keyboard support Brendan McGrath
@ 2016-12-10 21:17     ` Benjamin Tissoires
  2016-12-19 10:26       ` Jiri Kosina
  0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Tissoires @ 2016-12-10 21:17 UTC (permalink / raw)
  To: Brendan McGrath
  Cc: Jiri Kosina, Henrik Rydberg, linux-input, linux-kernel,
	Victor Vlasenko

Hi Brendan,

Sorry for breaking the existing device.

I have a few minor issues with your patch, but I'll let Jiri says
whether or not you need a v2.

On Dec 10 2016 or thereabouts, Brendan McGrath wrote:
> The previous submission which added Touchpad support broke the
> Keyboard support of this driver. This patch:
> 1. fixes the Keyboard support (by assigning drvdata->input);

This is actually what was breaking your keyboard (plus item 4 which is
cosmetic). Ideally, this should be in a separate patch.

> 2. renames NOTEBOOK_QUIRKS to KEYBOARD_QUIRKS;
> 3. adds the NO_INIT_REPORT quirk to the KEYBOARD_QUIRKS; and

Points 2 and 3 are actually improvements because I can't seem to find
that previously the keyboard had this quirk. I understand it is
valuable, but it should be in a different patch IMO.

> 4. sets the input->name to 'Asus Keyboard' for the keyboard 

Cosmetic, but so important :)

Anyway, the patch in its current form is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

But it might be good to split it in 2 (it will depend on Jiri I guess).

Let's hope we won't break the touchpad this time :)

Cheers,
Benjamin

> 
> Signed-off-by: Brendan McGrath <redmcg@redmandi.dyndns.org>
> ---
>  drivers/hid/hid-asus.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 96179b2..34a703c 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -64,7 +64,8 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
>  #define QUIRK_SKIP_INPUT_MAPPING	BIT(2)
>  #define QUIRK_IS_MULTITOUCH		BIT(3)
>  
> -#define NOTEBOOK_QUIRKS			QUIRK_FIX_NOTEBOOK_REPORT
> +#define KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
> +						 QUIRK_NO_INIT_REPORTS)
>  #define TOUCHPAD_QUIRKS			(QUIRK_NO_INIT_REPORTS | \
>  						 QUIRK_SKIP_INPUT_MAPPING | \
>  						 QUIRK_IS_MULTITOUCH)
> @@ -170,11 +171,11 @@ static int asus_raw_event(struct hid_device *hdev,
>  
>  static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  {
> +	struct input_dev *input = hi->input;
>  	struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
>  
>  	if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
>  		int ret;
> -		struct input_dev *input = hi->input;
>  
>  		input_set_abs_params(input, ABS_MT_POSITION_X, 0, MAX_X, 0, 0);
>  		input_set_abs_params(input, ABS_MT_POSITION_Y, 0, MAX_Y, 0, 0);
> @@ -191,10 +192,10 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  			hid_err(hdev, "Asus input mt init slots failed: %d\n", ret);
>  			return ret;
>  		}
> -
> -		drvdata->input = input;
>  	}
>  
> +	drvdata->input = input;
> +
>  	return 0;
>  }
>  
> @@ -286,7 +287,11 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		goto err_stop_hw;
>  	}
>  
> -	drvdata->input->name = "Asus TouchPad";
> +	if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
> +		drvdata->input->name = "Asus TouchPad";
> +	} else {
> +		drvdata->input->name = "Asus Keyboard";
> +	}
>  
>  	if (drvdata->quirks & QUIRK_IS_MULTITOUCH) {
>  		ret = asus_start_multitouch(hdev);
> @@ -315,7 +320,7 @@ static __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
>  
>  static const struct hid_device_id asus_devices[] = {
>  	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
> -		 USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD), NOTEBOOK_QUIRKS},
> +		 USB_DEVICE_ID_ASUSTEK_NOTEBOOK_KEYBOARD), KEYBOARD_QUIRKS},
>  	{ HID_I2C_DEVICE(USB_VENDOR_ID_ASUSTEK,
>  			 USB_DEVICE_ID_ASUSTEK_TOUCHPAD), TOUCHPAD_QUIRKS },
>  	{ }
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] HID: asus: Fix keyboard support
  2016-12-10 21:17     ` Benjamin Tissoires
@ 2016-12-19 10:26       ` Jiri Kosina
  0 siblings, 0 replies; 6+ messages in thread
From: Jiri Kosina @ 2016-12-19 10:26 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Brendan McGrath, Henrik Rydberg, linux-input, linux-kernel,
	Victor Vlasenko

On Sat, 10 Dec 2016, Benjamin Tissoires wrote:

> Sorry for breaking the existing device.
> 
> I have a few minor issues with your patch, but I'll let Jiri says
> whether or not you need a v2.
> 
> On Dec 10 2016 or thereabouts, Brendan McGrath wrote:
> > The previous submission which added Touchpad support broke the
> > Keyboard support of this driver. This patch:
> > 1. fixes the Keyboard support (by assigning drvdata->input);
> 
> This is actually what was breaking your keyboard (plus item 4 which is
> cosmetic). Ideally, this should be in a separate patch.
> 
> > 2. renames NOTEBOOK_QUIRKS to KEYBOARD_QUIRKS;
> > 3. adds the NO_INIT_REPORT quirk to the KEYBOARD_QUIRKS; and
> 
> Points 2 and 3 are actually improvements because I can't seem to find
> that previously the keyboard had this quirk. I understand it is
> valuable, but it should be in a different patch IMO.
> 
> > 4. sets the input->name to 'Asus Keyboard' for the keyboard 
> 
> Cosmetic, but so important :)
> 
> Anyway, the patch in its current form is:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> But it might be good to split it in 2 (it will depend on Jiri I guess).

Having it separate would be slightly better next time, but I don't 
consider that really to be a show stopper. Queuing in 
for-4.10/upstream-fixes, thanks.

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-12-19 10:26 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-29  7:59 [PATCH] HID: asus: Add i2c touchpad support Brendan McGrath
2016-11-29  9:13 ` Benjamin Tissoires
2016-11-29 15:15   ` Jiri Kosina
2016-12-10 10:20   ` [PATCH] HID: asus: Fix keyboard support Brendan McGrath
2016-12-10 21:17     ` Benjamin Tissoires
2016-12-19 10:26       ` Jiri Kosina

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).