staging: iio: adc: ad7280a: use devm_* APIs

devm_* APIs are device managed and make code simpler.

Signed-off-by Slawomir Stepien <[email protected]>

---
Since v2:
* iio_device_register -> devm_iio_device_register

Since v1:
* request_threaded_irq -> devm_request_threaded_irq

---
 drivers/staging/iio/adc/ad7280a.c | 61 +++++++++++--------------------
 1 file changed, 22 insertions(+), 39 deletions(-)

drivers/staging/iio/adc/ad7280a.c +7 -7

index b736275c10f5..770e016c325b 100644
@@ -8,7 +8,6 @@
 
 #include <linux/device.h>
 #include <linux/kernel.h>
-#include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/spi/spi.h>
 #include <linux/err.h>

@@ -492,8 +491,8 @@ static int ad7280_channel_init(struct ad7280_state *st)
 {
 	int dev, ch, cnt;
 
-	st->channels = kcalloc((st->slave_num + 1) * 12 + 2,
-			       sizeof(*st->channels), GFP_KERNEL);
+	st->channels = devm_kcalloc(&st->spi->dev, (st->slave_num + 1) * 12 + 2,
+				    sizeof(*st->channels), GFP_KERNEL);
 	if (!st->channels)
 		return -ENOMEM;
 

@@ -553,9 +552,9 @@ static int ad7280_attr_init(struct ad7280_state *st)
 {
 	int dev, ch, cnt;
 
-	st->iio_attr = kcalloc(2, sizeof(*st->iio_attr) *
-			       (st->slave_num + 1) * AD7280A_CELLS_PER_DEV,
-			       GFP_KERNEL);
+	st->iio_attr = devm_kcalloc(&st->spi->dev, 2, sizeof(*st->iio_attr) *
+				    (st->slave_num + 1) * AD7280A_CELLS_PER_DEV,
+				    GFP_KERNEL);
 	if (!st->iio_attr)
 		return -ENOMEM;
 

@@ -692,7 +691,8 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 	unsigned int *channels;
 	int i, ret;
 
-	channels = kcalloc(st->scan_cnt, sizeof(*channels), GFP_KERNEL);
+	channels = devm_kcalloc(&st->spi->dev, st->scan_cnt, sizeof(*channels),
+				GFP_KERNEL);
 	if (!channels)
 		return IRQ_HANDLED;
 

drivers/staging/iio/adc/ad7280a.c +14 -24

 	return IRQ_HANDLED;
 }

@@ -909,48 +909,38 @@ static int ad7280_probe(struct spi_device *spi)
 
 	ret = ad7280_attr_init(st);
 	if (ret < 0)
-		goto error_free_channels;
+		return ret;
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
 	if (ret)
-		goto error_free_attr;
+		return ret;
 
 	if (spi->irq > 0) {
 		ret = ad7280_write(st, AD7280A_DEVADDR_MASTER,
 				   AD7280A_ALERT, 1,
 				   AD7280A_ALERT_RELAY_SIG_CHAIN_DOWN);
 		if (ret)
-			goto error_unregister;
+			return ret;
 
 		ret = ad7280_write(st, ad7280a_devaddr(st->slave_num),
 				   AD7280A_ALERT, 0,
 				   AD7280A_ALERT_GEN_STATIC_HIGH |
 				   (pdata->chain_last_alert_ignore & 0xF));
 		if (ret)
-			goto error_unregister;
-
-		ret = request_threaded_irq(spi->irq,
-					   NULL,
-					   ad7280_event_handler,
-					   IRQF_TRIGGER_FALLING |
-					   IRQF_ONESHOT,
-					   indio_dev->name,
-					   indio_dev);
+			return ret;
+
+		ret = devm_request_threaded_irq(&spi->dev, spi->irq,
+						NULL,
+						ad7280_event_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						indio_dev->name,
+						indio_dev);
 		if (ret)
-			goto error_unregister;
+			return ret;
 	}
 
 	return 0;
-error_unregister:
-	iio_device_unregister(indio_dev);
-
-error_free_attr:
-	kfree(st->iio_attr);
-
-error_free_channels:
-	kfree(st->channels);
-
-	return ret;
 }
 
 static int ad7280_remove(struct spi_device *spi)

drivers/staging/iio/adc/ad7280a.c +0 -1

-
 	return 0;
 }
 

Quoted 3 lines · On Fri, 19 Oct 2018 20:20:13 +0200 Slawomir Stepien <[email protected]> wrote:

devm_* APIs are device managed and make code simpler.

Signed-off-by Slawomir Stepien <[email protected]>

Hi Slawomir,

There are some complexities in using the managed allocators, almost always around possible race conditions. See inline.

Quoted 13 lines

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

@@ -692,7 +691,8 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 	unsigned int *channels;
 	int i, ret;
 
-	channels = kcalloc(st->scan_cnt, sizeof(*channels), GFP_KERNEL);
+	channels = devm_kcalloc(&st->spi->dev, st->scan_cnt, sizeof(*channels),
+				GFP_KERNEL);
 	if (!channels)
 		return IRQ_HANDLED;
 

@@ -744,7 +744,7 @@ static irqreturn_t ad7280_event_handler(int irq, void *private)
 	}
 

drivers/staging/iio/adc/ad7280a.c static irqreturn_t ad7280_event_handler(int irq, void *private) · line 747

Quoted 3 lines

drivers/staging/iio/adc/ad7280a.c +1 -1

 out:
-	kfree(channels);
+	devm_kfree(&st->spi->dev, channels);

Now this I really don't want to see.
Using the managed framework is far from free. Please don't do it when the normal path is to free the buffer like this...

drivers/staging/iio/adc/ad7280a.c static int ad7280_remove(struct spi_device *spi) · line 952

Quoted 13 lines

drivers/staging/iio/adc/ad7280a.c +0 -4

 	return IRQ_HANDLED;
 }
 static int ad7280_remove(struct spi_device *spi)

@@ -958,16 +948,9 @@ static int ad7280_remove(struct spi_device *spi)
 	struct iio_dev *indio_dev = spi_get_drvdata(spi);
 	struct ad7280_state *st = iio_priv(indio_dev);
 
-	if (spi->irq > 0)
-		free_irq(spi->irq, indio_dev);
-	iio_device_unregister(indio_dev);
-
 	ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
 		     AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);

So here, you need to think very carefully about what the various steps are doing. By moving to devm_iio_device_unregister what difference has it made to the sequence of calls in remove?

The upshot is you just turned the device off before removing the interfaces which would allow userspace / kernel consumers to access the device. A classic race condition that 'might' open up opportunities for problems.

Often the reality is that these sorts of races have very minimal impact, but they do break the cardinal rule that code should be obviously right (if possible). Hence you can't do this sort of conversion so simply. You can consider using the devm_add_action approach to ensure the tear down is in the right order though...

Quoted 11 lines · Jonathan Cameron

On Fri, 19 Oct 2018 20:20:13 +0200
Slawomir Stepien <[email protected]> wrote:
=20

Quoted 3 lines

devm_* APIs are device managed and make code simpler. =20

Signed-off-by Slawomir Stepien <[email protected]> =20

=20
Hi Slawomir,
=20
There are some complexities in using the managed allocators, almost always around possible race conditions. See inline. =20

=20
Thank you so much for pointing the problems!
=20

Quoted 1 line

Review static irqreturn_t ad7280_event_handler(int irq, =

Quoted 1 line

(quoted diff) +0 -0

@@ -692,7 +691,8 @@ static irqreturn_t ad7280_event_handler(int irq, =

Quoted 5 lines

Review

Quoted 5 lines

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

 	unsigned int *channels;
 	int i, ret;

=20

(quoted diff) +1 -1

-	channels =3D kcalloc(st->scan_cnt, sizeof(*channels), GFP_KERNEL);
+	channels =3D devm_kcalloc(&st->spi->dev, st->scan_cnt, sizeof(*chan=

Quoted 5 lines

Review static irqreturn_t ad7280_event_handler(int irq, =

Quoted 5 lines

drivers/staging/iio/adc/ad7280a.c +1 -0

+				GFP_KERNEL);
 	if (!channels)
 		return IRQ_HANDLED;

=20

(quoted diff) +0 -0

@@ -744,7 +744,7 @@ static irqreturn_t ad7280_event_handler(int irq, =

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +0 -0

 	}

=20
out:

(quoted diff) +1 -1

-	kfree(channels);
+	devm_kfree(&st->spi->dev, channels); =20

=20
Now this I really don't want to see.
Using the managed framework is far from free. Please don't do it when t=

Quoted 1 line

normal path is to free the buffer like this... =20

=20
OK
=20

Quoted 19 lines

drivers/staging/iio/adc/ad7280a.c static int ad7280_remove(struct spi_device *spi)

Quoted 13 lines

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

 	return IRQ_HANDLED;
 }
 static int ad7280_remove(struct spi_device *spi)

@@ -958,16 +948,9 @@ static int ad7280_remove(struct spi_device *spi)
 	struct iio_dev *indio_dev =3D spi_get_drvdata(spi);
 	struct ad7280_state *st =3D iio_priv(indio_dev);

=20

drivers/staging/iio/adc/ad7280a.c +0 -4

-	if (spi->irq > 0)
-		free_irq(spi->irq, indio_dev);
-	iio_device_unregister(indio_dev);
-
 	ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
 		     AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb); =20

So here, you need to think very carefully about what the various steps are doing. By moving to devm_iio_device_unregister what difference has it made to the sequence of calls in remove? =20
The upshot is you just turned the device off before removing the interfaces which would allow userspace / kernel consumers to

Quoted 8 lines

access the device. A classic race condition that 'might' open up opportunities for problems.
=20
Often the reality is that these sorts of races have very minimal impact, but they do break the cardinal rule that code should be obviously right (if possible). Hence you can't do this sort of conversion so simply. You can consider using the devm_add_action approach to ensure the tear down is in the right order though... =20

=20
Yes I understand the problem here. I have some questions regarding devm_add_action that might solve the problem here:
=20
1. My understanding is that the action has to be added on the devres list=

the devm_iio_device_register call, so during unwinding the action will be=

after the call to devm_iio_device_unreg. Other order will be still not co=

Am I thinking correctly here?

=20
Please note that doing the action from probe is changing the current beha=

of the driver - we will put the device into power-down software state als=

probe() (if irq setup fails).

=20
2. devm_iio_device_unregister from what I see could be used here in place=

iio_device_unregister. Maybe that is the best way to go? =20

Quoted 26 lines

Quoted 20 lines

drivers/staging/iio/adc/ad7280a.c static int ad7280_remove(struct spi_device *spi) · line 952

Quoted 11 lines

static int ad7280_remove(struct spi_device *spi)

drivers/staging/iio/adc/ad7280a.c +0 -4

@@ -958,16 +948,9 @@ static int ad7280_remove(struct spi_device *spi)
 	struct iio_dev *indio_dev = spi_get_drvdata(spi);
 	struct ad7280_state *st = iio_priv(indio_dev);
 
-	if (spi->irq > 0)
-		free_irq(spi->irq, indio_dev);
-	iio_device_unregister(indio_dev);
-
 	ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
 		     AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);  

So here, you need to think very carefully about what the various steps are doing. By moving to devm_iio_device_unregister what difference has it made to the sequence of calls in remove?

The upshot is you just turned the device off before removing the interfaces which would allow userspace / kernel consumers to access the device. A classic race condition that 'might' open up opportunities for problems.

Quoted 5 lines

Often the reality is that these sorts of races have very minimal impact, but they do break the cardinal rule that code should be obviously right (if possible). Hence you can't do this sort of conversion so simply. You can consider using the devm_add_action approach to ensure the tear down is in the right order though...

Quoted 7 lines

Yes I understand the problem here. I have some questions regarding devm_add_action that might solve the problem here:

1. My understanding is that the action has to be added on the devres list before the devm_iio_device_register call, so during unwinding the action will be called after the call to devm_iio_device_unreg. Other order will be still not correct. Am I thinking correctly here?

Yes. That's correct.

Quoted 1 line

Quoted 3 lines

Please note that doing the action from probe is changing the current behaviour of the driver - we will put the device into power-down software state also from probe() (if irq setup fails).

True. In the case an irq being specified but not probing successfully we will fail the probe and put the device into a power down state. However, to my mind that's the right thing to do anyway. I can't see why we would want the device powered up having decided to abandon the attempt to load a driver for it? (am I missing something?)

The more 'interesting' question is why we are registering the interrupts after iio_device_register in the first place. We have exposed our userspace interfaces, but not yet an interrupt that I assume has something to do with them?

iio_device_register should almost always be the last thing run in probe.

Quoted 3 lines

2. devm_iio_device_unregister from what I see could be used here in place of iio_device_unregister. Maybe that is the best way to go?

Definitely not this one. The only rare case for manually using the counter parts to the devm_ setup functions is to replace some data or configuration rather to manually unwind the steps for some error path.

Hi
=20

Quoted 2 lines · Jonathan Cameron

Quoted 2 lines

Quoted 2 lines

Review static int ad7280_remove(struct spi_device *=

Quoted 2 lines

static int ad7280_remove(struct spi_device *spi)

(quoted diff) +0 -0

@@ -958,16 +948,9 @@ static int ad7280_remove(struct spi_device *=

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

drivers/staging/iio/adc/ad7280a.c static int ad7280_remove(struct spi_device *spi)

Quoted 9 lines

(quoted diff) +0 -0

 	struct iio_dev *indio_dev =3D spi_get_drvdata(spi);
 	struct ad7280_state *st =3D iio_priv(indio_dev);

=20

drivers/staging/iio/adc/ad7280a.c +0 -4

-	if (spi->irq > 0)
-		free_irq(spi->irq, indio_dev);
-	iio_device_unregister(indio_dev);
-
 	ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
 		     AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);   =20

So here, you need to think very carefully about what the various steps are doing. By moving to devm_iio_device_unregister what difference has it made to the sequence of calls in remove? =20
The upshot is you just turned the device off before removing the interfaces which would allow userspace / kernel consumers to

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

access the device. A classic race condition that 'might' open up opportunities for problems.
=20
Often the reality is that these sorts of races have very minimal impact, but they do break the cardinal rule that code should be obviously right (if possible). Hence you can't do this sort of conversion so simply. You can consider using the devm_add_action
approach to ensure the tear down is in the right order though... =

Quoted 5 lines

Quoted 5 lines

=20
Yes I understand the problem here. I have some questions regarding devm_add_action that might solve the problem here:
=20
1. My understanding is that the action has to be added on the devres =

Quoted 1 line

Quoted 1 line

the devm_iio_device_register call, so during unwinding the action wil=

Quoted 1 line

Quoted 1 line

after the call to devm_iio_device_unreg. Other order will be still no=

Quoted 4 lines

Quoted 1 line

Am I thinking correctly here? =20

Yes. That's correct. =20

Quoted 2 lines

=20
Please note that doing the action from probe is changing the current =

Quoted 1 line

Quoted 1 line

of the driver - we will put the device into power-down software state=

Quoted 2 lines

Quoted 1 line

probe() (if irq setup fails). =20

True. In the case an irq being specified but not probing successfully w=

Quoted 1 line

fail the probe and put the device into a power down state. However, to=

Quoted 2 lines

mind that's the right thing to do anyway. I can't see why we would want the device powered up having decided to abandon the attempt to load a d=

Quoted 1 line

for it? (am I missing something?) =20

=20
I'll send a patch with this action.
=20

Quoted 2 lines

The more 'interesting' question is why we are registering the interrupts after iio_device_register in the first place. We have exposed our user=

Quoted 1 line

interfaces, but not yet an interrupt that I assume has something to do =

Quoted 2 lines

=20
iio_device_register should almost always be the last thing run in probe=

=20
I've looked at the data sheet and the code and concluded that the order i=

Why? The irq handler can only fire after conversion is completed. The con=

can start only in two ways:
=20
(1) falling edge of CNVST input (default) which we don't control

(2) rising edge of CS, which we control
=20
Since we only using the 2nd option, then it is wise to allow users to hav=

connected and going down, before any readout of the values using this dri=

(this will change the CS). This way we will not loose any alert about UV =

=20
In case we have irq handler ready before iio_device_register, then I assu=

we might loose alert change event. Maybe it's hard (timing), but I think =

possible.
=20
What do you think?
=20

Quoted 1 line

Quoted 1 line

2. devm_iio_device_unregister from what I see could be used here in p=

Quoted 5 lines

Quoted 2 lines

iio_device_unregister. Maybe that is the best way to go?
=20

Definitely not this one. The only rare case for manually using the counter parts to the devm_ setup functions is to replace some data or configuration rather to manually unwind the steps for some error path. =

=20
OK, got it!
=20

Quoted 57 lines · On Mon, 29 Oct 2018 17:47:24 +0100 Slawomir Stepien <[email protected]> wrote:

Quoted 35 lines · Jonathan Cameron

Quoted 26 lines

Quoted 20 lines

drivers/staging/iio/adc/ad7280a.c static int ad7280_remove(struct spi_device *spi) · line 952

Quoted 11 lines

static int ad7280_remove(struct spi_device *spi)

drivers/staging/iio/adc/ad7280a.c +0 -4

@@ -958,16 +948,9 @@ static int ad7280_remove(struct spi_device *spi)
 	struct iio_dev *indio_dev = spi_get_drvdata(spi);
 	struct ad7280_state *st = iio_priv(indio_dev);
 
-	if (spi->irq > 0)
-		free_irq(spi->irq, indio_dev);
-	iio_device_unregister(indio_dev);
-
 	ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
 		     AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb);    

So here, you need to think very carefully about what the various steps are doing. By moving to devm_iio_device_unregister what difference has it made to the sequence of calls in remove?

The upshot is you just turned the device off before removing the interfaces which would allow userspace / kernel consumers to access the device. A classic race condition that 'might' open up opportunities for problems.

Quoted 5 lines

Often the reality is that these sorts of races have very minimal impact, but they do break the cardinal rule that code should be obviously right (if possible). Hence you can't do this sort of conversion so simply. You can consider using the devm_add_action approach to ensure the tear down is in the right order though...

Quoted 7 lines

Yes I understand the problem here. I have some questions regarding devm_add_action that might solve the problem here:

1. My understanding is that the action has to be added on the devres list before the devm_iio_device_register call, so during unwinding the action will be called after the call to devm_iio_device_unreg. Other order will be still not correct. Am I thinking correctly here?

Yes. That's correct.

Quoted 1 line

Quoted 8 lines

Quoted 3 lines

Please note that doing the action from probe is changing the current behaviour of the driver - we will put the device into power-down software state also from probe() (if irq setup fails).

True. In the case an irq being specified but not probing successfully we will fail the probe and put the device into a power down state. However, to my mind that's the right thing to do anyway. I can't see why we would want the device powered up having decided to abandon the attempt to load a driver for it? (am I missing something?)

I'll send a patch with this action.

Quoted 5 lines

The more 'interesting' question is why we are registering the interrupts after iio_device_register in the first place. We have exposed our userspace interfaces, but not yet an interrupt that I assume has something to do with them?

iio_device_register should almost always be the last thing run in probe.

I've looked at the data sheet and the code and concluded that the order is OK. Why? The irq handler can only fire after conversion is completed. The conversion can start only in two ways:

Quoted 1 line

(1) falling edge of CNVST input (default) which we don't control

That's a potential problem. We shouldn't start by default in a mode where interrupts can occur before we are potentially ready for them. They should only be enabled by a specific request from userspace.
A quick at the datasheet suggests this is easily done by writing 0 to the alert register and only enabling it on demand.

Quoted 5 lines

(2) rising edge of CS, which we control

Since we only using the 2nd option, then it is wise to allow users to have CNVST connected and going down, before any readout of the values using this driver (this will change the CS). This way we will not loose any alert about UV or OV.

I agree we are looking at theoretical race, but as I mentioned it's about obviously correct (and general correct ordering) rather than anthing else. In theory we can have very long delay between exposing the interfaces and setting up the interrupt. So it's possible to hit case 2 before we get the interrupt set up.

Quoted 5 lines · Jonathan Cameron

On Mon, 29 Oct 2018 17:47:24 +0100
Slawomir Stepien <[email protected]> wrote: =20

Quoted 3 lines

On pa=C5=BA 28, 2018 12:16, Jonathan Cameron wrote: =20

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review static int ad7280_remove(struct spi_devi=

Quoted 2 lines

static int ad7280_remove(struct spi_device *spi)

(quoted diff) +0 -0

@@ -958,16 +948,9 @@ static int ad7280_remove(struct spi_devi=

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review

Quoted 8 lines

(quoted diff) +0 -0

 	struct iio_dev *indio_dev =3D spi_get_drvdata(spi);
 	struct ad7280_state *st =3D iio_priv(indio_dev);

=20

(quoted diff) +0 -4

-	if (spi->irq > 0)
-		free_irq(spi->irq, indio_dev);
-	iio_device_unregister(indio_dev);
-
 	ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB=

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 1 line

AD7280A_CTRL_HB_PWRDN_SW | st->ctrl_hb); =20

So here, you need to think very carefully about what the various steps are doing. By moving to devm_iio_device_unregister what difference has it made to the sequence of calls in remove? =20
The upshot is you just turned the device off before removing the interfaces which would allow userspace / kernel consumers to access the device. A classic race condition that 'might' open up opportunities for problems.
=20
Often the reality is that these sorts of races have very minimal impact, but they do break the cardinal rule that code should be obviously right (if possible). Hence you can't do this sort of conversion so simply. You can consider using the devm_add_a=

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

approach to ensure the tear down is in the right order though..=

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

=20
Yes I understand the problem here. I have some questions regarding devm_add_action that might solve the problem here:
=20
1. My understanding is that the action has to be added on the dev=

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

the devm_iio_device_register call, so during unwinding the action=

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

after the call to devm_iio_device_unreg. Other order will be stil=

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

Am I thinking correctly here? =20

Yes. That's correct. =20

Quoted 2 lines

=20
Please note that doing the action from probe is changing the curr=

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

of the driver - we will put the device into power-down software s=

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

probe() (if irq setup fails). =20

True. In the case an irq being specified but not probing successful=

Quoted 1 line

Quoted 1 line

Quoted 1 line

fail the probe and put the device into a power down state. However=

Quoted 1 line

Quoted 1 line

Quoted 1 line

mind that's the right thing to do anyway. I can't see why we would=

Quoted 1 line

Quoted 1 line

Quoted 1 line

the device powered up having decided to abandon the attempt to load=

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

for it? (am I missing something?) =20

=20
I'll send a patch with this action.
=20

Quoted 1 line

The more 'interesting' question is why we are registering the inter=

Quoted 1 line

Quoted 1 line

Quoted 1 line

after iio_device_register in the first place. We have exposed our =

Quoted 1 line

Quoted 1 line

Quoted 1 line

interfaces, but not yet an interrupt that I assume has something to=

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

=20
iio_device_register should almost always be the last thing run in p=

Quoted 2 lines

Quoted 2 lines

=20
I've looked at the data sheet and the code and concluded that the ord=

Quoted 1 line

Quoted 1 line

Why? The irq handler can only fire after conversion is completed. The=

Quoted 4 lines

Quoted 3 lines

can start only in two ways:
=20
(1) falling edge of CNVST input (default) which we don't control =20

That's a potential problem. We shouldn't start by default in a mode wh=

Quoted 1 line

interrupts can occur before we are potentially ready for them. They sh=

Quoted 2 lines

only be enabled by a specific request from userspace.
A quick at the datasheet suggests this is easily done by writing 0 to t=

Quoted 1 line

alert register and only enabling it on demand. =20

=20
Do I understand this correctly: the interrupts should be enabled on user request, for example by writing 1 to additional iio_dev_attr, and disable=

writing 0 to that file?

Should that also be like this when using device tree node with interupt properties (e.g. interrupts) for that device (also working on that)?

=20

Quoted 3 lines

Quoted 3 lines

(2) rising edge of CS, which we control
=20
Since we only using the 2nd option, then it is wise to allow users to=

Quoted 1 line

Quoted 1 line

connected and going down, before any readout of the values using this=

Quoted 1 line

Quoted 1 line

(this will change the CS). This way we will not loose any alert about=

Quoted 1 line

I agree we are looking at theoretical race, but as I mentioned it's abo=

Quoted 1 line

obviously correct (and general correct ordering) rather than anthing el=

Quoted 1 line

In theory we can have very long delay between exposing the interfaces a=

Quoted 2 lines

setting up the interrupt. So it's possible to hit case 2 before we get the interrupt set up. =20

=20