Re: [PATCH v2 1/2] platform/x86:dell-laptop: Add knobs to change battery charge settings

[Andres Salomon <dilinger@queued.net>]

On Thu, 25 Jul 2024 01:01:58 +0200
Pali Rohár <pali@kernel.org> 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@kernel.org> 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