2024-01-29 16:41:32

by Szilard Fabian

[permalink] [raw]
Subject: [RFC PATCH] platform/x86/fujitsu-laptop: Add battery charge control support

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.
---
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);
+
+ /*
+ * 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);
--
2.43.0




2024-01-29 18:01:46

by Szilard Fabian

[permalink] [raw]
Subject: [RFC PATCH v2] platform/x86/fujitsu-laptop: Add battery charge control support

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 <[email protected]>
---
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);
+
+ /*
+ * 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);
--
2.43.0



2024-01-30 02:03:20

by Armin Wolf

[permalink] [raw]
Subject: Re: [RFC PATCH v2] platform/x86/fujitsu-laptop: Add battery charge control support

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 <[email protected]>
> ---
> 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);

2024-02-03 00:18:35

by Szilard Fabian

[permalink] [raw]
Subject: Re: [RFC PATCH v2] platform/x86/fujitsu-laptop: Add battery charge control support

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



2024-02-05 17:17:17

by Armin Wolf

[permalink] [raw]
Subject: Re: [RFC PATCH v2] platform/x86/fujitsu-laptop: Add battery charge control support

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


2024-02-05 23:38:26

by Jonathan Woithe

[permalink] [raw]
Subject: Re: [RFC PATCH v2] platform/x86/fujitsu-laptop: Add battery charge control support

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

2024-02-07 02:32:44

by Szilard Fabian

[permalink] [raw]
Subject: [RFC PATCH v3] platform/x86/fujitsu-laptop: Add battery charge control support

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 <[email protected]>
---
v3:
* added additional error handling
* removed if statement with device_create_file(), just returning that
function instead
* added bool charge_control_supported into struct fujitsu_laptop
* added a 'charge_control_add' and 'charge_control_remove' function to be
called from acpi_fujitsu_laptop_add() and acpi_fujitsu_laptop_remove()
* moved FUJ02E3 S006 probing logic from the ACPI battery hooks to the new
'charge_control_*' functions

v2:
Forgot to sign-off the original commit. Fixed, sorry for the
inconvenience.
---
drivers/platform/x86/fujitsu-laptop.c | 125 ++++++++++++++++++++++++++
1 file changed, 125 insertions(+)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 085e044e888e..2fcbc10a0d9d 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
@@ -132,6 +138,7 @@ struct fujitsu_laptop {
spinlock_t fifo_lock;
int flags_supported;
int flags_state;
+ bool charge_control_supported;
};

static struct acpi_device *fext;
@@ -164,6 +171,118 @@ 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);
+
+ if (s006_cc_return < 0)
+ return s006_cc_return;
+
+ /*
+ * 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);
+
+ if (status < 0)
+ return status;
+
+ return sprintf(buf, "%d\n", status);
+}
+
+static DEVICE_ATTR_RW(charge_control_end_threshold);
+
+/* ACPI battery hook */
+
+static int fujitsu_battery_add_hook(struct power_supply *battery,
+ struct acpi_battery_hook *hook)
+{
+ return device_create_file(&battery->dev,
+ &dev_attr_charge_control_end_threshold);
+}
+
+static int fujitsu_battery_remove_hook(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_hook,
+ .remove_battery = fujitsu_battery_remove_hook,
+ .name = "Fujitsu Battery Extension",
+};
+
+/*
+ * These functions are intended to be called from acpi_fujitsu_laptop_add and
+ * acpi_fujitsu_laptop_remove.
+ */
+
+static int fujitsu_battery_charge_control_add(struct acpi_device *device)
+{
+ struct fujitsu_laptop *priv = acpi_driver_data(device);
+ priv->charge_control_supported = false;
+
+ /*
+ * 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 < 0)
+ return s006_cc_return;
+
+ if (s006_cc_return == UNSUPPORTED_CMD)
+ return -ENODEV;
+
+ priv->charge_control_supported = true;
+ battery_hook_register(&battery_hook);
+
+ return 0;
+}
+
+static void fujitsu_battery_charge_control_remove(struct acpi_device *device)
+{
+ struct fujitsu_laptop *priv = acpi_driver_data(device);
+
+ if (priv->charge_control_supported)
+ battery_hook_unregister(&battery_hook);
+}
+
/* Hardware access for LCD brightness control */

static int set_lcd_level(struct acpi_device *device, int level)
@@ -839,6 +958,10 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
if (ret)
goto err_free_fifo;

+ ret = fujitsu_battery_charge_control_add(device);
+ if (ret < 0)
+ pr_warn("Unable to register battery charge control: %d\n", ret);
+
return 0;

err_free_fifo:
@@ -851,6 +974,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device)
{
struct fujitsu_laptop *priv = acpi_driver_data(device);

+ fujitsu_battery_charge_control_remove(device);
+
fujitsu_laptop_platform_remove(device);

kfifo_free(&priv->fifo);
--
2.43.0



2024-02-07 08:57:47

by Hans de Goede

[permalink] [raw]
Subject: Re: [RFC PATCH v3] platform/x86/fujitsu-laptop: Add battery charge control support

Hi Szilard,

+Cc Jelle van der Waa who has been working on userspace (upower + GNOME)
support for these thresholds.

On 2/7/24 03:32, Szilard Fabian wrote:
> 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 <[email protected]>

Thank you for your patch. Do you happen to know if there also
is a noticeable fixed start threshold which is like say always 5%
lower then then end threshold ?

Note I'm *not* asking you to also add a start threshold attribute
at this time. But we (Jelle and me mostly atm) are thinking about
a way to export the start threshold offset in cases like this
to userspace.

Regards,

Hans



> ---
> v3:
> * added additional error handling
> * removed if statement with device_create_file(), just returning that
> function instead
> * added bool charge_control_supported into struct fujitsu_laptop
> * added a 'charge_control_add' and 'charge_control_remove' function to be
> called from acpi_fujitsu_laptop_add() and acpi_fujitsu_laptop_remove()
> * moved FUJ02E3 S006 probing logic from the ACPI battery hooks to the new
> 'charge_control_*' functions
>
> v2:
> Forgot to sign-off the original commit. Fixed, sorry for the
> inconvenience.
> ---
> drivers/platform/x86/fujitsu-laptop.c | 125 ++++++++++++++++++++++++++
> 1 file changed, 125 insertions(+)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 085e044e888e..2fcbc10a0d9d 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
> @@ -132,6 +138,7 @@ struct fujitsu_laptop {
> spinlock_t fifo_lock;
> int flags_supported;
> int flags_state;
> + bool charge_control_supported;
> };
>
> static struct acpi_device *fext;
> @@ -164,6 +171,118 @@ 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);
> +
> + if (s006_cc_return < 0)
> + return s006_cc_return;
> +
> + /*
> + * 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);
> +
> + if (status < 0)
> + return status;
> +
> + return sprintf(buf, "%d\n", status);
> +}
> +
> +static DEVICE_ATTR_RW(charge_control_end_threshold);
> +
> +/* ACPI battery hook */
> +
> +static int fujitsu_battery_add_hook(struct power_supply *battery,
> + struct acpi_battery_hook *hook)
> +{
> + return device_create_file(&battery->dev,
> + &dev_attr_charge_control_end_threshold);
> +}
> +
> +static int fujitsu_battery_remove_hook(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_hook,
> + .remove_battery = fujitsu_battery_remove_hook,
> + .name = "Fujitsu Battery Extension",
> +};
> +
> +/*
> + * These functions are intended to be called from acpi_fujitsu_laptop_add and
> + * acpi_fujitsu_laptop_remove.
> + */
> +
> +static int fujitsu_battery_charge_control_add(struct acpi_device *device)
> +{
> + struct fujitsu_laptop *priv = acpi_driver_data(device);
> + priv->charge_control_supported = false;
> +
> + /*
> + * 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 < 0)
> + return s006_cc_return;
> +
> + if (s006_cc_return == UNSUPPORTED_CMD)
> + return -ENODEV;
> +
> + priv->charge_control_supported = true;
> + battery_hook_register(&battery_hook);
> +
> + return 0;
> +}
> +
> +static void fujitsu_battery_charge_control_remove(struct acpi_device *device)
> +{
> + struct fujitsu_laptop *priv = acpi_driver_data(device);
> +
> + if (priv->charge_control_supported)
> + battery_hook_unregister(&battery_hook);
> +}
> +
> /* Hardware access for LCD brightness control */
>
> static int set_lcd_level(struct acpi_device *device, int level)
> @@ -839,6 +958,10 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> if (ret)
> goto err_free_fifo;
>
> + ret = fujitsu_battery_charge_control_add(device);
> + if (ret < 0)
> + pr_warn("Unable to register battery charge control: %d\n", ret);
> +
> return 0;
>
> err_free_fifo:
> @@ -851,6 +974,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device)
> {
> struct fujitsu_laptop *priv = acpi_driver_data(device);
>
> + fujitsu_battery_charge_control_remove(device);
> +
> fujitsu_laptop_platform_remove(device);
>
> kfifo_free(&priv->fifo);


2024-02-08 01:56:38

by Szilard Fabian

[permalink] [raw]
Subject: Re: [RFC PATCH v3] platform/x86/fujitsu-laptop: Add battery charge control support

Hi,

On Wed, Feb 07, 2024 at 09:57:21AM +0100, Hans de Goede wrote:
> Thank you for your patch. Do you happen to know if there also
> is a noticeable fixed start threshold which is like say always 5%
> lower then then end threshold ?
There is a start threshold, it's always 11% lower than the end threshold.
I don't know yet if it's a fixed value or if there is a way to set it to a
different value. If I find something usable about that I'm more than willing
to create an another patch.

Regards,
Szilard



2024-02-08 09:12:19

by Hans de Goede

[permalink] [raw]
Subject: Re: [RFC PATCH v3] platform/x86/fujitsu-laptop: Add battery charge control support

Hi Szilard,

On 2/8/24 02:56, Szilard Fabian wrote:
> Hi,
>
> On Wed, Feb 07, 2024 at 09:57:21AM +0100, Hans de Goede wrote:
>> Thank you for your patch. Do you happen to know if there also
>> is a noticeable fixed start threshold which is like say always 5%
>> lower then then end threshold ?
> There is a start threshold, it's always 11% lower than the end threshold.
> I don't know yet if it's a fixed value or if there is a way to set it to a
> different value.

Thanks that is useful to know. We can add this to the driver once we have
come up with a way to export these kind of fixed start offsets to userspace.

Regards,

Hans



2024-02-15 12:47:04

by Ilpo Järvinen

[permalink] [raw]
Subject: Re: [RFC PATCH v3] platform/x86/fujitsu-laptop: Add battery charge control support

On Wed, 7 Feb 2024, Szilard Fabian wrote:

> 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 <[email protected]>
> ---
> v3:
> * added additional error handling
> * removed if statement with device_create_file(), just returning that
> function instead
> * added bool charge_control_supported into struct fujitsu_laptop
> * added a 'charge_control_add' and 'charge_control_remove' function to be
> called from acpi_fujitsu_laptop_add() and acpi_fujitsu_laptop_remove()
> * moved FUJ02E3 S006 probing logic from the ACPI battery hooks to the new
> 'charge_control_*' functions
>
> v2:
> Forgot to sign-off the original commit. Fixed, sorry for the
> inconvenience.
> ---
> drivers/platform/x86/fujitsu-laptop.c | 125 ++++++++++++++++++++++++++
> 1 file changed, 125 insertions(+)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 085e044e888e..2fcbc10a0d9d 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
> @@ -132,6 +138,7 @@ struct fujitsu_laptop {
> spinlock_t fifo_lock;
> int flags_supported;
> int flags_state;
> + bool charge_control_supported;
> };
>
> static struct acpi_device *fext;
> @@ -164,6 +171,118 @@ static int call_fext_func(struct acpi_device *device,
> return value;
> }
>
> +/* Battery charge control code */
> +

Please remove these empty lines between the comment and function (not
just this but the others too).

> +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);
> +
> + if (s006_cc_return < 0)
> + return s006_cc_return;
> +
> + /*
> + * 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);

Add empty line after declaration.

> +
> + if (status < 0)
> + return status;
> +
> + return sprintf(buf, "%d\n", status);

sysfs_emit()

> +}
> +
> +static DEVICE_ATTR_RW(charge_control_end_threshold);
> +
> +/* ACPI battery hook */
> +
> +static int fujitsu_battery_add_hook(struct power_supply *battery,
> + struct acpi_battery_hook *hook)
> +{
> + return device_create_file(&battery->dev,
> + &dev_attr_charge_control_end_threshold);
> +}
> +
> +static int fujitsu_battery_remove_hook(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_hook,
> + .remove_battery = fujitsu_battery_remove_hook,
> + .name = "Fujitsu Battery Extension",
> +};
> +
> +/*
> + * These functions are intended to be called from acpi_fujitsu_laptop_add and
> + * acpi_fujitsu_laptop_remove.
> + */
> +
> +static int fujitsu_battery_charge_control_add(struct acpi_device *device)
> +{
> + struct fujitsu_laptop *priv = acpi_driver_data(device);
> + priv->charge_control_supported = false;

Add a newline between these too as well.

> + /*
> + * 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 < 0)
> + return s006_cc_return;
> +
> + if (s006_cc_return == UNSUPPORTED_CMD)
> + return -ENODEV;
> +
> + priv->charge_control_supported = true;
> + battery_hook_register(&battery_hook);
> +
> + return 0;
> +}
> +
> +static void fujitsu_battery_charge_control_remove(struct acpi_device *device)
> +{
> + struct fujitsu_laptop *priv = acpi_driver_data(device);
> +
> + if (priv->charge_control_supported)
> + battery_hook_unregister(&battery_hook);
> +}
> +
> /* Hardware access for LCD brightness control */
>
> static int set_lcd_level(struct acpi_device *device, int level)
> @@ -839,6 +958,10 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> if (ret)
> goto err_free_fifo;
>
> + ret = fujitsu_battery_charge_control_add(device);
> + if (ret < 0)
> + pr_warn("Unable to register battery charge control: %d\n", ret);
> +
> return 0;
>
> err_free_fifo:
> @@ -851,6 +974,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device)
> {
> struct fujitsu_laptop *priv = acpi_driver_data(device);
>
> + fujitsu_battery_charge_control_remove(device);
> +
> fujitsu_laptop_platform_remove(device);
>
> kfifo_free(&priv->fifo);

Why is this posted as RFC?

--
i.


2024-02-15 20:20:54

by Szilard Fabian

[permalink] [raw]
Subject: Re: [RFC PATCH v3] platform/x86/fujitsu-laptop: Add battery charge control support

Hi,

On Thu, Feb 15, 2024 at 12:34:00PM +0100, Ilpo Järvinen wrote:
> Why is this posted as RFC?
It's my third patch into the Linux kernel (the first two being a i8042 quirk
table related patch) and I was not sure if everything is okay with v1/v2.
But it looks like the patch matured enough in v3 to reach it's near final
form and there was not too much to comment anymore. Sorry for any
inconvenience I've caused with that.

Thanks for your feedback. I'll post v4 in a moment.

Regards,
Szilard



2024-02-15 20:32:21

by Szilard Fabian

[permalink] [raw]
Subject: [PATCH v4] platform/x86/fujitsu-laptop: Add battery charge control support

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 <[email protected]>
---
v4:
* formatting fixes
* replaced sprintf() with sysfs_emit()

v3:
* added additional error handling
* removed if statement with device_create_file(), just returning that
function instead
* added bool charge_control_supported into struct fujitsu_laptop
* added a 'charge_control_add' and 'charge_control_remove' function to be
called from acpi_fujitsu_laptop_add() and acpi_fujitsu_laptop_remove()
* moved FUJ02E3 S006 probing logic from the ACPI battery hooks to the new
'charge_control_*' functions

v2:
Forgot to sign-off the original commit. Fixed, sorry for the
inconvenience.
---
drivers/platform/x86/fujitsu-laptop.c | 125 ++++++++++++++++++++++++++
1 file changed, 125 insertions(+)

diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 085e044e888e..69f9730bb14a 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
@@ -132,6 +138,7 @@ struct fujitsu_laptop {
spinlock_t fifo_lock;
int flags_supported;
int flags_state;
+ bool charge_control_supported;
};

static struct acpi_device *fext;
@@ -164,6 +171,118 @@ 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);
+
+ if (s006_cc_return < 0)
+ return s006_cc_return;
+
+ /*
+ * 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);
+
+ if (status < 0)
+ return status;
+
+ return sysfs_emit(buf, "%d\n", status);
+}
+
+static DEVICE_ATTR_RW(charge_control_end_threshold);
+
+/* ACPI battery hook */
+static int fujitsu_battery_add_hook(struct power_supply *battery,
+ struct acpi_battery_hook *hook)
+{
+ return device_create_file(&battery->dev,
+ &dev_attr_charge_control_end_threshold);
+}
+
+static int fujitsu_battery_remove_hook(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_hook,
+ .remove_battery = fujitsu_battery_remove_hook,
+ .name = "Fujitsu Battery Extension",
+};
+
+/*
+ * These functions are intended to be called from acpi_fujitsu_laptop_add and
+ * acpi_fujitsu_laptop_remove.
+ */
+static int fujitsu_battery_charge_control_add(struct acpi_device *device)
+{
+ struct fujitsu_laptop *priv = acpi_driver_data(device);
+
+ priv->charge_control_supported = false;
+
+ /*
+ * 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 < 0)
+ return s006_cc_return;
+
+ if (s006_cc_return == UNSUPPORTED_CMD)
+ return -ENODEV;
+
+ priv->charge_control_supported = true;
+ battery_hook_register(&battery_hook);
+
+ return 0;
+}
+
+static void fujitsu_battery_charge_control_remove(struct acpi_device *device)
+{
+ struct fujitsu_laptop *priv = acpi_driver_data(device);
+
+ if (priv->charge_control_supported)
+ battery_hook_unregister(&battery_hook);
+}
+
/* Hardware access for LCD brightness control */

static int set_lcd_level(struct acpi_device *device, int level)
@@ -839,6 +958,10 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
if (ret)
goto err_free_fifo;

+ ret = fujitsu_battery_charge_control_add(device);
+ if (ret < 0)
+ pr_warn("Unable to register battery charge control: %d\n", ret);
+
return 0;

err_free_fifo:
@@ -851,6 +974,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device)
{
struct fujitsu_laptop *priv = acpi_driver_data(device);

+ fujitsu_battery_charge_control_remove(device);
+
fujitsu_laptop_platform_remove(device);

kfifo_free(&priv->fifo);
--
2.43.1



2024-02-16 09:27:59

by Ilpo Järvinen

[permalink] [raw]
Subject: Re: [RFC PATCH v3] platform/x86/fujitsu-laptop: Add battery charge control support

On Thu, 15 Feb 2024, Szilard Fabian wrote:
> On Thu, Feb 15, 2024 at 12:34:00PM +0100, Ilpo Järvinen wrote:
> > Why is this posted as RFC?
>
> It's my third patch into the Linux kernel (the first two being a i8042 quirk
> table related patch) and I was not sure if everything is okay with v1/v2.
> But it looks like the patch matured enough in v3 to reach it's near final
> form and there was not too much to comment anymore. Sorry for any
> inconvenience I've caused with that.

No problem. I asked just to gauge if there was something you still planned
to do in addition to what is there. I kind of guessed that in this case
"RFC" mostly meant "I'm being careful here" more than that you'd have
something you felt still missing from the patch. :-)

--
i.

> Thanks for your feedback. I'll post v4 in a moment.
>
> Regards,
> Szilard
>
>

2024-02-18 04:52:05

by Jonathan Woithe

[permalink] [raw]
Subject: Re: [PATCH v4] platform/x86/fujitsu-laptop: Add battery charge control support

On Thu, Feb 15, 2024 at 08:31:43PM +0000, Szilard Fabian wrote:
> 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 <[email protected]>

For various reasons it's going to take me more time than I had hoped to be
in a position to test this patch on the Fujitsu S7020. However, the idea
behind the patch is good and it appears to have the necessary tests in place
to cope with models such as the S7020 which don't include the battery charge
control support. I would therefore be happy to see this patch (or a
subsequent revision) go into the kernel.

Acked-by: Jonathan Woithe <[email protected]>

> ---
> v4:
> * formatting fixes
> * replaced sprintf() with sysfs_emit()
>
> v3:
> * added additional error handling
> * removed if statement with device_create_file(), just returning that
> function instead
> * added bool charge_control_supported into struct fujitsu_laptop
> * added a 'charge_control_add' and 'charge_control_remove' function to be
> called from acpi_fujitsu_laptop_add() and acpi_fujitsu_laptop_remove()
> * moved FUJ02E3 S006 probing logic from the ACPI battery hooks to the new
> 'charge_control_*' functions
>
> v2:
> Forgot to sign-off the original commit. Fixed, sorry for the
> inconvenience.
> ---
> drivers/platform/x86/fujitsu-laptop.c | 125 ++++++++++++++++++++++++++
> 1 file changed, 125 insertions(+)
>
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 085e044e888e..69f9730bb14a 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
> @@ -132,6 +138,7 @@ struct fujitsu_laptop {
> spinlock_t fifo_lock;
> int flags_supported;
> int flags_state;
> + bool charge_control_supported;
> };
>
> static struct acpi_device *fext;
> @@ -164,6 +171,118 @@ 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);
> +
> + if (s006_cc_return < 0)
> + return s006_cc_return;
> +
> + /*
> + * 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);
> +
> + if (status < 0)
> + return status;
> +
> + return sysfs_emit(buf, "%d\n", status);
> +}
> +
> +static DEVICE_ATTR_RW(charge_control_end_threshold);
> +
> +/* ACPI battery hook */
> +static int fujitsu_battery_add_hook(struct power_supply *battery,
> + struct acpi_battery_hook *hook)
> +{
> + return device_create_file(&battery->dev,
> + &dev_attr_charge_control_end_threshold);
> +}
> +
> +static int fujitsu_battery_remove_hook(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_hook,
> + .remove_battery = fujitsu_battery_remove_hook,
> + .name = "Fujitsu Battery Extension",
> +};
> +
> +/*
> + * These functions are intended to be called from acpi_fujitsu_laptop_add and
> + * acpi_fujitsu_laptop_remove.
> + */
> +static int fujitsu_battery_charge_control_add(struct acpi_device *device)
> +{
> + struct fujitsu_laptop *priv = acpi_driver_data(device);
> +
> + priv->charge_control_supported = false;
> +
> + /*
> + * 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 < 0)
> + return s006_cc_return;
> +
> + if (s006_cc_return == UNSUPPORTED_CMD)
> + return -ENODEV;
> +
> + priv->charge_control_supported = true;
> + battery_hook_register(&battery_hook);
> +
> + return 0;
> +}
> +
> +static void fujitsu_battery_charge_control_remove(struct acpi_device *device)
> +{
> + struct fujitsu_laptop *priv = acpi_driver_data(device);
> +
> + if (priv->charge_control_supported)
> + battery_hook_unregister(&battery_hook);
> +}
> +
> /* Hardware access for LCD brightness control */
>
> static int set_lcd_level(struct acpi_device *device, int level)
> @@ -839,6 +958,10 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> if (ret)
> goto err_free_fifo;
>
> + ret = fujitsu_battery_charge_control_add(device);
> + if (ret < 0)
> + pr_warn("Unable to register battery charge control: %d\n", ret);
> +
> return 0;
>
> err_free_fifo:
> @@ -851,6 +974,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device)
> {
> struct fujitsu_laptop *priv = acpi_driver_data(device);
>
> + fujitsu_battery_charge_control_remove(device);
> +
> fujitsu_laptop_platform_remove(device);
>
> kfifo_free(&priv->fifo);
> --
> 2.43.1
>
>

2024-02-19 14:36:36

by Ilpo Järvinen

[permalink] [raw]
Subject: Re: [PATCH v4] platform/x86/fujitsu-laptop: Add battery charge control support

On Thu, 15 Feb 2024 20:31:43 +0000, Szilard Fabian wrote:

> 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.
>
> [...]


Thank you for your contribution, it has been applied to my local
review-ilpo branch. Note it will show up in the public
platform-drivers-x86/review-ilpo branch only once I've pushed my
local branch there, which might take a while.

The list of commits applied:
[1/1] platform/x86/fujitsu-laptop: Add battery charge control support
commit: 9b716ef48c90446b8d6d86726c6c3f0141e53091

--
i.


2024-02-27 10:18:30

by Ilpo Järvinen

[permalink] [raw]
Subject: Re: [PATCH v4] platform/x86/fujitsu-laptop: Add battery charge control support

On Mon, 19 Feb 2024, Ilpo J?rvinen wrote:

> On Thu, 15 Feb 2024 20:31:43 +0000, Szilard Fabian wrote:
>
> > 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.
> >
> > [...]
>
>
> Thank you for your contribution, it has been applied to my local
> review-ilpo branch. Note it will show up in the public
> platform-drivers-x86/review-ilpo branch only once I've pushed my
> local branch there, which might take a while.
>
> The list of commits applied:
> [1/1] platform/x86/fujitsu-laptop: Add battery charge control support
> commit: 9b716ef48c90446b8d6d86726c6c3f0141e53091

Hi,

Can you please take a look at the two build failures LKP has brought up.
They looked like some depends on and/or select clauses missing from
Kconfig.

I'll be merging the patches that fix those into the original commit so
don't spend too much fine-tuning the commit message.

--
i.

2024-02-27 12:05:09

by Szilard Fabian

[permalink] [raw]
Subject: Re: [PATCH v4] platform/x86/fujitsu-laptop: Add battery charge control support

On Tue, Feb 27, 2024 at 11:18:16AM +0100, Ilpo Järvinen wrote:
> Hi,
>
> Can you please take a look at the two build failures LKP has brought up.
> They looked like some depends on and/or select clauses missing from
> Kconfig.
Hi,

Modifying drivers/platform/x86/Kconfig to include `depends on ACPI_BATTERY`
in the FUJITSU_LAPTOP config block fixes the problem with gcc. I think this
should fix the problem with clang too.

I'll post v5 with that change in a moment.

Regards,
Szilard



2024-02-27 12:06:08

by Szilard Fabian

[permalink] [raw]
Subject: [PATCH v5] platform/x86/fujitsu-laptop: Add battery charge control support

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 <[email protected]>
---
v5:
* add ACPI_BATTERY dependency into Kconfig

v4:
* formatting fixes
* replaced sprintf() with sysfs_emit()

v3:
* added additional error handling
* removed if statement with device_create_file(), just returning that
function instead
* added bool charge_control_supported into struct fujitsu_laptop
* added a 'charge_control_add' and 'charge_control_remove' function to be
called from acpi_fujitsu_laptop_add() and acpi_fujitsu_laptop_remove()
* moved FUJ02E3 S006 probing logic from the ACPI battery hooks to the new
'charge_control_*' functions

v2:
Forgot to sign-off the original commit. Fixed, sorry for the
inconvenience.
---
drivers/platform/x86/Kconfig | 1 +
drivers/platform/x86/fujitsu-laptop.c | 125 ++++++++++++++++++++++++++
2 files changed, 126 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index bdd302274b9a..945295f98560 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -374,6 +374,7 @@ config FUJITSU_LAPTOP
depends on ACPI
depends on INPUT
depends on BACKLIGHT_CLASS_DEVICE
+ depends on ACPI_BATTERY
depends on ACPI_VIDEO || ACPI_VIDEO = n
select INPUT_SPARSEKMAP
select NEW_LEDS
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
index 085e044e888e..69f9730bb14a 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
@@ -132,6 +138,7 @@ struct fujitsu_laptop {
spinlock_t fifo_lock;
int flags_supported;
int flags_state;
+ bool charge_control_supported;
};

static struct acpi_device *fext;
@@ -164,6 +171,118 @@ 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);
+
+ if (s006_cc_return < 0)
+ return s006_cc_return;
+
+ /*
+ * 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);
+
+ if (status < 0)
+ return status;
+
+ return sysfs_emit(buf, "%d\n", status);
+}
+
+static DEVICE_ATTR_RW(charge_control_end_threshold);
+
+/* ACPI battery hook */
+static int fujitsu_battery_add_hook(struct power_supply *battery,
+ struct acpi_battery_hook *hook)
+{
+ return device_create_file(&battery->dev,
+ &dev_attr_charge_control_end_threshold);
+}
+
+static int fujitsu_battery_remove_hook(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_hook,
+ .remove_battery = fujitsu_battery_remove_hook,
+ .name = "Fujitsu Battery Extension",
+};
+
+/*
+ * These functions are intended to be called from acpi_fujitsu_laptop_add and
+ * acpi_fujitsu_laptop_remove.
+ */
+static int fujitsu_battery_charge_control_add(struct acpi_device *device)
+{
+ struct fujitsu_laptop *priv = acpi_driver_data(device);
+
+ priv->charge_control_supported = false;
+
+ /*
+ * 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 < 0)
+ return s006_cc_return;
+
+ if (s006_cc_return == UNSUPPORTED_CMD)
+ return -ENODEV;
+
+ priv->charge_control_supported = true;
+ battery_hook_register(&battery_hook);
+
+ return 0;
+}
+
+static void fujitsu_battery_charge_control_remove(struct acpi_device *device)
+{
+ struct fujitsu_laptop *priv = acpi_driver_data(device);
+
+ if (priv->charge_control_supported)
+ battery_hook_unregister(&battery_hook);
+}
+
/* Hardware access for LCD brightness control */

static int set_lcd_level(struct acpi_device *device, int level)
@@ -839,6 +958,10 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
if (ret)
goto err_free_fifo;

+ ret = fujitsu_battery_charge_control_add(device);
+ if (ret < 0)
+ pr_warn("Unable to register battery charge control: %d\n", ret);
+
return 0;

err_free_fifo:
@@ -851,6 +974,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device)
{
struct fujitsu_laptop *priv = acpi_driver_data(device);

+ fujitsu_battery_charge_control_remove(device);
+
fujitsu_laptop_platform_remove(device);

kfifo_free(&priv->fifo);
--
2.44.0



2024-02-27 12:54:42

by Ilpo Järvinen

[permalink] [raw]
Subject: Re: [PATCH v5] platform/x86/fujitsu-laptop: Add battery charge control support

On Tue, 27 Feb 2024, Szilard Fabian wrote:

> 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 <[email protected]>
> ---
> v5:
> * add ACPI_BATTERY dependency into Kconfig

Thanks. My intention was that you'd send a new patch on top of what is
already applied. But it doesn't matter anymore, I took the relevant line
out of it and squashed it into the original commit.


--
i.


2024-02-27 22:12:55

by Jonathan Woithe

[permalink] [raw]
Subject: Re: [PATCH v5] platform/x86/fujitsu-laptop: Add battery charge control support

On Tue, Feb 27, 2024 at 12:05:45PM +0000, Szilard Fabian wrote:
> 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 <[email protected]>

Acked-by: Jonathan Woithe <[email protected]>

> v5:
> * add ACPI_BATTERY dependency into Kconfig
>
> v4:
> * formatting fixes
> * replaced sprintf() with sysfs_emit()
>
> v3:
> * added additional error handling
> * removed if statement with device_create_file(), just returning that
> function instead
> * added bool charge_control_supported into struct fujitsu_laptop
> * added a 'charge_control_add' and 'charge_control_remove' function to be
> called from acpi_fujitsu_laptop_add() and acpi_fujitsu_laptop_remove()
> * moved FUJ02E3 S006 probing logic from the ACPI battery hooks to the new
> 'charge_control_*' functions
>
> v2:
> Forgot to sign-off the original commit. Fixed, sorry for the
> inconvenience.
> ---
> drivers/platform/x86/Kconfig | 1 +
> drivers/platform/x86/fujitsu-laptop.c | 125 ++++++++++++++++++++++++++
> 2 files changed, 126 insertions(+)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index bdd302274b9a..945295f98560 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -374,6 +374,7 @@ config FUJITSU_LAPTOP
> depends on ACPI
> depends on INPUT
> depends on BACKLIGHT_CLASS_DEVICE
> + depends on ACPI_BATTERY
> depends on ACPI_VIDEO || ACPI_VIDEO = n
> select INPUT_SPARSEKMAP
> select NEW_LEDS
> diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c
> index 085e044e888e..69f9730bb14a 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
> @@ -132,6 +138,7 @@ struct fujitsu_laptop {
> spinlock_t fifo_lock;
> int flags_supported;
> int flags_state;
> + bool charge_control_supported;
> };
>
> static struct acpi_device *fext;
> @@ -164,6 +171,118 @@ 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);
> +
> + if (s006_cc_return < 0)
> + return s006_cc_return;
> +
> + /*
> + * 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);
> +
> + if (status < 0)
> + return status;
> +
> + return sysfs_emit(buf, "%d\n", status);
> +}
> +
> +static DEVICE_ATTR_RW(charge_control_end_threshold);
> +
> +/* ACPI battery hook */
> +static int fujitsu_battery_add_hook(struct power_supply *battery,
> + struct acpi_battery_hook *hook)
> +{
> + return device_create_file(&battery->dev,
> + &dev_attr_charge_control_end_threshold);
> +}
> +
> +static int fujitsu_battery_remove_hook(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_hook,
> + .remove_battery = fujitsu_battery_remove_hook,
> + .name = "Fujitsu Battery Extension",
> +};
> +
> +/*
> + * These functions are intended to be called from acpi_fujitsu_laptop_add and
> + * acpi_fujitsu_laptop_remove.
> + */
> +static int fujitsu_battery_charge_control_add(struct acpi_device *device)
> +{
> + struct fujitsu_laptop *priv = acpi_driver_data(device);
> +
> + priv->charge_control_supported = false;
> +
> + /*
> + * 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 < 0)
> + return s006_cc_return;
> +
> + if (s006_cc_return == UNSUPPORTED_CMD)
> + return -ENODEV;
> +
> + priv->charge_control_supported = true;
> + battery_hook_register(&battery_hook);
> +
> + return 0;
> +}
> +
> +static void fujitsu_battery_charge_control_remove(struct acpi_device *device)
> +{
> + struct fujitsu_laptop *priv = acpi_driver_data(device);
> +
> + if (priv->charge_control_supported)
> + battery_hook_unregister(&battery_hook);
> +}
> +
> /* Hardware access for LCD brightness control */
>
> static int set_lcd_level(struct acpi_device *device, int level)
> @@ -839,6 +958,10 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> if (ret)
> goto err_free_fifo;
>
> + ret = fujitsu_battery_charge_control_add(device);
> + if (ret < 0)
> + pr_warn("Unable to register battery charge control: %d\n", ret);
> +
> return 0;
>
> err_free_fifo:
> @@ -851,6 +974,8 @@ static void acpi_fujitsu_laptop_remove(struct acpi_device *device)
> {
> struct fujitsu_laptop *priv = acpi_driver_data(device);
>
> + fujitsu_battery_charge_control_remove(device);
> +
> fujitsu_laptop_platform_remove(device);
>
> kfifo_free(&priv->fifo);
> --
> 2.44.0
>
>