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

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

	-EOPNOTSUPP

         EC_RES_INVALID_PARAM

	-EINVAL or -EBADMSG

         EC_RES_INVALID_VERSION

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

         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().

Guenter

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
API if that's what you'd like...

Brian