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

[Guenter Roeck <linux@roeck-us.net>]

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/ |