Message-ID: <20240722184132.l6nibqkpna2wkszo@pali>
Date: Mon, 22 Jul 2024 20:41:32 +0200
From: Pali Rohár <pali@...nel.org>
To: Andres Salomon <dilinger@...ued.net>
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 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.

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.

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.

Powered by blists - more mailing lists