Re: [PATCH v3 2/2] acpi: battery: Read Battery charge time for Steamdeck devices

[Umang Jain <uajain@igalia.com>]

Hi Rafael,

Thanks for the review

On 2026-11-09 18:22, Rafael J. Wysocki (Intel) wrote:
> On Wed, Oct 14, 2026 at 10:44 PM Umang Jain <uajain@igalia.com> wrote:
>>
>> Valve steamdeck devices can report battery status as
>> charging, even when low-powered power supply is connected
>> to the device. The battery status is reported as charging
>> through sysfs, even though in reality, it is discharging.
>>
>> To handle this case, read out Battery charge time ("_BCT")
>> as it is reported accurately on these devices, hence use that
>> information for report charging vs discharging status.
>> Battery charge time ("_BCT") with an eligible power supply unit
>> will have valid values whereas with a low-powered supply unit,
>> it shall report ACPI_BATTERY_VALUE_UNKNOWN. Use this as a
>> differentiator to determine, whether the battery of steamdeck
>> devices is really charging or not.
>>
>> Signed-off-by: Umang Jain <uajain@igalia.com>
> 
> I think that it would be better to merge this patch with the [1/2].
> 
>> ---
>>  drivers/acpi/battery.c | 25 ++++++++++++++++++++++++-
>>  1 file changed, 24 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index fa20900bea0e..b7818695b6cb 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -242,7 +242,7 @@ static int acpi_battery_get_property(struct power_supply *psy,
>>                                         : POWER_SUPPLY_STATUS_DISCHARGING;
>>                                 return 0;
>>                         }
>> -               else if (battery->state & ACPI_BATTERY_STATE_CHARGING)
>> +               else if (battery->state & ACPI_BATTERY_STATE_CHARGING) {
>>                         /* Check the rate and capacity to validate the status. */
>>                         if (!acpi_battery_is_full(battery) ||
>>                             (battery->rate_now != ACPI_BATTERY_VALUE_UNKNOWN &&
>> @@ -252,6 +252,29 @@ static int acpi_battery_get_property(struct power_supply *psy,
>>                                 /* Full and zero rate. */
>>                                 val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
>>                         }
>> +
>> +                       /*
>> +                        * For Valve Steamdeck devices, evaluate _BCT to determine
>> +                        * if we are really charging.
>> +                        */
>> +                       if (test_bit(ACPI_BATTERY_QUIRK_VALVE_STEAMDECK, &battery->flags) &&
>> +                           acpi_has_method(battery->device->handle, "_BCT")) {
> 
> Can you check the _BCT presence at the time when the quirk bit is set
> and avoid setting the bit when _BCT is not present?  That would allow
> you to just test the bit here.
> 
>> +                               unsigned long long bct;
>> +                               acpi_status status;
>> +                               union acpi_object arg0 = { ACPI_TYPE_INTEGER };
>> +                               struct acpi_object_list args = { 1, &arg0 };
>> +
>> +                               arg0.integer.value = 100;
>> +                               status = acpi_evaluate_integer(battery->device->handle, "_BCT",
>> +                                                              &args, &bct);
>> +                               if (ACPI_FAILURE(status) || bct > ACPI_UINT32_MAX)
>> +                                       acpi_handle_info(battery->device->handle,
> 
> This should be acpi_handle_debug(), but I'm not sure if this message
> is interesting at all, especially if _BCT fails every time.  Also, it
> may be confusing in the bct > ACPI_UINT32_MAX case because it may
> print "success" then.

Agree with the ACPI_UINT32_MAX comment. I would like to keep the message
with acpi_handle_debug and split the if branch to handle the bct >
ACPI_UINT32_MAX separately. In this case, I won't print the
acpi_format_exception(status) and simply write that bct value is just
invalid. How does that sound?

Thanks.

> 
>> +                                                        "_BCT evaluation failed: %s",
>> +                                                        acpi_format_exception(status));
>> +                               else if (bct == ACPI_BATTERY_VALUE_UNKNOWN)
>> +                                       val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
>> +                       }
>> +               }
>>                 else if (battery->state & ACPI_BATTERY_STATE_CHARGE_LIMITING)
>>                         val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
>>                 else if (acpi_battery_is_charged(battery))
>> --