On chromebooks, power_manager daemon normally shutsdown(S5) the device
when the battery charge falls below 4% threshold. Chromeos EC then
normally spends an hour in S5 before hibernating. If the battery charge
falls below critical threshold in the mean time, EC does a battery cutoff
instead of hibernating. On some chromebooks, S5 is optimal enough
resulting in EC hibernating without battery cut-off. This results in
battery deep-discharging. This is a bad user experience as battery
has to trickle charge before booting when the A.C is plugged in the next
time.
This patch exposes a sysfs file for an userland daemon to suggest EC if it
has to do a battery cut-off instead of hibernating when the system enters
S5 next time.
Signed-off-by: RaviChandra Sadineni <[email protected]>
---
V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
V2: Use kstrtobool() instead of kstrtou8() and add documentation.
.../ABI/testing/sysfs-class-chromeos | 10 ++++
drivers/platform/chrome/cros_ec_sysfs.c | 48 +++++++++++++++++++
2 files changed, 58 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos
diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
new file mode 100644
index 000000000000..d5ab22c44977
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-chromeos
@@ -0,0 +1,10 @@
+What: /sys/class/chromeos/cros_ec/battery-cuttoff
+Date: February 2019
+Contact: Ravi Chandra Sadineni <[email protected]>
+Description:
+ cros_ec battery cuttoff configuration. Only option
+ currently exposed is 'at-shutdown'.
+
+ 'at-shutdown' sends a host command to EC requesting
+ battery cutoff on next shutdown. If AC is plugged
+ in before next shutdown, EC ignores the request.
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index f34a50121064..6ef6b860c818 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -322,14 +322,62 @@ static ssize_t kb_wake_angle_store(struct device *dev,
return count;
}
+/* Battery cut-off control */
+static ssize_t battery_cutoff_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ec_params_battery_cutoff *param;
+ struct cros_ec_command *msg;
+ int ret;
+ struct cros_ec_dev *ec =
+ container_of(dev, struct cros_ec_dev, class_dev);
+ char *p;
+ int len;
+
+ msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ param = (struct ec_params_battery_cutoff *)msg->data;
+ msg->command = EC_CMD_BATTERY_CUT_OFF + ec->cmd_offset;
+ msg->version = 1;
+ msg->outsize = sizeof(*param);
+ msg->insize = 0;
+
+ p = memchr(buf, '\n', count);
+ len = p ? p - buf : count;
+
+ if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
+ param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
+ } else {
+ kfree(msg);
+ return -EINVAL;
+ }
+
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+ kfree(msg);
+ if (ret < 0)
+ return ret;
+ return count;
+}
+
/* Module initialization */
static DEVICE_ATTR_RW(reboot);
static DEVICE_ATTR_RO(version);
static DEVICE_ATTR_RO(flashinfo);
static DEVICE_ATTR_RW(kb_wake_angle);
+/*
+ * Currently EC does not expose a host command to read the status of
+ * battery cut-off configuration. Also there is no requirement to read
+ * the status of these flags from userland. So marking this attribute as
+ * write-only.
+ */
+static DEVICE_ATTR_WO(battery_cutoff);
static struct attribute *__ec_attrs[] = {
+ &dev_attr_battery_cutoff.attr,
&dev_attr_kb_wake_angle.attr,
&dev_attr_reboot.attr,
&dev_attr_version.attr,
--
2.20.1
Hi RaviChandra,
Some few comments below ...
On 5/3/19 1:53, RaviChandra Sadineni wrote:
> On chromebooks, power_manager daemon normally shutsdown(S5) the device
> when the battery charge falls below 4% threshold. Chromeos EC then
> normally spends an hour in S5 before hibernating. If the battery charge
> falls below critical threshold in the mean time, EC does a battery cutoff
Be coherent using the same spelling along all the patch for cutoff.
> instead of hibernating. On some chromebooks, S5 is optimal enough
> resulting in EC hibernating without battery cut-off. This results in
s/cut-off/cutoff/
> battery deep-discharging. This is a bad user experience as battery
> has to trickle charge before booting when the A.C is plugged in the next
> time.
>
> This patch exposes a sysfs file for an userland daemon to suggest EC if it
> has to do a battery cut-off instead of hibernating when the system enters
s/cut-off/cutoff/
> S5 next time.
>
> Signed-off-by: RaviChandra Sadineni <[email protected]>
> ---
>
> V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
> V2: Use kstrtobool() instead of kstrtou8() and add documentation.
>
>
> .../ABI/testing/sysfs-class-chromeos | 10 ++++
> drivers/platform/chrome/cros_ec_sysfs.c | 48 +++++++++++++++++++
> 2 files changed, 58 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos
>
> diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
> new file mode 100644
> index 000000000000..d5ab22c44977
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-chromeos
Please rebase the patch on top of chrome-platform for-next [1] or 5.1-rc1 when
released, so it applies cleanly.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/log/?h=for-next
> @@ -0,0 +1,10 @@
> +What: /sys/class/chromeos/cros_ec/battery-cuttoff
The name should match with the attribute name, so battery_cutoff in this case as
you defined the attribute name as battery_cutoff
Is this a common property for all the EC devices?
Note that we have
/sys/class/chromeos/<ec-device-name>/...
where <ec-device-name> can be cros_ec|cros_pd and many others in the future
On those devices that more than one EC is present we will have. E.g
cros_ec/battery_cutoff and cros_pd/battery_cutoff
cros_ec/battery_cutoff and cros_ish/battery_cutoff
Which I think is not what you want. Ideally I'd like to see visible the
attribute only on those ECs that support that feature. Is this command
associated with an EC feature?
If that's not possible I can assume writing to battery_cutoff for ECs that don't
implement it would return a protocol/command error.
> +Date: February 2019
> +Contact: Ravi Chandra Sadineni <[email protected]>
> +Description:
> + cros_ec battery cuttoff configuration. Only option
> + currently exposed is 'at-shutdown'.
> +
> + 'at-shutdown' sends a host command to EC requesting
> + battery cutoff on next shutdown. If AC is plugged
> + in before next shutdown, EC ignores the request.
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> index f34a50121064..6ef6b860c818 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -322,14 +322,62 @@ static ssize_t kb_wake_angle_store(struct device *dev,
> return count;
> }
>
> +/* Battery cut-off control */
s/cut-off/cutoff/
> +static ssize_t battery_cutoff_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ec_params_battery_cutoff *param;
> + struct cros_ec_command *msg;
> + int ret;
> + struct cros_ec_dev *ec =
> + container_of(dev, struct cros_ec_dev, class_dev);
use to_cros_ec_dev instead of container_of
> + char *p;
> + int len;
> +
> + msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + param = (struct ec_params_battery_cutoff *)msg->data;
> + msg->command = EC_CMD_BATTERY_CUT_OFF + ec->cmd_offset;
> + msg->version = 1;
> + msg->outsize = sizeof(*param);
> + msg->insize = 0;
> +
> + p = memchr(buf, '\n', count);
> + len = p ? p - buf : count;
> +
> + if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
> + param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
> + } else {
> + kfree(msg);
Use only one kfree, please. Remove this.
> + return -EINVAL;
count = -EINVAL;
goto exit;
> + }
> +
> + ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> + kfree(msg);
Only one point for kfree, please. Remove this.
> + if (ret < 0)
> + return ret;
count = ret;
exit:
kfree(msg);
> + return count;
> +}
> +
> /* Module initialization */
>
> static DEVICE_ATTR_RW(reboot);
> static DEVICE_ATTR_RO(version);
> static DEVICE_ATTR_RO(flashinfo);
> static DEVICE_ATTR_RW(kb_wake_angle);
> +/*
> + * Currently EC does not expose a host command to read the status of
> + * battery cut-off configuration. Also there is no requirement to read
s/cut-off/cutoff/
> + * the status of these flags from userland. So marking this attribute as
> + * write-only.
> + */
Can you also specify that is WO in the sysfs documentation. I understand
correctly that this command only applies when the device runs on battery? And
that if AC is plugged the flag is reset?
> +static DEVICE_ATTR_WO(battery_cutoff);
>
Note that the name should match with the sysfs documentation.
> static struct attribute *__ec_attrs[] = {
> + &dev_attr_battery_cutoff.attr,
> &dev_attr_kb_wake_angle.attr,
> &dev_attr_reboot.attr,
> &dev_attr_version.attr,
>
Thanks,
Enric
From: RaviChandra Sadineni <[email protected]>
On chromebooks, power_manager daemon normally shuts down(S5) the device
when the battery charge falls below 4% threshold. ChromeOS EC then
normally spends an hour in S5 before hibernating. If the battery charge
falls below critical threshold in the mean time, EC does a battery cutoff
instead of hibernating. On some chromebooks, S5 is optimal enough
resulting in EC hibernating without battery cutoff. This results in
battery deep discharging. This is a bad user experience as battery
has to trickle charge before booting when the AC is plugged in the next
time.
This patch exposes a sysfs file for an userland daemon to suggest EC if it
has to do a battery cutoff instead of hibernating when the system enters
S5 next time.
Signed-off-by: RaviChandra Sadineni <[email protected]>
---
V4: Addressed comments from Enric.
V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
V2: Use kstrtobool() instead of kstrtou8() and add documentation.
.../ABI/testing/sysfs-class-chromeos | 16 +++++++
drivers/platform/chrome/cros_ec_sysfs.c | 47 +++++++++++++++++++
2 files changed, 63 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
index 5819699d66ec..0927704d1629 100644
--- a/Documentation/ABI/testing/sysfs-class-chromeos
+++ b/Documentation/ABI/testing/sysfs-class-chromeos
@@ -30,3 +30,19 @@ Date: August 2015
KernelVersion: 4.2
Description:
Show the information about the EC software and hardware.
+
+What: /sys/class/chromeos/<ec-device-name>/battery_cuttoff
+Date: February 2019
+Contact: Ravi Chandra Sadineni <[email protected]>
+Description:
+ cros_ec battery cuttoff configuration. Only option
+ currently exposed is 'at-shutdown'.
+
+ 'at-shutdown' sends a host command to EC requesting
+ battery cutoff on next shutdown. If AC is plugged
+ in before next shutdown, EC ignores the request and
+ resets the flag.
+
+ Currently EC does not expose a host command to read
+ the status of battery cutoff configuration. Thus this
+ flag is write-only.
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index fe0b7614ae1b..a35e1188f28f 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -308,14 +308,61 @@ static ssize_t kb_wake_angle_store(struct device *dev,
return count;
}
+/* Battery cutoff control */
+static ssize_t battery_cutoff_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ec_params_battery_cutoff *param;
+ struct cros_ec_command *msg;
+ int ret;
+ struct cros_ec_dev *ec = to_cros_ec_dev(dev);
+ char *p;
+ int len;
+
+ msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ param = (struct ec_params_battery_cutoff *)msg->data;
+ msg->command = EC_CMD_BATTERY_CUT_OFF + ec->cmd_offset;
+ msg->version = 1;
+ msg->outsize = sizeof(*param);
+ msg->insize = 0;
+
+ p = memchr(buf, '\n', count);
+ len = p ? p - buf : count;
+
+ if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
+ param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
+ } else {
+ count = -EINVAL;
+ goto exit;
+ }
+
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+ if (ret < 0)
+ count = ret;
+exit:
+ kfree(msg);
+ return count;
+}
+
/* Module initialization */
static DEVICE_ATTR_RW(reboot);
static DEVICE_ATTR_RO(version);
static DEVICE_ATTR_RO(flashinfo);
static DEVICE_ATTR_RW(kb_wake_angle);
+/*
+ * Currently EC does not expose a host command to read the status of
+ * battery cutoff configuration. Also there is no requirement to read
+ * the flag from userland. So marking this attribute as write-only.
+ */
+static DEVICE_ATTR_WO(battery_cutoff);
static struct attribute *__ec_attrs[] = {
+ &dev_attr_battery_cutoff.attr,
&dev_attr_kb_wake_angle.attr,
&dev_attr_reboot.attr,
&dev_attr_version.attr,
--
2.20.1
On chromebooks, power_manager daemon normally shuts down(S5) the device
when the battery charge falls below 4% threshold. ChromeOS EC then
normally spends an hour in S5 before hibernating. If the battery charge
falls below critical threshold in the mean time, EC does a battery cutoff
instead of hibernating. On some chromebooks, S5 is optimal enough
resulting in EC hibernating without battery cutoff. This results in
battery deep discharging. This is a bad user experience as battery
has to trickle charge before booting when the AC is plugged in the next
time.
This patch exposes a sysfs file for an userland daemon to suggest EC if it
has to do a battery cutoff instead of hibernating when the system enters
S5 next time.
This attribute is present only if EC supports EC_FEATURE_BATTERY.
Signed-off-by: RaviChandra Sadineni <[email protected]>
---
V5: Expose flag only when EC_FEATURE_BATTERY is supported.
V4: Addressed comments from Enric.
V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
V2: Use kstrtobool() instead of kstrtou8() and add documentation.
.../ABI/testing/sysfs-class-chromeos | 16 ++++++
drivers/mfd/cros_ec_dev.c | 4 ++
drivers/platform/chrome/cros_ec_sysfs.c | 49 +++++++++++++++++++
include/linux/mfd/cros_ec.h | 2 +
4 files changed, 71 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
index 5819699d66ec..0927704d1629 100644
--- a/Documentation/ABI/testing/sysfs-class-chromeos
+++ b/Documentation/ABI/testing/sysfs-class-chromeos
@@ -30,3 +30,19 @@ Date: August 2015
KernelVersion: 4.2
Description:
Show the information about the EC software and hardware.
+
+What: /sys/class/chromeos/<ec-device-name>/battery_cuttoff
+Date: February 2019
+Contact: Ravi Chandra Sadineni <[email protected]>
+Description:
+ cros_ec battery cuttoff configuration. Only option
+ currently exposed is 'at-shutdown'.
+
+ 'at-shutdown' sends a host command to EC requesting
+ battery cutoff on next shutdown. If AC is plugged
+ in before next shutdown, EC ignores the request and
+ resets the flag.
+
+ Currently EC does not expose a host command to read
+ the status of battery cutoff configuration. Thus this
+ flag is write-only.
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index ed809fc97df8..7580be23dfb3 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -502,6 +502,10 @@ static int ec_device_probe(struct platform_device *pdev)
retval);
}
+ /* check whether EC_FEATURE_BATTERY is supported. */
+ if (cros_ec_check_features(ec, EC_FEATURE_BATTERY))
+ ec->has_battery = true;
+
return 0;
failed:
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index fe0b7614ae1b..3d9ab55dddc0 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -308,14 +308,61 @@ static ssize_t kb_wake_angle_store(struct device *dev,
return count;
}
+/* Battery cutoff control */
+static ssize_t battery_cutoff_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ec_params_battery_cutoff *param;
+ struct cros_ec_command *msg;
+ int ret;
+ struct cros_ec_dev *ec = to_cros_ec_dev(dev);
+ char *p;
+ int len;
+
+ msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
+ if (!msg)
+ return -ENOMEM;
+
+ param = (struct ec_params_battery_cutoff *)msg->data;
+ msg->command = EC_CMD_BATTERY_CUT_OFF + ec->cmd_offset;
+ msg->version = 1;
+ msg->outsize = sizeof(*param);
+ msg->insize = 0;
+
+ p = memchr(buf, '\n', count);
+ len = p ? p - buf : count;
+
+ if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
+ param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
+ } else {
+ count = -EINVAL;
+ goto exit;
+ }
+
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+ if (ret < 0)
+ count = ret;
+exit:
+ kfree(msg);
+ return count;
+}
+
/* Module initialization */
static DEVICE_ATTR_RW(reboot);
static DEVICE_ATTR_RO(version);
static DEVICE_ATTR_RO(flashinfo);
static DEVICE_ATTR_RW(kb_wake_angle);
+/*
+ * Currently EC does not expose a host command to read the status of
+ * battery cutoff configuration. Also there is no requirement to read
+ * the flag from userland. So marking this attribute as write-only.
+ */
+static DEVICE_ATTR_WO(battery_cutoff);
static struct attribute *__ec_attrs[] = {
+ &dev_attr_battery_cutoff.attr,
&dev_attr_kb_wake_angle.attr,
&dev_attr_reboot.attr,
&dev_attr_version.attr,
@@ -331,6 +378,8 @@ static umode_t cros_ec_ctrl_visible(struct kobject *kobj,
if (a == &dev_attr_kb_wake_angle.attr && !ec->has_kb_wake_angle)
return 0;
+ if (a == &dev_attr_battery_cutoff.attr && !ec->has_battery)
+ return 0;
return a->mode;
}
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 8f2a8918bfa3..de5280c96bd9 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -192,6 +192,7 @@ struct cros_ec_debugfs;
* @has_kb_wake_angle: True if at least 2 accelerometer are connected to the EC.
* @cmd_offset: Offset to apply for each command.
* @features: Features supported by the EC.
+ * @has_battery: True if EC supports EC_FEATURE_BATTERY.
*/
struct cros_ec_dev {
struct device class_dev;
@@ -202,6 +203,7 @@ struct cros_ec_dev {
bool has_kb_wake_angle;
u16 cmd_offset;
u32 features[2];
+ bool has_battery;
};
#define to_cros_ec_dev(dev) container_of(dev, struct cros_ec_dev, class_dev)
--
2.20.1
Hi Enric,
Thanks for the review.
On Thu, Mar 7, 2019 at 3:57 AM Enric Balletbo i Serra
<[email protected]> wrote:
>
> Hi RaviChandra,
>
> Some few comments below ...
>
>
> On 5/3/19 1:53, RaviChandra Sadineni wrote:
> > On chromebooks, power_manager daemon normally shutsdown(S5) the device
> > when the battery charge falls below 4% threshold. Chromeos EC then
> > normally spends an hour in S5 before hibernating. If the battery charge
> > falls below critical threshold in the mean time, EC does a battery cutoff
>
> Be coherent using the same spelling along all the patch for cutoff.
Done.
>
>
> > instead of hibernating. On some chromebooks, S5 is optimal enough
> > resulting in EC hibernating without battery cut-off. This results in
>
> s/cut-off/cutoff/
Done.
>
> > battery deep-discharging. This is a bad user experience as battery
> > has to trickle charge before booting when the A.C is plugged in the next
> > time.
> >
> > This patch exposes a sysfs file for an userland daemon to suggest EC if it
> > has to do a battery cut-off instead of hibernating when the system enters
>
> s/cut-off/cutoff/
Done.
>
> > S5 next time.
> >
> > Signed-off-by: RaviChandra Sadineni <[email protected]>
> > ---
> >
> > V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
> > V2: Use kstrtobool() instead of kstrtou8() and add documentation.
> >
> >
> > .../ABI/testing/sysfs-class-chromeos | 10 ++++
> > drivers/platform/chrome/cros_ec_sysfs.c | 48 +++++++++++++++++++
> > 2 files changed, 58 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-class-chromeos
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
> > new file mode 100644
> > index 000000000000..d5ab22c44977
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-chromeos
>
> Please rebase the patch on top of chrome-platform for-next [1] or 5.1-rc1 when
> released, so it applies cleanly.
Done.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/log/?h=for-next
>
> > @@ -0,0 +1,10 @@
> > +What: /sys/class/chromeos/cros_ec/battery-cuttoff
>
> The name should match with the attribute name, so battery_cutoff in this case as
> you defined the attribute name as battery_cutoff
>
> Is this a common property for all the EC devices?
>
> Note that we have
>
> /sys/class/chromeos/<ec-device-name>/...
>
> where <ec-device-name> can be cros_ec|cros_pd and many others in the future
>
> On those devices that more than one EC is present we will have. E.g
>
> cros_ec/battery_cutoff and cros_pd/battery_cutoff
> cros_ec/battery_cutoff and cros_ish/battery_cutoff
>
> Which I think is not what you want. Ideally I'd like to see visible the
> attribute only on those ECs that support that feature. Is this command
> associated with an EC feature?
Yes with EC_FEATURE_BATTERY. Now checking if EC supports EC_FEATURE_BATTERY.
>
> If that's not possible I can assume writing to battery_cutoff for ECs that don't
> implement it would return a protocol/command error.
>
>
> > +Date: February 2019
> > +Contact: Ravi Chandra Sadineni <[email protected]>
> > +Description:
> > + cros_ec battery cuttoff configuration. Only option
> > + currently exposed is 'at-shutdown'.
> > +
> > + 'at-shutdown' sends a host command to EC requesting
> > + battery cutoff on next shutdown. If AC is plugged
> > + in before next shutdown, EC ignores the request.
> > diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> > index f34a50121064..6ef6b860c818 100644
> > --- a/drivers/platform/chrome/cros_ec_sysfs.c
> > +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> > @@ -322,14 +322,62 @@ static ssize_t kb_wake_angle_store(struct device *dev,
> > return count;
> > }
> >
> > +/* Battery cut-off control */
>
> s/cut-off/cutoff/
Done.
>
> > +static ssize_t battery_cutoff_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct ec_params_battery_cutoff *param;
> > + struct cros_ec_command *msg;
> > + int ret;
> > + struct cros_ec_dev *ec =
> > + container_of(dev, struct cros_ec_dev, class_dev);
>
> use to_cros_ec_dev instead of container_of
>
> > + char *p;
> > + int len;
> > +
> > + msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
> > + if (!msg)
> > + return -ENOMEM;
> > +
> > + param = (struct ec_params_battery_cutoff *)msg->data;
> > + msg->command = EC_CMD_BATTERY_CUT_OFF + ec->cmd_offset;
> > + msg->version = 1;
> > + msg->outsize = sizeof(*param);
> > + msg->insize = 0;
> > +
> > + p = memchr(buf, '\n', count);
> > + len = p ? p - buf : count;
> > +
> > + if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
> > + param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
> > + } else {
> > + kfree(msg);
>
> Use only one kfree, please. Remove this.
>
> > + return -EINVAL;
>
> count = -EINVAL;
> goto exit;
>
> > + }
> > +
> > + ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> > + kfree(msg);
>
> Only one point for kfree, please. Remove this.
>
> > + if (ret < 0)
> > + return ret;
> count = ret;
>
> exit:
> kfree(msg);
> > + return count;
> > +}
> > +
> > /* Module initialization */
> >
> > static DEVICE_ATTR_RW(reboot);
> > static DEVICE_ATTR_RO(version);
> > static DEVICE_ATTR_RO(flashinfo);
> > static DEVICE_ATTR_RW(kb_wake_angle);
> > +/*
> > + * Currently EC does not expose a host command to read the status of
> > + * battery cut-off configuration. Also there is no requirement to read
>
> s/cut-off/cutoff/
>
> > + * the status of these flags from userland. So marking this attribute as
> > + * write-only.
> > + */
>
> Can you also specify that is WO in the sysfs documentation. I understand
> correctly that this command only applies when the device runs on battery? And
> that if AC is plugged the flag is reset?
Done.
Thanks,
Ravi
>
> > +static DEVICE_ATTR_WO(battery_cutoff);
> >
>
> Note that the name should match with the sysfs documentation.
>
> > static struct attribute *__ec_attrs[] = {
> > + &dev_attr_battery_cutoff.attr,
> > &dev_attr_kb_wake_angle.attr,
> > &dev_attr_reboot.attr,
> > &dev_attr_version.attr,
> >
>
> Thanks,
> Enric
Hi RaviChandra,
Many thanks for pushing this upstream. Some more comments below.
On 9/3/19 2:42, RaviChandra Sadineni wrote:
> On chromebooks, power_manager daemon normally shuts down(S5) the device
> when the battery charge falls below 4% threshold. ChromeOS EC then
> normally spends an hour in S5 before hibernating. If the battery charge
> falls below critical threshold in the mean time, EC does a battery cutoff
> instead of hibernating. On some chromebooks, S5 is optimal enough
> resulting in EC hibernating without battery cutoff. This results in
> battery deep discharging. This is a bad user experience as battery
> has to trickle charge before booting when the AC is plugged in the next
> time.
>
> This patch exposes a sysfs file for an userland daemon to suggest EC if it
> has to do a battery cutoff instead of hibernating when the system enters
> S5 next time.
>
> This attribute is present only if EC supports EC_FEATURE_BATTERY.
>
> Signed-off-by: RaviChandra Sadineni <[email protected]>
> ---
> V5: Expose flag only when EC_FEATURE_BATTERY is supported.
> V4: Addressed comments from Enric.
> V3: Make battery-cutoff generic and expose 'at-shutdown' flag.
> V2: Use kstrtobool() instead of kstrtou8() and add documentation.
>
> .../ABI/testing/sysfs-class-chromeos | 16 ++++++
> drivers/mfd/cros_ec_dev.c | 4 ++
> drivers/platform/chrome/cros_ec_sysfs.c | 49 +++++++++++++++++++
> include/linux/mfd/cros_ec.h | 2 +
> 4 files changed, 71 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-chromeos b/Documentation/ABI/testing/sysfs-class-chromeos
> index 5819699d66ec..0927704d1629 100644
> --- a/Documentation/ABI/testing/sysfs-class-chromeos
> +++ b/Documentation/ABI/testing/sysfs-class-chromeos
> @@ -30,3 +30,19 @@ Date: August 2015
> KernelVersion: 4.2
> Description:
> Show the information about the EC software and hardware.
> +
> +What: /sys/class/chromeos/<ec-device-name>/battery_cuttoff
Typo s/battery_cuttoff/battery_cutoff/
> +Date: February 2019
> +Contact: Ravi Chandra Sadineni <[email protected]>
> +Description:
> + cros_ec battery cuttoff configuration. Only option
Typo s/battery_cuttoff/battery_cutoff/
> + currently exposed is 'at-shutdown'.
> +
> + 'at-shutdown' sends a host command to EC requesting
> + battery cutoff on next shutdown. If AC is plugged
> + in before next shutdown, EC ignores the request and
> + resets the flag.
> +
> + Currently EC does not expose a host command to read
> + the status of battery cutoff configuration. Thus this
> + flag is write-only.
> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> index ed809fc97df8..7580be23dfb3 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -502,6 +502,10 @@ static int ec_device_probe(struct platform_device *pdev)
> retval);
> }
>
> + /* check whether EC_FEATURE_BATTERY is supported. */
> + if (cros_ec_check_features(ec, EC_FEATURE_BATTERY))
> + ec->has_battery = true;
> +
You can check in cros-ec-sysfs driver if the feature is supported, no need to do
this here and add a new variable. See below.
> return 0;
>
> failed:
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> index fe0b7614ae1b..3d9ab55dddc0 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -308,14 +308,61 @@ static ssize_t kb_wake_angle_store(struct device *dev,
> return count;
> }
>
> +/* Battery cutoff control */
> +static ssize_t battery_cutoff_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ec_params_battery_cutoff *param;
> + struct cros_ec_command *msg;
> + int ret;
> + struct cros_ec_dev *ec = to_cros_ec_dev(dev);
> + char *p;
> + int len;
nit: Reversed X-mas tree order if you don't mind.
> +
> + msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + param = (struct ec_params_battery_cutoff *)msg->data;
> + msg->command = EC_CMD_BATTERY_CUT_OFF + ec->cmd_offset;
> + msg->version = 1;
I looked at ectool cmd_battery_cut_off and if I understand correctly this
command is also possible for protocol version 0. I am wondering if we should
also handle this case. There is any reason to don't do that?
> + msg->outsize = sizeof(*param);
> + msg->insize = 0;
> +
> + p = memchr(buf, '\n', count);
> + len = p ? p - buf : count;
> +
> + if (len == 11 && !strncmp(buf, "at-shutdown", len)) {
> + param->flags = EC_BATTERY_CUTOFF_FLAG_AT_SHUTDOWN;
Can you prepare this for future flags defining and array of strings and using
sysfs_match_string helper?
> + } else {
> + count = -EINVAL;
> + goto exit;
> + }
> +
> + ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> + if (ret < 0)
> + count = ret;
> +exit:
> + kfree(msg);
> + return count;
> +}
> +
> /* Module initialization */
>
> static DEVICE_ATTR_RW(reboot);
> static DEVICE_ATTR_RO(version);
> static DEVICE_ATTR_RO(flashinfo);
> static DEVICE_ATTR_RW(kb_wake_angle);
> +/*
> + * Currently EC does not expose a host command to read the status of
> + * battery cutoff configuration. Also there is no requirement to read
> + * the flag from userland. So marking this attribute as write-only.
> + */
> +static DEVICE_ATTR_WO(battery_cutoff);
>
> static struct attribute *__ec_attrs[] = {
> + &dev_attr_battery_cutoff.attr,
> &dev_attr_kb_wake_angle.attr,
> &dev_attr_reboot.attr,
> &dev_attr_version.attr,
> @@ -331,6 +378,8 @@ static umode_t cros_ec_ctrl_visible(struct kobject *kobj,
>
> if (a == &dev_attr_kb_wake_angle.attr && !ec->has_kb_wake_angle)
> return 0;
> + if (a == &dev_attr_battery_cutoff.attr && !ec->has_battery)
> + return 0;
>
You can check directly if the feature is available. i.e
static int cros_ec_has_battery(struct cros_ec_dev *ec)
{
return ec->features[EC_FEATURE_BATTERY / 32] &
EC_FEATURE_MASK_0(EC_FEATURE_BATTERY);
}
> return a->mode;
> }
> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
> index 8f2a8918bfa3..de5280c96bd9 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -192,6 +192,7 @@ struct cros_ec_debugfs;
> * @has_kb_wake_angle: True if at least 2 accelerometer are connected to the EC.
> * @cmd_offset: Offset to apply for each command.
> * @features: Features supported by the EC.
> + * @has_battery: True if EC supports EC_FEATURE_BATTERY.
> */
> struct cros_ec_dev {
> struct device class_dev;
> @@ -202,6 +203,7 @@ struct cros_ec_dev {
> bool has_kb_wake_angle;
> u16 cmd_offset;
> u32 features[2];
> + bool has_battery;
Not needed.
Note that the reason why we have a has_kb_wake_angle is because there isn't an
EC_FEATURE for the wake_angle propriety, but this is not the case here.
features[2] has this info.
> };
>
> #define to_cros_ec_dev(dev) container_of(dev, struct cros_ec_dev, class_dev)
>
Thanks,
Enric