Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190405200742.107973-1-ncrews@chromium.org>
In-Reply-To: <20190405200742.107973-1-ncrews@chromium.org>
From:   Enric Balletbo Serra <eballetbo@gmail.com>
Date:   Fri, 5 Apr 2019 23:21:31 +0200
Message-ID: <CAFqH_52zYVjg1aZ4JujZxh591Opbkfw=7FS8Bmwmn5Wao3N-xw@mail.gmail.com>
Subject: Re: [PATCH 1/3] platform/chrome: wilco_ec: Standardize mailbox interface
To:     Nick Crews <ncrews@chromium.org>
Cc:     Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        Benson Leung <bleung@chromium.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        dlaurie@chromium.org, Daniel Erat <derat@google.com>,
        Guenter Roeck <groeck@google.com>,
        Dmitry Torokhov <dtor@google.com>, lamzin@google.com,
        sjg@gchromium.org,
        Alexandre Belloni <alexandre.belloni@bootlin.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Nick,

Missatge de Nick Crews <ncrews@chromium.org> del dia dv., 5 d=E2=80=99abr.
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]