| Message ID | 20200726220101.29059-5-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes | expand |
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(-) Acked-by: Thierry Reding <thierry.reding@gmail.com>
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
On Sat, Aug 01, 2020 at 09:21:30AM +0200, Uwe Kleine-König wrote: > 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. > Good point. I'll rephrase. > > - 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. > I'll be happy to add a comment. > > - 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. > Sorry, I don't get your point. This is the old convention, correct, which we still want to support at this point. Plus, it matches the current code, as surprosing as it may be. Guenter > > 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/ |
Hello Guenter, On Sat, Aug 01, 2020 at 09:32:19AM -0700, Guenter Roeck wrote: > > 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. > > Sorry, I don't get your point. This is the old convention, correct, > which we still want to support at this point. Plus, it matches the > current code, as surprosing as it may be. OK, so I understood correctly and everything is fine. Best regards Uwe
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. */ - if (result == EC_RES_INVALID_COMMAND) + switch (ret) { + case -EOPNOTSUPP: /* invalid command */ return -ENODEV; - 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; return -EPROTO; + default: + if (ret < 0) + return ret; + break; + } } return U8_MAX;
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(-)