[PATCH 1/3] iio: st_sensors: Init trigger before irq request

[PATCH 1/3] iio: st_sensors: Init trigger before irq request

[Crestez Dan Leonard]

This fixes a possible race where an interrupt arrives before complete
initialization and crashes because iio_trigger_get_drvdata returns NULL.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Giuseppe Barba <giuseppe.barba@st.com>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
I ran into this while breaking the driver. But since the interrupt line can be
shared the handler should always be able to accept and ignore a call.

 drivers/iio/common/st_sensors/st_sensors_trigger.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index 98bd5b3..7c2e6ab 100644
--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
@@ -89,6 +89,10 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
 		return -ENOMEM;
 	}
 
+	iio_trigger_set_drvdata(sdata->trig, indio_dev);
+	sdata->trig->ops = trigger_ops;
+	sdata->trig->dev.parent = sdata->dev;
+
 	irq = sdata->get_irq_data_ready(indio_dev);
 	irq_trig = irqd_get_trigger_type(irq_get_irq_data(irq));
 	/*
@@ -150,10 +154,6 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
 		goto iio_trigger_free;
 	}
 
-	iio_trigger_set_drvdata(sdata->trig, indio_dev);
-	sdata->trig->ops = trigger_ops;
-	sdata->trig->dev.parent = sdata->dev;
-
 	err = iio_trigger_register(sdata->trig);
 	if (err < 0) {
 		dev_err(&indio_dev->dev, "failed to register iio trigger.\n");
-- 
2.5.5

[PATCH 2/3] iio: st_sensors: Disable DRDY at init time

[Crestez Dan Leonard]

This fixes odd behavior after reboot.

The fact that we set the device to powerdown mode is not sufficient to
prevent DRDY being active because we might still have an unread sample.

Even if powerdown was sufficient keeping DRDY disabled while trigger is
not active is a good idea.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Giuseppe Barba <giuseppe.barba@st.com>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/common/st_sensors/st_sensors_core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 928ee68..9e59c90 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -363,6 +363,11 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
 	if (err < 0)
 		return err;
 
+	/* Disable DRDY, this might be still be enabled after reboot. */
+	err = st_sensors_set_dataready_irq(indio_dev, false);
+	if (err < 0)
+		return err;
+
 	if (sdata->current_fullscale) {
 		err = st_sensors_set_fullscale(indio_dev,
 						sdata->current_fullscale->num);
-- 
2.5.5

Re: [PATCH 2/3] iio: st_sensors: Disable DRDY at init time

[Jonathan Cameron]

On 13/05/16 19:43, Crestez Dan Leonard wrote:
> This fixes odd behavior after reboot.
> 
> The fact that we set the device to powerdown mode is not sufficient to
> prevent DRDY being active because we might still have an unread sample.
> 
> Even if powerdown was sufficient keeping DRDY disabled while trigger is
> not active is a good idea.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Again, this one seems obviously correct to me, but would like Denis Ack.

Jonathan
> ---
>  drivers/iio/common/st_sensors/st_sensors_core.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 928ee68..9e59c90 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -363,6 +363,11 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
>  	if (err < 0)
>  		return err;
>  
> +	/* Disable DRDY, this might be still be enabled after reboot. */
> +	err = st_sensors_set_dataready_irq(indio_dev, false);
> +	if (err < 0)
> +		return err;
> +
>  	if (sdata->current_fullscale) {
>  		err = st_sensors_set_fullscale(indio_dev,
>  						sdata->current_fullscale->num);
> 

Re: [PATCH 2/3] iio: st_sensors: Disable DRDY at init time

[Linus Walleij]

On Fri, May 13, 2016 at 8:43 PM, Crestez Dan Leonard
<leonard.crestez@intel.com> wrote:

> This fixes odd behavior after reboot.
>
> The fact that we set the device to powerdown mode is not sufficient to
> prevent DRDY being active because we might still have an unread sample.
>
> Even if powerdown was sufficient keeping DRDY disabled while trigger is
> not active is a good idea.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 2/3] iio: st_sensors: Disable DRDY at init time

[Jonathan Cameron]

On 24/05/16 12:28, Linus Walleij wrote:
> On Fri, May 13, 2016 at 8:43 PM, Crestez Dan Leonard
> <leonard.crestez@intel.com> wrote:
> 
>> This fixes odd behavior after reboot.
>>
>> The fact that we set the device to powerdown mode is not sufficient to
>> prevent DRDY being active because we might still have an unread sample.
>>
>> Even if powerdown was sufficient keeping DRDY disabled while trigger is
>> not active is a good idea.
>>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Giuseppe Barba <giuseppe.barba@st.com>
>> Cc: Denis Ciocca <denis.ciocca@st.com>
>> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Applied to the fixes-togreg-post-rc1 branch of iio.git.

Note I'm taking these without Denis ack as he is snowed under right now and
this set aren't terribly 'exciting' (hopefully)

Jonathan
> 
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 3/3] iio: st_sensors: Use level interrupts

[Crestez Dan Leonard]

As far as I can tell DRDY for ST sensors behaves as a level rather than
edge interrupt. Registering for IRQF_TRIGGER_RISING instead of
IRQF_TRIGGER_HIGH mostly works except when the sampling frequency is
high enough that new samples come before the new ones are read
completely. In that case the interrupt line remains high, no more rising
edges occur and the iio buffer stalls.

Configuring the interrupt as IRQF_TRIGGER_HIGH makes it work as
expected. This patch makes it so that st_sensors_trigger interrupt
request code doesn't mangle the request flags into IRQF_TRIGGER_RISING.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Giuseppe Barba <giuseppe.barba@st.com>
Cc: Denis Ciocca <denis.ciocca@st.com>
Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
This is an alternative fix to this patch:
	https://www.spinics.net/lists/linux-iio/msg24722.html
[PATCH 2/2 v6] iio: st_sensors: read surplus samples in trigger.

It's not clear if all st_sensors behave this way on DRDY. If they do it might
be better to always request HIGH even if the interrupt is initially configured
for RISING.

I tested on MinnowBoard Max which uses pinctrl-baytrail for gpio irqs. I
initially tested hacking the irq_trig to IRQF_TRIGGER_HIGH with an usb adaptor
(gpio-dln2) and it didn't work but I think that's because that driver doesn't
handle persistently high irqs properly.

 drivers/iio/common/st_sensors/st_sensors_trigger.c | 26 ++++++++++++----------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
index 7c2e6ab..ffd6edb 100644
--- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
+++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
@@ -99,13 +99,16 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
 	 * If the IRQ is triggered on falling edge, we need to mark the
 	 * interrupt as active low, if the hardware supports this.
 	 */
-	if (irq_trig == IRQF_TRIGGER_FALLING) {
+	if (irq_trig == IRQF_TRIGGER_FALLING || irq_trig == IRQF_TRIGGER_LOW) {
 		if (!sdata->sensor_settings->drdy_irq.addr_ihl) {
 			dev_err(&indio_dev->dev,
-				"falling edge specified for IRQ but hardware "
-				"only support rising edge, will request "
-				"rising edge\n");
-			irq_trig = IRQF_TRIGGER_RISING;
+				"active low specified for IRQ but hardware "
+				"only support active high, will request "
+				"active high\n");
+			if (irq_trig == IRQF_TRIGGER_FALLING)
+				irq_trig = IRQF_TRIGGER_RISING;
+			else if (irq_trig == IRQF_TRIGGER_LOW)
+				irq_trig = IRQF_TRIGGER_HIGH;
 		} else {
 			/* Set up INT active low i.e. falling edge */
 			err = st_sensors_write_data_with_mask(indio_dev,
@@ -114,18 +117,17 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
 			if (err < 0)
 				goto iio_trigger_free;
 			dev_info(&indio_dev->dev,
-				 "interrupts on the falling edge\n");
+				 "interrupts are active low\n");
 		}
-	} else if (irq_trig == IRQF_TRIGGER_RISING) {
+	} else if (irq_trig == IRQF_TRIGGER_RISING || irq_trig == IRQF_TRIGGER_HIGH) {
 		dev_info(&indio_dev->dev,
-			 "interrupts on the rising edge\n");
+			 "interrupts are active high\n");
 
 	} else {
 		dev_err(&indio_dev->dev,
-		"unsupported IRQ trigger specified (%lx), only "
-			"rising and falling edges supported, enforce "
-			"rising edge\n", irq_trig);
-		irq_trig = IRQF_TRIGGER_RISING;
+			"unsupported IRQ trigger specified (%lx) "
+			"enforce level high\n", irq_trig);
+		irq_trig = IRQF_TRIGGER_HIGH;
 	}
 
 	/*
-- 
2.5.5

Re: [PATCH 3/3] iio: st_sensors: Use level interrupts

[Jonathan Cameron]

On 13/05/16 19:43, Crestez Dan Leonard wrote:
> As far as I can tell DRDY for ST sensors behaves as a level rather than
> edge interrupt. Registering for IRQF_TRIGGER_RISING instead of
> IRQF_TRIGGER_HIGH mostly works except when the sampling frequency is
> high enough that new samples come before the new ones are read
> completely. In that case the interrupt line remains high, no more rising
> edges occur and the iio buffer stalls.
> 
> Configuring the interrupt as IRQF_TRIGGER_HIGH makes it work as
> expected. This patch makes it so that st_sensors_trigger interrupt
> request code doesn't mangle the request flags into IRQF_TRIGGER_RISING.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
> This is an alternative fix to this patch:
> 	https://www.spinics.net/lists/linux-iio/msg24722.html
> [PATCH 2/2 v6] iio: st_sensors: read surplus samples in trigger.
> 
> It's not clear if all st_sensors behave this way on DRDY. If they do it might
> be better to always request HIGH even if the interrupt is initially configured
> for RISING.
> 
> I tested on MinnowBoard Max which uses pinctrl-baytrail for gpio irqs. I
> initially tested hacking the irq_trig to IRQF_TRIGGER_HIGH with an usb adaptor
> (gpio-dln2) and it didn't work but I think that's because that driver doesn't
> handle persistently high irqs properly.
Hi Leonard / Linus,

I thought my memory was failing me a little when I read Linus' patch description
where he mentioned that we were missing pulsed interrupts. 

Years ago (probably at least 8) I ran into this exact issue with the lis3l02dq and
spent sometime with a scope tracking down the cause (this was way before threaded
interrupt etc were in mainline).  Anyhow, my recollection was that we didn't always
get a gap between the pulses. Hence it was possible to cross reading the registers
with new data coming in for other channels and never see it drop.

Distinctly remember scratching my head over how to deal with this for some time.
The snag I had was that I was working with an intel stargate 2 with a pxa27x which
didn't have any level interrupt support.  Hence I ended up with a cludge that
checked the gpio itself (was easier back then as you could get the gpio from
the IRQ :) and did a bonus read with no processing to hammer it down and get out
of being stuck.

Linus' checking the data ready register and firing another trigger was the
equivalent of this (back then we didn't have the two polling methods).
You can still see my hack in staging/iio/accel/lis3l02dq_ring.c in try_reenable.
Note the tryreenable was an attempt to bring sanity checking whether we need
a repeat poll into one place and push the repeat polling into the core - which
only works if all drivers hanging off the trigger can work fine with threaded
interrupts only.

Right now I think the try_reenable stuff has been broken for a long time...
It will call iio_trigger_poll
from a thread (typically).  Obviously missed this one when we converted over to
threaded interrupts (hmm)  Need to audit if that can ever be called from interrupt
context (to avoid making things worse!)
If that was fixed then we could use it for Linus's reread but it suffers the same
issue with no limit on the number of times it can go around (so that would want
fixing as well!)

This patch doesn't make things worse as I read it - except in the case where
no interrupt type is specified...  In that case it will try to make it high
which may not be supported.  Note that (if I'd merged the lis3l02dq driver in
as I've been meaning too for a long time) this would just have broken my boards..
(can we check if the interrupt supports it and fall back to the original default
if not? - it's even more comic on my board as IIRC the interrupt is shared via
a discrete NAND gate with a tsl2561 so everything is flipped as well)

The other question is how many users have one of these hooked up to an irq that
doesn't support level interrupts (such as my pxa27x - which I fire up every few
months - or your gpio-dln2 adapter).

If so I guess we aren't making things any worse for them but the issue you had
still exists + I still can't port my lis3l02dq driver over...  I wonder if we
ultimately end up having to support both fixes (though the one I did originally
and Linus effectively reinvented is hideous :)  If level interrupts are specified
then yours is great and simple, if we edge interrupts are specified we have
to play games to fake a level interrupt. Note that way back when I asked if anyone
had any generic code for doing exactly this and got the answer 'It's too fiddly'
IIRC - and I don't think anyone has ever taken it on since...

I'm also inclined on this fix in general to send it the slow route (next merge
window) rather than rushing it through.  It's invasive and it's not as though
we are looking at a regression here.

The other fix is a totally different matter and I'll pick that up shortly.

So in conclusion, in the first instance I'm keener on your solution which I think
is the technically correct one.  However, I fear we have enough crazy hardware
types who have this wired up to an in appropriate interrupt controller, that we
need not to make things worse than they are.  We also have the potential to make
them better perhaps by including the guts of Linus' suggestion as well (when not
in level interrupt mode).

Jonathan


> 
>  drivers/iio/common/st_sensors/st_sensors_trigger.c | 26 ++++++++++++----------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> index 7c2e6ab..ffd6edb 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> @@ -99,13 +99,16 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  	 * If the IRQ is triggered on falling edge, we need to mark the
>  	 * interrupt as active low, if the hardware supports this.
>  	 */
> -	if (irq_trig == IRQF_TRIGGER_FALLING) {
> +	if (irq_trig == IRQF_TRIGGER_FALLING || irq_trig == IRQF_TRIGGER_LOW) {
>  		if (!sdata->sensor_settings->drdy_irq.addr_ihl) {
>  			dev_err(&indio_dev->dev,
> -				"falling edge specified for IRQ but hardware "
> -				"only support rising edge, will request "
> -				"rising edge\n");
> -			irq_trig = IRQF_TRIGGER_RISING;
> +				"active low specified for IRQ but hardware "
> +				"only support active high, will request "
> +				"active high\n");
> +			if (irq_trig == IRQF_TRIGGER_FALLING)
> +				irq_trig = IRQF_TRIGGER_RISING;
> +			else if (irq_trig == IRQF_TRIGGER_LOW)
> +				irq_trig = IRQF_TRIGGER_HIGH;
>  		} else {
>  			/* Set up INT active low i.e. falling edge */
>  			err = st_sensors_write_data_with_mask(indio_dev,
> @@ -114,18 +117,17 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  			if (err < 0)
>  				goto iio_trigger_free;
>  			dev_info(&indio_dev->dev,
> -				 "interrupts on the falling edge\n");
> +				 "interrupts are active low\n");
>  		}
> -	} else if (irq_trig == IRQF_TRIGGER_RISING) {
> +	} else if (irq_trig == IRQF_TRIGGER_RISING || irq_trig == IRQF_TRIGGER_HIGH) {
>  		dev_info(&indio_dev->dev,
> -			 "interrupts on the rising edge\n");
> +			 "interrupts are active high\n");
>  
>  	} else {
>  		dev_err(&indio_dev->dev,
> -		"unsupported IRQ trigger specified (%lx), only "
> -			"rising and falling edges supported, enforce "
> -			"rising edge\n", irq_trig);
> -		irq_trig = IRQF_TRIGGER_RISING;
> +			"unsupported IRQ trigger specified (%lx) "
> +			"enforce level high\n", irq_trig);
> +		irq_trig = IRQF_TRIGGER_HIGH;
>  	}
>  
>  	/*
> 

Re: [PATCH 3/3] iio: st_sensors: Use level interrupts

[Linus Walleij]

On Fri, May 13, 2016 at 8:43 PM, Crestez Dan Leonard
<leonard.crestez@intel.com> wrote:

> As far as I can tell DRDY for ST sensors behaves as a level rather than
> edge interrupt. Registering for IRQF_TRIGGER_RISING instead of
> IRQF_TRIGGER_HIGH mostly works except when the sampling frequency is
> high enough that new samples come before the new ones are read
> completely. In that case the interrupt line remains high, no more rising
> edges occur and the iio buffer stalls.
>
> Configuring the interrupt as IRQF_TRIGGER_HIGH makes it work as
> expected. This patch makes it so that st_sensors_trigger interrupt
> request code doesn't mangle the request flags into IRQF_TRIGGER_RISING.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>

The analysis is probably correct: the sensors will hold the line low
until the values have been read and no data is ready in the pipe.

This is delicate when the sensors are used with GPIO controllers
as interrupt line handlers, because some of them (like mine
on Ux500) do not support level interrupts :(

That means that requesting the interrupt will fail.

What we need to do is try to request a level IRQ, and if that
fails, try to request an edge IRQ or we will regress existing platforms.

And then, as Jonathan says, use the approach in the other
approach for those systems that only do edge IRQs.

For the level IRQ, IRQF_ONESHOT must be used, or the
irq handler will lock up I suspect.

Keeping my loop in the irq thread from the other patch will likely
also get the events closer in time I suspect? But for a level IRQ
it is not strictly needed.

If we can agree on a version of this patch that tries first
level, then edge IRQ, I can rebase my patch on top of it
and test.

Yours,
Linus Walleij

Re: [PATCH 3/3] iio: st_sensors: Use level interrupts

[Crestez Dan Leonard]

On 05/24/2016 02:40 PM, Linus Walleij wrote:
> On Fri, May 13, 2016 at 8:43 PM, Crestez Dan Leonard
> <leonard.crestez@intel.com> wrote:
> 
>> As far as I can tell DRDY for ST sensors behaves as a level rather than
>> edge interrupt. Registering for IRQF_TRIGGER_RISING instead of
>> IRQF_TRIGGER_HIGH mostly works except when the sampling frequency is
>> high enough that new samples come before the new ones are read
>> completely. In that case the interrupt line remains high, no more rising
>> edges occur and the iio buffer stalls.
>>
>> Configuring the interrupt as IRQF_TRIGGER_HIGH makes it work as
>> expected. This patch makes it so that st_sensors_trigger interrupt
>> request code doesn't mangle the request flags into IRQF_TRIGGER_RISING.
>>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Giuseppe Barba <giuseppe.barba@st.com>
>> Cc: Denis Ciocca <denis.ciocca@st.com>
>> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> 
> The analysis is probably correct: the sensors will hold the line low
> until the values have been read and no data is ready in the pipe.
> 
> This is delicate when the sensors are used with GPIO controllers
> as interrupt line handlers, because some of them (like mine
> on Ux500) do not support level interrupts :(

This might be completely crazy, but wouldn't it be possible to support
gpio level interrupts purely in software? Just read the GPIO state again
after the interrupt is unmasked and retrigger.

This seems preferable to implementing per-driver workarounds. Perhaps it
would even fit in some gpio-irqchip glue.

-- 
Regards,
Leonard

Re: [PATCH 3/3] iio: st_sensors: Use level interrupts

[Jonathan Cameron]

On 24/05/16 13:35, Crestez Dan Leonard wrote:
> On 05/24/2016 02:40 PM, Linus Walleij wrote:
>> On Fri, May 13, 2016 at 8:43 PM, Crestez Dan Leonard
>> <leonard.crestez@intel.com> wrote:
>>
>>> As far as I can tell DRDY for ST sensors behaves as a level rather than
>>> edge interrupt. Registering for IRQF_TRIGGER_RISING instead of
>>> IRQF_TRIGGER_HIGH mostly works except when the sampling frequency is
>>> high enough that new samples come before the new ones are read
>>> completely. In that case the interrupt line remains high, no more rising
>>> edges occur and the iio buffer stalls.
>>>
>>> Configuring the interrupt as IRQF_TRIGGER_HIGH makes it work as
>>> expected. This patch makes it so that st_sensors_trigger interrupt
>>> request code doesn't mangle the request flags into IRQF_TRIGGER_RISING.
>>>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: Giuseppe Barba <giuseppe.barba@st.com>
>>> Cc: Denis Ciocca <denis.ciocca@st.com>
>>> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
>>
>> The analysis is probably correct: the sensors will hold the line low
>> until the values have been read and no data is ready in the pipe.
>>
>> This is delicate when the sensors are used with GPIO controllers
>> as interrupt line handlers, because some of them (like mine
>> on Ux500) do not support level interrupts :(
> 
> This might be completely crazy, but wouldn't it be possible to support
> gpio level interrupts purely in software? Just read the GPIO state again
> after the interrupt is unmasked and retrigger.
> 
> This seems preferable to implementing per-driver workarounds. Perhaps it
> would even fit in some gpio-irqchip glue.
That's precisely the question I raised way back when writing the lis3l02dq
driver.  Apparently someone once had a go but it never went anywhere...

Feel free to try! It would be great if possible.

Jonathan
> 

Re: [PATCH 3/3] iio: st_sensors: Use level interrupts

[Linus Walleij]

On Sun, May 29, 2016 at 9:29 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 24/05/16 13:35, Crestez Dan Leonard wrote:

>> This might be completely crazy, but wouldn't it be possible to support
>> gpio level interrupts purely in software? Just read the GPIO state again
>> after the interrupt is unmasked and retrigger.
>>
>> This seems preferable to implementing per-driver workarounds. Perhaps it
>> would even fit in some gpio-irqchip glue.
>
> That's precisely the question I raised way back when writing the lis3l02dq
> driver.  Apparently someone once had a go but it never went anywhere...

It is maybe possible to do in some cases. What you have to do
is to augment the GPIO irqchip driver to read the line status register
after handling an IRQ, if level IRQ is desired, and if the line is still high
after a rising edge or still low after a falling edge, just call the IRQ
handler again.

This will not work with threaded interrupt handlers, because that
code is in the "hard" path of the interrupt handling code, that
gets executed in response to an IRQ signal.

What it would take is code to go back into the "hard" irq handler
after the thread has run, and take a second lap in checking the
line levels.

I don't know if that is possible to achieve in Linux, hm :/

Yours,
Linus Walleij

Re: [PATCH 3/3] iio: st_sensors: Use level interrupts

[Jonathan Cameron]

On 02/06/16 15:36, Linus Walleij wrote:
> On Sun, May 29, 2016 at 9:29 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 24/05/16 13:35, Crestez Dan Leonard wrote:
> 
>>> This might be completely crazy, but wouldn't it be possible to support
>>> gpio level interrupts purely in software? Just read the GPIO state again
>>> after the interrupt is unmasked and retrigger.
>>>
>>> This seems preferable to implementing per-driver workarounds. Perhaps it
>>> would even fit in some gpio-irqchip glue.
>>
>> That's precisely the question I raised way back when writing the lis3l02dq
>> driver.  Apparently someone once had a go but it never went anywhere...
> 
> It is maybe possible to do in some cases. What you have to do
> is to augment the GPIO irqchip driver to read the line status register
> after handling an IRQ, if level IRQ is desired, and if the line is still high
> after a rising edge or still low after a falling edge, just call the IRQ
> handler again.
> 
> This will not work with threaded interrupt handlers, because that
> code is in the "hard" path of the interrupt handling code, that
> gets executed in response to an IRQ signal.
> 
> What it would take is code to go back into the "hard" irq handler
> after the thread has run, and take a second lap in checking the
> line levels.
> 
> I don't know if that is possible to achieve in Linux, hm :/
It would probably be possible to do deep in the irq chip as you'd do
the check at the point of the interrupt being unmasked.  A little nasty
to then flip back into interrupt context to re trigger the interrupt
but could probably be done - perhaps by maintaining a parallel
interrupt chip that is closer to an IIO trigger (i.e. entirely fake).

Would be easier if only rerunning the threaded part though..

Is it worth it?  Doubtful.

Cc'd the IRQCHIP maintainers in case they want to add anything or
give pointers to a previous similar discussion.

Jonathan
> 
> Yours,
> Linus Walleij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/3] iio: st_sensors: Use level interrupts

[Linus Walleij]

On Fri, May 13, 2016 at 8:43 PM, Crestez Dan Leonard
<leonard.crestez@intel.com> wrote:

> As far as I can tell DRDY for ST sensors behaves as a level rather than
> edge interrupt. Registering for IRQF_TRIGGER_RISING instead of
> IRQF_TRIGGER_HIGH mostly works except when the sampling frequency is
> high enough that new samples come before the new ones are read
> completely. In that case the interrupt line remains high, no more rising
> edges occur and the iio buffer stalls.
>
> Configuring the interrupt as IRQF_TRIGGER_HIGH makes it work as
> expected. This patch makes it so that st_sensors_trigger interrupt
> request code doesn't mangle the request flags into IRQF_TRIGGER_RISING.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
> This is an alternative fix to this patch:
>         https://www.spinics.net/lists/linux-iio/msg24722.html
> [PATCH 2/2 v6] iio: st_sensors: read surplus samples in trigger.

I have incorporated this approach into my patch now: please give
it a spin!

Yours,
Linus Walleij

Re: [PATCH 1/3] iio: st_sensors: Init trigger before irq request

[Jonathan Cameron]

On 13/05/16 19:43, Crestez Dan Leonard wrote:
> This fixes a possible race where an interrupt arrives before complete
> initialization and crashes because iio_trigger_get_drvdata returns NULL.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Looks 'obviously' correct to me, but I'd like Denis to confirm on this one.

Jonathan
> ---
> I ran into this while breaking the driver. But since the interrupt line can be
> shared the handler should always be able to accept and ignore a call.
> 
>  drivers/iio/common/st_sensors/st_sensors_trigger.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> index 98bd5b3..7c2e6ab 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> @@ -89,6 +89,10 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  		return -ENOMEM;
>  	}
>  
> +	iio_trigger_set_drvdata(sdata->trig, indio_dev);
> +	sdata->trig->ops = trigger_ops;
> +	sdata->trig->dev.parent = sdata->dev;
> +
>  	irq = sdata->get_irq_data_ready(indio_dev);
>  	irq_trig = irqd_get_trigger_type(irq_get_irq_data(irq));
>  	/*
> @@ -150,10 +154,6 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
>  		goto iio_trigger_free;
>  	}
>  
> -	iio_trigger_set_drvdata(sdata->trig, indio_dev);
> -	sdata->trig->ops = trigger_ops;
> -	sdata->trig->dev.parent = sdata->dev;
> -
>  	err = iio_trigger_register(sdata->trig);
>  	if (err < 0) {
>  		dev_err(&indio_dev->dev, "failed to register iio trigger.\n");
> 

Re: [PATCH 1/3] iio: st_sensors: Init trigger before irq request

[Linus Walleij]

On Fri, May 13, 2016 at 8:43 PM, Crestez Dan Leonard
<leonard.crestez@intel.com> wrote:

> This fixes a possible race where an interrupt arrives before complete
> initialization and crashes because iio_trigger_get_drvdata returns NULL.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Giuseppe Barba <giuseppe.barba@st.com>
> Cc: Denis Ciocca <denis.ciocca@st.com>
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
> I ran into this while breaking the driver. But since the interrupt line can be
> shared the handler should always be able to accept and ignore a call.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

However I think my patch fixing the thread issue will collide
by being close in context to this so it'd be great if you
could resend it on top of
"iio: st_sensors: switch to a threaded interrupt"?

Yours,
Linus Walleij

Re: [PATCH 1/3] iio: st_sensors: Init trigger before irq request

[Crestez Dan Leonard]

On 05/24/2016 02:27 PM, Linus Walleij wrote:
> On Fri, May 13, 2016 at 8:43 PM, Crestez Dan Leonard
> <leonard.crestez@intel.com> wrote:
> 
>> This fixes a possible race where an interrupt arrives before complete
>> initialization and crashes because iio_trigger_get_drvdata returns NULL.
>>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>> Cc: Giuseppe Barba <giuseppe.barba@st.com>
>> Cc: Denis Ciocca <denis.ciocca@st.com>
>> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
>> ---
>> I ran into this while breaking the driver. But since the interrupt line can be
>> shared the handler should always be able to accept and ignore a call.
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> However I think my patch fixing the thread issue will collide
> by being close in context to this so it'd be great if you
> could resend it on top of
> "iio: st_sensors: switch to a threaded interrupt"?

I checked my local git tree and this was already on top of v6 of that
patch. I think this issue is much less likely to cause problems without
it. Perhaps it should be incorporated into your patch?

I'll resend if it causes conflicts for Jonathan.

-- 
Regards,
Leonard

Re: [PATCH 1/3] iio: st_sensors: Init trigger before irq request

[Linus Walleij]

On Tue, May 24, 2016 at 2:33 PM, Crestez Dan Leonard
<leonard.crestez@intel.com> wrote:
> [Me]
>> However I think my patch fixing the thread issue will collide
>> by being close in context to this so it'd be great if you
>> could resend it on top of
>> "iio: st_sensors: switch to a threaded interrupt"?
>
> I checked my local git tree and this was already on top of v6 of that
> patch.

OK no problem then.

> I think this issue is much less likely to cause problems without
> it.

Yeah.

> Perhaps it should be incorporated into your patch?

No better keep them separate.

Yours,
Linus Walleij

Re: [PATCH 1/3] iio: st_sensors: Init trigger before irq request

[Jonathan Cameron]

On 24/05/16 22:54, Linus Walleij wrote:
> On Tue, May 24, 2016 at 2:33 PM, Crestez Dan Leonard
> <leonard.crestez@intel.com> wrote:
>> [Me]
>>> However I think my patch fixing the thread issue will collide
>>> by being close in context to this so it'd be great if you
>>> could resend it on top of
>>> "iio: st_sensors: switch to a threaded interrupt"?
>>
>> I checked my local git tree and this was already on top of v6 of that
>> patch.
> 
> OK no problem then.
> 
>> I think this issue is much less likely to cause problems without
>> it.
> 
> Yeah.
> 
>> Perhaps it should be incorporated into your patch?
> 
> No better keep them separate.
Went on fine. Applied to the fixes-togreg-post-rc1 branch.

Whilst technically this was ugly before, it only became an issue once
we had shared interrupts. As such I'm not marking it for stable.

Thanks,

Jonathan
> 
> Yours,
> Linus Walleij
>