* [PATCH v3] HID: logitech-hidpp: battery: provide CAPACITY property for newer devices
@ 2021-07-23 18:57 Hamza Mahfooz
2021-07-23 19:11 ` Bastien Nocera
2021-07-23 19:42 ` Filipe Laíns
0 siblings, 2 replies; 5+ messages in thread
From: Hamza Mahfooz @ 2021-07-23 18:57 UTC (permalink / raw)
To: linux-kernel
Cc: Bastien Nocera, Filipe Laíns, Hamza Mahfooz, Jiri Kosina,
Benjamin Tissoires, linux-input
For devices that only support the BATTERY_VOLTAGE (0x1001) feature, UPower
requires the additional information provided by this patch, to set them up.
Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>
---
v2: use ARRAY_SIZE() and set voltages[]'s size to 100
v3: add a check to ensure that exactly 100 elements are in voltages[]
---
drivers/hid/hid-logitech-hidpp.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 61635e629469..4921823144de 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1331,6 +1331,33 @@ static int hidpp20_battery_get_battery_voltage(struct hidpp_device *hidpp,
return 0;
}
+static int hidpp20_map_battery_capacity(int voltage)
+{
+ static const int voltages[] = {
+ 4186, 4156, 4143, 4133, 4122, 4113, 4103, 4094, 4086, 4075,
+ 4067, 4059, 4051, 4043, 4035, 4027, 4019, 4011, 4003, 3997,
+ 3989, 3983, 3976, 3969, 3961, 3955, 3949, 3942, 3935, 3929,
+ 3922, 3916, 3909, 3902, 3896, 3890, 3883, 3877, 3870, 3865,
+ 3859, 3853, 3848, 3842, 3837, 3833, 3828, 3824, 3819, 3815,
+ 3811, 3808, 3804, 3800, 3797, 3793, 3790, 3787, 3784, 3781,
+ 3778, 3775, 3772, 3770, 3767, 3764, 3762, 3759, 3757, 3754,
+ 3751, 3748, 3744, 3741, 3737, 3734, 3730, 3726, 3724, 3720,
+ 3717, 3714, 3710, 3706, 3702, 3697, 3693, 3688, 3683, 3677,
+ 3671, 3666, 3662, 3658, 3654, 3646, 3633, 3612, 3579, 3537
+ };
+
+ int i;
+
+ BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);
+
+ for (i = 0; i < ARRAY_SIZE(voltages); i++) {
+ if (voltage >= voltages[i])
+ return ARRAY_SIZE(voltages) - i;
+ }
+
+ return 0;
+}
+
static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp)
{
u8 feature_type;
@@ -1354,6 +1381,7 @@ static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp)
hidpp->battery.status = status;
hidpp->battery.voltage = voltage;
+ hidpp->battery.capacity = hidpp20_map_battery_capacity(voltage);
hidpp->battery.level = level;
hidpp->battery.charge_type = charge_type;
hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
@@ -1378,6 +1406,7 @@ static int hidpp20_battery_voltage_event(struct hidpp_device *hidpp,
if (voltage != hidpp->battery.voltage || status != hidpp->battery.status) {
hidpp->battery.voltage = voltage;
+ hidpp->battery.capacity = hidpp20_map_battery_capacity(voltage);
hidpp->battery.status = status;
hidpp->battery.level = level;
hidpp->battery.charge_type = charge_type;
@@ -3717,7 +3746,8 @@ static int hidpp_initialize_battery(struct hidpp_device *hidpp)
num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 3;
if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE ||
- hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_PERCENTAGE)
+ hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_PERCENTAGE ||
+ hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
battery_props[num_battery_props++] =
POWER_SUPPLY_PROP_CAPACITY;
--
2.32.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v3] HID: logitech-hidpp: battery: provide CAPACITY property for newer devices
2021-07-23 18:57 [PATCH v3] HID: logitech-hidpp: battery: provide CAPACITY property for newer devices Hamza Mahfooz
@ 2021-07-23 19:11 ` Bastien Nocera
2021-07-23 19:42 ` Filipe Laíns
1 sibling, 0 replies; 5+ messages in thread
From: Bastien Nocera @ 2021-07-23 19:11 UTC (permalink / raw)
To: Hamza Mahfooz, linux-kernel
Cc: Filipe Laíns, Jiri Kosina, Benjamin Tissoires, linux-input
On Fri, 2021-07-23 at 14:57 -0400, Hamza Mahfooz wrote:
> For devices that only support the BATTERY_VOLTAGE (0x1001) feature,
> UPower
> requires the additional information provided by this patch, to set
> them up.
>
> Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>
> ---
>
> v2: use ARRAY_SIZE() and set voltages[]'s size to 100
>
> v3: add a check to ensure that exactly 100 elements are in voltages[]
> ---
> <snip>
> + BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);
Sweet!
Reviewed-By: Bastien Nocera <hadess@hadess.net>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] HID: logitech-hidpp: battery: provide CAPACITY property for newer devices
2021-07-23 18:57 [PATCH v3] HID: logitech-hidpp: battery: provide CAPACITY property for newer devices Hamza Mahfooz
2021-07-23 19:11 ` Bastien Nocera
@ 2021-07-23 19:42 ` Filipe Laíns
2021-07-23 21:39 ` Hamza Mahfooz
1 sibling, 1 reply; 5+ messages in thread
From: Filipe Laíns @ 2021-07-23 19:42 UTC (permalink / raw)
To: Hamza Mahfooz, linux-kernel
Cc: Bastien Nocera, Jiri Kosina, Benjamin Tissoires, linux-input
[-- Attachment #1: Type: text/plain, Size: 4672 bytes --]
On Fri, 2021-07-23 at 14:57 -0400, Hamza Mahfooz wrote:
> For devices that only support the BATTERY_VOLTAGE (0x1001) feature, UPower
> requires the additional information provided by this patch, to set them up.
>
> Signed-off-by: Hamza Mahfooz <someguy@effective-light.com>
> ---
>
> v2: use ARRAY_SIZE() and set voltages[]'s size to 100
>
> v3: add a check to ensure that exactly 100 elements are in voltages[]
> ---
> drivers/hid/hid-logitech-hidpp.c | 32 +++++++++++++++++++++++++++++++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 61635e629469..4921823144de 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -1331,6 +1331,33 @@ static int hidpp20_battery_get_battery_voltage(struct
> hidpp_device *hidpp,
> return 0;
> }
>
> +static int hidpp20_map_battery_capacity(int voltage)
> +{
> + static const int voltages[] = {
> + 4186, 4156, 4143, 4133, 4122, 4113, 4103, 4094, 4086, 4075,
> + 4067, 4059, 4051, 4043, 4035, 4027, 4019, 4011, 4003, 3997,
> + 3989, 3983, 3976, 3969, 3961, 3955, 3949, 3942, 3935, 3929,
> + 3922, 3916, 3909, 3902, 3896, 3890, 3883, 3877, 3870, 3865,
> + 3859, 3853, 3848, 3842, 3837, 3833, 3828, 3824, 3819, 3815,
> + 3811, 3808, 3804, 3800, 3797, 3793, 3790, 3787, 3784, 3781,
> + 3778, 3775, 3772, 3770, 3767, 3764, 3762, 3759, 3757, 3754,
> + 3751, 3748, 3744, 3741, 3737, 3734, 3730, 3726, 3724, 3720,
> + 3717, 3714, 3710, 3706, 3702, 3697, 3693, 3688, 3683, 3677,
> + 3671, 3666, 3662, 3658, 3654, 3646, 3633, 3612, 3579, 3537
> + };
Hi Hamza,
I think it's important to note that this table depends on the battery technology
(type). While most devices use this map, not all do. I think it's reasonable to
just assume this is the map as we don't really have the capability to test all
products and the products that do not use this map are very few.
That said, I think we should definitely have a comment here nothing that, and
possible have some bounds checks for the reported voltage value hinting that
there may be bug.
Cheers,
Filipe Laíns
> +
> + int i;
> +
> + BUILD_BUG_ON(ARRAY_SIZE(voltages) != 100);
> +
> + for (i = 0; i < ARRAY_SIZE(voltages); i++) {
> + if (voltage >= voltages[i])
> + return ARRAY_SIZE(voltages) - i;
> + }
> +
> + return 0;
> +}
> +
> static int hidpp20_query_battery_voltage_info(struct hidpp_device *hidpp)
> {
> u8 feature_type;
> @@ -1354,6 +1381,7 @@ static int hidpp20_query_battery_voltage_info(struct
> hidpp_device *hidpp)
>
> hidpp->battery.status = status;
> hidpp->battery.voltage = voltage;
> + hidpp->battery.capacity = hidpp20_map_battery_capacity(voltage);
> hidpp->battery.level = level;
> hidpp->battery.charge_type = charge_type;
> hidpp->battery.online = status != POWER_SUPPLY_STATUS_NOT_CHARGING;
> @@ -1378,6 +1406,7 @@ static int hidpp20_battery_voltage_event(struct
> hidpp_device *hidpp,
>
> if (voltage != hidpp->battery.voltage || status != hidpp-
> >battery.status) {
> hidpp->battery.voltage = voltage;
> + hidpp->battery.capacity = hidpp20_map_battery_capacity(voltage);
> hidpp->battery.status = status;
> hidpp->battery.level = level;
> hidpp->battery.charge_type = charge_type;
> @@ -3717,7 +3746,8 @@ static int hidpp_initialize_battery(struct hidpp_device
> *hidpp)
> num_battery_props = ARRAY_SIZE(hidpp_battery_props) - 3;
>
> if (hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_MILEAGE ||
> - hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_PERCENTAGE)
> + hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_PERCENTAGE ||
> + hidpp->capabilities & HIDPP_CAPABILITY_BATTERY_VOLTAGE)
> battery_props[num_battery_props++] =
> POWER_SUPPLY_PROP_CAPACITY;
>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] HID: logitech-hidpp: battery: provide CAPACITY property for newer devices
2021-07-23 19:42 ` Filipe Laíns
@ 2021-07-23 21:39 ` Hamza Mahfooz
2021-07-31 22:08 ` Filipe Laíns
0 siblings, 1 reply; 5+ messages in thread
From: Hamza Mahfooz @ 2021-07-23 21:39 UTC (permalink / raw)
To: Filipe Laíns
Cc: linux-kernel, Bastien Nocera, Jiri Kosina, Benjamin Tissoires,
linux-input
On Fri, Jul 23 2021 at 08:42:32 PM +0100, Filipe Laíns
<lains@riseup.net> wrote:
> That said, I think we should definitely have a comment here nothing
> that, and
> possible have some bounds checks for the reported voltage value
> hinting that
> there may be bug.
Hey Filipe,
Do you have any thoughts on what the bounds ought to be?
3500 mV seems like a rather safe lower bound, however the upper bound
seems much more fuzzy.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v3] HID: logitech-hidpp: battery: provide CAPACITY property for newer devices
2021-07-23 21:39 ` Hamza Mahfooz
@ 2021-07-31 22:08 ` Filipe Laíns
0 siblings, 0 replies; 5+ messages in thread
From: Filipe Laíns @ 2021-07-31 22:08 UTC (permalink / raw)
To: Hamza Mahfooz
Cc: linux-kernel, Bastien Nocera, Jiri Kosina, Benjamin Tissoires,
linux-input
[-- Attachment #1: Type: text/plain, Size: 714 bytes --]
On Fri, 2021-07-23 at 17:39 -0400, Hamza Mahfooz wrote:
>
> On Fri, Jul 23 2021 at 08:42:32 PM +0100, Filipe Laíns
> <lains@riseup.net> wrote:
> > That said, I think we should definitely have a comment here nothing
> > that, and
> > possible have some bounds checks for the reported voltage value
> > hinting that
> > there may be bug.
>
> Hey Filipe,
>
> Do you have any thoughts on what the bounds ought to be?
> 3500 mV seems like a rather safe lower bound, however the upper bound
> seems much more fuzzy.
>
>
Hi Hamza,
Sorry for the delay getting back to you! The most relevant bound would be the
lower one, but I think 5000mV would be a good value.
Cheers,
Filipe Laíns
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-31 22:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 18:57 [PATCH v3] HID: logitech-hidpp: battery: provide CAPACITY property for newer devices Hamza Mahfooz
2021-07-23 19:11 ` Bastien Nocera
2021-07-23 19:42 ` Filipe Laíns
2021-07-23 21:39 ` Hamza Mahfooz
2021-07-31 22:08 ` Filipe Laíns
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).