Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220429174114.1277799-1-luzmaximilian@gmail.com>
In-Reply-To: <20220429174114.1277799-1-luzmaximilian@gmail.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Fri, 6 May 2022 20:43:02 +0200
Message-ID: <CAJZ5v0hoE0vn5BX586Ag4oLFRn3WoZJCUQZ1+jM1O6Vm+yyc4A@mail.gmail.com>
Subject: Re: [PATCH] ACPI: battery: Add "Not Charging" quirk for Microsoft
 Surface devices
To:     Maximilian Luz <luzmaximilian@gmail.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>,
        ACPI Devel Maling List <linux-acpi@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Apr 29, 2022 at 7:41 PM Maximilian Luz <luzmaximilian@gmail.com> wrote:
>
> Microsoft Surface devices have a limiter that sets a fixed maximum
> charge capacity for the battery. When that maximum capacity has been
> reached, charging stops. In that case, _BST returns a battery state
> field with both "charging" and "discharging" bits cleared. The battery
> driver, however, returns "unknown" as status.
>
> This seems to be the same behavior as observed on the ThinkPads, so
> let's use the same quirk to handle that.
>
> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
> ---
>
> For what it's worth, I don't think the ACPI spec explicitly states that
> any of the status bits need to be set, or that there are only the
> "charging" and "discharging" states. As far as I can tell, ACPI only
> states:
>
>     Notice that the Charging bit and the Discharging bit are mutually
>     exclusive and must not both be set at the same time. Even in
>     critical state, hardware should report the corresponding
>     charging/discharging state.
>
> But that does not exclude the case that no bit is set. So, strictly
> going by spec, I don't think it's necessary to put all of this behind a
> quirk.

I think that this should be covered by the patch I've just applied:

https://patchwork.kernel.org/project/linux-acpi/patch/20220427154053.499203-1-wse@tuxedocomputers.com/

Shouldn't it?

> ---
>  drivers/acpi/battery.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index dc208f5f5a1f..1c88504aae5b 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -1152,6 +1152,19 @@ static const struct dmi_system_id bat_dmi_table[] __initconst = {
>                         DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad"),
>                 },
>         },
> +       {
> +               /*
> +                * Microsoft Surface devices have an optional "battery
> +                * limiter". Due to this, there is a "Not Charging" state
> +                * similar to the one on the Lenovo ThinkPads, described above.
> +                */
> +               .callback = battery_quirk_not_charging,
> +               .ident = "Microsoft Surface",
> +               .matches = {
> +                       DMI_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
> +                       DMI_MATCH(DMI_PRODUCT_NAME, "Surface"),
> +               },
> +       },
>         {
>                 /* Microsoft Surface Go 3 */
>                 .callback = battery_notification_delay_quirk,
> --
> 2.36.0
>