|
From: Rafael J. W. <ra...@ke...> - 2018-02-09 10:49:47
|
On Fri, Feb 9, 2018 at 11:39 AM, Sebastian Reichel <sr...@ke...> wrote: > Hi, > > On Fri, Feb 09, 2018 at 10:22:54AM +0100, Rafael J. Wysocki wrote: >> On Thu, Feb 8, 2018 at 11:03 PM, Sebastian Reichel <sr...@ke...> wrote: >> > Hi, >> > >> > On Wed, Feb 07, 2018 at 03:58:44PM +0100, Ognjen Galic wrote: >> >> 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 is a new sysfs file, that should be documented. >> >> Right, I should have remembered about that, sorry. >> >> > Also this looks pretty generic. Just introduce new POWER_SUPPLY_PROP_ >> > entries for start/stop charging threshold and use them. >> >> What about doing this as a follow-up? > > Fine with me. OK Actually, I don't see any documentation whatever for ACPI battery and AC power supply properties, so I guess that needs to be added in general and I don't think it would be fair to ask Ognjen to do that in order to get the extension in. Why don't we pencil this in as work to do? |
|
From: Sebastian R. <sr...@ke...> - 2018-02-09 12:34:33
Attachments:
signature.asc
|
Hi, On Fri, Feb 09, 2018 at 11:49:40AM +0100, Rafael J. Wysocki wrote: > On Fri, Feb 9, 2018 at 11:39 AM, Sebastian Reichel <sr...@ke...> wrote: > > Hi, > > > > On Fri, Feb 09, 2018 at 10:22:54AM +0100, Rafael J. Wysocki wrote: > >> On Thu, Feb 8, 2018 at 11:03 PM, Sebastian Reichel <sr...@ke...> wrote: > >> > Hi, > >> > > >> > On Wed, Feb 07, 2018 at 03:58:44PM +0100, Ognjen Galic wrote: > >> >> 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 is a new sysfs file, that should be documented. > >> > >> Right, I should have remembered about that, sorry. > >> > >> > Also this looks pretty generic. Just introduce new POWER_SUPPLY_PROP_ > >> > entries for start/stop charging threshold and use them. > >> > >> What about doing this as a follow-up? > > > > Fine with me. > > OK > > Actually, I don't see any documentation whatever for ACPI battery and > AC power supply properties, so I guess that needs to be added in > general and I don't think it would be fair to ask Ognjen to do that in > order to get the extension in. > > Why don't we pencil this in as work to do? The generic ones are documented here: Documentation/power/power_supply_class.txt -- Sebastian |
|
From: Ognjen G. <smc...@gm...> - 2018-02-10 08:49:10
|
On Fri, Feb 09, 2018 at 01:34:14PM +0100, Sebastian Reichel wrote: > Hi, > > On Fri, Feb 09, 2018 at 11:49:40AM +0100, Rafael J. Wysocki wrote: > > On Fri, Feb 9, 2018 at 11:39 AM, Sebastian Reichel <sr...@ke...> wrote: > > > Hi, > > > > > > On Fri, Feb 09, 2018 at 10:22:54AM +0100, Rafael J. Wysocki wrote: > > >> On Thu, Feb 8, 2018 at 11:03 PM, Sebastian Reichel <sr...@ke...> wrote: > > >> > Hi, > > >> > > > >> > On Wed, Feb 07, 2018 at 03:58:44PM +0100, Ognjen Galic wrote: > > >> >> 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 is a new sysfs file, that should be documented. > > >> > > >> Right, I should have remembered about that, sorry. > > >> > > >> > Also this looks pretty generic. Just introduce new POWER_SUPPLY_PROP_ > > >> > entries for start/stop charging threshold and use them. > > >> > > >> What about doing this as a follow-up? > > > > > > Fine with me. > > > > OK > > > > Actually, I don't see any documentation whatever for ACPI battery and > > AC power supply properties, so I guess that needs to be added in > > general and I don't think it would be fair to ask Ognjen to do that in > > order to get the extension in. > > > > Why don't we pencil this in as work to do? > > The generic ones are documented here: Do you guys want me to send in another revision of the patch with some documentation on the sysfs API? > > Documentation/power/power_supply_class.txt > > -- Sebastian |
|
From: Andy S. <and...@gm...> - 2018-02-23 17:21:19
|
On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smc...@gm...> wrote: > Do you guys want me to send in another revision of the patch with some > documentation on the sysfs API? I noticed I didn't apply it because of some pending changes discussed, perhaps this one above. So, definitely please send a new version which addresses comments. -- With Best Regards, Andy Shevchenko |
|
From: Rafael J. W. <ra...@ke...> - 2018-02-24 09:07:36
|
On Fri, Feb 23, 2018 at 6:21 PM, Andy Shevchenko <and...@gm...> wrote: > On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smc...@gm...> wrote: > >> Do you guys want me to send in another revision of the patch with some >> documentation on the sysfs API? > > I noticed I didn't apply it because of some pending changes discussed, > perhaps this one above. > > So, definitely please send a new version which addresses comments. No, it actually has been applied already. What is needed is a follow-up patch, but I'll write on that later. Thanks, Rafael |
|
From: Ognjen G. <smc...@gm...> - 2018-02-24 10:31:52
|
On Sat, Feb 24, 2018 at 10:07:20AM +0100, Rafael J. Wysocki wrote: > On Fri, Feb 23, 2018 at 6:21 PM, Andy Shevchenko > <and...@gm...> wrote: > > On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smc...@gm...> wrote: > > > >> Do you guys want me to send in another revision of the patch with some > >> documentation on the sysfs API? > > > > I noticed I didn't apply it because of some pending changes discussed, > > perhaps this one above. > > > > So, definitely please send a new version which addresses comments. > > No, it actually has been applied already. So it has been queued up for 4.17? If it has, that is awesome! Thanks! > > What is needed is a follow-up patch, but I'll write on that later. > > Thanks, > Rafael |
|
From: Ognjen G. <smc...@gm...> - 2018-02-24 17:32:30
|
On Sat, Feb 24, 2018 at 10:07:20AM +0100, Rafael J. Wysocki wrote: > On Fri, Feb 23, 2018 at 6:21 PM, Andy Shevchenko > <and...@gm...> wrote: > > On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smc...@gm...> wrote: > > > >> Do you guys want me to send in another revision of the patch with some > >> documentation on the sysfs API? > > > > I noticed I didn't apply it because of some pending changes discussed, > > perhaps this one above. > > > > So, definitely please send a new version which addresses comments. > > No, it actually has been applied already. And also, how is it applied if it does not apply to 4.16-rc2 anymore? Do you need a new revision that solves the apply conflicts? Do you need a new revision that addresses the documentation? Did you apply it earlier than 4.16-rc2? When did you apply it? Did you apply it at all? > > What is needed is a follow-up patch, but I'll write on that later. > > Thanks, > Rafael |
|
From: Rafael J. W. <ra...@ke...> - 2018-02-26 18:17:04
|
On Sat, Feb 24, 2018 at 6:32 PM, Ognjen Galić <smc...@gm...> wrote: > On Sat, Feb 24, 2018 at 10:07:20AM +0100, Rafael J. Wysocki wrote: >> On Fri, Feb 23, 2018 at 6:21 PM, Andy Shevchenko >> <and...@gm...> wrote: >> > On Sat, Feb 10, 2018 at 10:48 AM, Ognjen Galić <smc...@gm...> wrote: >> > >> >> Do you guys want me to send in another revision of the patch with some >> >> documentation on the sysfs API? >> > >> > I noticed I didn't apply it because of some pending changes discussed, >> > perhaps this one above. >> > >> > So, definitely please send a new version which addresses comments. >> >> No, it actually has been applied already. > > And also, how is it applied if it does not apply to 4.16-rc2 anymore? > Do you need a new revision that solves the apply conflicts? > Do you need a new revision that addresses the documentation? > Did you apply it earlier than 4.16-rc2? > When did you apply it? > Did you apply it at all? Yes, I did, on top of 4.16-rc2, and I might have rebased it a bit. Please see linux-next and I will expose the acpi-battery branch containing this series later today and tomorrow. >> What is needed is a follow-up patch, but I'll write on that later. Please see my previous message in this thread regarding the above. |
|
From: Andy S. <and...@gm...> - 2017-12-12 19:10:40
|
On Sun, Dec 10, 2017 at 8:06 PM, Ognjen Galic <smc...@gm...> wrote:
> #include <linux/jiffies.h>
> #include <linux/workqueue.h>
> #include <linux/acpi.h>
> +#include <linux/power_supply.h>
> #include <linux/pci_ids.h>
> #include <linux/thinkpad_acpi.h>
> #include <sound/core.h>
> @@ -84,6 +85,8 @@
> #include <sound/initval.h>
> #include <linux/uaccess.h>
> #include <acpi/video.h>
> +#include <acpi/battery.h>
I would try to keep it more ordered (yes, I noticed that no order in
the original file), like
#include <linux/acpi.h>
#include <linux/pci_ids.h>
+#include <linux/power_supply.h>
#include <linux/thinkpad_acpi.h>
+#include <acpi/battery.h>
#include <acpi/video.h>
instead of above.
> +/**
I think the function name is missed here.
> + * 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_eval(char *method, int *ret, int param)
> +{
> + if (!acpi_evalf(hkey_handle, ret, method, "dd", param)) {
> + pr_err("%s: evaluate failed", method);
> + return AE_ERROR;
> + }
> +
> + if (*ret & METHOD_ERR) {
> + pr_err("%s evaluated but flagged as error", method);
> + return AE_ERROR;
> + }
> +
Just a side note: does it make any sense to switch to
acpi_handle_err() at some point?
> + return AE_OK;
> +}
> +static int tpacpi_battery_get(int what, int battery, int *ret)
> +{
> + /*
> + * On the stop value, if we return 0 that
> + * does not make any sense. 0 means Default, which
> + * means that charging stops at 100%, so we return
> + * that.
> + */
> + *ret = *ret == 0 ? 100 : *ret;
Hmm... Perhaps just
if (*ret == 0)
*ret = 100;
?
> + return 0;
> +}
> +
> +static int tpacpi_battery_set(int what, int battery, int value)
> +{
> +
> + int param = 0x0, ret = 0xFFFFFFFF;
> +
> + /* The first 8 bits are the value of the threshold */
> + param = value;
> + /* The battery ID is in bits 8-9, 2 bits */
> + param |= (battery << 8);
Redundant parens.
> +}
> +
> +static int tpacpi_battery_probe(int battery)
> +{
> + int ret = 0;
> +
> + /* Reset the struct */
> + battery_info.start_support[battery] = 0x0;
> + battery_info.stop_support[battery] = 0x0;
> + battery_info.start_charge[battery] = 0x0;
> + battery_info.stop_charge[battery] = 0x0;
> +
Hmm... If you reorganize your data struct to
struct _info {
};
struct _pdata {
...
struct _info info[3];
};
You can use memset() and memcpy() or just plain assignment whenever it's needed.
> + /* Individual addressing is in bit 9 */
> + if (ret & (1 << 9))
BIT(9) ?
> + /* Support is marked in bit 8 */
> + if (ret & (1 << 8))
BIT(8) ?
> + battery_info.start_support[battery] = 1;
> + else
> + return -ENODEV;
OK, here it is in align with the previous cond.
> + /* Support is marked in bit 8 */
> + if (ret & (1 << 8))
> + battery_info.stop_support[battery] = 1;
> + pr_info("battery %d registered (start %d, stop %d)",
> + battery,
> + battery_info.start_charge[battery],
> + battery_info.stop_charge[battery]);
> +
> + return 0;
> +}
> +
> +/* General helper functions */
> +
> +static int tpacpi_battery_get_id(const char *battery_name)
> +{
> +
> + if (strcmp(battery_name, "BAT0") == 0)
> + return BAT_PRIMARY;
> + else if (strcmp(battery_name, "BAT1") == 0)
1. Redundant 'else'.
2. Consider to use match_string().
> +}
> +static int tpacpi_battery_get_attr(struct device_attribute *attr)
> +{
> + if (strcmp(START_ATTR, attr->attr.name) == 0)
> + return THRESHOLD_START;
> + else if (strcmp(STOP_ATTR, attr->attr.name) == 0)
> + return THRESHOLD_STOP;
> +
> + pr_crit("Invalid attribute: %s", attr->attr.name);
> + return -EINVAL;
> +}
> +
> +/* sysfs interface */
> +
> +static ssize_t tpacpi_battery_store(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + int attr, battery;
> + long value;
> + struct power_supply *supply =
> + container_of(dev, struct power_supply, dev);
drivers/power/supply/power_supply_core.c:671:
Perhaps move it to the header of power supply subsystem?
> +
> + if (!supply) {
> + pr_crit("Can't upcast to power_supply!");
I recommend to reconsider levels on which you print some messages
here. Do we really need KERN_CRIT?
> + return -ENODEV;
> + }
> +
> + if (battery_info.individual_addressing)
> + /* BAT_PRIMARY or BAT_SECONDARY */
> + battery = tpacpi_battery_get_id(supply->desc->name);
> + else
> + battery = BAT_PRIMARY;
> +
> + if (kstrtol(buf, 10, &value))
...ul()?
> + return -EINVAL;
> +
> + attr = tpacpi_battery_get_attr(devattr);
> +
> + value = value == 100 ? 0 : value;
if (value == 100)
value = 0;
> +}
> +
> +static ssize_t tpacpi_battery_show(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + int ret = 0x0, attr, battery;
> + struct power_supply *supply =
> + container_of(dev, struct power_supply, dev);
See above.
> +}
> +
> +static const struct device_attribute battery_start = {
> + .show = tpacpi_battery_show,
> + .store = tpacpi_battery_store,
> + .attr = {
> + .name = START_ATTR,
> + .mode = 0644,
> + },
> +};
> +
> +static const struct device_attribute battery_stop = {
> + .show = tpacpi_battery_show,
> + .store = tpacpi_battery_store,
> + .attr = {
> + .name = STOP_ATTR,
> + .mode = 0644,
> + },
> +};
Don't we have some helper macros for above?
Something based on __ATTR_RW() ?
> +static int tpacpi_battery_add(struct power_supply *battery)
> +{
> + int batteryid = tpacpi_battery_get_id(battery->desc->name);
> +
> + pr_info("battery %s added", battery->desc->name);
> +
> + if (tpacpi_battery_probe(batteryid))
> + return -ENODEV;
> +
> + if (device_create_file(&battery->dev, &battery_start))
> + return -ENODEV;
> +
> + if (device_create_file(&battery->dev, &battery_stop))
> + return -ENODEV;
Shouldn't be done via group attribures?
> +
> + return 0;
> +}
> +static int __init tpacpi_battery_init(struct ibm_init_struct *ibm)
> +{
> + tp_features.battery = 0;
> + battery_hook_register(&battery_hook);
> +
> + pr_info("battery driver initialized");
This kind of messages is somehow useless. 'initcall_debug' will show you this.
> +
> + battery_info.individual_addressing = 0;
> + tp_features.battery = 1;
> +
> + return 0;
> +}
> +
> +static void tpacpi_battery_exit(void)
> +{
> + if (!tp_features.battery) {
> + pr_info("battery driver was not loaded, not de-registering");
> + return;
> + }
When might it happen?
> +
> + battery_hook_unregister(&battery_hook);
> +}
--
With Best Regards,
Andy Shevchenko
|
