Re: [PATCH RESEND 11/13] platform/chrome: cros_ec_lightbar - Control of suspend/resume lightbar sequence

[Benson Leung]

Hi Enric,

On 05/16/2017 09:13 AM, Enric Balletbo i Serra wrote:
> From: Eric Caruso <ejcaruso@xxxxxxxxxxxx>
> 
> Don't let EC control suspend/resume sequence. If the EC controls the
> lightbar and sets the sequence when it notices the chipset transitioning
> between states, we can't make exceptions for cases where we don't want
> to activate the lightbar. Instead, let's move the suspend/resume
> notifications into the kernel so we can selectively play the sequences.
> 
> Signed-off-by: Eric Caruso <ejcaruso@xxxxxxxxxxxx>
> Signed-off-by: Guenter Roeck <groeck@xxxxxxxxxxxx>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> Acked-by: Lee Jones <lee.jones@xxxxxxxxxx>

Signed-off-by: Benson Leung <bleung@xxxxxxxxxxxx>

Applied. Thanks.

> ---
>  drivers/platform/chrome/cros_ec_dev.c      | 37 +++++++++++++
>  drivers/platform/chrome/cros_ec_dev.h      |  6 +++
>  drivers/platform/chrome/cros_ec_lightbar.c | 85 ++++++++++++++++++++++++++++--
>  include/linux/mfd/cros_ec_commands.h       | 11 +++-
>  4 files changed, 134 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_dev.c b/drivers/platform/chrome/cros_ec_dev.c
> index 20ce1c2..7c26223 100644
> --- a/drivers/platform/chrome/cros_ec_dev.c
> +++ b/drivers/platform/chrome/cros_ec_dev.c
> @@ -21,6 +21,7 @@
>  #include <linux/mfd/core.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm.h>
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  
> @@ -435,6 +436,10 @@ static int ec_device_probe(struct platform_device *pdev)
>  	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
>  		cros_ec_sensors_register(ec);
>  
> +	/* Take control of the lightbar from the EC. */
> +	if (ec_has_lightbar(ec))
> +		lb_manual_suspend_ctrl(ec, 1);
> +
>  	return 0;
>  
>  failed:
> @@ -446,6 +451,10 @@ static int ec_device_remove(struct platform_device *pdev)
>  {
>  	struct cros_ec_dev *ec = dev_get_drvdata(&pdev->dev);
>  
> +	/* Let the EC take over the lightbar again. */
> +	if (ec_has_lightbar(ec))
> +		lb_manual_suspend_ctrl(ec, 0);
> +
>  	cros_ec_debugfs_remove(ec);
>  
>  	cdev_del(&ec->cdev);
> @@ -459,9 +468,37 @@ static const struct platform_device_id cros_ec_id[] = {
>  };
>  MODULE_DEVICE_TABLE(platform, cros_ec_id);
>  
> +static int ec_device_suspend(struct device *dev)
> +{
> +	struct cros_ec_dev *ec = dev_get_drvdata(dev);
> +
> +	if (ec_has_lightbar(ec))
> +		lb_suspend(ec);
> +
> +	return 0;
> +}
> +
> +static int ec_device_resume(struct device *dev)
> +{
> +	struct cros_ec_dev *ec = dev_get_drvdata(dev);
> +
> +	if (ec_has_lightbar(ec))
> +		lb_resume(ec);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops cros_ec_dev_pm_ops = {
> +#ifdef CONFIG_PM_SLEEP
> +	.suspend = ec_device_suspend,
> +	.resume = ec_device_resume,
> +#endif
> +};
> +
>  static struct platform_driver cros_ec_dev_driver = {
>  	.driver = {
>  		.name = "cros-ec-ctl",
> +		.pm = &cros_ec_dev_pm_ops,
>  	},
>  	.probe = ec_device_probe,
>  	.remove = ec_device_remove,
> diff --git a/drivers/platform/chrome/cros_ec_dev.h b/drivers/platform/chrome/cros_ec_dev.h
> index bfd2c84..45e9453 100644
> --- a/drivers/platform/chrome/cros_ec_dev.h
> +++ b/drivers/platform/chrome/cros_ec_dev.h
> @@ -43,4 +43,10 @@ struct cros_ec_readmem {
>  #define CROS_EC_DEV_IOCXCMD   _IOWR(CROS_EC_DEV_IOC, 0, struct cros_ec_command)
>  #define CROS_EC_DEV_IOCRDMEM  _IOWR(CROS_EC_DEV_IOC, 1, struct cros_ec_readmem)
>  
> +/* Lightbar utilities */
> +extern bool ec_has_lightbar(struct cros_ec_dev *ec);
> +extern int lb_manual_suspend_ctrl(struct cros_ec_dev *ec, uint8_t enable);
> +extern int lb_suspend(struct cros_ec_dev *ec);
> +extern int lb_resume(struct cros_ec_dev *ec);
> +
>  #endif /* _CROS_EC_DEV_H_ */
> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
> index 2667505..4df379d 100644
> --- a/drivers/platform/chrome/cros_ec_lightbar.c
> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
> @@ -341,6 +341,80 @@ static ssize_t sequence_show(struct device *dev,
>  	return ret;
>  }
>  
> +static int lb_send_empty_cmd(struct cros_ec_dev *ec, uint8_t cmd)
> +{
> +	struct ec_params_lightbar *param;
> +	struct cros_ec_command *msg;
> +	int ret;
> +
> +	msg = alloc_lightbar_cmd_msg(ec);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	param = (struct ec_params_lightbar *)msg->data;
> +	param->cmd = cmd;
> +
> +	ret = lb_throttle();
> +	if (ret)
> +		goto error;
> +
> +	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
> +	if (ret < 0)
> +		goto error;
> +	if (msg->result != EC_RES_SUCCESS) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +	ret = 0;
> +error:
> +	kfree(msg);
> +
> +	return ret;
> +}
> +
> +int lb_manual_suspend_ctrl(struct cros_ec_dev *ec, uint8_t enable)
> +{
> +	struct ec_params_lightbar *param;
> +	struct cros_ec_command *msg;
> +	int ret;
> +
> +	msg = alloc_lightbar_cmd_msg(ec);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	param = (struct ec_params_lightbar *)msg->data;
> +
> +	param->cmd = LIGHTBAR_CMD_MANUAL_SUSPEND_CTRL;
> +	param->manual_suspend_ctrl.enable = enable;
> +
> +	ret = lb_throttle();
> +	if (ret)
> +		goto error;
> +
> +	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
> +	if (ret < 0)
> +		goto error;
> +	if (msg->result != EC_RES_SUCCESS) {
> +		ret = -EINVAL;
> +		goto error;
> +	}
> +	ret = 0;
> +error:
> +	kfree(msg);
> +
> +	return ret;
> +}
> +
> +int lb_suspend(struct cros_ec_dev *ec)
> +{
> +	return lb_send_empty_cmd(ec, LIGHTBAR_CMD_SUSPEND);
> +}
> +
> +int lb_resume(struct cros_ec_dev *ec)
> +{
> +	return lb_send_empty_cmd(ec, LIGHTBAR_CMD_RESUME);
> +}
> +
>  static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
>  			      const char *buf, size_t count)
>  {
> @@ -473,6 +547,11 @@ static struct attribute *__lb_cmds_attrs[] = {
>  	NULL,
>  };
>  
> +bool ec_has_lightbar(struct cros_ec_dev *ec)
> +{
> +	return !!get_lightbar_version(ec, NULL, NULL);
> +}
> +
>  static umode_t cros_ec_lightbar_attrs_are_visible(struct kobject *kobj,
>  						  struct attribute *a, int n)
>  {
> @@ -489,10 +568,10 @@ static umode_t cros_ec_lightbar_attrs_are_visible(struct kobject *kobj,
>  		return 0;
>  
>  	/* Only instantiate this stuff if the EC has a lightbar */
> -	if (get_lightbar_version(ec, NULL, NULL))
> +	if (ec_has_lightbar(ec))
>  		return a->mode;
> -	else
> -		return 0;
> +
> +	return 0;
>  }
>  
>  struct attribute_group cros_ec_lightbar_attr_group = {
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index dbea580..190c8f4 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -1175,7 +1175,7 @@ struct ec_params_lightbar {
>  		struct {
>  			/* no args */
>  		} dump, off, on, init, get_seq, get_params_v0, get_params_v1,
> -			version, get_brightness, get_demo;
> +			version, get_brightness, get_demo, suspend, resume;
>  
>  		struct {
>  			uint8_t num;
> @@ -1193,6 +1193,10 @@ struct ec_params_lightbar {
>  			uint8_t led;
>  		} get_rgb;
>  
> +		struct {
> +			uint8_t enable;
> +		} manual_suspend_ctrl;
> +
>  		struct lightbar_params_v0 set_params_v0;
>  		struct lightbar_params_v1 set_params_v1;
>  		struct lightbar_program set_program;
> @@ -1229,7 +1233,7 @@ struct ec_response_lightbar {
>  			/* no return params */
>  		} off, on, init, set_brightness, seq, reg, set_rgb,
>  			demo, set_params_v0, set_params_v1,
> -			set_program;
> +			set_program, manual_suspend_ctrl, suspend, resume;
>  	};
>  } __packed;
>  
> @@ -1254,6 +1258,9 @@ enum lightbar_command {
>  	LIGHTBAR_CMD_GET_PARAMS_V1 = 16,
>  	LIGHTBAR_CMD_SET_PARAMS_V1 = 17,
>  	LIGHTBAR_CMD_SET_PROGRAM = 18,
> +	LIGHTBAR_CMD_MANUAL_SUSPEND_CTRL = 19,
> +	LIGHTBAR_CMD_SUSPEND = 20,
> +	LIGHTBAR_CMD_RESUME = 21,
>  	LIGHTBAR_NUM_CMDS
>  };
>  
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@xxxxxxxxxx
Chromium OS Project
bleung@xxxxxxxxxxxx

Attachment: signature.asc
Description: OpenPGP digital signature