Linux Input/HID development
 help / color / mirror / Atom feed
* [PATCH 0/6] HID: sony: More Sony controller fixes and improvements.
From: Frank Praznik @ 2014-03-01  3:58 UTC (permalink / raw)
  To: linux-input; +Cc: jkosina, dh.herrmann, Frank Praznik

This set consists of one bugfix, two mostly cosmetic changes and three larger
patches for the LED subsystem.

Patch #4 adds hardware blink support to the controller LEDs.  Values from 0 to
2.5 seconds are supported by the hardware.  The Sixaxis can set all of the LEDs
individually, but the DualShock 4 only has one global setting for the entire
light bar so only the value from the most recently set LED is used.

Patch #5 adds an LED trigger that reports the controller battery status via the
registered LEDs.  The LEDs will flash if the controller is charging or if the
battery is low, and remain solid otherwise.

Patch #6 initializes the LEDs to a default value of LED 1 on the Sixaxis and
blue on the DualShock 4 so there is some indication that the controller is
powered on and connected in the case of Bluetooth.  The code can be used to set
the LEDs based on the device number, but I'm not sure how to actually retrieve
the controller number from the system.  I saw the xpad patches posted a few
weeks ago where the minor number of the joydev device was used, but I'm under
the impression that doing that is not ideal.  Any suggestions?

^ permalink raw reply

* [PATCH 3/4] HID: sony: do not rely on hid_output_raw_report
From: Benjamin Tissoires @ 2014-03-01  0:20 UTC (permalink / raw)
  To: Benjamin Tissoires, Jiri Kosina, David Herrmann, David Barksdale,
	linux-input, linux-kernel
In-Reply-To: <1393633237-26496-1-git-send-email-benjamin.tissoires@redhat.com>

hid_out_raw_report is going to be obsoleted as it is not part of the
unified HID low level transport documentation
(Documentation/hid/hid-transport.txt)

To do so, we need to introduce two new quirks:
* HID_QUIRK_NO_OUTPUT_REPORTS: this quirks prevents the transport
  driver to use the interrupt channel to send output report (and thus
  force to use HID_REQ_SET_REPORT command)
* HID_QUIRK_SKIP_OUTPUT_REPORT_ID: this one forces usbhid to not
  include the report ID in the buffer it sends to the device through
  HID_REQ_SET_REPORT in case of an output report

This also fixes a regression introduced in commit 3a75b24949a8
(HID: hidraw: replace hid_output_raw_report() calls by appropriates ones).
The hidraw API was not able to communicate with the PS3 SixAxis
controllers in USB mode.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-sony.c        | 59 ++++++++++---------------------------------
 drivers/hid/hidraw.c          |  3 ++-
 drivers/hid/usbhid/hid-core.c |  7 ++++-
 include/linux/hid.h           |  2 ++
 4 files changed, 24 insertions(+), 47 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index b5fe65e..08eac71 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1007,45 +1007,6 @@ static int sony_mapping(struct hid_device *hdev, struct hid_input *hi,
 }
 
 /*
- * The Sony Sixaxis does not handle HID Output Reports on the Interrupt EP
- * like it should according to usbhid/hid-core.c::usbhid_output_raw_report()
- * so we need to override that forcing HID Output Reports on the Control EP.
- *
- * There is also another issue about HID Output Reports via USB, the Sixaxis
- * does not want the report_id as part of the data packet, so we have to
- * discard buf[0] when sending the actual control message, even for numbered
- * reports, humpf!
- */
-static int sixaxis_usb_output_raw_report(struct hid_device *hid, __u8 *buf,
-		size_t count, unsigned char report_type)
-{
-	struct usb_interface *intf = to_usb_interface(hid->dev.parent);
-	struct usb_device *dev = interface_to_usbdev(intf);
-	struct usb_host_interface *interface = intf->cur_altsetting;
-	int report_id = buf[0];
-	int ret;
-
-	if (report_type == HID_OUTPUT_REPORT) {
-		/* Don't send the Report ID */
-		buf++;
-		count--;
-	}
-
-	ret = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
-		HID_REQ_SET_REPORT,
-		USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
-		((report_type + 1) << 8) | report_id,
-		interface->desc.bInterfaceNumber, buf, count,
-		USB_CTRL_SET_TIMEOUT);
-
-	/* Count also the Report ID, in case of an Output report. */
-	if (ret > 0 && report_type == HID_OUTPUT_REPORT)
-		ret++;
-
-	return ret;
-}
-
-/*
  * Sending HID_REQ_GET_REPORT changes the operation mode of the ps3 controller
  * to "operational".  Without this, the ps3 controller will not report any
  * events.
@@ -1305,11 +1266,8 @@ static void sixaxis_state_worker(struct work_struct *work)
 	buf[10] |= sc->led_state[2] << 3;
 	buf[10] |= sc->led_state[3] << 4;
 
-	if (sc->quirks & SIXAXIS_CONTROLLER_USB)
-		hid_output_raw_report(sc->hdev, buf, sizeof(buf), HID_OUTPUT_REPORT);
-	else
-		hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf),
-				HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
+	hid_hw_raw_request(sc->hdev, 0x01, buf, sizeof(buf), HID_OUTPUT_REPORT,
+			HID_REQ_SET_REPORT);
 }
 
 static void dualshock4_state_worker(struct work_struct *work)
@@ -1659,7 +1617,18 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	}
 
 	if (sc->quirks & SIXAXIS_CONTROLLER_USB) {
-		hdev->hid_output_raw_report = sixaxis_usb_output_raw_report;
+		/*
+		 * The Sony Sixaxis does not handle HID Output Reports on the
+		 * Interrupt EP like it could, so we need to forcing HID Output
+		 * Reports to use HID_REQ_SET_REPORT on the Control EP.
+		 *
+		 * There is also another issue about HID Output Reports via USB,
+		 * the Sixaxis does not want the report_id as part of the data
+		 * packet, so we have to discard buf[0] when sending the actual
+		 * control message, even for numbered reports, humpf!
+		 */
+		hdev->quirks |= HID_QUIRK_NO_OUTPUT_REPORTS;
+		hdev->quirks |= HID_QUIRK_SKIP_OUTPUT_REPORT_ID;
 		ret = sixaxis_set_operational_usb(hdev);
 		sc->worker_initialized = 1;
 		INIT_WORK(&sc->state_worker, sixaxis_state_worker);
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 2cc484c..6537e58 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -149,7 +149,8 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
 		goto out_free;
 	}
 
-	if (report_type == HID_OUTPUT_REPORT) {
+	if ((report_type == HID_OUTPUT_REPORT) &&
+	    !(dev->quirks & HID_QUIRK_NO_OUTPUT_REPORTS)) {
 		ret = hid_hw_output_report(dev, buf, count);
 		/*
 		 * compatibility with old implementation of USB-HID and I2C-HID:
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 0d1d875..3bc7cad 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -894,7 +894,12 @@ static int usbhid_set_raw_report(struct hid_device *hid, unsigned int reportnum,
 	int ret, skipped_report_id = 0;
 
 	/* Byte 0 is the report number. Report data starts at byte 1.*/
-	buf[0] = reportnum;
+	if ((rtype == HID_OUTPUT_REPORT) &&
+	    (hid->quirks & HID_QUIRK_SKIP_OUTPUT_REPORT_ID))
+		buf[0] = 0;
+	else
+		buf[0] = reportnum;
+
 	if (buf[0] == 0x0) {
 		/* Don't send the Report ID */
 		buf++;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 5eb282e..2baf834 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -287,6 +287,8 @@ struct hid_item {
 #define HID_QUIRK_NO_EMPTY_INPUT		0x00000100
 #define HID_QUIRK_NO_INIT_INPUT_REPORTS		0x00000200
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		0x00010000
+#define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		0x00020000
+#define HID_QUIRK_NO_OUTPUT_REPORTS		0x00040000
 #define HID_QUIRK_FULLSPEED_INTERVAL		0x10000000
 #define HID_QUIRK_NO_INIT_REPORTS		0x20000000
 #define HID_QUIRK_NO_IGNORE			0x40000000
-- 
1.8.5.3


^ permalink raw reply related

* [PATCH 2/4] HID: cp2112: remove the last hid_output_raw_report() call
From: Benjamin Tissoires @ 2014-03-01  0:20 UTC (permalink / raw)
  To: Benjamin Tissoires, Jiri Kosina, David Herrmann, David Barksdale,
	linux-input, linux-kernel
In-Reply-To: <1393633237-26496-1-git-send-email-benjamin.tissoires@redhat.com>

I don't have access to the device, so I copied/pasted the code
from hidraw.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-cp2112.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 860db694..c4f87bd 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -290,7 +290,21 @@ static int cp2112_hid_output(struct hid_device *hdev, u8 *data, size_t count,
 	if (!buf)
 		return -ENOMEM;
 
-	ret = hdev->hid_output_raw_report(hdev, buf, count, report_type);
+	/* Fixme: test which function is actually called for output reports */
+	if (report_type == HID_OUTPUT_REPORT) {
+		ret = hid_hw_output_report(hdev, buf, count);
+		/*
+		 * compatibility with old implementation of USB-HID:
+		 * if the device does not support receiving output reports,
+		 * on an interrupt endpoint, fallback to SET_REPORT HID command.
+		 */
+		if (ret != -ENOSYS)
+			goto out_free;
+	}
+
+	ret = hid_hw_raw_request(hdev, buf[0], buf, count, report_type,
+				HID_REQ_SET_REPORT);
+out_free:
 	kfree(buf);
 	return ret;
 }
-- 
1.8.5.3


^ permalink raw reply related

* [PATCH 4/4] HID: remove hid_output_raw_report transport implementations
From: Benjamin Tissoires @ 2014-03-01  0:20 UTC (permalink / raw)
  To: Benjamin Tissoires, Jiri Kosina, David Herrmann, David Barksdale,
	linux-input, linux-kernel
In-Reply-To: <1393633237-26496-1-git-send-email-benjamin.tissoires@redhat.com>

Nobody calls hid_output_raw_report anymore, and nobody should.
We can now remove the various implementation in the different
transport drivers and the declarations.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 14 --------------
 drivers/hid/uhid.c            |  1 -
 drivers/hid/usbhid/hid-core.c | 12 ------------
 include/linux/hid.h           | 19 -------------------
 net/bluetooth/hidp/core.c     | 14 --------------
 5 files changed, 60 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index cbd44a7..8c52a07 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -596,19 +596,6 @@ static int i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
 	return ret;
 }
 
-static int __i2c_hid_output_raw_report(struct hid_device *hid, __u8 *buf,
-		size_t count, unsigned char report_type)
-{
-	struct i2c_client *client = hid->driver_data;
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
-	bool data = true; /* SET_REPORT */
-
-	if (report_type == HID_OUTPUT_REPORT)
-		data = le16_to_cpu(ihid->hdesc.wMaxOutputLength) == 0;
-
-	return i2c_hid_output_raw_report(hid, buf, count, report_type, data);
-}
-
 static int i2c_hid_output_report(struct hid_device *hid, __u8 *buf,
 		size_t count)
 {
@@ -1037,7 +1024,6 @@ static int i2c_hid_probe(struct i2c_client *client,
 
 	hid->driver_data = client;
 	hid->ll_driver = &i2c_hid_ll_driver;
-	hid->hid_output_raw_report = __i2c_hid_output_raw_report;
 	hid->dev.parent = &client->dev;
 	ACPI_COMPANION_SET(&hid->dev, ACPI_COMPANION(&client->dev));
 	hid->bus = BUS_I2C;
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 60acee4..7ed79be 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -400,7 +400,6 @@ static int uhid_dev_create(struct uhid_device *uhid,
 	hid->uniq[63] = 0;
 
 	hid->ll_driver = &uhid_hid_driver;
-	hid->hid_output_raw_report = uhid_hid_output_raw;
 	hid->bus = ev->u.create.bus;
 	hid->vendor = ev->u.create.vendor;
 	hid->product = ev->u.create.product;
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 3bc7cad..7b88f4c 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -950,17 +950,6 @@ static int usbhid_output_report(struct hid_device *hid, __u8 *buf, size_t count)
 	return ret;
 }
 
-static int usbhid_output_raw_report(struct hid_device *hid, __u8 *buf,
-		size_t count, unsigned char report_type)
-{
-	struct usbhid_device *usbhid = hid->driver_data;
-
-	if (usbhid->urbout && report_type != HID_FEATURE_REPORT)
-		return usbhid_output_report(hid, buf, count);
-
-	return usbhid_set_raw_report(hid, buf[0], buf, count, report_type);
-}
-
 static void usbhid_restart_queues(struct usbhid_device *usbhid)
 {
 	if (usbhid->urbout && !test_bit(HID_OUT_RUNNING, &usbhid->iofl))
@@ -1294,7 +1283,6 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
 
 	usb_set_intfdata(intf, hid);
 	hid->ll_driver = &usb_hid_driver;
-	hid->hid_output_raw_report = usbhid_output_raw_report;
 	hid->ff_init = hid_pidff_init;
 #ifdef CONFIG_USB_HIDDEV
 	hid->hiddev_connect = hiddev_connect;
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 2baf834..622eb4a 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -510,9 +510,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;
@@ -1020,22 +1017,6 @@ static inline int hid_hw_output_report(struct hid_device *hdev, __u8 *buf,
 }
 
 /**
- * hid_output_raw_report - send an output or a feature report to the device
- *
- * @hdev: hid device
- * @buf: raw data to transfer
- * @len: length of buf
- * @report_type: HID_FEATURE_REPORT or HID_OUTPUT_REPORT
- *
- * @return: count of data transfered, negative if error
- */
-static inline int hid_output_raw_report(struct hid_device *hdev, __u8 *buf,
-					size_t len, unsigned char report_type)
-{
-	return hdev->hid_output_raw_report(hdev, buf, len, report_type);
-}
-
-/**
  * hid_hw_idle - send idle request to device
  *
  * @hdev: hid device
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index 98e4840..514ddb5 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -382,18 +382,6 @@ static int hidp_output_report(struct hid_device *hid, __u8 *data, size_t count)
 				      data, count);
 }
 
-static int hidp_output_raw_report(struct hid_device *hid, unsigned char *data,
-		size_t count, unsigned char report_type)
-{
-	if (report_type == HID_OUTPUT_REPORT) {
-		return hidp_output_report(hid, data, count);
-	} else if (report_type != HID_FEATURE_REPORT) {
-		return -EINVAL;
-	}
-
-	return hidp_set_raw_report(hid, data[0], data, count, report_type);
-}
-
 static int hidp_raw_request(struct hid_device *hid, unsigned char reportnum,
 			    __u8 *buf, size_t len, unsigned char rtype,
 			    int reqtype)
@@ -776,8 +764,6 @@ static int hidp_setup_hid(struct hidp_session *session,
 	hid->dev.parent = &session->conn->hcon->dev;
 	hid->ll_driver = &hidp_hid_driver;
 
-	hid->hid_output_raw_report = hidp_output_raw_report;
-
 	/* True if device is blacklisted in drivers/hid/hid-core.c */
 	if (hid_ignore(hid)) {
 		hid_destroy_device(session->hid);
-- 
1.8.5.3

^ permalink raw reply related

* [PATCH 1/4] HID: cp2112: remove various hid_out_raw_report calls
From: Benjamin Tissoires @ 2014-03-01  0:20 UTC (permalink / raw)
  To: Benjamin Tissoires, Jiri Kosina, David Herrmann, David Barksdale,
	linux-input, linux-kernel
In-Reply-To: <1393633237-26496-1-git-send-email-benjamin.tissoires@redhat.com>

hid_out_raw_report is going to be obsoleted as it is not part of the
unified HID low level transport documentation
(Documentation/hid/hid-transport.txt)

  hid_output_raw_report(hdev, buf, sizeof(buf), HID_FEATURE_REPORT);
is strictly equivalent to:
  hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
		HID_FEATURE_REPORT, HID_REQ_SET_REPORT);

So use the new api.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-cp2112.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 1025982..860db694 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -185,8 +185,8 @@ static int cp2112_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 	buf[1] &= ~(1 << offset);
 	buf[2] = gpio_push_pull;
 
-	ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf),
-					  HID_FEATURE_REPORT);
+	ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	if (ret < 0) {
 		hid_err(hdev, "error setting GPIO config: %d\n", ret);
 		return ret;
@@ -207,8 +207,8 @@ static void cp2112_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	buf[1] = value ? 0xff : 0;
 	buf[2] = 1 << offset;
 
-	ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf),
-					  HID_FEATURE_REPORT);
+	ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	if (ret < 0)
 		hid_err(hdev, "error setting GPIO values: %d\n", ret);
 }
@@ -253,8 +253,8 @@ static int cp2112_gpio_direction_output(struct gpio_chip *chip,
 	buf[1] |= 1 << offset;
 	buf[2] = gpio_push_pull;
 
-	ret = hdev->hid_output_raw_report(hdev, buf, sizeof(buf),
-					  HID_FEATURE_REPORT);
+	ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(buf),
+				 HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
 	if (ret < 0) {
 		hid_err(hdev, "error setting GPIO config: %d\n", ret);
 		return ret;
-- 
1.8.5.3

^ permalink raw reply related

* [PATCH 0/4] HID: ll transport cleanup: final round
From: Benjamin Tissoires @ 2014-03-01  0:20 UTC (permalink / raw)
  To: Benjamin Tissoires, Jiri Kosina, David Herrmann, David Barksdale,
	linux-input, linux-kernel

Hi guys,

by shuffling around the hid files, I noticed that I introduced a regression
in commit 3a75b24949a8 (HID: hidraw: replace hid_output_raw_report() calls...).
We removed the hid_output_raw_report() calls in hidraw, which means that
the special sixasis handling was broken from the hidraw callers (I don't know
if there are any however).

So I took some time to finish this work. I am not particularly proud of patches
2/4 and 3/4:

For the patch 2/4, we need to get some feedbacks from David Barksdale,
because the spec seems a little bit nebulous to me and I'd prefer having real
tests to confirm that it is the right choice. I hope that David B. will send
his feedbacks soon. If not, I still would like to see this in 3.15 with the
rest of the cleanup to avoid having new drivers which use the old API.

Regarding 3/4, well, I added 2 quirks which are not so trivials, but I am not
sure I could do something else.

Anyway, happy reviewing.

Cheers,
Benjamin

Benjamin Tissoires (4):
  HID: cp2112: remove various hid_out_raw_report calls
  HID: cp2112: remove the last hid_output_raw_report() call
  HID: sony: do not rely on hid_out_raw_report
  HID: remove hid_output_raw_report transport implementations

 drivers/hid/hid-cp2112.c      | 28 +++++++++++++++-----
 drivers/hid/hid-sony.c        | 59 ++++++++++---------------------------------
 drivers/hid/hidraw.c          |  3 ++-
 drivers/hid/i2c-hid/i2c-hid.c | 14 ----------
 drivers/hid/uhid.c            |  1 -
 drivers/hid/usbhid/hid-core.c | 19 +++++---------
 include/linux/hid.h           | 21 ++-------------
 net/bluetooth/hidp/core.c     | 14 ----------
 8 files changed, 45 insertions(+), 114 deletions(-)

-- 
1.8.5.3

^ permalink raw reply

* CONTACT KENYA COMMERCIAL BANK LTD IMMEDIATELY FOR YOUR PAYMENT (3MILLION UNITED STATE DOLLARS)
From: Jantima Khuntaraksa @ 2014-02-28 21:11 UTC (permalink / raw)


Attention: Please, Kindly contact KENYA COMMERCIAL BANK immediately for your compensation payment of (3MILLION UNITED STATE DOLLARS) via email: Email:(services1@kenya.ncommercialbnk.com) quote your payment Ref No:KCB/00Y/2014 For purposes of immediate payment.   
 

^ permalink raw reply

* Re: [PATCH v4 7/9] devicetree: bindings: Document PM8921/8058 keypads
From: Josh Cartwright @ 2014-02-28 19:11 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Josh Cartwright, Dmitry Torokhov, linux-kernel, linux-arm-msm,
	linux-arm-kernel, linux-input, devicetree
In-Reply-To: <20140228183708.GF2487@codeaurora.org>

On Fri, Feb 28, 2014 at 10:37:08AM -0800, Stephen Boyd wrote:
> On 02/28, Josh Cartwright wrote:
> > On Thu, Feb 27, 2014 at 05:55:18PM -0800, Stephen Boyd wrote:
> > > +
> > > +EXAMPLE
> > > +
> > > +	keypad {
> > > +		compatible = "qcom,pm8921-keypad";
> > > +		interrupt-parent = <&pmicintc>;
> > > +		interrupts = <74 1>, <75 1>;
> > > +		linux,keymap = <
> > > +			MATRIX_KEY(0, 0, KEY_VOLUMEUP)
> > > +			MATRIX_KEY(0, 1, KEY_VOLUMEDOWN)
> > > +			MATRIX_KEY(0, 2, KEY_CAMERA_FOCUS)
> > > +			MATRIX_KEY(0, 3, KEY_CAMERA)
> > > +			>;
> > > +		keypad,num-rows = <1>;
> > > +		keypad,num-columns = <5>;
> > > +		debounce = <15>;
> > > +		scan-delay = <32>;
> > > +		row-hold = <91500>;
> > > +	};
> >
> > It odd to me that these newly created bindings don't have 'reg'
> > properties, even though the device clearly has a register region.
> >
> > I suppose it makes sense from a "port over from platform data to DT"
> > perspective, as these drivers have just assumed the location of their
> > registers to be fixed; however I suspect things will need to be changed
> > if/when we hope to share these drivers with pm8841/pm8941 and beyond...
> >
>
> Agreed. I would love it if the platform OF code would create
> IORESOURCE_REG resources for any reg properties that aren't
> translatable to CPU addresses. That way we don't have to pick out the
> reg property from DT with special OF code (like you've done in
> rtc-pm8xxx).

Yes, I agree this would be nice.  The rtc-pm8xxx register parsing magic
is misplaced/ugly.  I'll see about taking a crack at this and seeing
what it looks like.

> I'll throw the reg property into the binding so that in
> the future we can support the register moving around (although at the
> moment the driver will ignore it).

Great, I think this sounds good.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

^ permalink raw reply

* Re: [PATCH v4 7/9] devicetree: bindings: Document PM8921/8058 keypads
From: Stephen Boyd @ 2014-02-28 18:37 UTC (permalink / raw)
  To: Josh Cartwright
  Cc: Dmitry Torokhov, linux-kernel, linux-arm-msm, linux-arm-kernel,
	linux-input, devicetree
In-Reply-To: <20140228135527.GF7308@joshc.qualcomm.com>

On 02/28, Josh Cartwright wrote:
> On Thu, Feb 27, 2014 at 05:55:18PM -0800, Stephen Boyd wrote:
> 
> - linux,wakeup?
> - linux,no-auto-repeat?

Added.

> 
> > +
> > +EXAMPLE
> > +
> > +	keypad {
> > +		compatible = "qcom,pm8921-keypad";
> > +		interrupt-parent = <&pmicintc>;
> > +		interrupts = <74 1>, <75 1>;
> > +		linux,keymap = <
> > +			MATRIX_KEY(0, 0, KEY_VOLUMEUP)
> > +			MATRIX_KEY(0, 1, KEY_VOLUMEDOWN)
> > +			MATRIX_KEY(0, 2, KEY_CAMERA_FOCUS)
> > +			MATRIX_KEY(0, 3, KEY_CAMERA)
> > +			>;
> > +		keypad,num-rows = <1>;
> > +		keypad,num-columns = <5>;
> > +		debounce = <15>;
> > +		scan-delay = <32>;
> > +		row-hold = <91500>;
> > +	};
> 
> It odd to me that these newly created bindings don't have 'reg'
> properties, even though the device clearly has a register region.
> 
> I suppose it makes sense from a "port over from platform data to DT"
> perspective, as these drivers have just assumed the location of their
> registers to be fixed; however I suspect things will need to be changed
> if/when we hope to share these drivers with pm8841/pm8941 and beyond...
> 

Agreed. I would love it if the platform OF code would create
IORESOURCE_REG resources for any reg properties that aren't
translatable to CPU addresses. That way we don't have to pick out
the reg property from DT with special OF code (like you've done
in rtc-pm8xxx). I'll throw the reg property into the binding so
that in the future we can support the register moving around
(although at the moment the driver will ignore it).

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

^ permalink raw reply

* Re: [PATCH v4 5/9] Input: pmic8xxx-pwrkey - Migrate to DT
From: Stephen Boyd @ 2014-02-28 18:32 UTC (permalink / raw)
  To: Josh Cartwright
  Cc: Dmitry Torokhov, linux-kernel, linux-arm-msm, linux-arm-kernel,
	linux-input
In-Reply-To: <20140228034547.GE7308@joshc.qualcomm.com>

On 02/27, Josh Cartwright wrote:
> On Thu, Feb 27, 2014 at 05:55:16PM -0800, Stephen Boyd wrote:
> > The driver is only supported on DT enabled platforms. Convert the
> > driver to DT so that it can probe properly.
> > 
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > ---
> > --- a/drivers/input/misc/pmic8xxx-pwrkey.c
> > +++ b/drivers/input/misc/pmic8xxx-pwrkey.c
> > @@ -172,7 +178,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	platform_set_drvdata(pdev, pwrkey);
> > -	device_init_wakeup(&pdev->dev, pdata->wakeup);
> > +	device_init_wakeup(&pdev->dev, 1);
> 
> Is there a particular reason you aren't providing a 'linux,wakeup'
> property for pwrkey?
> 

In all the users of this device I've never seen wakeup set to 0.
Given that this is a power key it makes sense because this is
used to wake up a phone from suspend (also see the comment above
device_init_wakeup() where power buttons are explicitly called
out).

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

^ permalink raw reply

* Re: [PATCH v4 4/9] Input: pmic8xxx-keypad - Migrate to DT
From: Stephen Boyd @ 2014-02-28 18:29 UTC (permalink / raw)
  To: Josh Cartwright
  Cc: Dmitry Torokhov, linux-kernel, linux-arm-msm, linux-arm-kernel,
	linux-input
In-Reply-To: <20140228034138.GD7308@joshc.qualcomm.com>

On 02/27, Josh Cartwright wrote:
> Looks good, with the exception of one thing...
> 
> On Thu, Feb 27, 2014 at 05:55:15PM -0800, Stephen Boyd wrote:
> > The driver is only supported on DT enabled platforms. Convert the
> > driver to DT so that it can probe properly.
> > 
> > Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> > ---
> >  drivers/input/keyboard/pmic8xxx-keypad.c | 150 ++++++++++++++++++-------------
> > @@ -471,50 +514,27 @@ static void pmic8xxx_kp_close(struct input_dev *dev)
> >   */
> >  static int pmic8xxx_kp_probe(struct platform_device *pdev)
> >  {
> [..]
> > -	keymap_data = pdata->keymap_data;
> > -	if (!keymap_data) {
> > -		dev_err(&pdev->dev, "no keymap data supplied\n");
> > -		return -EINVAL;
> > -	}
> > +	repeat = !of_property_read_bool(pdev->dev.of_node,
> > +					"linux,input-no-autorepeat");
> > +	wakeup = !of_property_read_bool(pdev->dev.of_node,
> > +					"linux,keypad-wakeup");
> 
> I don't think you mean to invert this.
> 

Good catch. Thanks.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

^ permalink raw reply

* Re: wacom: Fixes for stylus pressure values for Thinkpad Yoga
From: Carl Worth @ 2014-02-28 18:26 UTC (permalink / raw)
  To: Ping Cheng, Dmitry Torokhov, Jason Gerecke
  Cc: linux-input, linux-kernel@vger.kernel.org
In-Reply-To: <CAF8JNh+7p7-cNkBv3CB3ZVsi7JrqzPf1bdzsA4rJqKUcY=dZ0Q@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

Ping Cheng <pinglinux@gmail.com> writes:
> Thank you for the heads up. I believe Jason's patchset 4 of 4
> (http://www.spinics.net/lists/linux-input/msg29435.html) fixed the
> issue for your device and for other's. The patch was submitted last
> month. If you can test the set on your device and give us a Tested-by
> here, it will help Dmitry to merge the patch upstream.
>
> Thank you for your effort.

Thanks, Ping.

Patches 2, 3 and 4 of Dmitry's series do everything that my series does,
(and a bit more since he also fixes the "unsigned char" for cases
besides the specific one I hit).

So those all get my:

Reviewed-by: Carl Worth <cworth@cworth.org>

I'll follow up more if I get a chance to test these as well.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 818 bytes --]

^ permalink raw reply

* [PATCH 3/3] HID: multitouch: add support of other generic collections in hid-mt
From: Benjamin Tissoires @ 2014-02-28 16:41 UTC (permalink / raw)
  To: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Stephane Chatty,
	linux-input, linux-kernel
In-Reply-To: <1393605685-17025-1-git-send-email-benjamin.tissoires@redhat.com>

The ANTON Touch Pad is a device which can switch from a multitouch
touchpad to a mouse. It thus presents several generic collections which
are currently ignored by hid-multitouch. Enable them by not ignoring
them in mt_input_mapping.
Adding also a suffix for them depending on their application.

Reported-by: Edel Maks <edelmaks@gmail.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-ids.h        |  3 ++
 drivers/hid/hid-multitouch.c | 82 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/hid.h          |  3 ++
 3 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 00be0d0..7045a71 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -67,6 +67,9 @@
 #define USB_VENDOR_ID_ALPS		0x0433
 #define USB_DEVICE_ID_IBM_GAMEPAD	0x1101
 
+#define USB_VENDOR_ID_ANTON		0x1130
+#define USB_DEVICE_ID_ANTON_TOUCH_PAD	0x3101
+
 #define USB_VENDOR_ID_APPLE		0x05ac
 #define USB_DEVICE_ID_APPLE_MIGHTYMOUSE	0x0304
 #define USB_DEVICE_ID_APPLE_MAGICMOUSE	0x030d
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 8250cc0..0d31139 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -84,6 +84,7 @@ struct mt_class {
 	__s32 sn_pressure;	/* Signal/noise ratio for pressure events */
 	__u8 maxcontacts;
 	bool is_indirect;	/* true for touchpads */
+	bool export_all_inputs;	/* do not ignore mouse, keyboards, etc... */
 };
 
 struct mt_fields {
@@ -133,6 +134,7 @@ static void mt_post_parse(struct mt_device *td);
 /* reserved					0x0010 */
 /* reserved					0x0011 */
 #define MT_CLS_WIN_8				0x0012
+#define MT_CLS_EXPORT_ALL_INPUTS		0x0013
 
 /* vendor specific classes */
 #define MT_CLS_3M				0x0101
@@ -196,6 +198,10 @@ static struct mt_class mt_classes[] = {
 			MT_QUIRK_IGNORE_DUPLICATES |
 			MT_QUIRK_HOVERING |
 			MT_QUIRK_CONTACT_CNT_ACCURATE },
+	{ .name = MT_CLS_EXPORT_ALL_INPUTS,
+		.quirks = MT_QUIRK_ALWAYS_VALID |
+			MT_QUIRK_CONTACT_CNT_ACCURATE,
+		.export_all_inputs = true },
 
 	/*
 	 * vendor specific classes
@@ -718,28 +724,52 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
 		unsigned long **bit, int *max)
 {
-	/* Only map fields from TouchScreen or TouchPad collections.
-	* We need to ignore fields that belong to other collections
-	* such as Mouse that might have the same GenericDesktop usages. */
-	if (field->application != HID_DG_TOUCHSCREEN &&
+	struct mt_device *td = hid_get_drvdata(hdev);
+
+	/*
+	 * If mtclass.export_all_inputs is not set, only map fields from
+	 * TouchScreen or TouchPad collections. We need to ignore fields
+	 * that belong to other collections such as Mouse that might have
+	 * the same GenericDesktop usages.
+	 */
+	if (!td->mtclass.export_all_inputs &&
+	    field->application != HID_DG_TOUCHSCREEN &&
 	    field->application != HID_DG_PEN &&
 	    field->application != HID_DG_TOUCHPAD)
 		return -1;
 
+	/*
+	 * some egalax touchscreens have "application == HID_DG_TOUCHSCREEN"
+	 * for the stylus.
+	 */
 	if (field->physical == HID_DG_STYLUS)
 		return 0;
 
-	return mt_touch_input_mapping(hdev, hi, field, usage, bit, max);
+	if (field->application == HID_DG_TOUCHSCREEN ||
+	    field->application == HID_DG_TOUCHPAD)
+		return mt_touch_input_mapping(hdev, hi, field, usage, bit, max);
+
+	/* let hid-core decide for the others */
+	return 0;
 }
 
 static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
 		unsigned long **bit, int *max)
 {
+	/*
+	 * some egalax touchscreens have "application == HID_DG_TOUCHSCREEN"
+	 * for the stylus.
+	 */
 	if (field->physical == HID_DG_STYLUS)
 		return 0;
 
-	return mt_touch_input_mapped(hdev, hi, field, usage, bit, max);
+	if (field->application == HID_DG_TOUCHSCREEN ||
+	    field->application == HID_DG_TOUCHPAD)
+		return mt_touch_input_mapped(hdev, hi, field, usage, bit, max);
+
+	/* let hid-core decide for the others */
+	return 0;
 }
 
 static int mt_event(struct hid_device *hid, struct hid_field *field,
@@ -846,14 +876,49 @@ static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 	struct mt_device *td = hid_get_drvdata(hdev);
 	char *name;
 	const char *suffix = NULL;
+	struct hid_field *field = hi->report->field[0];
 
 	if (hi->report->id == td->mt_report_id)
 		mt_touch_input_configured(hdev, hi);
 
+	/*
+	 * some egalax touchscreens have "application == HID_DG_TOUCHSCREEN"
+	 * for the stylus. Check this first, and then rely on the application
+	 * field.
+	 */
 	if (hi->report->field[0]->physical == HID_DG_STYLUS) {
 		suffix = "Pen";
 		/* force BTN_STYLUS to allow tablet matching in udev */
 		__set_bit(BTN_STYLUS, hi->input->keybit);
+	} else {
+		switch (field->application) {
+		case HID_GD_KEYBOARD:
+			suffix = "Keyboard";
+			break;
+		case HID_GD_KEYPAD:
+			suffix = "Keypad";
+			break;
+		case HID_GD_MOUSE:
+			suffix = "Mouse";
+			break;
+		case HID_DG_STYLUS:
+			suffix = "Pen";
+			/* force BTN_STYLUS to allow tablet matching in udev */
+			__set_bit(BTN_STYLUS, hi->input->keybit);
+			break;
+		case HID_DG_TOUCHSCREEN:
+			/* we do not set suffix = "Touchscreen" */
+			break;
+		case HID_GD_SYSTEM_CONTROL:
+			suffix = "System Control";
+			break;
+		case HID_CP_CONSUMER_CONTROL:
+			suffix = "Consumer Control";
+			break;
+		default:
+			suffix = "UNKNOWN";
+			break;
+		}
 	}
 
 	if (suffix) {
@@ -992,6 +1057,11 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_3M,
 			USB_DEVICE_ID_3M3266) },
 
+	/* Anton devices */
+	{ .driver_data = MT_CLS_EXPORT_ALL_INPUTS,
+		MT_USB_DEVICE(USB_VENDOR_ID_ANTON,
+			USB_DEVICE_ID_ANTON_TOUCH_PAD) },
+
 	/* Atmel panels */
 	{ .driver_data = MT_CLS_SERIAL,
 		MT_USB_DEVICE(USB_VENDOR_ID_ATMEL,
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 5eb282e..e224516 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -201,6 +201,7 @@ struct hid_item {
 #define HID_GD_VBRZ		0x00010045
 #define HID_GD_VNO		0x00010046
 #define HID_GD_FEATURE		0x00010047
+#define HID_GD_SYSTEM_CONTROL	0x00010080
 #define HID_GD_UP		0x00010090
 #define HID_GD_DOWN		0x00010091
 #define HID_GD_RIGHT		0x00010092
@@ -208,6 +209,8 @@ struct hid_item {
 
 #define HID_DC_BATTERYSTRENGTH	0x00060020
 
+#define HID_CP_CONSUMER_CONTROL	0x000c0001
+
 #define HID_DG_DIGITIZER	0x000d0001
 #define HID_DG_PEN		0x000d0002
 #define HID_DG_LIGHTPEN		0x000d0003
-- 
1.8.5.3


^ permalink raw reply related

* [PATCH 2/3] HID: multitouch: remove pen special handling
From: Benjamin Tissoires @ 2014-02-28 16:41 UTC (permalink / raw)
  To: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Stephane Chatty,
	linux-input, linux-kernel
In-Reply-To: <1393605685-17025-1-git-send-email-benjamin.tissoires@redhat.com>

Pens have a special handling in hid-mt as hybrid pen/touch devices
are quite common now. However, some fancy devices presents also
useful collections like mouse or keyboard.
The special case for the pen may not be a special case, and treat it as
a generic case.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-multitouch.c | 61 +++++++-------------------------------------
 1 file changed, 9 insertions(+), 52 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 3e81f4e..8250cc0 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -100,7 +100,6 @@ struct mt_device {
 	int cc_value_index;	/* contact count value index in the field */
 	unsigned last_slot_field;	/* the last field of a slot */
 	unsigned mt_report_id;	/* the report ID of the multitouch device */
-	unsigned pen_report_id;	/* the report ID of the pen device */
 	__s16 inputmode;	/* InputMode HID feature, -1 if non-existent */
 	__s16 inputmode_index;	/* InputMode HID feature index in the report */
 	__s16 maxcontact_report_id;	/* Maximum Contact Number HID feature,
@@ -342,45 +341,6 @@ static void mt_store_field(struct hid_usage *usage, struct mt_device *td,
 	f->usages[f->length++] = usage->hid;
 }
 
-static int mt_pen_input_mapping(struct hid_device *hdev, struct hid_input *hi,
-		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
-{
-	struct mt_device *td = hid_get_drvdata(hdev);
-
-	td->pen_report_id = field->report->id;
-
-	return 0;
-}
-
-static int mt_pen_input_mapped(struct hid_device *hdev, struct hid_input *hi,
-		struct hid_field *field, struct hid_usage *usage,
-		unsigned long **bit, int *max)
-{
-	return 0;
-}
-
-static int mt_pen_event(struct hid_device *hid, struct hid_field *field,
-				struct hid_usage *usage, __s32 value)
-{
-	/* let hid-input handle it */
-	return 0;
-}
-
-static void mt_pen_report(struct hid_device *hid, struct hid_report *report)
-{
-	struct hid_field *field = report->field[0];
-
-	input_sync(field->hidinput->input);
-}
-
-static void mt_pen_input_configured(struct hid_device *hdev,
-					struct hid_input *hi)
-{
-	/* force BTN_STYLUS to allow tablet matching in udev */
-	__set_bit(BTN_STYLUS, hi->input->keybit);
-}
-
 static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		struct hid_field *field, struct hid_usage *usage,
 		unsigned long **bit, int *max)
@@ -767,7 +727,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		return -1;
 
 	if (field->physical == HID_DG_STYLUS)
-		return mt_pen_input_mapping(hdev, hi, field, usage, bit, max);
+		return 0;
 
 	return mt_touch_input_mapping(hdev, hi, field, usage, bit, max);
 }
@@ -777,7 +737,7 @@ static int mt_input_mapped(struct hid_device *hdev, struct hid_input *hi,
 		unsigned long **bit, int *max)
 {
 	if (field->physical == HID_DG_STYLUS)
-		return mt_pen_input_mapped(hdev, hi, field, usage, bit, max);
+		return 0;
 
 	return mt_touch_input_mapped(hdev, hi, field, usage, bit, max);
 }
@@ -790,25 +750,22 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
 	if (field->report->id == td->mt_report_id)
 		return mt_touch_event(hid, field, usage, value);
 
-	if (field->report->id == td->pen_report_id)
-		return mt_pen_event(hid, field, usage, value);
-
-	/* ignore other reports */
-	return 1;
+	return 0;
 }
 
 static void mt_report(struct hid_device *hid, struct hid_report *report)
 {
 	struct mt_device *td = hid_get_drvdata(hid);
+	struct hid_field *field = report->field[0];
 
 	if (!(hid->claimed & HID_CLAIMED_INPUT))
 		return;
 
 	if (report->id == td->mt_report_id)
-		mt_touch_report(hid, report);
+		return mt_touch_report(hid, report);
 
-	if (report->id == td->pen_report_id)
-		mt_pen_report(hid, report);
+	if (field && field->hidinput && field->hidinput->input)
+		input_sync(field->hidinput->input);
 }
 
 static void mt_set_input_mode(struct hid_device *hdev)
@@ -895,7 +852,8 @@ static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 
 	if (hi->report->field[0]->physical == HID_DG_STYLUS) {
 		suffix = "Pen";
-		mt_pen_input_configured(hdev, hi);
+		/* force BTN_STYLUS to allow tablet matching in udev */
+		__set_bit(BTN_STYLUS, hi->input->keybit);
 	}
 
 	if (suffix) {
@@ -957,7 +915,6 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	td->maxcontact_report_id = -1;
 	td->cc_index = -1;
 	td->mt_report_id = -1;
-	td->pen_report_id = -1;
 	hid_set_drvdata(hdev, td);
 
 	td->fields = devm_kzalloc(&hdev->dev, sizeof(struct mt_fields),
-- 
1.8.5.3


^ permalink raw reply related

* [PATCH 1/3] HID: multitouch: remove registered devices with default behavior
From: Benjamin Tissoires @ 2014-02-28 16:41 UTC (permalink / raw)
  To: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Stephane Chatty,
	linux-input, linux-kernel
In-Reply-To: <1393605685-17025-1-git-send-email-benjamin.tissoires@redhat.com>

The default multitouch protocol class in use since the kernel v3.9 is
working quite well. Since its inclusion, the only devices we had to tweak
were those who really need quirks (GeneralTouch, FocalTech and Wistron,
the 3 of them are Win 7 certified ones).
The flow of new unhandled devices has stopped, which is great and I think
it's time to reduce the list of registered device.

This commit removes only the registration in the kernel of devices that
use the class MT_CLS_DEFAULT, or that can use it. By that, I mean that I
checked all the recordings I have, and the produced input device and
events are the same before and after applying the patch.

This gives two benefits:
- remove a bunch of lines of codes
- prevent bad handling of existing registered devices which are using a
different protocol while using the same VID/PID (I got the case of a
Quanta 3008 recently).

I also removed the associated classes (MT_CLS*). I kept their #define in
case people use the new_id sysfs node with a non standard class (their
should be really few people now, but we never now). This is why there
are /* reserved .... */.

Last, I add a comment on top of mt_devices[] definition to remember people
(and myself) not to include devices for the beauty of it.

To people still trying to add devices with the default class:
"""
Guys, try out your device under a kernel greater or equal to v3.9. If it
works, you are all set. Adding your VID/PID to the kernel only brings us
overload and you won't get anything from it _because_ even a backport of
this shiny patch will _not_ make the device work under 3.0, 3.2, 3.4 or
even 3.8.
So if it works, it works.
If it does not work, then yes, submit a patch or call for help.
In any cases, if you want me to do regression tests, I'd be happy to
get some traces of your device. But I won't patch the kernel if it works.
"""

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-multitouch.c | 129 ++++---------------------------------------
 1 file changed, 11 insertions(+), 118 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index f134d73..3e81f4e 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -128,16 +128,16 @@ static void mt_post_parse(struct mt_device *td);
 #define MT_CLS_CONFIDENCE_MINUS_ONE		0x0005
 #define MT_CLS_DUAL_INRANGE_CONTACTID		0x0006
 #define MT_CLS_DUAL_INRANGE_CONTACTNUMBER	0x0007
-#define MT_CLS_DUAL_NSMU_CONTACTID		0x0008
+/* reserved					0x0008 */
 #define MT_CLS_INRANGE_CONTACTNUMBER		0x0009
 #define MT_CLS_NSMU				0x000a
-#define MT_CLS_DUAL_CONTACT_NUMBER		0x0010
-#define MT_CLS_DUAL_CONTACT_ID			0x0011
+/* reserved					0x0010 */
+/* reserved					0x0011 */
 #define MT_CLS_WIN_8				0x0012
 
 /* vendor specific classes */
 #define MT_CLS_3M				0x0101
-#define MT_CLS_CYPRESS				0x0102
+/* reserved					0x0102 */
 #define MT_CLS_EGALAX				0x0103
 #define MT_CLS_EGALAX_SERIAL			0x0104
 #define MT_CLS_TOPSEED				0x0105
@@ -189,23 +189,9 @@ static struct mt_class mt_classes[] = {
 		.quirks = MT_QUIRK_VALID_IS_INRANGE |
 			MT_QUIRK_SLOT_IS_CONTACTNUMBER,
 		.maxcontacts = 2 },
-	{ .name = MT_CLS_DUAL_NSMU_CONTACTID,
-		.quirks = MT_QUIRK_NOT_SEEN_MEANS_UP |
-			MT_QUIRK_SLOT_IS_CONTACTID,
-		.maxcontacts = 2 },
 	{ .name = MT_CLS_INRANGE_CONTACTNUMBER,
 		.quirks = MT_QUIRK_VALID_IS_INRANGE |
 			MT_QUIRK_SLOT_IS_CONTACTNUMBER },
-	{ .name = MT_CLS_DUAL_CONTACT_NUMBER,
-		.quirks = MT_QUIRK_ALWAYS_VALID |
-			MT_QUIRK_CONTACT_CNT_ACCURATE |
-			MT_QUIRK_SLOT_IS_CONTACTNUMBER,
-		.maxcontacts = 2 },
-	{ .name = MT_CLS_DUAL_CONTACT_ID,
-		.quirks = MT_QUIRK_ALWAYS_VALID |
-			MT_QUIRK_CONTACT_CNT_ACCURATE |
-			MT_QUIRK_SLOT_IS_CONTACTID,
-		.maxcontacts = 2 },
 	{ .name = MT_CLS_WIN_8,
 		.quirks = MT_QUIRK_ALWAYS_VALID |
 			MT_QUIRK_IGNORE_DUPLICATES |
@@ -223,10 +209,6 @@ static struct mt_class mt_classes[] = {
 		.sn_height = 128,
 		.maxcontacts = 60,
 	},
-	{ .name = MT_CLS_CYPRESS,
-		.quirks = MT_QUIRK_NOT_SEEN_MEANS_UP |
-			MT_QUIRK_CYPRESS,
-		.maxcontacts = 10 },
 	{ .name = MT_CLS_EGALAX,
 		.quirks =  MT_QUIRK_SLOT_IS_CONTACTID |
 			MT_QUIRK_VALID_IS_INRANGE,
@@ -1034,6 +1016,12 @@ static void mt_remove(struct hid_device *hdev)
 	hid_hw_stop(hdev);
 }
 
+/*
+ * This list contains only:
+ * - VID/PID of products not working with the default multitouch handling
+ * - 2 generic rules.
+ * So there is no point in adding here any device with MT_CLS_DEFAULT.
+ */
 static const struct hid_device_id mt_devices[] = {
 
 	/* 3M panels */
@@ -1047,33 +1035,20 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_3M,
 			USB_DEVICE_ID_3M3266) },
 
-	/* ActionStar panels */
-	{ .driver_data = MT_CLS_NSMU,
-		MT_USB_DEVICE(USB_VENDOR_ID_ACTIONSTAR,
-			USB_DEVICE_ID_ACTIONSTAR_1011) },
-
 	/* Atmel panels */
 	{ .driver_data = MT_CLS_SERIAL,
 		MT_USB_DEVICE(USB_VENDOR_ID_ATMEL,
-			USB_DEVICE_ID_ATMEL_MULTITOUCH) },
-	{ .driver_data = MT_CLS_SERIAL,
-		MT_USB_DEVICE(USB_VENDOR_ID_ATMEL,
 			USB_DEVICE_ID_ATMEL_MXT_DIGITIZER) },
 
 	/* Baanto multitouch devices */
 	{ .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_BAANTO,
 			USB_DEVICE_ID_BAANTO_MT_190W2) },
+
 	/* Cando panels */
 	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
 		MT_USB_DEVICE(USB_VENDOR_ID_CANDO,
 			USB_DEVICE_ID_CANDO_MULTI_TOUCH) },
-	{ .driver_data = MT_CLS_DUAL_CONTACT_NUMBER,
-		MT_USB_DEVICE(USB_VENDOR_ID_CANDO,
-			USB_DEVICE_ID_CANDO_MULTI_TOUCH_10_1) },
-	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
-		MT_USB_DEVICE(USB_VENDOR_ID_CANDO,
-			USB_DEVICE_ID_CANDO_MULTI_TOUCH_11_6) },
 	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTNUMBER,
 		MT_USB_DEVICE(USB_VENDOR_ID_CANDO,
 			USB_DEVICE_ID_CANDO_MULTI_TOUCH_15_6) },
@@ -1088,16 +1063,6 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_CVTOUCH,
 			USB_DEVICE_ID_CVTOUCH_SCREEN) },
 
-	/* Cypress panel */
-	{ .driver_data = MT_CLS_CYPRESS,
-		HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
-			USB_DEVICE_ID_CYPRESS_TRUETOUCH) },
-
-	/* Data Modul easyMaxTouch */
-	{ .driver_data = MT_CLS_DEFAULT,
-		MT_USB_DEVICE(USB_VENDOR_ID_DATA_MODUL,
-			USB_VENDOR_ID_DATA_MODUL_EASYMAXTOUCH) },
-
 	/* eGalax devices (resistive) */
 	{ .driver_data = MT_CLS_EGALAX,
 		MT_USB_DEVICE(USB_VENDOR_ID_DWAV,
@@ -1156,11 +1121,6 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_DWAV,
 			USB_DEVICE_ID_DWAV_EGALAX_MULTITOUCH_A001) },
 
-	/* Elo TouchSystems IntelliTouch Plus panel */
-	{ .driver_data = MT_CLS_DUAL_CONTACT_ID,
-		MT_USB_DEVICE(USB_VENDOR_ID_ELO,
-			USB_DEVICE_ID_ELO_TS2515) },
-
 	/* Flatfrog Panels */
 	{ .driver_data = MT_CLS_FLATFROG,
 		MT_USB_DEVICE(USB_VENDOR_ID_FLATFROG,
@@ -1204,37 +1164,11 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_HANVON_ALT,
 			USB_DEVICE_ID_HANVON_ALT_MULTITOUCH) },
 
-	/* Ideacom panel */
-	{ .driver_data = MT_CLS_SERIAL,
-		MT_USB_DEVICE(USB_VENDOR_ID_IDEACOM,
-			USB_DEVICE_ID_IDEACOM_IDC6650) },
-	{ .driver_data = MT_CLS_SERIAL,
-		MT_USB_DEVICE(USB_VENDOR_ID_IDEACOM,
-			USB_DEVICE_ID_IDEACOM_IDC6651) },
-
 	/* Ilitek dual touch panel */
 	{  .driver_data = MT_CLS_NSMU,
 		MT_USB_DEVICE(USB_VENDOR_ID_ILITEK,
 			USB_DEVICE_ID_ILITEK_MULTITOUCH) },
 
-	/* IRTOUCH panels */
-	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID,
-		MT_USB_DEVICE(USB_VENDOR_ID_IRTOUCHSYSTEMS,
-			USB_DEVICE_ID_IRTOUCH_INFRARED_USB) },
-
-	/* LG Display panels */
-	{ .driver_data = MT_CLS_DEFAULT,
-		MT_USB_DEVICE(USB_VENDOR_ID_LG,
-			USB_DEVICE_ID_LG_MULTITOUCH) },
-
-	/* Lumio panels */
-	{ .driver_data = MT_CLS_CONFIDENCE_MINUS_ONE,
-		MT_USB_DEVICE(USB_VENDOR_ID_LUMIO,
-			USB_DEVICE_ID_CRYSTALTOUCH) },
-	{ .driver_data = MT_CLS_CONFIDENCE_MINUS_ONE,
-		MT_USB_DEVICE(USB_VENDOR_ID_LUMIO,
-			USB_DEVICE_ID_CRYSTALTOUCH_DUAL) },
-
 	/* MosArt panels */
 	{ .driver_data = MT_CLS_CONFIDENCE_MINUS_ONE,
 		MT_USB_DEVICE(USB_VENDOR_ID_ASUS,
@@ -1246,11 +1180,6 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_TURBOX,
 			USB_DEVICE_ID_TURBOX_TOUCHSCREEN_MOSART) },
 
-	/* Nexio panels */
-	{ .driver_data = MT_CLS_DEFAULT,
-		MT_USB_DEVICE(USB_VENDOR_ID_NEXIO,
-			USB_DEVICE_ID_NEXIO_MULTITOUCH_420)},
-
 	/* Panasonic panels */
 	{ .driver_data = MT_CLS_PANASONIC,
 		MT_USB_DEVICE(USB_VENDOR_ID_PANASONIC,
@@ -1264,11 +1193,6 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_NOVATEK,
 			USB_DEVICE_ID_NOVATEK_PCT) },
 
-	/* PenMount panels */
-	{ .driver_data = MT_CLS_CONFIDENCE,
-		MT_USB_DEVICE(USB_VENDOR_ID_PENMOUNT,
-			USB_DEVICE_ID_PENMOUNT_PCI) },
-
 	/* PixArt optical touch screen */
 	{ .driver_data = MT_CLS_INRANGE_CONTACTNUMBER,
 		MT_USB_DEVICE(USB_VENDOR_ID_PIXART,
@@ -1282,44 +1206,18 @@ static const struct hid_device_id mt_devices[] = {
 
 	/* PixCir-based panels */
 	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID,
-		MT_USB_DEVICE(USB_VENDOR_ID_HANVON,
-			USB_DEVICE_ID_HANVON_MULTITOUCH) },
-	{ .driver_data = MT_CLS_DUAL_INRANGE_CONTACTID,
 		MT_USB_DEVICE(USB_VENDOR_ID_CANDO,
 			USB_DEVICE_ID_CANDO_PIXCIR_MULTI_TOUCH) },
 
 	/* Quanta-based panels */
 	{ .driver_data = MT_CLS_CONFIDENCE_CONTACT_ID,
 		MT_USB_DEVICE(USB_VENDOR_ID_QUANTA,
-			USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH) },
-	{ .driver_data = MT_CLS_CONFIDENCE_CONTACT_ID,
-		MT_USB_DEVICE(USB_VENDOR_ID_QUANTA,
 			USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3001) },
-	{ .driver_data = MT_CLS_CONFIDENCE_CONTACT_ID,
-		MT_USB_DEVICE(USB_VENDOR_ID_QUANTA,
-			USB_DEVICE_ID_QUANTA_OPTICAL_TOUCH_3008) },
-
-	/* SiS panels */
-	{ .driver_data = MT_CLS_DEFAULT,
-		HID_USB_DEVICE(USB_VENDOR_ID_SIS_TOUCH,
-		USB_DEVICE_ID_SIS9200_TOUCH) },
-	{ .driver_data = MT_CLS_DEFAULT,
-		HID_USB_DEVICE(USB_VENDOR_ID_SIS_TOUCH,
-		USB_DEVICE_ID_SIS817_TOUCH) },
-	{ .driver_data = MT_CLS_DEFAULT,
-		HID_USB_DEVICE(USB_VENDOR_ID_SIS_TOUCH,
-		USB_DEVICE_ID_SIS1030_TOUCH) },
 
 	/* Stantum panels */
 	{ .driver_data = MT_CLS_CONFIDENCE,
-		MT_USB_DEVICE(USB_VENDOR_ID_STANTUM,
-			USB_DEVICE_ID_MTP)},
-	{ .driver_data = MT_CLS_CONFIDENCE,
 		MT_USB_DEVICE(USB_VENDOR_ID_STANTUM_STM,
 			USB_DEVICE_ID_MTP_STM)},
-	{ .driver_data = MT_CLS_DEFAULT,
-		MT_USB_DEVICE(USB_VENDOR_ID_STANTUM_SITRONIX,
-			USB_DEVICE_ID_MTP_SITRONIX)},
 
 	/* TopSeed panels */
 	{ .driver_data = MT_CLS_TOPSEED,
@@ -1378,11 +1276,6 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_XIROKU,
 			USB_DEVICE_ID_XIROKU_CSR2) },
 
-	/* Zytronic panels */
-	{ .driver_data = MT_CLS_SERIAL,
-		MT_USB_DEVICE(USB_VENDOR_ID_ZYTRONIC,
-			USB_DEVICE_ID_ZYTRONIC_ZXY100) },
-
 	/* Generic MT device */
 	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_MULTITOUCH, HID_ANY_ID, HID_ANY_ID) },
 
-- 
1.8.5.3

^ permalink raw reply related

* [PATCH 0/3] HID: multitouch cleanups and support for fancy devices
From: Benjamin Tissoires @ 2014-02-28 16:41 UTC (permalink / raw)
  To: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Stephane Chatty,
	linux-input, linux-kernel

Hi guys,

Ok, this patch series is not very consistent and could have been split in two...
Anyway, here is some work for hid-multitouch:

- the first patch is mainly a way for us to reduce the work load regarding hid-mt
  when device makers ask for an inclusion in the kernel.
  Also, I have been notified that a Quanta 3008 was not working, and it occurs
  that this particular device has a special handling in hid-mt. Of course, the
  hardware maker reused the same VID/PID, but changed the protocol.
  So yes, I am in favour of killing all unneeded special cases.

  I checked all the devices I was able to test, and this leaves us a bunch of
  devices which, I am sure, could be removed too (all the ones following
  MT_CLS_NSMU should be in this case). So, if anyone else wants to join me in
  this crusade, I'd be glad to receive patches.

- the second and third patch are the v2 of the series I sent back in December
  2013 ([PATCH 0/3] Change in handling different input device in hid-multitouch).
  Henrik made valuable comments, and I did not had the time to look at it and to
  figure out how to address them. Now I got a different look at this work, and
  I think I addressed the issues (no more function pointers, yeah!)

Happy reviewing.

Cheers,
Benjamin

PS: it might be Spring coming, but I am definitively trying to reduce the total
lines of code in the HID subsystem...

Benjamin Tissoires (3):
  HID: multitouch: remove default behaviors
  HID: multitouch: remove pen special handling
  HID: multitouch: add support of other generic collections in hid-mt

 drivers/hid/hid-ids.h        |   3 +
 drivers/hid/hid-multitouch.c | 270 +++++++++++++++----------------------------
 include/linux/hid.h          |   3 +
 3 files changed, 101 insertions(+), 175 deletions(-)

-- 
1.8.5.3

^ permalink raw reply

* Re: change kmalloc into vmalloc for large memory allocations
From: 'gregkh@linuxfoundation.org' @ 2014-02-28 16:33 UTC (permalink / raw)
  To: Wang, Yalin
  Cc: 'alsa-devel@alsa-project.org',
	'broonie@opensource.wolfsonmicro.com',
	'tiwai@suse.de', 'fweisbec@gmail.com',
	'mingo@redhat.com', 'linux-input@vger.kernel.org',
	'rydberg@euromail.se', 'lrg@ti.com',
	'pablo@netfilter.org', 'coreteam@netfilter.org',
	'linux-arm-msm@vger.kernel.org',
	'rostedt@goodmis.org',
	'netfilter@vger.kernel.org',
	'linux-arm-kernel@lists.infradead.org',
	'netdev@vger.kernel.org',
	'dmitry.torokhov@gmail.com', 'linux-kern
In-Reply-To: <35FD53F367049845BC99AC72306C23D102844605F38D@CNBJMBX05.corpusers.net>

On Fri, Feb 28, 2014 at 05:20:08PM +0800, Wang, Yalin wrote:
> Hi  
> 
> 
> Yeah,  
> Dma buffer must be allocated by kmalloc,
> 
> But the modules I list should can all be changed to use
> vmalloc, because the buffer is only used by software,
> Not by any hardware .

Are you sure about that?  The USB gadget driver needs DMA memory from
what I can tell, have you tried your change out on a system that does
not allow the USB controller to access non-DMA memory?

And I agree with Steve, just fix the individual drivers, don't do a
"hidden" change of where the memory is allocated from, that's not a good
idea and will cause problems later.

greg k-h

^ permalink raw reply

* Re: change kmalloc into vmalloc for large memory allocations
From: Takashi Iwai @ 2014-02-28 14:19 UTC (permalink / raw)
  To: Wang, Yalin
  Cc: 'alsa-devel@alsa-project.org',
	'broonie@opensource.wolfsonmicro.com',
	'fweisbec@gmail.com', 'dmitry.torokhov@gmail.com',
	'coreteam@netfilter.org',
	'linux-input@vger.kernel.org',
	'rydberg@euromail.se', 'lrg@ti.com',
	'pablo@netfilter.org',
	'linux-arm-msm@vger.kernel.org',
	'rostedt@goodmis.org',
	'netfilter@vger.kernel.org', 'mingo@redhat.com',
	'linux-arm-kernel@lists.infradead.org',
	'gregkh@linuxfoundation.org',
	"'linux-usb@vger.kernel.org'" <linux-us>
In-Reply-To: <35FD53F367049845BC99AC72306C23D102844605F38C@CNBJMBX05.corpusers.net>

At Fri, 28 Feb 2014 16:15:23 +0800,
Wang, Yalin wrote:
> 
> Hi 
> 
> 
> I find there is some drivers use kmalloc to allocate large 
> Memorys during module_init,  some can be changed to use vmalloc
> To save some low mem, I add log in kernel to track ,
> And list them here:
> 
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drivers/usb/gadget/f_mass_storage.c?h=master#n2724
> 
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/sound/soc/soc-core.c?h=master#n3772

At least the ASoC runtime case doesn't use the allocated memory as
buffer, and they are allocated only once per device, thus it shouldn't
be the problem you stated.  If it really consumes so much memory, we
need to rethink, instead of allocating an array but allocate each
object, for example.


thanks,

Takashi


> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_ftp.c?h=master#n603
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_h323_main.c?h=master#n1849
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_irc.c?h=master#n247
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_sane.c?h=master#n195
> 
> https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drivers/input/evdev.c?h=master#n403
> 
> 
> they allocate large memory from 10k~64K , 
> this will use lots of low mem, instead if we use 
> vmalloc for these drivers , it will be good for some devices
> like smart phone, we often encounter some errors like kmalloc failed 
> because there is not enough low mem , especially when the device has physical 
> memory less than 1GB .
> 
> 
> could this module change the memory allocation into vmalloc ?
> 
> I was thinking that if we can introduce a helper function in kmalloc.h like this :
> 
> Kmalloc(size, flags)
> {
> 	If (size > PAGE_SIZE && flags&CAN_USE_VMALLOC_FLAG)
> 		return vmalloc(size);
> 	Else
> 		return real_kmalloc(size);
> }
> 
> Kfree(ptr)
> {
> 	If (is_vmalloc_addr(ptr))
> 		Vfree(ptr);
> 	Else
> 		Kfree(ptr);
> }
> 
> 
> But we need add some flags to ensure always use kmalloc for
> Some special use (dma etc..)
> 
> How do you think of it ?
> 
> Thanks
> 
> 
> 

^ permalink raw reply

* Re: change kmalloc into vmalloc for large memory allocations
From: Steven Rostedt @ 2014-02-28 14:11 UTC (permalink / raw)
  To: Wang, Yalin
  Cc: 'alsa-devel@alsa-project.org',
	'linux-usb@vger.kernel.org', 'tiwai@suse.de',
	'fweisbec@gmail.com', 'dmitry.torokhov@gmail.com',
	'mingo@redhat.com', 'linux-input@vger.kernel.org',
	'rydberg@euromail.se', 'lrg@ti.com',
	'pablo@netfilter.org', 'coreteam@netfilter.org',
	'linux-arm-msm@vger.kernel.org',
	'netfilter@vger.kernel.org',
	'linux-arm-kernel@lists.infradead.org',
	'gregkh@linuxfoundation.org',
	'broonie@opensource.wolfsonmicro.com'
In-Reply-To: <35FD53F367049845BC99AC72306C23D102844605F38C@CNBJMBX05.corpusers.net>

On Fri, 28 Feb 2014 16:15:23 +0800
"Wang, Yalin" <Yalin.Wang@sonymobile.com> wrote:

> Kfree(ptr)
> {
> 	If (is_vmalloc_addr(ptr))
> 		Vfree(ptr);
> 	Else
> 		Kfree(ptr);
> }
> 
> 
> But we need add some flags to ensure always use kmalloc for
> Some special use (dma etc..)
> 
> How do you think of it ?

But vmalloc also takes up tlb entries. The more vmalloc space you have,
the more tlb entries that will need to be used, which will have a
performance affect on the entire system.

I would be against making kmalloc() secretly doing a vmalloc. If it is
better for a driver to use a vmalloc on large items, than change the
driver. Don't change a core function.

-- Steve

^ permalink raw reply

* Re: [PATCH v4 7/9] devicetree: bindings: Document PM8921/8058 keypads
From: Josh Cartwright @ 2014-02-28 13:55 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Dmitry Torokhov, linux-kernel, linux-arm-msm, linux-arm-kernel,
	linux-input, devicetree
In-Reply-To: <1393552520-9068-8-git-send-email-sboyd@codeaurora.org>

On Thu, Feb 27, 2014 at 05:55:18PM -0800, Stephen Boyd wrote:
> Document the keypad device found on PM8921 and PM8058 PMICs.
[..]
> +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
> @@ -0,0 +1,72 @@
> +Qualcomm PM8xxx PMIC Keypad
> +
> +PROPERTIES
> +
> +- compatible:
> +	Usage: required
> +	Value type: <string>
> +	Definition: must be one of:
> +		    "qcom,pm8058-keypad"
> +		    "qcom,pm8921-keypad"
> +- interrupts:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: the first interrupt specifies the key sense interrupt
> +		    and the second interrupt specifies the key stuck interrupt.
> +		    The format of the specifier is defined by the binding
> +		    document describing the node's interrupt parent.
> +
> +- linux,keymap:
> +	Usage: required
> +	Value type: <prop-encoded-array>
> +	Definition: the linux keymap. More information can be found in
> +		    input/matrix-keymap.txt.
> +
> +- keypad,num-rows:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: number of rows in the keymap. More information can be found
> +		    in input/matrix-keymap.txt.
> +
> +- keypad,num-columns:
> +	Usage: required
> +	Value type: <u32>
> +	Definition: number of columns in the keymap. More information can be
> +		    found in input/matrix-keymap.txt.
> +
> +- debounce:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: time in microseconds that key must be pressed or release
> +		    for key sense interrupt to trigger.
> +
> +- scan-delay:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: time in microseconds to pause between successive scans
> +		    of the matrix array.
> +
> +- row-hold:
> +	Usage: optional
> +	Value type: <u32>
> +	Definition: time in nanoseconds to pause between scans of each row in
> +		    the matrix array.

- linux,wakeup?
- linux,no-auto-repeat?

> +
> +EXAMPLE
> +
> +	keypad {
> +		compatible = "qcom,pm8921-keypad";
> +		interrupt-parent = <&pmicintc>;
> +		interrupts = <74 1>, <75 1>;
> +		linux,keymap = <
> +			MATRIX_KEY(0, 0, KEY_VOLUMEUP)
> +			MATRIX_KEY(0, 1, KEY_VOLUMEDOWN)
> +			MATRIX_KEY(0, 2, KEY_CAMERA_FOCUS)
> +			MATRIX_KEY(0, 3, KEY_CAMERA)
> +			>;
> +		keypad,num-rows = <1>;
> +		keypad,num-columns = <5>;
> +		debounce = <15>;
> +		scan-delay = <32>;
> +		row-hold = <91500>;
> +	};

It odd to me that these newly created bindings don't have 'reg'
properties, even though the device clearly has a register region.

I suppose it makes sense from a "port over from platform data to DT"
perspective, as these drivers have just assumed the location of their
registers to be fixed; however I suspect things will need to be changed
if/when we hope to share these drivers with pm8841/pm8941 and beyond...

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

^ permalink raw reply

* Re: change kmalloc into vmalloc for large memory allocations
From: Wang, Yalin @ 2014-02-28  9:20 UTC (permalink / raw)
  To: 'Huang Shijie'
  Cc: 'alsa-devel@alsa-project.org',
	'broonie@opensource.wolfsonmicro.com',
	'tiwai@suse.de', 'fweisbec@gmail.com',
	'dmitry.torokhov@gmail.com', 'mingo@redhat.com',
	'linux-input@vger.kernel.org',
	'rydberg@euromail.se', 'lrg@ti.com',
	'pablo@netfilter.org', 'coreteam@netfilter.org',
	'linux-arm-msm@vger.kernel.org',
	'rostedt@goodmis.org',
	'netfilter@vger.kernel.org',
	'linux-arm-kernel@lists.infradead.org',
	'gregkh@linuxfoundation.org'
In-Reply-To: <53104ECA.4010702@freescale.com>

Hi  


Yeah,  
Dma buffer must be allocated by kmalloc,

But the modules I list should can all be changed to use
vmalloc, because the buffer is only used by software,
Not by any hardware .


Thanks

-----Original Message-----
From: Huang Shijie [mailto:b32955@freescale.com] 
Sent: Friday, February 28, 2014 4:55 PM
To: Wang, Yalin
Cc: 'linux-kernel@vger.kernel.org'; 'linux-arm-msm@vger.kernel.org'; 'linux-arm-kernel@lists.infradead.org'; 'linux-input@vger.kernel.org'; 'balbi@ti.com'; 'gregkh@linuxfoundation.org'; 'lrg@ti.com'; 'broonie@opensource.wolfsonmicro.com'; 'perex@perex.cz'; 'tiwai@suse.de'; 'pablo@netfilter.org'; 'kaber@trash.net'; 'davem@davemloft.net'; 'rostedt@goodmis.org'; 'fweisbec@gmail.com'; 'mingo@redhat.com'; 'dmitry.torokhov@gmail.com'; 'rydberg@euromail.se'; 'linux-usb@vger.kernel.org'; 'alsa-devel@alsa-project.org'; 'netfilter-devel@vger.kernel.org'; 'netfilter@vger.kernel.org'; 'coreteam@netfilter.org'; 'netdev@vger.kernel.org'
Subject: Re: change kmalloc into vmalloc for large memory allocations

于 2014年02月28日 16:15, Wang, Yalin 写道:
> could this module change the memory allocation into vmalloc ?
the buffer allocated by vmalloc can not be used for the DMA.

I think that's why drivers use the kmalloc.

thanks
Huang Shijie

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply

* Re: change kmalloc into vmalloc for large memory allocations
From: Huang Shijie @ 2014-02-28  8:54 UTC (permalink / raw)
  To: Wang, Yalin
  Cc: 'alsa-devel@alsa-project.org',
	'broonie@opensource.wolfsonmicro.com',
	'tiwai@suse.de', 'fweisbec@gmail.com',
	'perex@perex.cz', 'dmitry.torokhov@gmail.com',
	'mingo@redhat.com', 'linux-input@vger.kernel.org',
	'rydberg@euromail.se', 'lrg@ti.com',
	'pablo@netfilter.org', 'coreteam@netfilter.org',
	'linux-arm-msm@vger.kernel.org',
	'rostedt@goodmis.org',
	'netfilter@vger.kernel.org',
	'linux-arm-kernel@lists.infradead.org', 'gregkh
In-Reply-To: <35FD53F367049845BC99AC72306C23D102844605F38C@CNBJMBX05.corpusers.net>

于 2014年02月28日 16:15, Wang, Yalin 写道:
> could this module change the memory allocation into vmalloc ?
the buffer allocated by vmalloc can not be used for the DMA.

I think that's why drivers use the kmalloc.

thanks
Huang Shijie


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply

* change kmalloc into vmalloc for large memory allocations
From: Wang, Yalin @ 2014-02-28  8:15 UTC (permalink / raw)
  To: 'linux-kernel@vger.kernel.org',
	'linux-arm-msm@vger.kernel.org',
	'linux-arm-kernel@lists.infradead.org',
	'linux-input@vger.kernel.org', 'balbi@ti.com',
	'gregkh@linuxfoundation.org', 'lrg@ti.com',
	'broonie@opensource.wolfsonmicro.com',
	'perex@perex.cz', 'tiwai@suse.de',
	'pablo@netfilter.org', 'kaber@trash.net',
	'davem@davemloft.net', 'rostedt@goodmis.org',
	'fweisbec@gmail.com', 'mingo@redhat.com',
	'dmitry.torokhov@gmail.com', 'rydber

Hi 


I find there is some drivers use kmalloc to allocate large 
Memorys during module_init,  some can be changed to use vmalloc
To save some low mem, I add log in kernel to track ,
And list them here:

https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drivers/usb/gadget/f_mass_storage.c?h=master#n2724

https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/sound/soc/soc-core.c?h=master#n3772

https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_ftp.c?h=master#n603
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_h323_main.c?h=master#n1849
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_irc.c?h=master#n247
https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/net/netfilter/nf_conntrack_sane.c?h=master#n195

https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/tree/drivers/input/evdev.c?h=master#n403


they allocate large memory from 10k~64K , 
this will use lots of low mem, instead if we use 
vmalloc for these drivers , it will be good for some devices
like smart phone, we often encounter some errors like kmalloc failed 
because there is not enough low mem , especially when the device has physical 
memory less than 1GB .


could this module change the memory allocation into vmalloc ?

I was thinking that if we can introduce a helper function in kmalloc.h like this :

Kmalloc(size, flags)
{
	If (size > PAGE_SIZE && flags&CAN_USE_VMALLOC_FLAG)
		return vmalloc(size);
	Else
		return real_kmalloc(size);
}

Kfree(ptr)
{
	If (is_vmalloc_addr(ptr))
		Vfree(ptr);
	Else
		Kfree(ptr);
}


But we need add some flags to ensure always use kmalloc for
Some special use (dma etc..)

How do you think of it ?

Thanks

^ permalink raw reply

* Re: [PATCH v4 5/9] Input: pmic8xxx-pwrkey - Migrate to DT
From: Josh Cartwright @ 2014-02-28  3:45 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Dmitry Torokhov, linux-kernel, linux-arm-msm, linux-arm-kernel,
	linux-input
In-Reply-To: <1393552520-9068-6-git-send-email-sboyd@codeaurora.org>

On Thu, Feb 27, 2014 at 05:55:16PM -0800, Stephen Boyd wrote:
> The driver is only supported on DT enabled platforms. Convert the
> driver to DT so that it can probe properly.
> 
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
> --- a/drivers/input/misc/pmic8xxx-pwrkey.c
> +++ b/drivers/input/misc/pmic8xxx-pwrkey.c
> @@ -172,7 +178,7 @@ static int pmic8xxx_pwrkey_probe(struct platform_device *pdev)
>  	}
>  
>  	platform_set_drvdata(pdev, pwrkey);
> -	device_init_wakeup(&pdev->dev, pdata->wakeup);
> +	device_init_wakeup(&pdev->dev, 1);

Is there a particular reason you aren't providing a 'linux,wakeup'
property for pwrkey?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

^ permalink raw reply

* Re: [PATCH v4 4/9] Input: pmic8xxx-keypad - Migrate to DT
From: Josh Cartwright @ 2014-02-28  3:41 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Dmitry Torokhov, linux-kernel, linux-arm-msm, linux-arm-kernel,
	linux-input
In-Reply-To: <1393552520-9068-5-git-send-email-sboyd@codeaurora.org>

Looks good, with the exception of one thing...

On Thu, Feb 27, 2014 at 05:55:15PM -0800, Stephen Boyd wrote:
> The driver is only supported on DT enabled platforms. Convert the
> driver to DT so that it can probe properly.
> 
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>  drivers/input/keyboard/pmic8xxx-keypad.c | 150 ++++++++++++++++++-------------
> @@ -471,50 +514,27 @@ static void pmic8xxx_kp_close(struct input_dev *dev)
>   */
>  static int pmic8xxx_kp_probe(struct platform_device *pdev)
>  {
[..]
> -	keymap_data = pdata->keymap_data;
> -	if (!keymap_data) {
> -		dev_err(&pdev->dev, "no keymap data supplied\n");
> -		return -EINVAL;
> -	}
> +	repeat = !of_property_read_bool(pdev->dev.of_node,
> +					"linux,input-no-autorepeat");
> +	wakeup = !of_property_read_bool(pdev->dev.of_node,
> +					"linux,keypad-wakeup");

I don't think you mean to invert this.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox