Am 26.07.24 um 00:15 schrieb Pali Rohár:
On Thursday 25 July 2024 16:24:57 Andres Salomon wrote:
On Thu, 25 Jul 2024 01:01:58 +0200
Pali Rohár <pali@xxxxxxxxxx> wrote:
On Wednesday 24 July 2024 18:23:18 Andres Salomon wrote:
On Wed, 24 Jul 2024 22:45:23 +0200
Pali Rohár <pali@xxxxxxxxxx> wrote:
On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote:
Hello, the driver change looks good. I have just few minor comments for
this change below.
Anyway, if there is somebody on the list with Dell laptop with 2 or 3
batteries, see below, it would be nice to check how such laptop would
behave with this patch. It does not seem that patch should cause
regression but always it is better to do testing if it is possible.
On Tuesday 23 July 2024 22:05:02 Andres Salomon wrote:
[...]
And because CLASS_TOKEN_WRITE is being repeated, what about defining
something like this?
static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value)
{
dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value);
}
Just an idea. Do you think that it could be useful in second patch?
Let me implement the other changes first and then take a look.
Ok.
For the helper function, I noticed that all of the CLASS_TOKEN_WRITEs
also have SELECT_TOKEN_STD except for one (dell_send_intensity). So I
think it makes sense to have the helper function also do that as well.
Eg,
static inline int dell_set_std_token_value(struct calling_interface_buffer *buffer, u16 tokenid, u32 value)
{
dell_send_request_for_tokenid(buffer, SELECT_TOKEN_STD, CLASS_TOKEN_WRITE, tokenid, value);
}
I agree with your renaming to dell_send_request_for_tokenid, btw.
+static int dell_battery_read(const int type)
+{
+ struct calling_interface_buffer buffer;
+ int err;
+
+ err = dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_READ,
+ SELECT_TOKEN_STD, type, 0);
+ if (err)
+ return err;
+
+ return buffer.output[1];
buffer.output[1] is of type u32. So theoretically it can contain value
above 2^31. For safety it would be better to check if the output[1]
value fits into INT_MAX and if not then return something like -ERANGE
(or some other better errno code).
I ended up returning -EIO here, with the logic that if we're getting
some nonsense value from SMBIOS, it could either be junk in the stored
values or communication corruption.
Likewise, I used -EIO for the checks in charge_control_start_threshold_show
and charge_control_end_threshold_show when SMBIOS returns values > 100%.
+ if (end < 0)
+ end = CHARGE_END_MAX;
+ if ((end - start) < CHARGE_MIN_DIFF)
nit: I'm not sure what is the correct coding style for kernel drivers
but I thought that parenthesis around (end - start) are not being
written.
I think it makes the comparison much easier to read. I'd prefer to
keep it, unless the coding style specifically forbids it.
As I said I'm really not sure. So if nobody would complain then you can
let it as is.
You can use ./scripts/checkpatch.pl application which is in git tree,
which does basic checks for code style. It cannot prove if something is
really correct but it can prove if something is incorrect.
+
+static u32 __init battery_get_supported_modes(void)
+{
+ u32 modes = 0;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
+ if (dell_smbios_find_token(battery_modes[i].token))
+ modes |= BIT(i);
+ }
+
+ return modes;
+}
+
+static void __init dell_battery_init(struct device *dev)
+{
+ battery_supported_modes = battery_get_supported_modes();
+
+ if (battery_supported_modes != 0)
+ battery_hook_register(&dell_battery_hook);
Anyway, how is this battery_hook_register() suppose to work on systems
with multiple batteries? The provided API is only for the primary
battery, but on older Dell laptop it was possible to connect up to
3 batteries. Would not this case some issues?
This interface is _only_ for controlling charging of the primary battery.
In theory, it should hopefully ignore any other batteries, which would
need to have their settings changed in the BIOS or with a special tool or
whatever.
That is OK. But where it is specified that the hook is being registered
only for the primary battery? Because from the usage it looks like that
the hook is applied either for all batteries present in the system or
for some one arbitrary chosen battery.
However, I'm basing that assumption on the SMBIOS interface. These tokens
are marked "Primary Battery". There are separate tokens marked "Battery
Slice", which from my understanding was an older type of battery that
From SMBIOS perspective it is clear, each battery seems to have its own
tokens.
The issue here is: how to tell kernel that the particular
dell_battery_hook has to be bound with the primary battery?
So from userspace, we've got the expectation that multiple batteries
would show up as /sys/class/power_supply/BAT0, /sys/class/power_supply/BAT1,
and so on.
Yes, I hope so.
The current BAT0 entry shows things like 'capacity' even without this
patch, and we're just piggybacking off of that to add charge_type and
other entries. So there shouldn't be any confusion there, agreed?
I have not looked at the battery_hook_register() code yet (seems that I
would have to properly read it and understand it). But does it mean that
battery_hook_register() is adding hook just for "BAT0"?
What I mean: cannot that hook be registered to "BAT1" too? Because if
yes then we should prevent it. Otherwise this hook which is for "Dell
Primary Battery" could be registered also for secondary battery "BAT1".
(I hope that now it is more clear what I mean).
Hi,
the battery hook is being registered to all ACPI batteries present on a given system,
so you need to do some manual filtering when .add_battery() is called.
As a side note: i suspect that "newer" Dell machines use a different interface for controlling
battery charging, since the Dell Power Manager software on my machine seems to provide some
additional features not found in this token-based interface.
Unfortunately i am not sure if reverse-engineering the Dell software is legal, does the kernel
community have some helping guides in this area? If it is legal, then i would happily volunteer
to do the reverse-engineering.
Otherwise maybe someone at Dell can provide some clarifications if a different interface for controlling
battery charging exists and how to use it?
Thanks,
Armin Wolf
In the kernel, we're registering the acpi_battery_hook as "Dell Primary
Battery Extension". The functions set up by that acpi_battery_hook are
the only ones using battery_support_modes. We could make it more explicit
by:
1) renaming it to primary_battery_modes, along with
dell_primary_battery_{init,exit} and/or
2) allocating the mode mask and strings dynamically, and storing that
inside of the dev kobject.
However, #2 seems overly complicated for what we're doing. In the
circumstances that we want to add support for secondary batteries,
we're going to need to query separate tokens, add another
acpi_battery_hook, and also add a second mask. Whether that's a global
variable or kept inside pdev seems like more of a style issue than
anything else.
#1 is easy enough to change, if you think that's necessary.
I think that "Dell Primary Battery Extension" is OK. All SMBIOS code is
currently primary-battery only.
The only my point is to prevent this &dell_battery_hook to be registered
for non-primary battery.
