[PATCH] of/i2c: support more interrupt specifiers

[PATCH] of/i2c: support more interrupt specifiers

[Bongkyu Kim]

This patch supports more interrupt specifiers for i2c client.

Signed-off-by: Bongkyu Kim <bongkyu.kim@lge.com>
---
 drivers/of/of_i2c.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
index 3550f3b..c6d9b7e 100644
--- a/drivers/of/of_i2c.c
+++ b/drivers/of/of_i2c.c
@@ -34,6 +34,7 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
 		struct dev_archdata dev_ad = {};
 		const __be32 *addr;
 		int len;
+		int nr = 0;
 
 		dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name);
 
@@ -57,7 +58,9 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
 			continue;
 		}
 
-		info.irq = irq_of_parse_and_map(node, 0);
+		info.irq = irq_of_parse_and_map(node, nr++);
+		while (irq_of_parse_and_map(node, nr))
+			nr++;
 		info.of_node = of_node_get(node);
 		info.archdata = &dev_ad;
 
-- 
1.8.0

Re: [PATCH] of/i2c: support more interrupt specifiers

[Rob Herring]

On 11/21/2012 02:58 AM, Bongkyu Kim wrote:
> This patch supports more interrupt specifiers for i2c client.

Why?

> Signed-off-by: Bongkyu Kim <bongkyu.kim@lge.com>
> ---
>  drivers/of/of_i2c.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> index 3550f3b..c6d9b7e 100644
> --- a/drivers/of/of_i2c.c
> +++ b/drivers/of/of_i2c.c
> @@ -34,6 +34,7 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
>  		struct dev_archdata dev_ad = {};
>  		const __be32 *addr;
>  		int len;
> +		int nr = 0;
>  
>  		dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name);
>  
> @@ -57,7 +58,9 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
>  			continue;
>  		}
>  
> -		info.irq = irq_of_parse_and_map(node, 0);
> +		info.irq = irq_of_parse_and_map(node, nr++);
> +		while (irq_of_parse_and_map(node, nr))
> +			nr++;

What's the point if you are just discarding the interrupt number? If
this is needed, then shouldn't you fix i2c_board_info first?

Rob

>  		info.of_node = of_node_get(node);
>  		info.archdata = &dev_ad;
>  
> 

RE: [PATCH] of/i2c: support more interrupt specifiers

[김봉규]

On 11/21/2012 10:41 PM, Rob Herring wrote:

> On 11/21/2012 02:58 AM, Bongkyu Kim wrote:
> > This patch supports more interrupt specifiers for i2c client.
> 
> Why?
> 

If we are registering a device has 2 interrupts like the below,
	i2c@f9968000 {
		lsm330_acc@1D {
			compatible = "ST,lsm330_acc",
			interrupts = <73 0x2 65 0x2>, /* #interrupt-cells =
<2> */
		};
	};

we can't use second irq(65) in a driver because of mapping first interrupt
only.
If second irq(65) is mapped, we can use by gpio_to_irq() function at a
driver.
So, I think that all interrupt specifiers should be parsed and mapped
if DT's interrupts property is allowed several interrupt specifiers.

> > Signed-off-by: Bongkyu Kim <bongkyu.kim-Hm3cg6mZ9cc@public.gmane.org>
> > ---
> >  drivers/of/of_i2c.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
> > index 3550f3b..c6d9b7e 100644
> > --- a/drivers/of/of_i2c.c
> > +++ b/drivers/of/of_i2c.c
> > @@ -34,6 +34,7 @@ void of_i2c_register_devices(struct i2c_adapter
> *adap)
> >  		struct dev_archdata dev_ad = {};
> >  		const __be32 *addr;
> >  		int len;
> > +		int nr = 0;
> >
> >  		dev_dbg(&adap->dev, "of_i2c: register %s\n", node-
> >full_name);
> >
> > @@ -57,7 +58,9 @@ void of_i2c_register_devices(struct i2c_adapter
> *adap)
> >  			continue;
> >  		}
> >
> > -		info.irq = irq_of_parse_and_map(node, 0);
> > +		info.irq = irq_of_parse_and_map(node, nr++);
> > +		while (irq_of_parse_and_map(node, nr))
> > +			nr++;
> 
> What's the point if you are just discarding the interrupt number? If
> this is needed, then shouldn't you fix i2c_board_info first?
> 
> Rob
> 

It is better that i2c_board_info will be fixed.
But, if it will be fixed, all i2c client drivers should be changed.
I think there're so many i2c drivers currently.
In my opinion, do not assign to i2c_board_info and just map for all
interrupts is the better.

Thanks,
Bongkyu.

> >  		info.of_node = of_node_get(node);
> >  		info.archdata = &dev_ad;
> >
> >

Re: [PATCH] of/i2c: support more interrupt specifiers

[Bongkyu Kim]

On 11/21/2012 10:41 PM, Rob Herring wrote:
> On 11/21/2012 02:58 AM, Bongkyu Kim wrote:
>> This patch supports more interrupt specifiers for i2c client.
> 
> Why?
> 

If we are registering a device has 2 interrupts like the below,
	i2c@f9968000 {
		lsm330_acc@1D {
			compatible = "ST,lsm330_acc",
			/* #interrupt-cells = <2> */
			interrupts = <73 0x2 65 0x2>,
		};
	};

we can't use second irq(65) in a driver because of mapping first
interrupt only.
If second irq(65) is mapped, we can use by gpio_to_irq() function at a
driver.
So, I think that all interrupt specifiers should be parsed and mapped
if DT's interrupts property is allowed several interrupt specifiers.

>> Signed-off-by: Bongkyu Kim <bongkyu.kim@lge.com>
>> ---
>>  drivers/of/of_i2c.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/of_i2c.c b/drivers/of/of_i2c.c
>> index 3550f3b..c6d9b7e 100644
>> --- a/drivers/of/of_i2c.c
>> +++ b/drivers/of/of_i2c.c
>> @@ -34,6 +34,7 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
>>  		struct dev_archdata dev_ad = {};
>>  		const __be32 *addr;
>>  		int len;
>> +		int nr = 0;
>>  
>>  		dev_dbg(&adap->dev, "of_i2c: register %s\n", node->full_name);
>>  
>> @@ -57,7 +58,9 @@ void of_i2c_register_devices(struct i2c_adapter *adap)
>>  			continue;
>>  		}
>>  
>> -		info.irq = irq_of_parse_and_map(node, 0);
>> +		info.irq = irq_of_parse_and_map(node, nr++);
>> +		while (irq_of_parse_and_map(node, nr))
>> +			nr++;
> 
> What's the point if you are just discarding the interrupt number? If
> this is needed, then shouldn't you fix i2c_board_info first?
> 
> Rob
> 

It is better that i2c_board_info will be fixed.
But, if it will be fixed, all i2c client drivers should be changed.
I think there're so many i2c drivers currently.
In my opinion, do not assign to i2c_board_info and just map for all
interrupts is the better.

Thanks,
Bongkyu.

>>  		info.of_node = of_node_get(node);
>>  		info.archdata = &dev_ad;
>>  
>>
> 

Re: [PATCH] of/i2c: support more interrupt specifiers

[Grant Likely]

On Mon, 26 Nov 2012 14:11:27 +0900, Bongkyu Kim <bongkyu.kim@lge.com> wrote:
> On 11/21/2012 10:41 PM, Rob Herring wrote:
> > On 11/21/2012 02:58 AM, Bongkyu Kim wrote:
> >> This patch supports more interrupt specifiers for i2c client.
> > 
> > Why?
> > 
> 
> If we are registering a device has 2 interrupts like the below,
> 	i2c@f9968000 {
> 		lsm330_acc@1D {
> 			compatible = "ST,lsm330_acc",
> 			/* #interrupt-cells = <2> */
> 			interrupts = <73 0x2 65 0x2>,
> 		};
> 	};
> 
> we can't use second irq(65) in a driver because of mapping first
> interrupt only.
> If second irq(65) is mapped, we can use by gpio_to_irq() function at a
> driver.
> So, I think that all interrupt specifiers should be parsed and mapped
> if DT's interrupts property is allowed several interrupt specifiers.

This doesn't look like a very good idea. If more than one interrupt is
provided, then each should be entirely parsed in the driver's .probe() or
in of_i2c_register_devices(), not split across the two. Using
gpio_to_irq() to fetch an irq number that was already calculated and
then thrown away is a bizarre back way of going about it. Plus it is
error-probe because nothing will check that the gpio and interrupts
properties agree.

Either the driver should do all the parsing of 2nd and subsequent irqs,
or the i2c subsystem should be extended to support multiple irq
resources.

g.