Supporting Battery Strength from my Bluetooth Mouse

Supporting Battery Strength from my Bluetooth Mouse

[Jeremy Fitzhardinge]

My Bluetooth mouse (HP branded, but some generic Taiwanese innards)
supports reporting its battery level.

The data appears in its HID descriptor as:

  INPUT(3)[INPUT]
    Field(0)
      Usage(1)
        0006.0020
      Logical Minimum(0)
      Logical Maximum(255)
      Report Size(8)
      Report Count(1)
      Report Offset(0)
      Flags( Variable Absolute )

which is consistent with the USB HID Usage Tables 1.12 spec, which lists
Usage Page 6 as Generic Device Controls and Usage ID 0x20 as Battery
Strength.  I'm assuming that this is commonplace on battery-powered
mice, but I only have one to check.

I've written an (untested) patch to display this correctly in debugfs.
(below)

My real question is how to get this out to somewhere useful, where
(ultimately) some GUI could show the battery information if its available?

Is the next step to define a new ABS_BATTERY_STRENGTH type for it (and
likewise for the other values, though I have no idea what the Security
stuff is about)?  Then at least it will appear in evdev in a useful way,
and becomes usermode's problem to deal with it.

(Hm, and I wonder what makes the mouse actually report the battery
level; I guess I'll only ever see a report if it actually changes.)

Any guidance?

Thanks,
    J

Subject: [PATCH] hid: decode Generic Device Controls Usage Page

The USB HID Usage Tables spec defines page 6 for Generic Device
Controls, the most useful of which (to me) is Battery Strength.

Signed-off-by: Jeremy Fitzhardinge <jeremy@goop.org>

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index ee80d73..01dd9a7 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -114,6 +114,14 @@ static const struct hid_usage_entry hid_usage_table[] = {
       {0, 0xbd, "FlareRelease"},
       {0, 0xbe, "LandingGear"},
       {0, 0xbf, "ToeBrake"},
+  {  6, 0, "GenericDeviceControls" },
+      {0, 0x20, "BatteryStrength" },
+      {0, 0x21, "WirelessChannel" },
+      {0, 0x22, "WirelessID" },
+      {0, 0x23, "DiscoverWirelessControl" },
+      {0, 0x24, "SecurityCodeCharacterEntered" },
+      {0, 0x25, "SecurityCodeCharactedErased" },
+      {0, 0x26, "SecurityCodeCleared" },
   {  7, 0, "Keyboard" },
   {  8, 0, "LED" },
       {0, 0x01, "NumLock"},

Re: Supporting Battery Strength from my Bluetooth Mouse

[Jiri Kosina]

On Fri, 18 Nov 2011, Jeremy Fitzhardinge wrote:

> My Bluetooth mouse (HP branded, but some generic Taiwanese innards)
> supports reporting its battery level.
> 
> The data appears in its HID descriptor as:
> 
>   INPUT(3)[INPUT]
>     Field(0)
>       Usage(1)
>         0006.0020
>       Logical Minimum(0)
>       Logical Maximum(255)
>       Report Size(8)
>       Report Count(1)
>       Report Offset(0)
>       Flags( Variable Absolute )
> 
> which is consistent with the USB HID Usage Tables 1.12 spec, which lists
> Usage Page 6 as Generic Device Controls and Usage ID 0x20 as Battery
> Strength.  I'm assuming that this is commonplace on battery-powered
> mice, but I only have one to check.
> 
> I've written an (untested) patch to display this correctly in debugfs.
> (below)

Hi Jeremy,

the debugfs patch is obviously correct(tm), so I will be queuing it, 
thanks.

> My real question is how to get this out to somewhere useful, where 
> (ultimately) some GUI could show the battery information if its 
> available?

Actually the proper way to do this is using power_supply class.

There alreasy is a HID driver that does this properly, and it's rather 
easy. Check drivers/hid/hid-wacom.c, especially all the code which is 
#ifdef CONFIG_HID_WACOM_POWER_SUPPLY

> (Hm, and I wonder what makes the mouse actually report the battery 
> level; I guess I'll only ever see a report if it actually changes.)

I'd expect that as a "default" behavior, yes. Unfortunately it might also 
be possible that there is some vendor-specific way to actually query the 
battery state, but it'd be difficult to find out without actually snooping 
the behavior in some other OS.

But as long as it contains proper entry in report descriptor, I'd expect 
it to send the report on its own.

-- 
Jiri Kosina
SUSE Labs

Re: Supporting Battery Strength from my Bluetooth Mouse

[Jeremy Fitzhardinge]

On 11/19/2011 03:18 AM, Jiri Kosina wrote:
> the debugfs patch is obviously correct(tm), so I will be queuing it, 
> thanks.

Thanks.

>> My real question is how to get this out to somewhere useful, where 
>> (ultimately) some GUI could show the battery information if its 
>> available?
> Actually the proper way to do this is using power_supply class.
>
> There alreasy is a HID driver that does this properly, and it's rather 
> easy. Check drivers/hid/hid-wacom.c, especially all the code which is 
> #ifdef CONFIG_HID_WACOM_POWER_SUPPLY

Hm, OK.  I wonder what the descriptor for that device looks like; I
wonder if it already uses Battery Strength, but just hard-coded?

Given that this is a generic HID thing, where should it go?  Should
hid-core.c go around trying to create new power supplies?

Seems to me that there's a good chance that random devices will claim to
supply Battery Strength info even if they don't have batteries because
they happen to share firmware with a device that does (similarly, its
amazing how many keys my keyboard is missing according to the
descriptor).  I guess the best way to handle it is have a hid-core
helper function which will register a power_supply if the driver asks it
to (and the descriptor contains Battery Strength).  Bluetooth, for
example, has a separate descriptor entry about whether the device is
battery powered which is much more likely to be accurate than the
generic HID descriptor, and it can call the power supply helper as part
of the HID setup.

Does that sound reasonable?

>> (Hm, and I wonder what makes the mouse actually report the battery 
>> level; I guess I'll only ever see a report if it actually changes.)
> I'd expect that as a "default" behavior, yes. Unfortunately it might also 
> be possible that there is some vendor-specific way to actually query the 
> battery state, but it'd be difficult to find out without actually snooping 
> the behavior in some other OS.
>
> But as long as it contains proper entry in report descriptor, I'd expect 
> it to send the report on its own.

My reading of the evdev documentation is that it would suppress
duplicate reports to usermode, so my little program wouldn't see any
events from /dev/input/eventX unless there were a change anyway, is that
right?

Thanks,
    J

Re: Supporting Battery Strength from my Bluetooth Mouse

[Jiri Kosina]

On Sat, 19 Nov 2011, Jeremy Fitzhardinge wrote:

> >> My real question is how to get this out to somewhere useful, where 
> >> (ultimately) some GUI could show the battery information if its 
> >> available?
> > Actually the proper way to do this is using power_supply class.
> >
> > There alreasy is a HID driver that does this properly, and it's rather 
> > easy. Check drivers/hid/hid-wacom.c, especially all the code which is 
> > #ifdef CONFIG_HID_WACOM_POWER_SUPPLY
> 
> Hm, OK.  I wonder what the descriptor for that device looks like; I
> wonder if it already uses Battery Strength, but just hard-coded?

Let's ask Przemo -- does the device present the battery status in its 
report descriptor at all (and if so, could you please share this part of 
report descriptor with us), or has its representation in the reports been 
pure guesswork?

> Given that this is a generic HID thing, where should it go?  Should 
> hid-core.c go around trying to create new power supplies?

If devices which present the battery status in a standard way start to 
appear, we definitely should make it more generic compared to the add-hoc 
handling we currently have in Wacom and Wiimote drivers.

> Seems to me that there's a good chance that random devices will claim to 
> supply Battery Strength info even if they don't have batteries because 
> they happen to share firmware with a device that does (similarly, its 
> amazing how many keys my keyboard is missing according to the 
> descriptor).  I guess the best way to handle it is have a hid-core 
> helper function which will register a power_supply if the driver asks it 
> to (and the descriptor contains Battery Strength).  

This would however force us to have a separate driver on HID bus for such 
devices. I'd prefer to have this in generic code (we are handling 
gazillions of devices just by hid-core/hid-input without any need for 
additional hidbus driver) if possible.

I haven't personally came across many devices that would present bogus 
Battery status in their report descriptor, but it'll probably require some 
more investigation.

> Bluetooth, for example, has a separate descriptor entry about whether 
> the device is battery powered which is much more likely to be accurate 
> than the generic HID descriptor, and it can call the power supply helper 
> as part of the HID setup.
> 
> Does that sound reasonable?

Making the battery / power_supply initialization part of low-level HID 
transport initialization (usb/bluetooth) makes probably the most sense, 
yes.

> > But as long as it contains proper entry in report descriptor, I'd 
> > expect it to send the report on its own.
> 
> My reading of the evdev documentation is that it would suppress
> duplicate reports to usermode, so my little program wouldn't see any
> events from /dev/input/eventX unless there were a change anyway, is that
> right?

That depends a bit on the type of the event (EV_KEY has to handle 
auto-repeat for example, etc). See the switch in input_handle_event() 
which contains the logic behind what is happening when 'duplicate' event 
is coming through input core.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: Supporting Battery Strength from my Bluetooth Mouse

[Jeremy Fitzhardinge]

On 11/20/2011 02:26 AM, Jiri Kosina wrote:
>> Given that this is a generic HID thing, where should it go?  Should 
>> hid-core.c go around trying to create new power supplies?
> If devices which present the battery status in a standard way start to 
> appear, we definitely should make it more generic compared to the add-hoc 
> handling we currently have in Wacom and Wiimote drivers.

OK.

> This would however force us to have a separate driver on HID bus for
> such devices. I'd prefer to have this in generic code (we are handling
> gazillions of devices just by hid-core/hid-input without any need for
> additional hidbus driver) if possible. I haven't personally came
> across many devices that would present bogus Battery status in their
> report descriptor, but it'll probably require some more investigation.

I have no problem just doing it for all devices unconditionally (well,
conditional on Battery Strength) if you don't think it would be a problem.

>> Bluetooth, for example, has a separate descriptor entry about whether 
>> the device is battery powered which is much more likely to be accurate 
>> than the generic HID descriptor, and it can call the power supply helper 
>> as part of the HID setup.
>>
>> Does that sound reasonable?
> Making the battery / power_supply initialization part of low-level HID 
> transport initialization (usb/bluetooth) makes probably the most sense, 
> yes.

Though unfortunately it looks like the SDP data that contains that
information is only parsed by usermode, and so isn't available to the
kernel.  That makes a generic HID-wide approach look more appealing.

> That depends a bit on the type of the event (EV_KEY has to handle 
> auto-repeat for example, etc). See the switch in input_handle_event() 
> which contains the logic behind what is happening when 'duplicate' event 
> is coming through input core.

In this case, it will be EV_ABS/ABS_MISC which does have duplicates
suppressed.

Now I need to work out the control flow.  What's the best place to hook
in "register something if something is present in the descriptor"?

    J

Re: Supporting Battery Strength from my Bluetooth Mouse

[Dmitry Torokhov]

On Mon, Nov 21, 2011 at 08:38:21AM -0800, Jeremy Fitzhardinge wrote:
> On 11/20/2011 02:26 AM, Jiri Kosina wrote:
> >> Given that this is a generic HID thing, where should it go?  Should 
> >> hid-core.c go around trying to create new power supplies?
> > If devices which present the battery status in a standard way start to 
> > appear, we definitely should make it more generic compared to the add-hoc 
> > handling we currently have in Wacom and Wiimote drivers.
> 
> OK.
> 
> > This would however force us to have a separate driver on HID bus for
> > such devices. I'd prefer to have this in generic code (we are handling
> > gazillions of devices just by hid-core/hid-input without any need for
> > additional hidbus driver) if possible. I haven't personally came
> > across many devices that would present bogus Battery status in their
> > report descriptor, but it'll probably require some more investigation.
> 
> I have no problem just doing it for all devices unconditionally (well,
> conditional on Battery Strength) if you don't think it would be a problem.
> 
> >> Bluetooth, for example, has a separate descriptor entry about whether 
> >> the device is battery powered which is much more likely to be accurate 
> >> than the generic HID descriptor, and it can call the power supply helper 
> >> as part of the HID setup.
> >>
> >> Does that sound reasonable?
> > Making the battery / power_supply initialization part of low-level HID 
> > transport initialization (usb/bluetooth) makes probably the most sense, 
> > yes.
> 
> Though unfortunately it looks like the SDP data that contains that
> information is only parsed by usermode, and so isn't available to the
> kernel.  That makes a generic HID-wide approach look more appealing.
> 
> > That depends a bit on the type of the event (EV_KEY has to handle 
> > auto-repeat for example, etc). See the switch in input_handle_event() 
> > which contains the logic behind what is happening when 'duplicate' event 
> > is coming through input core.
> 
> In this case, it will be EV_ABS/ABS_MISC which does have duplicates
> suppressed.

No, please do not try to route battery info through input subsystem;
power_supply seems to be the proper interface for it.

Thanks.

-- 
Dmitry

Re: Supporting Battery Strength from my Bluetooth Mouse

[Jeremy Fitzhardinge]

On 11/21/2011 09:36 AM, Dmitry Torokhov wrote:
>>> That depends a bit on the type of the event (EV_KEY has to handle 
>>> auto-repeat for example, etc). See the switch in input_handle_event() 
>>> which contains the logic behind what is happening when 'duplicate' event 
>>> is coming through input core.
>> In this case, it will be EV_ABS/ABS_MISC which does have duplicates
>> suppressed.
> No, please do not try to route battery info through input subsystem;
> power_supply seems to be the proper interface for it.

I was just referring to doing that for debugging/investigation
purposes.  I found hidraw which resolves that concern anyway.

Thanks,
    J

Re: Supporting Battery Strength from my Bluetooth Mouse

[Jiri Kosina]

On Mon, 21 Nov 2011, Jeremy Fitzhardinge wrote:

> > such devices. I'd prefer to have this in generic code (we are handling
> > gazillions of devices just by hid-core/hid-input without any need for
> > additional hidbus driver) if possible. I haven't personally came
> > across many devices that would present bogus Battery status in their
> > report descriptor, but it'll probably require some more investigation.
> 
> I have no problem just doing it for all devices unconditionally (well,
> conditional on Battery Strength) if you don't think it would be a problem.

It definitely is something we should at least try. It might turn out that 
there are way too many devices out there which break horribly with this, 
but hey, it's defined by spec, so it'd be strange not to even try to 
support it properly :)

> >> Bluetooth, for example, has a separate descriptor entry about whether 
> >> the device is battery powered which is much more likely to be accurate 
> >> than the generic HID descriptor, and it can call the power supply helper 
> >> as part of the HID setup.
> >>
> >> Does that sound reasonable?
> > Making the battery / power_supply initialization part of low-level HID 
> > transport initialization (usb/bluetooth) makes probably the most sense, 
> > yes.
> 
> Though unfortunately it looks like the SDP data that contains that
> information is only parsed by usermode, and so isn't available to the
> kernel.  That makes a generic HID-wide approach look more appealing.

I agree.

So I take it that you are now working on that, is my understanding 
correct?

Thanks a lot,

-- 
Jiri Kosina
SUSE Labs

Re: Supporting Battery Strength from my Bluetooth Mouse

[Jeremy Fitzhardinge]

On 11/21/2011 03:29 PM, Jiri Kosina wrote:
> So I take it that you are now working on that, is my understanding 
> correct?

Yep, though I'm still trying to see how it all fits together.  For
example, in which function should I check to see if BatteryStrength is
in the table?

Thanks,
    J

Re: Supporting Battery Strength from my Bluetooth Mouse

[Jiri Kosina]

On Mon, 21 Nov 2011, Jeremy Fitzhardinge wrote:

> > So I take it that you are now working on that, is my understanding 
> > correct?
> 
> Yep, though I'm still trying to see how it all fits together.  For
> example, in which function should I check to see if BatteryStrength is
> in the table?

Well, parsing HID report descriptor is not completely trivial, although 
it's not a heart surgery either.

It's all done by 'hid_parse()' main routine, which calls one of 
hid_parser_main(), hid_parser_global(), hid_parser_local() or 
hid_parse_reserved(), depending on the type of the item.

For hid-input usages, the most important part of the setup is then done in 
hidinput_configure_usage().

The actual 'reaction' to the event once it is signalled by device then 
happens in hidinput_hid_event(). This is being called from 
hid_process_event() handler, and individual drivers can register their own 
callbacks for event processing that get called before this 'global' 
handler executes (see for example logi_dj_ll_input_event() in 
drivers/hid/hid-logitech-dj.c).

Hope this helps,

-- 
Jiri Kosina
SUSE Labs

[PATCH RFC] hid-input: add support for HID devices reporting Battery Strength

[Jeremy Fitzhardinge]

Some HID devices, such as my Bluetooth mouse, report their battery
strength as an event.  Rather than passing it through as a strange
absolute input event, this patch registers it with the power_supply
subsystem as a battery, so that the device's Battery Strength can be
reported to usermode.

The battery appears in sysfs names
/sys/class/power_supply/hid-<UNIQ>-battery, and it is a child of the
battery-containing device, so it should be clear what it's the battery of.

Unfortunately on my current Fedora 16 system, while the battery does
appear in the UI, it is listed as a Laptop Battery with 0% charge (since
it ignores the "capacity" property of the battery and instead computes
it from the "energy*" fields, which we can't supply given the limited
information contained within the HID Report).

Still, this patch is the first step.

Signed-off-by: Jeremy Fitzhardinge <jeremy@goop.org>

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 22a4a05..3a97f1f 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -31,6 +31,11 @@ config HID
 
 	  If unsure, say Y.
 
+config HID_BATTERY_STRENGTH
+	bool
+	depends on POWER_SUPPLY
+	default y
+
 config HIDRAW
 	bool "/dev/hidraw raw HID device support"
 	depends on HID
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index f333139..83afb86 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -271,6 +271,97 @@ static __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
 	return logical_extents / physical_extents;
 }
 
+#ifdef CONFIG_HID_BATTERY_STRENGTH
+static enum power_supply_property hidinput_battery_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+};
+
+static int hidinput_get_battery_property(struct power_supply *psy,
+					 enum power_supply_property prop,
+					 union power_supply_propval *val)
+{
+	struct hid_device *dev = container_of(psy, struct hid_device, battery);
+	int ret = 0;
+
+	switch (prop) {
+	case POWER_SUPPLY_PROP_PRESENT:
+	case POWER_SUPPLY_PROP_ONLINE:
+		val->intval = 1;
+		break;
+
+	case POWER_SUPPLY_PROP_CAPACITY:
+		if (dev->battery_min < dev->battery_max &&
+		    dev->battery_val >= dev->battery_min &&
+		    dev->battery_val <= dev->battery_max)
+			val->intval = (100 * (dev->battery_val - dev->battery_min)) /
+				(dev->battery_max - dev->battery_min);
+		else
+			ret = -EINVAL;
+		break;
+
+	case POWER_SUPPLY_PROP_MODEL_NAME:
+		val->strval = dev->name;
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
+{
+	struct power_supply *battery = &dev->battery;
+	int ret;
+
+	if (battery->name != NULL)
+		return;		/* already initialized? */
+
+	battery->name = kasprintf(GFP_KERNEL, "hid-%s-battery", dev->uniq);
+	if (battery->name == NULL)
+		return;
+
+	battery->type = POWER_SUPPLY_TYPE_BATTERY;
+	battery->properties = hidinput_battery_props;
+	battery->num_properties = ARRAY_SIZE(hidinput_battery_props);
+	battery->use_for_apm = 0;
+	battery->get_property = hidinput_get_battery_property;
+
+	dev->battery_min = min;
+	dev->battery_max = max;
+
+	ret = power_supply_register(&dev->dev, battery);
+	if (ret != 0) {
+		hid_warn(dev, "can't register power supply: %d\n", ret);
+		kfree(battery->name);
+		battery->name = NULL;
+	}
+}
+
+static void hidinput_cleanup_battery(struct hid_device *dev)
+{
+	if (!dev->battery.name)
+		return;
+
+	power_supply_unregister(&dev->battery);
+	kfree(dev->battery.name);
+	dev->battery.name = NULL;
+}
+#else  /* !CONFIG_HID_BATTERY_STRENGTH */
+static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
+{
+}
+
+static void hidinput_cleanup_battery(struct hid_device *dev)
+{
+}
+#endif	/* CONFIG_HID_BATTERY_STRENGTH */
+
 static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_field *field,
 				     struct hid_usage *usage)
 {
@@ -629,6 +720,16 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 		}
 		break;
 
+	case HID_UP_GENDEVCTRLS:
+		if ((usage->hid & HID_USAGE) == 0x20) {	/* Battery Strength */
+			hidinput_setup_battery(device,
+					       field->logical_minimum,
+					       field->logical_maximum);
+			goto ignore;
+		} else
+			goto unknown;
+		break;
+
 	case HID_UP_HPVENDOR:	/* Reported on a Dutch layout HP5308 */
 		set_bit(EV_REP, input->evbit);
 		switch (usage->hid & HID_USAGE) {
@@ -760,6 +861,13 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 
 	input = field->hidinput->input;
 
+	if (usage->hid == HID_DC_BATTERYSTRENGTH) {
+		hid->battery_val = value;
+		hid_dbg(hid, "battery value is %d (range %d-%d)\n",
+			value, hid->battery_min, hid->battery_max);
+		return;
+	}
+
 	if (!usage->type)
 		return;
 
@@ -1010,6 +1118,8 @@ void hidinput_disconnect(struct hid_device *hid)
 {
 	struct hid_input *hidinput, *next;
 
+	hidinput_cleanup_battery(hid);
+
 	list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
 		list_del(&hidinput->list);
 		input_unregister_device(hidinput->input);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index c235e4e..214801d 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -72,6 +72,7 @@
 #include <linux/workqueue.h>
 #include <linux/input.h>
 #include <linux/semaphore.h>
+#include <linux/power_supply.h>
 
 /*
  * We parse each description item into this structure. Short items data
@@ -190,6 +191,7 @@ struct hid_item {
 #define HID_UP_UNDEFINED	0x00000000
 #define HID_UP_GENDESK		0x00010000
 #define HID_UP_SIMULATION	0x00020000
+#define HID_UP_GENDEVCTRLS	0x00060000
 #define HID_UP_KEYBOARD		0x00070000
 #define HID_UP_LED		0x00080000
 #define HID_UP_BUTTON		0x00090000
@@ -239,6 +241,8 @@ struct hid_item {
 #define HID_GD_RIGHT		0x00010092
 #define HID_GD_LEFT		0x00010093
 
+#define HID_DC_BATTERYSTRENGTH	0x00060020
+
 #define HID_DG_DIGITIZER	0x000d0001
 #define HID_DG_PEN		0x000d0002
 #define HID_DG_LIGHTPEN		0x000d0003
@@ -482,6 +486,18 @@ struct hid_device {							/* device report descriptor */
 	struct hid_driver *driver;
 	struct hid_ll_driver *ll_driver;
 
+#ifdef CONFIG_HID_BATTERY_STRENGTH
+	/*
+	 * Power supply information for HID devices which report
+	 * battery strength. power_supply is registered iff
+	 * battery.name is non-NULL.
+	 */
+	struct power_supply battery;
+	__s32 battery_min;
+	__s32 battery_max;
+	__s32 battery_val;
+#endif
+
 	unsigned int status;						/* see STAT flags above */
 	unsigned claimed;						/* Claimed by hidinput, hiddev? */
 	unsigned quirks;						/* Various quirks the device can pull on us */

Re: [PATCH RFC] hid-input: add support for HID devices reporting Battery Strength

[Chase Douglas]

On 11/23/2011 12:49 AM, Jeremy Fitzhardinge wrote:
> Some HID devices, such as my Bluetooth mouse, report their battery
> strength as an event.  Rather than passing it through as a strange
> absolute input event, this patch registers it with the power_supply
> subsystem as a battery, so that the device's Battery Strength can be
> reported to usermode.
> 
> The battery appears in sysfs names
> /sys/class/power_supply/hid-<UNIQ>-battery, and it is a child of the
> battery-containing device, so it should be clear what it's the battery of.
> 
> Unfortunately on my current Fedora 16 system, while the battery does
> appear in the UI, it is listed as a Laptop Battery with 0% charge (since
> it ignores the "capacity" property of the battery and instead computes
> it from the "energy*" fields, which we can't supply given the limited
> information contained within the HID Report).
> 
> Still, this patch is the first step.
> 
> Signed-off-by: Jeremy Fitzhardinge <jeremy@goop.org>
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 22a4a05..3a97f1f 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -31,6 +31,11 @@ config HID
>  
>  	  If unsure, say Y.
>  
> +config HID_BATTERY_STRENGTH
> +	bool
> +	depends on POWER_SUPPLY
> +	default y

This functionality will be great to have :). I'm curious why you made a
config option for it, though. It's not a big patch, I can't think of any
reason people wouldn't want it, and this could lead to dependency issues
(i.e. needing to sprinkle #ifdef HID_BATTERY_STRENGTH in drivers).

-- Chase

Re: [PATCH RFC] hid-input: add support for HID devices reporting Battery Strength

[Jeremy Fitzhardinge]

On 11/23/2011 08:36 AM, Chase Douglas wrote:
> On 11/23/2011 12:49 AM, Jeremy Fitzhardinge wrote:
>> Some HID devices, such as my Bluetooth mouse, report their battery
>> strength as an event.  Rather than passing it through as a strange
>> absolute input event, this patch registers it with the power_supply
>> subsystem as a battery, so that the device's Battery Strength can be
>> reported to usermode.
>>
>> The battery appears in sysfs names
>> /sys/class/power_supply/hid-<UNIQ>-battery, and it is a child of the
>> battery-containing device, so it should be clear what it's the battery of.
>>
>> Unfortunately on my current Fedora 16 system, while the battery does
>> appear in the UI, it is listed as a Laptop Battery with 0% charge (since
>> it ignores the "capacity" property of the battery and instead computes
>> it from the "energy*" fields, which we can't supply given the limited
>> information contained within the HID Report).
>>
>> Still, this patch is the first step.
>>
>> Signed-off-by: Jeremy Fitzhardinge <jeremy@goop.org>
>>
>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>> index 22a4a05..3a97f1f 100644
>> --- a/drivers/hid/Kconfig
>> +++ b/drivers/hid/Kconfig
>> @@ -31,6 +31,11 @@ config HID
>>  
>>  	  If unsure, say Y.
>>  
>> +config HID_BATTERY_STRENGTH
>> +	bool
>> +	depends on POWER_SUPPLY
>> +	default y
> This functionality will be great to have :). I'm curious why you made a
> config option for it, though. It's not a big patch, I can't think of any
> reason people wouldn't want it, and this could lead to dependency issues
> (i.e. needing to sprinkle #ifdef HID_BATTERY_STRENGTH in drivers).
>

It depends on CONFIG_POWER_SUPPLY, and I guess its possible that people
may want to unconfig it because the extra power supplies confuse
usermode (as they do Gnome 3, but non-fatally).  I'm just worried that
something might want to start hibernating my machine because the mouse
is running out :/...

However, I left it as a non-user-visible config option so that it
doesn't create any more noise on that front.

The Wacom driver's version of this also has its own user-visible config
option; I don't know if there's any reason you'd turn it off.

    J

Re: [PATCH RFC] hid-input: add support for HID devices reporting Battery Strength

[Przemo Firszt]

Dnia 2011-11-23, śro o godzinie 13:07 -0800, Jeremy Fitzhardinge pisze:
[..]
> The Wacom driver's version of this also has its own user-visible
config
> option; I don't know if there's any reason you'd turn it off.

Hi Jeremy,
It was just one of the options. See here:
http://www.spinics.net/lists/linux-bluetooth/msg04750.html

There is a bit more why wacom driver is reporting battery/power thriugh
sysfs. Search for "[PATCH] Expose wacom pen tablet battery and ac thru
power_supply class" on linux-bluetooth list.

-- 
Regards,
Przemo Firszt

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] hid-input: add support for HID devices reporting Battery Strength

[Jiri Kosina]

On Wed, 23 Nov 2011, Jeremy Fitzhardinge wrote:

> Some HID devices, such as my Bluetooth mouse, report their battery
> strength as an event.  Rather than passing it through as a strange
> absolute input event, this patch registers it with the power_supply
> subsystem as a battery, so that the device's Battery Strength can be
> reported to usermode.
> 
> The battery appears in sysfs names
> /sys/class/power_supply/hid-<UNIQ>-battery, and it is a child of the
> battery-containing device, so it should be clear what it's the battery of.
> 
> Unfortunately on my current Fedora 16 system, while the battery does
> appear in the UI, it is listed as a Laptop Battery with 0% charge (since
> it ignores the "capacity" property of the battery and instead computes
> it from the "energy*" fields, which we can't supply given the limited
> information contained within the HID Report).
> 
> Still, this patch is the first step.
> 
> Signed-off-by: Jeremy Fitzhardinge <jeremy@goop.org>

Thanks Jeremy, I am going to queue this for 3.3.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH RFC] hid-input: add support for HID devices reporting Battery Strength

[Daniel Nicoletti]

I've sent an email earlier asking for help with a GetFeature code, and now I
have a second patch on top of Jeremy's to provide the battery functionality
for devices that support reporting it.

If I understood correctly when talking to Jeremy he said his device
never actually
reported the status as an input event (sorry if I didn't understand it
correctly), and
after reading HID specs I believe it's really because it was meant to be probed,
I have an Apple Keyboard and Magic Trackpad both bluetooth batteries operated,
so using PacketLogger I saw that Mac OSX always ask the battery status using
the so called GetFeature.

What my patch does is basically:
- store the report id that matches the battery_strength
- setup the battery if 0x6.0x20 is found, even if that is reported as a feature
  (as it was meant to be but only the MagicTrackpad does)
- when upower or someone access /sys/class/power_supply/hid-*/capacity it
  will probe the device and return it's status.

It works great for both devices, but I have two concerns:
- the report_features function has a duplicated code
- it would be nice if it was possible for specific drivers to provide their own
  probe as there might be some strange devices... (but maybe it's
already possible)

I've talked to the upower dev and he fixed it to be able to show the
right percentage.

Here how the uevent file (in /sys/class/power_supply/hid-*/) looks like:
POWER_SUPPLY_NAME=hid-00:22:41:D9:18:E7-battery
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_ONLINE=1
POWER_SUPPLY_CAPACITY=66
POWER_SUPPLY_MODEL_NAME=MacAdmin’s keyboard
POWER_SUPPLY_STATUS=Discharging

POWER_SUPPLY_NAME=hid-70:CD:60:F5:FF:3F-battery
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_ONLINE=1
POWER_SUPPLY_CAPACITY=62
POWER_SUPPLY_MODEL_NAME=nexx’s Trackpad
POWER_SUPPLY_STATUS=Discharging

and the patch (note that this is my first kernel patch so sorry if
formatting is not ok, should I attach it?):
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index b9b8c75..8fac47c 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -277,6 +277,7 @@ static enum power_supply_property
hidinput_battery_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_STATUS
 };

 static int hidinput_get_battery_property(struct power_supply *psy,
@@ -285,6 +286,9 @@ static int hidinput_get_battery_property(struct
power_supply *psy,
 {
 	struct hid_device *dev = container_of(psy, struct hid_device, battery);
 	int ret = 0;
+	int ret_rep;
+	__u8 *buf = NULL;
+	unsigned char report_number = dev->battery_report_id;

 	switch (prop) {
 	case POWER_SUPPLY_PROP_PRESENT:
@@ -293,28 +297,45 @@ static int hidinput_get_battery_property(struct
power_supply *psy,
 		break;

 	case POWER_SUPPLY_PROP_CAPACITY:
-		if (dev->battery_min < dev->battery_max &&
-		    dev->battery_val >= dev->battery_min &&
-		    dev->battery_val <= dev->battery_max)
-			val->intval = (100 * (dev->battery_val - dev->battery_min)) /
-				(dev->battery_max - dev->battery_min);
-		else
+		buf = kmalloc(2 * sizeof(__u8), GFP_KERNEL);
+		if (!buf) {
+			ret = -ENOMEM;
+			break;
+		}
+
+		memset(buf, 0, sizeof(buf));
+		ret_rep = dev->hid_get_raw_report(dev, report_number, buf,
sizeof(buf), HID_FEATURE_REPORT);
+		if (ret_rep != 2) {
 			ret = -EINVAL;
+			break;
+		}
+
+		/* store the returned value */
+		/* I'm not calculating this using the logical_minimum and maximum */
+		/* because my device returns 0-100 even though the min and max are 0-255 */
+		val->intval = buf[1];
 		break;

 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = dev->name;
 		break;

+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
+
 	default:
 		ret = -EINVAL;
 		break;
 	}

+	if (buf) {
+		kfree(buf);
+	}
 	return ret;
 }

-static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
+static void hidinput_setup_battery(struct hid_device *dev, unsigned
id, s32 min, s32 max)
 {
 	struct power_supply *battery = &dev->battery;
 	int ret;
@@ -326,7 +347,7 @@ static void hidinput_setup_battery(struct
hid_device *dev, s32 min, s32 max)
 	if (battery->name == NULL)
 		return;

-	battery->type = POWER_SUPPLY_TYPE_BATTERY;
+	battery->type = POWER_SUPPLY_TYPE_USB;
 	battery->properties = hidinput_battery_props;
 	battery->num_properties = ARRAY_SIZE(hidinput_battery_props);
 	battery->use_for_apm = 0;
@@ -334,6 +355,7 @@ static void hidinput_setup_battery(struct
hid_device *dev, s32 min, s32 max)

 	dev->battery_min = min;
 	dev->battery_max = max;
+	dev->battery_report_id = id;

 	ret = power_supply_register(&dev->dev, battery);
 	if (ret != 0) {
@@ -353,7 +375,7 @@ static void hidinput_cleanup_battery(struct hid_device *dev)
 	dev->battery.name = NULL;
 }
 #else  /* !CONFIG_HID_BATTERY_STRENGTH */
-static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
+static void hidinput_setup_battery(struct hid_device *dev, unsigned
id, s32 min, s32 max)
 {
 }

@@ -723,6 +745,7 @@ static void hidinput_configure_usage(struct
hid_input *hidinput, struct hid_fiel
 	case HID_UP_GENDEVCTRLS:
 		if ((usage->hid & HID_USAGE) == 0x20) {	/* Battery Strength */
 			hidinput_setup_battery(device,
+					       field->report->id,
 					       field->logical_minimum,
 					       field->logical_maximum);
 			goto ignore;
@@ -997,15 +1020,23 @@ static void report_features(struct hid_device *hid)
 	struct hid_report *rep;
 	int i, j;

-	if (!drv->feature_mapping)
-		return;
-
 	rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
 	list_for_each_entry(rep, &rep_enum->report_list, list)
 		for (i = 0; i < rep->maxfield; i++)
-			for (j = 0; j < rep->field[i]->maxusage; j++)
-				drv->feature_mapping(hid, rep->field[i],
-						     rep->field[i]->usage + j);
+			for (j = 0; j < rep->field[i]->maxusage; j++) {
+				/* Verify if Battery Strength feature is available */
+				if (((rep->field[i]->usage + j)->hid & HID_USAGE_PAGE) ==
HID_UP_GENDEVCTRLS &&
+					((rep->field[i]->usage + j)->hid & HID_USAGE) == 0x20) {
+					hidinput_setup_battery(hid,
+							       rep->id,
+							       rep->field[i]->logical_minimum,
+							       rep->field[i]->logical_maximum);
+				}
+
+				if (drv->feature_mapping)
+					drv->feature_mapping(hid, rep->field[i],
+							     rep->field[i]->usage + j);
+			}
 }

 /*
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 7f344c3..b5df198 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -496,6 +496,7 @@ struct hid_device {							/* device report descriptor */
 	__s32 battery_min;
 	__s32 battery_max;
 	__s32 battery_val;
+	__s32 battery_report_id;
 #endif

 	unsigned int status;						/* see STAT flags above */
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] hid-input: add support for HID devices reporting Battery Strength

[Jeremy Fitzhardinge]

On 12/01/2011 09:52 PM, Daniel Nicoletti wrote:
> I've sent an email earlier asking for help with a GetFeature code, and now I
> have a second patch on top of Jeremy's to provide the battery functionality
> for devices that support reporting it.
>
> If I understood correctly when talking to Jeremy he said his device
> never actually
> reported the status as an input event (sorry if I didn't understand it
> correctly),

No, it does.  When the mouse first connects it reports the battery level
as an Input event.  Without my patch, X sees the battery level as a
strange little absolute axis on the mouse, which it ignores.

I don't know if the mouse also reports the battery later on
spontaneously; I haven't observed it, so I suspect you may be right that
you have to poll it to get further updates.

>  and
> after reading HID specs I believe it's really because it was meant to be probed,
> I have an Apple Keyboard and Magic Trackpad both bluetooth batteries operated,
> so using PacketLogger I saw that Mac OSX always ask the battery status using
> the so called GetFeature.

Could you cite a specific reference?  I don't see any references to
"GetFeature" in HID spec I have.  The closest I see is
"Get_Report(INPUT/OUTPUT/FEATURE)", which specifically says "This
request is not intended to be used for polling the device state on a
regular basis".  Though I'm not at all surprised if device manufacturers
ignore this, or that it is contradictory with some other part of the spec.

> What my patch does is basically:
> - store the report id that matches the battery_strength
> - setup the battery if 0x6.0x20 is found, even if that is reported as a feature
>   (as it was meant to be but only the MagicTrackpad does)
> - when upower or someone access /sys/class/power_supply/hid-*/capacity it
>   will probe the device and return it's status.

How often does usermode read this file?  Does it/can it select on the
file to wait for changes.

Given that battery level shouldn't change that quickly, I wonder if
having a timer poll the device every minute or something of that order
might be useful?

>
> It works great for both devices, but I have two concerns:
> - the report_features function has a duplicated code

Yes, but that's easy to fix.

> - it would be nice if it was possible for specific drivers to provide their own
>   probe as there might be some strange devices... (but maybe it's
> already possible)
I'd wait until there's a device which can't be handled by this mechanism
before trying to generalize it.  Unless we can unify the wacom driver,
but that seems to be in its own little world.

> I've talked to the upower dev and he fixed it to be able to show the
> right percentage.

Cool, but I have some concerns about that (see below).

> Here how the uevent file (in /sys/class/power_supply/hid-*/) looks like:
> POWER_SUPPLY_NAME=hid-00:22:41:D9:18:E7-battery
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_ONLINE=1
> POWER_SUPPLY_CAPACITY=66
> POWER_SUPPLY_MODEL_NAME=MacAdmin’s keyboard
> POWER_SUPPLY_STATUS=Discharging
>
> POWER_SUPPLY_NAME=hid-70:CD:60:F5:FF:3F-battery
> POWER_SUPPLY_PRESENT=1
> POWER_SUPPLY_ONLINE=1
> POWER_SUPPLY_CAPACITY=62
> POWER_SUPPLY_MODEL_NAME=nexx’s Trackpad
> POWER_SUPPLY_STATUS=Discharging
>
> and the patch (note that this is my first kernel patch so sorry if
> formatting is not ok, should I attach it?):
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index b9b8c75..8fac47c 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -277,6 +277,7 @@ static enum power_supply_property
> hidinput_battery_props[] = {
>  	POWER_SUPPLY_PROP_ONLINE,
>  	POWER_SUPPLY_PROP_CAPACITY,
>  	POWER_SUPPLY_PROP_MODEL_NAME,
> +	POWER_SUPPLY_PROP_STATUS
>  };
>
>  static int hidinput_get_battery_property(struct power_supply *psy,
> @@ -285,6 +286,9 @@ static int hidinput_get_battery_property(struct
> power_supply *psy,
>  {
>  	struct hid_device *dev = container_of(psy, struct hid_device, battery);
>  	int ret = 0;
> +	int ret_rep;
> +	__u8 *buf = NULL;
> +	unsigned char report_number = dev->battery_report_id;
>
>  	switch (prop) {
>  	case POWER_SUPPLY_PROP_PRESENT:
> @@ -293,28 +297,45 @@ static int hidinput_get_battery_property(struct
> power_supply *psy,
>  		break;
>
>  	case POWER_SUPPLY_PROP_CAPACITY:
> -		if (dev->battery_min < dev->battery_max &&
> -		    dev->battery_val >= dev->battery_min &&
> -		    dev->battery_val <= dev->battery_max)
> -			val->intval = (100 * (dev->battery_val - dev->battery_min)) /
> -				(dev->battery_max - dev->battery_min);
> -		else
> +		buf = kmalloc(2 * sizeof(__u8), GFP_KERNEL);

Why allocate a two byte buffer?  Why not just put it on the stack?  How
do you know that's the right size?

> +		if (!buf) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +
> +		memset(buf, 0, sizeof(buf));
> +		ret_rep = dev->hid_get_raw_report(dev, report_number, buf,
> sizeof(buf), HID_FEATURE_REPORT);
> +		if (ret_rep != 2) {
>  			ret = -EINVAL;
> +			break;
> +		}
> +
> +		/* store the returned value */
> +		/* I'm not calculating this using the logical_minimum and maximum */
> +		/* because my device returns 0-100 even though the min and max are 0-255 */
> +		val->intval = buf[1];

That's definitely not the case for my mouse.  I'm not sure what the
range actually is, but I've seen values reported > 100 with fresh
alkaline batteries.  Initially I also thought it was using a 0-100 range
despite the min/max, and I added a per-device quirk mechanism - but
since my device didn't need it, I did't include it in the patch
posting.  I'd reinstate it if there are devices that report 0-100.

>  		break;
>
>  	case POWER_SUPPLY_PROP_MODEL_NAME:
>  		val->strval = dev->name;
>  		break;
>
> +	case POWER_SUPPLY_PROP_STATUS:
> +		val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> +		break;
> +
>  	default:
>  		ret = -EINVAL;
>  		break;
>  	}
>
> +	if (buf) {
> +		kfree(buf);
> +	}
>  	return ret;
>  }
>
> -static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
> +static void hidinput_setup_battery(struct hid_device *dev, unsigned
> id, s32 min, s32 max)
>  {
>  	struct power_supply *battery = &dev->battery;
>  	int ret;
> @@ -326,7 +347,7 @@ static void hidinput_setup_battery(struct
> hid_device *dev, s32 min, s32 max)
>  	if (battery->name == NULL)
>  		return;
>
> -	battery->type = POWER_SUPPLY_TYPE_BATTERY;
> +	battery->type = POWER_SUPPLY_TYPE_USB;

What's the purpose of this?  Is it to make upower behave in a
different/better way?  My reading is that this means that it's powered
by USB ("Standard Downstream Port").  A bluetooth device doesn't have
any USB involved at all (except if the BT adapter itself is USB, which
is moot).

If upower is getting confused thinking that a "battery" is necessarily a
laptop battery, then I think it needs to be fixed to pay closer
attention to the device tree topology.

(looks up upower) Ah, I see.

The changes in the current upower.git look very suspect to me, if
nothing else because of the hard-coded "magicmouse" and "wacom", and the
use of POWER_SUPPLY_TYPE_USB.  Can upower not look to see if there's a
power-supply hanging off a HID device and report that as peripheral power?

> -	if (!drv->feature_mapping)
> -		return;
> -
>  	rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
>  	list_for_each_entry(rep, &rep_enum->report_list, list)
>  		for (i = 0; i < rep->maxfield; i++)
> -			for (j = 0; j < rep->field[i]->maxusage; j++)
> -				drv->feature_mapping(hid, rep->field[i],
> -						     rep->field[i]->usage + j);
> +			for (j = 0; j < rep->field[i]->maxusage; j++) {
> +				/* Verify if Battery Strength feature is available */
> +				if (((rep->field[i]->usage + j)->hid & HID_USAGE_PAGE) ==
> HID_UP_GENDEVCTRLS &&
> +					((rep->field[i]->usage + j)->hid & HID_USAGE) == 0x20) {
> +					hidinput_setup_battery(hid,
> +							       rep->id,
> +							       rep->field[i]->logical_minimum,
> +							       rep->field[i]->logical_maximum);
> +				}
> +
> +				if (drv->feature_mapping)
> +					drv->feature_mapping(hid, rep->field[i],
> +							     rep->field[i]->usage + j);
> +			}
I'd be inclined to split all this out into a separate function, so that
you can make it common with the code on the input path.

    J
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] hid-input: add support for HID devices reporting Battery Strength

[Daniel Nicoletti]

Thanks for the reply,

2011/12/2 Jeremy Fitzhardinge <jeremy@goop.org>:
>> If I understood correctly when talking to Jeremy he said his device
>> never actually
>> reported the status as an input event (sorry if I didn't understand it
>> correctly),
>
> No, it does.  When the mouse first connects it reports the battery level
> as an Input event.  Without my patch, X sees the battery level as a
> strange little absolute axis on the mouse, which it ignores.
>
> I don't know if the mouse also reports the battery later on
> spontaneously; I haven't observed it, so I suspect you may be right that
> you have to poll it to get further updates.

Right, my devices don't send any data if not asked, even if the battery change
it's status (what did happen because I've put some old batteries.
So in this case we will need to keep your code on the raw event and probably
check that into the get_properties part.

>
>>  and
>> after reading HID specs I believe it's really because it was meant to be probed,
>> I have an Apple Keyboard and Magic Trackpad both bluetooth batteries operated,
>> so using PacketLogger I saw that Mac OSX always ask the battery status using
>> the so called GetFeature.
>
> Could you cite a specific reference?  I don't see any references to
> "GetFeature" in HID spec I have.  The closest I see is
> "Get_Report(INPUT/OUTPUT/FEATURE)", which specifically says "This
> request is not intended to be used for polling the device state on a
> regular basis".  Though I'm not at all surprised if device manufacturers
> ignore this, or that it is contradictory with some other part of the spec.

Sure, HUT1_12v2.pdf section 4.8 Feature Notifications, at the end you
will see GetReport(Feature) (sorry it was GetReport() not GetFeature()).
Also when I run my code hcidump shows this, which means it knows what
a GetReport is: (and Mac OSX PackatLogger.app too)
HIDP: Get report: Feature report

>> What my patch does is basically:
>> - store the report id that matches the battery_strength
>> - setup the battery if 0x6.0x20 is found, even if that is reported as a feature
>>   (as it was meant to be but only the MagicTrackpad does)
>> - when upower or someone access /sys/class/power_supply/hid-*/capacity it
>>   will probe the device and return it's status.
>
> How often does usermode read this file?  Does it/can it select on the
> file to wait for changes.
>
> Given that battery level shouldn't change that quickly, I wonder if
> having a timer poll the device every minute or something of that order
> might be useful?

Yes, this is concerning but imo the /sys.../capacity file should return
the current value, and to do that it need to probe, it the probe timer
is too often then upower needs fixing, we have done our part into the
kernel...

>> It works great for both devices, but I have two concerns:
>> - the report_features function has a duplicated code
>
> Yes, but that's easy to fix.
>
>> - it would be nice if it was possible for specific drivers to provide their own
>>   probe as there might be some strange devices... (but maybe it's
>> already possible)
> I'd wait until there's a device which can't be handled by this mechanism
> before trying to generalize it.  Unless we can unify the wacom driver,
> but that seems to be in its own little world.

sounds like a better idea.

>> I've talked to the upower dev and he fixed it to be able to show the
>> right percentage.
>
> Cool, but I have some concerns about that (see below).
>
>> Here how the uevent file (in /sys/class/power_supply/hid-*/) looks like:
>> POWER_SUPPLY_NAME=hid-00:22:41:D9:18:E7-battery
>> POWER_SUPPLY_PRESENT=1
>> POWER_SUPPLY_ONLINE=1
>> POWER_SUPPLY_CAPACITY=66
>> POWER_SUPPLY_MODEL_NAME=MacAdmin’s keyboard
>> POWER_SUPPLY_STATUS=Discharging
>>
>> POWER_SUPPLY_NAME=hid-70:CD:60:F5:FF:3F-battery
>> POWER_SUPPLY_PRESENT=1
>> POWER_SUPPLY_ONLINE=1
>> POWER_SUPPLY_CAPACITY=62
>> POWER_SUPPLY_MODEL_NAME=nexx’s Trackpad
>> POWER_SUPPLY_STATUS=Discharging
>>
>> and the patch (note that this is my first kernel patch so sorry if
>> formatting is not ok, should I attach it?):
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index b9b8c75..8fac47c 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -277,6 +277,7 @@ static enum power_supply_property
>> hidinput_battery_props[] = {
>>       POWER_SUPPLY_PROP_ONLINE,
>>       POWER_SUPPLY_PROP_CAPACITY,
>>       POWER_SUPPLY_PROP_MODEL_NAME,
>> +     POWER_SUPPLY_PROP_STATUS
>>  };
>>
>>  static int hidinput_get_battery_property(struct power_supply *psy,
>> @@ -285,6 +286,9 @@ static int hidinput_get_battery_property(struct
>> power_supply *psy,
>>  {
>>       struct hid_device *dev = container_of(psy, struct hid_device, battery);
>>       int ret = 0;
>> +     int ret_rep;
>> +     __u8 *buf = NULL;
>> +     unsigned char report_number = dev->battery_report_id;
>>
>>       switch (prop) {
>>       case POWER_SUPPLY_PROP_PRESENT:
>> @@ -293,28 +297,45 @@ static int hidinput_get_battery_property(struct
>> power_supply *psy,
>>               break;
>>
>>       case POWER_SUPPLY_PROP_CAPACITY:
>> -             if (dev->battery_min < dev->battery_max &&
>> -                 dev->battery_val >= dev->battery_min &&
>> -                 dev->battery_val <= dev->battery_max)
>> -                     val->intval = (100 * (dev->battery_val - dev->battery_min)) /
>> -                             (dev->battery_max - dev->battery_min);
>> -             else
>> +             buf = kmalloc(2 * sizeof(__u8), GFP_KERNEL);
>
> Why allocate a two byte buffer?  Why not just put it on the stack?  How
> do you know that's the right size?

Sorry I'm not kernel expert I just saw some other code working like this..
I know the size because the first byte is the report_id (in my case 0x47 or 71),
but the following bytes are reported by Report Size(8) and Report Count(1)
as the raw function calculates the count by the buf size.
(note that I might be wrong, but for my devices it works like this).

>
>> +             if (!buf) {
>> +                     ret = -ENOMEM;
>> +                     break;
>> +             }
>> +
>> +             memset(buf, 0, sizeof(buf));
>> +             ret_rep = dev->hid_get_raw_report(dev, report_number, buf,
>> sizeof(buf), HID_FEATURE_REPORT);
>> +             if (ret_rep != 2) {
>>                       ret = -EINVAL;
>> +                     break;
>> +             }
>> +
>> +             /* store the returned value */
>> +             /* I'm not calculating this using the logical_minimum and maximum */
>> +             /* because my device returns 0-100 even though the min and max are 0-255 */
>> +             val->intval = buf[1];
>
> That's definitely not the case for my mouse.  I'm not sure what the
> range actually is, but I've seen values reported > 100 with fresh
> alkaline batteries.  Initially I also thought it was using a 0-100 range
> despite the min/max, and I added a per-device quirk mechanism - but
> since my device didn't need it, I did't include it in the patch
> posting.  I'd reinstate it if there are devices that report 0-100.

Sure, then we need some quirks... As even the keyboar that reports the max as
255 actually returns in 0-100 interval..

>
>>               break;
>>
>>       case POWER_SUPPLY_PROP_MODEL_NAME:
>>               val->strval = dev->name;
>>               break;
>>
>> +     case POWER_SUPPLY_PROP_STATUS:
>> +             val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
>> +             break;
>> +
>>       default:
>>               ret = -EINVAL;
>>               break;
>>       }
>>
>> +     if (buf) {
>> +             kfree(buf);
>> +     }
>>       return ret;
>>  }
>>
>> -static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
>> +static void hidinput_setup_battery(struct hid_device *dev, unsigned
>> id, s32 min, s32 max)
>>  {
>>       struct power_supply *battery = &dev->battery;
>>       int ret;
>> @@ -326,7 +347,7 @@ static void hidinput_setup_battery(struct
>> hid_device *dev, s32 min, s32 max)
>>       if (battery->name == NULL)
>>               return;
>>
>> -     battery->type = POWER_SUPPLY_TYPE_BATTERY;
>> +     battery->type = POWER_SUPPLY_TYPE_USB;
>
> What's the purpose of this?  Is it to make upower behave in a
> different/better way?  My reading is that this means that it's powered
> by USB ("Standard Downstream Port").  A bluetooth device doesn't have
> any USB involved at all (except if the BT adapter itself is USB, which
> is moot).
>
> If upower is getting confused thinking that a "battery" is necessarily a
> laptop battery, then I think it needs to be fixed to pay closer
> attention to the device tree topology.
>
> (looks up upower) Ah, I see.
>
> The changes in the current upower.git look very suspect to me, if
> nothing else because of the hard-coded "magicmouse" and "wacom", and the
> use of POWER_SUPPLY_TYPE_USB.  Can upower not look to see if there's a
> power-supply hanging off a HID device and report that as peripheral power?

I have to talk to Richard (ah you CC him in this email..), but tbh I think the
change in upower should just look at what properties the battery is
capable of providing,
I changed it to USB since IIRC using it as BATTERY made it think it was a power
supply of the computer..

>
>> -     if (!drv->feature_mapping)
>> -             return;
>> -
>>       rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
>>       list_for_each_entry(rep, &rep_enum->report_list, list)
>>               for (i = 0; i < rep->maxfield; i++)
>> -                     for (j = 0; j < rep->field[i]->maxusage; j++)
>> -                             drv->feature_mapping(hid, rep->field[i],
>> -                                                  rep->field[i]->usage + j);
>> +                     for (j = 0; j < rep->field[i]->maxusage; j++) {
>> +                             /* Verify if Battery Strength feature is available */
>> +                             if (((rep->field[i]->usage + j)->hid & HID_USAGE_PAGE) ==
>> HID_UP_GENDEVCTRLS &&
>> +                                     ((rep->field[i]->usage + j)->hid & HID_USAGE) == 0x20) {
>> +                                     hidinput_setup_battery(hid,
>> +                                                            rep->id,
>> +                                                            rep->field[i]->logical_minimum,
>> +                                                            rep->field[i]->logical_maximum);
>> +                             }
>> +
>> +                             if (drv->feature_mapping)
>> +                                     drv->feature_mapping(hid, rep->field[i],
>> +                                                          rep->field[i]->usage + j);
>> +                     }
> I'd be inclined to split all this out into a separate function, so that
> you can make it common with the code on the input path.

Yes, might be better I couldn't think of a nice way to share this part..
But in the end does my Patch probes your device? :D

Best,
Daniel
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] hid-input: add support for HID devices reporting Battery Strength

[Jeremy Fitzhardinge]

On 12/02/2011 10:29 AM, Daniel Nicoletti wrote:
> Thanks for the reply,
>
> 2011/12/2 Jeremy Fitzhardinge <jeremy@goop.org>:
>>> If I understood correctly when talking to Jeremy he said his device
>>> never actually
>>> reported the status as an input event (sorry if I didn't understand it
>>> correctly),
>> No, it does.  When the mouse first connects it reports the battery level
>> as an Input event.  Without my patch, X sees the battery level as a
>> strange little absolute axis on the mouse, which it ignores.
>>
>> I don't know if the mouse also reports the battery later on
>> spontaneously; I haven't observed it, so I suspect you may be right that
>> you have to poll it to get further updates.
> Right, my devices don't send any data if not asked, even if the battery change
> it's status (what did happen because I've put some old batteries.
> So in this case we will need to keep your code on the raw event and probably
> check that into the get_properties part.
>
>>>  and
>>> after reading HID specs I believe it's really because it was meant to be probed,
>>> I have an Apple Keyboard and Magic Trackpad both bluetooth batteries operated,
>>> so using PacketLogger I saw that Mac OSX always ask the battery status using
>>> the so called GetFeature.
>> Could you cite a specific reference?  I don't see any references to
>> "GetFeature" in HID spec I have.  The closest I see is
>> "Get_Report(INPUT/OUTPUT/FEATURE)", which specifically says "This
>> request is not intended to be used for polling the device state on a
>> regular basis".  Though I'm not at all surprised if device manufacturers
>> ignore this, or that it is contradictory with some other part of the spec.
> Sure, HUT1_12v2.pdf section 4.8 Feature Notifications, at the end you
> will see GetReport(Feature) (sorry it was GetReport() not GetFeature()).
> Also when I run my code hcidump shows this, which means it knows what
> a GetReport is: (and Mac OSX PackatLogger.app too)
> HIDP: Get report: Feature report
>
>>> What my patch does is basically:
>>> - store the report id that matches the battery_strength
>>> - setup the battery if 0x6.0x20 is found, even if that is reported as a feature
>>>   (as it was meant to be but only the MagicTrackpad does)
>>> - when upower or someone access /sys/class/power_supply/hid-*/capacity it
>>>   will probe the device and return it's status.
>> How often does usermode read this file?  Does it/can it select on the
>> file to wait for changes.
>>
>> Given that battery level shouldn't change that quickly, I wonder if
>> having a timer poll the device every minute or something of that order
>> might be useful?
> Yes, this is concerning but imo the /sys.../capacity file should return
> the current value, and to do that it need to probe, it the probe timer
> is too often then upower needs fixing, we have done our part into the
> kernel...
>
>>> It works great for both devices, but I have two concerns:
>>> - the report_features function has a duplicated code
>> Yes, but that's easy to fix.
>>
>>> - it would be nice if it was possible for specific drivers to provide their own
>>>   probe as there might be some strange devices... (but maybe it's
>>> already possible)
>> I'd wait until there's a device which can't be handled by this mechanism
>> before trying to generalize it.  Unless we can unify the wacom driver,
>> but that seems to be in its own little world.
> sounds like a better idea.
>
>>> I've talked to the upower dev and he fixed it to be able to show the
>>> right percentage.
>> Cool, but I have some concerns about that (see below).
>>
>>> Here how the uevent file (in /sys/class/power_supply/hid-*/) looks like:
>>> POWER_SUPPLY_NAME=hid-00:22:41:D9:18:E7-battery
>>> POWER_SUPPLY_PRESENT=1
>>> POWER_SUPPLY_ONLINE=1
>>> POWER_SUPPLY_CAPACITY=66
>>> POWER_SUPPLY_MODEL_NAME=MacAdmin’s keyboard
>>> POWER_SUPPLY_STATUS=Discharging
>>>
>>> POWER_SUPPLY_NAME=hid-70:CD:60:F5:FF:3F-battery
>>> POWER_SUPPLY_PRESENT=1
>>> POWER_SUPPLY_ONLINE=1
>>> POWER_SUPPLY_CAPACITY=62
>>> POWER_SUPPLY_MODEL_NAME=nexx’s Trackpad
>>> POWER_SUPPLY_STATUS=Discharging
>>>
>>> and the patch (note that this is my first kernel patch so sorry if
>>> formatting is not ok, should I attach it?):
>>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>>> index b9b8c75..8fac47c 100644
>>> --- a/drivers/hid/hid-input.c
>>> +++ b/drivers/hid/hid-input.c
>>> @@ -277,6 +277,7 @@ static enum power_supply_property
>>> hidinput_battery_props[] = {
>>>       POWER_SUPPLY_PROP_ONLINE,
>>>       POWER_SUPPLY_PROP_CAPACITY,
>>>       POWER_SUPPLY_PROP_MODEL_NAME,
>>> +     POWER_SUPPLY_PROP_STATUS
>>>  };
>>>
>>>  static int hidinput_get_battery_property(struct power_supply *psy,
>>> @@ -285,6 +286,9 @@ static int hidinput_get_battery_property(struct
>>> power_supply *psy,
>>>  {
>>>       struct hid_device *dev = container_of(psy, struct hid_device, battery);
>>>       int ret = 0;
>>> +     int ret_rep;
>>> +     __u8 *buf = NULL;
>>> +     unsigned char report_number = dev->battery_report_id;
>>>
>>>       switch (prop) {
>>>       case POWER_SUPPLY_PROP_PRESENT:
>>> @@ -293,28 +297,45 @@ static int hidinput_get_battery_property(struct
>>> power_supply *psy,
>>>               break;
>>>
>>>       case POWER_SUPPLY_PROP_CAPACITY:
>>> -             if (dev->battery_min < dev->battery_max &&
>>> -                 dev->battery_val >= dev->battery_min &&
>>> -                 dev->battery_val <= dev->battery_max)
>>> -                     val->intval = (100 * (dev->battery_val - dev->battery_min)) /
>>> -                             (dev->battery_max - dev->battery_min);
>>> -             else
>>> +             buf = kmalloc(2 * sizeof(__u8), GFP_KERNEL);
>> Why allocate a two byte buffer?  Why not just put it on the stack?  How
>> do you know that's the right size?
> Sorry I'm not kernel expert I just saw some other code working like this..
> I know the size because the first byte is the report_id (in my case 0x47 or 71),
> but the following bytes are reported by Report Size(8) and Report Count(1)
> as the raw function calculates the count by the buf size.
> (note that I might be wrong, but for my devices it works like this).
>
>>> +             if (!buf) {
>>> +                     ret = -ENOMEM;
>>> +                     break;
>>> +             }
>>> +
>>> +             memset(buf, 0, sizeof(buf));
>>> +             ret_rep = dev->hid_get_raw_report(dev, report_number, buf,
>>> sizeof(buf), HID_FEATURE_REPORT);
>>> +             if (ret_rep != 2) {
>>>                       ret = -EINVAL;
>>> +                     break;
>>> +             }
>>> +
>>> +             /* store the returned value */
>>> +             /* I'm not calculating this using the logical_minimum and maximum */
>>> +             /* because my device returns 0-100 even though the min and max are 0-255 */
>>> +             val->intval = buf[1];
>> That's definitely not the case for my mouse.  I'm not sure what the
>> range actually is, but I've seen values reported > 100 with fresh
>> alkaline batteries.  Initially I also thought it was using a 0-100 range
>> despite the min/max, and I added a per-device quirk mechanism - but
>> since my device didn't need it, I did't include it in the patch
>> posting.  I'd reinstate it if there are devices that report 0-100.
> Sure, then we need some quirks... As even the keyboar that reports the max as
> 255 actually returns in 0-100 interval..
>
>>>               break;
>>>
>>>       case POWER_SUPPLY_PROP_MODEL_NAME:
>>>               val->strval = dev->name;
>>>               break;
>>>
>>> +     case POWER_SUPPLY_PROP_STATUS:
>>> +             val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
>>> +             break;
>>> +
>>>       default:
>>>               ret = -EINVAL;
>>>               break;
>>>       }
>>>
>>> +     if (buf) {
>>> +             kfree(buf);
>>> +     }
>>>       return ret;
>>>  }
>>>
>>> -static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
>>> +static void hidinput_setup_battery(struct hid_device *dev, unsigned
>>> id, s32 min, s32 max)
>>>  {
>>>       struct power_supply *battery = &dev->battery;
>>>       int ret;
>>> @@ -326,7 +347,7 @@ static void hidinput_setup_battery(struct
>>> hid_device *dev, s32 min, s32 max)
>>>       if (battery->name == NULL)
>>>               return;
>>>
>>> -     battery->type = POWER_SUPPLY_TYPE_BATTERY;
>>> +     battery->type = POWER_SUPPLY_TYPE_USB;
>> What's the purpose of this?  Is it to make upower behave in a
>> different/better way?  My reading is that this means that it's powered
>> by USB ("Standard Downstream Port").  A bluetooth device doesn't have
>> any USB involved at all (except if the BT adapter itself is USB, which
>> is moot).
>>
>> If upower is getting confused thinking that a "battery" is necessarily a
>> laptop battery, then I think it needs to be fixed to pay closer
>> attention to the device tree topology.
>>
>> (looks up upower) Ah, I see.
>>
>> The changes in the current upower.git look very suspect to me, if
>> nothing else because of the hard-coded "magicmouse" and "wacom", and the
>> use of POWER_SUPPLY_TYPE_USB.  Can upower not look to see if there's a
>> power-supply hanging off a HID device and report that as peripheral power?
> I have to talk to Richard (ah you CC him in this email..), but tbh I think the
> change in upower should just look at what properties the battery is
> capable of providing,
> I changed it to USB since IIRC using it as BATTERY made it think it was a power
> supply of the computer..

Yes, but I think that's either a bug in upower or an ambiguity in what
power_supply means (or perhaps, what a power_supply is powering) -
either way, that's something deserving of further discussion.

But either way, the mouse definitely isn't being powered by USB - the
whole point is that it's battery powered.  The kernel shouldn't tell
lies to work around other problems.

>>> -     if (!drv->feature_mapping)
>>> -             return;
>>> -
>>>       rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
>>>       list_for_each_entry(rep, &rep_enum->report_list, list)
>>>               for (i = 0; i < rep->maxfield; i++)
>>> -                     for (j = 0; j < rep->field[i]->maxusage; j++)
>>> -                             drv->feature_mapping(hid, rep->field[i],
>>> -                                                  rep->field[i]->usage + j);
>>> +                     for (j = 0; j < rep->field[i]->maxusage; j++) {
>>> +                             /* Verify if Battery Strength feature is available */
>>> +                             if (((rep->field[i]->usage + j)->hid & HID_USAGE_PAGE) ==
>>> HID_UP_GENDEVCTRLS &&
>>> +                                     ((rep->field[i]->usage + j)->hid & HID_USAGE) == 0x20) {
>>> +                                     hidinput_setup_battery(hid,
>>> +                                                            rep->id,
>>> +                                                            rep->field[i]->logical_minimum,
>>> +                                                            rep->field[i]->logical_maximum);
>>> +                             }
>>> +
>>> +                             if (drv->feature_mapping)
>>> +                                     drv->feature_mapping(hid, rep->field[i],
>>> +                                                          rep->field[i]->usage + j);
>>> +                     }
>> I'd be inclined to split all this out into a separate function, so that
>> you can make it common with the code on the input path.
> Yes, might be better I couldn't think of a nice way to share this part..
> But in the end does my Patch probes your device? :D

Yes, it does!  I changed it to ask for the appropriate report
(INPUT/FEATURE) depending on how it appeared in the original descriptor,
and made a few other cleanups, which I'll post shortly.

    J
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[GIT PULL RFC] directly poll battery strength when reading power_supply

[Jeremy Fitzhardinge]

Hi,

This series of patches includes Daniel's patch to directly poll for the battery strength rather than
hoping the device will happen to send it, along with some cleanup patches from me.

The only functional change I made from Daniel's patch is keeping the power_supply type as BATTERY rather
than USB.

	J

The following changes since commit 1b2bdc70c10f9bf1455ea76a558261d7431b533c:

  hid-input: add support for HID devices reporting Battery Strength (2011-11-23 00:38:25 -0800)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git hid-battery

Daniel Nicoletti (1):
      hid-input: add support for HID devices reporting Battery Strength

Jeremy Fitzhardinge (7):
      hid-input: fix compile for !HID_BATTERY_STRENGTH
      hid-input/battery: remove apparently redundant kmalloc
      hid-input/battery: add quirks for battery
      hid-input/battery: deal with both FEATURE and INPUT report batteries
      hid-input/battery: make the battery setup common for INPUTs and FEATUREs
      hid-input/battery: power-supply type really *is* a battery
      hid-input/battery: remove battery_val

 drivers/hid/hid-core.c  |    2 +-
 drivers/hid/hid-input.c |  105 +++++++++++++++++++++++++++++++++++------------
 include/linux/hid.h     |    5 ++-
 3 files changed, 83 insertions(+), 29 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 848a56c..70f5192 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1157,7 +1157,7 @@ static bool hid_match_one_id(struct hid_device *hdev,
 		(id->product == HID_ANY_ID || id->product == hdev->product);
 }
 
-static const struct hid_device_id *hid_match_id(struct hid_device *hdev,
+const struct hid_device_id *hid_match_id(struct hid_device *hdev,
 		const struct hid_device_id *id)
 {
 	for (; id->bus; id++)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 83afb86..7360351 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -32,6 +32,8 @@
 #include <linux/hid.h>
 #include <linux/hid-debug.h>
 
+#include "hid-ids.h"
+
 #define unk	KEY_UNKNOWN
 
 static const unsigned char hid_keyboard[256] = {
@@ -277,14 +279,39 @@ static enum power_supply_property hidinput_battery_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
 	POWER_SUPPLY_PROP_CAPACITY,
 	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_STATUS
 };
 
+#define HID_BATTERY_QUIRK_PERCENT	(1 << 0) /* always reports percent */
+
+static const struct hid_device_id hid_battery_quirks[] = {
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICMOUSE),
+	  HID_BATTERY_QUIRK_PERCENT },
+	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE, USB_DEVICE_ID_APPLE_MAGICTRACKPAD),
+	  HID_BATTERY_QUIRK_PERCENT },
+	{}
+};
+
+static unsigned find_battery_quirk(struct hid_device *hdev)
+{
+	unsigned quirks = 0;
+	const struct hid_device_id *match;
+
+	match = hid_match_id(hdev, hid_battery_quirks);
+	if (match != NULL)
+		quirks = match->driver_data;
+
+	return quirks;
+}
+
 static int hidinput_get_battery_property(struct power_supply *psy,
 					 enum power_supply_property prop,
 					 union power_supply_propval *val)
 {
 	struct hid_device *dev = container_of(psy, struct hid_device, battery);
 	int ret = 0;
+	int ret_rep;
+	__u8 buf[2] = {};
 
 	switch (prop) {
 	case POWER_SUPPLY_PROP_PRESENT:
@@ -293,19 +320,29 @@ static int hidinput_get_battery_property(struct power_supply *psy,
 		break;
 
 	case POWER_SUPPLY_PROP_CAPACITY:
+		ret_rep = dev->hid_get_raw_report(dev, dev->battery_report_id,
+						  buf, sizeof(buf),
+						  dev->battery_report_type);
+		if (ret_rep != 2) {
+			ret = -EINVAL;
+			break;
+		}
+
 		if (dev->battery_min < dev->battery_max &&
-		    dev->battery_val >= dev->battery_min &&
-		    dev->battery_val <= dev->battery_max)
-			val->intval = (100 * (dev->battery_val - dev->battery_min)) /
+		    buf[1] >= dev->battery_min &&
+		    buf[1] <= dev->battery_max)
+			val->intval = (100 * (buf[1] - dev->battery_min)) /
 				(dev->battery_max - dev->battery_min);
-		else
-			ret = -EINVAL;
 		break;
 
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = dev->name;
 		break;
 
+	case POWER_SUPPLY_PROP_STATUS:
+		val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		break;
+
 	default:
 		ret = -EINVAL;
 		break;
@@ -314,17 +351,22 @@ static int hidinput_get_battery_property(struct power_supply *psy,
 	return ret;
 }
 
-static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
+static bool hidinput_setup_battery(struct hid_device *dev, unsigned report_type, struct hid_field *field)
 {
 	struct power_supply *battery = &dev->battery;
 	int ret;
+	unsigned quirks;
+	s32 min, max;
+
+	if (field->usage->hid != HID_DC_BATTERYSTRENGTH)
+		return false;	/* no match */
 
 	if (battery->name != NULL)
-		return;		/* already initialized? */
+		goto out;	/* already initialized? */
 
 	battery->name = kasprintf(GFP_KERNEL, "hid-%s-battery", dev->uniq);
 	if (battery->name == NULL)
-		return;
+		goto out;
 
 	battery->type = POWER_SUPPLY_TYPE_BATTERY;
 	battery->properties = hidinput_battery_props;
@@ -332,8 +374,20 @@ static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
 	battery->use_for_apm = 0;
 	battery->get_property = hidinput_get_battery_property;
 
+	quirks = find_battery_quirk(dev);
+
+	min = field->logical_minimum;
+	max = field->logical_maximum;
+
+	if (quirks & HID_BATTERY_QUIRK_PERCENT) {
+		min = 0;
+		max = 100;
+	}
+
 	dev->battery_min = min;
 	dev->battery_max = max;
+	dev->battery_report_type = report_type;
+	dev->battery_report_id = field->report->id;
 
 	ret = power_supply_register(&dev->dev, battery);
 	if (ret != 0) {
@@ -341,6 +395,9 @@ static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
 		kfree(battery->name);
 		battery->name = NULL;
 	}
+
+out:
+	return true;
 }
 
 static void hidinput_cleanup_battery(struct hid_device *dev)
@@ -353,8 +410,10 @@ static void hidinput_cleanup_battery(struct hid_device *dev)
 	dev->battery.name = NULL;
 }
 #else  /* !CONFIG_HID_BATTERY_STRENGTH */
-static void hidinput_setup_battery(struct hid_device *dev, s32 min, s32 max)
+static bool hidinput_setup_battery(struct hid_device *dev, unsigned report_type,
+				   struct hid_field *field)
 {
+	return false;
 }
 
 static void hidinput_cleanup_battery(struct hid_device *dev)
@@ -721,12 +780,9 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 		break;
 
 	case HID_UP_GENDEVCTRLS:
-		if ((usage->hid & HID_USAGE) == 0x20) {	/* Battery Strength */
-			hidinput_setup_battery(device,
-					       field->logical_minimum,
-					       field->logical_maximum);
+		if (hidinput_setup_battery(device, HID_INPUT_REPORT, field))
 			goto ignore;
-		} else
+		else
 			goto unknown;
 		break;
 
@@ -861,13 +917,6 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 
 	input = field->hidinput->input;
 
-	if (usage->hid == HID_DC_BATTERYSTRENGTH) {
-		hid->battery_val = value;
-		hid_dbg(hid, "battery value is %d (range %d-%d)\n",
-			value, hid->battery_min, hid->battery_max);
-		return;
-	}
-
 	if (!usage->type)
 		return;
 
@@ -990,15 +1039,17 @@ static void report_features(struct hid_device *hid)
 	struct hid_report *rep;
 	int i, j;
 
-	if (!drv->feature_mapping)
-		return;
-
 	rep_enum = &hid->report_enum[HID_FEATURE_REPORT];
 	list_for_each_entry(rep, &rep_enum->report_list, list)
 		for (i = 0; i < rep->maxfield; i++)
-			for (j = 0; j < rep->field[i]->maxusage; j++)
-				drv->feature_mapping(hid, rep->field[i],
-						     rep->field[i]->usage + j);
+			for (j = 0; j < rep->field[i]->maxusage; j++) {
+				/* Verify if Battery Strength feature is available */
+				hidinput_setup_battery(hid, HID_FEATURE_REPORT, rep->field[i]);
+
+				if (drv->feature_mapping)
+					drv->feature_mapping(hid, rep->field[i],
+							     rep->field[i]->usage + j);
+			}
 }
 
 /*
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 214801d..49d116e 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -495,7 +495,8 @@ struct hid_device {							/* device report descriptor */
 	struct power_supply battery;
 	__s32 battery_min;
 	__s32 battery_max;
-	__s32 battery_val;
+	__s32 battery_report_type;
+	__s32 battery_report_id;
 #endif
 
 	unsigned int status;						/* see STAT flags above */
@@ -735,6 +736,8 @@ int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
 int hid_check_keys_pressed(struct hid_device *hid);
 int hid_connect(struct hid_device *hid, unsigned int connect_mask);
 void hid_disconnect(struct hid_device *hid);
+const struct hid_device_id *hid_match_id(struct hid_device *hdev,
+					 const struct hid_device_id *id);
 
 /**
  * hid_map_usage - map usage input bits

Re: [GIT PULL RFC] directly poll battery strength when reading power_supply

[Jiri Kosina]

On Fri, 2 Dec 2011, Jeremy Fitzhardinge wrote:

> This series of patches includes Daniel's patch to directly poll for the battery strength rather than
> hoping the device will happen to send it, along with some cleanup patches from me.
> 
> The only functional change I made from Daniel's patch is keeping the 
> power_supply type as BATTERY rather than USB.
> 
> 	J
> 
> The following changes since commit 1b2bdc70c10f9bf1455ea76a558261d7431b533c:
> 
>   hid-input: add support for HID devices reporting Battery Strength (2011-11-23 00:38:25 -0800)
> 
> are available in the git repository at:
>   git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git hid-battery
> 
> Daniel Nicoletti (1):
>       hid-input: add support for HID devices reporting Battery Strength
> 
> Jeremy Fitzhardinge (7):
>       hid-input: fix compile for !HID_BATTERY_STRENGTH
>       hid-input/battery: remove apparently redundant kmalloc
>       hid-input/battery: add quirks for battery
>       hid-input/battery: deal with both FEATURE and INPUT report batteries
>       hid-input/battery: make the battery setup common for INPUTs and FEATUREs
>       hid-input/battery: power-supply type really *is* a battery
>       hid-input/battery: remove battery_val

I actually like the series. The only objection I'd have is that Daniel's 
patch is not properly Signed-off-by:, and therefore I am not pulling it 
now.

Thanks in advance,

-- 
Jiri Kosina
SUSE Labs

Re: [GIT PULL RFC] directly poll battery strength when reading power_supply

[Jeremy Fitzhardinge]

On 12/06/2011 01:17 AM, Jiri Kosina wrote:
> On Fri, 2 Dec 2011, Jeremy Fitzhardinge wrote:
>
>> This series of patches includes Daniel's patch to directly poll for the battery strength rather than
>> hoping the device will happen to send it, along with some cleanup patches from me.
>>
>> The only functional change I made from Daniel's patch is keeping the 
>> power_supply type as BATTERY rather than USB.
>>
>> 	J
>>
>> The following changes since commit 1b2bdc70c10f9bf1455ea76a558261d7431b533c:
>>
>>   hid-input: add support for HID devices reporting Battery Strength (2011-11-23 00:38:25 -0800)
>>
>> are available in the git repository at:
>>   git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git hid-battery
>>
>> Daniel Nicoletti (1):
>>       hid-input: add support for HID devices reporting Battery Strength
>>
>> Jeremy Fitzhardinge (7):
>>       hid-input: fix compile for !HID_BATTERY_STRENGTH
>>       hid-input/battery: remove apparently redundant kmalloc
>>       hid-input/battery: add quirks for battery
>>       hid-input/battery: deal with both FEATURE and INPUT report batteries
>>       hid-input/battery: make the battery setup common for INPUTs and FEATUREs
>>       hid-input/battery: power-supply type really *is* a battery
>>       hid-input/battery: remove battery_val
> I actually like the series. The only objection I'd have is that Daniel's 
> patch is not properly Signed-off-by:, and therefore I am not pulling it 
> now.

OK.  Daniel, can I add your S-O-B to your patch?

It also seems that it doesn't work with Apple devices yet, so it needs
another iteration or so.

    J

Re: [GIT PULL RFC] directly poll battery strength when reading power_supply

[Daniel Nicoletti]

Hi,
I was just informed that my HID patch to probe the
battery status was merged upstream, but it misses
my:
USB_DEVICE_ID_APPLE_ALU_WIRELESS_ANSI
on the hid-input.c file, also if you notice Jeremy Fitzhardinge
tested on  a 2011 model which had the issue, my model
is from 2007 and also has the issue, shouldn't we include
the models in between?
As I don't have commit access should I make a proper
patch just for this extra quirk?

BTW, these Apple devices allow for changing their
names, when you pair them on a Mac the OS put
it's hostname on the device, I don't think automating
to this level is useful but the feature is important
since my devices have my boss Mac hostname :/
is this already available on HID devices or would be
another specif addition?

Thanks.

static const struct hid_device_id hid_battery_quirks[] = {
	{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
			       USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI),
	  HID_BATTERY_QUIRK_PERCENT | HID_BATTERY_QUIRK_FEATURE },
	{}
};

Re: [GIT PULL RFC] directly poll battery strength when reading power_supply

[Richard Hughes]

On 3 December 2011 06:13, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> The only functional change I made from Daniel's patch is keeping the power_supply type as BATTERY rather
> than USB.

How is upower supposed to know that the device is:

a) a mouse
b) not powering the computer

Any ideas welcome. I suppose we could look at the hid device, but this
seems like a hack. Thanks.

Richard.

Re: [GIT PULL RFC] directly poll battery strength when reading power_supply

[Jeremy Fitzhardinge]

On 12/06/2011 01:56 AM, Richard Hughes wrote:
> On 3 December 2011 06:13, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>> The only functional change I made from Daniel's patch is keeping the power_supply type as BATTERY rather
>> than USB.
> How is upower supposed to know that the device is:
>
> a) a mouse
> b) not powering the computer
>
> Any ideas welcome. I suppose we could look at the hid device, but this
> seems like a hack. Thanks.

(CC power_supply maintainers: the discussion is how to represent
power_supplies for self-powered HID devices like wireless mice.)

I've been thinking about this.  I think the obvious fix is that the
power_supply should have a link to the root of the device tree it
powers.   So the system battery and line power should point to the root,
whereas a battery powered device would point to itself.
Something like  HID UPS would appear to be in the middle of the USB
devices, but actually powers the whole machine (or more).

In other words, the power_supply device is the management interface for
the power supply, and it appears in the device tree wherever it was
detected, but that location only has a vague and implicit relationship
with the hardware it actually powers; it needs another attribute to
explicitly denote that information.

And if the device being pointed to is a mouse, then upower knows its
powering a mouse.

I'll look at putting together a patch to see if my suggestion makes any
sense in practice.

Thanks,
    J

Re: [GIT PULL RFC] directly poll battery strength when reading power_supply

[Richard Hughes]

On 6 December 2011 17:10, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> Something like  HID UPS would appear to be in the middle of the USB
> devices, but actually powers the whole machine (or more).

Right, although I think we still need an explicit
"is_powering_computer" flag or something exposed to userspace
ortherwise we're back to heuristics and hand coded quirks.

> And if the device being pointed to is a mouse, then upower knows its
> powering a mouse.

Define "mouse" -- an input devices with two axis? I think an explicit
type might be clearer.

Richard.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL RFC] directly poll battery strength when reading power_supply

[Jeremy Fitzhardinge]

On 12/07/2011 04:51 AM, Richard Hughes wrote:
> On 6 December 2011 17:10, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>> Something like  HID UPS would appear to be in the middle of the USB
>> devices, but actually powers the whole machine (or more).
> Right, although I think we still need an explicit
> "is_powering_computer" flag or something exposed to userspace
> ortherwise we're back to heuristics and hand coded quirks.

Something like this?

diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index e15d4c9..21178eb 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
 	static char *capacity_level_text[] = {
 		"Unknown", "Critical", "Low", "Normal", "High", "Full"
 	};
+	static char *scope_text[] = {
+		"Unknown", "System", "Device"
+	};
 	ssize_t ret = 0;
 	struct power_supply *psy = dev_get_drvdata(dev);
 	const ptrdiff_t off = attr - power_supply_attrs;
@@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
 		return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
 	else if (off == POWER_SUPPLY_PROP_TYPE)
 		return sprintf(buf, "%s\n", type_text[value.intval]);
+	else if (off == POWER_SUPPLY_PROP_SCOPE)
+		return sprintf(buf, "%s\n", scope_text[value.intval]);
 	else if (off >= POWER_SUPPLY_PROP_MODEL_NAME)
 		return sprintf(buf, "%s\n", value.strval);
 
@@ -167,6 +172,7 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(time_to_full_now),
 	POWER_SUPPLY_ATTR(time_to_full_avg),
 	POWER_SUPPLY_ATTR(type),
+	POWER_SUPPLY_ATTR(scope),
 	/* Properties of type `const char *' */
 	POWER_SUPPLY_ATTR(model_name),
 	POWER_SUPPLY_ATTR(manufacturer),
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index a28f2df..2e3c827 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -74,6 +74,12 @@ enum {
 	POWER_SUPPLY_CAPACITY_LEVEL_FULL,
 };
 
+enum {
+	POWER_SUPPLY_SCOPE_UNKNOWN = 0,
+	POWER_SUPPLY_SCOPE_SYSTEM,
+	POWER_SUPPLY_SCOPE_DEVICE,
+};
+
 enum power_supply_property {
 	/* Properties of type `int' */
 	POWER_SUPPLY_PROP_STATUS = 0,
@@ -116,6 +122,7 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
 	POWER_SUPPLY_PROP_TIME_TO_FULL_AVG,
 	POWER_SUPPLY_PROP_TYPE, /* use power_supply.type instead */
+	POWER_SUPPLY_PROP_SCOPE,
 	/* Properties of type `const char *' */
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,


>> And if the device being pointed to is a mouse, then upower knows its
>> powering a mouse.
> Define "mouse" -- an input devices with two axis? I think an explicit
> type might be clearer.

Well, what are you really trying to represent?  Is it enough for upower
to say "here are some devices with batteries, this is everything I know
about them", and let its clients work out what they really are?  Does
upower need to work out what kind of device it is?

Thanks,
    J

Re: [GIT PULL RFC] directly poll battery strength when reading power_supply

[Richard Hughes]

On 7 December 2011 17:25, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> Something like this?
> @@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
>        static char *capacity_level_text[] = {
>                "Unknown", "Critical", "Low", "Normal", "High", "Full"
>        };
> +       static char *scope_text[] = {
> +               "Unknown", "System", "Device"
> +       };

That looks perfect.

> Well, what are you really trying to represent?  Is it enough for upower
> to say "here are some devices with batteries, this is everything I know
> about them", and let its clients work out what they really are?  Does
> upower need to work out what kind of device it is?

Well, upower just assigns a "kind" to each battery which means the
clients can for instance show all the laptop batteries, but ignore any
mice and keyboards. For practical purposes, it's so we suspend the
computer for a low power laptop battery, but not for a low power
keyboard :)

Richard.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL RFC] directly poll battery strength when reading power_supply

[Jeremy Fitzhardinge]

On 12/07/2011 09:29 AM, Richard Hughes wrote:
> On 7 December 2011 17:25, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>> Something like this?
>> @@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
>>        static char *capacity_level_text[] = {
>>                "Unknown", "Critical", "Low", "Normal", "High", "Full"
>>        };
>> +       static char *scope_text[] = {
>> +               "Unknown", "System", "Device"
>> +       };
> That looks perfect.

OK, I'll put it into a submittable form when I get the chance (oh, and
compile and test it).

>
> Well, upower just assigns a "kind" to each battery which means the
> clients can for instance show all the laptop batteries, but ignore any
> mice and keyboards. For practical purposes, it's so we suspend the
> computer for a low power laptop battery, but not for a low power
> keyboard :)

So you could make that determination from the scope?

    J

Re: [GIT PULL RFC] directly poll battery strength when reading power_supply

[Richard Hughes]

On 7 December 2011 17:36, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> So you could make that determination from the scope?

Yes, and then just use look for something like a "key" on the keyboard device.

Richard.

[GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

Hi,

This series adds a "scope" property to power supplies, so that a power
supply can indicate whether it powers the whole system, or just a
device.  This allows upowerd to distinguish between actual system power
supplies, and self-powered devices such as wireless mice.

Thanks,
    J

  Linux 3.2-rc2 (2011-11-15 15:02:59 -0200)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git power-supply-scope

Jeremy Fitzhardinge (3):
      power_supply: add SCOPE property to power supplies
      power_supply: allow powered device to be registered with supply
      power_supply: add scope properties to some common power_supplies

 drivers/acpi/ac.c                  |    4 ++++
 drivers/acpi/battery.c             |    5 +++++
 drivers/acpi/sbs.c                 |    9 +++++++++
 drivers/hid/hid-wacom.c            |   12 ++++++++++--
 drivers/hid/hid-wiimote.c          |    8 +++++++-
 drivers/power/power_supply_core.c  |    7 +++++++
 drivers/power/power_supply_sysfs.c |    6 ++++++
 include/linux/power_supply.h       |    8 ++++++++
 8 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
index 6512b20..ec36c82 100644
--- a/drivers/acpi/ac.c
+++ b/drivers/acpi/ac.c
@@ -142,6 +142,9 @@ static int get_ac_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_ONLINE:
 		val->intval = ac->state;
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -150,6 +153,7 @@ static int get_ac_property(struct power_supply *psy,
 
 static enum power_supply_property ac_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_SCOPE,
 };
 
 #ifdef CONFIG_ACPI_PROCFS_POWER
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 7711d94..1f68bb6 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -250,6 +250,9 @@ static int acpi_battery_get_property(struct power_supply *psy,
 		else
 			val->intval = battery->capacity_now * 1000;
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = battery->model_number;
 		break;
@@ -276,6 +279,7 @@ static enum power_supply_property charge_battery_props[] = {
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_FULL,
 	POWER_SUPPLY_PROP_CHARGE_NOW,
+	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
@@ -292,6 +296,7 @@ static enum power_supply_property energy_battery_props[] = {
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,
 	POWER_SUPPLY_PROP_ENERGY_NOW,
+	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 	POWER_SUPPLY_PROP_SERIAL_NUMBER,
diff --git a/drivers/acpi/sbs.c b/drivers/acpi/sbs.c
index 6e36d0c..fc35e42 100644
--- a/drivers/acpi/sbs.c
+++ b/drivers/acpi/sbs.c
@@ -171,6 +171,9 @@ static int sbs_get_ac_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_ONLINE:
 		val->intval = sbs->charger_present;
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -263,6 +266,9 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_TEMP:
 		val->intval = battery->temp_now - 2730;	// dK -> dC
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
+		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		val->strval = battery->device_name;
 		break;
@@ -277,6 +283,7 @@ static int acpi_sbs_battery_get_property(struct power_supply *psy,
 
 static enum power_supply_property sbs_ac_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_SCOPE,
 };
 
 static enum power_supply_property sbs_charge_battery_props[] = {
@@ -292,6 +299,7 @@ static enum power_supply_property sbs_charge_battery_props[] = {
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_CHARGE_FULL,
 	POWER_SUPPLY_PROP_CHARGE_NOW,
+	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_TEMP,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
@@ -311,6 +319,7 @@ static enum power_supply_property sbs_energy_battery_props[] = {
 	POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN,
 	POWER_SUPPLY_PROP_ENERGY_FULL,
 	POWER_SUPPLY_PROP_ENERGY_NOW,
+	POWER_SUPPLY_PROP_SCOPE,
 	POWER_SUPPLY_PROP_TEMP,
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index 17bb88f..ad39777 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -47,12 +47,14 @@ 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
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_SCOPE,
 };
 
 static enum power_supply_property wacom_ac_props[] = {
 	POWER_SUPPLY_PROP_PRESENT,
-	POWER_SUPPLY_PROP_ONLINE
+	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_SCOPE,
 };
 
 static int wacom_battery_get_property(struct power_supply *psy,
@@ -68,6 +70,9 @@ static int wacom_battery_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_PRESENT:
 		val->intval = 1;
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_DEVICE;
+		break;
 	case POWER_SUPPLY_PROP_CAPACITY:
 		/* show 100% battery capacity when charging */
 		if (power_state == 0)
@@ -99,6 +104,9 @@ static int wacom_ac_get_property(struct power_supply *psy,
 		else
 			val->intval = 0;
 		break;
+	case POWER_SUPPLY_PROP_SCOPE:
+		val->intval = POWER_SUPPLY_SCOPE_DEVICE;
+		break;
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/drivers/hid/hid-wiimote.c b/drivers/hid/hid-wiimote.c
index 76739c0..98e4828 100644
--- a/drivers/hid/hid-wiimote.c
+++ b/drivers/hid/hid-wiimote.c
@@ -136,7 +136,8 @@ static __u16 wiiproto_keymap[] = {
 };
 
 static enum power_supply_property wiimote_battery_props[] = {
-	POWER_SUPPLY_PROP_CAPACITY
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_SCOPE,
 };
 
 /* requires the state.lock spinlock to be held */
@@ -468,6 +469,11 @@ static int wiimote_battery_get_property(struct power_supply *psy,
 	int ret = 0, state;
 	unsigned long flags;
 
+	if (psp == POWER_SUPPLY_PROP_SCOPE) {
+		val->intval = POWER_SUPPLY_SCOPE_DEVICE;
+		return 0;
+	}
+
 	ret = wiimote_cmd_acquire(wdata);
 	if (ret)
 		return ret;
diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index 329b46b..bacf327 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -147,6 +147,12 @@ struct power_supply *power_supply_get_by_name(char *name)
 }
 EXPORT_SYMBOL_GPL(power_supply_get_by_name);
 
+
+int power_supply_powers(struct power_supply *psy, struct device *dev)
+{
+	return sysfs_create_link_nowarn(&psy->dev->kobj, &dev->kobj, "powers");
+}
+
 static void power_supply_dev_release(struct device *dev)
 {
 	pr_debug("device: '%s': %s\n", dev_name(dev), __func__);
@@ -202,6 +208,7 @@ EXPORT_SYMBOL_GPL(power_supply_register);
 void power_supply_unregister(struct power_supply *psy)
 {
 	cancel_work_sync(&psy->changed_work);
+	sysfs_remove_link(&psy->dev->kobj, "powers");
 	power_supply_remove_triggers(psy);
 	device_unregister(psy->dev);
 }
diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
index e15d4c9..21178eb 100644
--- a/drivers/power/power_supply_sysfs.c
+++ b/drivers/power/power_supply_sysfs.c
@@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
 	static char *capacity_level_text[] = {
 		"Unknown", "Critical", "Low", "Normal", "High", "Full"
 	};
+	static char *scope_text[] = {
+		"Unknown", "System", "Device"
+	};
 	ssize_t ret = 0;
 	struct power_supply *psy = dev_get_drvdata(dev);
 	const ptrdiff_t off = attr - power_supply_attrs;
@@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
 		return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
 	else if (off == POWER_SUPPLY_PROP_TYPE)
 		return sprintf(buf, "%s\n", type_text[value.intval]);
+	else if (off == POWER_SUPPLY_PROP_SCOPE)
+		return sprintf(buf, "%s\n", scope_text[value.intval]);
 	else if (off >= POWER_SUPPLY_PROP_MODEL_NAME)
 		return sprintf(buf, "%s\n", value.strval);
 
@@ -167,6 +172,7 @@ static struct device_attribute power_supply_attrs[] = {
 	POWER_SUPPLY_ATTR(time_to_full_now),
 	POWER_SUPPLY_ATTR(time_to_full_avg),
 	POWER_SUPPLY_ATTR(type),
+	POWER_SUPPLY_ATTR(scope),
 	/* Properties of type `const char *' */
 	POWER_SUPPLY_ATTR(model_name),
 	POWER_SUPPLY_ATTR(manufacturer),
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 204c18d..2e3c827 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -74,6 +74,12 @@ enum {
 	POWER_SUPPLY_CAPACITY_LEVEL_FULL,
 };
 
+enum {
+	POWER_SUPPLY_SCOPE_UNKNOWN = 0,
+	POWER_SUPPLY_SCOPE_SYSTEM,
+	POWER_SUPPLY_SCOPE_DEVICE,
+};
+
 enum power_supply_property {
 	/* Properties of type `int' */
 	POWER_SUPPLY_PROP_STATUS = 0,
@@ -116,6 +122,7 @@ enum power_supply_property {
 	POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
 	POWER_SUPPLY_PROP_TIME_TO_FULL_AVG,
 	POWER_SUPPLY_PROP_TYPE, /* use power_supply.type instead */
+	POWER_SUPPLY_PROP_SCOPE,
 	/* Properties of type `const char *' */
 	POWER_SUPPLY_PROP_MODEL_NAME,
 	POWER_SUPPLY_PROP_MANUFACTURER,
@@ -211,6 +218,7 @@ static inline int power_supply_is_system_supplied(void) { return -ENOSYS; }
 extern int power_supply_register(struct device *parent,
 				 struct power_supply *psy);
 extern void power_supply_unregister(struct power_supply *psy);
+extern int power_supply_powers(struct power_supply *psy, struct device *dev);
 
 /* For APM emulation, think legacy userspace. */
 extern struct class *power_supply_class;

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

Hello Jeremy,

On Wed, Dec 07, 2011 at 05:41:37PM -0800, Jeremy Fitzhardinge wrote:
> This series adds a "scope" property to power supplies, so that a power
> supply can indicate whether it powers the whole system, or just a
> device.  This allows upowerd to distinguish between actual system power
> supplies, and self-powered devices such as wireless mice.

I see one problem with this approach. Userland will need patching
to distinguish system and device power supplies. So, even with
the patch applied, old userland will behave incorrectly.

So, instead of the new 'scope' property, how about adding another
power supply type? I.e. DEVICE_BATTERY ? That type would be unknown
to the current userland, and thus should be ignored.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Richard Hughes]

On 8 December 2011 10:02, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> I see one problem with this approach. Userland will need patching
> to distinguish system and device power supplies. So, even with
> the patch applied, old userland will behave incorrectly.

Userland will need patching anyway, as old versions of upower assumed
anything showing up in /sys/class/power_supply was powering the
system.

Richard.

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

On Thu, Dec 08, 2011 at 10:05:46AM +0000, Richard Hughes wrote:
> On 8 December 2011 10:02, Anton Vorontsov <cbouatmailru@gmail.com> wrote:
> > I see one problem with this approach. Userland will need patching
> > to distinguish system and device power supplies. So, even with
> > the patch applied, old userland will behave incorrectly.
> 
> Userland will need patching anyway, as old versions of upower assumed
> anything showing up in /sys/class/power_supply was powering the
> system.

Oh, makes sense then. But see my other email regarding a bit more
universal scheme.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

On Wed, Dec 07, 2011 at 05:41:37PM -0800, Jeremy Fitzhardinge wrote:
[...]
> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
> index 6512b20..ec36c82 100644
> --- a/drivers/acpi/ac.c
> +++ b/drivers/acpi/ac.c
> @@ -142,6 +142,9 @@ static int get_ac_property(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_ONLINE:
>  		val->intval = ac->state;
>  		break;
> +	case POWER_SUPPLY_PROP_SCOPE:
> +		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}

Mm... how about the rest of the drivers? I.e. drivers/power/*battery.c?

I think it's not a great idea to patch every driver, would be better to make
it similar to how we handle power_supply.type (w/ default value 'SYSTEM').


But, thinking more about it... personally, from the ABI point of view, I'd
like to just see some kind of 'supplicants' directory in the power_supply
instances, with symlinks to an appropriate devices.

I.e.
/sys/class/power_supply/battery/supplicants/<device_name>
is a symlink to /sys/class/HID/.../device.

With a special meaning of an empty directory (or non-existent, or w/ a
symlink pointing to '/sys/devices/system') -- system power.

That way we may describe any possible power hierarchy.

>From the implementation point of view, for now power_supply may just
conditionally (by introducing power_supply.not_system_power flag)
expose power_supply.dev->parent to /sys/.../supplicants.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

On 12/08/2011 02:41 AM, Anton Vorontsov wrote:
> On Wed, Dec 07, 2011 at 05:41:37PM -0800, Jeremy Fitzhardinge wrote:
> [...]
>> diff --git a/drivers/acpi/ac.c b/drivers/acpi/ac.c
>> index 6512b20..ec36c82 100644
>> --- a/drivers/acpi/ac.c
>> +++ b/drivers/acpi/ac.c
>> @@ -142,6 +142,9 @@ static int get_ac_property(struct power_supply *psy,
>>  	case POWER_SUPPLY_PROP_ONLINE:
>>  		val->intval = ac->state;
>>  		break;
>> +	case POWER_SUPPLY_PROP_SCOPE:
>> +		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
>> +		break;
>>  	default:
>>  		return -EINVAL;
>>  	}
> Mm... how about the rest of the drivers? I.e. drivers/power/*battery.c?
>
> I think it's not a great idea to patch every driver, would be better to make
> it similar to how we handle power_supply.type (w/ default value 'SYSTEM').

Yes.  That patch was mostly so I could test the mechanism.  Certainly
general rule is that if there's no scope attribute then assume System.

> But, thinking more about it... personally, from the ABI point of view, I'd
> like to just see some kind of 'supplicants' directory in the power_supply
> instances, with symlinks to an appropriate devices.

Yes, that was my first proposal, and I have a patch to allow Device
scope power supplies to indicate which "device" it is (which may be a
root of a subtree of devices).  I'm not too sure about making it
multiple devices; at that point I'd be tempted to introduce the notion
of a "power bus" which points to multiple devices and make power
supplies point to that.

> I.e.
> /sys/class/power_supply/battery/supplicants/<device_name>
> is a symlink to /sys/class/HID/.../device.
>
> With a special meaning of an empty directory (or non-existent, or w/ a
> symlink pointing to '/sys/devices/system') -- system power.

Yes.  That's awkward to implement because the kobj isn't exported from
device/base.  But aside from that, its a somewhat awkward interface for
usermode, because it has to end up following symlink and resolving their
paths, and then having special hardcoded knowledge of what particular
paths mean.  When all upower really wants to know is "do I need to
suspend when this supply gets low?".

> That way we may describe any possible power hierarchy.
>
> From the implementation point of view, for now power_supply may just
> conditionally (by introducing power_supply.not_system_power flag)

How is that different from scope?

    J

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

On Thu, Dec 08, 2011 at 08:53:15AM -0800, Jeremy Fitzhardinge wrote:
> Yes.  That patch was mostly so I could test the mechanism.  Certainly
> general rule is that if there's no scope attribute then assume System.

Okay, great.

> > /sys/class/power_supply/battery/supplicants/<device_name>
> > is a symlink to /sys/class/HID/.../device.
> >
> > With a special meaning of an empty directory (or non-existent, or w/ a
> > symlink pointing to '/sys/devices/system') -- system power.
> 
> Yes.  That's awkward to implement because the kobj isn't exported from
> device/base.  But aside from that, its a somewhat awkward interface for
> usermode, because it has to end up following symlink and resolving their
> paths, and then having special hardcoded knowledge of what particular
> paths mean.  When all upower really wants to know is "do I need to
> suspend when this supply gets low?".

Mm... OK. I think you're right. The 'scope' thing is indeed useful by
itself.

> > That way we may describe any possible power hierarchy.
> >
> > From the implementation point of view, for now power_supply may just
> > conditionally (by introducing power_supply.not_system_power flag)
> 
> How is that different from scope?

No different at all, I'm fine with either power_supply.scope or any
other flag. :-)

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

On 12/08/2011 03:36 PM, Anton Vorontsov wrote:
>
> No different at all, I'm fine with either power_supply.scope or any
> other flag. :-)

So are you happy to pull that branch, or did you have some other
concerns with it?

Thanks,
    J

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

On Fri, Dec 09, 2011 at 12:18:47AM -0800, Jeremy Fitzhardinge wrote:
> On 12/08/2011 03:36 PM, Anton Vorontsov wrote:
> >
> > No different at all, I'm fine with either power_supply.scope or any
> > other flag. :-)
> 
> So are you happy to pull that branch, or did you have some other
> concerns with it?

I'm OK w/ the idea itself, but to actually pull that branch, the issue
w/ needless patching of battery drivers has to be fixed (i.e. drop the
changes in the drivers/acpi/).

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

On 12/09/2011 01:59 AM, Anton Vorontsov wrote:
> On Fri, Dec 09, 2011 at 12:18:47AM -0800, Jeremy Fitzhardinge wrote:
>> On 12/08/2011 03:36 PM, Anton Vorontsov wrote:
>>> No different at all, I'm fine with either power_supply.scope or any
>>> other flag. :-)
>> So are you happy to pull that branch, or did you have some other
>> concerns with it?
> I'm OK w/ the idea itself, but to actually pull that branch, the issue
> w/ needless patching of battery drivers has to be fixed (i.e. drop the
> changes in the drivers/acpi/).

OK, and just use the default System?  So only patch devices which are
actually self-powered?

    J

Re: [GIT PULL] power_supply: add power supply scope

[Anton Vorontsov]

A few more minor nits...

On Wed, Dec 07, 2011 at 05:41:37PM -0800, Jeremy Fitzhardinge wrote:
[...]
> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
> index 329b46b..bacf327 100644
> --- a/drivers/power/power_supply_core.c
> +++ b/drivers/power/power_supply_core.c
> @@ -147,6 +147,12 @@ struct power_supply *power_supply_get_by_name(char *name)
>  }
>  EXPORT_SYMBOL_GPL(power_supply_get_by_name);
>  
> +

Needless newline. :-)

> +int power_supply_powers(struct power_supply *psy, struct device *dev)
> +{
> +	return sysfs_create_link_nowarn(&psy->dev->kobj, &dev->kobj, "powers");
> +}

- This surely creates an important ABI to the userland. This has to be
  documented via Documentation/ABI (or at least via commit message that
  I don't see in this email :-).

- This functionality isn't used anywhere as of now (i.e. I don't see
  anyone calling this function), so let's omit it for now?

- I still don't think that this should be a single symlink, to me the
  more universal (so that we don't have to break ABI in the future)
  way would be 'powers' directory with symlinks in it.
  But maybe I'm not following where exactly the link will be created?
  Documentation or an example of the new sysfs structure would help.

> +
>  static void power_supply_dev_release(struct device *dev)
>  {
>  	pr_debug("device: '%s': %s\n", dev_name(dev), __func__);
> @@ -202,6 +208,7 @@ EXPORT_SYMBOL_GPL(power_supply_register);
>  void power_supply_unregister(struct power_supply *psy)
>  {
>  	cancel_work_sync(&psy->changed_work);
> +	sysfs_remove_link(&psy->dev->kobj, "powers");
>  	power_supply_remove_triggers(psy);
>  	device_unregister(psy->dev);
>  }
> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
> index e15d4c9..21178eb 100644
> --- a/drivers/power/power_supply_sysfs.c
> +++ b/drivers/power/power_supply_sysfs.c
> @@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
>  	static char *capacity_level_text[] = {
>  		"Unknown", "Critical", "Low", "Normal", "High", "Full"
>  	};
> +	static char *scope_text[] = {
> +		"Unknown", "System", "Device"
> +	};
>  	ssize_t ret = 0;
>  	struct power_supply *psy = dev_get_drvdata(dev);
>  	const ptrdiff_t off = attr - power_supply_attrs;
> @@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>  		return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
>  	else if (off == POWER_SUPPLY_PROP_TYPE)
>  		return sprintf(buf, "%s\n", type_text[value.intval]);
> +	else if (off == POWER_SUPPLY_PROP_SCOPE)
> +		return sprintf(buf, "%s\n", scope_text[value.intval]);

Should we really handle the PROP_SCOPE as a dynamic property?
Maybe do it similar to PROP_TYPE, so that drivers will only need to
specity the scope during registration, and not bother w/ handling
it in theirs get_property() callbacks?

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

On 12/09/2011 02:17 AM, Anton Vorontsov wrote:
> A few more minor nits...
>
> On Wed, Dec 07, 2011 at 05:41:37PM -0800, Jeremy Fitzhardinge wrote:
> [...]
>> diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
>> index 329b46b..bacf327 100644
>> --- a/drivers/power/power_supply_core.c
>> +++ b/drivers/power/power_supply_core.c
>> @@ -147,6 +147,12 @@ struct power_supply *power_supply_get_by_name(char *name)
>>  }
>>  EXPORT_SYMBOL_GPL(power_supply_get_by_name);
>>  
>> +
> Needless newline. :-)

OK.

>> +int power_supply_powers(struct power_supply *psy, struct device *dev)
>> +{
>> +	return sysfs_create_link_nowarn(&psy->dev->kobj, &dev->kobj, "powers");
>> +}
> - This surely creates an important ABI to the userland. This has to be
>   documented via Documentation/ABI (or at least via commit message that
>   I don't see in this email :-).

Sure, I'll add that.  It doesn't look like Documentation/ABI has
anything about the whole power_supply class, so I'll consider adding
that as out of scope for this work, but I'll fill out the commit comment.

> - This functionality isn't used anywhere as of now (i.e. I don't see
>   anyone calling this function), so let's omit it for now?

I have a patch in my hid-input battery series which use it.  Actually, I
can use it in this series for the Wacom and Wiimote HID power supplies.

> - I still don't think that this should be a single symlink, to me the
>   more universal (so that we don't have to break ABI in the future)
>   way would be 'powers' directory with symlinks in it.
>   But maybe I'm not following where exactly the link will be created?
>   Documentation or an example of the new sysfs structure would help.

At the moment, it's a pointer to a single device, which may have
sub-devices under it.  This matches the general device tree power
management model which assumes that the bus topology of the system is
also a reasonable approximation for the power distribution topology (if
nothing else, because if you power off a bridge device, you can't see
anything behind it so it may as well be considered powered off as well).

I understand your point about powering multiple devices, but it seems
like overengineering to implement it now unless you have some specific
users of that interface in mind.  If you did want to support a single
power supply powering multiple devices in future, you could add the
notion of a "power bus" device which distributes power to multiple
devices, without having to complicate the common case of powering a
single device, and without having to change the current semantics of the
"powers" link.

>> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
>> index e15d4c9..21178eb 100644
>> --- a/drivers/power/power_supply_sysfs.c
>> +++ b/drivers/power/power_supply_sysfs.c
>> @@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
>>  	static char *capacity_level_text[] = {
>>  		"Unknown", "Critical", "Low", "Normal", "High", "Full"
>>  	};
>> +	static char *scope_text[] = {
>> +		"Unknown", "System", "Device"
>> +	};
>>  	ssize_t ret = 0;
>>  	struct power_supply *psy = dev_get_drvdata(dev);
>>  	const ptrdiff_t off = attr - power_supply_attrs;
>> @@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>>  		return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
>>  	else if (off == POWER_SUPPLY_PROP_TYPE)
>>  		return sprintf(buf, "%s\n", type_text[value.intval]);
>> +	else if (off == POWER_SUPPLY_PROP_SCOPE)
>> +		return sprintf(buf, "%s\n", scope_text[value.intval]);
> Should we really handle the PROP_SCOPE as a dynamic property?
> Maybe do it similar to PROP_TYPE, so that drivers will only need to
> specity the scope during registration, and not bother w/ handling
> it in theirs get_property() callbacks?

I don't really know.  I guess its possible in theory that a device could
change scope on the fly if its power was re-routed.  But then, the type
can change too (if, for example, a UPS switched between AC and battery
power, or a HID device switching between corded USB power or cordless
battery power), so I'm not really sure what the rationale is either
way.  (I guess you model power supplies switching type as having
multiple power supplies which turn themselves on and offline?)

    J

Re: [GIT PULL] power_supply: add power supply scope

[Daniel Nicoletti]

>>> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
>>> index e15d4c9..21178eb 100644
>>> --- a/drivers/power/power_supply_sysfs.c
>>> +++ b/drivers/power/power_supply_sysfs.c
>>> @@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
>>>      static char *capacity_level_text[] = {
>>>              "Unknown", "Critical", "Low", "Normal", "High", "Full"
>>>      };
>>> +    static char *scope_text[] = {
>>> +            "Unknown", "System", "Device"
>>> +    };
>>>      ssize_t ret = 0;
>>>      struct power_supply *psy = dev_get_drvdata(dev);
>>>      const ptrdiff_t off = attr - power_supply_attrs;
>>> @@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>>>              return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
>>>      else if (off == POWER_SUPPLY_PROP_TYPE)
>>>              return sprintf(buf, "%s\n", type_text[value.intval]);
>>> +    else if (off == POWER_SUPPLY_PROP_SCOPE)
>>> +            return sprintf(buf, "%s\n", scope_text[value.intval]);
>> Should we really handle the PROP_SCOPE as a dynamic property?
>> Maybe do it similar to PROP_TYPE, so that drivers will only need to
>> specity the scope during registration, and not bother w/ handling
>> it in theirs get_property() callbacks?
>
> I don't really know.  I guess its possible in theory that a device could
> change scope on the fly if its power was re-routed.  But then, the type
> can change too (if, for example, a UPS switched between AC and battery
> power, or a HID device switching between corded USB power or cordless
> battery power), so I'm not really sure what the rationale is either
> way.  (I guess you model power supplies switching type as having
> multiple power supplies which turn themselves on and offline?)

But isn't the scope about powering or not the system? If so even if
the device is now using AC it will not be powering the computer.

Re: [GIT PULL] power_supply: add power supply scope

[Jeremy Fitzhardinge]

On 12/09/2011 12:00 PM, Daniel Nicoletti wrote:
>>>> diff --git a/drivers/power/power_supply_sysfs.c b/drivers/power/power_supply_sysfs.c
>>>> index e15d4c9..21178eb 100644
>>>> --- a/drivers/power/power_supply_sysfs.c
>>>> +++ b/drivers/power/power_supply_sysfs.c
>>>> @@ -63,6 +63,9 @@ static ssize_t power_supply_show_property(struct device *dev,
>>>>      static char *capacity_level_text[] = {
>>>>              "Unknown", "Critical", "Low", "Normal", "High", "Full"
>>>>      };
>>>> +    static char *scope_text[] = {
>>>> +            "Unknown", "System", "Device"
>>>> +    };
>>>>      ssize_t ret = 0;
>>>>      struct power_supply *psy = dev_get_drvdata(dev);
>>>>      const ptrdiff_t off = attr - power_supply_attrs;
>>>> @@ -95,6 +98,8 @@ static ssize_t power_supply_show_property(struct device *dev,
>>>>              return sprintf(buf, "%s\n", capacity_level_text[value.intval]);
>>>>      else if (off == POWER_SUPPLY_PROP_TYPE)
>>>>              return sprintf(buf, "%s\n", type_text[value.intval]);
>>>> +    else if (off == POWER_SUPPLY_PROP_SCOPE)
>>>> +            return sprintf(buf, "%s\n", scope_text[value.intval]);
>>> Should we really handle the PROP_SCOPE as a dynamic property?
>>> Maybe do it similar to PROP_TYPE, so that drivers will only need to
>>> specity the scope during registration, and not bother w/ handling
>>> it in theirs get_property() callbacks?
>> I don't really know.  I guess its possible in theory that a device could
>> change scope on the fly if its power was re-routed.  But then, the type
>> can change too (if, for example, a UPS switched between AC and battery
>> power, or a HID device switching between corded USB power or cordless
>> battery power), so I'm not really sure what the rationale is either
>> way.  (I guess you model power supplies switching type as having
>> multiple power supplies which turn themselves on and offline?)
> But isn't the scope about powering or not the system? If so even if
> the device is now using AC it will not be powering the computer.
>

Sure.  I was just commenting on the similarity between scope and type
with respect to whether they're immutable properties of a power supply
or things that can change over time.

    J

Re: [PATCH RFC] hid-input: add support for HID devices reporting Battery Strength

[Jeremy Fitzhardinge]

On 12/01/2011 09:52 PM, Daniel Nicoletti wrote:

and the patch (note that this is my first kernel patch so sorry if
formatting is not ok, should I attach it?):


It did get word-wrapped.

    J