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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Enric,

On 18/05/2018 18:19, Enric Balletbo Serra wrote:
> Hi Neil,
> 
> 2018-05-18 15:05 GMT+02:00 Neil Armstrong <narmstrong@xxxxxxxxxxxx>:
>> 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 | 40 +++++++++++++----
>>  include/linux/mfd/cros_ec.h             |  2 +-
>>  include/linux/mfd/cros_ec_commands.h    | 80 +++++++++++++++++++++++++++++++++
>>  3 files changed, 112 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>> index e7bbdf9..c4f6c44 100644
>> --- a/drivers/platform/chrome/cros_ec_proto.c
>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>> @@ -504,10 +504,31 @@ 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, ec_dev->event_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;
> 
> Personal opinion, but I don't like this static here, and also I don't
> think this is scalable. Could we ask for the command version?

I don't have an opinion, I only followed how it was implemented on the
chromeos kernel and adapted to mainline. If you have a better way, I'll use it !

> 
>>         int ret;
>>
>>         if (ec_dev->suspended) {
>> @@ -515,18 +536,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
>>                 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(struct ec_response_get_next_event_v1));
>> +               if (ret < 0 || msg->result != 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;
>>         }
>>
> 
> So we always do a failed transfer on all these EC devices that does
> not support CEC. I am wondering if wouldn't be better pass the command
> version to the cros_ec_get_next_event function. The driver should know
> which version to use, just a random idea.

No, the driver cannot know the command version, this depends on the FW version
and the platform. AFAIK this must be discovered.

> 
>> +       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 f36125e..32caef3 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..16c3a2b 100644
>> --- a/include/linux/mfd/cros_ec_commands.h
>> +++ b/include/linux/mfd/cros_ec_commands.h
> 
> This file is going to be very big and as requested by Lee I plan to
> convert this file to the kernel-doc format, this patch introduces some
> new structs so could you document the new structs in the suggested
> format?

Ok

> 
>> @@ -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
>> +
>> +/* 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,
> 
> Use the BIT() macro.

Ok

> 
>> +};
>>
>>  /*****************************************************************************/
>>  /*
>> --
>> 2.7.4
>>
> 
> Thanks,
>   Enric
> 




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux