Re: [PATCH v2 3/5] mfd: cros-ec: Introduce CEC commands and events definitions.

[Neil Armstrong]

Hi Hans,

On 15/05/2018 17:28, Hans Verkuil wrote:
> On 05/15/2018 04:42 PM, Neil Armstrong wrote:
>> The EC can expose a CEC bus, this patch adds the CEC related definitions
>> needed by the cros-ec-cec driver.
>> Having a 16 byte mkbp event size makes it possible to send CEC
>> messages from the EC to the AP directly inside the mkbp event
>> instead of first doing a notification and then a read.
>>
>> Signed-off-by: Stefan Adolfsson <sadolfsson@xxxxxxxxxxxx>
>> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
>> ---
>>  drivers/platform/chrome/cros_ec_proto.c | 42 +++++++++++++----
>>  include/linux/mfd/cros_ec.h             |  2 +-
>>  include/linux/mfd/cros_ec_commands.h    | 80 +++++++++++++++++++++++++++++++++
>>  3 files changed, 114 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>> index e7bbdf9..ba47f79 100644
>> --- a/drivers/platform/chrome/cros_ec_proto.c
>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>> @@ -504,29 +504,53 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
>>  }
>>  EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
>>  
>> +static int get_next_event_xfer(struct cros_ec_device *ec_dev,
>> +			       struct cros_ec_command *msg,
>> +			       int version, uint32_t size)
>> +{
>> +	int ret;
>> +
>> +	msg->version = version;
>> +	msg->command = EC_CMD_GET_NEXT_EVENT;
>> +	msg->insize = size;
>> +	msg->outsize = 0;
>> +
>> +	ret = cros_ec_cmd_xfer(ec_dev, msg);
>> +	if (ret > 0) {
>> +		ec_dev->event_size = ret - 1;
>> +		memcpy(&ec_dev->event_data, msg->data, size);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static int get_next_event(struct cros_ec_device *ec_dev)
>>  {
>>  	u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
>>  	struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
>> +	static int cmd_version = 1;
>>  	int ret;
>>  
>> +	BUILD_BUG_ON(sizeof(union ec_response_get_next_data_v1) != 16);
> 
> Use the define instead of hardcoding 16. I'm not really sure why you need this.
> If cec_message uses the right define for the array size (see my comment below),
> then this really can't go wrong, can it?

This is taken from the chrome kernelk, to be sure the size is ok, but yes it should be 16, I'll see
if I can drop this.

> 
>> +
>>  	if (ec_dev->suspended) {
>>  		dev_dbg(ec_dev->dev, "Device suspended.\n");
>>  		return -EHOSTDOWN;
>>  	}
>>  
>> -	msg->version = 0;
>> -	msg->command = EC_CMD_GET_NEXT_EVENT;
>> -	msg->insize = sizeof(ec_dev->event_data);
>> -	msg->outsize = 0;
>> +	if (cmd_version == 1) {
>> +		ret = get_next_event_xfer(ec_dev, msg, cmd_version,
>> +					  sizeof(ec_dev->event_data));
>> +		if (ret != EC_RES_INVALID_VERSION)
>> +			return ret;
>>  
>> -	ret = cros_ec_cmd_xfer(ec_dev, msg);
>> -	if (ret > 0) {
>> -		ec_dev->event_size = ret - 1;
>> -		memcpy(&ec_dev->event_data, msg->data,
>> -		       sizeof(ec_dev->event_data));
>> +		/* Fallback to version 0 for future send attempts */
>> +		cmd_version = 0;
>>  	}
>>  
>> +	ret = get_next_event_xfer(ec_dev, msg, cmd_version,
>> +				  sizeof(struct ec_response_get_next_event));
>> +
>>  	return ret;
>>  }
>>  
>> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
>> index 2d4e23c..f3415eb 100644
>> --- a/include/linux/mfd/cros_ec.h
>> +++ b/include/linux/mfd/cros_ec.h
>> @@ -147,7 +147,7 @@ struct cros_ec_device {
>>  	bool mkbp_event_supported;
>>  	struct blocking_notifier_head event_notifier;
>>  
>> -	struct ec_response_get_next_event event_data;
>> +	struct ec_response_get_next_event_v1 event_data;
>>  	int event_size;
>>  	u32 host_event_wake_mask;
>>  };
>> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
>> index f2edd99..18df466 100644
>> --- a/include/linux/mfd/cros_ec_commands.h
>> +++ b/include/linux/mfd/cros_ec_commands.h
>> @@ -804,6 +804,8 @@ enum ec_feature_code {
>>  	EC_FEATURE_MOTION_SENSE_FIFO = 24,
>>  	/* EC has RTC feature that can be controlled by host commands */
>>  	EC_FEATURE_RTC = 27,
>> +	/* EC supports CEC commands */
>> +	EC_FEATURE_CEC = 35,
>>  };
>>  
>>  #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
>> @@ -2078,6 +2080,12 @@ enum ec_mkbp_event {
>>  	/* EC sent a sysrq command */
>>  	EC_MKBP_EVENT_SYSRQ = 6,
>>  
>> +	/* Notify the AP that something happened on CEC */
>> +	EC_MKBP_CEC_EVENT = 8,
>> +
>> +	/* Send an incoming CEC message to the AP */
>> +	EC_MKBP_EVENT_CEC_MESSAGE = 9,
>> +
>>  	/* Number of MKBP events */
>>  	EC_MKBP_EVENT_COUNT,
>>  };
>> @@ -2093,12 +2101,31 @@ union ec_response_get_next_data {
>>  	uint32_t   sysrq;
>>  } __packed;
>>  
>> +union ec_response_get_next_data_v1 {
>> +	uint8_t   key_matrix[16];
>> +
>> +	/* Unaligned */
>> +	uint32_t  host_event;
>> +
>> +	uint32_t   buttons;
>> +	uint32_t   switches;
>> +	uint32_t   sysrq;
>> +	uint32_t   cec_events;
>> +	uint8_t    cec_message[16];
>> +} __packed;
>> +
>>  struct ec_response_get_next_event {
>>  	uint8_t event_type;
>>  	/* Followed by event data if any */
>>  	union ec_response_get_next_data data;
>>  } __packed;
>>  
>> +struct ec_response_get_next_event_v1 {
>> +	uint8_t event_type;
>> +	/* Followed by event data if any */
>> +	union ec_response_get_next_data_v1 data;
>> +} __packed;
>> +
>>  /* Bit indices for buttons and switches.*/
>>  /* Buttons */
>>  #define EC_MKBP_POWER_BUTTON	0
>> @@ -2828,6 +2855,59 @@ struct ec_params_reboot_ec {
>>  /* Current version of ACPI memory address space */
>>  #define EC_ACPI_MEM_VERSION_CURRENT 1
>>  
>> +/*****************************************************************************/
>> +/*
>> + * HDMI CEC commands
>> + *
>> + * These commands are for sending and receiving message via HDMI CEC
>> + */
>> +#define MAX_CEC_MSG_LEN 16
> 
> Hmm, uapi/linux/cec.h already defines CEC_MAX_MSG_SIZE with the same value.
> Perhaps it is better to include linux/cec.h here instead of creating a second
> define?
> 
> And shouldn't this define also be used for the cec_message array above?

These defines are tied to the Embedded Controller API, so it may be goog to keep them
separated from the rest of the Linux APO, I'll see about using the define in the event
struct, but I think they want to have a formal array size using a number.

Neil

> 
> Regards,
> 
> 	Hans
> 
>> +
>> +/* CEC message from the AP to be written on the CEC bus */
>> +#define EC_CMD_CEC_WRITE_MSG 0x00B8
>> +
>> +/* Message to write to the CEC bus */
>> +struct ec_params_cec_write {
>> +	uint8_t msg[MAX_CEC_MSG_LEN];
>> +} __packed;
>> +
>> +/* Set various CEC parameters */
>> +#define EC_CMD_CEC_SET 0x00BA
>> +
>> +struct ec_params_cec_set {
>> +	uint8_t cmd; /* enum cec_command */
>> +	union {
>> +		uint8_t enable;
>> +		uint8_t address;
>> +	};
>> +} __packed;
>> +
>> +/* Read various CEC parameters */
>> +#define EC_CMD_CEC_GET 0x00BB
>> +
>> +struct ec_params_cec_get {
>> +	uint8_t cmd; /* enum cec_command */
>> +} __packed;
>> +
>> +struct ec_response_cec_get {
>> +	union {
>> +		uint8_t enable;
>> +		uint8_t address;
>> +	};
>> +} __packed;
>> +
>> +enum cec_command {
>> +	/* CEC reading, writing and events enable */
>> +	CEC_CMD_ENABLE,
>> +	/* CEC logical address  */
>> +	CEC_CMD_LOGICAL_ADDRESS,
>> +};
>> +
>> +/* Events from CEC to AP */
>> +enum mkbp_cec_event {
>> +	EC_MKBP_CEC_SEND_OK			= 1 << 0,
>> +	EC_MKBP_CEC_SEND_FAILED			= 1 << 1,
>> +};
>>  
>>  /*****************************************************************************/
>>  /*
>>
>