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

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

[Przemo Firszt]

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

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

kind reagrds,
Przemo

[-- Attachment #2: 0001-Add-separate-mode-switching-function-wacom-bluetooth.patch --]
[-- Type: text/x-patch, Size: 2996 bytes --]

>From ac9a87b92cf2f8cc27a1d29123c5fa1a8bebd71a Mon Sep 17 00:00:00 2001
From: Przemo Firszt <przemo@firszt.eu>
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 <przemo@firszt.eu>
---
 drivers/hid/hid-wacom.c |   66 ++++++++++++++++++++++++++++------------------
 1 files changed, 40 insertions(+), 26 deletions(-)

diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index 4d2d2a2..f9d4939 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -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;
-- 
1.7.0.1

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

[Przemo Firszt]

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

Patch 2/2
--
Przemo


[-- Attachment #2: 0002-Add-sysfs-speed-attribute-for-wacom-bluetooth-tablet.patch --]
[-- Type: text/x-patch, Size: 2735 bytes --]

>From 92b0a06c748ea0a853c369595444dd411b32f314 Mon Sep 17 00:00:00 2001
From: Przemo Firszt <przemo@firszt.eu>
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 <przemo@firszt.eu>
---
 drivers/hid/hid-wacom.c |   38 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index f9d4939..34d96e0 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -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);
 
-- 
1.7.0.1

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

[Bastien Nocera]

On Thu, 2010-03-18 at 15:26 +0000, Przemo Firszt wrote:

> +       unsigned char speed;

Could you add some comments as to what values represent what 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);

You should check the retval of sscanf as well.

> +       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

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

[Przemo Firszt]

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

Dnia 2010-03-18, czw o godzinie 15:37 +0000, Bastien Nocera pisze:
> On Thu, 2010-03-18 at 15:26 +0000, Przemo Firszt wrote:
> 
> > +       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). 
> 
> > +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);
> 
> You should check the retval of sscanf as well.
Done, see attached patch.
> > +       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.
It is initialised by the very first call of wacom_poke from wacom_probe
function. Can you advise if it's enough?
> 
> 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


[-- Attachment #2: 0002-Add-sysfs-speed-attribute-for-wacom-bluetooth-tablet.patch --]
[-- Type: text/x-patch, Size: 2783 bytes --]

>From ec2c13649c3d7188199bfddc06b4211170c28b1c Mon Sep 17 00:00:00 2001
From: Przemo Firszt <przemo@firszt.eu>
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 <przemo@firszt.eu>
---
 drivers/hid/hid-wacom.c |   40 +++++++++++++++++++++++++++++++++++++++-
 1 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index f9d4939..97ef626 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -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);
 
-- 
1.7.0.1

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

[Jiri Kosina]

On Thu, 18 Mar 2010, Przemo Firszt wrote:

> > On Thu, 2010-03-18 at 15:26 +0000, Przemo Firszt wrote:
> > 
> > > +       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). 
> > 
> > > +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);
> > 
> > You should check the retval of sscanf as well.
> Done, see attached patch.
> > > +       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.
> It is initialised by the very first call of wacom_poke from wacom_probe
> function. Can you advise if it's enough?
> > 
> > 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,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Przemo Firszt]

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

Dnia 2010-03-22, pon o godzinie 09:48 +0100, Jiri Kosina pisze:
[..]
> 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

[-- Attachment #2: 0001-Add-Documentation-ABI-entry-for-wacom-reporting-spee.patch --]
[-- Type: text/x-patch, Size: 1088 bytes --]

>From f118300739d2592646ddb5d58b47df59db002873 Mon Sep 17 00:00:00 2001
From: Przemo Firszt <przemo@firszt.eu>
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 <przemo@firszt.eu>
---
 Documentation/ABI/testing/sysfs-wacom |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-wacom

diff --git a/Documentation/ABI/testing/sysfs-wacom b/Documentation/ABI/testing/sysfs-wacom
new file mode 100644
index 0000000..1517976
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-wacom
@@ -0,0 +1,10 @@
+What:		/sys/class/hidraw/hidraw*/device/speed
+Date:		April 2010
+Kernel Version:	2.6.35
+Contact:	linux-bluetooth@vger.kernel.org
+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.
-- 
1.7.0.2

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

[Jiri Kosina]

On Thu, 22 Apr 2010, Przemo Firszt wrote:

> > 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.

> Is Kernel Version: 2.6.35 OK?

Yup.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 1/2] Add separate mode switching function wacom bluetooth pen tablet.

[Bastien Nocera]

On Thu, 2010-03-18 at 15:25 +0000, Przemo Firszt wrote:
> Hi,
> Two patches to allow switching of reporting speed of wacom bluetooth
> tablet from userspace through sysfs. 

Signed-off-by: Bastien Nocera <hadess@hadess.net>

Cheers

Re: [PATCH 1/2] Add separate mode switching function wacom bluetooth pen tablet.

[Jiri Kosina]

On Thu, 18 Mar 2010, Bastien Nocera wrote:

> > Two patches to allow switching of reporting speed of wacom bluetooth
> > tablet from userspace through sysfs. 
> 
> Signed-off-by: Bastien Nocera <hadess@hadess.net>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.