On Fri, Jun 17, 2016 at 09:12:20PM -0700, Guenter Roeck wrote:

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

quoted

On Fri, Jun 17, 2016 at 02:41:51PM -0700, Guenter Roeck wrote:

quoted

On Fri, Jun 17, 2016 at 12:58:12PM -0700, Brian Norris wrote:

quoted

+int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
+			    struct cros_ec_command *msg)
+{
+	int ret;
+
+	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_SUCCESS)
+		return -EECRESULT - msg->result;

I have been wondering about the error return codes here, and if they should be
converted to standard Linux error codes. For example, I just hit error -1003
with a driver I am working on. This translates to EC_RES_INVALID_PARAM, or,
in Linux terms, -EINVAL. I think it would be better to use standard error
codes, especially since some of the errors are logged.

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

        EC_RES_INVALID_COMMAND
        EC_RES_INVALID_PARAM
        EC_RES_INVALID_VERSION
        EC_RES_INVALID_HEADER

Personal preference, but yes.

quoted

We lose a lot of information that way. And particularly, cros_ec_num_pwms()
won't be able to count as accurately. But I can just go back to not using this

You lost me there, sorry.

If you look at the currently-proposed user of this API (patch 4), some
of the code in cros_ec_num_pwms() looks like this:

               /*
                * We look for SUCCESS, INVALID_COMMAND, or INVALID_PARAM responses;
                * everything else is treated as an error
                */
               if (ret == -EECRESULT - EC_RES_INVALID_COMMAND)
                       return -ENODEV;
               else if (ret == -EECRESULT - EC_RES_INVALID_PARAM)
                       return i;
               else if (ret < 0)
                       return ret;

I'd really like to know the difference between EC_RES_INVALID_COMMAND,
EC_RES_INVALID_PARAM, and all other error codes. You're suggesting
aliasing those with generic Linux error codes which could easily be used
for other things (e.g., reporting errors from the I2C or SPI layers,
which I'd want to treat differently). So either I decrease the
preciseness of the above code, or I just use cros_ec_cmd_xfer() instead
of cros_ec_cmd_xfer_status(), and then have to reimplement the error
handling that cros_ec_cmd_xfer_status() was going to (sort of) abstract
away.

But maybe that just means that pwm-cros-ec.c is really the odd-man-out
use case compared to the other users of cros_ec_cmd_xfer_status() in the
Chrome OS kernel tree, and so I should be using cros_ec_cmd_xfer().

quoted

API if that's what you'd like...

That isn't what I suggested.

Brian
--
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