| Message ID | 20190831175659.7404-1-pedro@pedrovanzella.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [v5] hid-logitech-hidpp: read battery voltage from newer devices | expand |
On Sat, 2019-08-31 at 13:56 -0400, Pedro Vanzella wrote: > Newer Logitech mice report their battery voltage through feature > 0x1001 > instead of the battery levels through feature 0x1000. > > When the device is brought up and we try to query the battery, figure > out if it supports the old or the new feature. If it supports the new > feature, record the feature index and read the battery voltage and > its status. > > If everything went correctly, record the fact that we're capable > of querying battery voltage. > > Note that the protocol only gives us the current voltage in > millivolts. > > Like we do for other ways of interacting with the battery for other > devices, refresh the battery status and notify the power supply > subsystem of the changes in voltage and status. > > Since there are no known devices which implement both the old and the > new battery feature, we make sure to try the newer feature first and > only fall back to the old one if that fails. > > Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com> > --- > drivers/hid/hid-logitech-hidpp.c | 141 > ++++++++++++++++++++++++++++++- > 1 file changed, 137 insertions(+), 4 deletions(-) > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid- > logitech-hidpp.c > index 0179f7ed77e5..2f215e5be001 100644 > --- a/drivers/hid/hid-logitech-hidpp.c > +++ b/drivers/hid/hid-logitech-hidpp.c > @@ -87,6 +87,7 @@ MODULE_PARM_DESC(disable_tap_to_click, > #define HIDPP_CAPABILITY_HIDPP20_BATTERY BIT(1) > #define HIDPP_CAPABILITY_BATTERY_MILEAGE BIT(2) > #define HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS BIT(3) > +#define HIDPP_CAPABILITY_BATTERY_VOLTAGE BIT(4) > > /* > * There are two hidpp protocols in use, the first version hidpp10 > is known > @@ -135,12 +136,14 @@ struct hidpp_report { > struct hidpp_battery { > u8 feature_index; > u8 solar_feature_index; > + u8 voltage_feature_index; > struct power_supply_desc desc; > struct power_supply *ps; > char name[64]; > int status; > int capacity; > int level; > + int voltage; /* in millivolts */ > bool online; > }; > > @@ -1219,6 +1222,118 @@ static int hidpp20_battery_event(struct > hidpp_device *hidpp, > return 0; > } > > +/* --------------------------------------------------------------- > ----------- */ > +/* 0x1001: Battery > voltage */ > +/* --------------------------------------------------------------- > ----------- */ > + > +#define HIDPP_PAGE_BATTERY_VOLTAGE 0x1001 > + > +#define CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE 0x00 > + > +#define EVENT_BATTERY_VOLTAGE_STATUS_BROADCAST 0x00 > + > +static int hidpp20_battery_map_status_voltage(u8 data[3], int > *voltage) > +{ > + int status; > + > + switch (data[2]) { > + case 0x00: /* discharging */ > + status = POWER_SUPPLY_STATUS_DISCHARGING; > + break; > + case 0x10: /* wireless charging */ > + case 0x80: /* charging */ > + status = POWER_SUPPLY_STATUS_CHARGING; > + break; > + case 0x81: /* fully charged */ > + status = POWER_SUPPLY_STATUS_FULL; > + break; > + default: > + status = POWER_SUPPLY_STATUS_NOT_CHARGING; > + } > + > + *voltage = get_unaligned_be16(data); > + > + return status; > +} > + > +static int hidpp20_battery_get_battery_voltage(struct hidpp_device > *hidpp, > + u8 feature_index, > + int *status, int > *voltage) > +{ > + struct hidpp_report response; > + int ret; > + u8 *params = (u8 *)response.fap.params; > + > + ret = hidpp_send_fap_command_sync(hidpp, feature_index, > + CMD_BATTERY_VOLTAGE_GET_BATTE > RY_VOLTAGE, > + NULL, 0, &response); > + > + if (ret > 0) { > + hid_err(hidpp->hid_dev, "%s: received protocol error > 0x%02x\n", > + __func__, ret); > + return -EPROTO; > + } > + if (ret) > + return ret; > + > + hidpp->capabilities |= HIDPP_CAPABILITY_BATTERY_VOLTAGE; > + > + *status = hidpp20_battery_map_status_voltage(params, voltage); > + > + return 0; > +} > + > +static int hidpp20_query_battery_voltage_info(struct hidpp_device > *hidpp) > +{ > + u8 feature_type; > + int ret; > + int status, voltage; > + > + if (hidpp->battery.voltage_feature_index == 0xff) { > + ret = hidpp_root_get_feature(hidpp, > HIDPP_PAGE_BATTERY_VOLTAGE, > + &hidpp- > >battery.voltage_feature_index, > + &feature_type); > + if (ret) > + return ret; > + } > + > + ret = hidpp20_battery_get_battery_voltage(hidpp, > + hidpp- > >battery.voltage_feature_index, > + &status, &voltage); > + > + if (ret) > + return ret; > + > + hidpp->battery.status = status; > + hidpp->battery.voltage = voltage; > + hidpp->battery.online = status != > POWER_SUPPLY_STATUS_NOT_CHARGING; > + > + return 0; > +} > + > +static int hidpp20_battery_voltage_event(struct hidpp_device *hidpp, > + u8 *data, int size) > +{ > + struct hidpp_report *report = (struct hidpp_report *)data; > + int status, voltage; > + > + if (report->fap.feature_index != hidpp- > >battery.voltage_feature_index || > + report->fap.funcindex_clientid != > EVENT_BATTERY_VOLTAGE_STATUS_BROADCAST) > + return 0; > + > + status = hidpp20_battery_map_status_voltage(report->fap.params, > &voltage); > + > + hidpp->battery.online = status != > POWER_SUPPLY_STATUS_NOT_CHARGING; > + > + if (voltage != hidpp->battery.voltage || status != hidpp- > >battery.status) { > + hidpp->battery.voltage = voltage; > + hidpp->battery.status = status; > + if (hidpp->battery.ps) > + power_supply_changed(hidpp->battery.ps); > + } > + return 0; > +} > + > static enum power_supply_property hidpp_battery_props[] = { > POWER_SUPPLY_PROP_ONLINE, > POWER_SUPPLY_PROP_STATUS, > @@ -1228,6 +1343,7 @@ static enum power_supply_property > hidpp_battery_props[] = { > POWER_SUPPLY_PROP_SERIAL_NUMBER, > 0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY, */ > 0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY_LEVEL, */ > + 0, /* placeholder for POWER_SUPPLY_PROP_VOLTAGE_NOW, */ > }; > > static int hidpp_battery_get_property(struct power_supply *psy, > @@ -1265,6 +1381,9 @@ static int hidpp_battery_get_property(struct > power_supply *psy, > case POWER_SUPPLY_PROP_SERIAL_NUMBER: > val->strval = hidpp->hid_dev->uniq; > break; > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + val->intval = hidpp->battery.voltage; > + break; > default: > ret = -EINVAL; > break; > @@ -3083,6 +3202,9 @@ static int hidpp_raw_hidpp_event(struct > hidpp_device *hidpp, u8 *data, > ret = hidpp_solar_battery_event(hidpp, data, size); > if (ret != 0) > return ret; > + ret = hidpp20_battery_voltage_event(hidpp, data, size); > + if (ret != 0) > + return ret; > } > > if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) { > @@ -3204,12 +3326,16 @@ static int hidpp_initialize_battery(struct > hidpp_device *hidpp) > > hidpp->battery.feature_index = 0xff; > hidpp->battery.solar_feature_index = 0xff; > + hidpp->battery.voltage_feature_index = 0xff; > > if (hidpp->protocol_major >= 2) { > if (hidpp->quirks & HIDPP_QUIRK_CLASS_K750) > ret = hidpp_solar_request_battery_event(hidpp); > - else > - ret = hidpp20_query_battery_info(hidpp); > + else { > + ret = > hidpp20_query_battery_voltage_info(hidpp); > + if (ret) > + ret = > hidpp20_query_battery_info(hidpp); > + } > > if (ret) > return ret; > @@ -3234,7 +3360,7 @@ static int hidpp_initialize_battery(struct > hidpp_device *hidpp) > if (!battery_props) > return -ENOMEM; > > - num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 2; > + num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 3; > > if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE) > battery_props[num_battery_props++] = > @@ -3244,6 +3370,10 @@ static int hidpp_initialize_battery(struct > hidpp_device *hidpp) > battery_props[num_battery_props++] = > POWER_SUPPLY_PROP_CAPACITY_LEVEL; > > + if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE) > + battery_props[num_battery_props++] = > + POWER_SUPPLY_PROP_VOLTAGE_NOW; > + > battery = &hidpp->battery; > > n = atomic_inc_return(&battery_no) - 1; > @@ -3407,7 +3537,10 @@ static void hidpp_connect_event(struct > hidpp_device *hidpp) > else > hidpp10_query_battery_status(hidpp); > } else if (hidpp->capabilities & > HIDPP_CAPABILITY_HIDPP20_BATTERY) { > - hidpp20_query_battery_info(hidpp); > + if (hidpp->capabilities & > HIDPP_CAPABILITY_BATTERY_VOLTAGE) > + hidpp20_query_battery_voltage_info(hidpp); > + else > + hidpp20_query_battery_info(hidpp); > } > if (hidpp->battery.ps) > power_supply_changed(hidpp->battery.ps); Reviewed-by: Filipe Laíns <lains@archlinux.org> Thank you, Filipe Laíns
On Sat, 2019-08-31 at 13:56 -0400, Pedro Vanzella wrote: > Newer Logitech mice report their battery voltage through feature > 0x1001 > instead of the battery levels through feature 0x1000. > > When the device is brought up and we try to query the battery, figure > out if it supports the old or the new feature. If it supports the new > feature, record the feature index and read the battery voltage and > its status. FWIW, it wasn't clear to me that there were 3 bytes, and the last one would contain the battery status. I was under the impression reading this that the only thing the mouse would send back would be the voltage, so this might need a slight rewording. Did you test this with upower? Did it work as you expected? Cheers
On Sat, 2019-08-31 at 13:56 -0400, Pedro Vanzella wrote: > +static int hidpp20_battery_map_status_voltage(u8 data[3], int > *voltage) > +{ > + int status; > + > + switch (data[2]) { > + case 0x00: /* discharging */ > + status = POWER_SUPPLY_STATUS_DISCHARGING; > + break; > + case 0x10: /* wireless charging */ > + case 0x80: /* charging */ > + status = POWER_SUPPLY_STATUS_CHARGING; > + break; > + case 0x81: /* fully charged */ > + status = POWER_SUPPLY_STATUS_FULL; > + break; > + default: > + status = POWER_SUPPLY_STATUS_NOT_CHARGING; > + } > + > + *voltage = get_unaligned_be16(data); > + > + return status; > +} Here's the missing specification: +--------+-------------------------------------------------------------------------+ | byte | 2 | +--------+--------------+------------+------------+----------+----------+----------+ | bit | 0..2 | 3 | 4 | 5 | 6 | 7 | +--------+--------------+------------+------------+----------+----------+----------+ | buffer | chargeStatus | fastCharge | slowCharge | critical | (unused) | extPower | +--------+--------------+------------+------------+----------+----------+----------+ Table 1 - battery voltage (0x1001), getBatteryInfo() (ASE 0), 3rd byte +-------+--------------------------------------+ | value | meaning | +-------+--------------------------------------+ | 0 | Charging | +-------+--------------------------------------+ | 1 | End of charge (100% charged) | +-------+--------------------------------------+ | 2 | Charge stopped (any "normal" reason) | +-------+--------------------------------------+ | 7 | Hardware error | +-------+--------------------------------------+ Table 2 - chargeStatus value I know this is already on the 5th revision but could you please change hidpp20_battery_map_status_voltage() to properly handle the 3rd byte? Also, if you could make sure those values are properly exported via sysfs it would be great! We will need them for proper upower support. Sorry for not saying this in my first review. Since then I have done some testing and I discovered that if we want to get accurate upower support we will need the values exposed by the 3rd byte to be exported properly. I am not sure about the endianness of chargeStatus but you can find that easily. Thank you! Filipe Laíns
On 9/9/19 8:00 AM, Filipe Laíns wrote: > On Sat, 2019-08-31 at 13:56 -0400, Pedro Vanzella wrote: >> +static int hidpp20_battery_map_status_voltage(u8 data[3], int >> *voltage) >> +{ >> + int status; >> + >> + switch (data[2]) { >> + case 0x00: /* discharging */ >> + status = POWER_SUPPLY_STATUS_DISCHARGING; >> + break; >> + case 0x10: /* wireless charging */ >> + case 0x80: /* charging */ >> + status = POWER_SUPPLY_STATUS_CHARGING; >> + break; >> + case 0x81: /* fully charged */ >> + status = POWER_SUPPLY_STATUS_FULL; >> + break; >> + default: >> + status = POWER_SUPPLY_STATUS_NOT_CHARGING; >> + } >> + >> + *voltage = get_unaligned_be16(data); >> + >> + return status; >> +} > > Here's the missing specification: > > +--------+-------------------------------------------------------------------------+ > | byte | 2 | > +--------+--------------+------------+------------+----------+----------+----------+ > | bit | 0..2 | 3 | 4 | 5 | 6 | 7 | > +--------+--------------+------------+------------+----------+----------+----------+ > | buffer | chargeStatus | fastCharge | slowCharge | critical | (unused) | extPower | > +--------+--------------+------------+------------+----------+----------+----------+ > Table 1 - battery voltage (0x1001), getBatteryInfo() (ASE 0), 3rd byte > > +-------+--------------------------------------+ > | value | meaning | > +-------+--------------------------------------+ > | 0 | Charging | > +-------+--------------------------------------+ > | 1 | End of charge (100% charged) | > +-------+--------------------------------------+ > | 2 | Charge stopped (any "normal" reason) | > +-------+--------------------------------------+ > | 7 | Hardware error | > +-------+--------------------------------------+ > Table 2 - chargeStatus value > > I know this is already on the 5th revision but could you please change > hidpp20_battery_map_status_voltage() to properly handle the 3rd byte? > Also, if you could make sure those values are properly exported via > sysfs it would be great! We will need them for proper upower support. Sure thing. I'll have a new patch in a few days. Thanks for figuring that stuff out, by the way. > > Sorry for not saying this in my first review. Since then I have done > some testing and I discovered that if we want to get accurate upower > support we will need the values exposed by the 3rd byte to be > exported properly. > > I am not sure about the endianness of chargeStatus but you can find > that easily. > > Thank you! > Filipe Laíns >
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index 0179f7ed77e5..2f215e5be001 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -87,6 +87,7 @@ MODULE_PARM_DESC(disable_tap_to_click, #define HIDPP_CAPABILITY_HIDPP20_BATTERY BIT(1) #define HIDPP_CAPABILITY_BATTERY_MILEAGE BIT(2) #define HIDPP_CAPABILITY_BATTERY_LEVEL_STATUS BIT(3) +#define HIDPP_CAPABILITY_BATTERY_VOLTAGE BIT(4) /* * There are two hidpp protocols in use, the first version hidpp10 is known @@ -135,12 +136,14 @@ struct hidpp_report { struct hidpp_battery { u8 feature_index; u8 solar_feature_index; + u8 voltage_feature_index; struct power_supply_desc desc; struct power_supply *ps; char name[64]; int status; int capacity; int level; + int voltage; /* in millivolts */ bool online; }; @@ -1219,6 +1222,118 @@ static int hidpp20_battery_event(struct hidpp_device *hidpp, return 0; } +/* -------------------------------------------------------------------------- */ +/* 0x1001: Battery voltage */ +/* -------------------------------------------------------------------------- */ + +#define HIDPP_PAGE_BATTERY_VOLTAGE 0x1001 + +#define CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE 0x00 + +#define EVENT_BATTERY_VOLTAGE_STATUS_BROADCAST 0x00 + +static int hidpp20_battery_map_status_voltage(u8 data[3], int *voltage) +{ + int status; + + switch (data[2]) { + case 0x00: /* discharging */ + status = POWER_SUPPLY_STATUS_DISCHARGING; + break; + case 0x10: /* wireless charging */ + case 0x80: /* charging */ + status = POWER_SUPPLY_STATUS_CHARGING; + break; + case 0x81: /* fully charged */ + status = POWER_SUPPLY_STATUS_FULL; + break; + default: + status = POWER_SUPPLY_STATUS_NOT_CHARGING; + } + + *voltage = get_unaligned_be16(data); + + return status; +} + +static int hidpp20_battery_get_battery_voltage(struct hidpp_device *hidpp, + u8 feature_index, + int *status, int *voltage) +{ + struct hidpp_report response; + int ret; + u8 *params = (u8 *)response.fap.params; + + ret = hidpp_send_fap_command_sync(hidpp, feature_index, + CMD_BATTERY_VOLTAGE_GET_BATTERY_VOLTAGE, + NULL, 0, &response); + + if (ret > 0) { + hid_err(hidpp->hid_dev, "%s: received protocol error 0x%02x\n", + __func__, ret); + return -EPROTO; + } + if (ret) + return ret; + + hidpp->capabilities |= HIDPP_CAPABILITY_BATTERY_VOLTAGE; + + *status = hidpp20_battery_map_status_voltage(params, voltage); + + return 0; +} + +static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp) +{ + u8 feature_type; + int ret; + int status, voltage; + + if (hidpp->battery.voltage_feature_index == 0xff) { + ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_BATTERY_VOLTAGE, + &hidpp->battery.voltage_feature_index, + &feature_type); + if (ret) + return ret; + } + + ret = hidpp20_battery_get_battery_voltage(hidpp, + hidpp->battery.voltage_feature_index, + &status, &voltage); + + if (ret) + return ret; + + hidpp->battery.status = status; + hidpp->battery.voltage = voltage; + hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING; + + return 0; +} + +static int hidpp20_battery_voltage_event(struct hidpp_device *hidpp, + u8 *data, int size) +{ + struct hidpp_report *report = (struct hidpp_report *)data; + int status, voltage; + + if (report->fap.feature_index != hidpp->battery.voltage_feature_index || + report->fap.funcindex_clientid != EVENT_BATTERY_VOLTAGE_STATUS_BROADCAST) + return 0; + + status = hidpp20_battery_map_status_voltage(report->fap.params, &voltage); + + hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING; + + if (voltage != hidpp->battery.voltage || status != hidpp->battery.status) { + hidpp->battery.voltage = voltage; + hidpp->battery.status = status; + if (hidpp->battery.ps) + power_supply_changed(hidpp->battery.ps); + } + return 0; +} + static enum power_supply_property hidpp_battery_props[] = { POWER_SUPPLY_PROP_ONLINE, POWER_SUPPLY_PROP_STATUS, @@ -1228,6 +1343,7 @@ static enum power_supply_property hidpp_battery_props[] = { POWER_SUPPLY_PROP_SERIAL_NUMBER, 0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY, */ 0, /* placeholder for POWER_SUPPLY_PROP_CAPACITY_LEVEL, */ + 0, /* placeholder for POWER_SUPPLY_PROP_VOLTAGE_NOW, */ }; static int hidpp_battery_get_property(struct power_supply *psy, @@ -1265,6 +1381,9 @@ static int hidpp_battery_get_property(struct power_supply *psy, case POWER_SUPPLY_PROP_SERIAL_NUMBER: val->strval = hidpp->hid_dev->uniq; break; + case POWER_SUPPLY_PROP_VOLTAGE_NOW: + val->intval = hidpp->battery.voltage; + break; default: ret = -EINVAL; break; @@ -3083,6 +3202,9 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data, ret = hidpp_solar_battery_event(hidpp, data, size); if (ret != 0) return ret; + ret = hidpp20_battery_voltage_event(hidpp, data, size); + if (ret != 0) + return ret; } if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP10_BATTERY) { @@ -3204,12 +3326,16 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp) hidpp->battery.feature_index = 0xff; hidpp->battery.solar_feature_index = 0xff; + hidpp->battery.voltage_feature_index = 0xff; if (hidpp->protocol_major >= 2) { if (hidpp->quirks & HIDPP_QUIRK_CLASS_K750) ret = hidpp_solar_request_battery_event(hidpp); - else - ret = hidpp20_query_battery_info(hidpp); + else { + ret = hidpp20_query_battery_voltage_info(hidpp); + if (ret) + ret = hidpp20_query_battery_info(hidpp); + } if (ret) return ret; @@ -3234,7 +3360,7 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp) if (!battery_props) return -ENOMEM; - num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 2; + num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 3; if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE) battery_props[num_battery_props++] = @@ -3244,6 +3370,10 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp) battery_props[num_battery_props++] = POWER_SUPPLY_PROP_CAPACITY_LEVEL; + if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE) + battery_props[num_battery_props++] = + POWER_SUPPLY_PROP_VOLTAGE_NOW; + battery = &hidpp->battery; n = atomic_inc_return(&battery_no) - 1; @@ -3407,7 +3537,10 @@ static void hidpp_connect_event(struct hidpp_device *hidpp) else hidpp10_query_battery_status(hidpp); } else if (hidpp->capabilities & HIDPP_CAPABILITY_HIDPP20_BATTERY) { - hidpp20_query_battery_info(hidpp); + if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE) + hidpp20_query_battery_voltage_info(hidpp); + else + hidpp20_query_battery_info(hidpp); } if (hidpp->battery.ps) power_supply_changed(hidpp->battery.ps);
Newer Logitech mice report their battery voltage through feature 0x1001 instead of the battery levels through feature 0x1000. When the device is brought up and we try to query the battery, figure out if it supports the old or the new feature. If it supports the new feature, record the feature index and read the battery voltage and its status. If everything went correctly, record the fact that we're capable of querying battery voltage. Note that the protocol only gives us the current voltage in millivolts. Like we do for other ways of interacting with the battery for other devices, refresh the battery status and notify the power supply subsystem of the changes in voltage and status. Since there are no known devices which implement both the old and the new battery feature, we make sure to try the newer feature first and only fall back to the old one if that fails. Signed-off-by: Pedro Vanzella <pedro@pedrovanzella.com> --- drivers/hid/hid-logitech-hidpp.c | 141 ++++++++++++++++++++++++++++++- 1 file changed, 137 insertions(+), 4 deletions(-)