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

Commit Message

Andres Salomon Aug. 15, 2024, 11:28 p.m. UTC

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 <[e-mail]> and
Limonciello Mario <[e-mail]> submitted back in 2020:
https://lore.kernel.org/all/[e-mail]/
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 <[e-mail]>
---
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 <[e-mail]>:
    - 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

Comments

kernel test robot Aug. 16, 2024, 7:27 a.m. UTC | #1

Hi Andres,

kernel test robot noticed the following build warnings:

[auto build test WARNING on sre-power-supply/for-next]
[also build test WARNING on linus/master v6.11-rc3 next-20240816]
[cannot apply to amd-pstate/linux-next amd-pstate/bleeding-edge]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andres-Salomon/platform-x86-dell-laptop-remove-duplicate-code-w-battery-function/20240816-102156
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
patch link:    https://lore.kernel.org/r/20240815192848.3489d3e1%405400
patch subject: [PATCH v3 1/2] platform/x86:dell-laptop: Add knobs to change  battery charge settings
reproduce: (https://download.01.org/0day-ci/archive/20240816/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All warnings (new ones prefixed by >>):

   Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
   Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/reserved-memory/qcom
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/display/exynos/
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
>> 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
   Using alabaster theme

Ilpo Järvinen Aug. 16, 2024, 1:56 p.m. UTC | #2

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 <[e-mail]> and
> Limonciello Mario <[e-mail]> submitted back in 2020:
> https://lore.kernel.org/all/[e-mail]/
> 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 <[e-mail]>
> ---
> 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 <[e-mail]>:
>     - 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:	[e-mail]
> +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).

> +	{ 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.

> +};
> +static u32 battery_supported_modes;
> +
>  module_param(force_rfkill, bool, 0444);
>  MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
>  
> @@ -353,6 +367,32 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  	{ }
>  };
>  
> +/* -1 is a sentinel value, telling us to use token->value */
> +#define USE_TVAL ((u32) -1)
> +static int dell_send_request_for_tokenid(struct calling_interface_buffer *buffer,
> +					 u16 class, u16 select, u16 tokenid,
> +					 u32 val)
> +{
> +	struct calling_interface_token *token;
> +
> +	token = dell_smbios_find_token(tokenid);
> +	if (!token)
> +		return -ENODEV;
> +
> +	if (val == USE_TVAL)
> +		val = token->value;
> +
> +	dell_fill_request(buffer, token->location, val, 0, 0);
> +	return dell_send_request(buffer, class, select);
> +}
> +
> +static inline int dell_set_std_token_value(struct calling_interface_buffer *buffer,
> +		u16 tokenid, u32 value)
> +{
> +	return dell_send_request_for_tokenid(buffer, CLASS_TOKEN_WRITE,
> +			SELECT_TOKEN_STD, tokenid, value);
> +}
> +
>  /*
>   * Derived from information in smbios-wireless-ctl:
>   *
> @@ -2183,6 +2223,279 @@ static struct led_classdev mute_led_cdev = {
>  	.default_trigger = "audio-mute",
>  };
>  
> +static int dell_battery_set_mode(const u16 tokenid)
> +{
> +	struct calling_interface_buffer buffer;
> +
> +	return dell_set_std_token_value(&buffer, tokenid, USE_TVAL);
> +}
> +
> +static int dell_battery_read(const u16 tokenid)
> +{
> +	struct calling_interface_buffer buffer;
> +	int err;
> +
> +	err = dell_send_request_for_tokenid(&buffer, CLASS_TOKEN_READ,
> +			SELECT_TOKEN_STD, tokenid, 0);
> +	if (err)
> +		return err;
> +
> +	if (buffer.output[1] > INT_MAX)
> +		return -EIO;
> +
> +	return buffer.output[1];
> +}
> +
> +static bool dell_battery_mode_is_active(const u16 tokenid)
> +{
> +	struct calling_interface_token *token;
> +	int ret;
> +
> +	ret = dell_battery_read(tokenid);
> +	if (ret < 0)
> +		return false;
> +
> +	token = dell_smbios_find_token(tokenid);
> +	/* token's already verified by dell_battery_read() */
> +
> +	return token->value == (u16) ret;
> +}
> +
> +/*
> + * 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().

> +
> +	end = dell_battery_read(BAT_CUSTOM_CHARGE_END);
> +	if (end < 0)
> +		return end;
> +	if ((end - start) < CHARGE_MIN_DIFF)

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.

> +	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?
What's the benefit of obfuscation it with the extra variable & BIT()?

Pali Rohár Aug. 16, 2024, 4:33 p.m. UTC | #3

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 <[e-mail]> and
> > Limonciello Mario <[e-mail]> submitted back in 2020:
> > https://lore.kernel.org/all/[e-mail]/
> > 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 <[e-mail]>
> > ---
> > 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 <[e-mail]>:
> >     - 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:	[e-mail]
> > +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.

> > +	{ 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).

> > +};
> > +static u32 battery_supported_modes;
> > +
> >  module_param(force_rfkill, bool, 0444);
> >  MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
> >  
> > @@ -353,6 +367,32 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
> >  	{ }
> >  };
> >  
> > +/* -1 is a sentinel value, telling us to use token->value */
> > +#define USE_TVAL ((u32) -1)
> > +static int dell_send_request_for_tokenid(struct calling_interface_buffer *buffer,
> > +					 u16 class, u16 select, u16 tokenid,
> > +					 u32 val)
> > +{
> > +	struct calling_interface_token *token;
> > +
> > +	token = dell_smbios_find_token(tokenid);
> > +	if (!token)
> > +		return -ENODEV;
> > +
> > +	if (val == USE_TVAL)
> > +		val = token->value;
> > +
> > +	dell_fill_request(buffer, token->location, val, 0, 0);
> > +	return dell_send_request(buffer, class, select);
> > +}
> > +
> > +static inline int dell_set_std_token_value(struct calling_interface_buffer *buffer,
> > +		u16 tokenid, u32 value)
> > +{
> > +	return dell_send_request_for_tokenid(buffer, CLASS_TOKEN_WRITE,
> > +			SELECT_TOKEN_STD, tokenid, value);
> > +}
> > +
> >  /*
> >   * Derived from information in smbios-wireless-ctl:
> >   *
> > @@ -2183,6 +2223,279 @@ static struct led_classdev mute_led_cdev = {
> >  	.default_trigger = "audio-mute",
> >  };
> >  
> > +static int dell_battery_set_mode(const u16 tokenid)
> > +{
> > +	struct calling_interface_buffer buffer;
> > +
> > +	return dell_set_std_token_value(&buffer, tokenid, USE_TVAL);
> > +}
> > +
> > +static int dell_battery_read(const u16 tokenid)
> > +{
> > +	struct calling_interface_buffer buffer;
> > +	int err;
> > +
> > +	err = dell_send_request_for_tokenid(&buffer, CLASS_TOKEN_READ,
> > +			SELECT_TOKEN_STD, tokenid, 0);
> > +	if (err)
> > +		return err;
> > +
> > +	if (buffer.output[1] > INT_MAX)
> > +		return -EIO;
> > +
> > +	return buffer.output[1];
> > +}
> > +
> > +static bool dell_battery_mode_is_active(const u16 tokenid)
> > +{
> > +	struct calling_interface_token *token;
> > +	int ret;
> > +
> > +	ret = dell_battery_read(tokenid);
> > +	if (ret < 0)
> > +		return false;
> > +
> > +	token = dell_smbios_find_token(tokenid);
> > +	/* token's already verified by dell_battery_read() */
> > +
> > +	return token->value == (u16) ret;
> > +}
> > +
> > +/*
> > + * 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().
> 
> > +
> > +	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.
> 
> > +	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.

> 
> -- 
>  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++) {
> > +		if (!(battery_supported_modes & BIT(i)))
> > +			continue;
> > +
> > +		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;
> > +
> > +	if (start > CHARGE_START_MAX)
> > +		return -EIO;
> > +
> > +	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;
> > +	if (start < 0 || start > 100)
> > +		return -EINVAL;
> > +
> > +	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;
> > +
> > +	if (end > CHARGE_END_MAX)
> > +		return -EIO;
> > +
> > +	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;
> > +	if (end < 0 || end > 100)
> > +		return -EINVAL;
> > +
> > +	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)
> > +{
> > +	/* this currently only supports the primary battery */
> > +	if (strcmp(battery->desc->name, "BAT0") != 0)
> > +		return -ENODEV;
> > +
> > +	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);
> > +}
> > +
> > +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 +2532,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 +2607,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 +2626,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
> > +#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
> >

Pali Rohár Aug. 16, 2024, 5:16 p.m. UTC | #4

Hello Andres,

I have just comments for this change. It looks good. After resolving
them you can add my tag: Reviewed-by: Pali Rohár <[e-mail]>

On Thursday 15 August 2024 19:28:48 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 <[e-mail]> and
> Limonciello Mario <[e-mail]> submitted back in 2020:

I think that the information below is not needed to have in commit
message. We also do not include links or information about previous
version of patch sent by you.

> https://lore.kernel.org/all/[e-mail]/
> 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.

...

> +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++) {
> +		if (!(battery_supported_modes & BIT(i)))
> +			continue;
> +
> +		if (sysfs_streq(battery_modes[i].label, buf)) {
> +			matched = true;
> +			break;
> +		}
> +	}
> +	if (!matched || !(battery_supported_modes & BIT(i)))
> +		return -EINVAL;

Check for "!(battery_supported_modes & BIT(i))" is redundant here.
"matched" can be true only in case if the i-th mode is supported and was
specified in buffer.

> +
> +	err = dell_battery_set_mode(battery_modes[i].token);
> +	if (err)
> +		return err;
> +
> +	return size;
> +}

Andres Salomon Aug. 16, 2024, 6:52 p.m. UTC | #5

On Fri, 16 Aug 2024 18:33:41 +0200
Pali Rohár <[e-mail]> 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 <[e-mail]> and
> > > Limonciello Mario <[e-mail]> submitted back in 2020:
> > > https://lore.kernel.org/all/[e-mail]/
> > > 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 <[e-mail]>
> > > ---
> > > 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 <[e-mail]>:
> > >     - 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:	[e-mail]
> > > +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.

kernel test robot Aug. 18, 2024, 1:36 a.m. UTC | #6

Hi Andres,

kernel test robot noticed the following build warnings:

[auto build test WARNING on sre-power-supply/for-next]
[also build test WARNING on linus/master v6.11-rc3 next-20240816]
[cannot apply to amd-pstate/linux-next amd-pstate/bleeding-edge]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andres-Salomon/platform-x86-dell-laptop-remove-duplicate-code-w-battery-function/20240816-102156
base:   https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git for-next
patch link:    https://lore.kernel.org/r/20240815192848.3489d3e1%405400
patch subject: [PATCH v3 1/2] platform/x86:dell-laptop: Add knobs to change  battery charge settings
config: x86_64-randconfig-122-20240817 (https://download.01.org/0day-ci/archive/20240818/[e-mail]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240818/[e-mail]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[e-mail]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All warnings (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in arch/x86/mm/testmmiotrace.o
WARNING: modpost: missing MODULE_DESCRIPTION() in kernel/locking/test-ww_mutex.o
WARNING: modpost: missing MODULE_DESCRIPTION() in mm/kasan/kasan_test.o
>> WARNING: modpost: drivers/platform/x86/dell/dell-laptop: section mismatch in reference: dell_init+0x637 (section: .init.text) -> dell_battery_exit (section: .exit.text)
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_userspace.o

Ilpo Järvinen Aug. 19, 2024, 11:03 a.m. UTC | #7

On Fri, 16 Aug 2024, Pali Rohár 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 <[e-mail]> and
> > > Limonciello Mario <[e-mail]> submitted back in 2020:
> > > https://lore.kernel.org/all/[e-mail]/
> > > 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 <[e-mail]>
> > > ---
> > > 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 <[e-mail]>:
> > >     - 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:	[e-mail]
> > > +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.

As is, this splits "static const" from battery_modes. Naming anonymous 
struct isn't as bad problem as you seem to suggest (and named structs 
are also easier when grepping).

> > > +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.

If you refer to 'triggers', it is definitely not an example of something 
that is easy to follow. With triggers, it seems that there's undocumented 
array-index to hw-field bit mapping going on (or to be more precise, the 
documentation for the hw field is in a far-away documentation block so
it's practically impossible to make the connection when reading the 
struct).

> > 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.

Err, it's proposed to be const by this patch. Why that struct cannot be 
changed to be non-const? Clearly there is battery mode related data that 
is not const.


As a sidenote now that I had to look, this driver is doing other BIT(n) 
obfusction too (and open-coded FIELD_PREP() seem to be plenty as well).

--
 i.

Hans de Goede Aug. 19, 2024, 1:59 p.m. UTC | #8

Hi,

On 8/16/24 1:28 AM, 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 <[e-mail]> and
> Limonciello Mario <[e-mail]> submitted back in 2020:
> https://lore.kernel.org/all/[e-mail]/
> 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 <[e-mail]>

Thank you for your patch, some small comments below. For the next
version please also address Pali's and Ilpo's remarks.

> ---
> 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 <[e-mail]>:
>     - 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:	[e-mail]
> +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 ...".


> 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:

struct battery_mode_info {
	int token;
	const char *label;
};

static const struct battery_mode_info 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" },
};

Also whatever option you chose please align the second column using
spaces as done above.

<snip>

> +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);
> +}
> +
> +static void __exit dell_battery_exit(void)
> +{
> +	if (battery_supported_modes != 0)
> +		battery_hook_unregister(&dell_battery_hook);
> +}
> +

You cannot mark this __exit and also call it from the __init
dell_init() function to cleanup on errors, this lead to
the following error reported by the kernel test robot:

WARNING: modpost: drivers/platform/x86/dell/dell-laptop: section mismatch in reference: dell_init+0x637 (section: .init.text) -> dell_battery_exit (section: .exit.text)

to fix this please drop the __exit marking from this function.

Regards,

Hans

Andres Salomon Aug. 24, 2024, 2:39 a.m. UTC | #9

On Mon, 19 Aug 2024 15:59:45 +0200
Hans de Goede <[e-mail]> 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 <[e-mail]>:
> >     - 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:	[e-mail]
> > +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!

Hans de Goede Aug. 24, 2024, 12:35 p.m. UTC | #10

Hi,

On 8/24/24 4:39 AM, Andres Salomon wrote:
> On Mon, 19 Aug 2024 15:59:45 +0200
> Hans de Goede <[e-mail]> 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 <[e-mail]>:
>>>     - 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:	[e-mail]
>>> +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/

Yes that is what I head in mind, thank you for doing this.

I'll try to review that patch soon-ish.

I'll review and likely merge your new v4 "Add knobs" patch on Monday.

Regards,

Hans

Patch

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:	[e-mail]
+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[] = {
+	{ 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" },
+};
+static u32 battery_supported_modes;
+
 module_param(force_rfkill, bool, 0444);
 MODULE_PARM_DESC(force_rfkill, "enable rfkill on non whitelisted models");
 
@@ -353,6 +367,32 @@  static const struct dmi_system_id dell_quirks[] __initconst = {
 	{ }
 };
 
+/* -1 is a sentinel value, telling us to use token->value */
+#define USE_TVAL ((u32) -1)
+static int dell_send_request_for_tokenid(struct calling_interface_buffer *buffer,
+					 u16 class, u16 select, u16 tokenid,
+					 u32 val)
+{
+	struct calling_interface_token *token;
+
+	token = dell_smbios_find_token(tokenid);
+	if (!token)
+		return -ENODEV;
+
+	if (val == USE_TVAL)
+		val = token->value;
+
+	dell_fill_request(buffer, token->location, val, 0, 0);
+	return dell_send_request(buffer, class, select);
+}
+
+static inline int dell_set_std_token_value(struct calling_interface_buffer *buffer,
+		u16 tokenid, u32 value)
+{
+	return dell_send_request_for_tokenid(buffer, CLASS_TOKEN_WRITE,
+			SELECT_TOKEN_STD, tokenid, value);
+}
+
 /*
  * Derived from information in smbios-wireless-ctl:
  *
@@ -2183,6 +2223,279 @@  static struct led_classdev mute_led_cdev = {
 	.default_trigger = "audio-mute",
 };
 
+static int dell_battery_set_mode(const u16 tokenid)
+{
+	struct calling_interface_buffer buffer;
+
+	return dell_set_std_token_value(&buffer, tokenid, USE_TVAL);
+}
+
+static int dell_battery_read(const u16 tokenid)
+{
+	struct calling_interface_buffer buffer;
+	int err;
+
+	err = dell_send_request_for_tokenid(&buffer, CLASS_TOKEN_READ,
+			SELECT_TOKEN_STD, tokenid, 0);
+	if (err)
+		return err;
+
+	if (buffer.output[1] > INT_MAX)
+		return -EIO;
+
+	return buffer.output[1];
+}
+
+static bool dell_battery_mode_is_active(const u16 tokenid)
+{
+	struct calling_interface_token *token;
+	int ret;
+
+	ret = dell_battery_read(tokenid);
+	if (ret < 0)
+		return false;
+
+	token = dell_smbios_find_token(tokenid);
+	/* token's already verified by dell_battery_read() */
+
+	return token->value == (u16) ret;
+}
+
+/*
+ * 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;
+
+	end = dell_battery_read(BAT_CUSTOM_CHARGE_END);
+	if (end < 0)
+		return end;
+	if ((end - start) < CHARGE_MIN_DIFF)
+		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;
+
+	start = dell_battery_read(BAT_CUSTOM_CHARGE_START);
+	if (start < 0)
+		return start;
+	if ((end - start) < CHARGE_MIN_DIFF)
+		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)))
+			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++) {
+		if (!(battery_supported_modes & BIT(i)))
+			continue;
+
+		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;
+
+	if (start > CHARGE_START_MAX)
+		return -EIO;
+
+	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;
+	if (start < 0 || start > 100)
+		return -EINVAL;
+
+	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;
+
+	if (end > CHARGE_END_MAX)
+		return -EIO;
+
+	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;
+	if (end < 0 || end > 100)
+		return -EINVAL;
+
+	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)
+{
+	/* this currently only supports the primary battery */
+	if (strcmp(battery->desc->name, "BAT0") != 0)
+		return -ENODEV;
+
+	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);
+}
+
+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 +2532,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 +2607,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 +2626,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
+#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