Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH V5] platform/chrome: mfd/cros_ec_dev: Add sysfile to force
To:     RaviChandra Sadineni <ravisadineni@chromium.org>
Cc:     Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        linux-kernel@vger.kernel.org, Lee Jones <lee.jones@linaro.org>,
        tbroch@google.com
References: <404dd7fe-23de-e5c4-48c2-5cd77a14e72b@collabora.com>
 <20190309014239.79938-1-ravisadineni@chromium.org>
From:   Enric Balletbo i Serra <enric.balletbo@collabora.com>
Message-ID: <56681289-e231-4d83-b6d4-1a05e7f91146@collabora.com>
Date:   Wed, 13 Mar 2019 18:38:55 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.1
MIME-Version: 1.0
In-Reply-To: <20190309014239.79938-1-ravisadineni@chromium.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-GB
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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