ADC using ad7793.ko, such as AD7794, may read incorrect data.
Extra interrupt is pending if the data on DOUT contains a falling edge.
Therefore, wait_for_completion_timeout returns immediately.
This patch fixes the issue by setting IRQ_DISABLE_UNLAZY flag.
Signed-off-by: Masahiro Honda <[email protected]>
---
drivers/iio/adc/ad_sigma_delta.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index d8570f620..364051809 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -584,6 +584,7 @@ static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_de
init_completion(&sigma_delta->completion);
sigma_delta->irq_dis = true;
+ irq_set_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
ret = devm_request_irq(dev, sigma_delta->spi->irq,
ad_sd_data_rdy_trig_poll,
sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
--
2.34.1
On Mon, 2023-03-06 at 13:47 +0900, Masahiro Honda wrote:
> ADC using ad7793.ko, such as AD7794, may read incorrect data.
> Extra interrupt is pending if the data on DOUT contains a falling
> edge.
> Therefore, wait_for_completion_timeout returns immediately.
> This patch fixes the issue by setting IRQ_DISABLE_UNLAZY flag.
>
> Signed-off-by: Masahiro Honda <[email protected]>
> ---
> drivers/iio/adc/ad_sigma_delta.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iio/adc/ad_sigma_delta.c
> b/drivers/iio/adc/ad_sigma_delta.c
> index d8570f620..364051809 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -584,6 +584,7 @@ static int devm_ad_sd_probe_trigger(struct device
> *dev, struct iio_dev *indio_de
> init_completion(&sigma_delta->completion);
>
> sigma_delta->irq_dis = true;
> + irq_set_status_flags(sigma_delta->spi->irq,
> IRQ_DISABLE_UNLAZY);
Hmmm this looks to be a very likely issue for any device having to
brute force IRQ disabling by disabling the line (disable_irq()). That
said, I think the commit message can (needs) to be improved. The
message feels a bit confusing to me. Also, having a reference to
commit e9849777d0e2 ("genirq: Add flag to force mask in
disable_irq[_nosync]()")
would be nice to give some background on why this can be an issue.
Another thing that came to my mind is if the data is totally
garbage/wrong or is it just some outstanding sample?
Some research on this also seems to point that we should (need?) call
irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ.
I also wonder if we should keep this out of the library and have a per
device config? Sure, this won't make anything wrong but it will hurt
performance. OTOH, even though no one else ever reported this before,
it looks like this can be an issue for all of the supported sigma delta
ADCs.
- Nuno Sá
On Mon, Mar 6, 2023 at 10:30 PM Nuno Sá <[email protected]> wrote:
>
> Hmmm this looks to be a very likely issue for any device having to
> brute force IRQ disabling by disabling the line (disable_irq()). That
> said, I think the commit message can (needs) to be improved. The
> message feels a bit confusing to me. Also, having a reference to
>
> commit e9849777d0e2 ("genirq: Add flag to force mask in
> disable_irq[_nosync]()")
>
> would be nice to give some background on why this can be an issue.
>
> Another thing that came to my mind is if the data is totally
> garbage/wrong or is it just some outstanding sample?
>
>
> Some research on this also seems to point that we should (need?) call
> irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ.
>
Thank you for your reply. I'll improve it.
> I also wonder if we should keep this out of the library and have a per
> device config? Sure, this won't make anything wrong but it will hurt
> performance. OTOH, even though no one else ever reported this before,
> it looks like this can be an issue for all of the supported sigma delta
> ADCs.
>
I'll also investigate it.
Hi, Nuno
> On Mon, Mar 6, 2023 at 10:30 PM Nuno Sá <[email protected]> wrote:
> > Another thing that came to my mind is if the data is totally
> > garbage/wrong or is it just some outstanding sample?
I'm sorry for not answering your question. The data is the same as
the previous conversion even if the input voltage is changed. At this
time, a logic analyzer shows that the read operation is performed
immediately after the conversion is performed. The read operation
returns the previous conversion result because it is performed before
the completion of the conversion.
> > Some research on this also seems to point that we should (need?) call
> > irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ.
I have understood that I need to call irq_clear_status_flags. However,
I cannot find a code to free the IRQ in ad_sigma_delta.c and other
Sigma-Delta ADC driver source files. So, I would like to implement
only irq_set_status_flags.
On Mon, 2023-03-27 at 18:01 +0900, Masahiro Honda wrote:
Hi Masahiro,
Thanks for looking in more detail into this...
> Hi, Nuno
>
> > On Mon, Mar 6, 2023 at 10:30 PM Nuno Sá <[email protected]>
> > wrote:
> > > Another thing that came to my mind is if the data is totally
> > > garbage/wrong or is it just some outstanding sample?
>
> I'm sorry for not answering your question. The data is the same as
> the previous conversion even if the input voltage is changed. At this
> time, a logic analyzer shows that the read operation is performed
> immediately after the conversion is performed. The read operation
> returns the previous conversion result because it is performed before
> the completion of the conversion.
>
So, my suspicion was right... We are getting stalled data (which is
obviously not good). AFAIU, when disabling the IRQ, we don't
immediately mask the IRQ and we only do it in the next time an
interrupt (sample) comes which means (I think) we'll process (right
away) that outstanding interrupt next time we enable the IRQ.
> > > Some research on this also seems to point that we should (need?)
> > > call
> > > irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the
> > > IRQ.
>
> I have understood that I need to call irq_clear_status_flags.
> However,
> I cannot find a code to free the IRQ in ad_sigma_delta.c and other
> Sigma-Delta ADC driver source files. So, I would like to implement
> only irq_set_status_flags.
Well, that's because we are using devm_request_irq() which is a device
managed API. So, I can see two options in here...
1) You do not use devm_request_irq() and use request_irq() +
devm_add_action_or_reset() and in your release() function you would
call irq_clear_status_flags() + free_irq().
2) You add a devm_add_action_or_reset() after devm_request_irq() and
your release() function would only clear the flag. But in here we would
likely also have to be careful in the case where devm_request_irq()
fails. So option 2) seems a bit more "ugly".
I would likely go to option 1) but maybe Jonathan or others have better
ideas.
- Nuno Sá
On Mon, Mar 27, 2023 at 9:26 PM Nuno Sá <[email protected]> wrote:
>
> So, my suspicion was right... We are getting stalled data (which is
> obviously not good). AFAIU, when disabling the IRQ, we don't
> immediately mask the IRQ and we only do it in the next time an
> interrupt (sample) comes which means (I think) we'll process (right
> away) that outstanding interrupt next time we enable the IRQ.
>
Thank you. I understand.
> > > > Some research on this also seems to point that we should (need?)
> > > > call
> > > > irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the
> > > > IRQ.
> >
> > I have understood that I need to call irq_clear_status_flags.
> > However,
> > I cannot find a code to free the IRQ in ad_sigma_delta.c and other
> > Sigma-Delta ADC driver source files. So, I would like to implement
> > only irq_set_status_flags.
>
> Well, that's because we are using devm_request_irq() which is a device
> managed API. So, I can see two options in here...
>
> 1) You do not use devm_request_irq() and use request_irq() +
> devm_add_action_or_reset() and in your release() function you would
> call irq_clear_status_flags() + free_irq().
>
> 2) You add a devm_add_action_or_reset() after devm_request_irq() and
> your release() function would only clear the flag. But in here we would
> likely also have to be careful in the case where devm_request_irq()
> fails. So option 2) seems a bit more "ugly".
>
> I would likely go to option 1) but maybe Jonathan or others have better
> ideas.
>
Thank you very much for letting me know about the API and ideas.
I'll try to implement 1).