Messages in this thread | | | Date | Sat, 1 Aug 2020 09:21:30 +0200 | From | Uwe Kleine-König <> | Subject | Re: [PATCH v3 4/6] pwm: cros-ec: Accept more error codes from cros_ec_cmd_xfer_status |
| |
On Sun, Jul 26, 2020 at 03:00:59PM -0700, Guenter Roeck wrote: > Since commit c5cd2b47b203 ("platform/chrome: cros_ec_proto: Report command > not supported") we can no longer assume that cros_ec_cmd_xfer_status() > reports -EPROTO for all errors returned by the EC itself. A follow-up > patch will change cros_ec_cmd_xfer_status() to report additional errors > reported by the EC as distinguished Linux error codes. > > Handle this change by no longer assuming that only -EPROTO is used > to report all errors returned by the EC itself. Instead, support both > the old and the new error codes. > > Cc: Gwendal Grignou <gwendal@chromium.org> > Cc: Yu-Hsuan Hsu <yuhsuan@chromium.org> > Cc: Prashant Malani <pmalani@chromium.org> > Cc: Brian Norris <briannorris@chromium.org> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > v3: Added patch > > drivers/pwm/pwm-cros-ec.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c > index 09c08dee099e..ef05fba1bd37 100644 > --- a/drivers/pwm/pwm-cros-ec.c > +++ b/drivers/pwm/pwm-cros-ec.c > @@ -213,20 +213,27 @@ static int cros_ec_num_pwms(struct cros_ec_device *ec) > u32 result = 0; > > ret = __cros_ec_pwm_get_duty(ec, i, &result); > - /* We want to parse EC protocol errors */ > - if (ret < 0 && !(ret == -EPROTO && result)) > - return ret; > - > /* > * We look for SUCCESS, INVALID_COMMAND, or INVALID_PARAM > * responses; everything else is treated as an error. > */
This comment is at least misleading now.
> - if (result == EC_RES_INVALID_COMMAND) > + switch (ret) { > + case -EOPNOTSUPP: /* invalid command */ > return -ENODEV;
My first reaction here was to wonder why -EOPNOTSUPP isn't passed to the upper layer. OK, this is a loop to test the number of available devices.
> - else if (result == EC_RES_INVALID_PARAM) > + case -EINVAL: /* invalid parameter */ > return i; > - else if (result) > + case -EPROTO: > + /* Old or new error return code: Handle both */ > + if (result == EC_RES_INVALID_COMMAND) > + return -ENODEV; > + else if (result == EC_RES_INVALID_PARAM) > + return i;
If I understand correctly this surprising calling convention (output parameter is filled even though the function returned an error) is the old one that is to be fixed.
> return -EPROTO; > + default: > + if (ret < 0) > + return ret; > + break; > + } > } >
Best regards Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |