[PATCH 0/3] Delay I2C OF IRQ mapping until probe time

[PATCH 0/3] Delay I2C OF IRQ mapping until probe time

[Laurent Pinchart]

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

[PATCH 1/3] of/irq: Export of_irq_get()

[Laurent Pinchart]

The function will be used by the I2C core which can be compiled as a
module.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 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

Re: [PATCH 1/3] of/irq: Export of_irq_get()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

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 <laurent.pinchart+renesas@ideasonboard.com>

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] of/irq: Export of_irq_get()

[Laurent Pinchart]

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
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> 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

Re: [PATCH 1/3] of/irq: Export of_irq_get()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

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
> > > <laurent.pinchart+renesas@ideasonboard.com>
> > 
> > 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
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] of/irq: Export of_irq_get()

[Rob Herring]

On Sat, Nov 1, 2014 at 2:20 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> 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
>> > > <laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>> >
>> > 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 <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

>
>>
>> > > ---
>> > >
>> > >  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
>>

Re: [PATCH 1/3] of/irq: Export of_irq_get()

[Grant Likely]

On Thu, 30 Oct 2014 15:16:44 +0100
, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
 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 <laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> 
> 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 <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> 
> > ---
> >  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] of/irq: Export of_irq_get()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 291 bytes --]

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 <laurent.pinchart+renesas-ryLnwIuWjnhk3lzF8UVTdg@public.gmane.orgm>

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/3] Delay I2C OF IRQ mapping until probe time

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2071 bytes --]

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 <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/3] i2c: core: Dispose OF IRQ mapping at client removal time

[Laurent Pinchart]

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 <laurent.pinchart+renesas@ideasonboard.com>
---
 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

Re: [PATCH 2/3] i2c: core: Dispose OF IRQ mapping at client removal time

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1052 bytes --]

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 <laurent.pinchart+renesas-ryLnwIuWjnhk3lzF8UVTdg@public.gmane.orgm>
> ---
>  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 stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] i2c: core: Dispose OF IRQ mapping at client removal time

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 367 bytes --]

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 <laurent.pinchart+renesas-ryLnwIuWjnhk3lzF8UVTdg@public.gmane.orgm>

Added stable and applied to for-current, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 3/3] i2c: core: Map OF IRQ at probe time

[Laurent Pinchart]

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 <laurent.pinchart+renesas@ideasonboard.com>
---
 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

Re: [PATCH 3/3] i2c: core: Map OF IRQ at probe time

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

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 <laurent.pinchart+renesas-ryLnwIuWjnhk3lzF8UVTdg@public.gmane.orgm>

Applied to for-next, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] i2c: core: Map OF IRQ at probe time

[Geert Uytterhoeven]

Hi Laurent,

On Thu, Oct 30, 2014 at 2:59 PM, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> 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 <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  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 -- geert@linux-m68k.org

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