staging: iio: adis16240: Remove mutex_lock() and mutex_unlock() function call.

Remove mutex_lock() and mutex_unlock() function calls, as the adis16240_spi_read_signed() function can be run parallel and safely multiple times. Also remove the mutex.h header file and comment, which are no longer required.

As indio_dev is declared and initialized in adis16240_spi_read_signed(), again declaration to same type and initialization to same value is not required, remove it from adis16240_read_12bit_signed().

Simplify the return logic, by merging assignment and return into a single line.

Signed-off-by Varsha Rao <[email protected]>

---
Changes in v2:

- mutex_lock() and mutex_unlock() are removed, instead of replacing mlock with driver private lock.
- Commit message modified.
- mutex.h header file is removed.
- Unnecessary comment is removed.
- Return logic is compressed to single line.
- Removed indio_dev declaration and initialization from adis16240_read_12bit_signed().

 drivers/staging/iio/accel/adis16240.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

drivers/staging/iio/accel/adis16240.c +1 -16

index 27d7f6a..3a5f6ab 100644
@@ -10,7 +10,6 @@
 #include <linux/irq.h>
 #include <linux/gpio.h>
 #include <linux/delay.h>
-#include <linux/mutex.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
 #include <linux/spi/spi.h>

@@ -227,15 +226,7 @@ static ssize_t adis16240_read_12bit_signed(struct device *dev,
 					   struct device_attribute *attr,
 					   char *buf)
 {
-	ssize_t ret;
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-
-	/* Take the iio_dev status lock */
-	mutex_lock(&indio_dev->mlock);
-	ret =  adis16240_spi_read_signed(dev, attr, buf, 12);
-	mutex_unlock(&indio_dev->mlock);
-
-	return ret;
+	return adis16240_spi_read_signed(dev, attr, buf, 12);
 }
 
 static IIO_DEVICE_ATTR(in_accel_xyz_squared_peak_raw, 0444,

@@ -295,31 +286,25 @@ static int adis16240_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_CALIBBIAS:
 		bits = 10;
-		mutex_lock(&indio_dev->mlock);
 		addr = adis16240_addresses[chan->scan_index][0];
 		ret = adis_read_reg_16(st, addr, &val16);
 		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
 			return ret;
 		}
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
-		mutex_unlock(&indio_dev->mlock);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_PEAK:
 		bits = 10;
-		mutex_lock(&indio_dev->mlock);
 		addr = adis16240_addresses[chan->scan_index][1];
 		ret = adis_read_reg_16(st, addr, &val16);
 		if (ret) {
-			mutex_unlock(&indio_dev->mlock);
 			return ret;
 		}
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
-		mutex_unlock(&indio_dev->mlock);
 		return IIO_VAL_INT;
 	}
 	return -EINVAL;

It's a bit obscure, but isn't there the potential to get the check_status call spitting out text for the wrong read, or potentially miss a status value entirely?

(kind of more for Lars really, but feel free to comment Varsha!)

The various faults look like this wouldn't be an issue, but I'm not totally sure about the SPI failure and whether a following good read at exactly the wrong time might result in the error not being reported...

Quoted 5 lines · Varsha Rao

Remove mutex_lock() and mutex_unlock() function calls, as the adis16240_spi_read_signed() function can be run parallel and safely multiple times. Also remove the mutex.h header file and comment, which are no longer required.

Quoted 8 lines

As indio_dev is declared and initialized in adis16240_spi_read_signed(), again declaration to same type and initialization to same value is not required, remove it from adis16240_read_12bit_signed().

Simplify the return logic, by merging assignment and return into a single line.

Signed-off-by Varsha Rao <[email protected]>

It's a bit obscure, but isn't there the potential to get the check_status call spitting out text for the wrong read, or potentially miss a status value entirely?

(kind of more for Lars really, but feel free to comment Varsha!)

The various faults look like this wouldn't be an issue, but I'm not totally sure about the SPI failure and whether a following good read at exactly the wrong time might result in the error not being reported...

Quoted 18 lines · Jonathan Cameron

Quoted 5 lines · Varsha Rao

Remove mutex_lock() and mutex_unlock() function calls, as the adis16240_spi_read_signed() function can be run parallel and safely multiple times. Also remove the mutex.h header file and comment, which are no longer required.

Quoted 8 lines

As indio_dev is declared and initialized in adis16240_spi_read_signed(), again declaration to same type and initialization to same value is not required, remove it from adis16240_read_12bit_signed().

Simplify the return logic, by merging assignment and return into a single line.

Signed-off-by Varsha Rao <[email protected]>

It's a bit obscure, but isn't there the potential to get the check_status call spitting out text for the wrong read, or potentially miss a status value entirely?

Quoted 5 lines

(kind of more for Lars really, but feel free to comment Varsha!)

The various faults look like this wouldn't be an issue, but I'm not totally sure about the SPI failure and whether a following good read at exactly the wrong time might result in the error not being reported...

I don't think it matters. The error reporting is asynchronous anyway and the bits are read-to-clear. So even if two access see the error flag set in the data only one of them will be able to read the bits in the diag_stat register (unless the error is persistent, in which case the error flag in the data would also be persistent).

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html