[RFC,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 <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(+)
  

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