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

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

[Przemo Firszt]

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

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

[-- Attachment #2: 0001-Add-sysfs-battery-speed-attributes-for-wacom-bluetoo.patch --]
[-- Type: text/x-patch, Size: 5240 bytes --]

>From d61bb3eb0fd8109c235ade904a8d7184fc96b68a Mon Sep 17 00:00:00 2001
From: Przemo Firszt <przemo@firszt.eu>
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 <przemo@firszt.eu>
Acked-by: Ping Cheng <pingc@wacom.com>
---
 drivers/hid/hid-wacom.c |  113 +++++++++++++++++++++++++++++++++++++----------
 1 files changed, 89 insertions(+), 24 deletions(-)

diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index 8d3b46f..ce2782c 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -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;
-- 
1.6.6.1

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

[Bastien Nocera]

Hey Przemo,

On Sun, 2010-02-28 at 20:39 +0000, Przemo Firszt wrote:
> 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.

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)?

> 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

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

[Bastien Nocera]

On Tue, 2010-03-02 at 12:02 +0000, Bastien Nocera wrote:
> Hey Przemo,
> 
> On Sun, 2010-02-28 at 20:39 +0000, Przemo Firszt wrote:
> > 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.
> 
> A couple of comments:
<snip>
> - 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

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

[Jiri Kosina]

On Sun, 28 Feb 2010, Przemo Firszt wrote:

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

Hi Przemo,

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

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Przemo Firszt]

Dnia 2010-03-08, pon o godzinie 12:07 +0100, Jiri Kosina pisze:
> On Sun, 28 Feb 2010, Przemo Firszt wrote:
> 
> > 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.
> 
> 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

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

[Przemo Firszt]

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

Dnia 2010-03-02, wto o godzinie 12:11 +0000, Bastien Nocera pisze:
[..]
> > A couple of comments:
> <snip>
> > - 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

[-- Attachment #2: 0001-Expose-wacom-pen-tablet-battery-and-ac-thru-power_su.patch --]
[-- Type: text/x-patch, Size: 5490 bytes --]

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

diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index 8d3b46f..e5fa473 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -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));
 }
 
-- 
1.7.0.1

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

[Przemo Firszt]

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

Please ignore previous patch
--
Przemo

[-- Attachment #2: 0001-Expose-wacom-pen-tablet-battery-and-ac-thru-power_su.patch --]
[-- Type: text/x-patch, Size: 4655 bytes --]

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

diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index 8d3b46f..9fc2898 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -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));
 }
 
-- 
1.7.0.1

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

[Jiri Kosina]

On Tue, 9 Mar 2010, Przemo Firszt wrote:

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

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Przemo Firszt]

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

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

[-- Attachment #2: 0001-Expose-wacom-pen-tablet-battery-and-ac-thru-power_su.patch --]
[-- Type: text/x-patch, Size: 4979 bytes --]

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

diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index 8d3b46f..5c9cfe4 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -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));
 }
 
-- 
1.7.0.1

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

[Jiri Kosina]

On Wed, 10 Mar 2010, Przemo Firszt wrote:

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

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Przemo Firszt]

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

Dnia 2010-03-15, pon o godzinie 14:54 +0100, Jiri Kosina pisze:
> On Wed, 10 Mar 2010, Przemo Firszt wrote:
[..]
> > 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



[-- Attachment #2: 0001-Expose-wacom-pen-tablet-battery-and-ac-thru-power_su.patch --]
[-- Type: text/x-patch, Size: 5608 bytes --]

>From d6ec58bc114bdf0dd01bc0407f5f7bbb8161e45e Mon Sep 17 00:00:00 2001
From: Przemo Firszt <przemo@firszt.eu>
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 <przemo@firszt.eu>
---
 drivers/hid/Kconfig     |    8 +++
 drivers/hid/hid-wacom.c |  127 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 135 insertions(+), 0 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 71d4c07..8e1b505 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -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
diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index 8d3b46f..4d2d2a2 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -21,14 +21,88 @@
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/module.h>
+#ifdef CONFIG_HID_WACOM_POWER_SUPPLY
+#include <linux/power_supply.h>
+#endif
 
 #include "hid-ids.h"
 
 struct wacom_data {
 	__u16 tool;
 	unsigned char butstate;
+#ifdef CONFIG_HID_WACOM_POWER_SUPPLY
+	int battery_capacity;
+	struct power_supply battery;
+	struct power_supply ac;
+#endif
 };
 
+#ifdef CONFIG_HID_WACOM_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 +221,12 @@ static int wacom_raw_event(struct hid_device *hdev, struct hid_report *report,
 		input_sync(input);
 	}
 
+#ifdef CONFIG_HID_WACOM_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 +286,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 CONFIG_HID_WACOM_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 +369,15 @@ err_free:
 
 static void wacom_remove(struct hid_device *hdev)
 {
+#ifdef CONFIG_HID_WACOM_POWER_SUPPLY
+	struct wacom_data *wdata = hid_get_drvdata(hdev);
+#endif
 	hid_hw_stop(hdev);
+
+#ifdef CONFIG_HID_WACOM_POWER_SUPPLY
+	power_supply_unregister(&wdata->battery);
+	power_supply_unregister(&wdata->ac);
+#endif
 	kfree(hid_get_drvdata(hdev));
 }
 
-- 
1.7.0.1

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

[Jiri Kosina]

On Mon, 15 Mar 2010, Przemo Firszt wrote:

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

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

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

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

[Przemo Firszt]

Dnia 2010-03-16, wto o godzinie 11:49 +0100, Jiri Kosina pisze:
[..]
> I have applied it, thanks.
Thanks!
> One question still though ..
> 
> > +       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?
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