Message-ID: <31a528b8-8318-dc09-3a06-80f76771744a@redhat.com>
Date:   Thu, 23 Dec 2021 17:36:24 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH] ACPI: battery: Add the ThinkPad "Not Charging" quirk
Content-Language: en-US
To:     =?UTF-8?Q?Thomas_Wei=c3=9fschuh?= <linux@weissschuh.net>,
        linux-acpi@vger.kernel.org, Len Brown <lenb@kernel.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>
Cc:     linux-kernel@vger.kernel.org, ibm-acpi-devel@lists.sourceforge.net,
        platform-driver-x86@vger.kernel.org, markpearson@lenovo.com,
        Andy Shevchenko <andy.shevchenko@gmail.com>,
        Ognjen Galic <smclt30p@gmail.com>,
        "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
        Bastien Nocera <hadess@hadess.net>,
        David Zeuthen <davidz@redhat.com>,
        Richard Hughes <richard@hughsie.com>
References: <20211222212014.66971-1-linux@weissschuh.net>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20211222212014.66971-1-linux@weissschuh.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Thomas,

On 12/22/21 22:20, Thomas Weißschuh wrote:
> The EC/ACPI firmware on Lenovo ThinkPads used to report a status
> of "Unknown" when the battery is between the charge start and
> charge stop thresholds. On Windows, it reports "Not Charging"
> so the quirk has been added to also report correctly.
> 
> Now the "status" attribute returns "Not Charging" when the
> battery on ThinkPads is not physicaly charging.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Thanks, patch looks good to me.

As for the userspace issues in dealing with the
POWER_SUPPLY_STATUS_NOT_CHARGING status, those indeed
have long been fixed and this status is already returned
acpi//battery.c from the acpi_battery_handle_discharging()
function for a while no; and we have had no complaints
about that:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
> 
> This is the same as: https://patchwork.kernel.org/patch/10205359/
> 
> Previously this patch has been applied[0] but then reverted from -next
> because it caused a regression in UPower.
> This regression however has been fixed in UPower in late 2018[1],
> with the fixed version 0.99.10 released in early 2019 [2].
> So maybe it is now time to reintroduce this change.
> 
> Ognen:
> 
> As the patch was originally developed by you, could send a
> Signed-off-by-tag, so I can attribute you as co-developer?
> 
> Or maybe the original patch could just be re-applied?
> 
> The original patch had the following tags, which I'm not sure to handle
> for this case:
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Ognjen Galic <smclt30p@gmail.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Also Cc-ing the UPower maintainers for their opinion:
> 
> Cc: Bastien Nocera <hadess@hadess.net>
> Cc: David Zeuthen <davidz@redhat.com>
> Cc: Richard Hughes <richard@hughsie.com>
> 
> [0] Applied as 91eea70e5e5ce12eb1c7cd922e561fab43e201bd
> [1] https://gitlab.freedesktop.org/upower/upower/-/merge_requests/19/commits
> [2] https://gitlab.freedesktop.org/upower/upower/-/commit/215049e7b80c5f24cb35cd229a445c6cf19bd381
> ---
>  drivers/acpi/battery.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> 
> base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 8afa85d6eb6a..ead0114f27c9 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -53,6 +53,7 @@ static int battery_bix_broken_package;
>  static int battery_notification_delay_ms;
>  static int battery_ac_is_broken;
>  static int battery_check_pmic = 1;
> +static int battery_quirk_notcharging;
>  static unsigned int cache_time = 1000;
>  module_param(cache_time, uint, 0644);
>  MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
> @@ -217,6 +218,8 @@ static int acpi_battery_get_property(struct power_supply *psy,
>  			val->intval = POWER_SUPPLY_STATUS_CHARGING;
>  		else if (acpi_battery_is_charged(battery))
>  			val->intval = POWER_SUPPLY_STATUS_FULL;
> +		else if (battery_quirk_notcharging)
> +			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
>  		else
>  			val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
>  		break;
> @@ -1111,6 +1114,12 @@ battery_do_not_check_pmic_quirk(const struct dmi_system_id *d)
>  	return 0;
>  }
>  
> +static int __init battery_quirk_not_charging(const struct dmi_system_id *d)
> +{
> +	battery_quirk_notcharging = 1;
> +	return 0;
> +}
> +
>  static const struct dmi_system_id bat_dmi_table[] __initconst = {
>  	{
>  		/* NEC LZ750/LS */
> @@ -1155,6 +1164,19 @@ static const struct dmi_system_id bat_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_PRODUCT_VERSION, "Lenovo MIIX 320-10ICR"),
>  		},
>  	},
> +	{
> +		/*
> +		 * On Lenovo ThinkPads the BIOS specification defines
> +		 * a state when the bits for charging and discharging
> +		 * are both set to 0. That state is "Not Charging".
> +		 */
> +		.callback = battery_quirk_not_charging,
> +		.ident = "Lenovo ThinkPad",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad"),
> +		},
> +	},
>  	{},
>  };
>  
>