[pm:build-test 122/130] drivers/acpi/acpica/psutils.c:97:27: error: implicit declaration of function 'acpi_ut_safe_strncpy'
by kbuild test robot
tree: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git build-test
head: 3fd9789833d2e4a4a10ec7cf20b762290ed50b62
commit: fe2aa18300b9b49d94e509f045602beca40db0aa [122/130] ACPICA: Create and deploy safe version of strncpy
config: x86_64-randconfig-s2-01042137 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
git checkout fe2aa18300b9b49d94e509f045602beca40db0aa
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
In file included from drivers/acpi/acpica/accommon.h:56:0,
from drivers/acpi/acpica/psutils.c:45:
drivers/acpi/acpica/psutils.c: In function 'acpi_ps_init_op':
>> drivers/acpi/acpica/psutils.c:97:27: error: implicit declaration of function 'acpi_ut_safe_strncpy' [-Werror=implicit-function-declaration]
ACPI_DISASM_ONLY_MEMBERS(acpi_ut_safe_strncpy(op->common.aml_op_name,
^
drivers/acpi/acpica/aclocal.h:753:41: note: in definition of macro 'ACPI_DISASM_ONLY_MEMBERS'
#define ACPI_DISASM_ONLY_MEMBERS(a) a;
^
cc1: some warnings being treated as errors
vim +/acpi_ut_safe_strncpy +97 drivers/acpi/acpica/psutils.c
> 45 #include "accommon.h"
46 #include "acparser.h"
47 #include "amlcode.h"
48 #include "acconvert.h"
49
50 #define _COMPONENT ACPI_PARSER
51 ACPI_MODULE_NAME("psutils")
52
53 /*******************************************************************************
54 *
55 * FUNCTION: acpi_ps_create_scope_op
56 *
57 * PARAMETERS: None
58 *
59 * RETURN: A new Scope object, null on failure
60 *
61 * DESCRIPTION: Create a Scope and associated namepath op with the root name
62 *
63 ******************************************************************************/
64 union acpi_parse_object *acpi_ps_create_scope_op(u8 *aml)
65 {
66 union acpi_parse_object *scope_op;
67
68 scope_op = acpi_ps_alloc_op(AML_SCOPE_OP, aml);
69 if (!scope_op) {
70 return (NULL);
71 }
72
73 scope_op->named.name = ACPI_ROOT_NAME;
74 return (scope_op);
75 }
76
77 /*******************************************************************************
78 *
79 * FUNCTION: acpi_ps_init_op
80 *
81 * PARAMETERS: op - A newly allocated Op object
82 * opcode - Opcode to store in the Op
83 *
84 * RETURN: None
85 *
86 * DESCRIPTION: Initialize a parse (Op) object
87 *
88 ******************************************************************************/
89
90 void acpi_ps_init_op(union acpi_parse_object *op, u16 opcode)
91 {
92 ACPI_FUNCTION_ENTRY();
93
94 op->common.descriptor_type = ACPI_DESC_TYPE_PARSER;
95 op->common.aml_opcode = opcode;
96
> 97 ACPI_DISASM_ONLY_MEMBERS(acpi_ut_safe_strncpy(op->common.aml_op_name,
98 (acpi_ps_get_opcode_info
99 (opcode))->name,
100 sizeof(op->common.
101 aml_op_name)));
102 }
103
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
4 years, 7 months
[pm:bleeding-edge 122/129] drivers/acpi/acpica/psutils.c:97:27: error: implicit declaration of function 'acpi_ut_safe_strncpy'; did you mean 'acpi_ut_trace_str'?
by kbuild test robot
tree: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git bleeding-edge
head: e65587cfad0fd464e31bcf288c084b9f4d5c302c
commit: fe2aa18300b9b49d94e509f045602beca40db0aa [122/129] ACPICA: Create and deploy safe version of strncpy
config: x86_64-randconfig-x010-201800 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
git checkout fe2aa18300b9b49d94e509f045602beca40db0aa
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
In file included from drivers/acpi/acpica/accommon.h:56:0,
from drivers/acpi/acpica/psutils.c:45:
drivers/acpi/acpica/psutils.c: In function 'acpi_ps_init_op':
>> drivers/acpi/acpica/psutils.c:97:27: error: implicit declaration of function 'acpi_ut_safe_strncpy'; did you mean 'acpi_ut_trace_str'? [-Werror=implicit-function-declaration]
ACPI_DISASM_ONLY_MEMBERS(acpi_ut_safe_strncpy(op->common.aml_op_name,
^
drivers/acpi/acpica/aclocal.h:753:41: note: in definition of macro 'ACPI_DISASM_ONLY_MEMBERS'
#define ACPI_DISASM_ONLY_MEMBERS(a) a;
^
cc1: some warnings being treated as errors
vim +97 drivers/acpi/acpica/psutils.c
> 45 #include "accommon.h"
46 #include "acparser.h"
47 #include "amlcode.h"
48 #include "acconvert.h"
49
50 #define _COMPONENT ACPI_PARSER
51 ACPI_MODULE_NAME("psutils")
52
53 /*******************************************************************************
54 *
55 * FUNCTION: acpi_ps_create_scope_op
56 *
57 * PARAMETERS: None
58 *
59 * RETURN: A new Scope object, null on failure
60 *
61 * DESCRIPTION: Create a Scope and associated namepath op with the root name
62 *
63 ******************************************************************************/
64 union acpi_parse_object *acpi_ps_create_scope_op(u8 *aml)
65 {
66 union acpi_parse_object *scope_op;
67
68 scope_op = acpi_ps_alloc_op(AML_SCOPE_OP, aml);
69 if (!scope_op) {
70 return (NULL);
71 }
72
73 scope_op->named.name = ACPI_ROOT_NAME;
74 return (scope_op);
75 }
76
77 /*******************************************************************************
78 *
79 * FUNCTION: acpi_ps_init_op
80 *
81 * PARAMETERS: op - A newly allocated Op object
82 * opcode - Opcode to store in the Op
83 *
84 * RETURN: None
85 *
86 * DESCRIPTION: Initialize a parse (Op) object
87 *
88 ******************************************************************************/
89
90 void acpi_ps_init_op(union acpi_parse_object *op, u16 opcode)
91 {
92 ACPI_FUNCTION_ENTRY();
93
94 op->common.descriptor_type = ACPI_DESC_TYPE_PARSER;
95 op->common.aml_opcode = opcode;
96
> 97 ACPI_DISASM_ONLY_MEMBERS(acpi_ut_safe_strncpy(op->common.aml_op_name,
98 (acpi_ps_get_opcode_info
99 (opcode))->name,
100 sizeof(op->common.
101 aml_op_name)));
102 }
103
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
4 years, 7 months
[pm:build-test 15/15] drivers/base/power/domain.c:119:26: error: expected '=', ',', '; ', 'asm' or '__attribute__' before '->' token
by kbuild test robot
tree: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git build-test
head: 3fd9789833d2e4a4a10ec7cf20b762290ed50b62
commit: 3fd9789833d2e4a4a10ec7cf20b762290ed50b62 [15/15] PM / genpd: Stop/start devices without pm_runtime_force_suspend/resume()
config: x86_64-randconfig-x011-201800 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
git checkout 3fd9789833d2e4a4a10ec7cf20b762290ed50b62
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
drivers/base/power/domain.c: In function 'genpd_finish_suspend':
>> drivers/base/power/domain.c:1051:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (genpd->dev_ops.stop && genpd->dev_ops.start &&
^~
drivers/base/power/domain.c:1054:3: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
if (ret)
^~
drivers/base/power/domain.c: At top level:
>> drivers/base/power/domain.c:119:26: error: expected '=', ',', ';', 'asm' or '__attribute__' before '->' token
#define genpd_lock(p) p->lock_ops->lock(p)
^
>> drivers/base/power/domain.c:1058:2: note: in expansion of macro 'genpd_lock'
genpd_lock(genpd);
^~~~~~~~~~
drivers/base/power/domain.c:1059:7: error: expected '=', ',', ';', 'asm' or '__attribute__' before '->' token
genpd->suspended_count++;
^~
>> drivers/base/power/domain.c:1060:36: error: expected ')' before numeric constant
genpd_sync_power_off(genpd, true, 0);
^
drivers/base/power/domain.c:122:28: error: expected '=', ',', ';', 'asm' or '__attribute__' before '->' token
#define genpd_unlock(p) p->lock_ops->unlock(p)
^
>> drivers/base/power/domain.c:1061:2: note: in expansion of macro 'genpd_unlock'
genpd_unlock(genpd);
^~~~~~~~~~~~
>> drivers/base/power/domain.c:1063:2: error: expected identifier or '(' before 'return'
return 0;
^~~~~~
>> drivers/base/power/domain.c:1064:1: error: expected identifier or '(' before '}' token
}
^
drivers/base/power/domain.c: In function 'genpd_restore_noirq':
drivers/base/power/domain.c:1141:6: warning: unused variable 'ret' [-Wunused-variable]
int ret = 0;
^~~
drivers/base/power/domain.c: In function 'genpd_finish_suspend':
>> drivers/base/power/domain.c:1056:2: warning: control reaches end of non-void function [-Wreturn-type]
}
^
vim +119 drivers/base/power/domain.c
d716f479 Lina Iyer 2016-10-14 118
35241d12 Lina Iyer 2016-10-14 @119 #define genpd_lock(p) p->lock_ops->lock(p)
35241d12 Lina Iyer 2016-10-14 120 #define genpd_lock_nested(p, d) p->lock_ops->lock_nested(p, d)
35241d12 Lina Iyer 2016-10-14 121 #define genpd_lock_interruptible(p) p->lock_ops->lock_interruptible(p)
35241d12 Lina Iyer 2016-10-14 @122 #define genpd_unlock(p) p->lock_ops->unlock(p)
35241d12 Lina Iyer 2016-10-14 123
:::::: The code at line 119 was first introduced by commit
:::::: 35241d12f750d2f1556a9c85f175ce7044716881 PM / Domains: Abstract genpd locking
:::::: TO: Lina Iyer <lina.iyer(a)linaro.org>
:::::: CC: Rafael J. Wysocki <rafael.j.wysocki(a)intel.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
4 years, 7 months
Re: [Devel] [PATCH v12 1/4] battery: Add the battery hooking API
by Rafael J. Wysocki
On Thu, Jan 4, 2018 at 12:13 PM, Ognjen Galić <smclt30p(a)gmail.com> wrote:
> On Wed, Jan 03, 2018 at 06:40:12PM +0200, Andy Shevchenko wrote:
>> On Wed, Jan 3, 2018 at 4:53 PM, Ognjen Galić <smclt30p(a)gmail.com> wrote:
>> > On Wed, Jan 03, 2018 at 04:25:42PM +0200, Andy Shevchenko wrote:
>> >> On Wed, Jan 3, 2018 at 1:58 PM, Ognjen Galic <smclt30p(a)gmail.com> wrote:
>>
>> >> Thanks for an update. I have couple of minors. Otherwise look pretty much good!
>> >>
>> >> > drivers/acpi/battery.h | 11 ----
>> >> > include/acpi/battery.h | 21 +++++++
>> >>
>> >> There are -M and -C command line parameters to git format-patch.
>>
>> >> They can take an optional argument (percentage) of threshold.
>> >>
>> >> Playing with those numbers you can achieve
>>
>> ^^^^ Pay attention to the above
>>
>> >>
>> >> rename ...
>> >>
>> >> line and see actual diff.
>> >>
>> >> No need to resend because of this. Just an explanation for the future Git work.
>> >
>> > I did use thos options. I used the following command:
>> >
>> > git format-patch -M -C --notes -v12 -o ~/patches/. @^^^^
>> >
>> > I really don't know what you are targeting. :)
>>
>> Please, read what I wrote above and the manual of git-format-patch.
>>
>> >> > +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
>> >> > +{
>> >> > + struct list_head *position;
>> >> > + struct acpi_battery *battery;
>> >>
>> >> Missed empty line?
>> >
>> > checkpatch.pl complains if there are NOT empty lines between
>> > declarations and statements.
>>
>> checkpatch some times on one hand complains about something which it
>> should not, on the other didn't take into consideration cases like
>> this one.
>>
>> Your statement started with comment, btw.
>>
>> >> > + /*
>> >> > + * In order to remove a hook, we first need to
>> >> > + * de-register all the batteries that are registered.
>> >> > + */
>> >> > + if (lock)
>> >> > + mutex_lock(&hook_mutex);
>>
>> > I mean, it's not game-breaking, its just minor style stuff. I won't be
>> > sending more revisions because of these small issues, as I think its
>> > uneccessary to flood both Rafael and the mailing lists with patch
>> > revisions that remove or add a few spaces. No offence, it just got old.
>>
>> Yes, his call anyway to apply or ask you for amendments. I'm just
>> helping with review.
>
> Rafael, what do you think? Do you want these style/syntax issues fixed
> or is it good to go?
As long as the code is all correct technically, they are secondary.
That said I still need to look into your patches in detail and I need
more time for that.
Thanks,
Rafael
4 years, 7 months
Re: [Devel] [PATCH v12 1/4] battery: Add the battery hooking API
by Andy Shevchenko
On Wed, Jan 3, 2018 at 4:53 PM, Ognjen Galić <smclt30p(a)gmail.com> wrote:
> On Wed, Jan 03, 2018 at 04:25:42PM +0200, Andy Shevchenko wrote:
>> On Wed, Jan 3, 2018 at 1:58 PM, Ognjen Galic <smclt30p(a)gmail.com> wrote:
>> Thanks for an update. I have couple of minors. Otherwise look pretty much good!
>>
>> > drivers/acpi/battery.h | 11 ----
>> > include/acpi/battery.h | 21 +++++++
>>
>> There are -M and -C command line parameters to git format-patch.
>> They can take an optional argument (percentage) of threshold.
>>
>> Playing with those numbers you can achieve
^^^^ Pay attention to the above
>>
>> rename ...
>>
>> line and see actual diff.
>>
>> No need to resend because of this. Just an explanation for the future Git work.
>
> I did use thos options. I used the following command:
>
> git format-patch -M -C --notes -v12 -o ~/patches/. @^^^^
>
> I really don't know what you are targeting. :)
Please, read what I wrote above and the manual of git-format-patch.
>> > +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
>> > +{
>> > + struct list_head *position;
>> > + struct acpi_battery *battery;
>>
>> Missed empty line?
>
> checkpatch.pl complains if there are NOT empty lines between
> declarations and statements.
checkpatch some times on one hand complains about something which it
should not, on the other didn't take into consideration cases like
this one.
Your statement started with comment, btw.
>> > + /*
>> > + * In order to remove a hook, we first need to
>> > + * de-register all the batteries that are registered.
>> > + */
>> > + if (lock)
>> > + mutex_lock(&hook_mutex);
> I mean, it's not game-breaking, its just minor style stuff. I won't be
> sending more revisions because of these small issues, as I think its
> uneccessary to flood both Rafael and the mailing lists with patch
> revisions that remove or add a few spaces. No offence, it just got old.
Yes, his call anyway to apply or ask you for amendments. I'm just
helping with review.
>> > +static void __exit battery_hook_exit(void)
>> > +{
>> > + struct acpi_battery_hook *hook;
>> > + struct acpi_battery_hook *ptr;
>>
>> Missed empty line?
>
> See above about the checkpatch.pl stuff.
See above the answer.
--
With Best Regards,
Andy Shevchenko
4 years, 7 months
Re: [Devel] [PATCH v12 4/4] battery: Add the ThinkPad "Not Charging" quirk
by Andy Shevchenko
On Wed, Jan 3, 2018 at 1:59 PM, Ognjen Galic <smclt30p(a)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.
>
> Signed-off-by: Ognjen Galic <smclt30p(a)gmail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko(a)gmail.com>
> ---
>
> Notes:
> v7:
> * Added all the style changes as suggested by Rafael
> * Re-ordered the series to make this a post-implement
> bugfix
>
> v8:
> * No changes in this patch in v8
>
> v9:
> * No changes in this patch in v9
>
> v10:
> * No changes in this patch in v10
>
> v11:
> * Fix formatting of changelog
>
> v12:
> * No changes in this patch in v12
>
> drivers/acpi/battery.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 7089e31..279516e 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -74,6 +74,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_notcharging;
> static unsigned int cache_time = 1000;
> module_param(cache_time, uint, 0644);
> MODULE_PARM_DESC(cache_time, "cache time in milliseconds");
> @@ -225,6 +226,8 @@ 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;
> + else if (battery_quirk_notcharging)
> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> else
> val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> break;
> @@ -1309,6 +1312,12 @@ 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_notcharging = 1;
> + return 0;
> +}
> +
> static const struct dmi_system_id bat_dmi_table[] __initconst = {
> {
> .callback = battery_bix_broken_package_quirk,
> @@ -1326,6 +1335,19 @@ static const struct dmi_system_id bat_dmi_table[] __initconst = {
> DMI_MATCH(DMI_PRODUCT_NAME, "Aspire V5-573G"),
> },
> },
> + {
> + /*
> + * On Lenovo ThinkPads the BIOS specification defines
> + * a state when the bits for charging and discharging
> + * are both set to 0. That state is "Not Charging".
> + */
> + .callback = battery_quirk_not_charging,
> + .ident = "Lenovo ThinkPad",
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> + DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad"),
> + },
> + },
> {},
> };
>
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
4 years, 7 months
Re: [Devel] [PATCH v12 3/4] thinkpad_acpi: Add support for battery thresholds
by Andy Shevchenko
On Wed, Jan 3, 2018 at 1:59 PM, Ognjen Galic <smclt30p(a)gmail.com> wrote:
> thinkpad_acpi registers two new attributes for each battery:
>
> 1) Charge start threshold
> /sys/class/power_supply/BATN/charge_start_threshold
>
> Valid values are [0, 99]. A value of 0 turns off the
> start threshold wear control.
>
> 2) Charge stop threshold
> /sys/class/power_supply/BATN/charge_stop_threshold
>
> Valid values are [1, 100]. A value of 100 turns off
> the stop threshold wear control. This must be
> configured first.
>
> Signed-off-by: Ognjen Galic <smclt30p(a)gmail.com>
Thanks for an update!
> ---
>
> Notes:
> * Change int to acpi_status in tpacpi_battery_acpi_eval
Then you need to check return code with ACPI_FAILURE() or ACPI_SUCCESS() macros.
That is one of the additional burden while I can't see usefulness of
ACPI return codes in that function and why I suggested to use plain
int.
One more question: why don't you use dev_err()/dev_*() macros instead
of pr_*() ones?
(Note, it's a question, needs a bit of discussion, I would like to
hear a rationale of this, I think it might be one)
--
With Best Regards,
Andy Shevchenko
4 years, 7 months
Re: [Devel] [PATCH v12 2/4] pm: add to_power_supply macro to the API
by Andy Shevchenko
On Wed, Jan 3, 2018 at 1:59 PM, Ognjen Galic <smclt30p(a)gmail.com> wrote:
> This patch adds the to_power_supply macro to upcast
> a device to a power_supply struct.
>
> This is needed because the same piece of code using
> container_of is used in various other places, so we
> abstract away such low-level operations via a macro.
>
> Suggested-by: Andy Shevchenko <andriy.shevchenko(a)linux.intel.com>
> Signed-off-by: Ognjen Galic <smclt30p(a)gmail.com>
It took me a while to understand if those private implementations are
equivalent to what you introduced.
Looks so sysfs_create_group() adds the attributes to power supply
device kobject, thus the struct device pointer in the callback will be
power supply child, not the actual hardware which seems correct.
Reviewed-by: Andy Shevchenko <andy.shevchenko(a)gmail.com>
> ---
>
> Notes:
> v9:
> * Split the pm changes from the thinkpad_acpi patch
> into its own patch
>
> v10:
> * No changes in this patch in v10
>
> v11:
> * Fix changelog formatting
>
> v12:
> * Fix build issues in ds2781 and ds2780 battery drivers
>
> drivers/power/supply/ds2780_battery.c | 5 -----
> drivers/power/supply/ds2781_battery.c | 5 -----
> drivers/power/supply/power_supply_core.c | 2 +-
> include/linux/power_supply.h | 2 ++
> 4 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/power/supply/ds2780_battery.c b/drivers/power/supply/ds2780_battery.c
> index e5d81b4..370e910 100644
> --- a/drivers/power/supply/ds2780_battery.c
> +++ b/drivers/power/supply/ds2780_battery.c
> @@ -56,11 +56,6 @@ to_ds2780_device_info(struct power_supply *psy)
> return power_supply_get_drvdata(psy);
> }
>
> -static inline struct power_supply *to_power_supply(struct device *dev)
> -{
> - return dev_get_drvdata(dev);
> -}
> -
> static inline int ds2780_battery_io(struct ds2780_device_info *dev_info,
> char *buf, int addr, size_t count, int io)
> {
> diff --git a/drivers/power/supply/ds2781_battery.c b/drivers/power/supply/ds2781_battery.c
> index efe83ef..d1b5a19 100644
> --- a/drivers/power/supply/ds2781_battery.c
> +++ b/drivers/power/supply/ds2781_battery.c
> @@ -54,11 +54,6 @@ to_ds2781_device_info(struct power_supply *psy)
> return power_supply_get_drvdata(psy);
> }
>
> -static inline struct power_supply *to_power_supply(struct device *dev)
> -{
> - return dev_get_drvdata(dev);
> -}
> -
> static inline int ds2781_battery_io(struct ds2781_device_info *dev_info,
> char *buf, int addr, size_t count, int io)
> {
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 82f998a..feac7b0 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -668,7 +668,7 @@ EXPORT_SYMBOL_GPL(power_supply_powers);
>
> static void power_supply_dev_release(struct device *dev)
> {
> - struct power_supply *psy = container_of(dev, struct power_supply, dev);
> + struct power_supply *psy = to_power_supply(dev);
> dev_dbg(dev, "%s\n", __func__);
> kfree(psy);
> }
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 79e90b3..f0139b4 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -371,6 +371,8 @@ devm_power_supply_register_no_ws(struct device *parent,
> extern void power_supply_unregister(struct power_supply *psy);
> extern int power_supply_powers(struct power_supply *psy, struct device *dev);
>
> +#define to_power_supply(device) container_of(device, struct power_supply, dev)
> +
> extern void *power_supply_get_drvdata(struct power_supply *psy);
> /* For APM emulation, think legacy userspace. */
> extern struct class *power_supply_class;
> --
> 2.7.4
>
--
With Best Regards,
Andy Shevchenko
4 years, 7 months
Re: [Devel] [PATCH v12 1/4] battery: Add the battery hooking API
by Andy Shevchenko
On Wed, Jan 3, 2018 at 1:58 PM, Ognjen Galic <smclt30p(a)gmail.com> wrote:
> This is a patch that implements a generic hooking API for the
> generic ACPI battery driver.
>
> With this new generic API, drivers can expose platform specific
> behaviour via sysfs attributes in /sys/class/power_supply/BATn/
> in a generic way.
>
> A perfect example of the need for this API are Lenovo ThinkPads.
>
> Lenovo ThinkPads have a ACPI extension that allows the setting of
> start and stop charge thresholds in the EC and battery firmware
> via ACPI. The thinkpad_acpi module can use this API to expose
> sysfs attributes that it controls inside the ACPI battery driver
> sysfs tree, under /sys/class/power_supply/BATN/.
>
> The file drivers/acpi/battery.h has been moved to
> include/acpi/battery.h and the includes inside ac.c, sbs.c, and
> battery.c have been adjusted to reflect that.
>
> When drivers hooks into the API, the API calls add_battery() for
> each battery in the system that passes it a acpi_battery
> struct. Then, the drivers can use device_create_file() to create
> new sysfs attributes with that struct and identify the batteries
> for per-battery attributes.
Thanks for an update. I have couple of minors. Otherwise look pretty much good!
> drivers/acpi/battery.h | 11 ----
> include/acpi/battery.h | 21 +++++++
There are -M and -C command line parameters to git format-patch.
They can take an optional argument (percentage) of threshold.
Playing with those numbers you can achieve
rename ...
line and see actual diff.
No need to resend because of this. Just an explanation for the future Git work.
> +void __battery_hook_unregister(struct acpi_battery_hook *hook, int lock)
> +{
> + struct list_head *position;
> + struct acpi_battery *battery;
Missed empty line?
> + /*
> + * In order to remove a hook, we first need to
> + * de-register all the batteries that are registered.
> + */
> + if (lock)
> + mutex_lock(&hook_mutex);
> + list_for_each(position, &acpi_battery_list) {
> + battery = list_entry(position, struct acpi_battery, list);
list_for_each_enrty() ?
Or I'm missing something why it can't be used?
> + hook->remove_battery(battery->bat);
> + }
> +void battery_hook_register(struct acpi_battery_hook *hook)
> +{
> + struct acpi_battery *battery;
> + list_for_each_entry(battery, &acpi_battery_list, list) {
> + if (hook->add_battery(battery->bat)) {
> + /*
> + * If a add-battery returns non-zero,
> + * the registration of the extension has failed,
> + * and we will not add it to the list of loaded
> + * hooks.
> + */
> + pr_err("extension failed to load: %s",
> + hook->name);
Can it fit 80 characters?
I mean if it would be one line...
> + __battery_hook_unregister(hook, 0);
> + return;
> + }
> + }
> + pr_info("new extension: %s\n", hook->name);
> + mutex_unlock(&hook_mutex);
> +}
> +static void battery_hook_add_battery(struct acpi_battery *battery)
> +{
> + /*
> + * This function gets called right after the battery sysfs
> + * attributes have been added, so that the drivers that
> + * define custom sysfs attributes can add their own.
> + */
Perhaps it should go before function definition.
> + struct acpi_battery_hook *hook_node;
> +}
> +static void __exit battery_hook_exit(void)
> +{
> + struct acpi_battery_hook *hook;
> + struct acpi_battery_hook *ptr;
Missed empty line?
> + /*
> + * At this point, the acpi_bus_unregister_driver
> + * has called remove for all batteries. We just
> + * need to remove the hooks.
> + */
A common pattern to use func() [note parens] when refer to function.
> + list_for_each_entry_safe(hook, ptr, &battery_hook_list, list) {
> + __battery_hook_unregister(hook, 1);
> + }
> + mutex_destroy(&hook_mutex);
> +}
> battery->bat = power_supply_register_no_ws(&battery->device->dev,
> &battery->bat_desc, &psy_cfg);
>
> + battery_hook_add_battery(battery);
> +
> if (IS_ERR(battery->bat)) {
> int result = PTR_ERR(battery->bat);
I'm not sure why you need to add hook when power_supply_register_no_ws() failed.
> }
> -
> + battery_hook_remove_battery(battery);
No need to remove an empty line.
--
With Best Regards,
Andy Shevchenko
4 years, 7 months
Re: [Devel] [PATCH v11 3/5] thinkpad_acpi: Add support for battery thresholds
by Rafael J. Wysocki
On Wednesday, January 3, 2018 11:34:55 AM CET Ognjen Galić wrote:
> On Mon, Jan 01, 2018 at 12:24:39PM +0200, Andy Shevchenko wrote:
> > On Sun, Dec 31, 2017 at 4:17 PM, Ognjen Galic <smclt30p(a)gmail.com> wrote:
> > > thinkpad_acpi registers two new attributes for each battery:
> > >
> > > 1) Charge start threshold
> > > /sys/class/power_supply/BATN/charge_start_threshold
> > >
> > > Valid values are [0, 99]. A value of 0 turns off the
> > > start threshold wear control.
> > >
> > > 2) Charge stop threshold
> > > /sys/class/power_supply/BATN/charge_stop_threshold
> > >
> > > Valid values are [1, 100]. A value of 100 turns off
> > > the stop threshold wear control. This must be
> > > configured first.
> > >
> >
> > > This patch depends on the following patches:
> > >
> > > "battery: Add the battery hooking API"
> >
> > Since this is series, no need to put above into changelog.
> >
> > AFAICS it's not going to be backported either.
> >
> >
> > > +/**
> > > + * This evaluates a ACPI method call specific to the battery
> > > + * ACPI extension. The specifics are that an error is marked
> > > + * in the 32rd bit of the response, so we just check that here.
> > > + *
> > > + * Returns 0 on success
> > > + */
> > > +static int tpacpi_battery_acpi_eval(char *method, int *ret, int param)
> > > +{
> >
> > One problem and one trick you may do.
> >
> > A problem: you return ACPI status as int. We have special type for
> > that. So, be consistent with it.
> > Looking to acpi_evalf() which is private to the module, I think you
> > need to get rid of ACPI return codes here.
> >
> > A trick: since you are using only least significant byte of the
> > response, you can declare it as u8 *value (yeah, ret is not best name
> > here I think).
>
> Not really. In one place I need bit #9, so that will stay a int.
> I have changed the acpi return value to acpi_status tho.
OK, you can resend the updated series when you're ready.
Thanks,
Rafael
4 years, 7 months