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

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