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
