diff mbox

[09/10] mfd: cros_ec: Check result code from EC messages

Message ID 1402954800-28215-10-git-send-email-dianders@chromium.org (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Doug Anderson June 16, 2014, 9:39 p.m. UTC 
From: Bill Richardson <wfrichar@chromium.org>

Just because the host was able to talk to the EC doesn't mean that the EC
was happy with what it was told. Errors in communincation are not the same
as error messages from the EC itself.

This change lets the EC report its errors separately.

Signed-off-by: Bill Richardson <wfrichar@chromium.org>
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/mfd/cros_ec_i2c.c | 15 +++++++++++----
 drivers/mfd/cros_ec_spi.c | 26 ++++++++++++++------------
 2 files changed, 25 insertions(+), 16 deletions(-)

Comments

Simon Glass June 18, 2014, 3:43 a.m. UTC | #1 
Hi Doug,

On 16 June 2014 14:39, Doug Anderson <dianders@chromium.org> wrote:
> From: Bill Richardson <wfrichar@chromium.org>
>
> Just because the host was able to talk to the EC doesn't mean that the EC
> was happy with what it was told. Errors in communincation are not the same
> as error messages from the EC itself.
>
> This change lets the EC report its errors separately.
>
> Signed-off-by: Bill Richardson <wfrichar@chromium.org>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/mfd/cros_ec_i2c.c | 15 +++++++++++----
>  drivers/mfd/cros_ec_spi.c | 26 ++++++++++++++------------
>  2 files changed, 25 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
> index 5bb32f5..2276096 100644
> --- a/drivers/mfd/cros_ec_i2c.c
> +++ b/drivers/mfd/cros_ec_i2c.c
> @@ -92,11 +92,18 @@ static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
>         }
>
>         /* check response error code */
> -       if (i2c_msg[1].buf[0]) {
> -               dev_warn(ec_dev->dev, "command 0x%02x returned an error %d\n",
> -                        msg->command, i2c_msg[1].buf[0]);
> -               ret = -EINVAL;
> +       msg->result = i2c_msg[1].buf[0];
> +       switch (msg->result) {
> +       case EC_RES_SUCCESS:
> +               break;
> +       case EC_RES_IN_PROGRESS:
> +               ret = -EAGAIN;
> +               dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
> +                       msg->command);
>                 goto done;
> +       default:
> +               dev_warn(ec_dev->dev, "command 0x%02x returned %d\n",
> +                        msg->command, msg->result);
>         }
>
>         /* copy response packet payload and compute checksum */
> diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
> index 09ca789..4d34f1c 100644
> --- a/drivers/mfd/cros_ec_spi.c
> +++ b/drivers/mfd/cros_ec_spi.c
> @@ -289,21 +289,23 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
>                 goto exit;
>         }
>
> -       /* check response error code */
>         ptr = ec_dev->din;
> -       if (ptr[0]) {
> -               if (ptr[0] == EC_RES_IN_PROGRESS) {
> -                       dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
> -                               ec_msg->command);
> -                       ret = -EAGAIN;
> -                       goto exit;
> -               }
> -               dev_warn(ec_dev->dev, "command 0x%02x returned an error %d\n",
> -                        ec_msg->command, ptr[0]);
> -               debug_packet(ec_dev->dev, "in_err", ptr, len);
> -               ret = -EINVAL;
> +
> +       /* check response error code */
> +       ec_msg->result = ptr[0];
> +       switch (ec_msg->result) {
> +       case EC_RES_SUCCESS:
> +               break;
> +       case EC_RES_IN_PROGRESS:
> +               ret = -EAGAIN;
> +               dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
> +                       ec_msg->command);
>                 goto exit;
> +       default:
> +               dev_warn(ec_dev->dev, "command 0x%02x returned %d\n",
> +                        ec_msg->command, ec_msg->result);
>         }

Since this code is the same as the above, should it go in a common
function in cros_ec?

> +
>         len = ptr[1];
>         sum = ptr[0] + ptr[1];
>         if (len > ec_msg->insize) {
> --
> 2.0.0.526.g5318336
>

Regards,
Simon
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Doug Anderson June 18, 2014, 4:44 a.m. UTC | #2 
Simon,

On Tue, Jun 17, 2014 at 8:43 PM, Simon Glass <sjg@chromium.org> wrote:
>> diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
>> index 09ca789..4d34f1c 100644
>> --- a/drivers/mfd/cros_ec_spi.c
>> +++ b/drivers/mfd/cros_ec_spi.c
>> @@ -289,21 +289,23 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
>>                 goto exit;
>>         }
>>
>> -       /* check response error code */
>>         ptr = ec_dev->din;
>> -       if (ptr[0]) {
>> -               if (ptr[0] == EC_RES_IN_PROGRESS) {
>> -                       dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
>> -                               ec_msg->command);
>> -                       ret = -EAGAIN;
>> -                       goto exit;
>> -               }
>> -               dev_warn(ec_dev->dev, "command 0x%02x returned an error %d\n",
>> -                        ec_msg->command, ptr[0]);
>> -               debug_packet(ec_dev->dev, "in_err", ptr, len);
>> -               ret = -EINVAL;
>> +
>> +       /* check response error code */
>> +       ec_msg->result = ptr[0];
>> +       switch (ec_msg->result) {
>> +       case EC_RES_SUCCESS:
>> +               break;
>> +       case EC_RES_IN_PROGRESS:
>> +               ret = -EAGAIN;
>> +               dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
>> +                       ec_msg->command);
>>                 goto exit;
>> +       default:
>> +               dev_warn(ec_dev->dev, "command 0x%02x returned %d\n",
>> +                        ec_msg->command, ec_msg->result);
>>         }
>
> Since this code is the same as the above, should it go in a common
> function in cros_ec?

So you are thinking it should be written like:

ec_msg->result = ptr[0];
ret = cros_ec_check_result(ec_dev, ec_msg);
if (ret)
  goto exit;

---

int cros_ec_check_result(struct cros_ec_device *ec_dev, struct
cros_ec_command *ec_msg)
{
       switch (ec_msg->result) {
       case EC_RES_SUCCESS:
               return 0;
       case EC_RES_IN_PROGRESS:
               dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
                       ec_msg->command);
               return -EAGAIN;
       default:
               dev_warn(ec_dev->dev, "command 0x%02x returned %d\n",
                        ec_msg->command, ec_msg->result);
               return 0;
}

OK, that seems reasonable.  I'll plan to spin tomorrow with that.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones June 18, 2014, 7:35 a.m. UTC | #3 
On Tue, 17 Jun 2014, Doug Anderson wrote:

> Simon,
> 
> On Tue, Jun 17, 2014 at 8:43 PM, Simon Glass <sjg@chromium.org> wrote:
> >> diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
> >> index 09ca789..4d34f1c 100644
> >> --- a/drivers/mfd/cros_ec_spi.c
> >> +++ b/drivers/mfd/cros_ec_spi.c
> >> @@ -289,21 +289,23 @@ static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
> >>                 goto exit;
> >>         }
> >>
> >> -       /* check response error code */
> >>         ptr = ec_dev->din;
> >> -       if (ptr[0]) {
> >> -               if (ptr[0] == EC_RES_IN_PROGRESS) {
> >> -                       dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
> >> -                               ec_msg->command);
> >> -                       ret = -EAGAIN;
> >> -                       goto exit;
> >> -               }
> >> -               dev_warn(ec_dev->dev, "command 0x%02x returned an error %d\n",
> >> -                        ec_msg->command, ptr[0]);
> >> -               debug_packet(ec_dev->dev, "in_err", ptr, len);
> >> -               ret = -EINVAL;
> >> +
> >> +       /* check response error code */
> >> +       ec_msg->result = ptr[0];
> >> +       switch (ec_msg->result) {
> >> +       case EC_RES_SUCCESS:
> >> +               break;
> >> +       case EC_RES_IN_PROGRESS:
> >> +               ret = -EAGAIN;
> >> +               dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
> >> +                       ec_msg->command);
> >>                 goto exit;
> >> +       default:
> >> +               dev_warn(ec_dev->dev, "command 0x%02x returned %d\n",
> >> +                        ec_msg->command, ec_msg->result);
> >>         }
> >
> > Since this code is the same as the above, should it go in a common
> > function in cros_ec?
> 
> So you are thinking it should be written like:
> 
> ec_msg->result = ptr[0];
> ret = cros_ec_check_result(ec_dev, ec_msg);
> if (ret)
>   goto exit;
> 
> ---
> 
> int cros_ec_check_result(struct cros_ec_device *ec_dev, struct
> cros_ec_command *ec_msg)
> {
>        switch (ec_msg->result) {
>        case EC_RES_SUCCESS:
>                return 0;
>        case EC_RES_IN_PROGRESS:
>                dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
>                        ec_msg->command);
>                return -EAGAIN;
>        default:
>                dev_warn(ec_dev->dev, "command 0x%02x returned %d\n",
>                         ec_msg->command, ec_msg->result);
>                return 0;
> }
> 
> OK, that seems reasonable.  I'll plan to spin tomorrow with that.

+1

I'll do a proper review when you re-spin the patch.
diff mbox

Patch

diff --git a/drivers/mfd/cros_ec_i2c.c b/drivers/mfd/cros_ec_i2c.c
index 5bb32f5..2276096 100644
--- a/drivers/mfd/cros_ec_i2c.c
+++ b/drivers/mfd/cros_ec_i2c.c
@@ -92,11 +92,18 @@  static int cros_ec_cmd_xfer_i2c(struct cros_ec_device *ec_dev,
 	}
 
 	/* check response error code */
-	if (i2c_msg[1].buf[0]) {
-		dev_warn(ec_dev->dev, "command 0x%02x returned an error %d\n",
-			 msg->command, i2c_msg[1].buf[0]);
-		ret = -EINVAL;
+	msg->result = i2c_msg[1].buf[0];
+	switch (msg->result) {
+	case EC_RES_SUCCESS:
+		break;
+	case EC_RES_IN_PROGRESS:
+		ret = -EAGAIN;
+		dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
+			msg->command);
 		goto done;
+	default:
+		dev_warn(ec_dev->dev, "command 0x%02x returned %d\n",
+			 msg->command, msg->result);
 	}
 
 	/* copy response packet payload and compute checksum */
diff --git a/drivers/mfd/cros_ec_spi.c b/drivers/mfd/cros_ec_spi.c
index 09ca789..4d34f1c 100644
--- a/drivers/mfd/cros_ec_spi.c
+++ b/drivers/mfd/cros_ec_spi.c
@@ -289,21 +289,23 @@  static int cros_ec_cmd_xfer_spi(struct cros_ec_device *ec_dev,
 		goto exit;
 	}
 
-	/* check response error code */
 	ptr = ec_dev->din;
-	if (ptr[0]) {
-		if (ptr[0] == EC_RES_IN_PROGRESS) {
-			dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
-				ec_msg->command);
-			ret = -EAGAIN;
-			goto exit;
-		}
-		dev_warn(ec_dev->dev, "command 0x%02x returned an error %d\n",
-			 ec_msg->command, ptr[0]);
-		debug_packet(ec_dev->dev, "in_err", ptr, len);
-		ret = -EINVAL;
+
+	/* check response error code */
+	ec_msg->result = ptr[0];
+	switch (ec_msg->result) {
+	case EC_RES_SUCCESS:
+		break;
+	case EC_RES_IN_PROGRESS:
+		ret = -EAGAIN;
+		dev_dbg(ec_dev->dev, "command 0x%02x in progress\n",
+			ec_msg->command);
 		goto exit;
+	default:
+		dev_warn(ec_dev->dev, "command 0x%02x returned %d\n",
+			 ec_msg->command, ec_msg->result);
 	}
+
 	len = ptr[1];
 	sum = ptr[0] + ptr[1];
 	if (len > ec_msg->insize) {