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

[Jeremy Fitzhardinge <jeremy@goop.org>]

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