Message-ID: <db256dd0-7e0b-a03b-dff6-79da8f3dc8d5@linux.intel.com>
Date: Mon, 19 Aug 2024 14:03:27 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Pali Rohár <pali@...nel.org>
cc: Andres Salomon <dilinger@...ued.net>, LKML <linux-kernel@...r.kernel.org>, 
    platform-driver-x86@...r.kernel.org, Matthew Garrett <mjg59@...f.ucam.org>, 
    Sebastian Reichel <sre@...nel.org>, Hans de Goede <hdegoede@...hat.com>, 
    linux-pm@...r.kernel.org, Dell.Client.Kernel@...l.com
Subject: Re: [PATCH v3 1/2] platform/x86:dell-laptop: Add knobs to change
 battery charge settings

On Fri, 16 Aug 2024, 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 <perry_yuan@...l.com> and
> > > Limonciello Mario <Mario_Limonciello@...l.com> submitted back in 2020:
> > > https://lore.kernel.org/all/20200729065424.12851-1-Perry_Yuan@Dell.com/
> > > Both of their email addresses bounce, so I'm assuming they're no longer
> > > with the company. I've reworked most of the patch to make it smaller and
> > > cleaner.
> > > 
> > > Signed-off-by: Andres Salomon <dilinger@...ued.net>
> > > ---
> > > Changes in v3:
> > >     - switch tokenid and class types
> > >     - be stricter with results from both userspace and BIOS
> > >     - no longer allow failed BIOS reads
> > >     - rename/move dell_send_request_by_token_loc, and add helper function
> > >     - only allow registration for BAT0
> > >     - rename charge_type modes to match power_supply names
> > > Changes in v2, based on extensive feedback from Pali Rohár <pali@...nel.org>:
> > >     - code style changes
> > >     - change battery write API to use token->value instead of passed value
> > >     - stop caching current mode, instead querying SMBIOS as needed
> > >     - drop the separate list of charging modes enum
> > >     - rework the list of charging mode strings
> > >     - query SMBIOS for supported charging modes
> > >     - split dell_battery_custom_set() up
> > > ---
> > >  .../ABI/testing/sysfs-class-power-dell        |  33 ++
> > >  drivers/platform/x86/dell/Kconfig             |   1 +
> > >  drivers/platform/x86/dell/dell-laptop.c       | 316 ++++++++++++++++++
> > >  drivers/platform/x86/dell/dell-smbios.h       |   7 +
> > >  4 files changed, 357 insertions(+)
> > >  create mode 100644 Documentation/ABI/testing/sysfs-class-power-dell
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-class-power-dell b/Documentation/ABI/testing/sysfs-class-power-dell
> > > new file mode 100644
> > > index 000000000000..d8c542177558
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-class-power-dell
> > > @@ -0,0 +1,33 @@
> > > +What:		/sys/class/power_supply/<supply_name>/charge_type
> > > +Date:		August 2024
> > > +KernelVersion:	6.12
> > > +Contact:	linux-pm@...r.kernel.org
> > > +Description:
> > > +		Select the charging algorithm to use for the (primary)
> > > +		battery.
> > > +
> > > +		Standard:
> > > +			Fully charge the battery at a moderate rate.
> > > +		Fast:
> > > +			Quickly charge the battery using fast-charge
> > > +			technology. This is harder on the battery than
> > > +			standard charging and may lower its lifespan.
> > > +			The Dell BIOS calls this ExpressCharge™.
> > > +		Trickle:
> > > +			Users who primarily operate the system while
> > > +			plugged into an external power source can extend
> > > +			battery life with this mode. The Dell BIOS calls
> > > +			this "Primarily AC Use".
> > > +		Adaptive:
> > > +			Automatically optimize battery charge rate based
> > > +			on typical usage pattern.
> > > +		Custom:
> > > +			Use the charge_control_* properties to determine
> > > +			when to start and stop charging. Advanced users
> > > +			can use this to drastically extend battery life.
> > > +
> > > +		Access: Read, Write
> > > +		Valid values:
> > > +			      "Standard", "Fast", "Trickle",
> > > +			      "Adaptive", "Custom"
> > > +
> > > diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
> > > index 85a78ef91182..02405793163c 100644
> > > --- a/drivers/platform/x86/dell/Kconfig
> > > +++ b/drivers/platform/x86/dell/Kconfig
> > > @@ -49,6 +49,7 @@ config DELL_LAPTOP
> > >  	default m
> > >  	depends on DMI
> > >  	depends on BACKLIGHT_CLASS_DEVICE
> > > +	depends on ACPI_BATTERY
> > >  	depends on ACPI_VIDEO || ACPI_VIDEO = n
> > >  	depends on RFKILL || RFKILL = n
> > >  	depends on DELL_WMI || DELL_WMI = n
> > > diff --git a/drivers/platform/x86/dell/dell-laptop.c b/drivers/platform/x86/dell/dell-laptop.c
> > > index 6552dfe491c6..8cc05f0fab91 100644
> > > --- a/drivers/platform/x86/dell/dell-laptop.c
> > > +++ b/drivers/platform/x86/dell/dell-laptop.c
> > > @@ -22,11 +22,13 @@
> > >  #include <linux/io.h>
> > >  #include <linux/rfkill.h>
> > >  #include <linux/power_supply.h>
> > > +#include <linux/sysfs.h>
> > >  #include <linux/acpi.h>
> > >  #include <linux/mm.h>
> > >  #include <linux/i8042.h>
> > >  #include <linux/debugfs.h>
> > >  #include <linux/seq_file.h>
> > > +#include <acpi/battery.h>
> > >  #include <acpi/video.h>
> > >  #include "dell-rbtn.h"
> > >  #include "dell-smbios.h"
> > > @@ -99,6 +101,18 @@ static bool force_rfkill;
> > >  static bool micmute_led_registered;
> > >  static bool mute_led_registered;
> > >  
> > > +static const struct {
> > > +	int token;
> > > +	const char *label;
> > > +} battery_modes[] = {
> > 
> > Please don't try to do this in one go but split it into two (define and 
> > then declaration of the variable).
> 
> Why? Splitting definition of this anonymous structure and definition of
> variable would leak definition of anonymous structure of out the scope
> where it is used.

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.

Powered by blists - more mailing lists