Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220518091814.2028579-1-tzungbi@kernel.org> <20220518091814.2028579-4-tzungbi@kernel.org>
In-Reply-To: <20220518091814.2028579-4-tzungbi@kernel.org>
From:   Guenter Roeck <groeck@google.com>
Date:   Wed, 18 May 2022 09:28:25 -0700
Message-ID: <CABXOdTfHKgj+c4oA-EaVqRjaNmFn4i_GDEpEqdexvV0L=dNmXg@mail.gmail.com>
Subject: Re: [PATCH 3/4] platform/chrome: cros_ec_proto: update
 cros_ec_check_result() comment
To:     Tzung-Bi Shih <tzungbi@kernel.org>
Cc:     Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        chrome-platform@lists.linux.dev,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Doug Anderson <dianders@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, May 18, 2022 at 2:18 AM Tzung-Bi Shih <tzungbi@kernel.org> wrote:
>
> At first glance, cros_ec_check_result() is quite like cros_ec_map_error().
> They check for `ec_msg->result` and return corresponding errors.  However,
> as calling from `pkt_xfer` and `cmd_xfer`, cros_ec_check_result() should
> not report furthermore errors.  -EAGAIN is the only exception.
>
> See [1][2][3] for some known userland programs' code.  The return code
> from ioctl only denotes the EC communication status.  Userland programs
> would further analyze the `result` in struct cros_ec_command* for
> follow-up actions (e.g. [4]).
>
> To clarify, update the function comment.
>
> [1]: https://crrev.com/54400e93a75ef440a83d6eaac2cec066daf99cf0/util/comm-dev.c#154
> [2]: https://crrev.com/fe32670a89bf59e1aff84bba9dd3295657b85e9b/cros_ec_dev.c#296
> [3]: https://crrev.com/4e19eb1d89de0422ff1bbd3f7260b131c761098c/drivers/google/cros_ec_dev.c#120
> [4]: https://crrev.com/54400e93a75ef440a83d6eaac2cec066daf99cf0/util/comm-dev.c#164
>
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> ---
> Changes from previous version:
> (https://patchwork.kernel.org/project/chrome-platform/patch/20220517082817.1591526-1-tzungbi@kernel.org/)
> - Update the link of [3].

The patch should be marked as v2 if there is a previous version. Other
than that,

Reviewed-by: Guenter Roeck <groeck@chromium.org>

>
>  drivers/platform/chrome/cros_ec_proto.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 01ab58b3269b..13ced9d2dd71 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -204,9 +204,12 @@ EXPORT_SYMBOL(cros_ec_prepare_tx);
>   * @msg: Message to check.
>   *
>   * This is used by ChromeOS EC drivers to check the ec_msg->result for
> - * errors and to warn about them.
> + * EC_RES_IN_PROGRESS and to warn about them.
>   *
> - * Return: 0 on success or negative error code.
> + * The function should not check for furthermore error codes.  Otherwise,
> + * it would break the ABI.
> + *
> + * Return: -EAGAIN if ec_msg->result == EC_RES_IN_PROGRESS.  Otherwise, 0.
>   */
>  int cros_ec_check_result(struct cros_ec_device *ec_dev,
>                          struct cros_ec_command *msg)
> --
> 2.36.0.550.gb090851708-goog
>