lkml.org 
[lkml]   [2018]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/6] staging:iio:ad2s90: Make read_raw return spi_read's error code
From
Date

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

\
 
 \ /
  Last update: 2018-11-02 14:50    [W:0.058 / U:1.344 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site