| Message ID | 20240129175714.164326-2-szfabian@bluemarch.art (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [RFC,v2] platform/x86/fujitsu-laptop: Add battery charge control support | expand |
Am 29.01.24 um 19:00 schrieb Szilard Fabian: > This patch adds battery charge control support on Fujitsu notebooks > via the S006 method of the FUJ02E3 ACPI device. With this method it's > possible to set charge_control_end_threshold between 50 and 100%. > > Tested on Lifebook E5411 and Lifebook U728. Sadly I can't test this > patch on a dual battery one, but I didn't find any clue about > independent battery charge control on dual battery Fujitsu notebooks > either. And by that I mean checking the DSDT table of various Lifebook > notebooks and reverse engineering FUJ02E3.dll. > > Signed-off-by: Szilard Fabian <szfabian@bluemarch.art> > --- > v2: > Forgot to sign-off the original commit. Fixed, sorry for the > inconvenience. > --- > drivers/platform/x86/fujitsu-laptop.c | 95 +++++++++++++++++++++++++++ > 1 file changed, 95 insertions(+) > > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c > index 085e044e888e..bf3df74e4d63 100644 > --- a/drivers/platform/x86/fujitsu-laptop.c > +++ b/drivers/platform/x86/fujitsu-laptop.c > @@ -49,6 +49,8 @@ > #include <linux/kfifo.h> > #include <linux/leds.h> > #include <linux/platform_device.h> > +#include <linux/power_supply.h> > +#include <acpi/battery.h> > #include <acpi/video.h> > > #define FUJITSU_DRIVER_VERSION "0.6.0" > @@ -97,6 +99,10 @@ > #define BACKLIGHT_OFF (BIT(0) | BIT(1)) > #define BACKLIGHT_ON 0 > > +/* FUNC interface - battery control interface */ > +#define FUNC_S006_METHOD 0x1006 > +#define CHARGE_CONTROL_RW 0x21 > + > /* Scancodes read from the GIRB register */ > #define KEY1_CODE 0x410 > #define KEY2_CODE 0x411 > @@ -164,6 +170,91 @@ static int call_fext_func(struct acpi_device *device, > return value; > } > > +/* Battery charge control code */ > + > +static ssize_t charge_control_end_threshold_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + int value, ret; > + > + ret = kstrtouint(buf, 10, &value); > + if (ret) > + return ret; > + > + if (value < 50 || value > 100) > + return -EINVAL; > + > + int cc_end_value, s006_cc_return; > + > + cc_end_value = value * 0x100 + 0x20; > + s006_cc_return = call_fext_func(fext, FUNC_S006_METHOD, > + CHARGE_CONTROL_RW, cc_end_value, 0x0); Hi, Error handling is missing for call_fext_func(), as it can return an negative error code. > + > + /* > + * The S006 0x21 method returns 0x00 in case the provided value > + * is invalid. > + */ > + if (s006_cc_return == 0x00) > + return -EINVAL; > + > + return count; > +} > + > +static ssize_t charge_control_end_threshold_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int status; > + status = call_fext_func(fext, FUNC_S006_METHOD, > + CHARGE_CONTROL_RW, 0x21, 0x0); Same as above. > + > + return sprintf(buf, "%d\n", status); > +} > + > +static DEVICE_ATTR_RW(charge_control_end_threshold); > + > +/* ACPI battery hook */ > + > +static int fujitsu_battery_add(struct power_supply *battery, > + struct acpi_battery_hook *hook) > +{ > + /* Check if there is an existing FUJ02E3 ACPI device. */ > + if (fext == NULL) > + return -ENODEV; Can you put the struct acpi_battery_hook into the struct fujitsu_laptop and then use container_of() to retrieve the ACPI device from there? The dell-wmi-ddv driver does something similar. This would guarantee that the battery hook always accesses the correct ACPI device and you could drop this check. > + > + /* > + * Check if the S006 0x21 method exists by trying to get the current > + * battery charge limit. > + */ > + int s006_cc_return; > + s006_cc_return = call_fext_func(fext, FUNC_S006_METHOD, > + CHARGE_CONTROL_RW, 0x21, 0x0); > + if (s006_cc_return == UNSUPPORTED_CMD) > + return -ENODEV; Maybe this check should be done once during probe? > + > + if (device_create_file(&battery->dev, > + &dev_attr_charge_control_end_threshold)) > + return -ENODEV; > + > + return 0; Better to just return the result of device_create_file() here. Thanks, Armin Wolf > +} > + > +static int fujitsu_battery_remove(struct power_supply *battery, > + struct acpi_battery_hook *hook) > +{ > + device_remove_file(&battery->dev, > + &dev_attr_charge_control_end_threshold); > + > + return 0; > +} > + > +static struct acpi_battery_hook battery_hook = { > + .add_battery = fujitsu_battery_add, > + .remove_battery = fujitsu_battery_remove, > + .name = "Fujitsu Battery Extension", > +}; > + > /* Hardware access for LCD brightness control */ > > static int set_lcd_level(struct acpi_device *device, int level) > @@ -839,6 +930,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device) > if (ret) > goto err_free_fifo; > > + battery_hook_register(&battery_hook); > + > return 0; > > err_free_fifo: > @@ -851,6 +944,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device) > { > struct fujitsu_laptop *priv = acpi_driver_data(device); > > + battery_hook_unregister(&battery_hook); > + > fujitsu_laptop_platform_remove(device); > > kfifo_free(&priv->fifo);
Hello, On Tue, Jan 30, 2024 at 03:02:09AM +0100, Armin Wolf wrote: > Am 29.01.24 um 19:00 schrieb Szilard Fabian: > > + > > + return sprintf(buf, "%d\n", status); > > +} > > + > > +static DEVICE_ATTR_RW(charge_control_end_threshold); > > + > > +/* ACPI battery hook */ > > + > > +static int fujitsu_battery_add(struct power_supply *battery, > > + struct acpi_battery_hook *hook) > > +{ > > + /* Check if there is an existing FUJ02E3 ACPI device. */ > > + if (fext == NULL) > > + return -ENODEV; > > Can you put the struct acpi_battery_hook into the struct fujitsu_laptop > and then use container_of() to retrieve the ACPI device from there? > The dell-wmi-ddv driver does something similar. > > This would guarantee that the battery hook always accesses the correct ACPI device > and you could drop this check. > > > + > > + /* > > + * Check if the S006 0x21 method exists by trying to get the current > > + * battery charge limit. > > + */ > > + int s006_cc_return; > > + s006_cc_return = call_fext_func(fext, FUNC_S006_METHOD, > > + CHARGE_CONTROL_RW, 0x21, 0x0); > > + if (s006_cc_return == UNSUPPORTED_CMD) > > + return -ENODEV; > > Maybe this check should be done once during probe? What about the following scenario? - Put a bool into the struct fujitsu_laptop to store information about the machine's charge control ability. - The S006 0x21 method check with `battery_hook_register` gets moved into an 'init function'. In that 'init function' the bool gets set accordingly. - `battery_hook_unregister` gets moved into an 'exit function', where the bool gets read and when it's false nothing happens. - `fext` check gets removed from `fujitsu_battery_add` because it's redundant (more about that later). - The 'init function' gets called in `acpi_fujitsu_laptop_add` and the 'exit function' gets called in `acpi_fujitsu_laptop_remove`. With that scenario the code could be a little bit clearer in my opinion. And it is possible to drop the `fext` check because if the FUJ02E3 ACPI device exists `fext` gets set in the `acpi_fujitsu_laptop_add` function with an error check. (And the `fujitsu_battery_add` `fext` check was already redundant because `battery_hook_register` got called in `acpi_fujitsu_laptop_add`. `fext` gets set in the same function, and there is an error check already.) Thanks, Szilard
Am 03.02.24 um 01:17 schrieb Szilard Fabian: > Hello, > > On Tue, Jan 30, 2024 at 03:02:09AM +0100, Armin Wolf wrote: >> Am 29.01.24 um 19:00 schrieb Szilard Fabian: >>> + >>> + return sprintf(buf, "%d\n", status); >>> +} >>> + >>> +static DEVICE_ATTR_RW(charge_control_end_threshold); >>> + >>> +/* ACPI battery hook */ >>> + >>> +static int fujitsu_battery_add(struct power_supply *battery, >>> + struct acpi_battery_hook *hook) >>> +{ >>> + /* Check if there is an existing FUJ02E3 ACPI device. */ >>> + if (fext == NULL) >>> + return -ENODEV; >> Can you put the struct acpi_battery_hook into the struct fujitsu_laptop >> and then use container_of() to retrieve the ACPI device from there? >> The dell-wmi-ddv driver does something similar. >> >> This would guarantee that the battery hook always accesses the correct ACPI device >> and you could drop this check. >> >>> + >>> + /* >>> + * Check if the S006 0x21 method exists by trying to get the current >>> + * battery charge limit. >>> + */ >>> + int s006_cc_return; >>> + s006_cc_return = call_fext_func(fext, FUNC_S006_METHOD, >>> + CHARGE_CONTROL_RW, 0x21, 0x0); >>> + if (s006_cc_return == UNSUPPORTED_CMD) >>> + return -ENODEV; >> Maybe this check should be done once during probe? > What about the following scenario? > - Put a bool into the struct fujitsu_laptop to store information about the > machine's charge control ability. > - The S006 0x21 method check with `battery_hook_register` gets moved into > an 'init function'. In that 'init function' the bool gets set accordingly. > - `battery_hook_unregister` gets moved into an 'exit function', where the > bool gets read and when it's false nothing happens. > - `fext` check gets removed from `fujitsu_battery_add` because it's > redundant (more about that later). > - The 'init function' gets called in `acpi_fujitsu_laptop_add` and the 'exit > function' gets called in `acpi_fujitsu_laptop_remove`. > > With that scenario the code could be a little bit clearer in my opinion. > And it is possible to drop the `fext` check because if the FUJ02E3 ACPI > device exists `fext` gets set in the `acpi_fujitsu_laptop_add` function with > an error check. > (And the `fujitsu_battery_add` `fext` check was already redundant because > `battery_hook_register` got called in `acpi_fujitsu_laptop_add`. `fext` > gets set in the same function, and there is an error check already.) > > Thanks, > Szilard > This would work too. Armin Wolf
On Mon, Feb 05, 2024 at 06:07:46PM +0100, Armin Wolf wrote: > Am 03.02.24 um 01:17 schrieb Szilard Fabian: > > > Hello, > > > > On Tue, Jan 30, 2024 at 03:02:09AM +0100, Armin Wolf wrote: > > > Am 29.01.24 um 19:00 schrieb Szilard Fabian: > > > > + > > > > + return sprintf(buf, "%d\n", status); > > > > +} > > > > + > > > > +static DEVICE_ATTR_RW(charge_control_end_threshold); > > > > + > > > > +/* ACPI battery hook */ > > > > + > > > > +static int fujitsu_battery_add(struct power_supply *battery, > > > > + struct acpi_battery_hook *hook) > > > > +{ > > > > + /* Check if there is an existing FUJ02E3 ACPI device. */ > > > > + if (fext == NULL) > > > > + return -ENODEV; > > > Can you put the struct acpi_battery_hook into the struct fujitsu_laptop > > > and then use container_of() to retrieve the ACPI device from there? > > > The dell-wmi-ddv driver does something similar. > > > > > > This would guarantee that the battery hook always accesses the correct ACPI device > > > and you could drop this check. > > > > > > > + > > > > + /* > > > > + * Check if the S006 0x21 method exists by trying to get the current > > > > + * battery charge limit. > > > > + */ > > > > + int s006_cc_return; > > > > + s006_cc_return = call_fext_func(fext, FUNC_S006_METHOD, > > > > + CHARGE_CONTROL_RW, 0x21, 0x0); > > > > + if (s006_cc_return == UNSUPPORTED_CMD) > > > > + return -ENODEV; > > > Maybe this check should be done once during probe? > > What about the following scenario? > > - Put a bool into the struct fujitsu_laptop to store information about the > > machine's charge control ability. > > - The S006 0x21 method check with `battery_hook_register` gets moved into > > an 'init function'. In that 'init function' the bool gets set accordingly. > > - `battery_hook_unregister` gets moved into an 'exit function', where the > > bool gets read and when it's false nothing happens. > > - `fext` check gets removed from `fujitsu_battery_add` because it's > > redundant (more about that later). > > - The 'init function' gets called in `acpi_fujitsu_laptop_add` and the 'exit > > function' gets called in `acpi_fujitsu_laptop_remove`. > > > > With that scenario the code could be a little bit clearer in my opinion. > > And it is possible to drop the `fext` check because if the FUJ02E3 ACPI > > device exists `fext` gets set in the `acpi_fujitsu_laptop_add` function with > > an error check. > > (And the `fujitsu_battery_add` `fext` check was already redundant because > > `battery_hook_register` got called in `acpi_fujitsu_laptop_add`. `fext` > > gets set in the same function, and there is an error check already.) > > > > Thanks, > > Szilard > > > This would work too. I'm happy to see this work proceed. Once a revised patch is available I'll test it on my S7020. This should exercise the error recovery code because the functionality being addressed here almost certainly doesn't exist in a laptop as old as the S7020. Yes, my S7020 is still operational and in use. Regards jonathan
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c index 085e044e888e..bf3df74e4d63 100644 --- a/drivers/platform/x86/fujitsu-laptop.c +++ b/drivers/platform/x86/fujitsu-laptop.c @@ -49,6 +49,8 @@ #include <linux/kfifo.h> #include <linux/leds.h> #include <linux/platform_device.h> +#include <linux/power_supply.h> +#include <acpi/battery.h> #include <acpi/video.h> #define FUJITSU_DRIVER_VERSION "0.6.0" @@ -97,6 +99,10 @@ #define BACKLIGHT_OFF (BIT(0) | BIT(1)) #define BACKLIGHT_ON 0 +/* FUNC interface - battery control interface */ +#define FUNC_S006_METHOD 0x1006 +#define CHARGE_CONTROL_RW 0x21 + /* Scancodes read from the GIRB register */ #define KEY1_CODE 0x410 #define KEY2_CODE 0x411 @@ -164,6 +170,91 @@ static int call_fext_func(struct acpi_device *device, return value; } +/* Battery charge control code */ + +static ssize_t charge_control_end_threshold_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int value, ret; + + ret = kstrtouint(buf, 10, &value); + if (ret) + return ret; + + if (value < 50 || value > 100) + return -EINVAL; + + int cc_end_value, s006_cc_return; + + cc_end_value = value * 0x100 + 0x20; + s006_cc_return = call_fext_func(fext, FUNC_S006_METHOD, + CHARGE_CONTROL_RW, cc_end_value, 0x0); + + /* + * The S006 0x21 method returns 0x00 in case the provided value + * is invalid. + */ + if (s006_cc_return == 0x00) + return -EINVAL; + + return count; +} + +static ssize_t charge_control_end_threshold_show(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + int status; + status = call_fext_func(fext, FUNC_S006_METHOD, + CHARGE_CONTROL_RW, 0x21, 0x0); + + return sprintf(buf, "%d\n", status); +} + +static DEVICE_ATTR_RW(charge_control_end_threshold); + +/* ACPI battery hook */ + +static int fujitsu_battery_add(struct power_supply *battery, + struct acpi_battery_hook *hook) +{ + /* Check if there is an existing FUJ02E3 ACPI device. */ + if (fext == NULL) + return -ENODEV; + + /* + * Check if the S006 0x21 method exists by trying to get the current + * battery charge limit. + */ + int s006_cc_return; + s006_cc_return = call_fext_func(fext, FUNC_S006_METHOD, + CHARGE_CONTROL_RW, 0x21, 0x0); + if (s006_cc_return == UNSUPPORTED_CMD) + return -ENODEV; + + if (device_create_file(&battery->dev, + &dev_attr_charge_control_end_threshold)) + return -ENODEV; + + return 0; +} + +static int fujitsu_battery_remove(struct power_supply *battery, + struct acpi_battery_hook *hook) +{ + device_remove_file(&battery->dev, + &dev_attr_charge_control_end_threshold); + + return 0; +} + +static struct acpi_battery_hook battery_hook = { + .add_battery = fujitsu_battery_add, + .remove_battery = fujitsu_battery_remove, + .name = "Fujitsu Battery Extension", +}; + /* Hardware access for LCD brightness control */ static int set_lcd_level(struct acpi_device *device, int level) @@ -839,6 +930,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device) if (ret) goto err_free_fifo; + battery_hook_register(&battery_hook); + return 0; err_free_fifo: @@ -851,6 +944,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device) { struct fujitsu_laptop *priv = acpi_driver_data(device); + battery_hook_unregister(&battery_hook); + fujitsu_laptop_platform_remove(device); kfifo_free(&priv->fifo);
This patch adds battery charge control support on Fujitsu notebooks via the S006 method of the FUJ02E3 ACPI device. With this method it's possible to set charge_control_end_threshold between 50 and 100%. Tested on Lifebook E5411 and Lifebook U728. Sadly I can't test this patch on a dual battery one, but I didn't find any clue about independent battery charge control on dual battery Fujitsu notebooks either. And by that I mean checking the DSDT table of various Lifebook notebooks and reverse engineering FUJ02E3.dll. Signed-off-by: Szilard Fabian <szfabian@bluemarch.art> --- v2: Forgot to sign-off the original commit. Fixed, sorry for the inconvenience. --- drivers/platform/x86/fujitsu-laptop.c | 95 +++++++++++++++++++++++++++ 1 file changed, 95 insertions(+)