* Re: [PATCH 1/1] platform/x86: ideapad: Expose charge_types
2025-05-11 11:30 ` [PATCH 1/1] " Jelle van der Waa
@ 2025-05-11 22:54 ` Armin Wolf
2025-05-13 8:38 ` Jelle van der Waa
2025-05-12 6:53 ` Thomas Weißschuh
1 sibling, 1 reply; 6+ messages in thread
From: Armin Wolf @ 2025-05-11 22:54 UTC (permalink / raw)
To: Jelle van der Waa, Ike Panhc, Hans de Goede, Ilpo Järvinen
Cc: Jelle van der Waa, platform-driver-x86
Am 11.05.25 um 13:30 schrieb Jelle van der Waa:
> From: Jelle van der Waa <jvanderwaa@redhat.com>
>
> Some Ideapad models support a battery conservation mode which limits the
> battery charge threshold for longer battery longevity. This is currently
> exposed via a custom conservation_mode attribute in sysfs.
>
> The newly introduced charge_types sysfs attribute is a standardized
> replacement for laptops with a fixed end charge threshold. Setting it to
> `Long Life` would enable battery conservation mode. The standardized
> user space API would allow applications such as UPower to detect laptops
> which support this battery longevity mode and set it.
>
> Tested on an Lenovo ideapad U330p.
Hi,
i like the idea behind this patch series, the charge_types attribute is indeed
exactly what we need in this case.
>
> Signed-off-by: Jelle van der Waa <jvanderwaa@redhat.com>
> ---
> .../ABI/testing/sysfs-platform-ideapad-laptop | 2 +
> drivers/platform/x86/ideapad-laptop.c | 126 +++++++++++++++++-
> 2 files changed, 125 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> index 4989ab266682..83eca4c14503 100644
> --- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> +++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> @@ -32,6 +32,8 @@ Date: Aug 2017
> KernelVersion: 4.14
> Contact: platform-driver-x86@vger.kernel.org
> Description:
> + This interface is deprecated; please use /sys/class/power_supply/*/charge_types.
> +
Maybe it would make sense to move this attribute to Documentation/ABI/obsolete?
> Controls whether the conservation mode is enabled or not.
> This feature limits the maximum battery charge percentage to
> around 50-60% in order to prolong the lifetime of the battery.
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index ede483573fe0..fd9127ffd456 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -34,12 +34,17 @@
> #include <linux/wmi.h>
> #include "ideapad-laptop.h"
>
> +#include <linux/power_supply.h>
> +#include <acpi/battery.h>
Please make sure that IDEAPAD_LAPTOP depends on ACPI_BATTERY inside the Kconfig.
> #include <acpi/video.h>
>
> #include <dt-bindings/leds/common.h>
>
> #define IDEAPAD_RFKILL_DEV_NUM 3
>
> +#define IDEAPAD_CHARGE_TYPES (BIT(POWER_SUPPLY_CHARGE_TYPE_STANDARD) | \
> + BIT(POWER_SUPPLY_CHARGE_TYPE_LONGLIFE))
This macro is rather small and only used once. Please open-code it.
> +
> enum {
> CFG_CAP_BT_BIT = 16,
> CFG_CAP_3G_BIT = 17,
> @@ -162,6 +167,8 @@ struct ideapad_private {
> struct backlight_device *blightdev;
> struct ideapad_dytc_priv *dytc;
> struct dentry *debug;
> + struct acpi_battery_hook battery_hook;
> + struct power_supply *hooked_battery;
Unused attribute, please remove.
> unsigned long cfg;
> unsigned long r_touchpad_val;
> struct {
> @@ -589,6 +596,11 @@ static ssize_t camera_power_store(struct device *dev,
>
> static DEVICE_ATTR_RW(camera_power);
>
> +static void show_deprecation_warning(struct device *dev)
> +{
> + dev_warn_once(dev, "conservation_mode attribute has been deprecated, see charge_types.\n");
> +}
> +
> static ssize_t conservation_mode_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -597,6 +609,8 @@ static ssize_t conservation_mode_show(struct device *dev,
> unsigned long result;
> int err;
>
> + show_deprecation_warning(dev);
> +
> err = eval_gbmd(priv->adev->handle, &result);
> if (err)
> return err;
> @@ -612,6 +626,8 @@ static ssize_t conservation_mode_store(struct device *dev,
> bool state;
> int err;
>
> + show_deprecation_warning(dev);
> +
> err = kstrtobool(buf, &state);
> if (err)
> return err;
> @@ -1973,10 +1989,99 @@ static const struct dmi_system_id ctrl_ps2_aux_port_list[] = {
> {}
> };
>
> -static void ideapad_check_features(struct ideapad_private *priv)
> +static int ideapad_psy_ext_set_prop(struct power_supply *psy,
> + const struct power_supply_ext *ext,
Alignment should match open parenthesis.
> + void *ext_data,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + struct ideapad_private *priv = ext_data;
> + int err;
> +
> + if (psp != POWER_SUPPLY_PROP_CHARGE_TYPES)
> + return -EINVAL;
> +
> + err = exec_sbmc(priv->adev->handle,
> + (val->intval == POWER_SUPPLY_CHARGE_TYPE_LONGLIFE ?
> + SBMC_CONSERVATION_ON : SBMC_CONSERVATION_OFF));
AFAIK the power supply core does not check if val->intval holds a supported charge type value.
Please use a switch case to return -EINVAL in such cases.
> + if (err)
> + return err;
Please directly return the result of exec_sbmc() here.
> +
> + return 0;
> +}
> +
> +static int ideapad_psy_ext_get_prop(struct power_supply *psy,
> + const struct power_supply_ext *ext,
Alignment should match open parenthesis.
> + void *ext_data,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct ideapad_private *priv = ext_data;
> + unsigned long result;
> + int err;
> +
> + if (psp != POWER_SUPPLY_PROP_CHARGE_TYPES)
> + return -EINVAL;
> +
> + err = eval_gbmd(priv->adev->handle, &result);
> + if (err)
> + return err;
> +
> + if (test_bit(GBMD_CONSERVATION_STATE_BIT, &result))
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_LONGLIFE;
> + else
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
> +
> + return 0;
> +}
> +
> +static int ideapad_psy_prop_is_writeable(struct power_supply *psy,
> + const struct power_supply_ext *ext,
Alignment should match open parenthesis.
> + void *data,
> + enum power_supply_property psp)
> +{
> + if (psp == POWER_SUPPLY_PROP_CHARGE_TYPES)
> + return true;
> +
> + return false;
> +}
> +
> +static const enum power_supply_property ideapad_power_supply_props[] = {
> + POWER_SUPPLY_PROP_CHARGE_TYPES,
> +};
> +
> +static const struct power_supply_ext ideapad_battery_ext = {
> + .name = "ideapad",
Maybe using a more specific name like "ideapad_laptop"/"ideapad_acpi" would be better here?
> + .properties = ideapad_power_supply_props,
> + .num_properties = ARRAY_SIZE(ideapad_power_supply_props),
> + .charge_types = IDEAPAD_CHARGE_TYPES,
> + .get_property = ideapad_psy_ext_get_prop,
> + .set_property = ideapad_psy_ext_set_prop,
> + .property_is_writeable = ideapad_psy_prop_is_writeable,
> +};
> +
> +static int ideapad_battery_add(struct power_supply *battery,
> + struct acpi_battery_hook *hook)
> +{
> + struct ideapad_private *priv = container_of(hook, struct ideapad_private, battery_hook);
> +
> + return power_supply_register_extension(battery, &ideapad_battery_ext,
> + &priv->platform_device->dev, priv);
> +}
> +
> +static int ideapad_battery_remove(struct power_supply *battery,
> + struct acpi_battery_hook *hook)
> +{
> + power_supply_unregister_extension(battery, &ideapad_battery_ext);
> +
> + return 0;
> +}
> +
> +static int ideapad_check_features(struct ideapad_private *priv)
> {
> acpi_handle handle = priv->adev->handle;
> unsigned long val;
> + int err;
>
> priv->features.set_fn_lock_led =
> set_fn_lock_led || dmi_check_system(set_fn_lock_led_list);
> @@ -1991,8 +2096,19 @@ static void ideapad_check_features(struct ideapad_private *priv)
> if (!read_ec_data(handle, VPCCMD_R_FAN, &val))
> priv->features.fan_mode = true;
>
> - if (acpi_has_method(handle, "GBMD") && acpi_has_method(handle, "SBMC"))
> + if (acpi_has_method(handle, "GBMD") && acpi_has_method(handle, "SBMC")) {
> priv->features.conservation_mode = true;
> + priv->battery_hook.add_battery = ideapad_battery_add;
> + priv->battery_hook.remove_battery = ideapad_battery_remove;
> + priv->battery_hook.name = "Ideapad Battery Extension";
> +
> + err = devm_battery_hook_register(&priv->platform_device->dev, &priv->battery_hook);
> + if (err) {
> + dev_dbg(&priv->platform_device->dev,
> + "failed to register battery hook: %d\n", err);
I think this error message is unnecessary. Please remove it.
Thanks,
Armin Wolf
> + return err;
> + }
> + }
>
> if (acpi_has_method(handle, "DYTC"))
> priv->features.dytc = true;
> @@ -2027,6 +2143,8 @@ static void ideapad_check_features(struct ideapad_private *priv)
> }
> }
> }
> +
> + return 0;
> }
>
> #if IS_ENABLED(CONFIG_ACPI_WMI)
> @@ -2175,7 +2293,9 @@ static int ideapad_acpi_add(struct platform_device *pdev)
> if (err)
> return err;
>
> - ideapad_check_features(priv);
> + err = ideapad_check_features(priv);
> + if (err)
> + return err;
>
> ideapad_debugfs_init(priv);
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] platform/x86: ideapad: Expose charge_types
2025-05-11 11:30 ` [PATCH 1/1] " Jelle van der Waa
2025-05-11 22:54 ` Armin Wolf
@ 2025-05-12 6:53 ` Thomas Weißschuh
1 sibling, 0 replies; 6+ messages in thread
From: Thomas Weißschuh @ 2025-05-12 6:53 UTC (permalink / raw)
To: Jelle van der Waa
Cc: Ike Panhc, Hans de Goede, Ilpo Järvinen, Jelle van der Waa,
platform-driver-x86
On 2025-05-11 13:30:09+0200, Jelle van der Waa wrote:
> From: Jelle van der Waa <jvanderwaa@redhat.com>
>
> Some Ideapad models support a battery conservation mode which limits the
> battery charge threshold for longer battery longevity. This is currently
> exposed via a custom conservation_mode attribute in sysfs.
>
> The newly introduced charge_types sysfs attribute is a standardized
> replacement for laptops with a fixed end charge threshold. Setting it to
> `Long Life` would enable battery conservation mode. The standardized
> user space API would allow applications such as UPower to detect laptops
> which support this battery longevity mode and set it.
>
> Tested on an Lenovo ideapad U330p.
>
> Signed-off-by: Jelle van der Waa <jvanderwaa@redhat.com>
> ---
> .../ABI/testing/sysfs-platform-ideapad-laptop | 2 +
> drivers/platform/x86/ideapad-laptop.c | 126 +++++++++++++++++-
> 2 files changed, 125 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> index 4989ab266682..83eca4c14503 100644
> --- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> +++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> @@ -32,6 +32,8 @@ Date: Aug 2017
> KernelVersion: 4.14
> Contact: platform-driver-x86@vger.kernel.org
> Description:
> + This interface is deprecated; please use /sys/class/power_supply/*/charge_types.
> +
> Controls whether the conservation mode is enabled or not.
> This feature limits the maximum battery charge percentage to
> around 50-60% in order to prolong the lifetime of the battery.
> diff --git a/drivers/platform/x86/ideapad-laptop.c b/drivers/platform/x86/ideapad-laptop.c
> index ede483573fe0..fd9127ffd456 100644
> --- a/drivers/platform/x86/ideapad-laptop.c
> +++ b/drivers/platform/x86/ideapad-laptop.c
> @@ -34,12 +34,17 @@
> #include <linux/wmi.h>
> #include "ideapad-laptop.h"
>
> +#include <linux/power_supply.h>
Other linux/ includes are in the block above.
> +#include <acpi/battery.h>
> #include <acpi/video.h>
>
> #include <dt-bindings/leds/common.h>
>
> #define IDEAPAD_RFKILL_DEV_NUM 3
>
> +#define IDEAPAD_CHARGE_TYPES (BIT(POWER_SUPPLY_CHARGE_TYPE_STANDARD) | \
> + BIT(POWER_SUPPLY_CHARGE_TYPE_LONGLIFE))
> +
> enum {
> CFG_CAP_BT_BIT = 16,
> CFG_CAP_3G_BIT = 17,
> @@ -162,6 +167,8 @@ struct ideapad_private {
> struct backlight_device *blightdev;
> struct ideapad_dytc_priv *dytc;
> struct dentry *debug;
> + struct acpi_battery_hook battery_hook;
> + struct power_supply *hooked_battery;
hooked_battery is not used.
> unsigned long cfg;
> unsigned long r_touchpad_val;
> struct {
> @@ -589,6 +596,11 @@ static ssize_t camera_power_store(struct device *dev,
>
> static DEVICE_ATTR_RW(camera_power);
>
> +static void show_deprecation_warning(struct device *dev)
show_conservation_mode_deprecation_warning();
> +{
> + dev_warn_once(dev, "conservation_mode attribute has been deprecated, see charge_types.\n");
> +}
> +
> static ssize_t conservation_mode_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -597,6 +609,8 @@ static ssize_t conservation_mode_show(struct device *dev,
> unsigned long result;
> int err;
>
> + show_deprecation_warning(dev);
> +
> err = eval_gbmd(priv->adev->handle, &result);
> if (err)
> return err;
> @@ -612,6 +626,8 @@ static ssize_t conservation_mode_store(struct device *dev,
> bool state;
> int err;
>
> + show_deprecation_warning(dev);
> +
> err = kstrtobool(buf, &state);
> if (err)
> return err;
> @@ -1973,10 +1989,99 @@ static const struct dmi_system_id ctrl_ps2_aux_port_list[] = {
> {}
> };
>
> -static void ideapad_check_features(struct ideapad_private *priv)
> +static int ideapad_psy_ext_set_prop(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + void *ext_data,
> + enum power_supply_property psp,
> + const union power_supply_propval *val)
> +{
> + struct ideapad_private *priv = ext_data;
> + int err;
> +
> + if (psp != POWER_SUPPLY_PROP_CHARGE_TYPES)
> + return -EINVAL;
This check will never trigger. Same below)
> +
> + err = exec_sbmc(priv->adev->handle,
> + (val->intval == POWER_SUPPLY_CHARGE_TYPE_LONGLIFE ?
> + SBMC_CONSERVATION_ON : SBMC_CONSERVATION_OFF));
No need for braces.
Directly "return exec_sbmc()".
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +
> +static int ideapad_psy_ext_get_prop(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + void *ext_data,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct ideapad_private *priv = ext_data;
> + unsigned long result;
> + int err;
> +
> + if (psp != POWER_SUPPLY_PROP_CHARGE_TYPES)
> + return -EINVAL;
> +
> + err = eval_gbmd(priv->adev->handle, &result);
> + if (err)
> + return err;
> +
> + if (test_bit(GBMD_CONSERVATION_STATE_BIT, &result))
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_LONGLIFE;
> + else
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
> +
> + return 0;
> +}
> +
> +static int ideapad_psy_prop_is_writeable(struct power_supply *psy,
> + const struct power_supply_ext *ext,
> + void *data,
> + enum power_supply_property psp)
> +{
> + if (psp == POWER_SUPPLY_PROP_CHARGE_TYPES)
> + return true;
No need for the conditional.
> +
> + return false;
> +}
> +
> +static const enum power_supply_property ideapad_power_supply_props[] = {
> + POWER_SUPPLY_PROP_CHARGE_TYPES,
> +};
> +
> +static const struct power_supply_ext ideapad_battery_ext = {
> + .name = "ideapad",
> + .properties = ideapad_power_supply_props,
> + .num_properties = ARRAY_SIZE(ideapad_power_supply_props),
> + .charge_types = IDEAPAD_CHARGE_TYPES,
> + .get_property = ideapad_psy_ext_get_prop,
> + .set_property = ideapad_psy_ext_set_prop,
> + .property_is_writeable = ideapad_psy_prop_is_writeable,
> +};
> +
> +static int ideapad_battery_add(struct power_supply *battery,
> + struct acpi_battery_hook *hook)
No need for the linebreak, the line will be shorter than the one below.
> +{
> + struct ideapad_private *priv = container_of(hook, struct ideapad_private, battery_hook);
> +
> + return power_supply_register_extension(battery, &ideapad_battery_ext,
> + &priv->platform_device->dev, priv);
> +}
> +
> +static int ideapad_battery_remove(struct power_supply *battery,
> + struct acpi_battery_hook *hook)
> +{
> + power_supply_unregister_extension(battery, &ideapad_battery_ext);
> +
> + return 0;
> +}
> +
> +static int ideapad_check_features(struct ideapad_private *priv)
> {
> acpi_handle handle = priv->adev->handle;
> unsigned long val;
> + int err;
>
> priv->features.set_fn_lock_led =
> set_fn_lock_led || dmi_check_system(set_fn_lock_led_list);
> @@ -1991,8 +2096,19 @@ static void ideapad_check_features(struct ideapad_private *priv)
> if (!read_ec_data(handle, VPCCMD_R_FAN, &val))
> priv->features.fan_mode = true;
>
> - if (acpi_has_method(handle, "GBMD") && acpi_has_method(handle, "SBMC"))
> + if (acpi_has_method(handle, "GBMD") && acpi_has_method(handle, "SBMC")) {
> priv->features.conservation_mode = true;
> + priv->battery_hook.add_battery = ideapad_battery_add;
> + priv->battery_hook.remove_battery = ideapad_battery_remove;
> + priv->battery_hook.name = "Ideapad Battery Extension";
> +
> + err = devm_battery_hook_register(&priv->platform_device->dev, &priv->battery_hook);
> + if (err) {
> + dev_dbg(&priv->platform_device->dev,
> + "failed to register battery hook: %d\n", err);
> + return err;
Use dev_err_probe().
> + }
> + }
>
> if (acpi_has_method(handle, "DYTC"))
> priv->features.dytc = true;
> @@ -2027,6 +2143,8 @@ static void ideapad_check_features(struct ideapad_private *priv)
> }
> }
> }
> +
> + return 0;
> }
>
> #if IS_ENABLED(CONFIG_ACPI_WMI)
> @@ -2175,7 +2293,9 @@ static int ideapad_acpi_add(struct platform_device *pdev)
> if (err)
> return err;
>
> - ideapad_check_features(priv);
> + err = ideapad_check_features(priv);
> + if (err)
> + return err;
>
> ideapad_debugfs_init(priv);
>
> --
> 2.49.0
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread