Re: [PATCH v2 2/3] platform/x86: Battery charge mode in toshiba_acpi (sysfs)

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 9/2/22 20:00, Arvid Norlander wrote:
> This commit adds the ACPI battery hook which in turns adds the sysfs
> entries.
> 
> Because the Toshiba laptops only support two modes (eco or normal), which
> in testing correspond to 80% and 100% we simply round to the nearest
> possible level when set.
> 
> It is possible that Toshiba laptops other than the Z830 has different set
> points for the charging. If so, a quirk table could be introduced in the
> future for this. For now, assume that all laptops that support this feature
> work the same way.
> 
> Tested on a Toshiba Satellite Z830.
> 
> Signed-off-by: Arvid Norlander <lkml@vorpal.se>
> ---
>  drivers/platform/x86/toshiba_acpi.c | 97 +++++++++++++++++++++++++++++
>  1 file changed, 97 insertions(+)
> 
> diff --git a/drivers/platform/x86/toshiba_acpi.c b/drivers/platform/x86/toshiba_acpi.c
> index c927d5d0f8cd..fc953d6bcb93 100644
> --- a/drivers/platform/x86/toshiba_acpi.c
> +++ b/drivers/platform/x86/toshiba_acpi.c
> @@ -44,6 +44,7 @@
>  #include <linux/rfkill.h>
>  #include <linux/iio/iio.h>
>  #include <linux/toshiba.h>
> +#include <acpi/battery.h>
>  #include <acpi/video.h>
>  
>  MODULE_AUTHOR("John Belmonte");
> @@ -2981,6 +2982,92 @@ static int toshiba_acpi_setup_backlight(struct toshiba_acpi_dev *dev)
>  	return 0;
>  }
>  
> +
> +/* ACPI battery hooking */
> +static ssize_t charge_control_end_threshold_show(struct device *device,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	u32 state;
> +	int status;
> +
> +	if (toshiba_acpi == NULL) {
> +		pr_err("Toshiba ACPI object invalid\n");
> +		return -ENODEV;
> +	}

These and the other (toshiba_acpi == NULL) checks are not necessary,
battery_hook_register() is only called after setting toshiba_acpi to non NULL
and battery_hook_unregister() is called before setting it NULL again,
so toshiba_acpi can never be NULL when the callbacks run.

I have removed all the NULL checks while merging this.


> +
> +	status = toshiba_battery_charge_mode_get(toshiba_acpi, &state);
> +
> +	if (status != 0)
> +		return status;
> +
> +	if (state == 1)
> +		return sprintf(buf, "80\n");
> +	else
> +		return sprintf(buf, "100\n");
> +}
> +
> +static ssize_t charge_control_end_threshold_store(struct device *dev,
> +						  struct device_attribute *attr,
> +						  const char *buf,
> +						  size_t count)
> +{
> +	u32 value;
> +	int rval;
> +
> +	if (toshiba_acpi == NULL) {
> +		pr_err("Toshiba ACPI object invalid\n");
> +		return -ENODEV;
> +	}
> +
> +	rval = kstrtou32(buf, 10, &value);
> +	if (rval)
> +		return rval;
> +
> +	if (value < 1 || value > 100)
> +		return -EINVAL;
> +	rval = toshiba_battery_charge_mode_set(toshiba_acpi,
> +					       (value < 90) ? 1 : 0);
> +	if (rval < 0)
> +		return rval;
> +	else
> +		return count;
> +}
> +
> +static DEVICE_ATTR_RW(charge_control_end_threshold);
> +
> +static struct attribute *toshiba_acpi_battery_attrs[] = {
> +	&dev_attr_charge_control_end_threshold.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(toshiba_acpi_battery);
> +
> +static int toshiba_acpi_battery_add(struct power_supply *battery)
> +{
> +	if (toshiba_acpi == NULL) {
> +		pr_err("Init order issue\n");
> +		return -ENODEV;
> +	}
> +	if (!toshiba_acpi->battery_charge_mode_supported)
> +		return -ENODEV;
> +	if (device_add_groups(&battery->dev, toshiba_acpi_battery_groups))
> +		return -ENODEV;
> +	return 0;
> +}
> +
> +static int toshiba_acpi_battery_remove(struct power_supply *battery)
> +{
> +	device_remove_groups(&battery->dev, toshiba_acpi_battery_groups);
> +	return 0;
> +}
> +
> +static struct acpi_battery_hook battery_hook = {
> +	.add_battery = toshiba_acpi_battery_add,
> +	.remove_battery = toshiba_acpi_battery_remove,
> +	.name = "Toshiba Battery Extension",
> +};
> +
>  static void print_supported_features(struct toshiba_acpi_dev *dev)
>  {
>  	pr_info("Supported laptop features:");
> @@ -3063,6 +3150,9 @@ static int toshiba_acpi_remove(struct acpi_device *acpi_dev)
>  		rfkill_destroy(dev->wwan_rfk);
>  	}
>  
> +	if (dev->battery_charge_mode_supported)
> +		battery_hook_unregister(&battery_hook);
> +

battery_hook_[un]register() call code from the acpi_battery
kernel code/module. To make sure those symbols are actually available
we need to add: "depends on ACPI_BATTERY" to config ACPI_TOSHIBA
in Kconfig. I have done this while merging this.

Regards,

Hans




>  	if (toshiba_acpi)
>  		toshiba_acpi = NULL;
>  
> @@ -3246,6 +3336,13 @@ static int toshiba_acpi_add(struct acpi_device *acpi_dev)
>  
>  	toshiba_acpi = dev;
>  
> +	/*
> +	 * As the battery hook relies on the static variable toshiba_acpi being
> +	 * set, this must be done after toshiba_acpi is assigned.
> +	 */
> +	if (dev->battery_charge_mode_supported)
> +		battery_hook_register(&battery_hook);
> +
>  	return 0;
>  
>  error: