lkml.org 
[lkml]   [2019]   [Mar]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH V5] platform/chrome: mfd/cros_ec_dev: Add sysfile to force
From
Date
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 <ravisadineni@chromium.org>
> ---
> 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 <ravisadineni@chromium.org>
> +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
\
 
 \ /
  Last update: 2019-03-13 18:40    [W:0.096 / U:0.076 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site