diff mbox series

[v3,4/6] pwm: cros-ec: Accept more error codes from cros_ec_cmd_xfer_status

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

Commit Message

Guenter Roeck July 26, 2020, 10 p.m. UTC
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(-)

Comments

Thierry Reding July 27, 2020, 8:35 a.m. UTC | #1
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>
Uwe Kleine-König Aug. 1, 2020, 7:21 a.m. UTC | #2
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
Guenter Roeck Aug. 1, 2020, 4:32 p.m. UTC | #3
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/ |
Uwe Kleine-König Aug. 2, 2020, 8:27 p.m. UTC | #4
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 mbox series

Patch

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;