On Thu, Feb 22, 2018 at 11:10:07PM +0100, Rafael J. Wysocki wrote:
> On Sat, Feb 17, 2018 at 10:22 PM, Laszlo Toth <laszlth@xxxxxxxxx> wrote:
> > With a degraded battery, full_charge_capacity can be less
> > than design_capacity, however it's not sure that capacity_now's
> > max will follow.
> >
> > Example from an affected machine:
> > /sys/class/power_supply/BAT0/charge_full -> 4290000
> > /sys/class/power_supply/BAT0/charge_full_design -> 5900000
> > /sys/class/power_supply/BAT0/charge_now -> 5900000
> > /sys/class/power_supply/BAT0/capacity -> 137
> >
> > The battery is a degraded one with a full charge, and
> > charge_now is the value of charge_full_design instead of
> > charge_full.
> >
> > Added a new quirk to test and correct this, and
> > a new function to check if the battery is a degraded one
> > or not. This keeps the possibility to be over 100 if
> > it's really the case.
> >
> > Signed-off-by: Laszlo Toth <laszlth@xxxxxxxxx>
> > ---
> > drivers/acpi/battery.c | 28 ++++++++++++++++++++++++++++
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> > index 7128488..b2729a5 100644
> > --- a/drivers/acpi/battery.c
> > +++ b/drivers/acpi/battery.c
> > @@ -116,6 +116,10 @@ enum {
> > post-1.29 BIOS), but as of Nov. 2012, no such update is
> > available for the 2010 models. */
> > ACPI_BATTERY_QUIRK_THINKPAD_MAH,
> > + /* for batteries reporting current capacity with design capacity
> > + * on a full charge, but showing degradation in full charge cap.
> > + */
> > + ACPI_BATTERY_QUIRK_DESIGN_MAX_CAPACITY,
>
> I would call this ACPI_BATTERY_QUIRK_DEGRADED_FULL_CHARGE.
>
That's better, will change. Thanks
> > };
> >
> > struct acpi_battery {
> > @@ -201,6 +205,13 @@ static int acpi_battery_is_charged(struct acpi_battery *battery)
> > return 0;
> > }
> >
> > +static inline int acpi_battery_is_degraded(struct acpi_battery *battery)
>
> This can return bool and need not be inline (the compiler should be
> smart enough to make it inline if need be).
>
Ok, let's use bool.
For inline: thought a hint could be handy, then it can choose whether to ignore it
or not. But after all you're right, comp. can choose without the hint as
well.
> > +{
> > + if (battery->full_charge_capacity && battery->design_capacity)
> > + return battery->full_charge_capacity < battery->design_capacity;
> > + return 0;
>
> And the above can be written as
>
> return battery->full_charge_capacity && battery->design_capacity &&
> battery->full_charge_capacity < battery->design_capacity;
>
Sure.
> > +}
> > +
> > static int acpi_battery_get_property(struct power_supply *psy,
> > enum power_supply_property psp,
> > union power_supply_propval *val)
> > @@ -475,6 +486,9 @@ static int extract_battery_info(const int use_bix,
> > it's impossible to tell if they would need an adjustment
> > or not if their values were higher. */
> > }
> > + if (test_bit(ACPI_BATTERY_QUIRK_DESIGN_MAX_CAPACITY, &battery->flags) &&
> > + battery->capacity_now > battery->full_charge_capacity)
> > + battery->capacity_now = battery->full_charge_capacity;
>
> If you add an extra empty line below, do that here too.
>
Will do.
> > return result;
> > }
> >
> > @@ -567,6 +581,10 @@ static int acpi_battery_get_state(struct acpi_battery *battery)
> > battery->capacity_now = battery->capacity_now *
> > 10000 / battery->design_voltage;
> > }
> > + if (test_bit(ACPI_BATTERY_QUIRK_DESIGN_MAX_CAPACITY, &battery->flags) &&
> > + battery->capacity_now > battery->full_charge_capacity)
> > + battery->capacity_now = battery->full_charge_capacity;
> > +
> > return result;
> > }
> >
> > @@ -743,6 +761,16 @@ static void acpi_battery_quirks(struct acpi_battery *battery)
> > }
> > }
> > }
> > +
> > + if (test_bit(ACPI_BATTERY_QUIRK_DESIGN_MAX_CAPACITY, &battery->flags))
> > + return;
> > +
> > + if (acpi_battery_is_degraded(battery) &&
> > + battery->capacity_now > battery->full_charge_capacity) {
> > + set_bit(ACPI_BATTERY_QUIRK_DESIGN_MAX_CAPACITY,
> > + &battery->flags);
>
> You don't need to break this line (yes, I know that checkpatch.pl
> complains about it being too long).
>
Ok, after the rename I'll ignore the script if it'd produce something like this.
> > + battery->capacity_now = battery->full_charge_capacity;
> > + }
> > }
> >
> > static int acpi_battery_update(struct acpi_battery *battery, bool resume)
> > --
>
> Thanks,
> Rafael
Thanks,
Laszlo
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
