Message-ID: <20240722155232.4dea956b@5400>
Date: Mon, 22 Jul 2024 15:52:32 -0400
From: Andres Salomon <dilinger@...ued.net>
To: Pali Rohár <pali@...nel.org>
Cc: linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
 Matthew Garrett <mjg59@...f.ucam.org>, Sebastian Reichel <sre@...nel.org>,
 Hans de Goede <hdegoede@...hat.com>, Ilpo Järvinen
 <ilpo.jarvinen@...ux.intel.com>, linux-pm@...r.kernel.org,
 Dell.Client.Kernel@...l.com, Mario Limonciello <mario.limonciello@....com>
Subject: Re: [PATCH] platform/x86:dell-laptop: Add knobs to change battery
 charge settings

On Mon, 22 Jul 2024 20:41:32 +0200
Pali Rohár <pali@...nel.org> wrote:

> On Monday 22 July 2024 14:34:32 Andres Salomon wrote:
> > On Mon, 22 Jul 2024 09:18:45 +0200
> > Pali Rohár <pali@...nel.org> wrote:
> >   
> > > On Sunday 21 July 2024 19:58:51 Andres Salomon wrote:  
> > > > On Mon, 22 Jul 2024 01:40:37 +0200
> > > > Pali Rohár <pali@...nel.org> wrote:
> > > >     
> > > > > On Sunday 21 July 2024 19:37:16 Andres Salomon wrote:    
> > > > > > On Sun, 21 Jul 2024 11:02:38 +0200
> > > > > > Pali Rohár <pali@...nel.org> wrote:
> > > > > >       
> > > > > > > On Saturday 20 July 2024 22:06:06 Andres Salomon wrote:      
> > > > > > > > On Sat, 20 Jul 2024 11:55:07 +0200
> > > > > > > > Pali Rohár <pali@...nel.org> wrote:
> > > > > > > >         
> > > > > > > > > On Saturday 20 July 2024 05:24:19 Andres Salomon wrote:        
> > > > > > > > > > Thanks for the quick feedback! Responses below.
> > > > > > > > > > 
> > > > > > > > > > On Sat, 20 Jul 2024 10:40:19 +0200
> > > > > > > > > > Pali Rohár <pali@...nel.org> wrote:
> > > > > > > > > >           
> > > > > > 
> > > > > > [...]
> > > > > >       
> > > > > > > > > > > > +
> > > > > > > > > > > > +static void __init dell_battery_init(struct device *dev)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +	enum battery_charging_mode current_mode = DELL_BAT_MODE_NONE;
> > > > > > > > > > > > +
> > > > > > > > > > > > +	dell_battery_read_req(BAT_CUSTOM_MODE_TOKEN, (int *) &current_mode);
> > > > > > > > > > > > +	if (current_mode != DELL_BAT_MODE_NONE) {            
> > > > > > > > > > > 
> > > > > > > > > > > I quite do not understand how is this code suppose to work.
> > > > > > > > > > > 
> > > > > > > > > > > Why is there mix of custom kernel enum battery_charging_mode and return
> > > > > > > > > > > value from Dell's API?          
> > > > > > > > > > 
> > > > > > > > > > This is from the original patch from Dell; tbh, I'm not sure. It does
> > > > > > > > > > work, though. That is, current_mode ends up holding the correct value
> > > > > > > > > > based on what was previously set, even if the charging mode is set from
> > > > > > > > > > the BIOS.
> > > > > > > > > > 
> > > > > > > > > > I just scanned through the libsmbios code to see what it's doing, and
> > > > > > > > > > it appears to loop through every charging mode to check if its active.
> > > > > > > > > > I'm not really sure that makes much more sense, so I'll try some more
> > > > > > > > > > tests.          
> > > > > > > > > 
> > > > > > > > > Keyboard backlight code (kbd_get_first_active_token_bit) is doing also
> > > > > > > > > this type scan. If I remember correctly, for every keyboard backlight
> > > > > > > > > token we just know the boolean value - if the token is set or not.
> > > > > > > > > 
> > > > > > > > > It would really nice to see what (raw) value is returned by the
> > > > > > > > > dell_battery_read_req(token) function for every battery token and for
> > > > > > > > > every initial state.        
> > > > > > > > 
> > > > > > > > I checked this. The BIOS sets the mode value in every related token
> > > > > > > > location. I'm still not really sure what libsmbios is doing, but the
> > > > > > > > kernel code seems to arbitrarily choose one of the token locations
> > > > > > > > to read from. This makes sense to me now.
> > > > > > > > 
> > > > > > > > In the BIOS when I set the mode to "ExpressCharge",
> > > > > > > > this what I pulled for each token location:
> > > > > > > > 
> > > > > > > > [    5.704651] dell-laptop dell-laptop: BAT_CUSTOM_MODE_TOKEN value: 2
> > > > > > > > [    5.707015] dell-laptop dell-laptop: BAT_PRI_AC_MODE_TOKEN value: 2
> > > > > > > > [    5.709114] dell-laptop dell-laptop: BAT_ADAPTIVE_MODE_TOKEN value: 2
> > > > > > > > [    5.711041] dell-laptop dell-laptop: BAT_STANDARD_MODE_TOKEN value: 2
> > > > > > > > [    5.713705] dell-laptop dell-laptop: BAT_EXPRESS_MODE_TOKEN value: 2
> > > > > > > > 
> > > > > > > > Similar story when I set it to Custom (all were '5'), or Standard ('1').
> > > > > > > > When I set it from linux as well, it changed all location values.        
> > > > > > > 
> > > > > > > Interesting... Anyway, I still think that the API could be similar to
> > > > > > > what is used in keyboard backlight.
> > > > > > > 
> > > > > > > Could you please dump all information about each token? They are in
> > > > > > > struct calling_interface_token returned by dell_smbios_find_token.
> > > > > > > 
> > > > > > > I'm interesting in tokenID, location and value.
> > > > > > > 
> > > > > > > Ideally to compare what is in token->value and then in buffer.output[1]
> > > > > > > (in case dell_send_request does not fail).      
> > > > > > 
> > > > > > 
> > > > > > Alright, here's what I see:
> > > > > > 
> > > > > > [    5.904775] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5
> > > > > > [    5.908675] dell_laptop: dell_battery_read_req: buffer.output[1]=3
> > > > > > [    5.908680] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 3
> > > > > > [    5.908682] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3
> > > > > > [    5.910922] dell_laptop: dell_battery_read_req: buffer.output[1]=3
> > > > > > [    5.910926] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 3
> > > > > > [    5.910928] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4
> > > > > > [    5.913042] dell_laptop: dell_battery_read_req: buffer.output[1]=3
> > > > > > [    5.913046] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 3
> > > > > > [    5.913048] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1
> > > > > > [    5.914996] dell_laptop: dell_battery_read_req: buffer.output[1]=3
> > > > > > [    5.914999] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 3
> > > > > > [    5.915000] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2
> > > > > > [    5.916723] dell_laptop: dell_battery_read_req: buffer.output[1]=3
> > > > > > [    5.916724] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 3
> > > > > > [    5.916725] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535
> > > > > > [    5.918727] dell_laptop: dell_battery_read_req: buffer.output[1]=65
> > > > > > [    5.918731] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65
> > > > > > [    5.918734] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535
> > > > > > [    5.920864] dell_laptop: dell_battery_read_req: buffer.output[1]=85
> > > > > > [    5.920867] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85      
> > > > > 
> > > > > Perfect. And can you check dumps when the mode is set to some other than BAT_PRI_AC_MODE_TOKEN?    
> > > > 
> > > > Here's Express:
> > > > 
> > > > [    5.880090] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5
> > > > [    5.882011] dell_laptop: dell_battery_read_req: buffer.output[1]=2
> > > > [    5.882014] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 2
> > > > [    5.882016] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3
> > > > [    5.894513] dell_laptop: dell_battery_read_req: buffer.output[1]=2
> > > > [    5.894518] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 2
> > > > [    5.894520] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4
> > > > [    5.913870] dell_laptop: dell_battery_read_req: buffer.output[1]=2
> > > > [    5.913874] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 2
> > > > [    5.913875] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1
> > > > [    5.915622] dell_laptop: dell_battery_read_req: buffer.output[1]=2
> > > > [    5.915625] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 2
> > > > [    5.915626] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2
> > > > [    5.917349] dell_laptop: dell_battery_read_req: buffer.output[1]=2
> > > > [    5.917351] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 2
> > > > [    5.917352] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535
> > > > [    5.919068] dell_laptop: dell_battery_read_req: buffer.output[1]=65
> > > > [    5.919070] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65
> > > > [    5.919071] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535
> > > > [    5.920780] dell_laptop: dell_battery_read_req: buffer.output[1]=85
> > > > [    5.920782] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85
> > > > 
> > > > And here's Adaptive:
> > > > 
> > > > [    5.945319] dell_laptop: dell_battery_read_req: token requested: 0x343, tokenID=0x343, location=0x343, value=5
> > > > [    5.973685] dell_laptop: dell_battery_read_req: buffer.output[1]=4
> > > > [    5.973690] dell_laptop: dell_battery_init: BAT_CUSTOM_MODE_TOKEN value: 4
> > > > [    5.973692] dell_laptop: dell_battery_read_req: token requested: 0x341, tokenID=0x341, location=0x341, value=3
> > > > [    5.976533] dell_laptop: dell_battery_read_req: buffer.output[1]=4
> > > > [    5.976538] dell_laptop: dell_battery_init: BAT_PRI_AC_MODE_TOKEN value: 4
> > > > [    5.976540] dell_laptop: dell_battery_read_req: token requested: 0x342, tokenID=0x342, location=0x342, value=4
> > > > [    5.981013] dell_laptop: dell_battery_read_req: buffer.output[1]=4
> > > > [    5.981018] dell_laptop: dell_battery_init: BAT_ADAPTIVE_MODE_TOKEN value: 4
> > > > [    5.981020] dell_laptop: dell_battery_read_req: token requested: 0x346, tokenID=0x346, location=0x346, value=1
> > > > [    5.983474] dell_laptop: dell_battery_read_req: buffer.output[1]=4
> > > > [    5.983479] dell_laptop: dell_battery_init: BAT_STANDARD_MODE_TOKEN value: 4
> > > > [    5.983481] dell_laptop: dell_battery_read_req: token requested: 0x347, tokenID=0x347, location=0x347, value=2
> > > > [    5.985881] dell_laptop: dell_battery_read_req: buffer.output[1]=4
> > > > [    5.985885] dell_laptop: dell_battery_init: BAT_EXPRESS_MODE_TOKEN value: 4
> > > > [    5.985887] dell_laptop: dell_battery_read_req: token requested: 0x349, tokenID=0x349, location=0x349, value=65535
> > > > [    5.988332] dell_laptop: dell_battery_read_req: buffer.output[1]=65
> > > > [    5.988337] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_START value: 65
> > > > [    5.988339] dell_laptop: dell_battery_read_req: token requested: 0x34a, tokenID=0x34a, location=0x34a, value=65535
> > > > [    5.990769] dell_laptop: dell_battery_read_req: buffer.output[1]=85
> > > > [    5.990774] dell_laptop: dell_battery_init: BAT_CUSTOM_CHARGE_END value: 85
> > > > 
> > > > 
> > > > 
> > > > -- 
> > > > I'm available for contract & employment work, see:
> > > > https://spindle.queued.net/~dilinger/resume-tech.pdf    
> > > 
> > > Nice! So it is exactly same as API of keyboard backlight tokens. Thanks.
> > > 
> > > In dell_battery_write_req function you can drop second "val" argument
> > > and replace it by token->value. So the dell_fill_request call in that
> > > function would look like:
> > > 
> > >     dell_fill_request(&buffer, token->location, token->value, 0, 0);  
> > 
> > 
> > Well, except that we use dell_battery_write_req for writing the charge
> > start/end values as well (in dell_battery_custom_set). Those can't be
> > obtained from token->value.
> > 
> > We could have two separate functions for that, or set 'val' to a
> > sentinel value (0) that, if detected, we set val=token->value. I'm
> > still not really understanding the point, though.  
> 
> I think that two separate functions would be needed. One which set
> battery mode (enum) and which set custom thresholds.
> 
> > > 
> > > And then you can mimic the usage as it is done in keyboard backlight
> > > functions (kbd_get_first_active_token_bit).
> > > 
> > > If you do not know what I mean then later (today or tomorrow) I can
> > > write code example of the functionality.  
> > 
> > Sorry, I still don't understand what the goal is here. Is the goal to
> > not pull from a random location to determine the current charging mode?
> > Is the goal to determine what charging modes are currently supported
> > (and if so, I don't see how)? Is the goal to avoid having the kernel
> > hardcode a list of enums that the BIOS might have different values
> > for? Is the goal to merge the keyboard backlight and battery setting
> > functions?  
> 
> Avoid having the kernel hardcoded values for enums which SMBIOS
> provides. Future (or maybe also older) modes may have different enum
> values. So we should use what SMBIOS provides to us.
> 

*nod*, that makes sense.


> Also to determinate which charging modes are supported by the current HW
> configuration. If BIOS does not support some mode or does not allow to
> set some mode, kernel should not export this as supported option.


Alright, I'll see what I can do about this.

Unfortunately, with no real way to test it, it's very theoretical for me.
For what it's worth, I've checked on 6 separate laptops (all Latitudes,
ranging from an e7240 to a 5310), and they all have the exact same
(primary) battery charging options in the BIOS; nothing greyed out. That
includes a newer one that's missing a (non-removable) battery, and the
e7240 with removable battery. Even without batteries, the BIOS settings
didn't change. The only real change was the BIOS in the e7240, which had
a separate section for slice batteries. I don't doubt that the long life
batteries have a way to grey out the ExpressCharge option, but it's
unclear if querying ExpressCharge when a long life battery is installed
actually returns an error, or has an empty value in its SMBIOS token
value, or returns empty output (like kbd_get_token_bit checks for), or
what. And for the older removable batteries, the BIOS probably had
to be smart enough to handle the case where you have a normal battery
installed, set the BIOS to ExpressCharge, and then swap in a long life
battery. I would suspect the BIOS can handle it, but probably still keeps
the current value set to ExpressCharge and just quietly does a Standard
charge?

The only way to grey out any of the primary battery options is to select
"Enable Advanced Battery Charge Mode", which is a separate thing that
allows setting ExpressCharge during workdays and then automatically
disabling it at night. When that option is set and the primary battery
charge options are greyed out, the dump from the kernel driver looks the
same.

> 
> If you do not see how to do it, please give me some time, I will send
> you an example. Going to look at it right now.
> 
> Merging keyboard backlight and battery code is bonus, not required.
> But I thought that it would be easier to build a new code from common
> blocks.



-- 
I'm available for contract & employment work, see:
https://spindle.queued.net/~dilinger/resume-tech.pdf

Powered by blists - more mailing lists