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

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

[Daniel Campello]

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@chromium.org>
---

 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 */
 } __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 */
 } __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)
 {
-	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");

+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

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@chromium.org>

On Fri, Sep 6, 2019 at 4:42 PM Daniel Campello <campello@chromium.org> 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@chromium.org>
> ---
>
>  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
>

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

[Daniel Campello]

On Fri, Sep 06, 2019 at 06:20:51PM -0400, Nick Crews wrote:
> Thanks for the patch Daniel! A few thoughts that I didn't
> have on the review on Gerrit, sorry :) After those changes,
>
No problem, I will take care of them.

> Reviewed-by: Nick Crews <ncrews@chromium.org>
>
> >         u8 sub_cmd;     /* Always SUB_CMD_H1_GPIO */
>
> This comment should be removed.
>
I overlooked this. Done.

> This comment should be moved to h1_gpio_get().
>
Agree

> > -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*.
>
send_ec_cmd() sounds good to me but ret is already used. Will change
to out_val as discussed offline.

> 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);
> > +}
> > +

Will do. Thanks again.