Problems with commit 9ec36cafe4 (of/irq: do irq resolution in platform_get_irq) and mfd client devices

Problems with commit 9ec36cafe4 (of/irq: do irq resolution in platform_get_irq) and mfd client devices

[Guenter Roeck]

Hi,

I have an mfd master and client drivers on a system which has devicetree
enabled. The mfd master driver passes interrupts to the clients using 
mfd cells and 'struct resource'. The client driver is a platform driver
which retrieves the irq using platform_get_irq().

After commit 9ec36cafe (of/irq: do irq resolution in platform_get_irq),
this code no longer works. This is because platform_get_irq() does no
longer call platform_get_resource() if OF is enabled and if dev->of_node
is not NULL (it is not NULL because there is other [static] information
which is passed to the client with devicetree data). 

Any idea how to solve this problem ? How do I now pass a virtual interrupt
from an mfd master to its clients if devicetree is enabled ?

Thanks,
Guenter

Re: Problems with commit 9ec36cafe4 (of/irq: do irq resolution in platform_get_irq) and mfd client devices

[Rob Herring]

On Tue, Jun 17, 2014 at 1:10 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> Hi,
>
> I have an mfd master and client drivers on a system which has devicetree
> enabled. The mfd master driver passes interrupts to the clients using
> mfd cells and 'struct resource'. The client driver is a platform driver
> which retrieves the irq using platform_get_irq().
>
> After commit 9ec36cafe (of/irq: do irq resolution in platform_get_irq),
> this code no longer works. This is because platform_get_irq() does no
> longer call platform_get_resource() if OF is enabled and if dev->of_node
> is not NULL (it is not NULL because there is other [static] information
> which is passed to the client with devicetree data).
>
> Any idea how to solve this problem ? How do I now pass a virtual interrupt
> from an mfd master to its clients if devicetree is enabled ?

The node ptr points to the MFD node or a child node? If there are
child nodes in DT, then why not define interrupts there too? If there
are not child nodes, then perhaps the child drivers should not have DT
knowledge.

Does it fail to get an interrupt or gets the parent interrupt instead?

We could probably make an error fall-back to looking at resources. Or
try to get irq from resources first, then call of_irq_get.

Rob

Re: Problems with commit 9ec36cafe4 (of/irq: do irq resolution in platform_get_irq) and mfd client devices

[Guenter Roeck]

On Tue, Jun 17, 2014 at 03:08:24PM -0500, Rob Herring wrote:
> On Tue, Jun 17, 2014 at 1:10 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > Hi,
> >
> > I have an mfd master and client drivers on a system which has devicetree
> > enabled. The mfd master driver passes interrupts to the clients using
> > mfd cells and 'struct resource'. The client driver is a platform driver
> > which retrieves the irq using platform_get_irq().
> >
> > After commit 9ec36cafe (of/irq: do irq resolution in platform_get_irq),
> > this code no longer works. This is because platform_get_irq() does no
> > longer call platform_get_resource() if OF is enabled and if dev->of_node
> > is not NULL (it is not NULL because there is other [static] information
> > which is passed to the client with devicetree data).
> >
> > Any idea how to solve this problem ? How do I now pass a virtual interrupt
> > from an mfd master to its clients if devicetree is enabled ?
> 
> The node ptr points to the MFD node or a child node? If there are
> child nodes in DT, then why not define interrupts there too? If there
> are not child nodes, then perhaps the child drivers should not have DT
> knowledge.
> 
There is a whole bunch of secondary data in the child's dt node.
One of the child/client drivers is an i2c controller with attached
i2c muxes and several i2c devices, another is a gpio controller
with a large number of gpio pins which itself acts as interrupt
controller.

> Does it fail to get an interrupt or gets the parent interrupt instead?
> 
It fails to get an interrupt and returns -EINVAL.

> We could probably make an error fall-back to looking at resources. Or
> try to get irq from resources first, then call of_irq_get.
> 
I submitted a patch implementing the first approach a few minutes ago.
That fixes the problem for me. Not sure if that is the right solution
though, as it doesn't handle -EPROBE_DEFER. Let me know when you see
the patch if there is a better way to handle it (maybe abort with
-EPROBE_DEFER if of_irq_get returns it would do).

Guenter

Re: Problems with commit 9ec36cafe4 (of/irq: do irq resolution in platform_get_irq) and mfd client devices

[Rob Herring]

On Tue, Jun 17, 2014 at 3:29 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On Tue, Jun 17, 2014 at 03:08:24PM -0500, Rob Herring wrote:
>> On Tue, Jun 17, 2014 at 1:10 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > Hi,
>> >
>> > I have an mfd master and client drivers on a system which has devicetree
>> > enabled. The mfd master driver passes interrupts to the clients using
>> > mfd cells and 'struct resource'. The client driver is a platform driver
>> > which retrieves the irq using platform_get_irq().
>> >
>> > After commit 9ec36cafe (of/irq: do irq resolution in platform_get_irq),
>> > this code no longer works. This is because platform_get_irq() does no
>> > longer call platform_get_resource() if OF is enabled and if dev->of_node
>> > is not NULL (it is not NULL because there is other [static] information
>> > which is passed to the client with devicetree data).
>> >
>> > Any idea how to solve this problem ? How do I now pass a virtual interrupt
>> > from an mfd master to its clients if devicetree is enabled ?
>>
>> The node ptr points to the MFD node or a child node? If there are
>> child nodes in DT, then why not define interrupts there too? If there
>> are not child nodes, then perhaps the child drivers should not have DT
>> knowledge.
>>
> There is a whole bunch of secondary data in the child's dt node.
> One of the child/client drivers is an i2c controller with attached
> i2c muxes and several i2c devices, another is a gpio controller
> with a large number of gpio pins which itself acts as interrupt
> controller.

Then why not put interrupt data into the child nodes?

>> Does it fail to get an interrupt or gets the parent interrupt instead?
>>
> It fails to get an interrupt and returns -EINVAL.
>
>> We could probably make an error fall-back to looking at resources. Or
>> try to get irq from resources first, then call of_irq_get.
>>
> I submitted a patch implementing the first approach a few minutes ago.
> That fixes the problem for me. Not sure if that is the right solution
> though, as it doesn't handle -EPROBE_DEFER. Let me know when you see
> the patch if there is a better way to handle it (maybe abort with
> -EPROBE_DEFER if of_irq_get returns it would do).

Humm, don't see it. In any case, you would need to fall back only if
the error is not EPROBE_DEFER.

Rob

Re: Problems with commit 9ec36cafe4 (of/irq: do irq resolution in platform_get_irq) and mfd client devices

[Guenter Roeck]

On 06/17/2014 02:39 PM, Rob Herring wrote:
> On Tue, Jun 17, 2014 at 3:29 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Tue, Jun 17, 2014 at 03:08:24PM -0500, Rob Herring wrote:
>>> On Tue, Jun 17, 2014 at 1:10 PM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>> Hi,
>>>>
>>>> I have an mfd master and client drivers on a system which has devicetree
>>>> enabled. The mfd master driver passes interrupts to the clients using
>>>> mfd cells and 'struct resource'. The client driver is a platform driver
>>>> which retrieves the irq using platform_get_irq().
>>>>
>>>> After commit 9ec36cafe (of/irq: do irq resolution in platform_get_irq),
>>>> this code no longer works. This is because platform_get_irq() does no
>>>> longer call platform_get_resource() if OF is enabled and if dev->of_node
>>>> is not NULL (it is not NULL because there is other [static] information
>>>> which is passed to the client with devicetree data).
>>>>
>>>> Any idea how to solve this problem ? How do I now pass a virtual interrupt
>>>> from an mfd master to its clients if devicetree is enabled ?
>>>
>>> The node ptr points to the MFD node or a child node? If there are
>>> child nodes in DT, then why not define interrupts there too? If there
>>> are not child nodes, then perhaps the child drivers should not have DT
>>> knowledge.
>>>
>> There is a whole bunch of secondary data in the child's dt node.
>> One of the child/client drivers is an i2c controller with attached
>> i2c muxes and several i2c devices, another is a gpio controller
>> with a large number of gpio pins which itself acts as interrupt
>> controller.
>
> Then why not put interrupt data into the child nodes?
>

Seems to me how interrupts are passed on between mfd master and child driver
(and the fact that there _is_ a mfd master and slave driver) is implementation
dependent. I thought that implementation dependencies are not supposed
to be expressed in devicetree data. Maybe that changed.

Even then it seems to me that if we can no longer rely on resource data
to pass information between mfd master and slaves under some circumstances,
this needs to be documented. Bold and underlined. Also, the new method
of passing such information needs to be described somewhere. Expecting
mfd driver implementers to figure this out on their own seems a bit
heavy handed - even more so since the patches are being backported
to older kernels (which is how I found out). Sure, the code I am dealing
with is (at least for now) not in the upstream kernel, but introducing
a subtle API change like this one as part of a stable release is really
not appropriate. Or at least I think so.

>>> Does it fail to get an interrupt or gets the parent interrupt instead?
>>>
>> It fails to get an interrupt and returns -EINVAL.
>>
>>> We could probably make an error fall-back to looking at resources. Or
>>> try to get irq from resources first, then call of_irq_get.
>>>
>> I submitted a patch implementing the first approach a few minutes ago.
>> That fixes the problem for me. Not sure if that is the right solution
>> though, as it doesn't handle -EPROBE_DEFER. Let me know when you see
>> the patch if there is a better way to handle it (maybe abort with
>> -EPROBE_DEFER if of_irq_get returns it would do).
>
> Humm, don't see it. In any case, you would need to fall back only if
> the error is not EPROBE_DEFER.
>
Weird, I don't see the patch on lkml either. I'll resend with the correction.

Guenter