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

[Andres Salomon <dilinger@queued.net>]

On Mon, 19 Aug 2024 15:59:45 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 8/16/24 1:28 AM, Andres Salomon wrote:
[...]
> 
> > ---
> > Changes in v3:
> >     - switch tokenid and class types
> >     - be stricter with results from both userspace and BIOS
> >     - no longer allow failed BIOS reads
> >     - rename/move dell_send_request_by_token_loc, and add helper function
> >     - only allow registration for BAT0
> >     - rename charge_type modes to match power_supply names
> > Changes in v2, based on extensive feedback from Pali Rohár <pali@kernel.org>:
> >     - code style changes
> >     - change battery write API to use token->value instead of passed value
> >     - stop caching current mode, instead querying SMBIOS as needed
> >     - drop the separate list of charging modes enum
> >     - rework the list of charging mode strings
> >     - query SMBIOS for supported charging modes
> >     - split dell_battery_custom_set() up
> > ---
> >  .../ABI/testing/sysfs-class-power-dell        |  33 ++
> >  drivers/platform/x86/dell/Kconfig             |   1 +
> >  drivers/platform/x86/dell/dell-laptop.c       | 316 ++++++++++++++++++
> >  drivers/platform/x86/dell/dell-smbios.h       |   7 +
> >  4 files changed, 357 insertions(+)
> >  create mode 100644 Documentation/ABI/testing/sysfs-class-power-dell
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-class-power-dell b/Documentation/ABI/testing/sysfs-class-power-dell
> > new file mode 100644
> > index 000000000000..d8c542177558
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-power-dell
> > @@ -0,0 +1,33 @@
> > +What:		/sys/class/power_supply/<supply_name>/charge_type
> > +Date:		August 2024
> > +KernelVersion:	6.12
> > +Contact:	linux-pm@vger.kernel.org
> > +Description:
> > +		Select the charging algorithm to use for the (primary)
> > +		battery.
> > +
> > +		Standard:
> > +			Fully charge the battery at a moderate rate.
> > +		Fast:
> > +			Quickly charge the battery using fast-charge
> > +			technology. This is harder on the battery than
> > +			standard charging and may lower its lifespan.
> > +			The Dell BIOS calls this ExpressCharge™.
> > +		Trickle:
> > +			Users who primarily operate the system while
> > +			plugged into an external power source can extend
> > +			battery life with this mode. The Dell BIOS calls
> > +			this "Primarily AC Use".
> > +		Adaptive:
> > +			Automatically optimize battery charge rate based
> > +			on typical usage pattern.
> > +		Custom:
> > +			Use the charge_control_* properties to determine
> > +			when to start and stop charging. Advanced users
> > +			can use this to drastically extend battery life.
> > +
> > +		Access: Read, Write
> > +		Valid values:
> > +			      "Standard", "Fast", "Trickle",
> > +			      "Adaptive", "Custom"
> > +  
> 
> As the kernel test robot reports:
> 
> Warning: /sys/class/power_supply/<supply_name>/charge_type is defined 2 times:  ./Documentation/ABI/testing/sysfs-class-power-dell:0  ./Documentation/ABI/testing/sysfs-class-power:375
> 
> So please drop this. What you could do is instead submit (as a separate) patch
> an update to Documentation/ABI/testing/sysfs-class-power:375 to use your IMHO
> more readable version.
> 
> And when doing so I think it would best to replace "The Dell BIOS calls this ..."
> with "In vendor tooling this may also be called ...".
> 
> 

Is this what you had in mind? I don't see many users of charge_type, and
I'm not sure whether the assumptions I'm making there are correct.

https://lore.kernel.org/lkml/20240820041942.30ed42f3@5400/

> > diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> > index 85a78ef91182..02405793163c 100644
> > --- a/drivers/platform/x86/dell/Kconfig
> > +++ b/drivers/platform/x86/dell/Kconfig
> > @@ -49,6 +49,7 @@ config DELL_LAPTOP
> >  	default m
> >  	depends on DMI
> >  	depends on BACKLIGHT_CLASS_DEVICE
> > +	depends on ACPI_BATTERY
> >  	depends on ACPI_VIDEO || ACPI_VIDEO = n
> >  	depends on RFKILL || RFKILL = n
> >  	depends on DELL_WMI || DELL_WMI = n
> > diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> > index 6552dfe491c6..8cc05f0fab91 100644
> > --- a/drivers/platform/x86/dell/dell-laptop.c
> > +++ b/drivers/platform/x86/dell/dell-laptop.c
> > @@ -22,11 +22,13 @@
> >  #include <linux/io.h>
> >  #include <linux/rfkill.h>
> >  #include <linux/power_supply.h>
> > +#include <linux/sysfs.h>
> >  #include <linux/acpi.h>
> >  #include <linux/mm.h>
> >  #include <linux/i8042.h>
> >  #include <linux/debugfs.h>
> >  #include <linux/seq_file.h>
> > +#include <acpi/battery.h>
> >  #include <acpi/video.h>
> >  #include "dell-rbtn.h"
> >  #include "dell-smbios.h"
> > @@ -99,6 +101,18 @@ static bool force_rfkill;
> >  static bool micmute_led_registered;
> >  static bool mute_led_registered;
> >  
> > +static const struct {
> > +	int token;
> > +	const char *label;
> > +} battery_modes[] = {
> > +	{ BAT_STANDARD_MODE_TOKEN, "Standard" },
> > +	{ BAT_EXPRESS_MODE_TOKEN, "Fast" },
> > +	{ BAT_PRI_AC_MODE_TOKEN, "Trickle" },
> > +	{ BAT_ADAPTIVE_MODE_TOKEN, "Adaptive" },
> > +	{ BAT_CUSTOM_MODE_TOKEN, "Custom" },
> > +};  
> 
> So Ilpo suggested to split this (first declare the struct type and then
> declare an array of that type) and Pali suggested to keep this as is.
> 
> > +static u32 battery_supported_modes;  
> 
> I see there also is some disagreement on keeping battery_modes[]
> const vs adding a flag for this in the struct.
> 
> IMHO in both cases either option is fine, so you (Andres) get
> to choose which solution you prefer in either case.
> 
> I do see there is some confusion about Ilpo's split suggestion,
> to clarify that, what I believe is Ilpo meant doing something
> like this:

Thanks for clarify that, my fault for misreading what they'd written!






-- 
I'm available for contract & employment work, please contact me if
interested.