Messages in this thread | | | Subject | Re: [PATCH v2 1/6] staging:iio:ad2s90: Make read_raw return spi_read's error code | From | Matheus Tavares <> | Date | Fri, 2 Nov 2018 10:49:59 -0300 |
| |
On 10/28/18 1:40 PM, Jonathan Cameron wrote: > On Fri, 26 Oct 2018 23:00:00 -0300 > Matheus Tavares <matheus.bernardino@usp.br> 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 <matheus.bernardino@usp.br> > 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(&st->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(&st->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(&st->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(&st->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(&st->lock); >> >> return IIO_VAL_INT; > The 'standard' if slightly nasty way of doing this is: > > return ret ? ret : IIO_VAL_INT; >
| |