Skip to contentSkip to conversation

iio: chemical: ams-iaq-core: remove mutex and replace with iio_device_*_direct_mode helpers

PATCH In review Matt Ranostay 2 participants 2 messages Aug 24, 2016 – Aug 29, 2016 IIO

lore
Share & tools
lore

Matt Ranostay

author
#0 lore raw

PATCH 1/1 iio: chemical: ams-iaq-core: remove mutex and replace with iio_device_*_direct_mode helpers

Using a separate mutex doesn't make sense with the iio_device_*_direct_mode helpers that use the indio_dev->mlock mutex that already exists.

Signed-off-by Matt Ranostay <[email protected]>
---
 drivers/iio/chemical/ams-iaq-core.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/iio/chemical/ams-iaq-core.c +5 -4
index 41a8e6f2e31d..4dd5a4db6a75 100644
@@ -36,7 +36,6 @@ struct ams_iaqcore_reading {
 
 struct ams_iaqcore_data {
 	struct i2c_client *client;
-	struct mutex lock;
 	unsigned long last_update;
 
 	struct ams_iaqcore_reading buffer;

@@ -108,7 +107,10 @@ static int ams_iaqcore_read_raw(struct iio_dev *indio_dev,
 	if (mask != IIO_CHAN_INFO_PROCESSED)
 		return -EINVAL;
 
-	mutex_lock(&data->lock);
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
 	ret = ams_iaqcore_get_measurement(data);
 
 	if (ret)

@@ -134,7 +136,7 @@ static int ams_iaqcore_read_raw(struct iio_dev *indio_dev,
 	}
 
 err_out:
-	mutex_unlock(&data->lock);
+	iio_device_release_direct_mode(indio_dev);
 
 	return ret;
 }

@@ -160,7 +162,6 @@ static int ams_iaqcore_probe(struct i2c_client *client,
 
 	/* so initial reading will complete */
 	data->last_update = jiffies - HZ;
-	mutex_init(&data->lock);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &ams_iaqcore_info,
Signature · 1 line
2.7.4
maintainer
#1 lore raw
Quoted 3 lines · Matt Ranostay

Using a separate mutex doesn't make sense with the iio_device_*_direct_mode helpers that use the indio_dev->mlock mutex that already exists.

I'm not keen on using those wrappers for anything other than what their naming defines (or the underlying mutex for that matter). Their implementation is effectively opaque to the drivers.

There are no guarantees that we won't do something different and nefarious in there some time in the future.

Hence if there is an element of a driver that needs protecting it makes sense to keep the lock explicitly in the driver where it is nice and obvious that it is doing it's job.

Clearly we need to be careful about ordering to avoid deadlock, but that's not exactly tricky in most cases!

Hence, I prefer this as it was before this patch.

Jonathan

Quoted 48 lines
Signed-off-by Matt Ranostay <[email protected]>
---
 drivers/iio/chemical/ams-iaq-core.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/iio/chemical/ams-iaq-core.c +5 -4
index 41a8e6f2e31d..4dd5a4db6a75 100644
@@ -36,7 +36,6 @@ struct ams_iaqcore_reading {
 
 struct ams_iaqcore_data {
 	struct i2c_client *client;
-	struct mutex lock;
 	unsigned long last_update;
 
 	struct ams_iaqcore_reading buffer;

@@ -108,7 +107,10 @@ static int ams_iaqcore_read_raw(struct iio_dev *indio_dev,
 	if (mask != IIO_CHAN_INFO_PROCESSED)
 		return -EINVAL;
 
-	mutex_lock(&data->lock);
+	ret = iio_device_claim_direct_mode(indio_dev);
+	if (ret)
+		return ret;
+
 	ret = ams_iaqcore_get_measurement(data);
 
 	if (ret)

@@ -134,7 +136,7 @@ static int ams_iaqcore_read_raw(struct iio_dev *indio_dev,
 	}
 
 err_out:
-	mutex_unlock(&data->lock);
+	iio_device_release_direct_mode(indio_dev);
 
 	return ret;
 }

@@ -160,7 +162,6 @@ static int ams_iaqcore_probe(struct i2c_client *client,
 
 	/* so initial reading will complete */
 	data->last_update = jiffies - HZ;
-	mutex_init(&data->lock);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &ams_iaqcore_info,