Re: [PATCH] platform/chrome: wilco_ec: Add debugfs test_event file

[Nick Crews]

Thanks for the patch Daniel! A few thoughts that I didn't
have on the review on Gerrit, sorry :) After those changes,

Reviewed-by: Nick Crews <ncrews@xxxxxxxxxxxx>

On Fri, Sep 6, 2019 at 4:42 PM Daniel Campello <campello@xxxxxxxxxxxx> wrote:
>
> This change introduces a new debugfs file 'test_event' that when written
> to causes the EC to generate a test event.
>
> Signed-off-by: Daniel Campello <campello@xxxxxxxxxxxx>
> ---
>
>  drivers/platform/chrome/wilco_ec/debugfs.c | 33 +++++++++++++++++-----
>  1 file changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/platform/chrome/wilco_ec/debugfs.c b/drivers/platform/chrome/wilco_ec/debugfs.c
> index 8d65a1e2f1a3..2103c3ed8385 100644
> --- a/drivers/platform/chrome/wilco_ec/debugfs.c
> +++ b/drivers/platform/chrome/wilco_ec/debugfs.c
> @@ -160,29 +160,29 @@ static const struct file_operations fops_raw = {
>
>  #define CMD_KB_CHROME          0x88
>  #define SUB_CMD_H1_GPIO                0x0A
> +#define SUB_CMD_TEST_EVENT     0x0B
>
> -struct h1_gpio_status_request {
> +struct ec_request {
>         u8 cmd;         /* Always CMD_KB_CHROME */
>         u8 reserved;
>         u8 sub_cmd;     /* Always SUB_CMD_H1_GPIO */

This comment should be removed.

>  } __packed;
>
> -struct hi_gpio_status_response {
> +struct ec_response {
>         u8 status;      /* 0 if allowed */
>         u8 val;         /* BIT(0)=ENTRY_TO_FACT_MODE, BIT(1)=SPI_CHROME_SEL */

This comment should be moved to h1_gpio_get().

>  } __packed;
>
> -static int h1_gpio_get(void *arg, u64 *val)
> +static int write_to_mailbox(struct wilco_ec_device *ec, u8 sub_cmd, u64 *val)

What about send_ec_cmd() or similar? Something that communicates that
we are sometimes telling the EC to do something, and sometimes reading something
back. Also, since we are adding in another layer in here, we can fix
the signature from
the one required by a debugfs attribute. Use "ret" instead of "val"
and make it a u8*.

>  {
> -       struct wilco_ec_device *ec = arg;
> -       struct h1_gpio_status_request rq;
> -       struct hi_gpio_status_response rs;
> +       struct ec_request rq;
> +       struct ec_response rs;
>         struct wilco_ec_message msg;
>         int ret;
>
>         memset(&rq, 0, sizeof(rq));
>         rq.cmd = CMD_KB_CHROME;
> -       rq.sub_cmd = SUB_CMD_H1_GPIO;
> +       rq.sub_cmd = sub_cmd;
>
>         memset(&msg, 0, sizeof(msg));
>         msg.type = WILCO_EC_MSG_LEGACY;
> @@ -201,8 +201,25 @@ static int h1_gpio_get(void *arg, u64 *val)
>         return 0;
>  }
>
> +static int h1_gpio_get(void *arg, u64 *val)
> +{
> +       return write_to_mailbox(arg, SUB_CMD_H1_GPIO, val);
> +}
> +
>  DEFINE_DEBUGFS_ATTRIBUTE(fops_h1_gpio, h1_gpio_get, NULL, "0x%02llx\n");
>

A one line comment as to what test_event does?

> +static int test_event_set(void *arg, u64 val)
> +{
> +       u64 ret;
> +
> +       return write_to_mailbox(arg, SUB_CMD_TEST_EVENT, &ret);
> +}
> +
> +/* Format set to NULL since it is only used on read operations which are
> + * forbidden by file permissions.
> + */
> +DEFINE_DEBUGFS_ATTRIBUTE(fops_test_event, NULL, test_event_set, NULL);
> +
>  /**
>   * wilco_ec_debugfs_probe() - Create the debugfs node
>   * @pdev: The platform device, probably created in core.c
> @@ -226,6 +243,8 @@ static int wilco_ec_debugfs_probe(struct platform_device *pdev)
>         debugfs_create_file("raw", 0644, debug_info->dir, NULL, &fops_raw);
>         debugfs_create_file("h1_gpio", 0444, debug_info->dir, ec,
>                             &fops_h1_gpio);
> +       debugfs_create_file("test_event", 0200, debug_info->dir, ec,
> +                           &fops_test_event);
>
>         return 0;
>  }
> --
> 2.23.0.162.g0b9fbb3734-goog
>