platform-driver-x86.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] platform/x86: Battery charge mode in toshiba_acpi
@ 2022-09-02 18:00 Arvid Norlander
  2022-09-02 18:00 ` [PATCH 1/3] platform/x86: Battery charge mode in toshiba_acpi (internals) Arvid Norlander
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Arvid Norlander @ 2022-09-02 18:00 UTC (permalink / raw)
  To: platform-driver-x86, linux-pm
  Cc: Sebastian Reichel, Hans de Goede, Azael Avalos, Arvid Norlander

Hi,

Here we go again.

Note that this patch series edits in the same place as my patch series
adding HWMON support for the fan, so there will be a trivial merge
conflict, as both series insert new functions in the same location in the
file. Hopefully this is not a big issue, but if so I can rebase one on top
of the other.

Changelog
=========
v2:
  * Fix compiler warning discovered by "kernel test robot" in patch 2
    (real issue).
  * Added Acked-by in patch 3 (Thanks Sebastian Reichel).


Mostly original (from v1 of this series) cover letter follows:

Summary
=======

This patch series implements battery charge control for Toshiba Satellite
Z830 (and posssibly some other models). 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

Best regards,
Arvid Norlander


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         | 166 ++++++++++++++++++++
 2 files changed, 170 insertions(+), 1 deletion(-)


base-commit: b90cb1053190353cc30f0fef0ef1f378ccc063c5
-- 
2.37.3


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/3] platform/x86: Battery charge mode in toshiba_acpi (internals)
  2022-09-02 18:00 [PATCH v2 0/3] platform/x86: Battery charge mode in toshiba_acpi Arvid Norlander
@ 2022-09-02 18:00 ` Arvid Norlander
  2022-09-02 18:00 ` [PATCH v2 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs) Arvid Norlander
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Arvid Norlander @ 2022-09-02 18:00 UTC (permalink / raw)
  To: platform-driver-x86, linux-pm
  Cc: Sebastian Reichel, Hans de Goede, Azael Avalos, 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.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)
  2022-09-02 18:00 [PATCH v2 0/3] platform/x86: Battery charge mode in toshiba_acpi Arvid Norlander
  2022-09-02 18:00 ` [PATCH 1/3] platform/x86: Battery charge mode in toshiba_acpi (internals) Arvid Norlander
@ 2022-09-02 18:00 ` Arvid Norlander
  2022-09-09 17:19   ` Hans de Goede
  2022-09-02 18:00 ` [PATCH v2 3/3] docs: ABI: charge_control_end_threshold may not support all values Arvid Norlander
  2022-09-09 17:25 ` [PATCH v2 0/3] platform/x86: Battery charge mode in toshiba_acpi Hans de Goede
  3 siblings, 1 reply; 6+ messages in thread
From: Arvid Norlander @ 2022-09-02 18:00 UTC (permalink / raw)
  To: platform-driver-x86, linux-pm
  Cc: Sebastian Reichel, Hans de Goede, Azael Avalos, 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 | 97 +++++++++++++++++++++++++++++
 1 file changed, 97 insertions(+)

diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
index c927d5d0f8cd..fc953d6bcb93 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,92 @@ 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)
+{
+	u32 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 (status != 0)
+		return status;
+
+	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 +3150,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 +3336,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.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 3/3] docs: ABI: charge_control_end_threshold may not support all values
  2022-09-02 18:00 [PATCH v2 0/3] platform/x86: Battery charge mode in toshiba_acpi Arvid Norlander
  2022-09-02 18:00 ` [PATCH 1/3] platform/x86: Battery charge mode in toshiba_acpi (internals) Arvid Norlander
  2022-09-02 18:00 ` [PATCH v2 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs) Arvid Norlander
@ 2022-09-02 18:00 ` Arvid Norlander
  2022-09-09 17:25 ` [PATCH v2 0/3] platform/x86: Battery charge mode in toshiba_acpi Hans de Goede
  3 siblings, 0 replies; 6+ messages in thread
From: Arvid Norlander @ 2022-09-02 18:00 UTC (permalink / raw)
  To: platform-driver-x86, linux-pm
  Cc: Sebastian Reichel, Hans de Goede, Azael Avalos, Arvid Norlander,
	Sebastian Reichel

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

Acked-by: Sebastian Reichel <sebastian.reichel@collabora.com>
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.3


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)
  2022-09-02 18:00 ` [PATCH v2 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs) Arvid Norlander
@ 2022-09-09 17:19   ` Hans de Goede
  0 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2022-09-09 17:19 UTC (permalink / raw)
  To: Arvid Norlander, platform-driver-x86, linux-pm
  Cc: Sebastian Reichel, Azael Avalos

Hi,

On 9/2/22 20:00, Arvid Norlander wrote:
> 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 | 97 +++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index c927d5d0f8cd..fc953d6bcb93 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,92 @@ 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)
> +{
> +	u32 state;
> +	int status;
> +
> +	if (toshiba_acpi == NULL) {
> +		pr_err("Toshiba ACPI object invalid\n");
> +		return -ENODEV;
> +	}

These and the other (toshiba_acpi == NULL) checks are not necessary,
battery_hook_register() is only called after setting toshiba_acpi to non NULL
and battery_hook_unregister() is called before setting it NULL again,
so toshiba_acpi can never be NULL when the callbacks run.

I have removed all the NULL checks while merging this.


> +
> +	status = toshiba_battery_charge_mode_get(toshiba_acpi, &state);
> +
> +	if (status != 0)
> +		return status;
> +
> +	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 +3150,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);
> +

battery_hook_[un]register() call code from the acpi_battery
kernel code/module. To make sure those symbols are actually available
we need to add: "depends on ACPI_BATTERY" to config ACPI_TOSHIBA
in Kconfig. I have done this while merging this.

Regards,

Hans




>  	if (toshiba_acpi)
>  		toshiba_acpi = NULL;
>  
> @@ -3246,6 +3336,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:


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 0/3] platform/x86: Battery charge mode in toshiba_acpi
  2022-09-02 18:00 [PATCH v2 0/3] platform/x86: Battery charge mode in toshiba_acpi Arvid Norlander
                   ` (2 preceding siblings ...)
  2022-09-02 18:00 ` [PATCH v2 3/3] docs: ABI: charge_control_end_threshold may not support all values Arvid Norlander
@ 2022-09-09 17:25 ` Hans de Goede
  3 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2022-09-09 17:25 UTC (permalink / raw)
  To: Arvid Norlander, platform-driver-x86, linux-pm
  Cc: Sebastian Reichel, Azael Avalos

Hi,

On 9/2/22 20:00, Arvid Norlander wrote:
> Hi,
> 
> Here we go again.
> 
> Note that this patch series edits in the same place as my patch series
> adding HWMON support for the fan, so there will be a trivial merge
> conflict, as both series insert new functions in the same location in the
> file. Hopefully this is not a big issue, but if so I can rebase one on top
> of the other.
> 
> Changelog
> =========
> v2:
>   * Fix compiler warning discovered by "kernel test robot" in patch 2
>     (real issue).
>   * Added Acked-by in patch 3 (Thanks Sebastian Reichel).
> 
> 
> Mostly original (from v1 of this series) cover letter follows:
> 
> Summary
> =======
> 
> This patch series implements battery charge control for Toshiba Satellite
> Z830 (and posssibly some other models). 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
> 
> Best regards,
> Arvid Norlander
> 
> 
> 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         | 166 ++++++++++++++++++++
>  2 files changed, 170 insertions(+), 1 deletion(-)

Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans




^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-09-09 17:25 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-02 18:00 [PATCH v2 0/3] platform/x86: Battery charge mode in toshiba_acpi Arvid Norlander
2022-09-02 18:00 ` [PATCH 1/3] platform/x86: Battery charge mode in toshiba_acpi (internals) Arvid Norlander
2022-09-02 18:00 ` [PATCH v2 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs) Arvid Norlander
2022-09-09 17:19   ` Hans de Goede
2022-09-02 18:00 ` [PATCH v2 3/3] docs: ABI: charge_control_end_threshold may not support all values Arvid Norlander
2022-09-09 17:25 ` [PATCH v2 0/3] platform/x86: Battery charge mode in toshiba_acpi Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).