staging: iio: accel: replace mlock with driver private lock
Subsystems (4)
PATCH 1/1 staging: iio: accel: replace mlock with driver private lock
IIO subsystem is redefining iio_dev mlock to be used by IIO core only for protecting device operating mode changes.
In the driver adis164201, a struct adis16201_state has been introduced to store driver state information. Wherever iio_dev->mlock was used to protect hardware state changes, it has been replaced by driver private lock.
--- drivers/staging/iio/accel/adis16201.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)
drivers/staging/iio/accel/adis16201.c +20 -14
@@ -150,6 +150,11 @@#define ADIS16201_ERROR_ACTIVE BIT(14) +struct adis16201_state { +struct adis adis;/* adis device */ +struct mutex lock;/* protect state changes */ +}; +enum adis16201_scan {ADIS16201_SCAN_ACC_X ,ADIS16201_SCAN_ACC_Y ,
@@ -172,7 +177,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev, int *val, int *val2, long mask) { - struct adis *st = iio_priv(indio_dev); + struct adis16201_state *st = iio_priv(indio_dev);int ret;int bits;u8 addr;
@@ -223,17 +228,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,default :return -EINVAL ; } -mutex_lock (&indio_dev->mlock); +mutex_lock (&st->lock); addr = adis16201_addresses[chan->scan_index]; - ret =adis_read_reg_16 (st, addr, &val16); + ret =adis_read_reg_16 (&st->adis, addr, &val16);if (ret) { -mutex_unlock (&indio_dev->mlock); +mutex_unlock (&st->lock);return ret; } val16 &= (1 << bits) -1 ; val16 = (s16)(val16 << (16 - bits)) >> (16 - bits); *val = val16; -mutex_unlock (&indio_dev->mlock); +mutex_unlock (&st->lock);return IIO_VAL_INT ; }return -EINVAL ;
@@ -245,7 +250,7 @@ static int adis16201_write_raw(struct iio_dev *indio_dev, int val2, long mask) { -struct adis *st =iio_priv (indio_dev); +struct adis16201_state *st =iio_priv (indio_dev);int bits;s16 val16;u8 addr;
@@ -264,7 +269,7 @@ static int adis16201_write_raw(struct iio_dev *indio_dev, } val16 = val & ((1 << bits) -1 ); addr = adis16201_addresses[chan->scan_index]; -return adis_write_reg_16 (st, addr, val16); +return adis_write_reg_16 (&st->adis, addr, val16); }return -EINVAL ; }
@@ -318,7 +323,7 @@ static const struct adis_data adis16201_data = {static int adis16201_probe (struct spi_device *spi) {int ret; -struct adis *st; +struct adis16201_state *st;struct iio_dev *indio_dev;/* setup the industrialio driver allocated elements */
@@ -327,6 +332,7 @@ static int adis16201_probe(struct spi_device *spi)return -ENOMEM ; st =iio_priv (indio_dev); +mutex_init (&st->lock);/* this is only used for removal purposes */ spi_set_drvdata (spi, indio_dev);
@@ -338,15 +344,15 @@ static int adis16201_probe(struct spi_device *spi) indio_dev->num_channels =ARRAY_SIZE (adis16201_channels); indio_dev->modes =INDIO_DIRECT_MODE ; - ret =adis_init (st, indio_dev, spi, &adis16201_data); + ret =adis_init (&st->adis, indio_dev, spi, &adis16201_data);if (ret)return ret; - ret =adis_setup_buffer_and_trigger (st, indio_dev,NULL ); + ret =adis_setup_buffer_and_trigger (&st->adis, indio_dev,NULL );if (ret)return ret;/* Get the device into a sane initial state */ - ret =adis_initial_startup (st); + ret =adis_initial_startup (&st->adis);if (ret) goto error_cleanup_buffer_trigger;
@@ -356,17 +362,17 @@ static int adis16201_probe(struct spi_device *spi)return 0 ; error_cleanup_buffer_trigger: -adis_cleanup_buffer_and_trigger (st, indio_dev); +adis_cleanup_buffer_and_trigger (&st->adis, indio_dev);return ret; }static int adis16201_remove (struct spi_device *spi) {struct iio_dev *indio_dev =spi_get_drvdata (spi); -struct adis *st =iio_priv (indio_dev); +struct adis16201_state *st =iio_priv (indio_dev);iio_device_unregister (indio_dev); -adis_cleanup_buffer_and_trigger (st, indio_dev); +adis_cleanup_buffer_and_trigger (&st->adis, indio_dev);return 0 ; }
Signature · 1 line
2.7.4
Quoted 39 lines · Aishwarya Pant
IIO subsystem is redefining
iio_devmlock to be used by IIO core only for protecting device operating mode changes.In the driver adis164201, a struct
adis16201_statehas been introduced to store driver state information. Whereveriio_dev->mlockwas used to protect hardware state changes, it has been replaced by driver private lock.Signed-off-by Aishwarya Pant <[email protected]>--- drivers/staging/iio/accel/adis16201.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)drivers/staging/iio/accel/adis16201.c +6
-1@@ -150,6 +150,11 @@#define ADIS16201_ERROR_ACTIVE BIT(14) +struct adis16201_state { +struct adis adis;/* adis device */ +struct mutex lock;/* protect state changes */ +}; +enum adis16201_scan {ADIS16201_SCAN_ACC_X ,ADIS16201_SCAN_ACC_Y ,@@ -172,7 +177,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev, int *val, int *val2, long mask) { - struct adis *st = iio_priv(indio_dev); + struct adis16201_state *st = iio_priv(indio_dev);int ret;int bits;u8 addr;
drivers/staging/iio/accel/adis16201.c static int adis16201_read_raw(struct iio_dev *indio_dev, · line 231
Quoted 5 lines
drivers/staging/iio/accel/adis16201.c +1
-1@@ -223,17 +228,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,default :return -EINVAL ; } -mutex_lock (&indio_dev->mlock); +mutex_lock (&st->lock);
I think this lock could simply be removed completely. adis_read_reg_16() is already atomic and there is nothing else to protect here.
Quoted 16 lines
drivers/staging/iio/accel/adis16201.c +3
-3addr = adis16201_addresses[chan->scan_index]; - ret =adis_read_reg_16 (st, addr, &val16); + ret =adis_read_reg_16 (&st->adis, addr, &val16);if (ret) { -mutex_unlock (&indio_dev->mlock); +mutex_unlock (&st->lock);return ret; } val16 &= (1 << bits) -1 ; val16 = (s16)(val16 << (16 - bits)) >> (16 - bits); *val = val16; -mutex_unlock (&indio_dev->mlock); +mutex_unlock (&st->lock);return IIO_VAL_INT ; }return -EINVAL ;
Quoted 40 lines · Lars-Peter Clausen
Quoted 39 lines · Aishwarya Pant
IIO subsystem is redefining
iio_devmlock to be used by IIO core only for protecting device operating mode changes.In the driver adis164201, a struct
adis16201_statehas been introduced to store driver state information. Whereveriio_dev->mlockwas used to protect hardware state changes, it has been replaced by driver private lock.Signed-off-by Aishwarya Pant <[email protected]>--- drivers/staging/iio/accel/adis16201.c | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-)drivers/staging/iio/accel/adis16201.c +6
-1@@ -150,6 +150,11 @@#define ADIS16201_ERROR_ACTIVE BIT(14) +struct adis16201_state { +struct adis adis;/* adis device */ +struct mutex lock;/* protect state changes */ +}; +enum adis16201_scan {ADIS16201_SCAN_ACC_X ,ADIS16201_SCAN_ACC_Y ,@@ -172,7 +177,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev, int *val, int *val2, long mask) { - struct adis *st = iio_priv(indio_dev); + struct adis16201_state *st = iio_priv(indio_dev);int ret;int bits;u8 addr;@@ -223,17 +228,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
Quoted 8 lines
drivers/staging/iio/accel/adis16201.c static int adis16201_read_raw(struct iio_dev *indio_dev, · line 231
Quoted 5 lines
drivers/staging/iio/accel/adis16201.c +1
-1default :return -EINVAL ; } -mutex_lock (&indio_dev->mlock); +mutex_lock (&st->lock);I think this lock could simply be removed completely.
adis_read_reg_16()is already atomic and there is nothing else to protect here.
I believed the lock was put in here to prevent race condition in this line:
addr = adis16201_addresses[chan->scan_index];
Is that not the case here?
Quoted 18 lines
Quoted 17 lines
drivers/staging/iio/accel/adis16201.c +3
-3addr = adis16201_addresses[chan->scan_index]; - ret =adis_read_reg_16 (st, addr, &val16); + ret =adis_read_reg_16 (&st->adis, addr, &val16);if (ret) { -mutex_unlock (&indio_dev->mlock); +mutex_unlock (&st->lock);return ret; } val16 &= (1 << bits) -1 ; val16 = (s16)(val16 << (16 - bits)) >> (16 - bits); *val = val16; -mutex_unlock (&indio_dev->mlock); +mutex_unlock (&st->lock);return IIO_VAL_INT ; }return -EINVAL ;