Messages in this thread | | | From | Enric Balletbo Serra <> | Date | Fri, 5 Apr 2019 23:21:31 +0200 | Subject | Re: [PATCH 1/3] platform/chrome: wilco_ec: Standardize mailbox interface |
| |
Hi Nick,
Missatge de Nick Crews <ncrews@chromium.org> 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 <ncrews@chromium.org> > Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com> > --- > 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?
[snip]
| |