Re: [PATCH v2 1/6] staging:iio:ad2s90: Make read_raw return spi_read's error code
On Fri, 2 Nov 2018 10:49:59 -0300
Matheus Tavares wrote:
> On 10/28/18 1:40 PM, Jonathan Cameron wrote:
> > On Fri, 26 Oct 2018 23:00:00 -0300
> > Matheus Tavares wrote:
> >
> >> Previously, when spi_read returned an error code inside ad2s90_read_raw,
> >> the code was ignored and IIO_VAL_INT was returned. This patch makes the
> >> function return the error code returned by spi_read when it fails.
> >>
> >> Signed-off-by: Matheus Tavares
> > Hi Matheus,
> >
> > One quick process note is that it takes people a while to get around to
> > reviewing
> > a series, so whilst it's tempting to very quickly send out a fix the moment
> > someone points out something that needs fixing, it is perhaps better to wait
> > at least a few days to see if you can pick up a few more reviews before you
> > do a V2.
> >
> > A few comments on this one inline. I think it can be done 'slightly'
> > (and I mean only slightly) nicer than the version you have. Result is the
> > same though.
> >
> > Thanks,
> >
> > Jonathan
> >
> >> ---
> >> drivers/staging/iio/resolver/ad2s90.c | 9 ++---
> >> 1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/resolver/ad2s90.c
> >> b/drivers/staging/iio/resolver/ad2s90.c
> >> index 59586947a936..11fac9f90148 100644
> >> --- a/drivers/staging/iio/resolver/ad2s90.c
> >> +++ b/drivers/staging/iio/resolver/ad2s90.c
> >> @@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
> >>struct ad2s90_state *st = iio_priv(indio_dev);
> >>
> >>mutex_lock(>lock);
> >> +
> > Unconnected change. I'm not against the change in principle but please
> > group white space tidying up in it's own patch.
> >
> >>ret = spi_read(st->sdev, st->rx, 2);
> >> - if (ret)
> >> - goto error_ret;
> >> + if (ret < 0) {
> >> + mutex_unlock(>lock);
> >> + return ret;
> > I'd actually prefer to keep the return path the same as before as then
> > it is easy (if the function gets more complex in future) to be sure
> > that all paths unlock the mutex.
>
>
> Ok, got it! But then, in patch 5, when we add the switch for
> IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW, should I keep the goto and
> label inside the switch case? I mean, should it be something like this:
>
>
> switch (m) {
> case IIO_CHAN_INFO_SCALE:
> ... // Does not use mutex
> case IIO_CHAN_INFO_RAW:
> mutex_lock(>lock);
> ret = spi_read(st->sdev, st->rx, 2);
> if (ret)
> goto error_ret;
> *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
>
> error_ret:
> mutex_unlock(>lock);
>
> return ret ? ret : IIO_VAL_INT;
> default:
> break;
> }
This is was of those ugly signs that perhaps a given block of code
should be factored out as a separate function. It does feel like
overkill here though given how short the ugly bit will be ;)
So now I get why you did the refactor in the first place (I
hadn't made the connection).
I think on balance your first answer was the best one given
this is going to be deeper nested. If the function gets
more complex then this block should factored out anyway once the switch
statement is there then we go back to the simple exit path.
Thanks,
Jonathan
>
>
> Matheus
>
>
> >> + }
> >> +
> >>*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
> >>
> >> -error_ret:
> >>mutex_unlock(>lock);
> >>
> >>return IIO_VAL_INT;
> > The 'standard' if slightly nasty way of doing this is:
> >
> > return ret ? ret : IIO_VAL_INT;
> >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/6] staging:iio:ad2s90: Make read_raw return spi_read's error code
On 10/28/18 1:40 PM, Jonathan Cameron wrote:
On Fri, 26 Oct 2018 23:00:00 -0300
Matheus Tavares wrote:
Previously, when spi_read returned an error code inside ad2s90_read_raw,
the code was ignored and IIO_VAL_INT was returned. This patch makes the
function return the error code returned by spi_read when it fails.
Signed-off-by: Matheus Tavares
Hi Matheus,
One quick process note is that it takes people a while to get around to
reviewing
a series, so whilst it's tempting to very quickly send out a fix the moment
someone points out something that needs fixing, it is perhaps better to wait
at least a few days to see if you can pick up a few more reviews before you
do a V2.
A few comments on this one inline. I think it can be done 'slightly'
(and I mean only slightly) nicer than the version you have. Result is the
same though.
Thanks,
Jonathan
---
drivers/staging/iio/resolver/ad2s90.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s90.c
b/drivers/staging/iio/resolver/ad2s90.c
index 59586947a936..11fac9f90148 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
struct ad2s90_state *st = iio_priv(indio_dev);
mutex_lock(>lock);
+
Unconnected change. I'm not against the change in principle but please
group white space tidying up in it's own patch.
ret = spi_read(st->sdev, st->rx, 2);
- if (ret)
- goto error_ret;
+ if (ret < 0) {
+ mutex_unlock(>lock);
+ return ret;
I'd actually prefer to keep the return path the same as before as then
it is easy (if the function gets more complex in future) to be sure
that all paths unlock the mutex.
Ok, got it! But then, in patch 5, when we add the switch for
IIO_CHAN_INFO_SCALE and IIO_CHAN_INFO_RAW, should I keep the goto and
label inside the switch case? I mean, should it be something like this:
switch (m) {
case IIO_CHAN_INFO_SCALE:
... // Does not use mutex
case IIO_CHAN_INFO_RAW:
mutex_lock(>lock);
ret = spi_read(st->sdev, st->rx, 2);
if (ret)
goto error_ret;
*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
error_ret:
mutex_unlock(>lock);
return ret ? ret : IIO_VAL_INT;
default:
break;
}
Matheus
+ }
+
*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
-error_ret:
mutex_unlock(>lock);
return IIO_VAL_INT;
The 'standard' if slightly nasty way of doing this is:
return ret ? ret : IIO_VAL_INT;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/6] staging:iio:ad2s90: Make read_raw return spi_read's error code
On Fri, 26 Oct 2018 23:00:00 -0300
Matheus Tavares wrote:
> Previously, when spi_read returned an error code inside ad2s90_read_raw,
> the code was ignored and IIO_VAL_INT was returned. This patch makes the
> function return the error code returned by spi_read when it fails.
>
> Signed-off-by: Matheus Tavares
Hi Matheus,
One quick process note is that it takes people a while to get around to
reviewing
a series, so whilst it's tempting to very quickly send out a fix the moment
someone points out something that needs fixing, it is perhaps better to wait
at least a few days to see if you can pick up a few more reviews before you
do a V2.
A few comments on this one inline. I think it can be done 'slightly'
(and I mean only slightly) nicer than the version you have. Result is the
same though.
Thanks,
Jonathan
> ---
> drivers/staging/iio/resolver/ad2s90.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s90.c
> b/drivers/staging/iio/resolver/ad2s90.c
> index 59586947a936..11fac9f90148 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
> struct ad2s90_state *st = iio_priv(indio_dev);
>
> mutex_lock(>lock);
> +
Unconnected change. I'm not against the change in principle but please
group white space tidying up in it's own patch.
> ret = spi_read(st->sdev, st->rx, 2);
> - if (ret)
> - goto error_ret;
> + if (ret < 0) {
> + mutex_unlock(>lock);
> + return ret;
I'd actually prefer to keep the return path the same as before as then
it is easy (if the function gets more complex in future) to be sure
that all paths unlock the mutex.
> + }
> +
> *val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
>
> -error_ret:
> mutex_unlock(>lock);
>
> return IIO_VAL_INT;
The 'standard' if slightly nasty way of doing this is:
return ret ? ret : IIO_VAL_INT;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/6] staging:iio:ad2s90: Make read_raw return spi_read's error code
Previously, when spi_read returned an error code inside ad2s90_read_raw,
the code was ignored and IIO_VAL_INT was returned. This patch makes the
function return the error code returned by spi_read when it fails.
Signed-off-by: Matheus Tavares
---
drivers/staging/iio/resolver/ad2s90.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s90.c
b/drivers/staging/iio/resolver/ad2s90.c
index 59586947a936..11fac9f90148 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -35,12 +35,15 @@ static int ad2s90_read_raw(struct iio_dev *indio_dev,
struct ad2s90_state *st = iio_priv(indio_dev);
mutex_lock(>lock);
+
ret = spi_read(st->sdev, st->rx, 2);
- if (ret)
- goto error_ret;
+ if (ret < 0) {
+ mutex_unlock(>lock);
+ return ret;
+ }
+
*val = (((u16)(st->rx[0])) << 4) | ((st->rx[1] & 0xF0) >> 4);
-error_ret:
mutex_unlock(>lock);
return IIO_VAL_INT;
--
2.18.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
