Hello Brian,

On 06/20/2016 02:10 PM, Brian Norris wrote:

Hi,

On Mon, Jun 20, 2016 at 10:44:55AM -0700, Brian Norris wrote:

quoted

On Mon, Jun 20, 2016 at 09:46:57AM -0400, Javier Martinez Canillas wrote:

quoted

On 06/18/2016 01:09 PM, Guenter Roeck wrote:

quoted

On 06/17/2016 06:08 PM, Brian Norris wrote:

quoted

How do you propose we do that? Do all of the following become EINVAL?

Yes, I would just do that.

The idea of this helper is to remove duplicated code and AFAICT what most EC
drivers do is something similar to the following:

        ret = cros_ec_cmd_xfer(ec, msg);
        if (ret < 0)
	   	return ret;

        if (msg->result != EC_RES_SUCCESS) {
                dev_dbg(ec->dev, "EC result %d\n", msg->result);
                return -EINVAL;
        }

So in practice what most drivers really care is if the result was successful
or not, I don't see specific EC error handling in the EC drivers.  The real
EC error code is still in the message anyways so drivers that do cares about
the real EC error can look at msg->result instead.

[...]

quoted

quoted

quoted

quoted

         EC_RES_INVALID_COMMAND

    -EOPNOTSUPP

quoted

         EC_RES_INVALID_PARAM

    -EINVAL or -EBADMSG

quoted

         EC_RES_INVALID_VERSION

    -EPROTO or -EBADR or -EBADE or -EBADRQC or -EPROTOOPT

quoted

         EC_RES_INVALID_HEADER

    -EPROTO or -EBADR or -EBADE

Doesn't look that bad to me. Also, the raw error could still be logged,
for example with dev_dbg().

Yes, I think that adding a dev_dbg() with the real EC error code should
be enough, that's basically what drivers do since they can't propagate
the EC error to higher layers anyways.

I'll take a look at adding an error code translation table when I get a
chance. Hopefully that doesn't delay the others who are planning to use
this API shortly...

Actually, I had some second thoughts, and others brought similar
concerns up to me:

What do we really win by doing the translation? It'll be difficult to do
a 1:1 translation, and any time the EC adds additional error types,
we'll have to update the translation. What's more, AFAICT, no one will
really be looking at the translated error codes now, and will just be
blindly passing them on. So maybe it makes more sense to just pick a
single error code and pass that on instead. Possibly just -EPROTO (or
nominate your favorite) for all EC error results, and if someone cares,

I think I didn't make clear in my previous email but yes, I also think
that a translate table for errors is not a good idea and that just a
single -EINVAL (or another error if is more suitable for all EC errors)
will be enough. I say -EINVAL because that's what you first mentioned
in a previous email and that's what most EC clients drivers are using.

I do believe that EC specific errors shouldn't be returned in a Linux
function though for the reasons I explained before.

they decode msg->result in their driver, or check the dev_dbg() log?

And yes, I also think that a dev_dbg() is a good idea (the more data
we have about an error, the better) and as mentioned the EC error can
always be found in the EC message result field.
 

Brian

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html