Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	Jonathan Cameron <jic23@kernel.org>,
	Benson Leung <bleung@chromium.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Gwendal Grignou <gwendal@chromium.org>,
	Brian Norris <briannorris@chromium.org>,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-input@vger.kernel.org, linux-pwm@vger.kernel.org,
	Yu-Hsuan Hsu <yuhsuan@chromium.org>,
	Prashant Malani <pmalani@chromium.org>
Subject: Re: [PATCH v3 4/6] pwm: cros-ec: Accept more error codes from cros_ec_cmd_xfer_status
Date: Sat, 1 Aug 2020 09:32:19 -0700
Message-ID: <20200801163219.GA230759@roeck-us.net> (raw)
In-Reply-To: <20200801072130.tmm7b4vtizshmmyo@pengutronix.de>

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



  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-26 22:00 [PATCH v3 0/6] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes Guenter Roeck
2020-07-26 22:00 ` [PATCH v3 1/6] iio: cros_ec: Accept -EOPNOTSUPP as 'not supported' error code Guenter Roeck
2020-07-26 22:00 ` [PATCH v3 2/6] cros_ec_lightbar: Accept more error codes from cros_ec_cmd_xfer_status Guenter Roeck
2020-07-26 22:00 ` [PATCH v3 3/6] platform/chrome: cros_ec_sysfs: Report range of error codes from EC Guenter Roeck
2020-07-26 22:00 ` [PATCH v3 4/6] pwm: cros-ec: Accept more error codes from cros_ec_cmd_xfer_status Guenter Roeck
2020-07-27  8:35   ` Thierry Reding
2020-08-01  7:21   ` Uwe Kleine-König
2020-08-01 16:32     ` Guenter Roeck [this message]
2020-08-02 20:27       ` Uwe Kleine-König
2020-07-26 22:01 ` [PATCH v3 5/6] platform/input: cros_ec: Replace -ENOTSUPP with -ENOPROTOOPT Guenter Roeck
2020-07-30  4:53   ` Dmitry Torokhov
2020-07-26 22:01 ` [PATCH v3 6/6] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes Guenter Roeck
2020-07-29 22:21   ` Brian Norris
2020-07-29 23:22     ` Guenter Roeck
2020-07-29 23:29       ` Brian Norris
2020-07-29 22:27 ` [PATCH v3 0/6] " Brian Norris

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200801163219.GA230759@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=bleung@chromium.org \
    --cc=briannorris@chromium.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=enric.balletbo@collabora.com \
    --cc=gwendal@chromium.org \
    --cc=jic23@kernel.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=pmalani@chromium.org \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=yuhsuan@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git