Hi Daniel Some comments below. "Daniel J. Ogorchock" <djogorchock@xxxxxxxxx> wrote: > This patch adds power_supply functionality to the switch controller > driver for its battery. > > Signed-off-by: Daniel J. Ogorchock <djogorchock@xxxxxxxxx> > --- > drivers/hid/Kconfig | 1 + > drivers/hid/hid-nintendo.c | 134 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 135 insertions(+) > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index af4d543c0ff9..c05bfb6ac577 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -715,6 +715,7 @@ config HID_NINTENDO > depends on HID > depends on NEW_LEDS > depends on LEDS_CLASS > + select POWER_SUPPLY > help > Adds support for the Nintendo Switch Joy-Cons and Pro Controller. > All controllers support bluetooth, and the Pro Controller also supports > diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c > index c3eec9b7c99c..adecd6790fe9 100644 > --- a/drivers/hid/hid-nintendo.c > +++ b/drivers/hid/hid-nintendo.c > @@ -11,6 +11,7 @@ > * https://github.com/MTCKC/ProconXInput > * hid-wiimote kernel hid driver > * hid-logitech-hidpp driver > + * hid-sony driver > * > * This driver supports the Nintendo Switch Joy-Cons and Pro Controllers. The > * Pro Controllers can either be used over USB or Bluetooth. > @@ -27,6 +28,7 @@ > #include <linux/input.h> > #include <linux/leds.h> > #include <linux/module.h> > +#include <linux/power_supply.h> > #include <linux/spinlock.h> > > /* > @@ -192,6 +194,7 @@ struct joycon_ctlr { > struct input_dev *input; > struct led_classdev leds[JC_NUM_LEDS]; > enum joycon_ctlr_state ctlr_state; > + spinlock_t lock; > > /* The following members are used for synchronous sends/receives */ > enum joycon_msg_type msg_type; > @@ -209,6 +212,12 @@ struct joycon_ctlr { > struct joycon_stick_cal right_stick_cal_x; > struct joycon_stick_cal right_stick_cal_y; > > + /* power supply data */ > + struct power_supply *battery; > + struct power_supply_desc battery_desc; > + u8 battery_capacity; > + bool battery_charging; > + bool host_powered; > }; > > static int __joycon_hid_send(struct hid_device *hdev, u8 *data, size_t len) > @@ -439,9 +448,41 @@ static void joycon_parse_report(struct joycon_ctlr *ctlr, > struct joycon_input_report *rep) > { > struct input_dev *dev = ctlr->input; > + unsigned long flags; > + u8 tmp; > u32 btns; > u32 id = ctlr->hdev->product; > > + /* Parse the battery status */ > + tmp = rep->bat_con; > + spin_lock_irqsave(&ctlr->lock, flags); > + ctlr->host_powered = tmp & BIT(0); > + ctlr->battery_charging = tmp & BIT(4); > + tmp = tmp >> 5; > + switch (tmp) { > + case 0: /* empty */ > + ctlr->battery_capacity = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL; > + break; > + case 1: /* low */ > + ctlr->battery_capacity = POWER_SUPPLY_CAPACITY_LEVEL_LOW; > + break; > + case 2: /* medium */ > + ctlr->battery_capacity = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL; > + break; > + case 3: /* high */ > + ctlr->battery_capacity = POWER_SUPPLY_CAPACITY_LEVEL_HIGH; > + break; > + case 4: /* full */ > + ctlr->battery_capacity = POWER_SUPPLY_CAPACITY_LEVEL_FULL; > + break; > + default: > + ctlr->battery_capacity = POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN; > + hid_warn(ctlr->hdev, "Invalid battery status\n"); > + break; This break is not needed but we seem to use a "break" in a lot of "default:" cases, so it could be on purpose. > + } > + spin_unlock_irqrestore(&ctlr->lock, flags); > + > + /* Parse the buttons and sticks */ > btns = hid_field_extract(ctlr->hdev, rep->button_status, 0, 24); > > if (id != USB_DEVICE_ID_NINTENDO_JOYCONR) { > @@ -741,6 +782,91 @@ static int joycon_player_leds_create(struct joycon_ctlr *ctlr) > return 0; > } > > +static int joycon_battery_get_property(struct power_supply *supply, > + enum power_supply_property prop, > + union power_supply_propval *val) > +{ > + struct joycon_ctlr *ctlr = power_supply_get_drvdata(supply); > + unsigned long flags; > + int ret = 0; > + u8 capacity; > + bool charging; > + bool powered; > + > + spin_lock_irqsave(&ctlr->lock, flags); > + capacity = ctlr->battery_capacity; > + charging = ctlr->battery_charging; > + powered = ctlr->host_powered; > + spin_unlock_irqrestore(&ctlr->lock, flags); > + > + switch (prop) { > + 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_LEVEL: > + val->intval = capacity; > + break; > + case POWER_SUPPLY_PROP_STATUS: > + if (charging) > + val->intval = POWER_SUPPLY_STATUS_CHARGING; > + else if (capacity == POWER_SUPPLY_CAPACITY_LEVEL_FULL && > + powered) > + val->intval = POWER_SUPPLY_STATUS_FULL; > + else > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + break; > + default: > + ret = -EINVAL; > + break; This break is not needed. > + } > + return ret; > +} > + > +static enum power_supply_property joycon_battery_props[] = { > + POWER_SUPPLY_PROP_PRESENT, > + POWER_SUPPLY_PROP_CAPACITY_LEVEL, > + POWER_SUPPLY_PROP_SCOPE, > + POWER_SUPPLY_PROP_STATUS, > +}; > + > +static int joycon_power_supply_create(struct joycon_ctlr *ctlr) > +{ > + struct hid_device *hdev = ctlr->hdev; > + struct power_supply_config supply_config = { .drv_data = ctlr, }; > + const char * const name_fmt = "nintendo_switch_controller_battery_%s"; > + int ret = 0; > + > + /* Set initially to unknown before receiving first input report */ > + ctlr->battery_capacity = POWER_SUPPLY_CAPACITY_LEVEL_UNKNOWN; > + > + /* Configure the battery's description */ > + ctlr->battery_desc.properties = joycon_battery_props; > + ctlr->battery_desc.num_properties = > + ARRAY_SIZE(joycon_battery_props); > + ctlr->battery_desc.get_property = joycon_battery_get_property; > + ctlr->battery_desc.type = POWER_SUPPLY_TYPE_BATTERY; > + ctlr->battery_desc.use_for_apm = 0; > + ctlr->battery_desc.name = devm_kasprintf(&hdev->dev, GFP_KERNEL, > + name_fmt, > + dev_name(&hdev->dev)); > + if (!ctlr->battery_desc.name) > + return -ENOMEM; > + > + ctlr->battery = devm_power_supply_register(&hdev->dev, > + &ctlr->battery_desc, > + &supply_config); > + if (IS_ERR(ctlr->battery)) { > + ret = PTR_ERR(ctlr->battery); > + hid_err(hdev, "Failed to register battery; ret=%d\n", ret); > + return ret; > + } > + power_supply_powers(ctlr->battery, &hdev->dev); We should probably check the return value of this function. Cheers, Silvan
