staging: iio: ad7606: use iio_device_{claim|release}_direct_mode()

PATCH Superseded Alison Schofield 3 participants 8 messages Apr 1, 2016 – Apr 10, 2016

Subsystems (4)

Staging IIO Analog Devices IIO drivers Staging - industrial io

lore

Share & tools

lore

Alison Schofield

author

2016-04-01 16:54 UTC #0 lore raw

PATCH 1/1 staging: iio: ad7606: use iio_device_{claim|release}_direct_mode()

Two instances are moved to the new claim/release API:

In the first instance, the driver was using mlock followed by iio_buffer_enabled(). Replace that code with the new API to guarantee the device stays in direct mode. There is no change in driver behavior.

In the second instance, the driver was not using mlock to hold the device in direct mode, but should have been. Here we introduce the new API to guarantee direct mode. This is a change in driver behavior.

Signed-off-by Alison Schofield <[email protected]>

---
 drivers/staging/iio/adc/ad7606_core.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

drivers/staging/iio/adc/ad7606_core.c +8 -7

index 6dbc811..f914b8d 100644
@@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
-		if (iio_buffer_enabled(indio_dev))
-			ret = -EBUSY;
-		else
-			ret = ad7606_scan_direct(indio_dev, chan->address);
-		mutex_unlock(&indio_dev->mlock);
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7606_scan_direct(indio_dev, chan->address);
+		iio_device_release_direct_mode(indio_dev);
 
 		if (ret < 0)
 			return ret;

@@ -411,8 +411,9 @@ static irqreturn_t ad7606_interrupt(int irq, void *dev_id)
 	struct iio_dev *indio_dev = dev_id;
 	struct ad7606_state *st = iio_priv(indio_dev);
 
-	if (iio_buffer_enabled(indio_dev)) {
+	if (!iio_device_claim_direct_mode(indio_dev))  {
 		schedule_work(&st->poll_work);
+		iio_device_release_direct_mode(indio_dev);
 	} else {
 		st->done = true;
 		wake_up_interruptible(&st->wq_data_avail);

Signature · 1 line

2.1.4

Jonathan Cameron

maintainer

2016-04-03 09:09 UTC #1 lore raw

Quoted 39 lines · Alison Schofield

Two instances are moved to the new claim/release API:
In the first instance, the driver was using mlock followed by iio_buffer_enabled(). Replace that code with the new API to guarantee the device stays in direct mode. There is no change in driver behavior.
In the second instance, the driver was not using mlock to hold the device in direct mode, but should have been. Here we introduce the new API to guarantee direct mode. This is a change in driver behavior.
Signed-off-by Alison Schofield <[email protected]>
---
 drivers/staging/iio/adc/ad7606_core.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/staging/iio/adc/ad7606_core.c +6 -6
index 6dbc811..f914b8d 100644
@@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
-		if (iio_buffer_enabled(indio_dev))
-			ret = -EBUSY;
-		else
-			ret = ad7606_scan_direct(indio_dev, chan->address);
-		mutex_unlock(&indio_dev->mlock);
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7606_scan_direct(indio_dev, chan->address);
+		iio_device_release_direct_mode(indio_dev);
 
 		if (ret < 0)
 			return ret;

drivers/staging/iio/adc/ad7606_core.c static irqreturn_t ad7606_interrupt(int irq, void *dev_id) · line 416

Quoted 7 lines

drivers/staging/iio/adc/ad7606_core.c +2 -1

@@ -411,8 +411,9 @@ static irqreturn_t ad7606_interrupt(int irq, void *dev_id)
 	struct iio_dev *indio_dev = dev_id;
 	struct ad7606_state *st = iio_priv(indio_dev);
 
-	if (iio_buffer_enabled(indio_dev)) {
+	if (!iio_device_claim_direct_mode(indio_dev))  {
 		schedule_work(&st->poll_work);
+		iio_device_release_direct_mode(indio_dev);

Unfortunately this won't work. That interrupt is still in traditional non threaded form. This will take a mutex in a top half interrupt handler where a sleep cannot occur.

I'm just wondering how expensive it would be to fix this by moving that over to a threaded handler. In the poll_work case (buffer) it would be cleaner to do so. I'm really confused what the intended interrupt handler
is in here. I 'think' the sequence is:

Trigger fires the convst pin whether in top half or the bottom half of a threaded interrupt, but not both - I guess this works, if it is rather 'unusual'.

We then get a interrupt to indicate that it has finished conversion and that filters through to actually fill the buffer via a traditional top half / bottom half interrupt handler.

So if we were to convert that to a threaded interrupt (with no top half / non threaded part), we could drop the schedule_work and just call ad7606_poll_bh_to_ring from the thread handler.

In the direct read case I doubt we care about the delay in dropping to a thread prior to signalling the data is ready.

Can't think why this driver is still in staging :)

Lars, any interest from Analog in getting this one cleaned up? Also do you have any test hardware, if we mess around with this interrupt handling?

Jonathan

Quoted 4 lines

drivers/staging/iio/adc/ad7606_core.c +0 -0

 	} else {
 		st->done = true;
 		wake_up_interruptible(&st->wq_data_avail);

Alison Schofield

author

2016-04-05 17:41 UTC #2 lore raw

Quoted 71 lines · Jonathan Cameron

Quoted 39 lines · Alison Schofield
Two instances are moved to the new claim/release API:
In the first instance, the driver was using mlock followed by iio_buffer_enabled(). Replace that code with the new API to guarantee the device stays in direct mode. There is no change in driver behavior.
In the second instance, the driver was not using mlock to hold the device in direct mode, but should have been. Here we introduce the new API to guarantee direct mode. This is a change in driver behavior.
Signed-off-by Alison Schofield <[email protected]>
---
 drivers/staging/iio/adc/ad7606_core.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
drivers/staging/iio/adc/ad7606_core.c +6 -6
index 6dbc811..f914b8d 100644
@@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
-		if (iio_buffer_enabled(indio_dev))
-			ret = -EBUSY;
-		else
-			ret = ad7606_scan_direct(indio_dev, chan->address);
-		mutex_unlock(&indio_dev->mlock);
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7606_scan_direct(indio_dev, chan->address);
+		iio_device_release_direct_mode(indio_dev);
 
 		if (ret < 0)
 			return ret;
drivers/staging/iio/adc/ad7606_core.c static irqreturn_t ad7606_interrupt(int irq, void *dev_id) · line 416
Quoted 7 lines
drivers/staging/iio/adc/ad7606_core.c +2 -1
@@ -411,8 +411,9 @@ static irqreturn_t ad7606_interrupt(int irq, void *dev_id)
 	struct iio_dev *indio_dev = dev_id;
 	struct ad7606_state *st = iio_priv(indio_dev);
 
-	if (iio_buffer_enabled(indio_dev)) {
+	if (!iio_device_claim_direct_mode(indio_dev))  {
 		schedule_work(&st->poll_work);
+		iio_device_release_direct_mode(indio_dev);
Unfortunately this won't work. That interrupt is still in traditional non threaded form. This will take a mutex in a top half interrupt handler where a sleep cannot occur.
I'm just wondering how expensive it would be to fix this by moving that over to a threaded handler. In the poll_work case (buffer) it would be cleaner to do so. I'm really confused what the intended interrupt handler
is in here. I 'think' the sequence is:
Trigger fires the convst pin whether in top half or the bottom half of a threaded interrupt, but not both - I guess this works, if it is rather 'unusual'.
We then get a interrupt to indicate that it has finished conversion and that filters through to actually fill the buffer via a traditional top half / bottom half interrupt handler.
So if we were to convert that to a threaded interrupt (with no top half / non threaded part), we could drop the schedule_work and just call ad7606_poll_bh_to_ring from the thread handler.
In the direct read case I doubt we care about the delay in dropping to a thread prior to signalling the data is ready.

Quoted 6 lines

Can't think why this driver is still in staging :)
Lars, any interest from Analog in getting this one cleaned up? Also do you have any test hardware, if we mess around with this interrupt handling?
Jonathan

I see the problem. Thanks for the review and the details for the redesign. Plan to v2 this patch leaving out this piece. I'll keep the interrupt handler rework on my radar pending Lars's comments.

alisons

Quoted 7 lines

Quoted 6 lines
drivers/staging/iio/adc/ad7606_core.c +0 -0
 	} else {
 		st->done = true;
 		wake_up_interruptible(&st->wq_data_avail);

Lars-Peter Clausen

2016-04-06 18:06 UTC #3 lore raw

In reply to Jonathan Cameron

Quoted 11 lines · Jonathan Cameron

Quoted 10 lines · Alison Schofield
Two instances are moved to the new claim/release API:
In the first instance, the driver was using mlock followed by iio_buffer_enabled(). Replace that code with the new API to guarantee the device stays in direct mode. There is no change in driver behavior.
In the second instance, the driver was not using mlock to hold the device in direct mode, but should have been. Here we introduce the new API to guarantee direct mode. This is a change in driver behavior.

Quoted 6 lines

Review
Quoted 6 lines
Signed-off-by Alison Schofield <[email protected]>
---
 drivers/staging/iio/adc/ad7606_core.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
(quoted diff) +0 -0

b/drivers/staging/iio/adc/ad7606_core.c

Quoted 20 lines

index 6dbc811..f914b8d 100644

drivers/staging/iio/adc/ad7606_core.c +6 -6

@@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
-		if (iio_buffer_enabled(indio_dev))
-			ret = -EBUSY;
-		else
-			ret = ad7606_scan_direct(indio_dev, chan->address);
-		mutex_unlock(&indio_dev->mlock);
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7606_scan_direct(indio_dev, chan->address);
+		iio_device_release_direct_mode(indio_dev);

Quoted 3 lines

drivers/staging/iio/adc/ad7606_core.c static irqreturn_t ad7606_interrupt(int irq, void
Quoted 3 lines
drivers/staging/iio/adc/ad7606_core.c +0 -0
 		if (ret < 0)
 			return ret;
@@ -411,8 +411,9 @@ static irqreturn_t ad7606_interrupt(int irq, void

*dev_id)

Quoted 11 lines

drivers/staging/iio/adc/ad7606_core.c static irqreturn_t ad7606_interrupt(int irq, void *dev_id) · line 416
Quoted 7 lines
drivers/staging/iio/adc/ad7606_core.c +2 -1
 	struct iio_dev *indio_dev = dev_id;
 	struct ad7606_state *st = iio_priv(indio_dev);
 
-	if (iio_buffer_enabled(indio_dev)) {
+	if (!iio_device_claim_direct_mode(indio_dev))  {
 		schedule_work(&st->poll_work);
+		iio_device_release_direct_mode(indio_dev);
Unfortunately this won't work. That interrupt is still in traditional non threaded form. This will take a mutex in a top half interrupt handler where a sleep cannot occur.

Quoted 2 lines

I'm just wondering how expensive it would be to fix this by moving that over to a threaded handler. In the poll_work case (buffer) it would be cleaner

to do

Quoted 11 lines

so. I'm really confused what the intended interrupt handler is in here. I 'think' the sequence is:
Trigger fires the convst pin whether in top half or the bottom half of a threaded interrupt, but not both - I guess this works, if it is rather 'unusual'.
We then get a interrupt to indicate that it has finished conversion and that filters through to actually fill the buffer via a traditional top half / bottom half interrupt handler.

Quoted 8 lines

So if we were to convert that to a threaded interrupt (with no top half / non threaded part), we could drop the schedule_work and just call ad7606_poll_bh_to_ring from the thread handler.
In the direct read case I doubt we care about the delay in dropping to a thread prior to signalling the data is ready.
Can't think why this driver is still in staging :)

Yeah, we should leave this driver out from the conversion for now. The whole convst pin handling need a major rework. It shouldn't really be in the driver and usually you wouldn't want to use to use a GPIO and software timer since that gives you way to much jitter for good results. You'd probably use something like a PWM.

Quoted 3 lines

Lars, any interest from Analog in getting this one cleaned up? Also do you have any test hardware, if we mess around with this interrupt handling?

I have the hardware somewhere in some storage bay, but just converting this over to threaded interrupt handling is not really a solution. So, if you want to get rid of the iio_buffer_enabled() in the interrupt handler a simple solution is to register preenable and postdisable callbacks where you set a flag in the driver struct to indicate that it is in buffered mode or not.

Jonathan Cameron

maintainer

2016-04-10 14:00 UTC #4 lore raw

Quoted 75 lines · Lars-Peter Clausen

Quoted 11 lines · Jonathan Cameron
Quoted 10 lines · Alison Schofield
Two instances are moved to the new claim/release API:
In the first instance, the driver was using mlock followed by iio_buffer_enabled(). Replace that code with the new API to guarantee the device stays in direct mode. There is no change in driver behavior.
In the second instance, the driver was not using mlock to hold the device in direct mode, but should have been. Here we introduce the new API to guarantee direct mode. This is a change in driver behavior.
Quoted 6 lines
Review
Quoted 6 lines
Signed-off-by Alison Schofield <[email protected]>
---
 drivers/staging/iio/adc/ad7606_core.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/iio/adc/ad7606_core.c
Quoted 20 lines
Quoted 20 lines
index 6dbc811..f914b8d 100644
drivers/staging/iio/adc/ad7606_core.c +6 -6
@@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
-		if (iio_buffer_enabled(indio_dev))
-			ret = -EBUSY;
-		else
-			ret = ad7606_scan_direct(indio_dev, chan->address);
-		mutex_unlock(&indio_dev->mlock);
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7606_scan_direct(indio_dev, chan->address);
+		iio_device_release_direct_mode(indio_dev);
 
Quoted 3 lines
drivers/staging/iio/adc/ad7606_core.c static irqreturn_t ad7606_interrupt(int irq, void
Quoted 3 lines
drivers/staging/iio/adc/ad7606_core.c +0 -0
 		if (ret < 0)
 			return ret;
@@ -411,8 +411,9 @@ static irqreturn_t ad7606_interrupt(int irq, void
*dev_id)
Quoted 11 lines
drivers/staging/iio/adc/ad7606_core.c static irqreturn_t ad7606_interrupt(int irq, void *dev_id) · line 416
Quoted 7 lines
drivers/staging/iio/adc/ad7606_core.c +2 -1
 	struct iio_dev *indio_dev = dev_id;
 	struct ad7606_state *st = iio_priv(indio_dev);
 
-	if (iio_buffer_enabled(indio_dev)) {
+	if (!iio_device_claim_direct_mode(indio_dev))  {
 		schedule_work(&st->poll_work);
+		iio_device_release_direct_mode(indio_dev);
Unfortunately this won't work. That interrupt is still in traditional non threaded form. This will take a mutex in a top half interrupt handler where a sleep cannot occur.
Quoted 2 lines
I'm just wondering how expensive it would be to fix this by moving that over to a threaded handler. In the poll_work case (buffer) it would be cleaner
to do
Quoted 18 lines
so. I'm really confused what the intended interrupt handler is in here. I 'think' the sequence is:
Trigger fires the convst pin whether in top half or the bottom half of a threaded interrupt, but not both - I guess this works, if it is rather 'unusual'.
We then get a interrupt to indicate that it has finished conversion and that filters through to actually fill the buffer via a traditional top half / bottom half interrupt handler.
So if we were to convert that to a threaded interrupt (with no top half / non threaded part), we could drop the schedule_work and just call ad7606_poll_bh_to_ring from the thread handler.
In the direct read case I doubt we care about the delay in dropping to a thread prior to signalling the data is ready.

Quoted 7 lines

Quoted 1 line
Can't think why this driver is still in staging :)
Yeah, we should leave this driver out from the conversion for now. The whole convst pin handling need a major rework. It shouldn't really be in the driver and usually you wouldn't want to use to use a GPIO and software timer since that gives you way to much jitter for good results. You'd probably use something like a PWM.

Agreed, you would normally move this into hardware, but we'd still need to allow for the possibility of it being software triggered - even if ugly.

Anyhow, lets park this for now.

Quoted 11 lines

Quoted 4 lines
Lars, any interest from Analog in getting this one cleaned up? Also do you have any test hardware, if we mess around with this interrupt handling?
I have the hardware somewhere in some storage bay, but just converting this over to threaded interrupt handling is not really a solution. So, if you want to get rid of the iio_buffer_enabled() in the interrupt handler a simple solution is to register preenable and postdisable callbacks where you set a flag in the driver struct to indicate that it is in buffered mode or not.

Alison Schofield

author

2016-04-06 04:11 UTC #5 lore raw

PATCH v2 1/1 staging: iio: ad7606: use iio_device_{claim|release}_direct_mode()

In reply to Alison Schofield

Replace the code that guarantees the device stays in direct mode with iio_device_{claim|release}_direct_mode() which does same.

Signed-off-by Alison Schofield <[email protected]>

---
Changed in v2:
- removed improper application of claim/release from intr handler
- updated changelog

 drivers/staging/iio/adc/ad7606_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/staging/iio/adc/ad7606_core.c +6 -6

index 6dbc811..f79ee61 100644
@@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
-		if (iio_buffer_enabled(indio_dev))
-			ret = -EBUSY;
-		else
-			ret = ad7606_scan_direct(indio_dev, chan->address);
-		mutex_unlock(&indio_dev->mlock);
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7606_scan_direct(indio_dev, chan->address);
+		iio_device_release_direct_mode(indio_dev);
 
 		if (ret < 0)
 			return ret;

Signature · 1 line

2.1.4

Lars-Peter Clausen

Acked-by on v2 1/1

2016-04-06 21:06 UTC #6 lore raw

Quoted 4 lines · Alison Schofield

Replace the code that guarantees the device stays in direct mode with iio_device_{claim|release}_direct_mode() which does same.
Signed-off-by Alison Schofield <[email protected]>

Looks good, thanks.

Acked-by Lars-Peter Clausen <[email protected]>

Quoted 32 lines

---
Changed in v2:
- removed improper application of claim/release from intr handler
- updated changelog

 drivers/staging/iio/adc/ad7606_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/staging/iio/adc/ad7606_core.c +6 -6

index 6dbc811..f79ee61 100644
@@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
-		if (iio_buffer_enabled(indio_dev))
-			ret = -EBUSY;
-		else
-			ret = ad7606_scan_direct(indio_dev, chan->address);
-		mutex_unlock(&indio_dev->mlock);
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7606_scan_direct(indio_dev, chan->address);
+		iio_device_release_direct_mode(indio_dev);
 
 		if (ret < 0)
 			return ret;

Jonathan Cameron

maintainer Applied

2016-04-10 13:59 UTC #7 lore raw

Quoted 10 lines · Lars-Peter Clausen

Quoted 4 lines · Alison Schofield
Replace the code that guarantees the device stays in direct mode with iio_device_{claim|release}_direct_mode() which does same.
Signed-off-by Alison Schofield <[email protected]>
Looks good, thanks.
Acked-by Lars-Peter Clausen <[email protected]>

Applied to the togreg branch of iio.git - thanks.

Jonathan

Quoted 33 lines

Quoted 32 lines

---
Changed in v2:
- removed improper application of claim/release from intr handler
- updated changelog

 drivers/staging/iio/adc/ad7606_core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

drivers/staging/iio/adc/ad7606_core.c +6 -6

index 6dbc811..f79ee61 100644
@@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
-		mutex_lock(&indio_dev->mlock);
-		if (iio_buffer_enabled(indio_dev))
-			ret = -EBUSY;
-		else
-			ret = ad7606_scan_direct(indio_dev, chan->address);
-		mutex_unlock(&indio_dev->mlock);
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		ret = ad7606_scan_direct(indio_dev, chan->address);
+		iio_device_release_direct_mode(indio_dev);
 
 		if (ret < 0)
 			return ret;