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
