From: lars@metafoo.de (Lars-Peter Clausen)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH V2] media: i2c: Add ADV761X support
Date: Wed, 27 Nov 2013 17:48:10 +0100 [thread overview]
Message-ID: <5296224A.6060906@metafoo.de> (raw)
In-Reply-To: <2150651.hQNra4Rlob@avalon>
[...]
>>>>>> The driver enables multiple interrupts on the chip, however, the
>>>>>> adv7604_isr callback doesn't seem to handle them correctly.
>>>>>> According to the docs:
>>>>>> "If an interrupt event occurs, and then a second interrupt event occurs
>>>>>> before the system controller has cleared or masked the first interrupt
>>>>>> event, the ADV7611 does not generate a second interrupt signal."
>>>>>>
>>>>>> However, the interrupt_service_routine doesn't account for that.
>>>>>> For example, in case fmt_change interrupt happens while
>>>>>> fmt_change_digital interrupt is being processed by the adv7604_isr
>>>>>> routine. If fmt_change status is set just before we clear
>>>>>> fmt_change_digital, we never clear fmt_change. Thus, we end up with
>>>>>> fmt_change interrupt missed and therefore further interrupts disabled.
>>>>>> I've tried to call the adv7604_isr routine in a loop and return from
>>>>>> the worlqueue only when all interrupt status bits are cleared. This did
>>>>>> help a bit, but sometimes I started getting lots of I2C read/write
>>>>>> errors for some reason.
>>>>>
>>>>> I'm not sure if there is much that can be done about this. The code
>>>>> reads the interrupt status, then clears the interrupts right after.
>>>>> There is always a race condition there since this isn't atomic ('read
>>>>> and clear'). Unless Lars-Peter has a better idea?
>>>>>
>>>>> What can be improved, though, is to clear not just the interrupts that
>>>>> were read, but all the interrupts that are unmasked. You are right, you
>>>>> could loose an interrupt that way.
>>>>
>>>> Wouldn't level-trigerred interrupts fix the issue ?
>>
>> In this case we need to disable the IRQ line in the IRQ handler and
>> re-enable it in the workqueue. (we can't call the interrupt service routine
>> from the interrupt context.)
>
> Can't we just flag the interrupt in a non-threaded IRQ handler, acknowledge
> the interrupt and then schedule work on a workqueue for the bottom half ?
Acknowledging the interrupt will require a non IRQ context, since it has to
do I2C transfers.
- Lars
WARNING: multiple messages have this Message-ID (diff)
From: Lars-Peter Clausen <lars@metafoo.de>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Valentine <valentine.barshak@cogentembedded.com>,
Hans Verkuil <hansverk@cisco.com>,
Hans Verkuil <hverkuil@xs4all.nl>,
linux-media@vger.kernel.org,
Mauro Carvalho Chehab <m.chehab@samsung.com>,
Hans Verkuil <hans.verkuil@cisco.com>,
Guennadi Liakhovetski <g.liakhovetski@gmx.de>,
Simon Horman <horms@verge.net.au>,
Linus Walleij <linus.walleij@linaro.org>,
linux-arm-kernel@lists.infradead.org,
Wolfram Sang <wsa@the-dreams.de>
Subject: Re: [PATCH V2] media: i2c: Add ADV761X support
Date: Wed, 27 Nov 2013 17:48:10 +0100 [thread overview]
Message-ID: <5296224A.6060906@metafoo.de> (raw)
In-Reply-To: <2150651.hQNra4Rlob@avalon>
[...]
>>>>>> The driver enables multiple interrupts on the chip, however, the
>>>>>> adv7604_isr callback doesn't seem to handle them correctly.
>>>>>> According to the docs:
>>>>>> "If an interrupt event occurs, and then a second interrupt event occurs
>>>>>> before the system controller has cleared or masked the first interrupt
>>>>>> event, the ADV7611 does not generate a second interrupt signal."
>>>>>>
>>>>>> However, the interrupt_service_routine doesn't account for that.
>>>>>> For example, in case fmt_change interrupt happens while
>>>>>> fmt_change_digital interrupt is being processed by the adv7604_isr
>>>>>> routine. If fmt_change status is set just before we clear
>>>>>> fmt_change_digital, we never clear fmt_change. Thus, we end up with
>>>>>> fmt_change interrupt missed and therefore further interrupts disabled.
>>>>>> I've tried to call the adv7604_isr routine in a loop and return from
>>>>>> the worlqueue only when all interrupt status bits are cleared. This did
>>>>>> help a bit, but sometimes I started getting lots of I2C read/write
>>>>>> errors for some reason.
>>>>>
>>>>> I'm not sure if there is much that can be done about this. The code
>>>>> reads the interrupt status, then clears the interrupts right after.
>>>>> There is always a race condition there since this isn't atomic ('read
>>>>> and clear'). Unless Lars-Peter has a better idea?
>>>>>
>>>>> What can be improved, though, is to clear not just the interrupts that
>>>>> were read, but all the interrupts that are unmasked. You are right, you
>>>>> could loose an interrupt that way.
>>>>
>>>> Wouldn't level-trigerred interrupts fix the issue ?
>>
>> In this case we need to disable the IRQ line in the IRQ handler and
>> re-enable it in the workqueue. (we can't call the interrupt service routine
>> from the interrupt context.)
>
> Can't we just flag the interrupt in a non-threaded IRQ handler, acknowledge
> the interrupt and then schedule work on a workqueue for the bottom half ?
Acknowledging the interrupt will require a non IRQ context, since it has to
do I2C transfers.
- Lars
next prev parent reply other threads:[~2013-11-27 16:48 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-11-15 12:54 [PATCH V2] media: i2c: Add ADV761X support Valentine Barshak
2013-11-19 9:50 ` Hans Verkuil
2013-11-20 10:14 ` Valentine
2013-11-20 11:19 ` Hans Verkuil
2013-11-20 12:24 ` Valentine
2013-11-20 15:42 ` Hans Verkuil
2013-11-20 15:53 ` Valentine
2013-11-26 21:28 ` Valentine
2013-11-26 21:43 ` Lars-Peter Clausen
2013-11-26 21:57 ` Valentine
2013-11-26 22:02 ` Lars-Peter Clausen
2013-11-26 22:00 ` Laurent Pinchart
2013-11-26 22:03 ` Lars-Peter Clausen
2013-11-26 22:03 ` Laurent Pinchart
2013-11-26 22:06 ` Lars-Peter Clausen
2013-11-29 20:07 ` Lars-Peter Clausen
2013-11-27 8:21 ` Hans Verkuil
2013-11-27 9:59 ` Lars-Peter Clausen
2013-11-27 11:26 ` Hans Verkuil
2013-11-27 10:29 ` Valentine
2013-11-27 11:18 ` Hans Verkuil
2013-11-27 11:39 ` Laurent Pinchart
2013-11-27 12:14 ` Hans Verkuil
2013-11-27 12:32 ` Valentine
2013-11-27 13:07 ` Lars-Peter Clausen
2013-11-27 13:46 ` Valentine
2013-11-27 16:40 ` Laurent Pinchart
2013-11-27 16:40 ` Laurent Pinchart
2013-11-27 16:48 ` Lars-Peter Clausen [this message]
2013-11-27 16:48 ` Lars-Peter Clausen
2013-11-29 10:37 ` Linus Walleij
2013-11-29 10:37 ` Linus Walleij
2013-11-29 10:45 ` Lars-Peter Clausen
2013-11-29 10:45 ` Lars-Peter Clausen
2013-11-29 12:14 ` Valentine
2013-11-29 12:14 ` Valentine
2013-11-29 13:46 ` Linus Walleij
2013-11-29 13:46 ` Linus Walleij
2013-11-29 13:42 ` Linus Walleij
2013-11-29 13:42 ` Linus Walleij
2013-11-29 13:48 ` Lars-Peter Clausen
2013-11-29 13:48 ` Lars-Peter Clausen
2013-11-29 19:52 ` Linus Walleij
2013-11-29 19:52 ` Linus Walleij
2013-11-29 20:03 ` Laurent Pinchart
2013-11-29 20:03 ` Laurent Pinchart
2013-11-29 20:05 ` Lars-Peter Clausen
2013-11-29 20:05 ` Lars-Peter Clausen
2013-11-29 20:09 ` Linus Walleij
2013-11-29 20:09 ` Linus Walleij
2013-11-27 14:50 ` Lars-Peter Clausen
2013-11-27 16:29 ` Laurent Pinchart
2013-11-27 16:32 ` Laurent Pinchart
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5296224A.6060906@metafoo.de \
--to=lars@metafoo.de \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.