Hello,
I recently ran into an issue with the OF IRQ parsing code in the I2C core
(of_i2c_register_devices in drivers/i2c/i2c-core.c).
My DT contains the following nodes.
gpio1: gpio@e6051000 {
...
#interrupt-cells = <2>;
interrupt-controller;
clocks = <&mstp9_clks R8A7790_CLK_GPIO1>;
};
iic2: i2c@e6520000 {
#address-cells = <1>;
#size-cells = <0>;
...
hdmi@39 {
compatible = "adi,adv7511w";
reg = <0x39>;
interrupt-parent = <&gpio1>;
interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
...
};
};
mstp9_clks: mstp9_clks@e6150994 {
...
};
The i2c@e6520000 node is probed before the gpio@e6051000 node. The
of_i2c_register_devices() function tries to register all children, including
hdmi@39. It tries to parse and map the I2C client IRQ by calling
irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't
probed yet. The adv7511 driver later probes the hdmi@39 device and gets
client->irq set to 0.
As we can't control the probe order in the general case we need to rely on
deferred probing. This patch series is an attempt to do so, by moving IRQ
mapping from device registration time to device probe time were probing can be
deferred.
Laurent Pinchart (3):
of/irq: Export of_irq_get()
i2c: core: Dispose OF IRQ mapping at client removal time
i2c: core: Map OF IRQ at probe time
drivers/i2c/i2c-core.c | 14 ++++++++++++--
drivers/of/irq.c | 1 +
2 files changed, 13 insertions(+), 2 deletions(-)
--
Regards,
Laurent Pinchart
The function will be used by the I2C core which can be compiled as a
module.
Signed-off-by: Laurent Pinchart <[email protected]>
---
drivers/of/irq.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 1471e0a223a5..0d7765807f49 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -405,6 +405,7 @@ int of_irq_get(struct device_node *dev, int index)
return irq_create_of_mapping(&oirq);
}
+EXPORT_SYMBOL_GPL(of_irq_get);
/**
* of_irq_get_byname - Decode a node's IRQ and return it as a Linux irq number
--
2.0.4
Clients instantiated from OF get an IRQ mapping created at device
registration time. Dispose the mapping when the client is removed.
Signed-off-by: Laurent Pinchart <[email protected]>
---
drivers/i2c/i2c-core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6a7f79..258765b29684 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -670,6 +670,9 @@ static int i2c_device_remove(struct device *dev)
status = driver->remove(client);
}
+ if (dev->of_node)
+ irq_dispose_mapping(client->irq);
+
dev_pm_domain_detach(&client->dev, true);
return status;
}
--
2.0.4
I2C clients instantiated from OF get their IRQ mapped at device
registration time. This leads to the IRQ being silently ignored if the
related irqchip hasn't been proved yet.
Fix this by moving IRQ mapping at probe time using of_get_irq(). The
function operates as irq_of_parse_and_map() but additionally returns
-EPROBE_DEFER if the irqchip isn't available, allowing us to defer I2C
client probing.
Signed-off-by: Laurent Pinchart <[email protected]>
---
drivers/i2c/i2c-core.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 258765b29684..c6694f232240 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -631,6 +631,15 @@ static int i2c_device_probe(struct device *dev)
if (!client)
return 0;
+ if (!client->irq && dev->of_node) {
+ int irq = of_irq_get(dev->of_node, 0);
+
+ if (irq < 0)
+ return irq;
+
+ client->irq = irq;
+ }
+
driver = to_i2c_driver(dev->driver);
if (!driver->probe || !driver->id_table)
return -ENODEV;
@@ -1412,7 +1421,6 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
continue;
}
- info.irq = irq_of_parse_and_map(node, 0);
info.of_node = of_node_get(node);
info.archdata = &dev_ad;
@@ -1426,7 +1434,6 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
node->full_name);
of_node_put(node);
- irq_dispose_mapping(info.irq);
continue;
}
}
--
2.0.4
On Thu, Oct 30, 2014 at 03:59:36PM +0200, Laurent Pinchart wrote:
> The function will be used by the I2C core which can be compiled as a
> module.
>
> Signed-off-by: Laurent Pinchart <[email protected]>
I think it makes sense if I take this via I2C to get the dependencies
for the later patches right?
> ---
> drivers/of/irq.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 1471e0a223a5..0d7765807f49 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -405,6 +405,7 @@ int of_irq_get(struct device_node *dev, int index)
>
> return irq_create_of_mapping(&oirq);
> }
> +EXPORT_SYMBOL_GPL(of_irq_get);
>
> /**
> * of_irq_get_byname - Decode a node's IRQ and return it as a Linux irq number
> --
> 2.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 30 October 2014 15:16:44 Wolfram Sang wrote:
> On Thu, Oct 30, 2014 at 03:59:36PM +0200, Laurent Pinchart wrote:
> > The function will be used by the I2C core which can be compiled as a
> > module.
> >
> > Signed-off-by: Laurent Pinchart
> > <[email protected]>
>
> I think it makes sense if I take this via I2C to get the dependencies
> for the later patches right?
It would be easier, yes.
> > ---
> >
> > drivers/of/irq.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index 1471e0a223a5..0d7765807f49 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -405,6 +405,7 @@ int of_irq_get(struct device_node *dev, int index)
> >
> > return irq_create_of_mapping(&oirq);
> >
> > }
> >
> > +EXPORT_SYMBOL_GPL(of_irq_get);
> >
> > /**
> >
> > * of_irq_get_byname - Decode a node's IRQ and return it as a Linux irq
> > number
--
Regards,
Laurent Pinchart
On Thu, Oct 30, 2014 at 03:59:37PM +0200, Laurent Pinchart wrote:
> Clients instantiated from OF get an IRQ mapping created at device
> registration time. Dispose the mapping when the client is removed.
>
> Signed-off-by: Laurent Pinchart <[email protected]>
> ---
> drivers/i2c/i2c-core.c | 3 +++
> 1 file changed, 3 insertions(+)
If this is needed regardless of patch 3/3, then presumably it should be
Cc'ed to [email protected] since it fixes a bug that's been there
for quite some time?
Thierry
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 2f90ac6a7f79..258765b29684 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -670,6 +670,9 @@ static int i2c_device_remove(struct device *dev)
> status = driver->remove(client);
> }
>
> + if (dev->of_node)
> + irq_dispose_mapping(client->irq);
> +
> dev_pm_domain_detach(&client->dev, true);
> return status;
> }
> --
> 2.0.4
>
On Thu, Oct 30, 2014 at 03:59:35PM +0200, Laurent Pinchart wrote:
> Hello,
>
> I recently ran into an issue with the OF IRQ parsing code in the I2C core
> (of_i2c_register_devices in drivers/i2c/i2c-core.c).
>
> My DT contains the following nodes.
>
> gpio1: gpio@e6051000 {
> ...
> #interrupt-cells = <2>;
> interrupt-controller;
> clocks = <&mstp9_clks R8A7790_CLK_GPIO1>;
> };
>
> iic2: i2c@e6520000 {
> #address-cells = <1>;
> #size-cells = <0>;
> ...
> hdmi@39 {
> compatible = "adi,adv7511w";
> reg = <0x39>;
> interrupt-parent = <&gpio1>;
> interrupts = <15 IRQ_TYPE_EDGE_FALLING>;
> ...
> };
> };
>
> mstp9_clks: mstp9_clks@e6150994 {
> ...
> };
>
> The i2c@e6520000 node is probed before the gpio@e6051000 node. The
> of_i2c_register_devices() function tries to register all children, including
> hdmi@39. It tries to parse and map the I2C client IRQ by calling
> irq_of_parse_and_map(), which returns 0 as the interrupt controller isn't
> probed yet. The adv7511 driver later probes the hdmi@39 device and gets
> client->irq set to 0.
>
> As we can't control the probe order in the general case we need to rely on
> deferred probing. This patch series is an attempt to do so, by moving IRQ
> mapping from device registration time to device probe time were probing can be
> deferred.
>
> Laurent Pinchart (3):
> of/irq: Export of_irq_get()
> i2c: core: Dispose OF IRQ mapping at client removal time
> i2c: core: Map OF IRQ at probe time
>
> drivers/i2c/i2c-core.c | 14 ++++++++++++--
> drivers/of/irq.c | 1 +
> 2 files changed, 13 insertions(+), 2 deletions(-)
The series:
Reviewed-by: Thierry Reding <[email protected]>
On Thu, Oct 30, 2014 at 04:17:19PM +0200, Laurent Pinchart wrote:
> On Thursday 30 October 2014 15:16:44 Wolfram Sang wrote:
> > On Thu, Oct 30, 2014 at 03:59:36PM +0200, Laurent Pinchart wrote:
> > > The function will be used by the I2C core which can be compiled as a
> > > module.
> > >
> > > Signed-off-by: Laurent Pinchart
> > > <[email protected]>
> >
> > I think it makes sense if I take this via I2C to get the dependencies
> > for the later patches right?
>
> It would be easier, yes.
Oh, DT maintainers are not on CC. Then I can wait pretty long for an
ack ;) Fixing that.
>
> > > ---
> > >
> > > drivers/of/irq.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > > index 1471e0a223a5..0d7765807f49 100644
> > > --- a/drivers/of/irq.c
> > > +++ b/drivers/of/irq.c
> > > @@ -405,6 +405,7 @@ int of_irq_get(struct device_node *dev, int index)
> > >
> > > return irq_create_of_mapping(&oirq);
> > >
> > > }
> > >
> > > +EXPORT_SYMBOL_GPL(of_irq_get);
> > >
> > > /**
> > >
> > > * of_irq_get_byname - Decode a node's IRQ and return it as a Linux irq
> > > number
>
> --
> Regards,
>
> Laurent Pinchart
>
On Sat, Nov 1, 2014 at 2:20 AM, Wolfram Sang <[email protected]> wrote:
> On Thu, Oct 30, 2014 at 04:17:19PM +0200, Laurent Pinchart wrote:
>> On Thursday 30 October 2014 15:16:44 Wolfram Sang wrote:
>> > On Thu, Oct 30, 2014 at 03:59:36PM +0200, Laurent Pinchart wrote:
>> > > The function will be used by the I2C core which can be compiled as a
>> > > module.
>> > >
>> > > Signed-off-by: Laurent Pinchart
>> > > <[email protected]>
>> >
>> > I think it makes sense if I take this via I2C to get the dependencies
>> > for the later patches right?
>>
>> It would be easier, yes.
>
> Oh, DT maintainers are not on CC. Then I can wait pretty long for an
> ack ;) Fixing that.
Acked-by: Rob Herring <[email protected]>
>
>>
>> > > ---
>> > >
>> > > drivers/of/irq.c | 1 +
>> > > 1 file changed, 1 insertion(+)
>> > >
>> > > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
>> > > index 1471e0a223a5..0d7765807f49 100644
>> > > --- a/drivers/of/irq.c
>> > > +++ b/drivers/of/irq.c
>> > > @@ -405,6 +405,7 @@ int of_irq_get(struct device_node *dev, int index)
>> > >
>> > > return irq_create_of_mapping(&oirq);
>> > >
>> > > }
>> > >
>> > > +EXPORT_SYMBOL_GPL(of_irq_get);
>> > >
>> > > /**
>> > >
>> > > * of_irq_get_byname - Decode a node's IRQ and return it as a Linux irq
>> > > number
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
On Thu, 30 Oct 2014 15:16:44 +0100
, Wolfram Sang <[email protected]>
wrote:
> On Thu, Oct 30, 2014 at 03:59:36PM +0200, Laurent Pinchart wrote:
> > The function will be used by the I2C core which can be compiled as a
> > module.
> >
> > Signed-off-by: Laurent Pinchart <[email protected]>
>
> I think it makes sense if I take this via I2C to get the dependencies
> for the later patches right?
Agreed.
Acked-by: Grant Likely <[email protected]>
>
> > ---
> > drivers/of/irq.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> > index 1471e0a223a5..0d7765807f49 100644
> > --- a/drivers/of/irq.c
> > +++ b/drivers/of/irq.c
> > @@ -405,6 +405,7 @@ int of_irq_get(struct device_node *dev, int index)
> >
> > return irq_create_of_mapping(&oirq);
> > }
> > +EXPORT_SYMBOL_GPL(of_irq_get);
> >
> > /**
> > * of_irq_get_byname - Decode a node's IRQ and return it as a Linux irq number
> > --
> > 2.0.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 30, 2014 at 03:59:37PM +0200, Laurent Pinchart wrote:
> Clients instantiated from OF get an IRQ mapping created at device
> registration time. Dispose the mapping when the client is removed.
>
> Signed-off-by: Laurent Pinchart <[email protected]>
Added stable and applied to for-current, thanks!
On Thu, Oct 30, 2014 at 03:59:36PM +0200, Laurent Pinchart wrote:
> The function will be used by the I2C core which can be compiled as a
> module.
>
> Signed-off-by: Laurent Pinchart <[email protected]>
Applied to for-next, thanks!
On Thu, Oct 30, 2014 at 03:59:38PM +0200, Laurent Pinchart wrote:
> I2C clients instantiated from OF get their IRQ mapped at device
> registration time. This leads to the IRQ being silently ignored if the
> related irqchip hasn't been proved yet.
>
> Fix this by moving IRQ mapping at probe time using of_get_irq(). The
> function operates as irq_of_parse_and_map() but additionally returns
> -EPROBE_DEFER if the irqchip isn't available, allowing us to defer I2C
> client probing.
>
> Signed-off-by: Laurent Pinchart <[email protected]>
Applied to for-next, thanks!
Hi Laurent,
On Thu, Oct 30, 2014 at 2:59 PM, Laurent Pinchart
<[email protected]> wrote:
> I2C clients instantiated from OF get their IRQ mapped at device
> registration time. This leads to the IRQ being silently ignored if the
> related irqchip hasn't been proved yet.
>
> Fix this by moving IRQ mapping at probe time using of_get_irq(). The
> function operates as irq_of_parse_and_map() but additionally returns
> -EPROBE_DEFER if the irqchip isn't available, allowing us to defer I2C
> client probing.
>
> Signed-off-by: Laurent Pinchart <[email protected]>
> ---
> drivers/i2c/i2c-core.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 258765b29684..c6694f232240 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -631,6 +631,15 @@ static int i2c_device_probe(struct device *dev)
> if (!client)
> return 0;
>
> + if (!client->irq && dev->of_node) {
> + int irq = of_irq_get(dev->of_node, 0);
> +
> + if (irq < 0)
> + return irq;
This change broke all i2c slaves not having interrupts (e.g. da9210 and at24
on r8a7791/koelsch), which now fail to probe:
-DA9210: 1000 mV at 4600 mA
...
i2c /dev entries driver
-at24 2-0050: 256 byte 24c02 EEPROM, writable, 16 bytes/write
+at24: probe of 2-0050 failed with error -22
i2c-rcar e6530000.i2c: probed
...
+da9210: probe of 6-0068 failed with error -22
+i2c-sh_mobile e60b0000.i2c: I2C adapter 6, bus speed 100000 Hz, DMA=y
i2c_device_probe() fails because of_irq_get() returns -EINVAL,
originating from of_irq_parse_one() not finding an "interrupts" property
(there is none).
Apparently you overlooked a difference between irq_of_parse_and_map()
and of_get_irq(): the former returns 0 if there's no "interrupts" property,
while the latter forwards the error code from of_irq_parse_one.
I see two ways to fix this:
1. Make of_get_irq() return 0 if no "interrupts" property was found, to
make it behave more similar to irq_of_parse_and_map().
Does any code rely on the error forwarding?
2. Make i2c_device_probe() only return if -EPROBE_DEFER, and ignore
all other error codes.
It seems irq_create_of_mapping() never returns an error code, but
0 in case of failures?
As platform_get_irq{,_byname}() do the latter, I'll cook a patch
to do that.
> + client->irq = irq;
> + }
> +
> driver = to_i2c_driver(dev->driver);
> if (!driver->probe || !driver->id_table)
> return -ENODEV;
> @@ -1412,7 +1421,6 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
> continue;
> }
>
> - info.irq = irq_of_parse_and_map(node, 0);
> info.of_node = of_node_get(node);
> info.archdata = &dev_ad;
>
> @@ -1426,7 +1434,6 @@ static void of_i2c_register_devices(struct i2c_adapter *adap)
> dev_err(&adap->dev, "of_i2c: Failure registering %s\n",
> node->full_name);
> of_node_put(node);
> - irq_dispose_mapping(info.irq);
> continue;
> }
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds