Add separate mode switching function wacom bluetooth pen tablet.

PATCH Applied Przemo Firszt 3 participants 9 messages Mar 18, 2010 – Apr 23, 2010

Subsystems (2)

Bluetooth drivers HID

lore

Share & tools

lore

Przemo Firszt

author

2010-03-18 15:25 UTC #0 lore raw

PATCH 1/2 Add separate mode switching function wacom bluetooth pen tablet.

Hi,
Two patches to allow switching of reporting speed of wacom bluetooth tablet from userspace through sysfs.

kind reagrds,
Przemo

Quoted 1 line

From ac9a87b92cf2f8cc27a1d29123c5fa1a8bebd71a Mon Sep 17 00:00:00 2001

From: Przemo Firszt <[email protected]>
Date: Thu, 18 Mar 2010 11:58:22 +0000
Subject: [PATCH 1/2] Add separate mode switching function wacom bluetooth pen tablet.

wacom_poke function allows to switch tablet reporting speed. The patch dosen't add any new functionality, but it's preparation for user-space speed switching through sysfs.

Signed-off-by Przemo Firszt <[email protected]>

---
 drivers/hid/hid-wacom.c |   66 ++++++++++++++++++++++++++++------------------
 1 files changed, 40 insertions(+), 26 deletions(-)

drivers/hid/hid-wacom.c +40 ~~-26~~

index 4d2d2a2..f9d4939 100644
@@ -103,6 +103,44 @@ static int wacom_ac_get_property(struct power_supply *psy,
 }
 #endif
 
+static void wacom_poke(struct hid_device *hdev, u8 speed)
+{
+	int limit, ret;
+	char rep_data[2];
+
+	rep_data[0] = 0x03 ; rep_data[1] = 0x00;
+	limit = 3;
+	do {
+		ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
+				HID_FEATURE_REPORT);
+	} while (ret < 0 && limit-- > 0);
+
+	if (ret >= 0) {
+		if (speed == 0)
+			rep_data[0] = 0x05;
+		else
+			rep_data[0] = 0x06;
+
+		rep_data[1] = 0x00;
+		limit = 3;
+		do {
+			ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
+					HID_FEATURE_REPORT);
+		} while (ret < 0 && limit-- > 0);
+
+		if (ret >= 0)
+			return;
+	}
+
+	/*
+	 * Note that if the raw queries fail, it's not a hard failure and it
+	 * is safe to continue
+	 */
+	dev_warn(&hdev->dev, "failed to poke device, command %d, err %d\n",
+				rep_data[0], ret);
+	return;
+}
+
 static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *raw_data, int size)
 {

@@ -236,9 +274,7 @@ static int wacom_probe(struct hid_device *hdev,
 	struct hid_input *hidinput;
 	struct input_dev *input;
 	struct wacom_data *wdata;
-	char rep_data[2];
 	int ret;
-	int limit;
 
 	wdata = kzalloc(sizeof(*wdata), GFP_KERNEL);
 	if (wdata == NULL) {

@@ -261,30 +297,8 @@ static int wacom_probe(struct hid_device *hdev,
 		goto err_free;
 	}
 
-	/*
-	 * Note that if the raw queries fail, it's not a hard failure and it
-	 * is safe to continue
-	 */
-
-	/* Set Wacom mode2 */
-	rep_data[0] = 0x03; rep_data[1] = 0x00;
-	limit = 3;
-	do {
-		ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
-				HID_FEATURE_REPORT);
-	} while (ret < 0 && limit-- > 0);
-	if (ret < 0)
-		dev_warn(&hdev->dev, "failed to poke device #1, %d\n", ret);
-
-	/* 0x06 - high reporting speed, 0x05 - low speed */
-	rep_data[0] = 0x06; rep_data[1] = 0x00;
-	limit = 3;
-	do {
-		ret = hdev->hid_output_raw_report(hdev, rep_data, 2,
-				HID_FEATURE_REPORT);
-	} while (ret < 0 && limit-- > 0);
-	if (ret < 0)
-		dev_warn(&hdev->dev, "failed to poke device #2, %d\n", ret);
+	/* Set Wacom mode 2 with high reporting speed */
+	wacom_poke(hdev, 1);
 
 #ifdef CONFIG_HID_WACOM_POWER_SUPPLY
 	wdata->battery.properties = wacom_battery_props;

Signature · 1 line

1.7.0.1

Przemo Firszt

author

2010-03-18 15:26 UTC #1 lore raw

PATCH 2/2 Add sysfs speed attribute for wacom bluetooth tablet

Patch 2/2
--
Przemo

Quoted 1 line

From 92b0a06c748ea0a853c369595444dd411b32f314 Mon Sep 17 00:00:00 2001

From: Przemo Firszt <[email protected]>
Date: Thu, 18 Mar 2010 14:34:34 +0000
Subject: [PATCH 2/2] Add sysfs speed attribute for wacom bluetooth tablet

The attribute allows to change reporting speed of tablet from userspace through sysfs file. The attribute is RW, valid values: 0 is low speed, 1 is high speed. High speed is the default setting. Using low speed is a workaround if you experience lag when using the tablet.

Signed-off-by Przemo Firszt <[email protected]>

---
 drivers/hid/hid-wacom.c |   38 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 37 insertions(+), 1 deletions(-)

drivers/hid/hid-wacom.c +37 -1

index f9d4939..34d96e0 100644
@@ -30,6 +30,7 @@
 struct wacom_data {
 	__u16 tool;
 	unsigned char butstate;
+	unsigned char speed;
 #ifdef CONFIG_HID_WACOM_POWER_SUPPLY
 	int battery_capacity;
 	struct power_supply battery;

@@ -105,6 +106,7 @@ static int wacom_ac_get_property(struct power_supply *psy,
 
 static void wacom_poke(struct hid_device *hdev, u8 speed)
 {
+	struct wacom_data *wdata = hid_get_drvdata(hdev);
 	int limit, ret;
 	char rep_data[2];

@@ -128,8 +130,10 @@ static void wacom_poke(struct hid_device *hdev, u8 speed)
 					HID_FEATURE_REPORT);
 		} while (ret < 0 && limit-- > 0);
 
-		if (ret >= 0)
+		if (ret >= 0) {
+			wdata->speed = speed;
 			return;
+		}
 	}
 
 	/*

@@ -141,6 +145,33 @@ static void wacom_poke(struct hid_device *hdev, u8 speed)
 	return;
 }
 
+static ssize_t wacom_show_speed(struct device *dev,
+				struct device_attribute
+				*attr, char *buf)
+{
+	struct wacom_data *wdata = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%i\n", wdata->speed);
+}
+
+static ssize_t wacom_store_speed(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
+	int new_speed;
+
+	sscanf(buf, "%1d", &new_speed);
+	if (new_speed == 0 || new_speed == 1) {
+		wacom_poke(hdev, new_speed);
+		return strnlen(buf, PAGE_SIZE);
+	} else
+		return -EINVAL;
+}
+
+static DEVICE_ATTR(speed, S_IRUGO | S_IWUGO,
+		wacom_show_speed, wacom_store_speed);
+
 static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *raw_data, int size)
 {

@@ -297,6 +328,11 @@ static int wacom_probe(struct hid_device *hdev,
 		goto err_free;
 	}
 
+	ret = device_create_file(&hdev->dev, &dev_attr_speed);
+	if (ret)
+		dev_warn(&hdev->dev,
+			"can't create sysfs speed attribute err: %d\n", ret);
+
 	/* Set Wacom mode 2 with high reporting speed */
 	wacom_poke(hdev, 1);

Signature · 1 line

1.7.0.1

Bastien Nocera

2010-03-18 15:37 UTC #2 lore raw

Re: [PATCH 2/2] Add sysfs speed attribute for wacom bluetooth tablet

Quoted 1 line · Przemo Firszt

+ unsigned char speed;

Could you add some comments as to what values represent what speed?

drivers/hid/hid-wacom.c static void wacom_poke(struct hid_device *hdev, u8 speed) · line 164

Quoted 9 lines

drivers/hid/hid-wacom.c +5 -0

+static ssize_t wacom_store_speed(struct device *dev,
+                               struct device_attribute *attr,
+                               const char *buf, size_t count)
+{
+       struct hid_device *hdev = container_of(dev, struct hid_device,

dev);

drivers/hid/hid-wacom.c +3 -0

+       int new_speed;
+
+       sscanf(buf, "%1d", &new_speed);

You should check the retval of sscanf as well.

drivers/hid/hid-wacom.c static void wacom_poke(struct hid_device *hdev, u8 speed) · line 173

Quoted 10 lines

drivers/hid/hid-wacom.c +8 -0

+       if (new_speed == 0 || new_speed == 1) {
+               wacom_poke(hdev, new_speed);
+               return strnlen(buf, PAGE_SIZE);
+       } else
+               return -EINVAL;
+} 
 
 
+static DEVICE_ATTR(speed, S_IRUGO | S_IWUGO,
+               wacom_show_speed, wacom_store_speed);

If there's only 2 speeds available, and it's actually a boolean, I'd rather you used a name like "high_speed".

Furthermore, wdata->speed doesn't look like it's initialised.

Did you test whether speed switching works after the device has been started up?

Cheers

Przemo Firszt

author

2010-03-18 19:12 UTC #3 lore raw

Re: [PATCH 2/2] Add sysfs speed attribute for wacom bluetooth tablet

Dnia 2010-03-18, czw o godzinie 15:37 +0000, Bastien Nocera pisze:

Quoted 5 lines · Przemo Firszt

Quoted 2 lines
+ unsigned char speed;
Could you add some comments as to what values represent what speed?

I changed name from 'speed' to 'high_speed' as per your comment below, so I think there is no need (unless you think otherwise).

Quoted 12 lines

Review

Quoted 10 lines

(quoted diff) +5 -0

+static ssize_t wacom_store_speed(struct device *dev,
+                               struct device_attribute *attr,
+                               const char *buf, size_t count)
+{
+       struct hid_device *hdev = container_of(dev, struct hid_device,

dev);

(quoted diff) +3 -0

+       int new_speed;
+
+       sscanf(buf, "%1d", &new_speed);

You should check the retval of sscanf as well.

Done, see attached patch.

Quoted 8 lines

(quoted diff) +6 -0

+       if (new_speed == 0 || new_speed == 1) {
+               wacom_poke(hdev, new_speed);
+               return strnlen(buf, PAGE_SIZE);
+       } else
+               return -EINVAL;
+}

Quoted 7 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+static DEVICE_ATTR(speed, S_IRUGO | S_IWUGO,
+               wacom_show_speed, wacom_store_speed);
If there's only 2 speeds available, and it's actually a boolean, I'd rather you used a name like "high_speed".
Furthermore, wdata->speed doesn't look like it's initialised.

It is initialised by the very first call of wacom_poke from wacom_probe function. Can you advise if it's enough?

Quoted 3 lines

Did you test whether speed switching works after the device has been started up?

Yes, it works like a charm, both ways.
[przemo@pldmachine ~]$ echo 0 > /sys/class/hidraw/hidraw1/device/speed and lag is gone,
[przemo@pldmachine ~]$ echo 1 > /sys/class/hidraw/hidraw1/device/speed and "rubber band" effect is back.

Thanks for your comments,
Przemo

Quoted 1 line

From ec2c13649c3d7188199bfddc06b4211170c28b1c Mon Sep 17 00:00:00 2001

From: Przemo Firszt <[email protected]>
Date: Thu, 18 Mar 2010 14:34:34 +0000
Subject: [PATCH 2/2] Add sysfs speed attribute for wacom bluetooth tablet

Signed-off-by Przemo Firszt <[email protected]>

---
 drivers/hid/hid-wacom.c |   40 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 39 insertions(+), 1 deletions(-)

drivers/hid/hid-wacom.c +39 -1

index f9d4939..97ef626 100644
@@ -30,6 +30,7 @@
 struct wacom_data {
 	__u16 tool;
 	unsigned char butstate;
+	unsigned char high_speed;
 #ifdef CONFIG_HID_WACOM_POWER_SUPPLY
 	int battery_capacity;
 	struct power_supply battery;

@@ -105,6 +106,7 @@ static int wacom_ac_get_property(struct power_supply *psy,
 
 static void wacom_poke(struct hid_device *hdev, u8 speed)
 {
+	struct wacom_data *wdata = hid_get_drvdata(hdev);
 	int limit, ret;
 	char rep_data[2];

@@ -128,8 +130,10 @@ static void wacom_poke(struct hid_device *hdev, u8 speed)
 					HID_FEATURE_REPORT);
 		} while (ret < 0 && limit-- > 0);
 
-		if (ret >= 0)
+		if (ret >= 0) {
+			wdata->high_speed = speed;
 			return;
+		}
 	}
 
 	/*

@@ -141,6 +145,35 @@ static void wacom_poke(struct hid_device *hdev, u8 speed)
 	return;
 }
 
+static ssize_t wacom_show_speed(struct device *dev,
+				struct device_attribute
+				*attr, char *buf)
+{
+	struct wacom_data *wdata = dev_get_drvdata(dev);
+
+	return snprintf(buf, PAGE_SIZE, "%i\n", wdata->high_speed);
+}
+
+static ssize_t wacom_store_speed(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct hid_device *hdev = container_of(dev, struct hid_device, dev);
+	int new_speed;
+
+	if (sscanf(buf, "%1d", &new_speed ) != 1)
+		return -EINVAL;
+
+	if (new_speed == 0 || new_speed == 1) {
+		wacom_poke(hdev, new_speed);
+		return strnlen(buf, PAGE_SIZE);
+	} else
+		return -EINVAL;
+}
+
+static DEVICE_ATTR(speed, S_IRUGO | S_IWUGO,
+		wacom_show_speed, wacom_store_speed);
+
 static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *raw_data, int size)
 {

@@ -297,6 +330,11 @@ static int wacom_probe(struct hid_device *hdev,
 		goto err_free;
 	}
 
+	ret = device_create_file(&hdev->dev, &dev_attr_speed);
+	if (ret)
+		dev_warn(&hdev->dev,
+			"can't create sysfs speed attribute err: %d\n", ret);
+
 	/* Set Wacom mode 2 with high reporting speed */
 	wacom_poke(hdev, 1);

Signature · 1 line

1.7.0.1

Jiri Kosina

2010-03-22 08:48 UTC #4 lore raw

Re: [PATCH 2/2] Add sysfs speed attribute for wacom bluetooth tablet

Quoted 38 lines · Przemo Firszt

Quoted 5 lines · Przemo Firszt
Quoted 2 lines
+ unsigned char speed;
Could you add some comments as to what values represent what speed?
I changed name from 'speed' to 'high_speed' as per your comment below, so I think there is no need (unless you think otherwise).
Quoted 12 lines
drivers/hid/hid-wacom.c static void wacom_poke(struct hid_device *hdev, u8 speed)
Quoted 10 lines
drivers/hid/hid-wacom.c +5 -0
+static ssize_t wacom_store_speed(struct device *dev,
+                               struct device_attribute *attr,
+                               const char *buf, size_t count)
+{
+       struct hid_device *hdev = container_of(dev, struct hid_device,
dev);
drivers/hid/hid-wacom.c +3 -0
+       int new_speed;
+
+       sscanf(buf, "%1d", &new_speed);
You should check the retval of sscanf as well.
Done, see attached patch.
Quoted 8 lines
Quoted 8 lines
drivers/hid/hid-wacom.c +6 -0
+       if (new_speed == 0 || new_speed == 1) {
+               wacom_poke(hdev, new_speed);
+               return strnlen(buf, PAGE_SIZE);
+       } else
+               return -EINVAL;
+} 
Quoted 7 lines
drivers/hid/hid-wacom.c static void wacom_poke(struct hid_device *hdev, u8 speed) · line 175
Quoted 2 lines
drivers/hid/hid-wacom.c +2 -0
+static DEVICE_ATTR(speed, S_IRUGO | S_IWUGO,
+               wacom_show_speed, wacom_store_speed);
If there's only 2 speeds available, and it's actually a boolean, I'd rather you used a name like "high_speed".
Furthermore, wdata->speed doesn't look like it's initialised.
It is initialised by the very first call of wacom_poke from wacom_probe function. Can you advise if it's enough?
Quoted 1 line

Quoted 7 lines

Quoted 2 lines
Did you test whether speed switching works after the device has been started up?
Yes, it works like a charm, both ways.
[przemo@pldmachine ~]$ echo 0 > /sys/class/hidraw/hidraw1/device/speed and lag is gone,
[przemo@pldmachine ~]$ echo 1 > /sys/class/hidraw/hidraw1/device/speed and "rubber band" effect is back.

The patch looks fine to me, so I have applied it to my tree.

Could you also please send a followup patch which would add Documentation/ABI entry for the speed attribute?

Thanks,

Signature · 2 lines

Jiri Kosina
SUSE Labs, Novell Inc.

Przemo Firszt

author

2010-04-22 19:51 UTC #5 lore raw

Re: [PATCH 2/2] Add sysfs speed attribute for wacom bluetooth tablet

Dnia 2010-03-22, pon o godzinie 09:48 +0100, Jiri Kosina pisze:
[..]

Quoted 4 lines

The patch looks fine to me, so I have applied it to my tree.
Could you also please send a followup patch which would add Documentation/ABI entry for the speed attribute?

Hi Jiri,
Sorry for the delayed reply. See attached patch for ABI entry. Is Kernel Version: 2.6.35 OK?

Thanks,
Przemo

Quoted 1 line

From f118300739d2592646ddb5d58b47df59db002873 Mon Sep 17 00:00:00 2001

From: Przemo Firszt <[email protected]>
Date: Wed, 21 Apr 2010 22:16:25 +0100
Subject: [PATCH] Add Documentation/ABI entry for wacom reporting speed attribute.

Signed-off-by Przemo Firszt <[email protected]>

---
 Documentation/ABI/testing/sysfs-wacom |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-wacom

Documentation/ABI/testing/sysfs-wacom new file +10 -0

new file mode 100644
index 0000000..1517976
@@ -0,0 +1,10 @@
+What:		/sys/class/hidraw/hidraw*/device/speed
+Date:		April 2010
+Kernel Version:	2.6.35
+Contact:	[email protected]
+Description:
+		The /sys/class/hidraw/hidraw*/device/speed file controls
+		reporting speed of wacom bluetooth tablet. Reading from
+		this file returns 1 if tablet reports in high speed mode
+		or 0 otherwise. Writing to this file one of these values
+		switches reporting speed.

Signature · 1 line

1.7.0.2

Jiri Kosina

Applied

2010-04-23 00:16 UTC #6 lore raw

Re: [PATCH 2/2] Add sysfs speed attribute for wacom bluetooth tablet

Quoted 6 lines · Przemo Firszt

Quoted 4 lines
The patch looks fine to me, so I have applied it to my tree.
Could you also please send a followup patch which would add Documentation/ABI entry for the speed attribute?
Hi Jiri,
Sorry for the delayed reply. See attached patch for ABI entry.

Applied.

Quoted 1 line

Is Kernel Version: 2.6.35 OK?

Yup.

Thanks,

Signature · 2 lines

Jiri Kosina
SUSE Labs, Novell Inc.

Bastien Nocera

2010-03-18 15:39 UTC #7 lore raw

In reply to Przemo Firszt

Quoted 3 lines · Przemo Firszt

Hi,
Two patches to allow switching of reporting speed of wacom bluetooth tablet from userspace through sysfs.

Signed-off-by Bastien Nocera <[email protected]>

Cheers

Jiri Kosina

Applied

2010-03-22 08:47 UTC #8 lore raw

Quoted 4 lines · Bastien Nocera

Quoted 2 lines
Two patches to allow switching of reporting speed of wacom bluetooth tablet from userspace through sysfs.
Signed-off-by Bastien Nocera <[email protected]>

Applied, thanks.

Signature · 2 lines

Jiri Kosina
SUSE Labs, Novell Inc.