Resumitting this after having rebased it against the latest changes.
The gaming line of Logitech devices doesn't use the old hidpp20
feature for battery level reporting. Instead, they report the
current voltage of the battery, in millivolts.
This patch set handles this case by adding a quirk to the
devices we know to have this new feature, in both wired
and wireless mode.
This version of the patch set is better split, as well as adding the
quirk to make sure we don't needlessly probe every device connected.
This quirk allows us to pick which devices support the 0x1001 hidpp
feature to read the battery voltage.
Signed-off-by: Pedro Vanzella <[email protected]>
---
drivers/hid/hid-logitech-hidpp.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 0179f7ed77e5..402ddba93adc 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -59,7 +59,8 @@ MODULE_PARM_DESC(disable_tap_to_click,
#define HIDPP_QUIRK_CLASS_G920 BIT(3)
#define HIDPP_QUIRK_CLASS_K750 BIT(4)
-/* bits 2..20 are reserved for classes */
+/* bits 2..1f are reserved for classes */
+#define HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 BIT(20)
/* #define HIDPP_QUIRK_CONNECT_EVENTS BIT(21) disabled */
#define HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS BIT(22)
#define HIDPP_QUIRK_NO_HIDINPUT BIT(23)
@@ -3732,6 +3733,13 @@ static const struct hid_device_id hidpp_devices[] = {
LDJ_DEVICE(0xb30b),
.driver_data = HIDPP_QUIRK_HIDPP_CONSUMER_VENDOR_KEYS },
+ { /* Logitech G403 Gaming Mouse over Lightspeed */
+ LDJ_DEVICE(0x405d),
+ .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
+ { /* Logitech G900 Gaming Mouse over Lightspeed */
+ LDJ_DEVICE(0x4053),
+ .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
+
{ LDJ_DEVICE(HID_ANY_ID) },
{ /* Keyboard LX501 (Y-RR53) */
@@ -3750,13 +3758,15 @@ static const struct hid_device_id hidpp_devices[] = {
{ L27MHZ_DEVICE(HID_ANY_ID) },
{ /* Logitech G403 Wireless Gaming Mouse over USB */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC082) },
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC082),
+ .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
{ /* Logitech G703 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC087) },
{ /* Logitech G703 Hero Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC090) },
{ /* Logitech G900 Gaming Mouse over USB */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC081) },
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC081),
+ .driver_data = HIDPP_QUIRK_BATTERY_VOLTAGE_X1001 },
{ /* Logitech G903 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC086) },
{ /* Logitech G903 Hero Gaming Mouse over USB */
--
2.23.0
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.
Signed-off-by: Pedro Vanzella <[email protected]>
---
drivers/hid/hid-logitech-hidpp.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 06bee97d33b4..9f09ed6abbad 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1310,6 +1310,35 @@ static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp)
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;
+ bool changed;
+
+ if (report->fap.feature_index != hidpp->battery.voltage_feature_index ||
+ report->fap.funcindex_clientid !=
+ EVENT_BATTERY_LEVEL_STATUS_BROADCAST)
+ return 0;
+
+ status = hidpp20_battery_map_status_voltage(report->fap.params,
+ &voltage);
+
+ hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
+
+ changed = voltage != hidpp->battery.voltage ||
+ status != hidpp->battery.status;
+
+ if (changed) {
+ 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,
@@ -3178,6 +3207,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) {
--
2.23.0
If we know the device supports reading its voltage, report that.
Note that the protocol only gives us the current voltage in millivolts.
Signed-off-by: Pedro Vanzella <[email protected]>
---
drivers/hid/hid-logitech-hidpp.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index e1ddc9cdcc8b..06bee97d33b4 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1319,6 +1319,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,
@@ -1356,6 +1357,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;
@@ -3328,7 +3332,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++] =
@@ -3338,6 +3342,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;
--
2.23.0
When the device is brought up, if it's one of the devices we know
supports battery voltage checking, figure out the feature index
and get the battery voltage and status.
If everything went correctly, record the fact that we're capable
of querying battery voltage.
Signed-off-by: Pedro Vanzella <[email protected]>
---
drivers/hid/hid-logitech-hidpp.c | 95 ++++++++++++++++++++++++++++++++
1 file changed, 95 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 402ddba93adc..e1ddc9cdcc8b 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -88,6 +88,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
@@ -136,12 +137,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;
};
@@ -1220,6 +1223,93 @@ 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
+
+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 = (data[0] << 8) + data[1];
+
+ 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 enum power_supply_property hidpp_battery_props[] = {
POWER_SUPPLY_PROP_ONLINE,
POWER_SUPPLY_PROP_STATUS,
@@ -3205,10 +3295,13 @@ 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 if (hidpp->quirks & HIDPP_QUIRK_BATTERY_VOLTAGE_X1001)
+ ret = hidpp20_query_battery_voltage_info(hidpp);
else
ret = hidpp20_query_battery_info(hidpp);
@@ -3409,6 +3502,8 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
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);
}
if (hidpp->battery.ps)
power_supply_changed(hidpp->battery.ps);
--
2.23.0
Hi Pedro,
On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella <[email protected]> wrote:
>
> Resumitting this after having rebased it against the latest changes.
thanks for resubmitting. Sorry I wasn't able to provide feedback on
the last revision
>
> The gaming line of Logitech devices doesn't use the old hidpp20
> feature for battery level reporting. Instead, they report the
> current voltage of the battery, in millivolts.
>
> This patch set handles this case by adding a quirk to the
> devices we know to have this new feature, in both wired
> and wireless mode.
So the quirk is in the end a bad idea after all. I had some chats with
Filipe that made me realize this.
Re-reading our previous exchanges also made me understood why I wasn't
happy with the initial submission: for every request the code was
checking both features 0x1000 and 0x1001 when we can remember this
once and for all during hidpp_initialize_battery().
So I think we should remove the useless quirk in the end (bad idea
from me, I concede), and instead during hidpp_initialize_battery() set
the correct HIDPP_CAPABILITY_*.
Not entirely sure if we should try to call 0x1000, or 0x1001 or if we
should rely on the 0x0001 feature to know which feature is available,
but this should be implementation detail.
>
> This version of the patch set is better split, as well as adding the
> quirk to make sure we don't needlessly probe every device connected.
It is for sure easy to review, but doesn't make much sense in the end.
I think we should squash all the patches together as you are just
adding one feature in the driver, and it is a little bit disturbing to
first add the quirk that has no use, then set up the structs when they
are not used, and so on, so forth.
Cheers,
Benjamin
>
>
Hi Benjamin,
On 8/23/19 4:25 AM, Benjamin Tissoires wrote:
> Hi Pedro,
>
> On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella <[email protected]> wrote:
>>
>> Resumitting this after having rebased it against the latest changes.
>
> thanks for resubmitting. Sorry I wasn't able to provide feedback on
> the last revision
>
No worries, I know how these things are.
>>
>> The gaming line of Logitech devices doesn't use the old hidpp20
>> feature for battery level reporting. Instead, they report the
>> current voltage of the battery, in millivolts.
>>
>> This patch set handles this case by adding a quirk to the
>> devices we know to have this new feature, in both wired
>> and wireless mode.
>
> So the quirk is in the end a bad idea after all. I had some chats with
> Filipe that made me realize this.
I actually resubmitted by Filipe's request, since the patches weren't
applying cleanly anymore. The idea was to apply these patches and in the
future refactor the code to use the feature discovery routines.
> Re-reading our previous exchanges also made me understood why I wasn't
> happy with the initial submission: for every request the code was
> checking both features 0x1000 and 0x1001 when we can remember this
> once and for all during hidpp_initialize_battery().
Yeah I wasn't too happy about this either, but since there was nothing
prohibiting some device to have both features enabled, I thought this
wasn't too horrible.
>
> So I think we should remove the useless quirk in the end (bad idea
> from me, I concede), and instead during hidpp_initialize_battery() set
> the correct HIDPP_CAPABILITY_*.
> Not entirely sure if we should try to call 0x1000, or 0x1001 or if we
> should rely on the 0x0001 feature to know which feature is available,
> but this should be implementation detail.
I like the idea of calling 0x0001 once on device boot much better. I
think we could easily just fetch the location for the features we know
about and want to deal with once only. This also makes sure we support
every single device that supports this feature, so that is a huge plus.
In fact, I think we should _not_ call 0x0001 on battery init, but only
call battery init _after_ we called 0x0001 and discovered either 0x1000
or 0x1001 (or the solar battery feature, or any other one that might
crop up in the feature).
>
>>
>> This version of the patch set is better split, as well as adding the
>> quirk to make sure we don't needlessly probe every device connected.
>
> It is for sure easy to review, but doesn't make much sense in the end.
> I think we should squash all the patches together as you are just
> adding one feature in the driver, and it is a little bit disturbing to
> first add the quirk that has no use, then set up the structs when they
> are not used, and so on, so forth.
You're right. My first instinct was to send a single patch. As much as I
tested this, I always feel like breaking the patch up post-facto will
break a git bisect in the future and everyone will hate me :P
So we (you, me and Filipe) should probably come up with an action plan
here. The way I see it there are two issues here: one is adding this
feature, and the other is refactoring to use feature discovery for all
features. There are advantages and disadvantages to doing one or another
first and we might want to discuss that.
By merging this first (probably after I resubmit it as a single squashed
patch) we get to test it a bit better and have a usable feature sooner.
Plenty of people have been requesting this and there is plenty of stuff
that can be built on top of it, but only once this is actually merged I
think.
On the other hand, by first refactoring the rest of the code to use
0x0001 we avoid some rework on this patch. It should be minor, as most
functions here do all the heavy lifting after the initial feature
discovery, and are thus mostly independent from how that is done.
I'm happy either way, so just let me know what you guys decide.
If you guys (or anyone else reading this on the public list, really) has
any input - naming things being notoriosly hard, I'm actually happy with
nitpicking - I'd appreciate it. On that note, come to think of it, I'm
not 100% sure reporting the voltage in milivolts is the standard way. I
looked through the docs, but found no solid guideline. It was either
that or a float, so I think I made the right call here, but still.
- Pedro
>
> Cheers,
> Benjamin
>
>>
>>
On Fri, Aug 23, 2019 at 4:22 PM Pedro Vanzella <[email protected]> wrote:
>
> Hi Benjamin,
>
> On 8/23/19 4:25 AM, Benjamin Tissoires wrote:
> > Hi Pedro,
> >
> > On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella <[email protected]> wrote:
> >>
> >> Resumitting this after having rebased it against the latest changes.
> >
> > thanks for resubmitting. Sorry I wasn't able to provide feedback on
> > the last revision
> >
>
> No worries, I know how these things are.
>
> >>
> >> The gaming line of Logitech devices doesn't use the old hidpp20
> >> feature for battery level reporting. Instead, they report the
> >> current voltage of the battery, in millivolts.
> >>
> >> This patch set handles this case by adding a quirk to the
> >> devices we know to have this new feature, in both wired
> >> and wireless mode.
> >
> > So the quirk is in the end a bad idea after all. I had some chats with
> > Filipe that made me realize this.
>
> I actually resubmitted by Filipe's request, since the patches weren't
> applying cleanly anymore. The idea was to apply these patches and in the
> future refactor the code to use the feature discovery routines.
>
> > Re-reading our previous exchanges also made me understood why I wasn't
> > happy with the initial submission: for every request the code was
> > checking both features 0x1000 and 0x1001 when we can remember this
> > once and for all during hidpp_initialize_battery().
>
> Yeah I wasn't too happy about this either, but since there was nothing
> prohibiting some device to have both features enabled, I thought this
> wasn't too horrible.
I honestly don't think we will find one device that has both features
enabled. It doesn't make sense as the "new" feature allows for fine
tuning in the software, which would be moot if you also enable the
percentage.
>
> >
> > So I think we should remove the useless quirk in the end (bad idea
> > from me, I concede), and instead during hidpp_initialize_battery() set
> > the correct HIDPP_CAPABILITY_*.
> > Not entirely sure if we should try to call 0x1000, or 0x1001 or if we
> > should rely on the 0x0001 feature to know which feature is available,
> > but this should be implementation detail.
>
> I like the idea of calling 0x0001 once on device boot much better. I
> think we could easily just fetch the location for the features we know
> about and want to deal with once only. This also makes sure we support
> every single device that supports this feature, so that is a huge plus.
>
> In fact, I think we should _not_ call 0x0001 on battery init, but only
> call battery init _after_ we called 0x0001 and discovered either 0x1000
> or 0x1001 (or the solar battery feature, or any other one that might
> crop up in the feature).
ack for that
>
> >
> >>
> >> This version of the patch set is better split, as well as adding the
> >> quirk to make sure we don't needlessly probe every device connected.
> >
> > It is for sure easy to review, but doesn't make much sense in the end.
> > I think we should squash all the patches together as you are just
> > adding one feature in the driver, and it is a little bit disturbing to
> > first add the quirk that has no use, then set up the structs when they
> > are not used, and so on, so forth.
>
> You're right. My first instinct was to send a single patch. As much as I
> tested this, I always feel like breaking the patch up post-facto will
> break a git bisect in the future and everyone will hate me :P
as long as the patches are compiling and are not breaking, git bisect
will not be a problem. However, we might end up with the last one,
which is not very explicit in what it does as it just enables the
features implemented previously.
>
> So we (you, me and Filipe) should probably come up with an action plan
> here. The way I see it there are two issues here: one is adding this
> feature, and the other is refactoring to use feature discovery for all
> features. There are advantages and disadvantages to doing one or another
> first and we might want to discuss that.
>
> By merging this first (probably after I resubmit it as a single squashed
> patch) we get to test it a bit better and have a usable feature sooner.
> Plenty of people have been requesting this and there is plenty of stuff
> that can be built on top of it, but only once this is actually merged I
> think.
>
> On the other hand, by first refactoring the rest of the code to use
> 0x0001 we avoid some rework on this patch. It should be minor, as most
> functions here do all the heavy lifting after the initial feature
> discovery, and are thus mostly independent from how that is done.
>
> I'm happy either way, so just let me know what you guys decide.
I think we should merge your v3 squashed series with a slight
autodetection during battery init, like the method you used in the v1.
This would remove the quirk, but keep the straightforward commands
when addressing battery data.
Relying on 0x0001 should be done separately and can come in in a later
patch IMO (unless you plan to work on it, in which case you can send
both at once).
The problem I have with quirks, and that I explained to Filipe on IRC
is that this is kernel ABI. Even if there is a very low chance we have
someone using this, re-using the same drv_data bit in the future might
break someone's device.
>
> If you guys (or anyone else reading this on the public list, really) has
> any input - naming things being notoriosly hard, I'm actually happy with
> nitpicking - I'd appreciate it. On that note, come to think of it, I'm
> not 100% sure reporting the voltage in milivolts is the standard way. I
> looked through the docs, but found no solid guideline. It was either
> that or a float, so I think I made the right call here, but still.
I am not sure either. Adding Bastien as he has a lot more experience in upower.
But I am under the impression that the kernel part is more "try to
deal with whatever the hardware provides, and deal with it in user
space".
On Fri, 2019-08-23 at 16:32 +0200, Benjamin Tissoires wrote:
> The problem I have with quirks, and that I explained to Filipe on IRC
> is that this is kernel ABI. Even if there is a very low chance we have
> someone using this, re-using the same drv_data bit in the future might
> break someone's device.
But we can reserve those bits. I don't expect there to be a ton of
devices that need to be quirked, so using the remaining bits should be
perfectly fine. Do you agree?
On Fri, 2019-08-23 at 10:22 -0400, Pedro Vanzella wrote:
> I actually resubmitted by Filipe's request, since the patches weren't
> applying cleanly anymore. The idea was to apply these patches and in the
> future refactor the code to use the feature discovery routines.
Yes, I want to refactor everything so I though there was no point in us
changing the patch set again. I did not review the first revision of
the patch set so if that works for you (Benjamin) we can just merge
that.
> So we (you, me and Filipe) should probably come up with an action plan
> here. The way I see it there are two issues here: one is adding this
> feature, and the other is refactoring to use feature discovery for all
> features. There are advantages and disadvantages to doing one or another
> first and we might want to discuss that.
>
> By merging this first (probably after I resubmit it as a single squashed
> patch) we get to test it a bit better and have a usable feature sooner.
> Plenty of people have been requesting this and there is plenty of stuff
> that can be built on top of it, but only once this is actually merged I
> think.
>
> On the other hand, by first refactoring the rest of the code to use
> 0x0001 we avoid some rework on this patch. It should be minor, as most
> functions here do all the heavy lifting after the initial feature
> discovery, and are thus mostly independent from how that is done.
>
> I'm happy either way, so just let me know what you guys decide.
I am also fine either way so I think we should just re-send the first
revision of your patch set as Benjamin requested.
Thank you,
Filipe Laíns
On Fri, Aug 23, 2019 at 4:48 PM Filipe Laíns <[email protected]> wrote:
>
> On Fri, 2019-08-23 at 16:32 +0200, Benjamin Tissoires wrote:
> > The problem I have with quirks, and that I explained to Filipe on IRC
> > is that this is kernel ABI. Even if there is a very low chance we have
> > someone using this, re-using the same drv_data bit in the future might
> > break someone's device.
>
> But we can reserve those bits. I don't expect there to be a ton of
> devices that need to be quirked, so using the remaining bits should be
> perfectly fine. Do you agree?
Nope. If the code is not ready for upstream, it should not be included.
Cheers,
Benjamin
On 8/23/19 10:32 AM, Benjamin Tissoires wrote:
> On Fri, Aug 23, 2019 at 4:22 PM Pedro Vanzella <[email protected]> wrote:
>>
>> Hi Benjamin,
>>
>> On 8/23/19 4:25 AM, Benjamin Tissoires wrote:
>>> Hi Pedro,
>>>
>>> On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella <[email protected]> wrote:
>>>>
>>>> Resumitting this after having rebased it against the latest changes.
>>>
>>> thanks for resubmitting. Sorry I wasn't able to provide feedback on
>>> the last revision
>>>
>>
>> No worries, I know how these things are.
>>
>>>>
>>>> The gaming line of Logitech devices doesn't use the old hidpp20
>>>> feature for battery level reporting. Instead, they report the
>>>> current voltage of the battery, in millivolts.
>>>>
>>>> This patch set handles this case by adding a quirk to the
>>>> devices we know to have this new feature, in both wired
>>>> and wireless mode.
>>>
>>> So the quirk is in the end a bad idea after all. I had some chats with
>>> Filipe that made me realize this.
>>
>> I actually resubmitted by Filipe's request, since the patches weren't
>> applying cleanly anymore. The idea was to apply these patches and in the
>> future refactor the code to use the feature discovery routines.
>>
>>> Re-reading our previous exchanges also made me understood why I wasn't
>>> happy with the initial submission: for every request the code was
>>> checking both features 0x1000 and 0x1001 when we can remember this
>>> once and for all during hidpp_initialize_battery().
>>
>> Yeah I wasn't too happy about this either, but since there was nothing
>> prohibiting some device to have both features enabled, I thought this
>> wasn't too horrible.
>
> I honestly don't think we will find one device that has both features
> enabled. It doesn't make sense as the "new" feature allows for fine
> tuning in the software, which would be moot if you also enable the
> percentage.
>
>>
>>>
>>> So I think we should remove the useless quirk in the end (bad idea
>>> from me, I concede), and instead during hidpp_initialize_battery() set
>>> the correct HIDPP_CAPABILITY_*.
>>> Not entirely sure if we should try to call 0x1000, or 0x1001 or if we
>>> should rely on the 0x0001 feature to know which feature is available,
>>> but this should be implementation detail.
>>
>> I like the idea of calling 0x0001 once on device boot much better. I
>> think we could easily just fetch the location for the features we know
>> about and want to deal with once only. This also makes sure we support
>> every single device that supports this feature, so that is a huge plus.
>>
>> In fact, I think we should _not_ call 0x0001 on battery init, but only
>> call battery init _after_ we called 0x0001 and discovered either 0x1000
>> or 0x1001 (or the solar battery feature, or any other one that might
>> crop up in the feature).
>
> ack for that
>
>>
>>>
>>>>
>>>> This version of the patch set is better split, as well as adding the
>>>> quirk to make sure we don't needlessly probe every device connected.
>>>
>>> It is for sure easy to review, but doesn't make much sense in the end.
>>> I think we should squash all the patches together as you are just
>>> adding one feature in the driver, and it is a little bit disturbing to
>>> first add the quirk that has no use, then set up the structs when they
>>> are not used, and so on, so forth.
>>
>> You're right. My first instinct was to send a single patch. As much as I
>> tested this, I always feel like breaking the patch up post-facto will
>> break a git bisect in the future and everyone will hate me :P
>
> as long as the patches are compiling and are not breaking, git bisect
> will not be a problem. However, we might end up with the last one,
> which is not very explicit in what it does as it just enables the
> features implemented previously.
>
>>
>> So we (you, me and Filipe) should probably come up with an action plan
>> here. The way I see it there are two issues here: one is adding this
>> feature, and the other is refactoring to use feature discovery for all
>> features. There are advantages and disadvantages to doing one or another
>> first and we might want to discuss that.
>>
>> By merging this first (probably after I resubmit it as a single squashed
>> patch) we get to test it a bit better and have a usable feature sooner.
>> Plenty of people have been requesting this and there is plenty of stuff
>> that can be built on top of it, but only once this is actually merged I
>> think.
>>
>> On the other hand, by first refactoring the rest of the code to use
>> 0x0001 we avoid some rework on this patch. It should be minor, as most
>> functions here do all the heavy lifting after the initial feature
>> discovery, and are thus mostly independent from how that is done.
>>
>> I'm happy either way, so just let me know what you guys decide.
>
> I think we should merge your v3 squashed series with a slight
> autodetection during battery init, like the method you used in the v1.
> This would remove the quirk, but keep the straightforward commands
> when addressing battery data.
Alright, I rebased against for-5.4/logitech to make sure, squashed
everything and restored the detection code from v1, removing the quirk.
Tested and it works on both wired and wireless on my G900.
>
> Relying on 0x0001 should be done separately and can come in in a later
> patch IMO (unless you plan to work on it, in which case you can send
> both at once).
0x0001 is quite the task and I think Filipe already has a good plan to
tackle it, so I'll leave that for him.
>
> The problem I have with quirks, and that I explained to Filipe on IRC
> is that this is kernel ABI. Even if there is a very low chance we have
> someone using this, re-using the same drv_data bit in the future might
> break someone's device.
>
>>
>> If you guys (or anyone else reading this on the public list, really) has
>> any input - naming things being notoriosly hard, I'm actually happy with
>> nitpicking - I'd appreciate it. On that note, come to think of it, I'm
>> not 100% sure reporting the voltage in milivolts is the standard way. I
>> looked through the docs, but found no solid guideline. It was either
>> that or a float, so I think I made the right call here, but still.
>
> I am not sure either. Adding Bastien as he has a lot more experience in upower.
>
> But I am under the impression that the kernel part is more "try to
> deal with whatever the hardware provides, and deal with it in user
> space".
>
I'll submit v4 as a single patch in the next couple of minutes.
- Pedro