Re: [PATCH 4/4] HID: wiiu-drc: Add battery reporting

[Barnabás Pőcze]

Hi


2021. május 3., hétfő 1:28 keltezéssel, Emmanuel Gil Peyrot írta:

> On my DRC the values only go between 142 (battery LED blinking red
> before shutdown) and 178 (charge LED stopping), it seems to be the same
> on other units according to other testers.
>
> A spinlock is used to avoid the battery level and status from being
> reported unsynchronised.
>
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@xxxxxxxxxxxx>
> ---
>  drivers/hid/hid-wiiu-drc.c | 107 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
>
> diff --git a/drivers/hid/hid-wiiu-drc.c b/drivers/hid/hid-wiiu-drc.c
> index 80faaaad2bb5..119d55542e31 100644
> --- a/drivers/hid/hid-wiiu-drc.c
> +++ b/drivers/hid/hid-wiiu-drc.c
> @@ -15,6 +15,8 @@
>  #include <linux/device.h>
>  #include <linux/hid.h>
>  #include <linux/module.h>
> +#include <linux/power_supply.h>
> +#include <linux/spinlock.h>
>  #include "hid-ids.h"
>
>  #define DEVICE_NAME	"Nintendo Wii U gamepad"
> @@ -69,6 +71,11 @@
>  #define MAGNET_MIN	ACCEL_MIN
>  #define MAGNET_MAX	ACCEL_MAX
>
> +/* Battery constants */
> +#define BATTERY_MIN	142
> +#define BATTERY_MAX	178
> +#define BATTERY_CAPACITY(val) ((val - BATTERY_MIN) * 100 / (BATTERY_MAX - BATTERY_MIN))

There's `fixp_linear_interpolate()` in linux/fixp-arithmetic.h,
you could use that.


> +
>  /*
>   * The device is setup with multiple input devices:
>   * - A joypad with the buttons and sticks.
> @@ -77,10 +84,17 @@
>   */
>
>  struct drc {
> +	spinlock_t lock;

I believe a `spin_lock_init()` call is missing from the code.


> +
>  	struct input_dev *joy_input_dev;
>  	struct input_dev *touch_input_dev;
>  	struct input_dev *accel_input_dev;
>  	struct hid_device *hdev;
> +	struct power_supply *battery;
> +	struct power_supply_desc battery_desc;
> +
> +	u8 battery_energy;
> +	int battery_status;
>  };
>
>  static int drc_raw_event(struct hid_device *hdev, struct hid_report *report,
> @@ -89,6 +103,7 @@ static int drc_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	struct drc *drc = hid_get_drvdata(hdev);
>  	int i, x, y, z, pressure, base;
>  	u32 buttons;
> +	unsigned long flags;
>
>  	if (len != 128)
>  		return 0;
> @@ -206,6 +221,17 @@ static int drc_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	input_report_abs(drc->accel_input_dev, ABS_WHEEL, (int16_t)z);
>  	input_sync(drc->accel_input_dev);
>
> +	/* battery */
> +	spin_lock_irqsave(&drc->lock, flags);
> +	drc->battery_energy = data[5];
> +	if (drc->battery_energy == BATTERY_MAX)
> +		drc->battery_status = POWER_SUPPLY_STATUS_FULL;
> +	else if ((data[4] & 0x40) != 0)

Maybe `if (data[4] & BIT(BATTERY_CHARGING))` or `if (data[4] & BATTERY_CHARGING_BIT)`
would be better.


> +		drc->battery_status = POWER_SUPPLY_STATUS_CHARGING;
> +	else
> +		drc->battery_status = POWER_SUPPLY_STATUS_DISCHARGING;
> +	spin_unlock_irqrestore(&drc->lock, flags);
> +
>  	/* let hidraw and hiddev handle the report */
>  	return 0;
>  }
> @@ -309,10 +335,67 @@ static bool drc_setup_accel(struct drc *drc,
>  	return true;
>  }
>
> +static enum power_supply_property drc_battery_props[] = {
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_CAPACITY,
> +	POWER_SUPPLY_PROP_SCOPE,
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_ENERGY_NOW,
> +	POWER_SUPPLY_PROP_ENERGY_EMPTY,
> +	POWER_SUPPLY_PROP_ENERGY_FULL,
> +};
> +
> +static int drc_battery_get_property(struct power_supply *psy,
> +				    enum power_supply_property psp,
> +				    union power_supply_propval *val)
> +{
> +	struct drc *drc = power_supply_get_drvdata(psy);
> +	unsigned long flags;
> +	int ret = 0;
> +	u8 battery_energy;
> +	int battery_status;
> +
> +	spin_lock_irqsave(&drc->lock, flags);
> +	battery_energy = drc->battery_energy;
> +	battery_status = drc->battery_status;
> +	spin_unlock_irqrestore(&drc->lock, flags);
> +
> +	switch (psp) {
> +	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:
> +		val->intval = BATTERY_CAPACITY(battery_energy);
> +		break;
> +	case POWER_SUPPLY_PROP_STATUS:
> +		val->intval = battery_status;
> +		break;
> +	case POWER_SUPPLY_PROP_ENERGY_NOW:
> +		val->intval = battery_energy;
> +		break;
> +	case POWER_SUPPLY_PROP_ENERGY_EMPTY:
> +		val->intval = BATTERY_MIN;
> +		break;
> +	case POWER_SUPPLY_PROP_ENERGY_FULL:
> +		val->intval = BATTERY_MAX;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +	return ret;
> +}
> +
>  static bool drc_setup_joypad(struct drc *drc,
>  		struct hid_device *hdev)
>  {
>  	struct input_dev *input_dev;
> +	struct power_supply_config psy_cfg = { .drv_data = drc, };
> +	int ret;
> +	static uint8_t drc_num = 0;

You probably need an atomic integer here and use `atomic_fetch_inc()`
in the `devm_kasprintf()` call.


>
>  	input_dev = allocate_and_setup(hdev, DEVICE_NAME " Joypad");
>  	if (!input_dev)
> @@ -350,6 +433,30 @@ static bool drc_setup_joypad(struct drc *drc,
>
>  	drc->joy_input_dev = input_dev;
>
> +	drc->battery_desc.properties = drc_battery_props;
> +	drc->battery_desc.num_properties = ARRAY_SIZE(drc_battery_props);
> +	drc->battery_desc.get_property = drc_battery_get_property;
> +	drc->battery_desc.type = POWER_SUPPLY_TYPE_BATTERY;
> +	drc->battery_desc.use_for_apm = 0;
> +
> +	/*
> +	 * TODO: It might be better to use the interface number as the drc_num,
> +	 * but I don’t know how to fetch it from here.  In userland it is
> +	 * /sys/devices/platform/latte/d140000.usb/usb3/3-1/3-1:1.?/bInterfaceNumber
> +	 */

The interface number is not globally unique in any way as far as I can tell,
maybe the combination of the bus, port, device, interface numbers is.


> +	drc->battery_desc.name = devm_kasprintf(&hdev->dev, GFP_KERNEL, "wiiu-drc-%i-battery", drc_num++);
> +	if (!drc->battery_desc.name)
> +		return -ENOMEM;

The function returns `bool`. You might want to change it to `int` and return
a proper errno.


> +
> +	drc->battery = devm_power_supply_register(&hdev->dev, &drc->battery_desc, &psy_cfg);
> +	if (IS_ERR(drc->battery)) {
> +		ret = PTR_ERR(drc->battery);
> +		hid_err(hdev, "Unable to register battery device\n");
> +		return ret;
> +	}
> +
> +	power_supply_powers(drc->battery, &hdev->dev);
> +
>  	return true;
>  }
>
> --
> 2.31.1


Regards,
Barnabás Pőcze