[PATCH v9] staging: iio: adis16060: Remove iio_dev mlock and refactor code

[PATCH v9] staging: iio: adis16060: Remove iio_dev mlock and refactor code

[simran singhal]

The IIO subsystem is redefining iio_dev->mlock to be used by
the IIO core only for protecting device operating mode changes.
ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.

In this driver, mlock was being used to protect hardware state changes.

In the driver, buf_lock protects both the adis16060_spi_write() and
adis16060_spi_read() functions and both are always called in pair.
First write, then read. Refactor the code to have one single function
adis16060_spi_write_then_read() protected by the buf_lock. This removes
the need for additional locking via mlock, so this locking is removed.

Signed-off-by: simran singhal <singhalsimran0@gmail.com>
---

 v9:
   -Change the name of function from adis16060_spi_write_than_read()
    to adis16060_spi_write_then_read(). change "than" to "then" as
    its time depended.
   -Add mutex_unlock
 
 drivers/staging/iio/gyro/adis16060_core.c | 35 +++++++++----------------------
 1 file changed, 10 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/iio/gyro/adis16060_core.c b/drivers/staging/iio/gyro/adis16060_core.c
index c9d46e7..9675245 100644
--- a/drivers/staging/iio/gyro/adis16060_core.c
+++ b/drivers/staging/iio/gyro/adis16060_core.c
@@ -40,25 +40,20 @@ struct adis16060_state {
 
 static struct iio_dev *adis16060_iio_dev;
 
-static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
+static int adis16060_spi_write_then_read(struct iio_dev *indio_dev,
+					 u8 conf, u16 *val)
 {
 	int ret;
 	struct adis16060_state *st = iio_priv(indio_dev);
 
 	mutex_lock(&st->buf_lock);
-	st->buf[2] = val; /* The last 8 bits clocked in are latched */
+	st->buf[2] = conf; /* The last 8 bits clocked in are latched */
 	ret = spi_write(st->us_w, st->buf, 3);
-	mutex_unlock(&st->buf_lock);
-
-	return ret;
-}
-
-static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
-{
-	int ret;
-	struct adis16060_state *st = iio_priv(indio_dev);
 
-	mutex_lock(&st->buf_lock);
+	if (ret < 0) {
+		mutex_unlock(&st->buf_lock);
+		return ret;
+	}
 
 	ret = spi_read(st->us_r, st->buf, 3);
 
@@ -86,17 +81,11 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
-		/* Take the iio_dev status lock */
-		mutex_lock(&indio_dev->mlock);
-		ret = adis16060_spi_write(indio_dev, chan->address);
+		ret = adis16060_spi_write_then_read(indio_dev,
+						    chan->address, &tval);
 		if (ret < 0)
-			goto out_unlock;
+			return ret;
 
-		ret = adis16060_spi_read(indio_dev, &tval);
-		if (ret < 0)
-			goto out_unlock;
-
-		mutex_unlock(&indio_dev->mlock);
 		*val = tval;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_OFFSET:
@@ -110,10 +99,6 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
 	}
 
 	return -EINVAL;
-
-out_unlock:
-	mutex_unlock(&indio_dev->mlock);
-	return ret;
 }
 
 static const struct iio_info adis16060_info = {
-- 
2.7.4

Re: [PATCH v9] staging: iio: adis16060: Remove iio_dev mlock and refactor code

[Jonathan Cameron]

On 23/03/17 09:20, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> 
> In this driver, mlock was being used to protect hardware state changes.
> 
> In the driver, buf_lock protects both the adis16060_spi_write() and
> adis16060_spi_read() functions and both are always called in pair.
> First write, then read. Refactor the code to have one single function
> adis16060_spi_write_then_read() protected by the buf_lock. This removes
> the need for additional locking via mlock, so this locking is removed.
> 
> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
Unfortunately I've now pushed out v8 as togreg which is a non rebasing tree.
As such, could you post a separate patch, on top of that making the
than -> then change?

Thanks,

Jonathan
> ---
> 
>  v9:
>    -Change the name of function from adis16060_spi_write_than_read()
>     to adis16060_spi_write_then_read(). change "than" to "then" as
>     its time depended.
>    -Add mutex_unlock
As Alison keeps mentioning, ideal is to keep all previous change logs
here as it allows those who last looked at say v5 to know what changed
in between without reading a lot of threads!
>  
>  drivers/staging/iio/gyro/adis16060_core.c | 35 +++++++++----------------------
>  1 file changed, 10 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..9675245 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -40,25 +40,20 @@ struct adis16060_state {
>  
>  static struct iio_dev *adis16060_iio_dev;
>  
> -static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
> +static int adis16060_spi_write_then_read(struct iio_dev *indio_dev,
> +					 u8 conf, u16 *val)
>  {
>  	int ret;
>  	struct adis16060_state *st = iio_priv(indio_dev);
>  
>  	mutex_lock(&st->buf_lock);
> -	st->buf[2] = val; /* The last 8 bits clocked in are latched */
> +	st->buf[2] = conf; /* The last 8 bits clocked in are latched */
>  	ret = spi_write(st->us_w, st->buf, 3);
> -	mutex_unlock(&st->buf_lock);
> -
> -	return ret;
> -}
> -
> -static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
> -{
> -	int ret;
> -	struct adis16060_state *st = iio_priv(indio_dev);
>  
> -	mutex_lock(&st->buf_lock);
> +	if (ret < 0) {
> +		mutex_unlock(&st->buf_lock);
> +		return ret;
> +	}
>  
>  	ret = spi_read(st->us_r, st->buf, 3);
>  
> @@ -86,17 +81,11 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> -		/* Take the iio_dev status lock */
> -		mutex_lock(&indio_dev->mlock);
> -		ret = adis16060_spi_write(indio_dev, chan->address);
> +		ret = adis16060_spi_write_then_read(indio_dev,
> +						    chan->address, &tval);
>  		if (ret < 0)
> -			goto out_unlock;
> +			return ret;
>  
> -		ret = adis16060_spi_read(indio_dev, &tval);
> -		if (ret < 0)
> -			goto out_unlock;
> -
> -		mutex_unlock(&indio_dev->mlock);
>  		*val = tval;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_OFFSET:
> @@ -110,10 +99,6 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>  	}
>  
>  	return -EINVAL;
> -
> -out_unlock:
> -	mutex_unlock(&indio_dev->mlock);
> -	return ret;
>  }
>  
>  static const struct iio_info adis16060_info = {
> 

Re: [PATCH v9] staging: iio: adis16060: Remove iio_dev mlock and refactor code

[SIMRAN SINGHAL]

On Sat, Mar 25, 2017 at 11:16 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 23/03/17 09:20, simran singhal wrote:
>> The IIO subsystem is redefining iio_dev->mlock to be used by
>> the IIO core only for protecting device operating mode changes.
>> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>>
>> In this driver, mlock was being used to protect hardware state changes.
>>
>> In the driver, buf_lock protects both the adis16060_spi_write() and
>> adis16060_spi_read() functions and both are always called in pair.
>> First write, then read. Refactor the code to have one single function
>> adis16060_spi_write_then_read() protected by the buf_lock. This removes
>> the need for additional locking via mlock, so this locking is removed.
>>
>> Signed-off-by: simran singhal <singhalsimran0@gmail.com>
> Unfortunately I've now pushed out v8 as togreg which is a non rebasing tree.
> As such, could you post a separate patch, on top of that making the
> than -> then change?
>

ok, I will send the patch with than-> then change.

> Thanks,
>
> Jonathan
>> ---
>>
>>  v9:
>>    -Change the name of function from adis16060_spi_write_than_read()
>>     to adis16060_spi_write_then_read(). change "than" to "then" as
>>     its time depended.
>>    -Add mutex_unlock
> As Alison keeps mentioning, ideal is to keep all previous change logs
> here as it allows those who last looked at say v5 to know what changed
> in between without reading a lot of threads!
>>

Sorry, will not repeate this in future.

>>  drivers/staging/iio/gyro/adis16060_core.c | 35 +++++++++----------------------
>>  1 file changed, 10 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/staging/iio/gyro/adis16060_core.c b/drivers/staging/iio/gyro/adis16060_core.c
>> index c9d46e7..9675245 100644
>> --- a/drivers/staging/iio/gyro/adis16060_core.c
>> +++ b/drivers/staging/iio/gyro/adis16060_core.c
>> @@ -40,25 +40,20 @@ struct adis16060_state {
>>
>>  static struct iio_dev *adis16060_iio_dev;
>>
>> -static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
>> +static int adis16060_spi_write_then_read(struct iio_dev *indio_dev,
>> +                                      u8 conf, u16 *val)
>>  {
>>       int ret;
>>       struct adis16060_state *st = iio_priv(indio_dev);
>>
>>       mutex_lock(&st->buf_lock);
>> -     st->buf[2] = val; /* The last 8 bits clocked in are latched */
>> +     st->buf[2] = conf; /* The last 8 bits clocked in are latched */
>>       ret = spi_write(st->us_w, st->buf, 3);
>> -     mutex_unlock(&st->buf_lock);
>> -
>> -     return ret;
>> -}
>> -
>> -static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
>> -{
>> -     int ret;
>> -     struct adis16060_state *st = iio_priv(indio_dev);
>>
>> -     mutex_lock(&st->buf_lock);
>> +     if (ret < 0) {
>> +             mutex_unlock(&st->buf_lock);
>> +             return ret;
>> +     }
>>
>>       ret = spi_read(st->us_r, st->buf, 3);
>>
>> @@ -86,17 +81,11 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>>
>>       switch (mask) {
>>       case IIO_CHAN_INFO_RAW:
>> -             /* Take the iio_dev status lock */
>> -             mutex_lock(&indio_dev->mlock);
>> -             ret = adis16060_spi_write(indio_dev, chan->address);
>> +             ret = adis16060_spi_write_then_read(indio_dev,
>> +                                                 chan->address, &tval);
>>               if (ret < 0)
>> -                     goto out_unlock;
>> +                     return ret;
>>
>> -             ret = adis16060_spi_read(indio_dev, &tval);
>> -             if (ret < 0)
>> -                     goto out_unlock;
>> -
>> -             mutex_unlock(&indio_dev->mlock);
>>               *val = tval;
>>               return IIO_VAL_INT;
>>       case IIO_CHAN_INFO_OFFSET:
>> @@ -110,10 +99,6 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>>       }
>>
>>       return -EINVAL;
>> -
>> -out_unlock:
>> -     mutex_unlock(&indio_dev->mlock);
>> -     return ret;
>>  }
>>
>>  static const struct iio_info adis16060_info = {
>>
>