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

[Andres Salomon <dilinger@queued.net>]

On Sun, 21 Jul 2024 11:02:38 +0200
Pali Rohár <pali@kernel.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@kernel.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@kernel.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



> 
> > > 
> > > Because it is really suspicious if dell_battery_read_req() would return
> > > value of the enum battery_charging_mode (as this is kernel enum).
> > > 
> > > 
> > > Also another important thing. In past it was possible to buy from Dell
> > > special batteries with long warranty (3+ years). I'm not sure if these
> > > batteries are still available for end-user customers. With this type of
> > > battery, it was not possible to change charging mode to ExpressCharge
> > > (bios option was fade-out). I do not have such battery anymore, but I
> > > would expect that the firmware disabled BAT_EXPRESS_MODE_TOKEN as mark
> > > it as unavailable.
> > > 
> > > I think that we should scan list of available tokens, like it is done
> > > for keyboard backlight in kbd_init_tokens(). And export via sysfs only
> > > those battery modes for which there is available token.  
> > 
> > I agree, but I'm not seeing a way to do that right now given how the
> > BIOS sets the mode. Maybe libsmbios has some clues...  
> 
> Try to get those dumps and I will try to do something with it.
> 
> > 
> > 
> > 
> > 
> > -- 
> > I'm available for contract & employment work, see:
> > https://spindle.queued.net/~dilinger/resume-tech.pdf  



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