Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Brian Norris]

On Fri, Jun 17, 2016 at 09:12:20PM -0700, Guenter Roeck wrote:
> On 06/17/2016 06:08 PM, Brian Norris wrote:
> >On Fri, Jun 17, 2016 at 02:41:51PM -0700, Guenter Roeck wrote:
> >>On Fri, Jun 17, 2016 at 12:58:12PM -0700, Brian Norris wrote:
> >>>+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.
> 
> >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().

> >API if that's what you'd like...
> >
> 
> That isn't what I suggested.

Brian