[PATCH] iio: adc: ad7791: fix IRQ flags

[PATCH] iio: adc: ad7791: fix IRQ flags

[Nuno Sá]

The interrupt is triggered on the falling edge rather than being a level
low interrupt.

Fixes: da4d3d6bb9f6 ("iio: adc: ad-sigma-delta: Allow custom IRQ flags")
Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/adc/ad7791.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7791.c b/drivers/iio/adc/ad7791.c
index fee8d129a5f0..86effe8501b4 100644
--- a/drivers/iio/adc/ad7791.c
+++ b/drivers/iio/adc/ad7791.c
@@ -253,7 +253,7 @@ static const struct ad_sigma_delta_info ad7791_sigma_delta_info = {
 	.has_registers = true,
 	.addr_shift = 4,
 	.read_mask = BIT(3),
-	.irq_flags = IRQF_TRIGGER_LOW,
+	.irq_flags = IRQF_TRIGGER_FALLING,
 };
 
 static int ad7791_read_raw(struct iio_dev *indio_dev,
-- 
2.39.1

Re: [PATCH] iio: adc: ad7791: fix IRQ flags

[Jonathan Cameron]

On Fri, 20 Jan 2023 13:46:45 +0100
Nuno Sá <nuno.sa@analog.com> wrote:

> The interrupt is triggered on the falling edge rather than being a level
> low interrupt.
> 
> Fixes: da4d3d6bb9f6 ("iio: adc: ad-sigma-delta: Allow custom IRQ flags")
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

What are the symptoms of this?  Given the ad_sigma_delta.c irq handler
disables the interrupt until after the data read is done (at which point the
level is presumably high again), I don't immediately see why the change
here has any impact - either we trigger on the fall, or on the fact it
has become low..

Or is there a board other there that only does end interrupts that is causing
problems?

Jonathan

> ---
>  drivers/iio/adc/ad7791.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ad7791.c b/drivers/iio/adc/ad7791.c
> index fee8d129a5f0..86effe8501b4 100644
> --- a/drivers/iio/adc/ad7791.c
> +++ b/drivers/iio/adc/ad7791.c
> @@ -253,7 +253,7 @@ static const struct ad_sigma_delta_info ad7791_sigma_delta_info = {
>  	.has_registers = true,
>  	.addr_shift = 4,
>  	.read_mask = BIT(3),
> -	.irq_flags = IRQF_TRIGGER_LOW,
> +	.irq_flags = IRQF_TRIGGER_FALLING,
>  };
>  
>  static int ad7791_read_raw(struct iio_dev *indio_dev,

Re: [PATCH] iio: adc: ad7791: fix IRQ flags

[Nuno Sá]

On Sat, 2023-01-21 at 16:58 +0000, Jonathan Cameron wrote:
> On Fri, 20 Jan 2023 13:46:45 +0100
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > The interrupt is triggered on the falling edge rather than being a
> > level
> > low interrupt.
> > 
> > Fixes: da4d3d6bb9f6 ("iio: adc: ad-sigma-delta: Allow custom IRQ
> > flags")
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> What are the symptoms of this?  Given the ad_sigma_delta.c irq
> handler
> disables the interrupt until after the data read is done (at which
> point the
> level is presumably high again), I don't immediately see why the
> change
> here has any impact - either we trigger on the fall, or on the fact
> it
> has become low..
> 
> 

Honestly I did not checked this in any HW. This was just by inspecting
the datasheet and confirming that the LOW IRQ is not coherent with what
we have in other sigma delta ADCs.

However, after some git blaming, I found this [1] which shows that this
might be an issue...

Hmm, maybe makes sense to add a link to the bellow patch in the commit
description...
 
[1]:https://lore.kernel.org/linux-iio/20200113102653.20900-3-alexandru.tachici@analog.com/

- Nuno Sá

Re: [PATCH] iio: adc: ad7791: fix IRQ flags

[Nuno Sá]

On Mon, 2023-01-30 at 10:01 +0100, Nuno Sá wrote:
> On Sat, 2023-01-21 at 16:58 +0000, Jonathan Cameron wrote:
> > On Fri, 20 Jan 2023 13:46:45 +0100
> > Nuno Sá <nuno.sa@analog.com> wrote:
> > 
> > > The interrupt is triggered on the falling edge rather than being
> > > a
> > > level
> > > low interrupt.
> > > 
> > > Fixes: da4d3d6bb9f6 ("iio: adc: ad-sigma-delta: Allow custom IRQ
> > > flags")
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > 
> > What are the symptoms of this?  Given the ad_sigma_delta.c irq
> > handler
> > disables the interrupt until after the data read is done (at which
> > point the
> > level is presumably high again), I don't immediately see why the
> > change
> > here has any impact - either we trigger on the fall, or on the fact
> > it
> > has become low..
> > 
> > 
> 
> Honestly I did not checked this in any HW. This was just by
> inspecting
> the datasheet and confirming that the LOW IRQ is not coherent with
> what
> we have in other sigma delta ADCs.
> 
> However, after some git blaming, I found this [1] which shows that
> this
> might be an issue...
> 
> Hmm, maybe makes sense to add a link to the bellow patch in the
> commit
> description...
>  
> [1]:
> https://lore.kernel.org/linux-iio/20200113102653.20900-3-alexandru.tachici@analog.com
> /
> 
> - Nuno Sá

Hi Jonathan,

Anything that I should do in this one? As I did not tested it, it might
not be a real issue but I still think the patch is good even though it
might not deserve a Fixes tag...

- Nuno Sá

Re: [PATCH] iio: adc: ad7791: fix IRQ flags

[Jonathan Cameron]

On Mon, 06 Feb 2023 12:13:50 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Mon, 2023-01-30 at 10:01 +0100, Nuno Sá wrote:
> > On Sat, 2023-01-21 at 16:58 +0000, Jonathan Cameron wrote:  
> > > On Fri, 20 Jan 2023 13:46:45 +0100
> > > Nuno Sá <nuno.sa@analog.com> wrote:
> > >   
> > > > The interrupt is triggered on the falling edge rather than being
> > > > a
> > > > level
> > > > low interrupt.
> > > > 
> > > > Fixes: da4d3d6bb9f6 ("iio: adc: ad-sigma-delta: Allow custom IRQ
> > > > flags")
> > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>  
> > > 
> > > What are the symptoms of this?  Given the ad_sigma_delta.c irq
> > > handler
> > > disables the interrupt until after the data read is done (at which
> > > point the
> > > level is presumably high again), I don't immediately see why the
> > > change
> > > here has any impact - either we trigger on the fall, or on the fact
> > > it
> > > has become low..
> > > 
> > >   
> > 
> > Honestly I did not checked this in any HW. This was just by
> > inspecting
> > the datasheet and confirming that the LOW IRQ is not coherent with
> > what
> > we have in other sigma delta ADCs.
> > 
> > However, after some git blaming, I found this [1] which shows that
> > this
> > might be an issue...
> > 
> > Hmm, maybe makes sense to add a link to the bellow patch in the
> > commit
> > description...
> >  
> > [1]:
> > https://lore.kernel.org/linux-iio/20200113102653.20900-3-alexandru.tachici@analog.com
> > /
> > 
> > - Nuno Sá  
> 
> Hi Jonathan,
> 
> Anything that I should do in this one? As I did not tested it, it might
> not be a real issue but I still think the patch is good even though it
> might not deserve a Fixes tag...
> 
> - Nuno Sá

Applied to the fixes-togreg rbanch of iio.git.  I left the fixes tag
but just to be awkward didn't mark it for stable (it'll get picked up anyway
probably but I didn't want to imply there was any rush in doing so ;)

Thanks,

Jonathan