| Message ID | 20171215165750.GA6282@thinkpad (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Shevchenko |
| Headers | show |
On Fri, Dec 15, 2017 at 5:57 PM, Ognjen Galic <smclt30p@gmail.com> 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. > > Tested-by: Kevin Locke <kevin@kevinlocke.name> > Tested-by: Christoph Böhmwalder <christoph@boehmwalder.at> > Signed-off-by: Ognjen Galic <smclt30p@gmail.com> It doesn't look like this is related to the [1/3] and [3/3], so why do you make it part of the series? > --- > drivers/acpi/battery.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > index 2951d07..81e9b4e 100644 > --- a/drivers/acpi/battery.c > +++ b/drivers/acpi/battery.c > @@ -71,6 +71,7 @@ static async_cookie_t async_cookie; > static bool battery_driver_registered; > static int battery_bix_broken_package; > static int battery_notification_delay_ms; > +static int battery_quirk_thinkpad_notcharging; Drop "thinkpad" from this name as somebody may need to use the quirk for a machine from a different vendor in the future. > static unsigned int cache_time = 1000; > module_param(cache_time, uint, 0644); > MODULE_PARM_DESC(cache_time, "cache time in milliseconds"); > @@ -222,6 +223,13 @@ 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; > + /* > + * On the Lenovo ThinkPad ACPI implementation, when > + * neither bits 0 or 1 are set, that state is > + * considered as "Not Charging". > + */ > + else if (battery_quirk_thinkpad_notcharging) > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > else > val->intval = POWER_SUPPLY_STATUS_UNKNOWN; > break; > @@ -1301,6 +1309,13 @@ battery_notification_delay_quirk(const struct dmi_system_id *d) > return 0; > } > > +static int __init > +battery_quirk_not_charging(const struct dmi_system_id *d) Don't break the above line (it will be over 80 chars long, but that's fine). > +{ > + battery_quirk_thinkpad_notcharging = 1; > + return 0; > +} > + > static const struct dmi_system_id bat_dmi_table[] __initconst = { > { > .callback = battery_bix_broken_package_quirk, > @@ -1318,6 +1333,14 @@ static const struct dmi_system_id bat_dmi_table[] __initconst = { > DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-573G"), > }, > }, > + { Add a comment to describe this quirk (which it is needed in the first place). > + .callback = battery_quirk_not_charging, > + .ident = "Lenovo ThinkPad", > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > + DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad"), > + }, > + }, > {}, > }; > > -- Thanks, Rafael
On Sat, Dec 16, 2017 at 1:33 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Fri, Dec 15, 2017 at 5:57 PM, Ognjen Galic <smclt30p@gmail.com> 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. >> >> Tested-by: Kevin Locke <kevin@kevinlocke.name> >> Tested-by: Christoph Böhmwalder <christoph@boehmwalder.at> >> Signed-off-by: Ognjen Galic <smclt30p@gmail.com> > > It doesn't look like this is related to the [1/3] and [3/3], so why do > you make it part of the series? > >> --- >> drivers/acpi/battery.c | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c >> index 2951d07..81e9b4e 100644 >> --- a/drivers/acpi/battery.c >> +++ b/drivers/acpi/battery.c >> @@ -71,6 +71,7 @@ static async_cookie_t async_cookie; >> static bool battery_driver_registered; >> static int battery_bix_broken_package; >> static int battery_notification_delay_ms; >> +static int battery_quirk_thinkpad_notcharging; > > Drop "thinkpad" from this name as somebody may need to use the quirk > for a machine from a different vendor in the future. > >> static unsigned int cache_time = 1000; >> module_param(cache_time, uint, 0644); >> MODULE_PARM_DESC(cache_time, "cache time in milliseconds"); >> @@ -222,6 +223,13 @@ 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; >> + /* >> + * On the Lenovo ThinkPad ACPI implementation, when >> + * neither bits 0 or 1 are set, that state is >> + * considered as "Not Charging". >> + */ >> + else if (battery_quirk_thinkpad_notcharging) >> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; >> else >> val->intval = POWER_SUPPLY_STATUS_UNKNOWN; >> break; >> @@ -1301,6 +1309,13 @@ battery_notification_delay_quirk(const struct dmi_system_id *d) >> return 0; >> } >> >> +static int __init >> +battery_quirk_not_charging(const struct dmi_system_id *d) > > Don't break the above line (it will be over 80 chars long, but that's fine). > >> +{ >> + battery_quirk_thinkpad_notcharging = 1; >> + return 0; >> +} >> + >> static const struct dmi_system_id bat_dmi_table[] __initconst = { >> { >> .callback = battery_bix_broken_package_quirk, >> @@ -1318,6 +1333,14 @@ static const struct dmi_system_id bat_dmi_table[] __initconst = { >> DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-573G"), >> }, >> }, >> + { > > Add a comment to describe this quirk (which it is needed in the first place). s/whichj/why/ > >> + .callback = battery_quirk_not_charging, >> + .ident = "Lenovo ThinkPad", >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), >> + DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad"), >> + }, >> + }, >> {}, >> }; >> >> --
On Sat, Dec 16, 2017 at 01:33:55AM +0100, Rafael J. Wysocki wrote: > On Fri, Dec 15, 2017 at 5:57 PM, Ognjen Galic <smclt30p@gmail.com> 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. > > > > Tested-by: Kevin Locke <kevin@kevinlocke.name> > > Tested-by: Christoph Böhmwalder <christoph@boehmwalder.at> > > Signed-off-by: Ognjen Galic <smclt30p@gmail.com> > > It doesn't look like this is related to the [1/3] and [3/3], so why do > you make it part of the series? > I made it the same series because it is practically the same feature set. Without this patch and with 1/3 and 3/3 applied, there is a bug where the status attribute would show "Unknown" for a battery that is between the start and stop thresholds while attached to AC. > > --- > > drivers/acpi/battery.c | 23 +++++++++++++++++++++++ > > 1 file changed, 23 insertions(+) > > > > diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c > > index 2951d07..81e9b4e 100644 > > --- a/drivers/acpi/battery.c > > +++ b/drivers/acpi/battery.c > > @@ -71,6 +71,7 @@ static async_cookie_t async_cookie; > > static bool battery_driver_registered; > > static int battery_bix_broken_package; > > static int battery_notification_delay_ms; > > +static int battery_quirk_thinkpad_notcharging; > > Drop "thinkpad" from this name as somebody may need to use the quirk > for a machine from a different vendor in the future. > > > static unsigned int cache_time = 1000; > > module_param(cache_time, uint, 0644); > > MODULE_PARM_DESC(cache_time, "cache time in milliseconds"); > > @@ -222,6 +223,13 @@ 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; > > + /* > > + * On the Lenovo ThinkPad ACPI implementation, when > > + * neither bits 0 or 1 are set, that state is > > + * considered as "Not Charging". > > + */ > > + else if (battery_quirk_thinkpad_notcharging) > > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > > else > > val->intval = POWER_SUPPLY_STATUS_UNKNOWN; > > break; > > @@ -1301,6 +1309,13 @@ battery_notification_delay_quirk(const struct dmi_system_id *d) > > return 0; > > } > > > > +static int __init > > +battery_quirk_not_charging(const struct dmi_system_id *d) > > Don't break the above line (it will be over 80 chars long, but that's fine). > > > +{ > > + battery_quirk_thinkpad_notcharging = 1; > > + return 0; > > +} > > + > > static const struct dmi_system_id bat_dmi_table[] __initconst = { > > { > > .callback = battery_bix_broken_package_quirk, > > @@ -1318,6 +1333,14 @@ static const struct dmi_system_id bat_dmi_table[] __initconst = { > > DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-573G"), > > }, > > }, > > + { > > Add a comment to describe this quirk (which it is needed in the first place). > > > + .callback = battery_quirk_not_charging, > > + .ident = "Lenovo ThinkPad", > > + .matches = { > > + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), > > + DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad"), > > + }, > > + }, > > {}, > > }; > > > > -- > > Thanks, > Rafael
On Sat, Dec 16, 2017 at 9:48 AM, Ognjen Galic <smclt30p@gmail.com> wrote: > On Sat, Dec 16, 2017 at 01:33:55AM +0100, Rafael J. Wysocki wrote: >> On Fri, Dec 15, 2017 at 5:57 PM, Ognjen Galic <smclt30p@gmail.com> 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. >> > >> > Tested-by: Kevin Locke <kevin@kevinlocke.name> >> > Tested-by: Christoph Böhmwalder <christoph@boehmwalder.at> >> > Signed-off-by: Ognjen Galic <smclt30p@gmail.com> >> >> It doesn't look like this is related to the [1/3] and [3/3], so why do >> you make it part of the series? >> > > I made it the same series because it is practically the same feature > set. Without this patch and with 1/3 and 3/3 applied, there is a bug > where the status attribute would show "Unknown" for a battery that is > between the start and stop thresholds while attached to AC. OK, fair enough. I would reorder it as the first patch in the series, then, because the other two are more related to each other. Thanks, Rafael
On 16/12/2017, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Sat, Dec 16, 2017 at 9:48 AM, Ognjen Galic <smclt30p@gmail.com> wrote: >> On Sat, Dec 16, 2017 at 01:33:55AM +0100, Rafael J. Wysocki wrote: >>> On Fri, Dec 15, 2017 at 5:57 PM, Ognjen Galic <smclt30p@gmail.com> >>> 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. >>> > >>> > Tested-by: Kevin Locke <kevin@kevinlocke.name> >>> > Tested-by: Christoph Böhmwalder <christoph@boehmwalder.at> >>> > Signed-off-by: Ognjen Galic <smclt30p@gmail.com> >>> >>> It doesn't look like this is related to the [1/3] and [3/3], so why do >>> you make it part of the series? >>> >> >> I made it the same series because it is practically the same feature >> set. Without this patch and with 1/3 and 3/3 applied, there is a bug >> where the status attribute would show "Unknown" for a battery that is >> between the start and stop thresholds while attached to AC. > > OK, fair enough. > > I would reorder it as the first patch in the series, then, because the > other two are more related to each other. > > Thanks, > Rafael > Do I really need now to re-order and re-send the whole patchset for it to get merged? Thanks.
On Mon, Dec 18, 2017 at 12:21 PM, Ognjen Galić <smclt30p@gmail.com> wrote: > On 16/12/2017, Rafael J. Wysocki <rafael@kernel.org> wrote: >> I would reorder it as the first patch in the series, then, because the >> other two are more related to each other. > Do I really need now to re-order and re-send the whole patchset for it > to get merged? Through which tree it's supposed to go?
On 18/12/2017, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Dec 18, 2017 at 12:21 PM, Ognjen Galić <smclt30p@gmail.com> wrote: >> On 16/12/2017, Rafael J. Wysocki <rafael@kernel.org> wrote: > >>> I would reorder it as the first patch in the series, then, because the >>> other two are more related to each other. > >> Do I really need now to re-order and re-send the whole patchset for it >> to get merged? > > Through which tree it's supposed to go? > > -- > With Best Regards, > Andy Shevchenko > I'm afraid I don't understand the question. If you mean by Linux tree, it was written while 4.15-rc2 was active. Thanks, Gala
On Mon, Dec 18, 2017 at 11:21 AM, Ognjen Galić <smclt30p@gmail.com> wrote: > On 16/12/2017, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Sat, Dec 16, 2017 at 9:48 AM, Ognjen Galic <smclt30p@gmail.com> wrote: >>> On Sat, Dec 16, 2017 at 01:33:55AM +0100, Rafael J. Wysocki wrote: >>>> On Fri, Dec 15, 2017 at 5:57 PM, Ognjen Galic <smclt30p@gmail.com> >>>> 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. >>>> > >>>> > Tested-by: Kevin Locke <kevin@kevinlocke.name> >>>> > Tested-by: Christoph Böhmwalder <christoph@boehmwalder.at> >>>> > Signed-off-by: Ognjen Galic <smclt30p@gmail.com> >>>> >>>> It doesn't look like this is related to the [1/3] and [3/3], so why do >>>> you make it part of the series? >>>> >>> >>> I made it the same series because it is practically the same feature >>> set. Without this patch and with 1/3 and 3/3 applied, there is a bug >>> where the status attribute would show "Unknown" for a battery that is >>> between the start and stop thresholds while attached to AC. >> >> OK, fair enough. >> >> I would reorder it as the first patch in the series, then, because the >> other two are more related to each other. >> >> Thanks, >> Rafael >> > > Do I really need now to re-order and re-send the whole patchset for it > to get merged? I'm not sure I will take anything of it at this time. Thanks, Rafael
On Mon, Dec 18, 2017 at 1:36 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Dec 18, 2017 at 12:21 PM, Ognjen Galić <smclt30p@gmail.com> wrote: >> On 16/12/2017, Rafael J. Wysocki <rafael@kernel.org> wrote: > >>> I would reorder it as the first patch in the series, then, because the >>> other two are more related to each other. > >> Do I really need now to re-order and re-send the whole patchset for it >> to get merged? > > Through which tree it's supposed to go? Wait, wait. I haven't commented patches [1,3/3] yet, so no ACKs assumed, please. Thanks, Rafael
On Mon, Dec 18, 2017 at 7:31 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Mon, Dec 18, 2017 at 1:36 PM, Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> On Mon, Dec 18, 2017 at 12:21 PM, Ognjen Galić <smclt30p@gmail.com> wrote: >>> On 16/12/2017, Rafael J. Wysocki <rafael@kernel.org> wrote: >> >>>> I would reorder it as the first patch in the series, then, because the >>>> other two are more related to each other. >> >>> Do I really need now to re-order and re-send the whole patchset for it >>> to get merged? >> >> Through which tree it's supposed to go? > > Wait, wait. I haven't commented patches [1,3/3] yet, so no ACKs > assumed, please. I would prefer if you can take it through linux-pm. Whenever you are okay with the series let me know I would review PDx86 bits.
On Tue, Dec 19, 2017 at 3:34 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Dec 18, 2017 at 7:31 PM, Rafael J. Wysocki <rafael@kernel.org> wrote: >> On Mon, Dec 18, 2017 at 1:36 PM, Andy Shevchenko >> <andy.shevchenko@gmail.com> wrote: >>> On Mon, Dec 18, 2017 at 12:21 PM, Ognjen Galić <smclt30p@gmail.com> wrote: >>>> On 16/12/2017, Rafael J. Wysocki <rafael@kernel.org> wrote: >>> >>>>> I would reorder it as the first patch in the series, then, because the >>>>> other two are more related to each other. >>> >>>> Do I really need now to re-order and re-send the whole patchset for it >>>> to get merged? >>> >>> Through which tree it's supposed to go? >> >> Wait, wait. I haven't commented patches [1,3/3] yet, so no ACKs >> assumed, please. > > I would prefer if you can take it through linux-pm. No problem. > Whenever you are okay with the series let me know I would review PDx86 bits. OK Thanks, Rafael
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c index 2951d07..81e9b4e 100644 --- a/drivers/acpi/battery.c +++ b/drivers/acpi/battery.c @@ -71,6 +71,7 @@ static async_cookie_t async_cookie; static bool battery_driver_registered; static int battery_bix_broken_package; static int battery_notification_delay_ms; +static int battery_quirk_thinkpad_notcharging; static unsigned int cache_time = 1000; module_param(cache_time, uint, 0644); MODULE_PARM_DESC(cache_time, "cache time in milliseconds"); @@ -222,6 +223,13 @@ 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; + /* + * On the Lenovo ThinkPad ACPI implementation, when + * neither bits 0 or 1 are set, that state is + * considered as "Not Charging". + */ + else if (battery_quirk_thinkpad_notcharging) + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; else val->intval = POWER_SUPPLY_STATUS_UNKNOWN; break; @@ -1301,6 +1309,13 @@ battery_notification_delay_quirk(const struct dmi_system_id *d) return 0; } +static int __init +battery_quirk_not_charging(const struct dmi_system_id *d) +{ + battery_quirk_thinkpad_notcharging = 1; + return 0; +} + static const struct dmi_system_id bat_dmi_table[] __initconst = { { .callback = battery_bix_broken_package_quirk, @@ -1318,6 +1333,14 @@ static const struct dmi_system_id bat_dmi_table[] __initconst = { DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-573G"), }, }, + { + .callback = battery_quirk_not_charging, + .ident = "Lenovo ThinkPad", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), + DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad"), + }, + }, {}, };