platform/chrome: cros_ec_proto: Also return invalid command in cros_ec_cmd_xfer_status
diff mbox series

Message ID 20190403135123.8723-1-enric.balletbo@collabora.com
State In Next
Commit ae8378b081fa970a65001de909d8d6d8deea79b7
Headers show
Series
  • platform/chrome: cros_ec_proto: Also return invalid command in cros_ec_cmd_xfer_status
Related show

Commit Message

Enric Balletbo i Serra April 3, 2019, 1:51 p.m. UTC
The commit 9798ac6d32c1 ("mfd: cros_ec: Add cros_ec_cmd_xfer_status()
helper") was introduced to remove duplicated code in several users of
the cros_ec_cmd_xfer function. The idea behind that commit is mask all
the EC errors as protocol error when the EC command fails, what is
really useful because as a user you probably don't care of the exact
error reported by the EC, you only want to know if the command succeeded
or not.

That works for most of the errors returned by the EC, but we might be
also interested on report the EC_RES_INVALID_COMMAND, which happens when
the command is not supported by the EC. An use case for this is when you
need to show/hide a sysfs attribute for a specific command and you don't
have a EC feature flag that tells you that the command is supported or
not.

While here, also document properly the cros_ec_cmd_xfer_status
function.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/platform/chrome/cros_ec_proto.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

Guenter Roeck April 3, 2019, 4:25 p.m. UTC | #1
On Wed, Apr 3, 2019 at 6:51 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> The commit 9798ac6d32c1 ("mfd: cros_ec: Add cros_ec_cmd_xfer_status()
> helper") was introduced to remove duplicated code in several users of
> the cros_ec_cmd_xfer function. The idea behind that commit is mask all
> the EC errors as protocol error when the EC command fails, what is
> really useful because as a user you probably don't care of the exact
> error reported by the EC, you only want to know if the command succeeded
> or not.
>
> That works for most of the errors returned by the EC, but we might be
> also interested on report the EC_RES_INVALID_COMMAND, which happens when
> the command is not supported by the EC. An use case for this is when you
> need to show/hide a sysfs attribute for a specific command and you don't
> have a EC feature flag that tells you that the command is supported or
> not.
>
> While here, also document properly the cros_ec_cmd_xfer_status
> function.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

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

> ---
>
>  drivers/platform/chrome/cros_ec_proto.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 97a068dff192..a4e0d3a6d891 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -478,6 +478,16 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>  }
>  EXPORT_SYMBOL(cros_ec_cmd_xfer);
>
> +/**
> + * cros_ec_cmd_xfer_status() - Send EC command.
> + * @ec_dev: EC device.
> + * @msg: EC message data for request and response.
> + *
> + * Return:
> + * 0 - OK
> + * -EINVAL - Invalid command.
> + * -EPROTO - Protocol error.
> + */
>  int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
>                             struct cros_ec_command *msg)
>  {
> @@ -486,6 +496,11 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
>         ret = cros_ec_cmd_xfer(ec_dev, msg);
>         if (ret < 0) {
>                 dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
> +       } else if (msg->result == EC_RES_INVALID_COMMAND) {
> +               dev_dbg(ec_dev->dev,
> +                       "Command %d not supported for this device\n",
> +                       msg->command);
> +               return -EINVAL;
>         } else if (msg->result != EC_RES_SUCCESS) {
>                 dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
>                 return -EPROTO;
> --
> 2.20.1
>
Enric Balletbo Serra April 11, 2019, 3:40 p.m. UTC | #2
Hi,

Missatge de Guenter Roeck <groeck@google.com> del dia dc., 3 d’abr.
2019 a les 18:27:
>
> On Wed, Apr 3, 2019 at 6:51 AM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
> >
> > The commit 9798ac6d32c1 ("mfd: cros_ec: Add cros_ec_cmd_xfer_status()
> > helper") was introduced to remove duplicated code in several users of
> > the cros_ec_cmd_xfer function. The idea behind that commit is mask all
> > the EC errors as protocol error when the EC command fails, what is
> > really useful because as a user you probably don't care of the exact
> > error reported by the EC, you only want to know if the command succeeded
> > or not.
> >
> > That works for most of the errors returned by the EC, but we might be
> > also interested on report the EC_RES_INVALID_COMMAND, which happens when
> > the command is not supported by the EC. An use case for this is when you
> > need to show/hide a sysfs attribute for a specific command and you don't
> > have a EC feature flag that tells you that the command is supported or
> > not.
> >

Actually, I discarded this patch, I assumed wrongly that this patch
will avoid some boilerplate on the above cases but that's not true
because there is no difference between check that ret == -EINVAL or
check that msg->result == EC_RES_INVALID_COMMAND.

> > While here, also document properly the cros_ec_cmd_xfer_status
> > function.
> >

Also, this function was properly documented in the header, so that is
not needed, my bad.

Thanks,
 Enric

> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>
> Reviewed-by: Guenter Roeck <groeck@chromium.org>
>
> > ---
> >
> >  drivers/platform/chrome/cros_ec_proto.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index 97a068dff192..a4e0d3a6d891 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -478,6 +478,16 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> >  }
> >  EXPORT_SYMBOL(cros_ec_cmd_xfer);
> >
> > +/**
> > + * cros_ec_cmd_xfer_status() - Send EC command.
> > + * @ec_dev: EC device.
> > + * @msg: EC message data for request and response.
> > + *
> > + * Return:
> > + * 0 - OK
> > + * -EINVAL - Invalid command.
> > + * -EPROTO - Protocol error.
> > + */
> >  int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
> >                             struct cros_ec_command *msg)
> >  {
> > @@ -486,6 +496,11 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
> >         ret = cros_ec_cmd_xfer(ec_dev, msg);
> >         if (ret < 0) {
> >                 dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
> > +       } else if (msg->result == EC_RES_INVALID_COMMAND) {
> > +               dev_dbg(ec_dev->dev,
> > +                       "Command %d not supported for this device\n",
> > +                       msg->command);
> > +               return -EINVAL;
> >         } else if (msg->result != EC_RES_SUCCESS) {
> >                 dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
> >                 return -EPROTO;
> > --
> > 2.20.1
> >

Patch
diff mbox series

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 97a068dff192..a4e0d3a6d891 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -478,6 +478,16 @@  int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer);
 
+/**
+ * cros_ec_cmd_xfer_status() - Send EC command.
+ * @ec_dev: EC device.
+ * @msg: EC message data for request and response.
+ *
+ * Return:
+ * 0 - OK
+ * -EINVAL - Invalid command.
+ * -EPROTO - Protocol error.
+ */
 int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
 			    struct cros_ec_command *msg)
 {
@@ -486,6 +496,11 @@  int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
 	ret = cros_ec_cmd_xfer(ec_dev, msg);
 	if (ret < 0) {
 		dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
+	} else if (msg->result == EC_RES_INVALID_COMMAND) {
+		dev_dbg(ec_dev->dev,
+			"Command %d not supported for this device\n",
+			msg->command);
+		return -EINVAL;
 	} else if (msg->result != EC_RES_SUCCESS) {
 		dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
 		return -EPROTO;