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

[Hans de Goede <hdegoede@redhat.com>]

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