Hi Enric, Thanks for the review.
On Thu, Mar 7, 2019 at 3:57 AM Enric Balletbo i Serra <enric.balle...@collabora.com> 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 <ravisadin...@chromium.org> > > --- > > > > 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 <ravisadin...@chromium.org> > > +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
