Skip to contentSkip to conversation

Add sysfs battery & speed attributes for wacom bluetooth tablet

PATCH In review Przemo Firszt 3 participants 13 messages Feb 28, 2010 – Mar 16, 2010

Subsystems (2)
Bluetooth drivers HID
lore
Share & tools
lore

Przemo Firszt

author
#0 lore raw

PATCH 1/1 Add sysfs battery & speed attributes for wacom bluetooth tablet

Hi,
I need your opinion if reporting battery condition/changing reporting speed of a bluetooth device through sysfs is an acceptable practice.

[PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet

The patch creates 2 sysfs attributes:
The battery attribute is read-only and it appears in:
/sys/bus/hid/devices/{btaddr}/battery /sys/class/bluetooth/hci*:*/{btaddr}/battery /sys/class/hidraw/hidraw*/device/battery
Capacity values are in %, zero value means AC plug is connected.

The speed attribute allows to poke reporting speed of wacom tablet. This attribute is RW, valid values are: 5 for low speed, 6 is high speed. High speed is the default value. Using low speed is a workaround if you experience big delay between move of stylus on the tablet and move of cursor on screen.

--
Kind regards,
Przemo

Quoted 1 line

From d61bb3eb0fd8109c235ade904a8d7184fc96b68a Mon Sep 17 00:00:00 2001

From: Przemo Firszt <[email protected]>
Date: Sat, 20 Feb 2010 12:58:41 +0000
Subject: [PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet

The patch creates 2 sysfs attributes:
The battery attribute is read-only and it appears in:
/sys/bus/hid/devices/{btaddr}/battery /sys/class/bluetooth/hci*:*/{btaddr}/battery /sys/class/hidraw/hidraw*/device/battery
Capacity values are in %, zero value means AC plug is connected.

The speed attribute allows to poke reporting speed of wacom tablet. This attribute is RW, valid values are: 5 for low speed, 6 is high speed. High speed is the default value. Using low speed is a workaround if you experience big delay between move of stylus on the tablet and move of cursor on screen.

Signed-off-by Przemo Firszt <[email protected]>
Acked-by Ping Cheng <[email protected]>
---
 drivers/hid/hid-wacom.c |  113 +++++++++++++++++++++++++++++++++++++----------
 1 files changed, 89 insertions(+), 24 deletions(-)
drivers/hid/hid-wacom.c +89 -24
index 8d3b46f..ce2782c 100644
@@ -24,11 +24,86 @@
 
 #include "hid-ids.h"
 
+#define WACOM_SPEED_LO	0X05
+#define WACOM_SPEED_HI	0x06
+
 struct wacom_data {
 	__u16 tool;
 	unsigned char butstate;
+	unsigned char battery;
 };
 
+/* Battery capacity, 0 value means AC connected */
+static unsigned short batcap[8] = { 1, 15, 25, 35, 50, 70, 100, 0 };
+
+static void wacom_poke(struct hid_device *hdev, u8 data)
+{
+	struct wacom_data *wdata = hid_get_drvdata(hdev);
+	int limit;
+	int ret;
+	char rep_data[2];
+
+	/*
+	 * Note that if the raw queries fail, it's not a hard failure and it
+	 * is safe to continue
+	 */
+	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)
+		rep_data[0] = data ; 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) {
+		wdata->speed = data;
+		return;
+	}
+
+	dev_warn(&hdev->dev, "failed to poke device, command %d, err %d\n",
+				data, ret);
+}
+
+static ssize_t wacom_show_battery(struct device *dev, struct device_attribute
+				*attr, char *buf)
+{
+	struct wacom_data *wdata = dev_get_drvdata(dev);
+	return snprintf(buf, PAGE_SIZE, "%i\n", batcap[wdata->battery]);
+}
+
+static DEVICE_ATTR(battery, S_IRUGO, wacom_show_battery, NULL);
+
+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 == WACOM_SPEED_HI || new_speed == WACOM_SPEED_LO) {
+		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)
 {

@@ -147,6 +222,11 @@ static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		input_sync(input);
 	}
 
+	/* Store current battery capacity */
+	rw = (data[7] >> 2 & 0x07);
+	if (rw != wdata->battery)
+		wdata->battery = rw;
+
 	return 1;
 }
 

@@ -156,9 +236,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) {

@@ -181,30 +259,17 @@ 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
-	 */
+	ret = device_create_file(&hdev->dev, &dev_attr_battery);
+	if (ret)
+		dev_warn(&hdev->dev,
+			"can't create sysfs battery attribute err: %d\n", ret);
 
-	/* 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);
+	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);
 
-	/* 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);
+	wacom_poke(hdev, WACOM_SPEED_HI);
 
 	hidinput = list_entry(hdev->inputs.next, struct hid_input, list);
 	input = hidinput->input;
Signature · 1 line
1.6.6.1
#1 lore raw

Hey Przemo,

Quoted 4 lines · Przemo Firszt

Hi,
I need your opinion if reporting battery condition/changing reporting speed of a bluetooth device through sysfs is an acceptable practice.

Quoted 8 lines

[PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet

The patch creates 2 sysfs attributes:
The battery attribute is read-only and it appears in:
/sys/bus/hid/devices/{btaddr}/battery /sys/class/bluetooth/hci*:*/{btaddr}/battery /sys/class/hidraw/hidraw*/device/battery
Capacity values are in %, zero value means AC plug is connected.

A couple of comments:
- battery status and adapter status should probably be 2 separate values. I don't know what the hardware actually exports, but I hope to have access to the specs of the device soon, which should answer that question.
- isn't there a more kernel-y way to export that data, so that it's automatically picked up by things like upower (né DeviceKit-power)?

Quoted 5 lines

The speed attribute allows to poke reporting speed of wacom tablet. This attribute is RW, valid values are: 5 for low speed, 6 is high speed. High speed is the default value. Using low speed is a workaround if you experience big delay between move of stylus on the tablet and move of cursor on screen.

5 and 6 are magic values. I'd rather have a boolean "high_speed" attribute which would switch between high-speed and low-speed.

Finally, the patch should be split in at least 2 parts, one to add the sysfs infrastructure and one of the properties, and another one adding the property itself (maybe even 3, if we were to split the sysfs enablement from the properties themselves).

But that's a minor problem, and if the maintainers are happy taking a single patch for the features, then I'm happy as well :)

Cheers

#2 lore raw
Quoted 17 lines · Bastien Nocera

Hey Przemo,

Quoted 4 lines · Przemo Firszt

Hi,
I need your opinion if reporting battery condition/changing reporting speed of a bluetooth device through sysfs is an acceptable practice.

Quoted 8 lines

[PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet

The patch creates 2 sysfs attributes:
The battery attribute is read-only and it appears in:
/sys/bus/hid/devices/{btaddr}/battery /sys/class/bluetooth/hci*:*/{btaddr}/battery /sys/class/hidraw/hidraw*/device/battery
Capacity values are in %, zero value means AC plug is connected.

A couple of comments:

<snip>

Quoted 2 lines

- isn't there a more kernel-y way to export that data, so that it's automatically picked up by things like upower (né DeviceKit-power)?

I've been told it should use the power_supply class, so it would work pretty much out-of-the-box with things like upower and gnome-power-manager.

Cheers

author
#3 lore raw

PATCH 1/1 Expose wacom pen tablet battery and ac thru power_supply class

Dnia 2010-03-02, wto o godzinie 12:11 +0000, Bastien Nocera pisze:
[..]

Quoted 8 lines
Quoted 1 line

A couple of comments:

<snip>

Quoted 2 lines

- isn't there a more kernel-y way to export that data, so that it's automatically picked up by things like upower (né DeviceKit-power)?

I've been told it should use the power_supply class, so it would work pretty much out-of-the-box with things like upower and gnome-power-manager.

Thanks for the comments - it was exactly that what I was looking for! :-)
Please find attached revamped patch - this time battery & ac patch only. Speed switching isn't yet ready for release.

For some reason gnome-power-manager isn't updating the values properly, despite of that they are OK in:
/sys/class/power_supply/wacom_{ac,battery}/uevent

I'm not sure if it's a result of lack of .external_power_changed (it's missing in some drivers using power_supply) or something else. Any hints?

Cheers,
Przemo

Quoted 1 line

From c56101ee1fe781a46e43de930b94ee3397b08b69 Mon Sep 17 00:00:00 2001

From: Przemo Firszt <[email protected]>
Date: Fri, 5 Mar 2010 17:19:44 +0000
Subject: [PATCH] Expose wacom pen tablet battery and ac thru power_supply class

This patch exposes wacom pen tablet battery capacity and ac state thru power_supply class is sysfs.

Signed-off-by Przemo Firszt <[email protected]>
---
 drivers/hid/hid-wacom.c |  150 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 150 insertions(+), 0 deletions(-)
drivers/hid/hid-wacom.c +150 -0
index 8d3b46f..e5fa473 100644
@@ -21,14 +21,118 @@
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/module.h>
+#include <linux/power_supply.h>
 
 #include "hid-ids.h"
 
 struct wacom_data {
 	__u16 tool;
 	unsigned char butstate;
+	int battery_capacity;
+	struct power_supply battery;
+	struct power_supply ac;
 };
 
+/*percent of battery capacity, 0 means AC online*/
+static unsigned short batcap[8] = { 1, 15, 25, 35, 50, 70, 100, 0 };
+
+static enum power_supply_property wacom_battery_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_CAPACITY
+};
+
+static enum power_supply_property wacom_ac_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE
+};
+
+static int wacom_battery_get_property(struct power_supply *psy,
+				enum power_supply_property psp,
+				union power_supply_propval *val)
+{
+	struct wacom_data *wdata = container_of(psy,
+					struct wacom_data, battery);
+	int power_state = batcap[wdata->battery_capacity];
+	int ret = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = 1;
+		break;
+	case POWER_SUPPLY_PROP_CAPACITY:
+		/* show 100% battery capacity when charging */
+		if (power_state == 0)
+			val->intval = 100;
+		else
+			val->intval = power_state;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static int wacom_ac_get_property(struct power_supply *psy,
+				enum power_supply_property psp,
+				union power_supply_propval *val)
+{
+	struct wacom_data *wdata = container_of(psy, struct wacom_data, ac);
+	int power_state = batcap[wdata->battery_capacity];
+	int ret = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		/* fall through */
+	case POWER_SUPPLY_PROP_ONLINE:
+		if (power_state == 0)
+			val->intval = 1;
+		else
+			val->intval = 0;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static void wacom_poke(struct hid_device *hdev, u8 data)
+{
+	int limit;
+	int ret;
+	char rep_data[2];
+	/*
+	 * data has to be one of those:
+	 * 0x05 - low reporting speed
+	 * 0x06 - high reporting speed
+	 */
+
+	/*
+	 * Note that if the raw queries fail, it's not a hard failure and it
+	 * is safe to continue
+	 */
+	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) {
+		rep_data[0] = data ; 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, command %d, err %d\n"
+				, data, ret);
+	}
+
+}
+
 static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *raw_data, int size)
 {

@@ -147,6 +251,11 @@ static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		input_sync(input);
 	}
 
+	/* Store current battery capacity */
+	rw = (data[7] >> 2 & 0x07);
+	if (rw != wdata->battery_capacity)
+		wdata->battery_capacity = rw;
+
 	return 1;
 }
 

@@ -206,6 +315,43 @@ static int wacom_probe(struct hid_device *hdev,
 	if (ret < 0)
 		dev_warn(&hdev->dev, "failed to poke device #2, %d\n", ret);
 
+	wdata->battery.properties = wacom_battery_props;
+	wdata->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
+	wdata->battery.get_property = wacom_battery_get_property;
+	wdata->battery.name = "wacom_battery";
+	wdata->battery.type = POWER_SUPPLY_TYPE_BATTERY;
+	wdata->battery.use_for_apm = 0;
+
+	ret = power_supply_register(&hdev->dev, &wdata->battery);
+	if (ret) {
+		dev_warn(&hdev->dev,
+			"can't create sysfs battery attribute, err: %d\n", ret);
+		/*
+		 * battery attribute is not critical for the tablet, but if it
+		 * failed then there is no need to create ac attribute
+		 */
+		goto move_on;
+	}
+
+	wdata->ac.properties = wacom_ac_props;
+	wdata->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
+	wdata->ac.get_property = wacom_ac_get_property;
+	wdata->ac.name = "wacom_ac";
+	wdata->ac.type = POWER_SUPPLY_TYPE_MAINS;
+	wdata->ac.use_for_apm = 0;
+
+	ret = power_supply_register(&hdev->dev, &wdata->ac);
+	if (ret) {
+		dev_warn(&hdev->dev,
+			"can't create ac battery attribute, err: %d\n", ret);
+		/*
+		 * ac attribute is not critical for the tablet, but if it
+		 * failed then we don't want to battery attribute to exist
+		 */
+		power_supply_unregister(&wdata->battery);
+	}
+
+move_on:
 	hidinput = list_entry(hdev->inputs.next, struct hid_input, list);
 	input = hidinput->input;
 

@@ -250,7 +396,11 @@ err_free:
 
 static void wacom_remove(struct hid_device *hdev)
 {
+	struct wacom_data *wdata = hid_get_drvdata(hdev);
+
 	hid_hw_stop(hdev);
+	power_supply_unregister(&wdata->battery);
+	power_supply_unregister(&wdata->ac);
 	kfree(hid_get_drvdata(hdev));
 }
Signature · 1 line
1.7.0.1
author
#4 lore raw

Re: [PATCH] Expose wacom pen tablet battery and ac thru power_supply class

Please ignore previous patch
--
Przemo

Quoted 1 line

From 74b4e83f11b74d800850b659c81d64fe32fd8ca8 Mon Sep 17 00:00:00 2001

From: Przemo Firszt <[email protected]>
Date: Fri, 5 Mar 2010 17:19:44 +0000
Subject: [PATCH] Expose wacom pen tablet battery and ac thru power_supply class

This patch exposes wacom pen tablet battery capacity and ac state thru power_supply class is sysfs.

Signed-off-by Przemo Firszt <[email protected]>
---
 drivers/hid/hid-wacom.c |  114 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 114 insertions(+), 0 deletions(-)
drivers/hid/hid-wacom.c +114 -0
index 8d3b46f..9fc2898 100644
@@ -21,14 +21,82 @@
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/module.h>
+#include <linux/power_supply.h>
 
 #include "hid-ids.h"
 
 struct wacom_data {
 	__u16 tool;
 	unsigned char butstate;
+	int battery_capacity;
+	struct power_supply battery;
+	struct power_supply ac;
 };
 
+/*percent of battery capacity, 0 means AC online*/
+static unsigned short batcap[8] = { 1, 15, 25, 35, 50, 70, 100, 0 };
+
+static enum power_supply_property wacom_battery_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_CAPACITY
+};
+
+static enum power_supply_property wacom_ac_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE
+};
+
+static int wacom_battery_get_property(struct power_supply *psy,
+				enum power_supply_property psp,
+				union power_supply_propval *val)
+{
+	struct wacom_data *wdata = container_of(psy,
+					struct wacom_data, battery);
+	int power_state = batcap[wdata->battery_capacity];
+	int ret = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = 1;
+		break;
+	case POWER_SUPPLY_PROP_CAPACITY:
+		/* show 100% battery capacity when charging */
+		if (power_state == 0)
+			val->intval = 100;
+		else
+			val->intval = power_state;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static int wacom_ac_get_property(struct power_supply *psy,
+				enum power_supply_property psp,
+				union power_supply_propval *val)
+{
+	struct wacom_data *wdata = container_of(psy, struct wacom_data, ac);
+	int power_state = batcap[wdata->battery_capacity];
+	int ret = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		/* fall through */
+	case POWER_SUPPLY_PROP_ONLINE:
+		if (power_state == 0)
+			val->intval = 1;
+		else
+			val->intval = 0;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
 static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *raw_data, int size)
 {

@@ -147,6 +215,11 @@ static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		input_sync(input);
 	}
 
+	/* Store current battery capacity */
+	rw = (data[7] >> 2 & 0x07);
+	if (rw != wdata->battery_capacity)
+		wdata->battery_capacity = rw;
+
 	return 1;
 }
 

@@ -206,6 +279,43 @@ static int wacom_probe(struct hid_device *hdev,
 	if (ret < 0)
 		dev_warn(&hdev->dev, "failed to poke device #2, %d\n", ret);
 
+	wdata->battery.properties = wacom_battery_props;
+	wdata->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
+	wdata->battery.get_property = wacom_battery_get_property;
+	wdata->battery.name = "wacom_battery";
+	wdata->battery.type = POWER_SUPPLY_TYPE_BATTERY;
+	wdata->battery.use_for_apm = 0;
+
+	ret = power_supply_register(&hdev->dev, &wdata->battery);
+	if (ret) {
+		dev_warn(&hdev->dev,
+			"can't create sysfs battery attribute, err: %d\n", ret);
+		/*
+		 * battery attribute is not critical for the tablet, but if it
+		 * failed then there is no need to create ac attribute
+		 */
+		goto move_on;
+	}
+
+	wdata->ac.properties = wacom_ac_props;
+	wdata->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
+	wdata->ac.get_property = wacom_ac_get_property;
+	wdata->ac.name = "wacom_ac";
+	wdata->ac.type = POWER_SUPPLY_TYPE_MAINS;
+	wdata->ac.use_for_apm = 0;
+
+	ret = power_supply_register(&hdev->dev, &wdata->ac);
+	if (ret) {
+		dev_warn(&hdev->dev,
+			"can't create ac battery attribute, err: %d\n", ret);
+		/*
+		 * ac attribute is not critical for the tablet, but if it
+		 * failed then we don't want to battery attribute to exist
+		 */
+		power_supply_unregister(&wdata->battery);
+	}
+
+move_on:
 	hidinput = list_entry(hdev->inputs.next, struct hid_input, list);
 	input = hidinput->input;
 

@@ -250,7 +360,11 @@ err_free:
 
 static void wacom_remove(struct hid_device *hdev)
 {
+	struct wacom_data *wdata = hid_get_drvdata(hdev);
+
 	hid_hw_stop(hdev);
+	power_supply_unregister(&wdata->battery);
+	power_supply_unregister(&wdata->ac);
 	kfree(hid_get_drvdata(hdev));
 }
Signature · 1 line
1.7.0.1
#5 lore raw

Re: [PATCH] Expose wacom pen tablet battery and ac thru power_supply class

Quoted 1 line · Przemo Firszt

Please ignore previous patch

Hi Prezemo,

thanks for basing the patch on power_supply infrastructure.

Anyway, you'll have to sort out the new dependency on CONFIG_POWER_SUPPLY somehow (compiling the battery code out from the driver if CONFIG_POWER_SUPPLY is unset, or selecting it directly from Kconfig).

Thanks,

Signature · 2 lines
Jiri Kosina
SUSE Labs, Novell Inc.
author
#6 lore raw

Re: [PATCH] Expose wacom pen tablet battery and ac thru power_supply class

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

Quoted 3 lines

Anyway, you'll have to sort out the new dependency on CONFIG_POWER_SUPPLY somehow (compiling the battery code out from the driver if CONFIG_POWER_SUPPLY is unset, or selecting it directly from Kconfig).

Thanks for checking the patch.
See attached updated version - is it OK?
Does it make sense to add a line to Kconfig to explain that CONFIG_POWER_SUPPLY/CONFIG_POWER_SUPPLY_MODULE is required to enable monitoring battery/ac state?

Cheers,
Przemo

Quoted 1 line

From f5eb6397ebd3a001289f676383df5adaaa17f8ea Mon Sep 17 00:00:00 2001

From: Przemo Firszt <[email protected]>
Date: Fri, 5 Mar 2010 17:19:44 +0000
Subject: [PATCH] Expose wacom pen tablet battery and ac thru power_supply class

This patch exposes wacom pen tablet battery capacity and ac state thru power_supply class is sysfs.

Signed-off-by Przemo Firszt <[email protected]>
---
 drivers/hid/hid-wacom.c |  128 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 128 insertions(+), 0 deletions(-)
drivers/hid/hid-wacom.c +128 -0
index 8d3b46f..5c9cfe4 100644
@@ -21,14 +21,89 @@
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/module.h>
+#if defined(CONFIG_POWER_SUPPLY) || defined(CONFIG_POWER_SUPPLY_MODULE)
+#define ENABLE_POWER_SUPPLY
+#include <linux/power_supply.h>
+#endif
 
 #include "hid-ids.h"
 
 struct wacom_data {
 	__u16 tool;
 	unsigned char butstate;
+#ifdef ENABLE_POWER_SUPPLY
+	int battery_capacity;
+	struct power_supply battery;
+	struct power_supply ac;
+#endif
 };
 
+#ifdef ENABLE_POWER_SUPPLY
+/*percent of battery capacity, 0 means AC online*/
+static unsigned short batcap[8] = { 1, 15, 25, 35, 50, 70, 100, 0 };
+
+static enum power_supply_property wacom_battery_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_CAPACITY
+};
+
+static enum power_supply_property wacom_ac_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE
+};
+
+static int wacom_battery_get_property(struct power_supply *psy,
+				enum power_supply_property psp,
+				union power_supply_propval *val)
+{
+	struct wacom_data *wdata = container_of(psy,
+					struct wacom_data, battery);
+	int power_state = batcap[wdata->battery_capacity];
+	int ret = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = 1;
+		break;
+	case POWER_SUPPLY_PROP_CAPACITY:
+		/* show 100% battery capacity when charging */
+		if (power_state == 0)
+			val->intval = 100;
+		else
+			val->intval = power_state;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static int wacom_ac_get_property(struct power_supply *psy,
+				enum power_supply_property psp,
+				union power_supply_propval *val)
+{
+	struct wacom_data *wdata = container_of(psy, struct wacom_data, ac);
+	int power_state = batcap[wdata->battery_capacity];
+	int ret = 0;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		/* fall through */
+	case POWER_SUPPLY_PROP_ONLINE:
+		if (power_state == 0)
+			val->intval = 1;
+		else
+			val->intval = 0;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+#endif
+
 static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		u8 *raw_data, int size)
 {

@@ -147,6 +222,12 @@ static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		input_sync(input);
 	}
 
+#ifdef ENABLE_POWER_SUPPLY
+	/* Store current battery capacity */
+	rw = (data[7] >> 2 & 0x07);
+	if (rw != wdata->battery_capacity)
+		wdata->battery_capacity = rw;
+#endif
 	return 1;
 }
 

@@ -206,6 +287,45 @@ static int wacom_probe(struct hid_device *hdev,
 	if (ret < 0)
 		dev_warn(&hdev->dev, "failed to poke device #2, %d\n", ret);
 
+#ifdef ENABLE_POWER_SUPPLY
+	wdata->battery.properties = wacom_battery_props;
+	wdata->battery.num_properties = ARRAY_SIZE(wacom_battery_props);
+	wdata->battery.get_property = wacom_battery_get_property;
+	wdata->battery.name = "wacom_battery";
+	wdata->battery.type = POWER_SUPPLY_TYPE_BATTERY;
+	wdata->battery.use_for_apm = 0;
+
+	ret = power_supply_register(&hdev->dev, &wdata->battery);
+	if (ret) {
+		dev_warn(&hdev->dev,
+			"can't create sysfs battery attribute, err: %d\n", ret);
+		/*
+		 * battery attribute is not critical for the tablet, but if it
+		 * failed then there is no need to create ac attribute
+		 */
+		goto move_on;
+	}
+
+	wdata->ac.properties = wacom_ac_props;
+	wdata->ac.num_properties = ARRAY_SIZE(wacom_ac_props);
+	wdata->ac.get_property = wacom_ac_get_property;
+	wdata->ac.name = "wacom_ac";
+	wdata->ac.type = POWER_SUPPLY_TYPE_MAINS;
+	wdata->ac.use_for_apm = 0;
+
+	ret = power_supply_register(&hdev->dev, &wdata->ac);
+	if (ret) {
+		dev_warn(&hdev->dev,
+			"can't create ac battery attribute, err: %d\n", ret);
+		/*
+		 * ac attribute is not critical for the tablet, but if it
+		 * failed then we don't want to battery attribute to exist
+		 */
+		power_supply_unregister(&wdata->battery);
+	}
+
+move_on:
+#endif
 	hidinput = list_entry(hdev->inputs.next, struct hid_input, list);
 	input = hidinput->input;
 

@@ -250,7 +370,15 @@ err_free:
 
 static void wacom_remove(struct hid_device *hdev)
 {
+#ifdef ENABLE_POWER_SUPPLY
+	struct wacom_data *wdata = hid_get_drvdata(hdev);
+#endif
 	hid_hw_stop(hdev);
+
+#ifdef ENABLE_POWER_SUPPLY
+	power_supply_unregister(&wdata->battery);
+	power_supply_unregister(&wdata->ac);
+#endif
 	kfree(hid_get_drvdata(hdev));
 }
Signature · 1 line
1.7.0.1
#7 lore raw

Re: [PATCH] Expose wacom pen tablet battery and ac thru power_supply class

Quoted 8 lines · Przemo Firszt
Quoted 3 lines

Anyway, you'll have to sort out the new dependency on CONFIG_POWER_SUPPLY somehow (compiling the battery code out from the driver if CONFIG_POWER_SUPPLY is unset, or selecting it directly from Kconfig).

Thanks for checking the patch.
See attached updated version - is it OK?
Does it make sense to add a line to Kconfig to explain that CONFIG_POWER_SUPPLY/CONFIG_POWER_SUPPLY_MODULE is required to enable monitoring battery/ac state?

Either that, or introducing separate CONFIG sub-option for the Wacom driver might be reasonable option as well.

Otherwise the patch looks good.

Thanks,

Signature · 2 lines
Jiri Kosina
SUSE Labs, Novell Inc.
author
#8 lore raw

Re: [PATCH] Expose wacom pen tablet battery and ac thru power_supply class

Dnia 2010-03-15, pon o godzinie 14:54 +0100, Jiri Kosina pisze:

Quoted 1 line · Przemo Firszt

[..]

Quoted 6 lines
Quoted 3 lines

Does it make sense to add a line to Kconfig to explain that CONFIG_POWER_SUPPLY/CONFIG_POWER_SUPPLY_MODULE is required to enable monitoring battery/ac state?

Either that, or introducing separate CONFIG sub-option for the Wacom driver might be reasonable option as well.

Hi Jiri,
Thanks for help - next time I'll know how to do it :-) I went the CONFIG sub-option way - see attached.

thanks,
Przemo

Quoted 1 line

From d6ec58bc114bdf0dd01bc0407f5f7bbb8161e45e Mon Sep 17 00:00:00 2001

From: Przemo Firszt <[email protected]>
Date: Mon, 15 Mar 2010 19:16:23 +0000
Subject: [PATCH] Expose wacom pen tablet battery and ac thru power_supply class

This patch exposes wacom pen tablet battery capacity and ac state thru power_supply class is sysfs.

Signed-off-by Przemo Firszt <[email protected]>
---
 drivers/hid/Kconfig     |    8 +++
 drivers/hid/hid-wacom.c |  127 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 135 insertions(+), 0 deletions(-)
drivers/hid/Kconfig +8 -0
index 71d4c07..8e1b505 100644
@@ -357,6 +357,14 @@ config HID_WACOM
 	---help---
 	Support for Wacom Graphire Bluetooth tablet.
 
+config HID_WACOM_POWER_SUPPLY
+	bool "Wacom Bluetooth devices power supply status support"
+	depends on HID_WACOM
+	select POWER_SUPPLY
+	---help---
+	  Say Y here if you want to enable power supply status monitoring for
+	  Wacom Bluetooth devices.
+
 config HID_ZEROPLUS
 	tristate "Zeroplus based game controller support" if EMBEDDED
 	depends on USB_HID
drivers/hid/hid-wacom.c +127 -0

160 lines not on this page · raw message

Signature · 1 line
1.7.0.1
Applied
#9 lore raw

Re: [PATCH] Expose wacom pen tablet battery and ac thru power_supply class

Quoted 9 lines · Przemo Firszt
Quoted 6 lines
Quoted 3 lines

Does it make sense to add a line to Kconfig to explain that CONFIG_POWER_SUPPLY/CONFIG_POWER_SUPPLY_MODULE is required to enable monitoring battery/ac state?

Either that, or introducing separate CONFIG sub-option for the Wacom driver might be reasonable option as well.

Hi Jiri,
Thanks for help - next time I'll know how to do it :-) I went the CONFIG sub-option way - see attached.

I have applied it, thanks.

One question still though ..

drivers/hid/hid-wacom.c line 73

Quoted 7 lines
drivers/hid/hid-wacom.c +7 -0
+       case POWER_SUPPLY_PROP_CAPACITY:
+               /* show 100% battery capacity when charging */
+               if (power_state == 0)
+                       val->intval = 100;
+               else
+                       val->intval = power_state;
+               break;

Why is it not possible to show the actual percentage in the charging state as well?

Thanks,

Signature · 2 lines
Jiri Kosina
SUSE Labs, Novell Inc.
author
#10 lore raw

Re: [PATCH] Expose wacom pen tablet battery and ac thru power_supply class

Dnia 2010-03-16, wto o godzinie 11:49 +0100, Jiri Kosina pisze:
[..]

Quoted 1 line

I have applied it, thanks.

Thanks!

Quoted 10 lines

One question still though ..

drivers/hid/hid-wacom.c line 73

Quoted 7 lines
drivers/hid/hid-wacom.c +7 -0
+       case POWER_SUPPLY_PROP_CAPACITY:
+               /* show 100% battery capacity when charging */
+               if (power_state == 0)
+                       val->intval = 100;
+               else
+                       val->intval = power_state;
+               break;
Quoted 2 lines

Why is it not possible to show the actual percentage in the charging state as well?

The device doesn't report capacity during charging as far as I can tell. We could keep last reported value (bad idea IMHO), set it to 0 (even worse) or set it to 100% - a bit misleading, but I can't see any other option.

cheers,
Przemo

#11 lore raw
In reply to Przemo Firszt
Quoted 13 lines · Przemo Firszt

Hi,
I need your opinion if reporting battery condition/changing reporting speed of a bluetooth device through sysfs is an acceptable practice.

[PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet

The patch creates 2 sysfs attributes:
The battery attribute is read-only and it appears in:
/sys/bus/hid/devices/{btaddr}/battery /sys/class/bluetooth/hci*:*/{btaddr}/battery /sys/class/hidraw/hidraw*/device/battery
Capacity values are in %, zero value means AC plug is connected.

Quoted 5 lines

The speed attribute allows to poke reporting speed of wacom tablet. This attribute is RW, valid values are: 5 for low speed, 6 is high speed. High speed is the default value. Using low speed is a workaround if you experience big delay between move of stylus on the tablet and move of cursor on screen.

Hi Przemo,

do you see any obstacle to using CONFIG_POWER_SUPPLY (see drivers/power) infrastructure for this?

Thanks,

Signature · 2 lines
Jiri Kosina
SUSE Labs, Novell Inc.
author
#12 lore raw

Dnia 2010-03-08, pon o godzinie 12:07 +0100, Jiri Kosina pisze:

Quoted 21 lines · Przemo Firszt
Quoted 14 lines

Hi,
I need your opinion if reporting battery condition/changing reporting speed of a bluetooth device through sysfs is an acceptable practice.

[PATCH] Add sysfs battery & speed attributes for wacom bluetooth tablet

The patch creates 2 sysfs attributes:
The battery attribute is read-only and it appears in:
/sys/bus/hid/devices/{btaddr}/battery /sys/class/bluetooth/hci*:*/{btaddr}/battery /sys/class/hidraw/hidraw*/device/battery
Capacity values are in %, zero value means AC plug is connected.

Quoted 5 lines

The speed attribute allows to poke reporting speed of wacom tablet. This attribute is RW, valid values are: 5 for low speed, 6 is high speed. High speed is the default value. Using low speed is a workaround if you experience big delay between move of stylus on the tablet and move of cursor on screen.

Quoted 4 lines

Hi Przemo,

do you see any obstacle to using CONFIG_POWER_SUPPLY (see drivers/power) infrastructure for this?

No, I don't. I'm very sorry for the delay, but I've been rather busy in last few days. I've got patch implementing usage of power_supply ready - I just wanted to send it together with switching of reporting speed. I'll clean it and send it ASAP (but not today :-( ).
--
Cheers,
Przemo