[PATCH 0/3] platform/x86: Battery charge mode in toshiba_acpi

[PATCH 0/3] platform/x86: Battery charge mode in toshiba_acpi

[Arvid Norlander]

This is an improved version of the battery charge control for Toshiba
Satellite Z830. The full background is available in the two emails linked
below, but a short summary will follow, including only what is relevant
for battery charge control.


Background (from link 1)
==========

The Toshiba Satellite/Portege Z830 supports not charging the battery fully
in order to prolong battery life. Unlike for example ThinkPads where this
control is granular here it is just off/on. When off it charges to 100%.
When on it charges to about 80%.

According to the Windows program used to control the feature the setting
will not take effect until the battery has been discharged to around 50%.
However, in my testing it takes effect as soon as the charge drops below
80%. On Windows Toshiba branded this feature as "Eco charging"

In the following example ACPI calls I will use the following newly defined
constants:
#define HCI_BATTERY_CHARGE_MODE 0xba
#define BATTERY_CHARGE_FULL 0
#define BATTERY_CHARGE_80_PERCENT 1

To set the feature:
  {HCI_SET, HCI_BATTERY_CHARGE_MODE, charge_mode, 0, 0, 0}
To query for the existence of the feature:
  {HCI_GET, HCI_BATTERY_CHARGE_MODE, 0, 0, 0, 0}
To read the feature:
  {HCI_GET, HCI_BATTERY_CHARGE_MODE, 0, 0, 0, 1}

The read may need to be retried if TOS_DATA_NOT_AVAILABLE is returned as
the status code. This rarely happens (I have never observed it on Linux),
but I have seen it happen under Windows once, and the software did retry
it.


Improvements
============

As discussed in link 2 & 3 below, the original approach was suboptimal.

This patch series instead consists of two patches.

The first patch implements detecting the feature as well as internal
getter/setter methods.

The second patch adds battery hooks (heavily based on the code for this in
thinkpad_acpi) which creates the standard charge_control_end_threshold file
under /sys/class/power_supply/BAT1.

Side note: There is no BAT0 on this Toshiba, I'm not sure why the numbering
ends up starting from 1 instead of 0 here. This differs from my Thinkpads,
where the numbering starts from 0, with BAT1 being the second battery.
However, I haven't spent much effort investigating this, as it did not seem
important.

Patch 3 updates the ABI test documentation as suggested by Hans de Goede.
Note that only the charge_control_end_threshold is updated, as this is the
only limit supported by the Toshiba Z830. Possibly
charge_control_start_threshold should also be updated similarly, or would
it be better to wait for an actual example of this in the wild first?

Link (1): https://www.spinics.net/lists/platform-driver-x86/msg34314.html
Link (2): https://www.spinics.net/lists/platform-driver-x86/msg34354.html
Link (3): https://www.spinics.net/lists/platform-driver-x86/msg34320.html

Arvid Norlander (3):
  platform/x86: Battery charge mode in toshiba_acpi (internals)
  platform/x86: Battery charge mode in toshiba_acpi (sysfs)
  docs: ABI: charge_control_end_threshold may not support all values

 Documentation/ABI/testing/sysfs-class-power |   5 +-
 drivers/platform/x86/toshiba_acpi.c         | 162 ++++++++++++++++++++
 2 files changed, 166 insertions(+), 1 deletion(-)


base-commit: 1c23f9e627a7b412978b4e852793c5e3c3efc555
-- 
2.37.2

[PATCH 1/3] platform/x86: Battery charge mode in toshiba_acpi (internals)

[Arvid Norlander]

This commit adds the internal functions to control the Toshiba laptop.

Unlike for example ThinkPads where this control is granular here it is
just off/on. When off it charges to 100%. When on it charges to about 80%.

Controlling this setting is done via HCI register 0x00ba. Setting to value
1 will result in limiting the charing to 80% of the battery capacity,
while setting it to 0 will allow charging to 100%.

Reading the current state is a bit weird, and needs a 1 set in the last
position of the query for whatever reason. In addition, the read may
return 0x8d20 (Data not available) rarely, so a retry mechanism is needed.

According to the Windows program used to control the feature the setting
will not take effect until the battery has been discharged to around 50%.
However, in my testing it takes effect as soon as the charge drops below
80%. On Windows Toshiba branded this feature as "Eco charging".

Signed-off-by: Arvid Norlander <lkml@vorpal.se>
---
 drivers/platform/x86/toshiba_acpi.c | 69 +++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index 0fc9e8b8827b..c927d5d0f8cd 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -112,6 +112,7 @@ MODULE_LICENSE("GPL");
 #define HCI_KBD_ILLUMINATION		0x0095
 #define HCI_ECO_MODE			0x0097
 #define HCI_ACCELEROMETER2		0x00a6
+#define HCI_BATTERY_CHARGE_MODE		0x00ba
 #define HCI_SYSTEM_INFO			0xc000
 #define SCI_PANEL_POWER_ON		0x010d
 #define SCI_ILLUMINATION		0x014e
@@ -201,6 +202,7 @@ struct toshiba_acpi_dev {
 	unsigned int usb_three_supported:1;
 	unsigned int wwan_supported:1;
 	unsigned int cooling_method_supported:1;
+	unsigned int battery_charge_mode_supported:1;
 	unsigned int sysfs_created:1;
 	unsigned int special_functions;
 
@@ -1282,6 +1284,69 @@ static int toshiba_cooling_method_set(struct toshiba_acpi_dev *dev, u32 state)
 	return (result == TOS_SUCCESS || result == TOS_SUCCESS2) ? 0 : -EIO;
 }
 
+/* Battery charge control */
+static void toshiba_battery_charge_mode_available(struct toshiba_acpi_dev *dev)
+{
+	u32 in[TCI_WORDS] = { HCI_GET, HCI_BATTERY_CHARGE_MODE, 0, 0, 0, 0 };
+	u32 out[TCI_WORDS];
+	acpi_status status;
+
+	dev->battery_charge_mode_supported = 0;
+
+	status = tci_raw(dev, in, out);
+	if (ACPI_FAILURE(status)) {
+		pr_err("ACPI call to get Battery Charge Mode failed\n");
+		return;
+	}
+
+	if (out[0] != TOS_SUCCESS && out[0] != TOS_SUCCESS2)
+		return;
+
+	dev->battery_charge_mode_supported = 1;
+}
+
+static int toshiba_battery_charge_mode_get(struct toshiba_acpi_dev *dev, u32 *state)
+{
+	u32 in[TCI_WORDS] = { HCI_GET, HCI_BATTERY_CHARGE_MODE, 0, 0, 0, 0x1 };
+	u32 out[TCI_WORDS];
+	int retries = 3;
+
+	do {
+		acpi_status status = tci_raw(dev, in, out);
+
+		if (ACPI_FAILURE(status))
+			pr_err("ACPI call to get Battery Charge Mode failed\n");
+		switch (out[0]) {
+		case TOS_SUCCESS:
+		case TOS_SUCCESS2:
+			*state = out[2];
+			return 0;
+		case TOS_NOT_SUPPORTED:
+			return -ENODEV;
+		case TOS_DATA_NOT_AVAILABLE:
+			retries--;
+			break;
+		default:
+			return -EIO;
+		}
+	} while (retries);
+
+	return -EIO;
+}
+
+static int toshiba_battery_charge_mode_set(struct toshiba_acpi_dev *dev, u32 state)
+{
+	u32 result = hci_write(dev, HCI_BATTERY_CHARGE_MODE, state);
+
+	if (result == TOS_FAILURE)
+		pr_err("ACPI call to set Battery Charge Mode failed\n");
+
+	if (result == TOS_NOT_SUPPORTED)
+		return -ENODEV;
+
+	return (result == TOS_SUCCESS || result == TOS_SUCCESS2) ? 0 : -EIO;
+}
+
 /* Transflective Backlight */
 static int get_tr_backlight_status(struct toshiba_acpi_dev *dev, u32 *status)
 {
@@ -2956,6 +3021,8 @@ static void print_supported_features(struct toshiba_acpi_dev *dev)
 		pr_cont(" wwan");
 	if (dev->cooling_method_supported)
 		pr_cont(" cooling-method");
+	if (dev->battery_charge_mode_supported)
+		pr_cont(" battery-charge-mode");
 
 	pr_cont("\n");
 }
@@ -3163,6 +3230,8 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 
 	toshiba_cooling_method_available(dev);
 
+	toshiba_battery_charge_mode_available(dev);
+
 	print_supported_features(dev);
 
 	ret = sysfs_create_group(&dev->acpi_dev->dev.kobj,
-- 
2.37.2

[PATCH 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)

[Arvid Norlander]

This commit adds the ACPI battery hook which in turns adds the sysfs
entries.

Because the Toshiba laptops only support two modes (eco or normal), which
in testing correspond to 80% and 100% we simply round to the nearest
possible level when set.

It is possible that Toshiba laptops other than the Z830 has different set
points for the charging. If so, a quirk table could be introduced in the
future for this. For now, assume that all laptops that support this feature
work the same way.

Tested on a Toshiba Satellite Z830.

Signed-off-by: Arvid Norlander <lkml@vorpal.se>
---
 drivers/platform/x86/toshiba_acpi.c | 93 +++++++++++++++++++++++++++++
 1 file changed, 93 insertions(+)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index c927d5d0f8cd..8e272b4336c6 100644
--- a/drivers/platform/x86/toshiba_acpi.c
+++ b/drivers/platform/x86/toshiba_acpi.c
@@ -44,6 +44,7 @@
 #include <linux/rfkill.h>
 #include <linux/iio/iio.h>
 #include <linux/toshiba.h>
+#include <acpi/battery.h>
 #include <acpi/video.h>
 
 MODULE_AUTHOR("John Belmonte");
@@ -2981,6 +2982,88 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
 	return 0;
 }
 
+
+/* ACPI battery hooking */
+static ssize_t charge_control_end_threshold_show(struct device *device,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	int state;
+	int status;
+
+	if (toshiba_acpi == NULL) {
+		pr_err("Toshiba ACPI object invalid\n");
+		return -ENODEV;
+	}
+
+	status = toshiba_battery_charge_mode_get(toshiba_acpi, &state);
+	if (state == 1)
+		return sprintf(buf, "80\n");
+	else
+		return sprintf(buf, "100\n");
+}
+
+static ssize_t charge_control_end_threshold_store(struct device *dev,
+						  struct device_attribute *attr,
+						  const char *buf,
+						  size_t count)
+{
+	u32 value;
+	int rval;
+
+	if (toshiba_acpi == NULL) {
+		pr_err("Toshiba ACPI object invalid\n");
+		return -ENODEV;
+	}
+
+	rval = kstrtou32(buf, 10, &value);
+	if (rval)
+		return rval;
+
+	if (value < 1 || value > 100)
+		return -EINVAL;
+	rval = toshiba_battery_charge_mode_set(toshiba_acpi,
+					       (value < 90) ? 1 : 0);
+	if (rval < 0)
+		return rval;
+	else
+		return count;
+}
+
+static DEVICE_ATTR_RW(charge_control_end_threshold);
+
+static struct attribute *toshiba_acpi_battery_attrs[] = {
+	&dev_attr_charge_control_end_threshold.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(toshiba_acpi_battery);
+
+static int toshiba_acpi_battery_add(struct power_supply *battery)
+{
+	if (toshiba_acpi == NULL) {
+		pr_err("Init order issue\n");
+		return -ENODEV;
+	}
+	if (!toshiba_acpi->battery_charge_mode_supported)
+		return -ENODEV;
+	if (device_add_groups(&battery->dev, toshiba_acpi_battery_groups))
+		return -ENODEV;
+	return 0;
+}
+
+static int toshiba_acpi_battery_remove(struct power_supply *battery)
+{
+	device_remove_groups(&battery->dev, toshiba_acpi_battery_groups);
+	return 0;
+}
+
+static struct acpi_battery_hook battery_hook = {
+	.add_battery = toshiba_acpi_battery_add,
+	.remove_battery = toshiba_acpi_battery_remove,
+	.name = "Toshiba Battery Extension",
+};
+
 static void print_supported_features(struct toshiba_acpi_dev *dev)
 {
 	pr_info("Supported laptop features:");
@@ -3063,6 +3146,9 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
 		rfkill_destroy(dev->wwan_rfk);
 	}
 
+	if (dev->battery_charge_mode_supported)
+		battery_hook_unregister(&battery_hook);
+
 	if (toshiba_acpi)
 		toshiba_acpi = NULL;
 
@@ -3246,6 +3332,13 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
 
 	toshiba_acpi = dev;
 
+	/*
+	 * As the battery hook relies on the static variable toshiba_acpi being
+	 * set, this must be done after toshiba_acpi is assigned.
+	 */
+	if (dev->battery_charge_mode_supported)
+		battery_hook_register(&battery_hook);
+
 	return 0;
 
 error:
-- 
2.37.2

[PATCH 3/3] docs: ABI: charge_control_end_threshold may not support all values

[Arvid Norlander]

Some laptops (for example Toshiba Satellite Z830) only supports some fixed
values. Allow for this and document the expected behaviour in such cases.

Wording suggested by Hans de Goede.

Signed-off-by: Arvid Norlander <lkml@vorpal.se>
---
 Documentation/ABI/testing/sysfs-class-power | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
index a9ce63cfbe87..e434fc523291 100644
--- a/Documentation/ABI/testing/sysfs-class-power
+++ b/Documentation/ABI/testing/sysfs-class-power
@@ -364,7 +364,10 @@ Date:		April 2019
 Contact:	linux-pm@vger.kernel.org
 Description:
 		Represents a battery percentage level, above which charging will
-		stop.
+		stop. Not all hardware is capable of setting this to an arbitrary
+		percentage. Drivers will round written values to the nearest
+		supported value. Reading back the value will show the actual
+		threshold set by the driver.
 
 		Access: Read, Write
 
-- 
2.37.2

Re: [PATCH 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)

[kernel test robot]

Hi Arvid,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 1c23f9e627a7b412978b4e852793c5e3c3efc555]

url:    https://github.com/intel-lab-lkp/linux/commits/Arvid-Norlander/platform-x86-Battery-charge-mode-in-toshiba_acpi/20220829-033110
base:   1c23f9e627a7b412978b4e852793c5e3c3efc555
config: x86_64-randconfig-a014-20220829 (https://download.01.org/0day-ci/archive/20220829/202208290605.gE9IGbxE-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/a85fa4a6b19ae082f6018a07da93db965fb5ba11
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Arvid-Norlander/platform-x86-Battery-charge-mode-in-toshiba_acpi/20220829-033110
        git checkout a85fa4a6b19ae082f6018a07da93db965fb5ba11
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/platform/x86/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/platform/x86/toshiba_acpi.c:2992:6: warning: variable 'status' set but not used [-Wunused-but-set-variable]
           int status;
               ^
   1 warning generated.


vim +/status +2992 drivers/platform/x86/toshiba_acpi.c

  2984	
  2985	
  2986	/* ACPI battery hooking */
  2987	static ssize_t charge_control_end_threshold_show(struct device *device,
  2988							 struct device_attribute *attr,
  2989							 char *buf)
  2990	{
  2991		int state;
> 2992		int status;
  2993	
  2994		if (toshiba_acpi == NULL) {
  2995			pr_err("Toshiba ACPI object invalid\n");
  2996			return -ENODEV;
  2997		}
  2998	
  2999		status = toshiba_battery_charge_mode_get(toshiba_acpi, &state);
  3000		if (state == 1)
  3001			return sprintf(buf, "80\n");
  3002		else
  3003			return sprintf(buf, "100\n");
  3004	}
  3005	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)

[kernel test robot]

Hi Arvid,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 1c23f9e627a7b412978b4e852793c5e3c3efc555]

url:    https://github.com/intel-lab-lkp/linux/commits/Arvid-Norlander/platform-x86-Battery-charge-mode-in-toshiba_acpi/20220829-033110
base:   1c23f9e627a7b412978b4e852793c5e3c3efc555
config: x86_64-randconfig-a002-20220829 (https://download.01.org/0day-ci/archive/20220829/202208290628.Er7oPvnk-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/a85fa4a6b19ae082f6018a07da93db965fb5ba11
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Arvid-Norlander/platform-x86-Battery-charge-mode-in-toshiba_acpi/20220829-033110
        git checkout a85fa4a6b19ae082f6018a07da93db965fb5ba11
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/platform/x86/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/platform/x86/toshiba_acpi.c: In function 'charge_control_end_threshold_show':
>> drivers/platform/x86/toshiba_acpi.c:2992:13: warning: variable 'status' set but not used [-Wunused-but-set-variable]
    2992 |         int status;
         |             ^~~~~~


vim +/status +2992 drivers/platform/x86/toshiba_acpi.c

  2984	
  2985	
  2986	/* ACPI battery hooking */
  2987	static ssize_t charge_control_end_threshold_show(struct device *device,
  2988							 struct device_attribute *attr,
  2989							 char *buf)
  2990	{
  2991		int state;
> 2992		int status;
  2993	
  2994		if (toshiba_acpi == NULL) {
  2995			pr_err("Toshiba ACPI object invalid\n");
  2996			return -ENODEV;
  2997		}
  2998	
  2999		status = toshiba_battery_charge_mode_get(toshiba_acpi, &state);
  3000		if (state == 1)
  3001			return sprintf(buf, "80\n");
  3002		else
  3003			return sprintf(buf, "100\n");
  3004	}
  3005	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)

[Arvid Norlander]

Hi,

On 2022-08-29 00:15, kernel test robot wrote:
> Hi Arvid,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on 1c23f9e627a7b412978b4e852793c5e3c3efc555]

<snip>

> vim +/status +2992 drivers/platform/x86/toshiba_acpi.c
> 
>   2984	
>   2985	
>   2986	/* ACPI battery hooking */
>   2987	static ssize_t charge_control_end_threshold_show(struct device *device,
>   2988							 struct device_attribute *attr,
>   2989							 char *buf)
>   2990	{
>   2991		int state;
>> 2992		int status;

Will be fixed in the next version, it should be used. Waiting for human feedback too first.

It would be nice to see these warnings locally, anyone know how to turn them on?

>   2993	
>   2994		if (toshiba_acpi == NULL) {
>   2995			pr_err("Toshiba ACPI object invalid\n");
>   2996			return -ENODEV;
>   2997		}
>   2998	
>   2999		status = toshiba_battery_charge_mode_get(toshiba_acpi, &state);
>   3000		if (state == 1)
>   3001			return sprintf(buf, "80\n");
>   3002		else
>   3003			return sprintf(buf, "100\n");
>   3004	}
>   3005	
> 

Best regards,
Arvid Norlander

Re: [PATCH 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)

[Arvid Norlander]

[-- Attachment #1: Type: text/plain, Size: 1149 bytes --]

Hi,

On 2022-08-29 00:15, kernel test robot wrote:
> Hi Arvid,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on 1c23f9e627a7b412978b4e852793c5e3c3efc555]

<snip>

> vim +/status +2992 drivers/platform/x86/toshiba_acpi.c
> 
>   2984	
>   2985	
>   2986	/* ACPI battery hooking */
>   2987	static ssize_t charge_control_end_threshold_show(struct device *device,
>   2988							 struct device_attribute *attr,
>   2989							 char *buf)
>   2990	{
>   2991		int state;
>> 2992		int status;

Will be fixed in the next version, it should be used. Waiting for human feedback too first.

It would be nice to see these warnings locally, anyone know how to turn them on?

>   2993	
>   2994		if (toshiba_acpi == NULL) {
>   2995			pr_err("Toshiba ACPI object invalid\n");
>   2996			return -ENODEV;
>   2997		}
>   2998	
>   2999		status = toshiba_battery_charge_mode_get(toshiba_acpi, &state);
>   3000		if (state == 1)
>   3001			return sprintf(buf, "80\n");
>   3002		else
>   3003			return sprintf(buf, "100\n");
>   3004	}
>   3005	
> 

Best regards,
Arvid Norlander

Re: [PATCH 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)

[Miguel Ojeda]

On Mon, Aug 29, 2022 at 12:50 AM Arvid Norlander <lkml@vorpal.se> wrote:
>
> It would be nice to see these warnings locally, anyone know how to turn them on?

Compiling with W=1 (and possibly with Clang) should show them. Or
doesn't that work for you?

Cheers,
Miguel

Re: [PATCH 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)

[Miguel Ojeda]

[-- Attachment #1: Type: text/plain, Size: 275 bytes --]

On Mon, Aug 29, 2022 at 12:50 AM Arvid Norlander <lkml@vorpal.se> wrote:
>
> It would be nice to see these warnings locally, anyone know how to turn them on?

Compiling with W=1 (and possibly with Clang) should show them. Or
doesn't that work for you?

Cheers,
Miguel

Re: [PATCH 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)

[Nick Desaulniers]

On Sun, Aug 28, 2022 at 3:16 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi Arvid,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on 1c23f9e627a7b412978b4e852793c5e3c3efc555]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Arvid-Norlander/platform-x86-Battery-charge-mode-in-toshiba_acpi/20220829-033110
> base:   1c23f9e627a7b412978b4e852793c5e3c3efc555
> config: x86_64-randconfig-a014-20220829 (https://download.01.org/0day-ci/archive/20220829/202208290605.gE9IGbxE-lkp@intel.com/config)

Note: this was from a randconfig; it's highly likely you need this
funky config to repro.  ^ should be the link to the config.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)

[Nick Desaulniers]

[-- Attachment #1: Type: text/plain, Size: 724 bytes --]

On Sun, Aug 28, 2022 at 3:16 PM kernel test robot <lkp@intel.com> wrote:
>
> Hi Arvid,
>
> Thank you for the patch! Perhaps something to improve:
>
> [auto build test WARNING on 1c23f9e627a7b412978b4e852793c5e3c3efc555]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Arvid-Norlander/platform-x86-Battery-charge-mode-in-toshiba_acpi/20220829-033110
> base:   1c23f9e627a7b412978b4e852793c5e3c3efc555
> config: x86_64-randconfig-a014-20220829 (https://download.01.org/0day-ci/archive/20220829/202208290605.gE9IGbxE-lkp(a)intel.com/config)

Note: this was from a randconfig; it's highly likely you need this
funky config to repro.  ^ should be the link to the config.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 0/3] platform/x86: Battery charge mode in toshiba_acpi

[Hans de Goede]

Hi Arvid,

On 8/28/22 21:29, Arvid Norlander wrote:
> This is an improved version of the battery charge control for Toshiba
> Satellite Z830. The full background is available in the two emails linked
> below, but a short summary will follow, including only what is relevant
> for battery charge control.

Thank you for your work on this.

Overall 3 patches look good to me.

Sebastian, any chance you could take a look at patch 3/3
and maybe give me an ack for merging that through the pdx86
tree ?   (assuming you are ok with it)


2 small remarks about patch 2/3:

Remark 1:

+	rval = toshiba_battery_charge_mode_set(toshiba_acpi,
+					       (value < 90) ? 1 : 0);

Playing Devil's advocate here: to a casual reader this looks
a bit weird, why would I want to enable "charge mode"
(whatever that is).

IMHO it would be better to call the set (and get) function something
like e.g.:  toshiba_battery_set_eco_charge_mode()  So basicaly
add "eco" somewhere in the name. IIRC that is what Toshiba themselves
use right ?  I think that makes the meaning of the mode being 0 vs
it being one more clear.

That and/or add an enum for the 0/1 values and use the enum instead,
the enum could e.g. look something like this:

enum {
	TOSHIBA_CHARGE_FULL_CHARGE,
	TOSHIBA_CHARGE_ECO_MODE,
};

Note either of the suggested changes would be enough to make
the code more clear. Also this and especially the suggested
names are just a suggestion.


Remark 2:

+static int toshiba_acpi_battery_add(struct power_supply *battery)
+{
+	if (toshiba_acpi == NULL) {
+		pr_err("Init order issue\n");
+		return -ENODEV;


This will never happen. The hook is only registered when
toshiba_acpi != NULL and it will get unregistered before
toshiba_acpi gets set to NULL on remove.

+	}
+	if (!toshiba_acpi->battery_charge_mode_supported)
+		return -ENODEV;

If toshiba_acpi->battery_charge_mode_supported == false then
the acpi_battery_hook battery_hook will never get registered
and thus this will never get called.

+	if (device_add_groups(&battery->dev, toshiba_acpi_battery_groups))
+		return -ENODEV;
+	return 0;
+}

So you really only need the device_add_groups() which should never
faill and if it does fail then propagating the actual error would
be better.

So all in all IMHO this function can be simplified to just:

static int toshiba_acpi_battery_add(struct power_supply *battery)
{
	return device_add_groups(&battery->dev, toshiba_acpi_battery_groups);
}


Regards,

Hans







> 
> 
> Background (from link 1)
> ==========
> 
> The Toshiba Satellite/Portege Z830 supports not charging the battery fully
> in order to prolong battery life. Unlike for example ThinkPads where this
> control is granular here it is just off/on. When off it charges to 100%.
> When on it charges to about 80%.
> 
> According to the Windows program used to control the feature the setting
> will not take effect until the battery has been discharged to around 50%.
> However, in my testing it takes effect as soon as the charge drops below
> 80%. On Windows Toshiba branded this feature as "Eco charging"
> 
> In the following example ACPI calls I will use the following newly defined
> constants:
> #define HCI_BATTERY_CHARGE_MODE 0xba
> #define BATTERY_CHARGE_FULL 0
> #define BATTERY_CHARGE_80_PERCENT 1
> 
> To set the feature:
>   {HCI_SET, HCI_BATTERY_CHARGE_MODE, charge_mode, 0, 0, 0}
> To query for the existence of the feature:
>   {HCI_GET, HCI_BATTERY_CHARGE_MODE, 0, 0, 0, 0}
> To read the feature:
>   {HCI_GET, HCI_BATTERY_CHARGE_MODE, 0, 0, 0, 1}
> 
> The read may need to be retried if TOS_DATA_NOT_AVAILABLE is returned as
> the status code. This rarely happens (I have never observed it on Linux),
> but I have seen it happen under Windows once, and the software did retry
> it.
> 
> 
> Improvements
> ============
> 
> As discussed in link 2 & 3 below, the original approach was suboptimal.
> 
> This patch series instead consists of two patches.
> 
> The first patch implements detecting the feature as well as internal
> getter/setter methods.
> 
> The second patch adds battery hooks (heavily based on the code for this in
> thinkpad_acpi) which creates the standard charge_control_end_threshold file
> under /sys/class/power_supply/BAT1.
> 
> Side note: There is no BAT0 on this Toshiba, I'm not sure why the numbering
> ends up starting from 1 instead of 0 here. This differs from my Thinkpads,
> where the numbering starts from 0, with BAT1 being the second battery.
> However, I haven't spent much effort investigating this, as it did not seem
> important.
> 
> Patch 3 updates the ABI test documentation as suggested by Hans de Goede.
> Note that only the charge_control_end_threshold is updated, as this is the
> only limit supported by the Toshiba Z830. Possibly
> charge_control_start_threshold should also be updated similarly, or would
> it be better to wait for an actual example of this in the wild first?
> 
> Link (1): https://www.spinics.net/lists/platform-driver-x86/msg34314.html
> Link (2): https://www.spinics.net/lists/platform-driver-x86/msg34354.html
> Link (3): https://www.spinics.net/lists/platform-driver-x86/msg34320.html
> 
> Arvid Norlander (3):
>   platform/x86: Battery charge mode in toshiba_acpi (internals)
>   platform/x86: Battery charge mode in toshiba_acpi (sysfs)
>   docs: ABI: charge_control_end_threshold may not support all values
> 
>  Documentation/ABI/testing/sysfs-class-power |   5 +-
>  drivers/platform/x86/toshiba_acpi.c         | 162 ++++++++++++++++++++
>  2 files changed, 166 insertions(+), 1 deletion(-)
> 
> 
> base-commit: 1c23f9e627a7b412978b4e852793c5e3c3efc555

Re: [PATCH 3/3] docs: ABI: charge_control_end_threshold may not support all values

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

Hi,

On Sun, Aug 28, 2022 at 09:29:20PM +0200, Arvid Norlander wrote:
> Some laptops (for example Toshiba Satellite Z830) only supports some fixed
> values. Allow for this and document the expected behaviour in such cases.
> 
> Wording suggested by Hans de Goede.
> 
> Signed-off-by: Arvid Norlander <lkml@vorpal.se>
> ---

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  Documentation/ABI/testing/sysfs-class-power | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-power b/Documentation/ABI/testing/sysfs-class-power
> index a9ce63cfbe87..e434fc523291 100644
> --- a/Documentation/ABI/testing/sysfs-class-power
> +++ b/Documentation/ABI/testing/sysfs-class-power
> @@ -364,7 +364,10 @@ Date:		April 2019
>  Contact:	linux-pm@vger.kernel.org
>  Description:
>  		Represents a battery percentage level, above which charging will
> -		stop.
> +		stop. Not all hardware is capable of setting this to an arbitrary
> +		percentage. Drivers will round written values to the nearest
> +		supported value. Reading back the value will show the actual
> +		threshold set by the driver.
>  
>  		Access: Read, Write
>  
> -- 
> 2.37.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]