[PATCH 0/3] iio: adc: Fix flags in sigma-delta drivers

[PATCH 0/3] iio: adc: Fix flags in sigma-delta drivers

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

Some sigma-delta drivers use wrong irq_flags specified in the
ad_sigma_delta_info struct. Add the flags corresponding to the
interrupt type specified in the data-sheets of each chip.

Alexandru Tachici (3):
  iio: adc: ad7192: Fix IRQ flag
  iio: adc: ad7780: Fix IRQ flag
  iio: adc: ad7923: Fix IRQ flag

 drivers/iio/adc/ad7192.c | 1 +
 drivers/iio/adc/ad7780.c | 2 +-
 drivers/iio/adc/ad7793.c | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

--
2.25.1

[PATCH 1/3] iio: adc: ad7192: Fix IRQ flag

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

IRQ type in ad_sigma_delta_info struct was missing.

Fixes: da4d3d6bb9f6 ("iio: adc: ad-sigma-delta: Allow custom IRQ flags")
Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7192.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
index ee8ed9481025..2121a812b0c3 100644
--- a/drivers/iio/adc/ad7192.c
+++ b/drivers/iio/adc/ad7192.c
@@ -293,6 +293,7 @@ static const struct ad_sigma_delta_info ad7192_sigma_delta_info = {
 	.has_registers = true,
 	.addr_shift = 3,
 	.read_mask = BIT(6),
+	.irq_flags = IRQF_TRIGGER_FALLING,
 };
 
 static const struct ad_sd_calib_data ad7192_calib_arr[8] = {
-- 
2.25.1

[PATCH 2/3] iio: adc: ad7780: Fix IRQ flag

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

Correct IRQ flag here is falling.

Fixes: da4d3d6bb9f6 ("iio: adc: ad-sigma-delta: Allow custom IRQ flags")
Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7780.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7780.c b/drivers/iio/adc/ad7780.c
index 42bb952f4738..b6e8c8abf6f4 100644
--- a/drivers/iio/adc/ad7780.c
+++ b/drivers/iio/adc/ad7780.c
@@ -203,7 +203,7 @@ static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {
 	.set_mode = ad7780_set_mode,
 	.postprocess_sample = ad7780_postprocess_sample,
 	.has_registers = false,
-	.irq_flags = IRQF_TRIGGER_LOW,
+	.irq_flags = IRQF_TRIGGER_FALLING,
 };
 
 #define _AD7780_CHANNEL(_bits, _wordsize, _mask_all)		\
-- 
2.25.1

[PATCH 3/3] iio: adc: ad7923: Fix IRQ flag

[alexandru.tachici]

From: Alexandru Tachici <alexandru.tachici@analog.com>

Correct IRQ flag here is falling.

Fixes: da4d3d6bb9f6 ("iio: adc: ad-sigma-delta: Allow custom IRQ flags")
Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
---
 drivers/iio/adc/ad7793.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ad7793.c b/drivers/iio/adc/ad7793.c
index ef3e2d3ecb0c..0e7ab3fb072a 100644
--- a/drivers/iio/adc/ad7793.c
+++ b/drivers/iio/adc/ad7793.c
@@ -206,7 +206,7 @@ static const struct ad_sigma_delta_info ad7793_sigma_delta_info = {
 	.has_registers = true,
 	.addr_shift = 3,
 	.read_mask = BIT(6),
-	.irq_flags = IRQF_TRIGGER_LOW,
+	.irq_flags = IRQF_TRIGGER_FALLING,
 };
 
 static const struct ad_sd_calib_data ad7793_calib_arr[6] = {
-- 
2.25.1

Re: [PATCH 0/3] iio: adc: Fix flags in sigma-delta drivers

[Jonathan Cameron]

On Thu, 29 Jul 2021 11:47:28 +0300
<alexandru.tachici@analog.com> wrote:

> From: Alexandru Tachici <alexandru.tachici@analog.com>
> 
> Some sigma-delta drivers use wrong irq_flags specified in the
> ad_sigma_delta_info struct. Add the flags corresponding to the
> interrupt type specified in the data-sheets of each chip.
> 

The complexity here is that we normally now leave this to the
firmware description of the interrupt.  The reason for that is
people have an annoying habit of building boards where there is
an inverter used as a cheap level converter on the interrupt line.

It an also be somewhat 'fun' to identify from a datasheet if
the signal is actually an edge interrupt or a level interrupt and
in the case of devices that don't have any autonomous triggering
(i.e. won't grab new data unless you tell the to) the difference
is irrelevant.  Take the ad7780 for example.  It has an interrupt
that remains low unless
a) The data is read - normal case I'd imagine
b) New data capture occurs (slowly at 10Hz)

So we aren't dealing with a short pulse, or a situation where we
have an interrupt that will stay set after the data is read
(both of which would make this definitely an edge interrupt to avoid
 possibility of either missing or double triggering).

Hence whilst it will work as an edge interrupt, it will also work
fine with the existing level interrupt and in some ways level is
more appropriate as the interrupt will remain set if you don't read
it (for a while anyway and after that it's not safe to read).

For extra fun / background for anyone else reading this thread,
this interrupt line is also the data out line, so
we absolutely 'must' keep the interrupt disabled until we are done
with the read out, but that's handled in the ad_sigma_delta core.

Conclusion, I'm not sure these are actually wrong, and if we were
doing this today we wouldn't have them anyway...  So have we seen
any problems with current code?

Jonathan


> Alexandru Tachici (3):
>   iio: adc: ad7192: Fix IRQ flag
>   iio: adc: ad7780: Fix IRQ flag
>   iio: adc: ad7923: Fix IRQ flag
> 
>  drivers/iio/adc/ad7192.c | 1 +
>  drivers/iio/adc/ad7780.c | 2 +-
>  drivers/iio/adc/ad7793.c | 2 +-
>  3 files changed, 3 insertions(+), 2 deletions(-)
> 
> --
> 2.25.1

Re: [PATCH 0/3] iio: adc: Fix flags in sigma-delta drivers

[alexandru.tachici]

In Sigma-Delta devices the SDO line is also used as an interrupt.
Leaving IRQ on level instead of falling might trigger a sample read
when the IRQ is enabled, as the SDO line is already low. Not sure
if SDO line will always imediately go high in ad_sd_buffer_postenable
before the IRQ is enabled.

Also the datasheets seem to explicitly say the falling edge of the SDO
should be used as an interrupt.

-Alexandru

Re: [PATCH 0/3] iio: adc: Fix flags in sigma-delta drivers

[Jonathan Cameron]

On Tue, 3 Aug 2021 12:17:41 +0300
<alexandru.tachici@analog.com> wrote:

> In Sigma-Delta devices the SDO line is also used as an interrupt.
> Leaving IRQ on level instead of falling might trigger a sample read
> when the IRQ is enabled, as the SDO line is already low. Not sure
> if SDO line will always imediately go high in ad_sd_buffer_postenable
> before the IRQ is enabled.

ok. That last bit sounds like a good justification.  It might do the
wrong thing on initial startup.  Please resend with that in the patch
descriptions for those you are modifying.

For those where it is being introduced, we probably want to leave the
interrupt type to firmware (i.e. patch 1)

> 
> Also the datasheets seem to explicitly say the falling edge of the SDO
> should be used as an interrupt.

Reference?

> 
> -Alexandru