* [PATCH V2] media: i2c: Add ADV761X support
[not found] ` <5295E641.6060603@cogentembedded.com>
@ 2013-11-27 16:40 ` Laurent Pinchart
2013-11-27 16:48 ` Lars-Peter Clausen
2013-11-29 10:37 ` Linus Walleij
0 siblings, 2 replies; 12+ messages in thread
From: Laurent Pinchart @ 2013-11-27 16:40 UTC (permalink / raw)
To: linux-arm-kernel
Hi Valentine,
(CC'ing Linus Walleij, Wolfram Sang and LAKML)
On Wednesday 27 November 2013 16:32:01 Valentine wrote:
> On 11/27/2013 04:14 PM, Hans Verkuil wrote:
> > On 11/27/13 12:39, Laurent Pinchart wrote:
> >> On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
> >>> On 11/26/2013 10:28 PM, Valentine wrote:
> >>>> On 11/20/2013 07:53 PM, Valentine wrote:
> >>>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
[snip]
> >>> So I want to keep the interrupt_service_routine(). However, adding a
> >>> gpio field to the platform_data that, if set, will tell the driver to
> >>> request an irq and setup a workqueue that calls
> >>> interrupt_service_routine() would be fine with me. That will benefit a
> >>> lot of people since using gpios is much more common.
> >>
> >> We should use the i2c_board_info.irq field for that, not a field in the
> >> platform data structure. The IRQ line could be hooked up to a non-GPIO
> >> IRQ.
> >
> > Yes, of course. Although the adv7604 has two interrupt lines, so if you
> > would want to use the second, then that would still have to be specified
> > through the platform data.
>
> In this case the GPIO should be configured as interrupt source in the
> platform code. But this doesn't seem to work with R-Car GPIO since it is
> initialized later, and the gpio_to_irq function returns an error.
> The simplest way seemed to use a GPIO number in the platform data
> to have the adv driver configure the pin and request the IRQ.
> I'm not sure how to easily defer I2C board info IRQ setup (and
> camera/subdevice probing) until GPIO driver is ready.
Good question. This looks like a core problem to me, not specific to the
adv761x driver. Linus, Wolfram, do you have a comment on that ?
> >>>> 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 ?
> This however didn't seem to work with R-Car GPIO. Calling
> disable_irq_nosync(irq); from the GPIO LEVEL interrupt handler doesn't seem
> to disable it for some reason.
>
> Also if the isr is called by the upper level camera driver, we assume that
> it needs special handling (disabling/enabling) for the ADV76xx interrupt
> although it uses the API interrupt_service_routine callback. Not a big
> deal, but still doesn't look pretty to me.
>
> > See my earlier reply.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] media: i2c: Add ADV761X support
2013-11-27 16:40 ` [PATCH V2] media: i2c: Add ADV761X support Laurent Pinchart
@ 2013-11-27 16:48 ` Lars-Peter Clausen
2013-11-29 10:37 ` Linus Walleij
1 sibling, 0 replies; 12+ messages in thread
From: Lars-Peter Clausen @ 2013-11-27 16:48 UTC (permalink / raw)
To: linux-arm-kernel
[...]
>>>>>> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] media: i2c: Add ADV761X support
2013-11-27 16:40 ` [PATCH V2] media: i2c: Add ADV761X support Laurent Pinchart
2013-11-27 16:48 ` Lars-Peter Clausen
@ 2013-11-29 10:37 ` Linus Walleij
2013-11-29 10:45 ` Lars-Peter Clausen
1 sibling, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2013-11-29 10:37 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 27, 2013 at 5:40 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> (CC'ing Linus Walleij, Wolfram Sang and LAKML)
> On Wednesday 27 November 2013 16:32:01 Valentine wrote:
>> On 11/27/2013 04:14 PM, Hans Verkuil wrote:
>> > Yes, of course. Although the adv7604 has two interrupt lines, so if you
>> > would want to use the second, then that would still have to be specified
>> > through the platform data.
>>
>> In this case the GPIO should be configured as interrupt source in the
>> platform code. But this doesn't seem to work with R-Car GPIO since it is
>> initialized later, and the gpio_to_irq function returns an error.
>> The simplest way seemed to use a GPIO number in the platform data
>> to have the adv driver configure the pin and request the IRQ.
>> I'm not sure how to easily defer I2C board info IRQ setup (and
>> camera/subdevice probing) until GPIO driver is ready.
>
> Good question. This looks like a core problem to me, not specific to the
> adv761x driver. Linus, Wolfram, do you have a comment on that ?
So we recently has a large-ish discussion involving me, Stephen
Warren and Jean-Christophe, leading to the conclusion that the
gpio_chip and irq_chip abstractions are orthogonal, and you should
be able to request a GPIO or IRQ without interacting with the other
subsystem.
Specifically you should be able to request an IRQ from the irq_chip
portions of the driver without first requesting the GPIO line.
Some drivers already support this.
We added an internal API to the gpiolib so that the lib, *internally*
can be made aware that a certain GPIO line is used for IRQs,
see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1
"gpio: add API to be strict about GPIO IRQ usage"
So I guess the answer to the question is something like, fix
the GPIO driver to stop requiring the GPIO lines to be requested
and configured before being used as IRQs, delete that code,
and while you're at it add a call to gpiod_lock_as_irq()
to your GPIO driver in the right spot: examples are on the
mailing list and my mark-irqs branch in the GPIO tree.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] media: i2c: Add ADV761X support
2013-11-29 10:37 ` Linus Walleij
@ 2013-11-29 10:45 ` Lars-Peter Clausen
2013-11-29 12:14 ` Valentine
2013-11-29 13:42 ` Linus Walleij
0 siblings, 2 replies; 12+ messages in thread
From: Lars-Peter Clausen @ 2013-11-29 10:45 UTC (permalink / raw)
To: linux-arm-kernel
On 11/29/2013 11:37 AM, Linus Walleij wrote:
> On Wed, Nov 27, 2013 at 5:40 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> (CC'ing Linus Walleij, Wolfram Sang and LAKML)
>> On Wednesday 27 November 2013 16:32:01 Valentine wrote:
>>> On 11/27/2013 04:14 PM, Hans Verkuil wrote:
>
>>>> Yes, of course. Although the adv7604 has two interrupt lines, so if you
>>>> would want to use the second, then that would still have to be specified
>>>> through the platform data.
>>>
>>> In this case the GPIO should be configured as interrupt source in the
>>> platform code. But this doesn't seem to work with R-Car GPIO since it is
>>> initialized later, and the gpio_to_irq function returns an error.
>>> The simplest way seemed to use a GPIO number in the platform data
>>> to have the adv driver configure the pin and request the IRQ.
>>> I'm not sure how to easily defer I2C board info IRQ setup (and
>>> camera/subdevice probing) until GPIO driver is ready.
>>
>> Good question. This looks like a core problem to me, not specific to the
>> adv761x driver. Linus, Wolfram, do you have a comment on that ?
>
> So we recently has a large-ish discussion involving me, Stephen
> Warren and Jean-Christophe, leading to the conclusion that the
> gpio_chip and irq_chip abstractions are orthogonal, and you should
> be able to request a GPIO or IRQ without interacting with the other
> subsystem.
>
> Specifically you should be able to request an IRQ from the irq_chip
> portions of the driver without first requesting the GPIO line.
>
> Some drivers already support this.
>
> We added an internal API to the gpiolib so that the lib, *internally*
> can be made aware that a certain GPIO line is used for IRQs,
> see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1
> "gpio: add API to be strict about GPIO IRQ usage"
>
> So I guess the answer to the question is something like, fix
> the GPIO driver to stop requiring the GPIO lines to be requested
> and configured before being used as IRQs, delete that code,
> and while you're at it add a call to gpiod_lock_as_irq()
> to your GPIO driver in the right spot: examples are on the
> mailing list and my mark-irqs branch in the GPIO tree.
As far as I understand it this already works more or less with the driver.
The problem is that the IRQ numbers are dynamically allocated, while the
GPIO numbers apparently are not. So the board code knows the the GPIO number
at compile time and can pass this to the diver which then does a gpio_to_irq
to lookup the IRQ number. This of course isn't really a problem with
devicetree, but only with platform board code.
- Lars
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] media: i2c: Add ADV761X support
2013-11-29 10:45 ` Lars-Peter Clausen
@ 2013-11-29 12:14 ` Valentine
2013-11-29 13:46 ` Linus Walleij
2013-11-29 13:42 ` Linus Walleij
1 sibling, 1 reply; 12+ messages in thread
From: Valentine @ 2013-11-29 12:14 UTC (permalink / raw)
To: linux-arm-kernel
On 11/29/2013 02:45 PM, Lars-Peter Clausen wrote:
> On 11/29/2013 11:37 AM, Linus Walleij wrote:
>> On Wed, Nov 27, 2013 at 5:40 PM, Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com> wrote:
>>> (CC'ing Linus Walleij, Wolfram Sang and LAKML)
>>> On Wednesday 27 November 2013 16:32:01 Valentine wrote:
>>>> On 11/27/2013 04:14 PM, Hans Verkuil wrote:
>>
>>>>> Yes, of course. Although the adv7604 has two interrupt lines, so if you
>>>>> would want to use the second, then that would still have to be specified
>>>>> through the platform data.
>>>>
>>>> In this case the GPIO should be configured as interrupt source in the
>>>> platform code. But this doesn't seem to work with R-Car GPIO since it is
>>>> initialized later, and the gpio_to_irq function returns an error.
>>>> The simplest way seemed to use a GPIO number in the platform data
>>>> to have the adv driver configure the pin and request the IRQ.
>>>> I'm not sure how to easily defer I2C board info IRQ setup (and
>>>> camera/subdevice probing) until GPIO driver is ready.
>>>
>>> Good question. This looks like a core problem to me, not specific to the
>>> adv761x driver. Linus, Wolfram, do you have a comment on that ?
>>
>> So we recently has a large-ish discussion involving me, Stephen
>> Warren and Jean-Christophe, leading to the conclusion that the
>> gpio_chip and irq_chip abstractions are orthogonal, and you should
>> be able to request a GPIO or IRQ without interacting with the other
>> subsystem.
>>
>> Specifically you should be able to request an IRQ from the irq_chip
>> portions of the driver without first requesting the GPIO line.
>>
>> Some drivers already support this.
>>
>> We added an internal API to the gpiolib so that the lib, *internally*
>> can be made aware that a certain GPIO line is used for IRQs,
>> see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1
>> "gpio: add API to be strict about GPIO IRQ usage"
>>
>> So I guess the answer to the question is something like, fix
>> the GPIO driver to stop requiring the GPIO lines to be requested
>> and configured before being used as IRQs, delete that code,
>> and while you're at it add a call to gpiod_lock_as_irq()
>> to your GPIO driver in the right spot: examples are on the
>> mailing list and my mark-irqs branch in the GPIO tree.
>
> As far as I understand it this already works more or less with the driver.
> The problem is that the IRQ numbers are dynamically allocated, while the
> GPIO numbers apparently are not. So the board code knows the the GPIO number
> at compile time and can pass this to the diver which then does a gpio_to_irq
> to lookup the IRQ number.
This is correct.
> This of course isn't really a problem with
> devicetree, but only with platform board code.
I'm not sure what's the difference here and why it is not a problem with devicetree?
The other problem with R-Car GPIO driver is that it apparently does not support level IRQs.
>
> - Lars
>
Thanks,
Val.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] media: i2c: Add ADV761X support
2013-11-29 10:45 ` Lars-Peter Clausen
2013-11-29 12:14 ` Valentine
@ 2013-11-29 13:42 ` Linus Walleij
2013-11-29 13:48 ` Lars-Peter Clausen
1 sibling, 1 reply; 12+ messages in thread
From: Linus Walleij @ 2013-11-29 13:42 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 11/29/2013 11:37 AM, Linus Walleij wrote:
(...)
>> Specifically you should be able to request an IRQ from the irq_chip
>> portions of the driver without first requesting the GPIO line.
>>
>> Some drivers already support this.
>>
>> We added an internal API to the gpiolib so that the lib, *internally*
>> can be made aware that a certain GPIO line is used for IRQs,
>> see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1
>> "gpio: add API to be strict about GPIO IRQ usage"
>>
>> So I guess the answer to the question is something like, fix
>> the GPIO driver to stop requiring the GPIO lines to be requested
>> and configured before being used as IRQs, delete that code,
>> and while you're at it add a call to gpiod_lock_as_irq()
>> to your GPIO driver in the right spot: examples are on the
>> mailing list and my mark-irqs branch in the GPIO tree.
>
> As far as I understand it this already works more or less with the driver.
> The problem is that the IRQ numbers are dynamically allocated, while the
> GPIO numbers apparently are not. So the board code knows the the GPIO number
> at compile time and can pass this to the diver which then does a gpio_to_irq
> to lookup the IRQ number. This of course isn't really a problem with
> devicetree, but only with platform board code.
This has been solved *also* for platform board code by the new, fresh
GPIO descriptor mechanism, see Documentation/gpio/*
in Torvalds' git HEAD.
In your board file provide something like that:
http://marc.info/?l=linux-gpio&m=138546046203600&w=2
Then switch the driver to use the gpiod_* interface like:
http://marc.info/?l=linux-gpio&m=138546036028076&w=2
Problem solved.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] media: i2c: Add ADV761X support
2013-11-29 12:14 ` Valentine
@ 2013-11-29 13:46 ` Linus Walleij
0 siblings, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2013-11-29 13:46 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Nov 29, 2013 at 1:14 PM, Valentine
<valentine.barshak@cogentembedded.com> wrote:
> On 11/29/2013 02:45 PM, Lars-Peter Clausen wrote:
>> On 11/29/2013 11:37 AM, Linus Walleij wrote:
>>> So I guess the answer to the question is something like, fix
>>> the GPIO driver to stop requiring the GPIO lines to be requested
>>> and configured before being used as IRQs, delete that code,
>>> and while you're at it add a call to gpiod_lock_as_irq()
>>> to your GPIO driver in the right spot: examples are on the
>>> mailing list and my mark-irqs branch in the GPIO tree.
>>
>> As far as I understand it this already works more or less with the driver.
>> The problem is that the IRQ numbers are dynamically allocated, while the
>> GPIO numbers apparently are not. So the board code knows the the GPIO
>> number
>> at compile time and can pass this to the diver which then does a
>> gpio_to_irq
>> to lookup the IRQ number.
(...)
>> This of course isn't really a problem with
>> devicetree, but only with platform board code.
>
> I'm not sure what's the difference here and why it is not a problem with
> devicetree?
It's no problem when using devicetree because you can obtain
the GPIOs directly from the node with of_get_gpio()
and of_get_named_gpio() in the special DT probe path.
But don't do that! Instead switch the whole driver, and preferably
the whole platform, to use descriptors.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] media: i2c: Add ADV761X support
2013-11-29 13:42 ` Linus Walleij
@ 2013-11-29 13:48 ` Lars-Peter Clausen
2013-11-29 19:52 ` Linus Walleij
0 siblings, 1 reply; 12+ messages in thread
From: Lars-Peter Clausen @ 2013-11-29 13:48 UTC (permalink / raw)
To: linux-arm-kernel
On 11/29/2013 02:42 PM, Linus Walleij wrote:
> On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 11/29/2013 11:37 AM, Linus Walleij wrote:
> (...)
>>> Specifically you should be able to request an IRQ from the irq_chip
>>> portions of the driver without first requesting the GPIO line.
>>>
>>> Some drivers already support this.
>>>
>>> We added an internal API to the gpiolib so that the lib, *internally*
>>> can be made aware that a certain GPIO line is used for IRQs,
>>> see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1
>>> "gpio: add API to be strict about GPIO IRQ usage"
>>>
>>> So I guess the answer to the question is something like, fix
>>> the GPIO driver to stop requiring the GPIO lines to be requested
>>> and configured before being used as IRQs, delete that code,
>>> and while you're at it add a call to gpiod_lock_as_irq()
>>> to your GPIO driver in the right spot: examples are on the
>>> mailing list and my mark-irqs branch in the GPIO tree.
>>
>> As far as I understand it this already works more or less with the driver.
>> The problem is that the IRQ numbers are dynamically allocated, while the
>> GPIO numbers apparently are not. So the board code knows the the GPIO number
>> at compile time and can pass this to the diver which then does a gpio_to_irq
>> to lookup the IRQ number. This of course isn't really a problem with
>> devicetree, but only with platform board code.
>
> This has been solved *also* for platform board code by the new, fresh
> GPIO descriptor mechanism, see Documentation/gpio/*
> in Torvalds' git HEAD.
This works when the GPIO numbers are dynamically allocated (which are static
in this case), but not for IRQ numbers.
- Lars
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] media: i2c: Add ADV761X support
2013-11-29 13:48 ` Lars-Peter Clausen
@ 2013-11-29 19:52 ` Linus Walleij
2013-11-29 20:03 ` Laurent Pinchart
2013-11-29 20:05 ` Lars-Peter Clausen
0 siblings, 2 replies; 12+ messages in thread
From: Linus Walleij @ 2013-11-29 19:52 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Nov 29, 2013 at 2:48 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 11/29/2013 02:42 PM, Linus Walleij wrote:
>> On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>
>>> As far as I understand it this already works more or less with the driver.
>>> The problem is that the IRQ numbers are dynamically allocated, while the
>>> GPIO numbers apparently are not. So the board code knows the the GPIO number
>>> at compile time and can pass this to the diver which then does a gpio_to_irq
>>> to lookup the IRQ number. This of course isn't really a problem with
>>> devicetree, but only with platform board code.
>>
>> This has been solved *also* for platform board code by the new, fresh
>> GPIO descriptor mechanism, see Documentation/gpio/*
>> in Torvalds' git HEAD.
>
> This works when the GPIO numbers are dynamically allocated (which are static
> in this case), but not for IRQ numbers.
Sorry I don't get what you're after here. I'm not the subsystem
maintainer for IRQ chips ...
In the DT boot path for platform or AMBA devices the IRQs
are automatically resolved to resources and passed with the
device so that is certainly not the problem, right?
I guess you may be referring to the problem of instatiating
a dynamic IRQ chip in *board code* and then passing the
obtained dynamic IRQ numbers as resources to the
devices also created in a board file?
That would be like you're asking for a function that would
return the base of an irq_chip, that needs to be discussed
with the irq maintainers, so not much I can say, but maybe
I misunderstood this?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] media: i2c: Add ADV761X support
2013-11-29 19:52 ` Linus Walleij
@ 2013-11-29 20:03 ` Laurent Pinchart
2013-11-29 20:05 ` Lars-Peter Clausen
1 sibling, 0 replies; 12+ messages in thread
From: Laurent Pinchart @ 2013-11-29 20:03 UTC (permalink / raw)
To: linux-arm-kernel
Hi Linus,
On Friday 29 November 2013 20:52:17 Linus Walleij wrote:
> On Fri, Nov 29, 2013 at 2:48 PM, Lars-Peter Clausen wrote:
> > On 11/29/2013 02:42 PM, Linus Walleij wrote:
> >> On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen wrote:
> >>> As far as I understand it this already works more or less with the
> >>> driver.
> >>> The problem is that the IRQ numbers are dynamically allocated, while the
> >>> GPIO numbers apparently are not. So the board code knows the the GPIO
> >>> number at compile time and can pass this to the diver which then does a
> >>> gpio_to_irq to lookup the IRQ number. This of course isn't really a
> >>> problem with devicetree, but only with platform board code.
> >>
> >> This has been solved *also* for platform board code by the new, fresh
> >> GPIO descriptor mechanism, see Documentation/gpio/*
> >> in Torvalds' git HEAD.
> >
> > This works when the GPIO numbers are dynamically allocated (which are
> > static in this case), but not for IRQ numbers.
>
> Sorry I don't get what you're after here. I'm not the subsystem maintainer
> for IRQ chips ...
>
> In the DT boot path for platform or AMBA devices the IRQs are automatically
> resolved to resources and passed with the device so that is certainly not
> the problem, right?
>
> I guess you may be referring to the problem of instatiating a dynamic IRQ
> chip in *board code* and then passing the obtained dynamic IRQ numbers as
> resources to the devices also created in a board file?
What we need is to pass an IRQ number to the device driver through platform
device resources. When the adv761x IRQ line is connected to a GPIO (for which
we know the number), the natural way to do so it to call gpio_to_irq() to
convert the GPIO number to the IRQ number. However, calling this function in
board code will fail as the GPIO driver hasn't probed the GPIO device.
We could pass the GPIO number to the device through platform data, but if the
IRQ line is connected to a SoC dedicated IRQ input not handled by a GPIO
driver that won't work either. Furthermore, this would just be a workaround,
as the driver needs an IRQ and shouldn't have to care about GPIOs.
> That would be like you're asking for a function that would return the base
> of an irq_chip, that needs to be discussed with the irq maintainers, so not
> much I can say, but maybe I misunderstood this?
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] media: i2c: Add ADV761X support
2013-11-29 19:52 ` Linus Walleij
2013-11-29 20:03 ` Laurent Pinchart
@ 2013-11-29 20:05 ` Lars-Peter Clausen
2013-11-29 20:09 ` Linus Walleij
1 sibling, 1 reply; 12+ messages in thread
From: Lars-Peter Clausen @ 2013-11-29 20:05 UTC (permalink / raw)
To: linux-arm-kernel
On 11/29/2013 08:52 PM, Linus Walleij wrote:
> On Fri, Nov 29, 2013 at 2:48 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 11/29/2013 02:42 PM, Linus Walleij wrote:
>>> On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>
>>>> As far as I understand it this already works more or less with the driver.
>>>> The problem is that the IRQ numbers are dynamically allocated, while the
>>>> GPIO numbers apparently are not. So the board code knows the the GPIO number
>>>> at compile time and can pass this to the diver which then does a gpio_to_irq
>>>> to lookup the IRQ number. This of course isn't really a problem with
>>>> devicetree, but only with platform board code.
>>>
>>> This has been solved *also* for platform board code by the new, fresh
>>> GPIO descriptor mechanism, see Documentation/gpio/*
>>> in Torvalds' git HEAD.
>>
>> This works when the GPIO numbers are dynamically allocated (which are static
>> in this case), but not for IRQ numbers.
>
> Sorry I don't get what you're after here. I'm not the subsystem
> maintainer for IRQ chips ...
I'm trying to explain, that the problem is not about GPIO number lookup, but
rather about IRQ number lookup :)
>
> In the DT boot path for platform or AMBA devices the IRQs
> are automatically resolved to resources and passed with the
> device so that is certainly not the problem, right?
Yep, what I said earlier, this is a problem that's solved by using DT.
>
> I guess you may be referring to the problem of instatiating
> a dynamic IRQ chip in *board code* and then passing the
> obtained dynamic IRQ numbers as resources to the
> devices also created in a board file?
>
Yes.
> That would be like you're asking for a function that would
> return the base of an irq_chip, that needs to be discussed
> with the irq maintainers, so not much I can say, but maybe
> I misunderstood this?
I my opinion the best solution for this problem is to have the same lookup
mechanism we've had for clocks, regulators, etc and now also GPIOs.
- Lars
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] media: i2c: Add ADV761X support
2013-11-29 20:05 ` Lars-Peter Clausen
@ 2013-11-29 20:09 ` Linus Walleij
0 siblings, 0 replies; 12+ messages in thread
From: Linus Walleij @ 2013-11-29 20:09 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Nov 29, 2013 at 9:05 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 11/29/2013 08:52 PM, Linus Walleij wrote:
>> I guess you may be referring to the problem of instatiating
>> a dynamic IRQ chip in *board code* and then passing the
>> obtained dynamic IRQ numbers as resources to the
>> devices also created in a board file?
>>
>
> Yes.
>
>> That would be like you're asking for a function that would
>> return the base of an irq_chip, that needs to be discussed
>> with the irq maintainers, so not much I can say, but maybe
>> I misunderstood this?
>
> I my opinion the best solution for this problem is to have the same lookup
> mechanism we've had for clocks, regulators, etc and now also GPIOs.
Hm this needs to be discussed with some irq people...
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-11-29 20:09 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1384520071-16463-1-git-send-email-valentine.barshak@cogentembedded.com>
[not found] ` <5295E231.9030200@cisco.com>
[not found] ` <5295E641.6060603@cogentembedded.com>
2013-11-27 16:40 ` [PATCH V2] media: i2c: Add ADV761X support Laurent Pinchart
2013-11-27 16:48 ` Lars-Peter Clausen
2013-11-29 10:37 ` Linus Walleij
2013-11-29 10:45 ` Lars-Peter Clausen
2013-11-29 12:14 ` Valentine
2013-11-29 13:46 ` Linus Walleij
2013-11-29 13:42 ` Linus Walleij
2013-11-29 13:48 ` Lars-Peter Clausen
2013-11-29 19:52 ` Linus Walleij
2013-11-29 20:03 ` Laurent Pinchart
2013-11-29 20:05 ` Lars-Peter Clausen
2013-11-29 20:09 ` Linus Walleij
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).