Re: [PATCH v2 1/2] acpi: battery: Query Battery charge time ("_BCT") method

[Umang Jain]

Hi Rafael,

Thanks for the review.

On 2026-10-12 17:15, Rafael J. Wysocki (Intel) wrote:
> On Wed, Sep 23, 2026 at 10:28 AM Umang Jain <uajain@xxxxxxxxxx> wrote:
>>
>> Add plumbing for querying the Battery Charge Time ("_BCT") method.
>> If present, set ACPI_BATTERY_BCT_PRESENT in battery flags to denote
>> that the method is supported in ACPI.
>>
>> Signed-off-by: Umang Jain <uajain@xxxxxxxxxx>
>> ---
>>  drivers/acpi/battery.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index 670853ec3a4d..a1b7a821abaf 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -76,6 +76,7 @@ MODULE_DEVICE_TABLE(acpi, battery_device_ids);
>>  enum {
>>         ACPI_BATTERY_ALARM_PRESENT,
>>         ACPI_BATTERY_XINFO_PRESENT,
>> +       ACPI_BATTERY_BCT_PRESENT,
>>         ACPI_BATTERY_QUIRK_PERCENTAGE_CAPACITY,
>>         /* On Lenovo Thinkpad models from 2010 and 2011, the power unit
>>          * switches between mWh and mAh depending on whether the system
>> @@ -110,6 +111,7 @@ struct acpi_battery {
>>
>>         struct mutex property_lock; /* Protects properties below. */
>>         unsigned long update_time;
>> +       unsigned long long charge_time;
> 
> This doesn't need to be unsigned long long.  As per the spec, unsigned
> int should be sufficient.
> 
>>         int revision;
>>         int rate_now;
>>         int capacity_now;
>> @@ -613,6 +615,10 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
>>         int result = 0;
>>         acpi_status status = 0;
>>         struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +       union acpi_object arg0 = { ACPI_TYPE_INTEGER };
>> +       struct acpi_object_list args = { 1, &arg0 };
>> +
>> +       arg0.integer.value = 100;
> 
> Please move the definitions of the variables related to _BCT
> evaluation to the code block where it gets evaluated.
> 
>>
>>         lockdep_assert_held(&battery->property_lock);
>>
>> @@ -638,6 +644,15 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
>>         battery->update_time = jiffies;
>>         kfree(buffer.pointer);
>>
>> +       if (test_bit(ACPI_BATTERY_BCT_PRESENT, &battery->flags)) {
>> +               status = acpi_evaluate_integer(battery->device->handle, "_BCT",
>> +                                              &args, &battery->charge_time);
> 
> Currently, you only need to evaluate _BCT to implement the quirk in
> the second patch AFAICS, so it is not useful to evaluate _BCT if the
> quirk is not going to be used.  Any chance to rearrange the code to
> avoid evaluating _BCT when it is not needed?

Currently yes, _BCT is not useful in all cases, but since it is a ACPI
specification component, I didn't keep it behind the quirk and read as a
global param to read out.

I've re-arranged the code to evaluate _BCT if the quirk is present, but
want to double sure if that's the design we need to go ahead with?

> 
>> +       if (ACPI_FAILURE(status))
>> +               acpi_handle_info(battery->device->handle,
>> +                                "_BCT evaluation failed: %s",
>> +                                acpi_format_exception(status));
> 
> Missing indentation.

Looks fine to me, what's wrong? The patches sent are sanitized with
./scripts/checkpatch.pl, unless I am missing something?
> 
>> +       }
>> +
>>         /* For buggy DSDTs that report negative 16-bit values for either
>>          * charging or discharging current and/or report 0 as 65536
>>          * due to bad math.
>> @@ -1083,6 +1098,8 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
>>                         if (result)
>>                                 return result;
>>                         acpi_battery_init_alarm(battery);
>> +                       if (acpi_has_method(battery->device->handle, "_BCT"))
>> +                               set_bit(ACPI_BATTERY_BCT_PRESENT, &battery->flags);
>>                 }
>>
>>                 result = acpi_battery_get_state(battery);
>> --