Message-ID: <20240816145244.7de8c4c7@5400>
Date: Fri, 16 Aug 2024 14:52:44 -0400
From: Andres Salomon <dilinger@...ued.net>
To: Pali Rohár <pali@...nel.org>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, LKML
 <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>, linux-pm@...r.kernel.org,
 Dell.Client.Kernel@...l.com
Subject: Re: [PATCH v3 1/2] platform/x86:dell-laptop: Add knobs to change
 battery charge settings

On Fri, 16 Aug 2024 18:33:41 +0200
Pali Rohár <pali@...nel.org> wrote:

> On Friday 16 August 2024 16:56:24 Ilpo Järvinen wrote:
> > On Thu, 15 Aug 2024, Andres Salomon wrote:
> >   
> > > The Dell BIOS allows you to set custom charging modes, which is useful
> > > in particular for extending battery life. This adds support for tweaking
> > > those various settings from Linux via sysfs knobs. One might, for
> > > example, have their laptop plugged into power at their desk the vast
> > > majority of the time and choose fairly aggressive battery-saving
> > > settings (eg, only charging once the battery drops below 50% and only
> > > charging up to 80%). When leaving for a trip, it would be more useful
> > > to instead switch to a standard charging mode (top off at 100%, charge
> > > any time power is available). Rebooting into the BIOS to change the
> > > charging mode settings is a hassle.
> > > 
> > > For the Custom charging type mode, we reuse the common
> > > charge_control_{start,end}_threshold sysfs power_supply entries. The
> > > BIOS also has a bunch of other charging modes (with varying levels of
> > > usefulness), so this also adds a 'charge_type' sysfs entry that maps the
> > > standard values to Dell-specific ones and documents those mappings in
> > > sysfs-class-power-dell.
> > > 
> > > This work is based on a patch by Perry Yuan <perry_yuan@...l.com> and
> > > Limonciello Mario <Mario_Limonciello@...l.com> submitted back in 2020:
> > > https://lore.kernel.org/all/20200729065424.12851-1-Perry_Yuan@Dell.com/
> > > Both of their email addresses bounce, so I'm assuming they're no longer
> > > with the company. I've reworked most of the patch to make it smaller and
> > > cleaner.
> > > 
> > > Signed-off-by: Andres Salomon <dilinger@...ued.net>
> > > ---
> > > 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@...nel.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@...r.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"
> > > +
> > > 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[] = {  
> > 
> > Please don't try to do this in one go but split it into two (define and 
> > then declaration of the variable).  
> 
> Why? Splitting definition of this anonymous structure and definition of
> variable would leak definition of anonymous structure of out the scope
> where it is used.


Also, it's two different arrays that we then have to keep synced as we
add new modes. That's the main reason I went for the combined struct.


> 
> > > +	{ 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" },  
> > 
> > I suggest aligning the strings with tabs for better readability.  
> 
> For aligning something for better readability in "git diff" and
> "git show" (which includes also git format-patch and emails) is better
> to use spaces and not tabs. tab-alignment in git makes worse readability
> (due to name of the token).
> 

Okay, so I'm hearing to align them, but to use spaces? I'll line everything
up with where "Standard" and "Adaptive" are.

[...]
> > > +
> > > +/*
> > > + * The rules: the minimum start charging value is 50%. The maximum
> > > + * start charging value is 95%. The minimum end charging value is
> > > + * 55%. The maximum end charging value is 100%. And finally, there
> > > + * has to be at least a 5% difference between start & end values.
> > > + */
> > > +#define CHARGE_START_MIN	50
> > > +#define CHARGE_START_MAX	95
> > > +#define CHARGE_END_MIN		55
> > > +#define CHARGE_END_MAX		100
> > > +#define CHARGE_MIN_DIFF		5
> > > +
> > > +static int dell_battery_set_custom_charge_start(int start)
> > > +{
> > > +	struct calling_interface_buffer buffer;
> > > +	int end;
> > > +
> > > +	if (start < CHARGE_START_MIN)
> > > +		start = CHARGE_START_MIN;
> > > +	else if (start > CHARGE_START_MAX)
> > > +		start = CHARGE_START_MAX;  
> > 
> > We have clamp().

Thanks, I'll use that.

> >   
> > > +
> > > +	end = dell_battery_read(BAT_CUSTOM_CHARGE_END);
> > > +	if (end < 0)
> > > +		return end;
> > > +	if ((end - start) < CHARGE_MIN_DIFF)  
> > 
> > Extra parenthesis.  
> 
> I pointed about this in previous version and from the discussion the
> conclusion was that there is no reason to remove extra parenthesis.
> 
> > > +		start = end - CHARGE_MIN_DIFF;
> > > +
> > > +	return dell_set_std_token_value(&buffer, BAT_CUSTOM_CHARGE_START,
> > > +			start);
> > > +}
> > > +
> > > +static int dell_battery_set_custom_charge_end(int end)
> > > +{
> > > +	struct calling_interface_buffer buffer;
> > > +	int start;
> > > +
> > > +	if (end < CHARGE_END_MIN)
> > > +		end = CHARGE_END_MIN;
> > > +	else if (end > CHARGE_END_MAX)
> > > +		end = CHARGE_END_MAX;  
> > 
> > clamp.
> > 

+1

  
> > > +	start = dell_battery_read(BAT_CUSTOM_CHARGE_START);
> > > +	if (start < 0)
> > > +		return start;
> > > +	if ((end - start) < CHARGE_MIN_DIFF)  
> > 
> > Extra parenthesis.
> >   
> > > +		end = start + CHARGE_MIN_DIFF;
> > > +
> > > +	return dell_set_std_token_value(&buffer, BAT_CUSTOM_CHARGE_END, end);
> > > +}
> > > +
> > > +static ssize_t charge_type_show(struct device *dev,
> > > +		struct device_attribute *attr,
> > > +		char *buf)
> > > +{
> > > +	ssize_t count = 0;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> > > +		bool active;
> > > +
> > > +		if (!(battery_supported_modes & BIT(i)))  
> > 
> > Why not store this supported information into battery_modes itself?  
> 
> Same style is already used in other parts in this driver / source file.
> 
> > What's the benefit of obfuscation it with the extra variable & BIT()?  
> 
> In my opinion, this is not obfuscation but clear and common style how to
> check which values of some enumeration are supported.
> 
> Storing this kind of information into battery_modes is not possible
> because battery_modes is constant array with constant data.
> 

Yep. It's not a given that all modes will be supported by the BIOS.


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

Powered by blists - more mailing lists