[lkml]   [2019]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 1/3] platform/chrome: wilco_ec: Standardize mailbox interface
Hi Nick,

Missatge de Nick Crews <> del dia dv., 5 d’abr.
2019 a les 22:09:
> The current API for the wilco EC mailbox interface is bad.
> It assumes that most messages sent to the EC follow a similar structure,
> with a command byte in MBOX[0], followed by a junk byte, followed by
> actual data. This doesn't happen in several cases, such as setting the
> RTC time, using the raw debugfs interface, and reading or writing
> properties such as the Peak Shift policy (this last to be submitted soon).
> Similarly for the response message from the EC, the current interface
> assumes that the first byte of data is always 0, and the second byte
> is unused. However, in both setting and getting the RTC time, in the
> debugfs interface, and for reading and writing properties, this isn't
> true.
> The current way to resolve this is to use WILCO_EC_FLAG_RAW* flags to
> specify when and when not to skip these initial bytes in the sent and
> received message. They are confusing and used so much that they are
> normal, and not exceptions. In addition, the first byte of
> response in the debugfs interface is still always skipped, which is
> weird, since this raw interface should be giving the entire result.
> Additionally, sent messages assume the first byte is a command, and so
> struct wilco_ec_message contains the "command" field. In setting or
> getting properties however, the first byte is not a command, and so this
> field has to be filled with a byte that isn't actually a command. This
> is again inconsistent.
> wilco_ec_message contains a result field as well, copied from
> wilco_ec_response->result. The message result field should be removed:
> if the message fails, the cause is already logged, and the callers are
> alerted. They will never care about the actual state of the result flag.
> These flags and different cases make the wilco_ec_transfer() function,
> used in wilco_ec_mailbox(), really gross, dealing with a bunch of
> different cases. It's difficult to figure out what it is doing.
> Finally, making these assumptions about the structure of a message make
> it so that the messages do not correspond well with the specification
> for the EC's mailbox interface. For instance, this interface
> specification may say that MBOX[9] in the received message contains
> some information, but the calling code needs to remember that the first
> byte of response is always skipped, and because it didn't set the
> RESPONSE_RAW flag, the next byte is also skipped, so this information
> is actually contained within wilco_ec_message->response_data[7]. This
> makes it difficult to maintain this code in the future.
> To fix these problems this patch standardizes the mailbox interface by:
> - Removing the WILCO_EC_FLAG_RAW* flags
> - Removing the command and reserved_raw bytes from wilco_ec_request
> - Removing the mbox0 byte from wilco_ec_response
> - Simplifying wilco_ec_transfer() because of these changes
> - Gives the callers of wilco_ec_mailbox() the responsibility of exactly
> and consistently defining the structure of the mailbox request and
> response
> - Removing command and result from wilco_ec_message.
> This results in the reduction of total code, and makes it much more
> maintainable and understandable.
> Signed-off-by: Nick Crews <>
> Acked-by: Alexandre Belloni <>
> ---
> drivers/platform/chrome/wilco_ec/debugfs.c | 43 ++++++++-------
> drivers/platform/chrome/wilco_ec/mailbox.c | 53 ++++++++----------
> drivers/rtc/rtc-wilco-ec.c | 63 +++++++++++++---------
> include/linux/platform_data/wilco-ec.h | 22 +-------
> 4 files changed, 83 insertions(+), 98 deletions(-)

Now I'm confused, isn't this the same patch I picked this morning from
you and is already applied in chrome-platform for-next?


 \ /
  Last update: 2019-04-05 23:22    [W:0.053 / U:31.024 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site