Message-ID: <20170622234307.GA126262@decatoncale.mtv.corp.google.com>
Date:   Thu, 22 Jun 2017 16:43:07 -0700
From:   Benson Leung <bleung@...gle.com>
To:     Enric Balletbo i Serra <enric.balletbo@...labora.com>
Cc:     olof@...om.net, bleung@...omium.org, linux-kernel@...r.kernel.org,
        lee.jones@...aro.org, Eric Caruso <ejcaruso@...omium.org>,
        Guenter Roeck <groeck@...omium.org>
Subject: Re: [PATCH RESEND 10/13] platform/chrome: cros_ec_lightbar - Add
 lightbar program feature to sysfs

Hi Enric,

On Tue, May 16, 2017 at 06:13:16PM +0200, Enric Balletbo i Serra wrote:
> From: Eric Caruso <ejcaruso@...omium.org>
> 
> Add a program feature so we can upload and run programs for lightbar
> sequences. We should be able to use this to shift sequences out of the
> EC and save space there.
> 
>   $ cat <suitable program bin> > /sys/devices/.../cros_ec/program
>   $ echo program > /sys/devices/.../cros_ec/sequence
> 
> Signed-off-by: Eric Caruso <ejcaruso@...omium.org>
> Signed-off-by: Guenter Roeck <groeck@...omium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> Acked-by: Lee Jones <lee.jones@...aro.org>

Signed-off-by: Benson Leung <bleung@...omium.org>
Applied.

> ---
>  drivers/platform/chrome/cros_ec_lightbar.c | 69 +++++++++++++++++++++++++++++-
>  include/linux/mfd/cros_ec_commands.h       | 12 +++++-
>  2 files changed, 79 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
> index 8df3d44..2667505 100644
> --- a/drivers/platform/chrome/cros_ec_lightbar.c
> +++ b/drivers/platform/chrome/cros_ec_lightbar.c
> @@ -295,7 +295,8 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
>  
>  static char const *seqname[] = {
>  	"ERROR", "S5", "S3", "S0", "S5S3", "S3S0",
> -	"S0S3", "S3S5", "STOP", "RUN", "PULSE", "TEST", "KONAMI",
> +	"S0S3", "S3S5", "STOP", "RUN", "KONAMI",
> +	"TAP", "PROGRAM",
>  };
>  
>  static ssize_t sequence_show(struct device *dev,
> @@ -390,6 +391,69 @@ static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
>  	return ret;
>  }
>  
> +static ssize_t program_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	int extra_bytes, max_size, ret;
> +	struct ec_params_lightbar *param;
> +	struct cros_ec_command *msg;
> +	struct cros_ec_dev *ec = container_of(dev, struct cros_ec_dev,
> +					      class_dev);
> +
> +	/*
> +	 * We might need to reject the program for size reasons. The EC
> +	 * enforces a maximum program size, but we also don't want to try
> +	 * and send a program that is too big for the protocol. In order
> +	 * to ensure the latter, we also need to ensure we have extra bytes
> +	 * to represent the rest of the packet.
> +	 */
> +	extra_bytes = sizeof(*param) - sizeof(param->set_program.data);
> +	max_size = min(EC_LB_PROG_LEN, ec->ec_dev->max_request - extra_bytes);
> +	if (count > max_size) {
> +		dev_err(dev, "Program is %u bytes, too long to send (max: %u)",
> +			(unsigned int)count, max_size);
> +
> +		return -EINVAL;
> +	}
> +
> +	msg = alloc_lightbar_cmd_msg(ec);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	ret = lb_throttle();
> +	if (ret)
> +		goto exit;
> +
> +	dev_info(dev, "Copying %zu byte program to EC", count);
> +
> +	param = (struct ec_params_lightbar *)msg->data;
> +	param->cmd = LIGHTBAR_CMD_SET_PROGRAM;
> +
> +	param->set_program.size = count;
> +	memcpy(param->set_program.data, buf, count);
> +
> +	/*
> +	 * We need to set the message size manually or else it will use
> +	 * EC_LB_PROG_LEN. This might be too long, and the program
> +	 * is unlikely to use all of the space.
> +	 */
> +	msg->outsize = count + extra_bytes;
> +
> +	ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
> +	if (ret < 0)
> +		goto exit;
> +	if (msg->result != EC_RES_SUCCESS) {
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	ret = count;
> +exit:
> +	kfree(msg);
> +
> +	return ret;
> +}
> +
>  /* Module initialization */
>  
>  static DEVICE_ATTR_RW(interval_msec);
> @@ -397,12 +461,15 @@ static DEVICE_ATTR_RO(version);
>  static DEVICE_ATTR_WO(brightness);
>  static DEVICE_ATTR_WO(led_rgb);
>  static DEVICE_ATTR_RW(sequence);
> +static DEVICE_ATTR_WO(program);
> +
>  static struct attribute *__lb_cmds_attrs[] = {
>  	&dev_attr_interval_msec.attr,
>  	&dev_attr_version.attr,
>  	&dev_attr_brightness.attr,
>  	&dev_attr_led_rgb.attr,
>  	&dev_attr_sequence.attr,
> +	&dev_attr_program.attr,
>  	NULL,
>  };
>  
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index 1b19e424..dbea580 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -1162,6 +1162,13 @@ struct lightbar_params_v1 {
>  	struct rgb_s color[8];			/* 0-3 are Google colors */
>  } __packed;
>  
> +/* Lightbar program */
> +#define EC_LB_PROG_LEN 192
> +struct lightbar_program {
> +	uint8_t size;
> +	uint8_t data[EC_LB_PROG_LEN];
> +};
> +
>  struct ec_params_lightbar {
>  	uint8_t cmd;		      /* Command (see enum lightbar_command) */
>  	union {
> @@ -1188,6 +1195,7 @@ struct ec_params_lightbar {
>  
>  		struct lightbar_params_v0 set_params_v0;
>  		struct lightbar_params_v1 set_params_v1;
> +		struct lightbar_program set_program;
>  	};
>  } __packed;
>  
> @@ -1220,7 +1228,8 @@ struct ec_response_lightbar {
>  		struct {
>  			/* no return params */
>  		} off, on, init, set_brightness, seq, reg, set_rgb,
> -			demo, set_params_v0, set_params_v1;
> +			demo, set_params_v0, set_params_v1,
> +			set_program;
>  	};
>  } __packed;
>  
> @@ -1244,6 +1253,7 @@ enum lightbar_command {
>  	LIGHTBAR_CMD_GET_DEMO = 15,
>  	LIGHTBAR_CMD_GET_PARAMS_V1 = 16,
>  	LIGHTBAR_CMD_SET_PARAMS_V1 = 17,
> +	LIGHTBAR_CMD_SET_PROGRAM = 18,
>  	LIGHTBAR_NUM_CMDS
>  };
>  
> -- 
> 2.9.3
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@...gle.com
Chromium OS Project
bleung@...omium.org

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists