Message-ID: <20240724204523.xb6rp7ba6yqi5klt@pali>
Date: Wed, 24 Jul 2024 22:45:23 +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
Subject: Re: [PATCH v2 1/2] platform/x86:dell-laptop: Add knobs to change
 battery charge settings

On Wednesday 24 July 2024 22:34:03 Pali Rohár wrote:
> Hello, the driver change looks good. I have just few minor comments for
> this change below.
> 
> Anyway, if there is somebody on the list with Dell laptop with 2 or 3
> batteries, see below, it would be nice to check how such laptop would
> behave with this patch. It does not seem that patch should cause
> regression but always it is better to do testing if it is possible.
> 
> On Tuesday 23 July 2024 22:05:02 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 (only charging once the battery drops to 50% and only charging
> > up to 80%). When leaving for a trip, they might want to switch those
> > settings to charge to 100% and charge any time power is available;
> > rebooting into the BIOS to change those 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 adds a new Dell-specific sysfs entry called
> > 'charge_type' that's documented in sysfs-class-power-dell (and looks a
> > lot like sysfs-class-power-wilco).
> > 
> > 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 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        |  31 ++
> >  drivers/platform/x86/dell/Kconfig             |   1 +
> >  drivers/platform/x86/dell/dell-laptop.c       | 288 ++++++++++++++++++
> >  drivers/platform/x86/dell/dell-smbios.h       |   7 +
> >  4 files changed, 327 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..ef72c5f797ce
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-power-dell
> > @@ -0,0 +1,31 @@
> > +What:		/sys/class/power_supply/<supply_name>/charge_type
> > +Date:		July 2024
> > +KernelVersion:	6.11
> > +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.
> > +		ExpressCharge™:
> > +			Quickly charge the battery using fast-charge
> > +			technology. This is harder on the battery than
> > +			standard charging and may lower its lifespan.
> > +		Primarily AC Use:
> > +			Users who primarily operate the system while
> > +			plugged into an external power source can extend
> > +			battery life with this mode.
> > +		Adaptive:
> > +			Automatically optimize battery charge rate based
> > +			on typical usage.
> > +		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", "express", "primarily_ac",
> > +			      "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..aae9a95fb188 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, "express" },
> > +	{ BAT_PRI_AC_MODE_TOKEN, "primarily_ac" },
> > +	{ BAT_ADAPTIVE_MODE_TOKEN, "adaptive" },
> > +	{ BAT_CUSTOM_MODE_TOKEN, "custom" },
> > +};
> > +static u32 battery_supported_modes;
> > +
> >  module_param(force_rfkill, bool, 0444);
> >  MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
> >  
> > @@ -2183,6 +2197,277 @@ static struct led_classdev mute_led_cdev = {
> >  	.default_trigger = "audio-mute",
> >  };
> >  
> > +static int dell_send_request_by_token_loc(struct calling_interface_buffer *buffer,
> > +					  u16 class, u16 select, int type,
> > +					  u32 val)
> 
> "int type" argument is not type, but tokenid. tokenid is 16-bit unsigned
> integer, so it would be better to use "u16 tokenid" instead of "int type".
> And same applies for all functions below too.
> 
> And I think that better name for this function could be
> "dell_set_token_value" or something like that. "_by_token_loc" sounds
> strange, this function is setting smbios token to some value.

Ou. Just now I figured out that this function is universal and is doing
both SET and GET, based on the "u16 select" parameter. It is not
SET-ONLY as I originally thought. So what about rather name
"dell_send_request_for_tokenid"?

And because CLASS_TOKEN_WRITE is being repeated, what about defining
something like this?

    static inline int dell_set_token_value(struct calling_interface_buffer *buffer, u16 class, u16 tokenid, u32 value)
    {
        dell_send_request_for_tokenid(buffer, class, CLASS_TOKEN_WRITE, tokenid, value);
    }

Just an idea. Do you think that it could be useful in second patch?

> > +{
> > +	struct calling_interface_token *token;
> > +
> > +	token = dell_smbios_find_token(type);
> > +	if (!token)
> > +		return -ENODEV;
> > +
> > +	/* -1 is a sentinel value, telling us to use token->value */
> > +	if (val == (u32)-1)
> > +		val = token->value;
> > +
> > +	dell_fill_request(buffer, token->location, val, 0, 0);
> > +	return dell_send_request(buffer, class, select);
> > +}
> > +
> > +static int dell_battery_set_mode(const int type)
> > +{
> > +	struct calling_interface_buffer buffer;
> > +
> > +	/* -1 means use the value from the token */
> > +	return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE,
> > +			SELECT_TOKEN_STD, type, -1);
> > +}
> > +
> > +static int dell_battery_read(const int type)
> > +{
> > +	struct calling_interface_buffer buffer;
> > +	int err;
> > +
> > +	err = dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_READ,
> > +			SELECT_TOKEN_STD, type, 0);
> > +	if (err)
> > +		return err;
> > +
> > +	return buffer.output[1];
> 
> buffer.output[1] is of type u32. So theoretically it can contain value
> above 2^31. For safety it would be better to check if the output[1]
> value fits into INT_MAX and if not then return something like -ERANGE
> (or some other better errno code).
> 
> > +}
> > +
> > +static bool dell_battery_mode_is_active(const int type)
> > +{
> > +	struct calling_interface_token *token;
> > +
> > +	token = dell_smbios_find_token(type);
> > +	if (!token)
> > +		return false;
> 
> This check is redundant and can be dropped.
> 
> dell_battery_read() returns -ENODEV when token (type) does not exist.
> So you could treat negative return value from dell_battery_read() as
> "return false".
> 
> > +
> > +	return token->value == dell_battery_read(type);
> 
> token->value is of unsigned u16 type and dell_battery_read() returns
> signed int. So maybe some explicit cast could useful for documentation
> purposes.
> 
> > +}
> > +
> > +/* The rules: the minimum start charging value is 50%. The maximum
> 
> nit: Multiline non-network comment starts with "/*" on separate line and
> comment text continue on next line (not the same line).
> 
> > + * 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;
> > +
> > +	end = dell_battery_read(BAT_CUSTOM_CHARGE_END);
> > +	/* a failed read is okay */
> 
> Why is failed read okay? It sounds strange if we ignore firmware errors.
> I think that if reading the custom charge value is failing we should not
> continue and trying to set/change custom charge value.
> 
> Also if the returned value is above 100 (%), should be continue?
> 
> > +	if (end < 0)
> > +		end = CHARGE_END_MAX;
> > +	if ((end - start) < CHARGE_MIN_DIFF)
> 
> nit: I'm not sure what is the correct coding style for kernel drivers
> but I thought that parenthesis around (end - start) are not being
> written.
> 
> > +		start = end - CHARGE_MIN_DIFF;
> > +
> > +	return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE,
> > +			SELECT_TOKEN_STD, 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;
> > +
> > +	start = dell_battery_read(BAT_CUSTOM_CHARGE_START);
> > +	/* a failed read is okay */
> > +	if (start < 0)
> > +		start = CHARGE_START_MIN;
> > +	if ((end - start) < CHARGE_MIN_DIFF)
> > +		end = start + CHARGE_MIN_DIFF;
> > +
> > +	return dell_send_request_by_token_loc(&buffer, CLASS_TOKEN_WRITE,
> > +			SELECT_TOKEN_STD, 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)))
> > +			continue;
> > +
> > +		active = dell_battery_mode_is_active(battery_modes[i].token);
> > +		count += sysfs_emit_at(buf, count, active ? "[%s] " : "%s ",
> > +				battery_modes[i].label);
> > +	}
> > +
> > +	/* convert the last space to a newline */
> > +	if (count > 0)
> > +		count--;
> > +	count += sysfs_emit_at(buf, count, "\n");
> > +
> > +	return count;
> > +}
> > +
> > +static ssize_t charge_type_store(struct device *dev,
> > +		struct device_attribute *attr,
> > +		const char *buf, size_t size)
> > +{
> > +	bool matched = false;
> > +	int err, i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> 
> nit: Personally I would put the "if (!(battery_supported_modes & BIT(i)))"
> check here with continue, to have same pattern in _show and _store
> functions. And also if we want to support battery mode which will have
> different tokens on different machines (see below for possibility).
> 
> > +		if (sysfs_streq(battery_modes[i].label, buf)) {
> > +			matched = true;
> > +			break;
> > +		}
> > +	}
> > +	if (!matched || !(battery_supported_modes & BIT(i)))
> > +		return -EINVAL;
> > +
> > +	err = dell_battery_set_mode(battery_modes[i].token);
> > +	if (err)
> > +		return err;
> > +
> > +	return size;
> > +}
> > +
> > +static ssize_t charge_control_start_threshold_show(struct device *dev,
> > +		struct device_attribute *attr,
> > +		char *buf)
> > +{
> > +	int start;
> > +
> > +	start = dell_battery_read(BAT_CUSTOM_CHARGE_START);
> > +	if (start < 0)
> > +		return start;
> 
> IIRC the value is in percentage. So we should also check that returned
> value is not above 100 (and return some error in case).
> 
> > +
> > +	return sysfs_emit(buf, "%d\n", start);
> > +}
> > +
> > +static ssize_t charge_control_start_threshold_store(struct device *dev,
> > +		struct device_attribute *attr,
> > +		const char *buf, size_t size)
> > +{
> > +	int ret, start;
> > +
> > +	ret = kstrtoint(buf, 10, &start);
> > +	if (ret)
> > +		return ret;
> 
> I think that there should be some sanity validation. If format is
> percentage then we should not accept from userspace value outside of
> [0, 100] range.
> 
> > +
> > +	ret = dell_battery_set_custom_charge_start(start);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return size;
> > +}
> > +
> > +static ssize_t charge_control_end_threshold_show(struct device *dev,
> > +		struct device_attribute *attr,
> > +		char *buf)
> > +{
> > +	int end;
> > +
> > +	end = dell_battery_read(BAT_CUSTOM_CHARGE_END);
> > +	if (end < 0)
> > +		return end;
> > +
> > +	return sysfs_emit(buf, "%d\n", end);
> > +}
> > +
> > +static ssize_t charge_control_end_threshold_store(struct device *dev,
> > +		struct device_attribute *attr,
> > +		const char *buf, size_t size)
> > +{
> > +	int ret, end;
> > +
> > +	ret = kstrtouint(buf, 10, &end);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = dell_battery_set_custom_charge_end(end);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return size;
> > +}
> > +
> > +static DEVICE_ATTR_RW(charge_control_start_threshold);
> > +static DEVICE_ATTR_RW(charge_control_end_threshold);
> > +static DEVICE_ATTR_RW(charge_type);
> > +
> > +static struct attribute *dell_battery_attrs[] = {
> > +	&dev_attr_charge_control_start_threshold.attr,
> > +	&dev_attr_charge_control_end_threshold.attr,
> > +	&dev_attr_charge_type.attr,
> > +	NULL,
> > +};
> > +ATTRIBUTE_GROUPS(dell_battery);
> > +
> > +static int dell_battery_add(struct power_supply *battery,
> > +		struct acpi_battery_hook *hook)
> > +{
> > +	return device_add_groups(&battery->dev, dell_battery_groups);
> > +}
> > +
> > +static int dell_battery_remove(struct power_supply *battery,
> > +		struct acpi_battery_hook *hook)
> > +{
> > +	device_remove_groups(&battery->dev, dell_battery_groups);
> > +	return 0;
> > +}
> > +
> > +static struct acpi_battery_hook dell_battery_hook = {
> > +	.add_battery = dell_battery_add,
> > +	.remove_battery = dell_battery_remove,
> > +	.name = "Dell Primary Battery Extension",
> > +};
> > +
> > +static u32 __init battery_get_supported_modes(void)
> > +{
> > +	u32 modes = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(battery_modes); i++) {
> > +		if (dell_smbios_find_token(battery_modes[i].token))
> > +			modes |= BIT(i);
> > +	}
> > +
> > +	return modes;
> > +}
> > +
> > +static void __init dell_battery_init(struct device *dev)
> > +{
> > +	battery_supported_modes = battery_get_supported_modes();
> > +
> > +	if (battery_supported_modes != 0)
> > +		battery_hook_register(&dell_battery_hook);
> 
> Anyway, how is this battery_hook_register() suppose to work on systems
> with multiple batteries? The provided API is only for the primary
> battery, but on older Dell laptop it was possible to connect up to
> 3 batteries. Would not this case some issues?
> 
> I have one Dell laptop which has an option to connect secondary
> battery, but I do not have the compatible secondary battery to test it.
> 
> Has somebody Dell laptop with 2 or 3 batteries? It would be really good
> to test this patch how it would behave...
> 
> > +}
> > +
> > +static void __exit dell_battery_exit(void)
> > +{
> > +	if (battery_supported_modes != 0)
> > +		battery_hook_unregister(&dell_battery_hook);
> > +}
> > +
> >  static int __init dell_init(void)
> >  {
> >  	struct calling_interface_token *token;
> > @@ -2219,6 +2504,7 @@ static int __init dell_init(void)
> >  		touchpad_led_init(&platform_device->dev);
> >  
> >  	kbd_led_init(&platform_device->dev);
> > +	dell_battery_init(&platform_device->dev);
> >  
> >  	dell_laptop_dir = debugfs_create_dir("dell_laptop", NULL);
> >  	debugfs_create_file("rfkill", 0444, dell_laptop_dir, NULL,
> > @@ -2293,6 +2579,7 @@ static int __init dell_init(void)
> >  	if (mute_led_registered)
> >  		led_classdev_unregister(&mute_led_cdev);
> >  fail_led:
> > +	dell_battery_exit();
> >  	dell_cleanup_rfkill();
> >  fail_rfkill:
> >  	platform_device_del(platform_device);
> > @@ -2311,6 +2598,7 @@ static void __exit dell_exit(void)
> >  	if (quirks && quirks->touchpad_led)
> >  		touchpad_led_exit();
> >  	kbd_led_exit();
> > +	dell_battery_exit();
> >  	backlight_device_unregister(dell_backlight_device);
> >  	if (micmute_led_registered)
> >  		led_classdev_unregister(&micmute_led_cdev);
> > diff --git a/drivers/platform/x86/dell/dell-smbios.h b/drivers/platform/x86/dell/dell-smbios.h
> > index ea0cc38642a2..77baa15eb523 100644
> > --- a/drivers/platform/x86/dell/dell-smbios.h
> > +++ b/drivers/platform/x86/dell/dell-smbios.h
> > @@ -33,6 +33,13 @@
> >  #define KBD_LED_AUTO_50_TOKEN	0x02EB
> >  #define KBD_LED_AUTO_75_TOKEN	0x02EC
> >  #define KBD_LED_AUTO_100_TOKEN	0x02F6
> > +#define BAT_PRI_AC_MODE_TOKEN	0x0341
> > +#define BAT_ADAPTIVE_MODE_TOKEN	0x0342
> > +#define BAT_CUSTOM_MODE_TOKEN	0x0343
> > +#define BAT_STANDARD_MODE_TOKEN	0x0346
> > +#define BAT_EXPRESS_MODE_TOKEN	0x0347
> 
> There is defined also tokenid 0x0348 which is per dell libsmbios's
> token_list.csv documentation also for custom mode. It is marked as
> deprecated, so seems that it is for old laptops, which do not support
> tokenid 0x0343.
> 
> What about defining '#define BAT_CUSTOM_OLD_MODE_TOKEN 0x0348', then
> adding '{ BAT_CUSTOM_OLD_MODE_TOKEN, "custom" }' into battery_modes and
> changing battery_get_supported_modes() to mask out custom_old bit when
> the normal custom is supported? Seems that it would be OK, just the
> charge_type_store() needs modification as written above.
> 
> Just an idea. What do you think?
> 
> > +#define BAT_CUSTOM_CHARGE_START	0x0349
> > +#define BAT_CUSTOM_CHARGE_END	0x034A
> >  #define GLOBAL_MIC_MUTE_ENABLE	0x0364
> >  #define GLOBAL_MIC_MUTE_DISABLE	0x0365
> >  #define GLOBAL_MUTE_ENABLE	0x058C
> > -- 
> > 2.39.2
> > 
> > 
> > 
> > 
> > -- 
> > I'm available for contract & employment work, see:
> > https://spindle.queued.net/~dilinger/resume-tech.pdf

Powered by blists - more mailing lists