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.
> > > > > +
> > > > > +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.
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?
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'm available for contract & employment work, see:
https://spindle.queued.net/~dilinger/resume-tech.pdf
