IRQs in i2c-mpc.c

IRQs in i2c-mpc.c

[Jon Smirl]

I'm doing final clean up the i2c open firmware support. What is the
intention of this code? Is it meant for a missing IRQ attribute to
return a zero and not get an IRQ for the device? Does the device
function without an IRQ? If so, it needs a comment explaining things.
Otherwise IRQ of zero should be an error.

	i2c->irq = irq_of_parse_and_map(op->node, 0);
	if (i2c->irq <= 0) {
		result = -ENXIO;
		goto fail_irq;
	}
	
	if (i2c->irq != 0)
		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
					  	IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
			printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
			goto fail_irq;
		}

-- 
Jon Smirl
jonsmirl@gmail.com

Re: IRQs in i2c-mpc.c

[Kumar Gala]

On Nov 10, 2007, at 1:44 PM, Jon Smirl wrote:

> I'm doing final clean up the i2c open firmware support. What is the
> intention of this code? Is it meant for a missing IRQ attribute to
> return a zero and not get an IRQ for the device? Does the device
> function without an IRQ? If so, it needs a comment explaining things.
> Otherwise IRQ of zero should be an error.
>
> 	i2c->irq = irq_of_parse_and_map(op->node, 0);
> 	if (i2c->irq <= 0) {
> 		result = -ENXIO;
> 		goto fail_irq;
> 	}
> 	
> 	if (i2c->irq != 0)
> 		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
> 					  	IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
> 			printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
> 			goto fail_irq;
> 		}

Looking at the current driver it looks like we could get ride of if  
check since the previous code checked the return of platform_get_irq().

- k

Re: IRQs in i2c-mpc.c

[Jon Smirl]

On 11/10/07, Kumar Gala <galak@kernel.crashing.org> wrote:
> Looking at the current driver it looks like we could get ride of if
> check since the previous code checked the return of platform_get_irq().

The code was a snippet from the larger patch that is converting i2c
from being a platform driver to a of_platform driver.

The question is, what to do about a missing IRQ tag in the device tree
or a IRQ of zero. What is an error and what should be ignored, etc.

+static int mpc_i2c_probe(struct of_device *op, const struct
of_device_id *match)
 {
 	int result = 0;
 	struct mpc_i2c *i2c;
-	struct fsl_i2c_platform_data *pdata;
-	struct resource *r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
-	pdata = (struct fsl_i2c_platform_data *) pdev->dev.platform_data;

 	if (!(i2c = kzalloc(sizeof(*i2c), GFP_KERNEL))) {
 		return -ENOMEM;
 	}

-	i2c->irq = platform_get_irq(pdev, 0);
-	if (i2c->irq < 0) {
-		result = -ENXIO;
-		goto fail_get_irq;
-	}
-	i2c->flags = pdata->device_flags;
-	init_waitqueue_head(&i2c->queue);
+	if (of_get_property(op->node, "dfsrr", NULL))
+		i2c->flags |= FSL_I2C_DEV_SEPARATE_DFSRR;

-	i2c->base = ioremap((phys_addr_t)r->start, MPC_I2C_REGION);
+	if (of_device_is_compatible(op->node, "mpc5200-i2c"))
+		i2c->flags |= FSL_I2C_DEV_CLOCK_5200;

+	init_waitqueue_head(&i2c->queue);
+
+	i2c->base = of_iomap(op->node, 0);
 	if (!i2c->base) {
 		printk(KERN_ERR "i2c-mpc - failed to map controller\n");
 		result = -ENOMEM;
 		goto fail_map;
 	}

+	i2c->irq = irq_of_parse_and_map(op->node, 0);
+	if (i2c->irq < 0) {
+		result = -ENXIO;
+		goto fail_irq;
+	}
+	
 	if (i2c->irq != 0)
 		if ((result = request_irq(i2c->irq, mpc_i2c_isr,
-					  IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
-			printk(KERN_ERR
-			       "i2c-mpc - failed to attach interrupt\n");
+					  	IRQF_SHARED, "i2c-mpc", i2c)) < 0) {
+			printk(KERN_ERR "i2c-mpc - failed to attach interrupt\n");
 			goto fail_irq;
 		}

 	mpc_i2c_setclock(i2c);
-	platform_set_drvdata(pdev, i2c);
+	
+	dev_set_drvdata(&op->dev, i2c);

 	i2c->adap = mpc_ops;
-	i2c->adap.nr = pdev->id;
 	i2c_set_adapdata(&i2c->adap, i2c);
-	i2c->adap.dev.parent = &pdev->dev;
-	if ((result = i2c_add_numbered_adapter(&i2c->adap)) < 0) {
+	i2c->adap.dev.parent = &op->dev;
+	if ((result = i2c_add_adapter(&i2c->adap)) < 0) {
 		printk(KERN_ERR "i2c-mpc - failed to add adapter\n");
 		goto fail_add;
 	}
+	
+	of_register_i2c_devices(&i2c->adap, op->node);

 	return result;

-      fail_add:
+fail_add:
 	if (i2c->irq != 0)
 		free_irq(i2c->irq, i2c);
-      fail_irq:
+fail_irq:
 	iounmap(i2c->base);
-      fail_map:
-      fail_get_irq:
+fail_map:
 	kfree(i2c);
 	return result;
 };



-- 
Jon Smirl
jonsmirl@gmail.com

Re: IRQs in i2c-mpc.c

[Kumar Gala]

On Nov 10, 2007, at 5:16 PM, Jon Smirl wrote:

> On 11/10/07, Kumar Gala <galak@kernel.crashing.org> wrote:
>> Looking at the current driver it looks like we could get ride of if
>> check since the previous code checked the return of  
>> platform_get_irq().
>
> The code was a snippet from the larger patch that is converting i2c
> from being a platform driver to a of_platform driver.
>
> The question is, what to do about a missing IRQ tag in the device tree
> or a IRQ of zero. What is an error and what should be ignored, etc.

I think the lack of an IRQ in the device tree should be an error.  If  
the IRQ value is zero, than its zero.

- k

Re: IRQs in i2c-mpc.c

[Jon Smirl]

On 11/10/07, Kumar Gala <galak@kernel.crashing.org> wrote:
>
> On Nov 10, 2007, at 5:16 PM, Jon Smirl wrote:
>
> > On 11/10/07, Kumar Gala <galak@kernel.crashing.org> wrote:
> >> Looking at the current driver it looks like we could get ride of if
> >> check since the previous code checked the return of
> >> platform_get_irq().
> >
> > The code was a snippet from the larger patch that is converting i2c
> > from being a platform driver to a of_platform driver.
> >
> > The question is, what to do about a missing IRQ tag in the device tree
> > or a IRQ of zero. What is an error and what should be ignored, etc.
>
> I think the lack of an IRQ in the device tree should be an error.  If
> the IRQ value is zero, than its zero.

irq_of_parse_and_map() returns NO_IRQ when the irq parameter is missing.

This API appears to be broken. In asm-powerpc/irq.h NO_IRQ is defined
as (0). There is no way to tell an error in the attribute from a valid
attribute selecting interrupt zero.

NO_IRQ used to be (-1).

/* This number is used when no interrupt has been assigned */
#ifdef CONFIG_PPC_MERGE
#define NO_IRQ                  (0)
#else
#define NO_IRQ                  (-1)


-- 
Jon Smirl
jonsmirl@gmail.com

Re: IRQs in i2c-mpc.c

[Benjamin Herrenschmidt]

> irq_of_parse_and_map() returns NO_IRQ when the irq parameter is missing.
> 
> This API appears to be broken. In asm-powerpc/irq.h NO_IRQ is defined
> as (0). There is no way to tell an error in the attribute from a valid
> attribute selecting interrupt zero.

No, it is not broken. Interrupt numbers in arch/powerpc are virtually
mapped and 0 is never a legal value, thus NO_IRQ is 0. This was done in
part because Linus wanted it that way.

The hardware number can perfectly be 0 though, that's a different thing,
and has nothing to do with what you pass to request_irq().

Ben.

Re: IRQs in i2c-mpc.c

[Jon Smirl]

On 11/11/07, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> > irq_of_parse_and_map() returns NO_IRQ when the irq parameter is missing.
> >
> > This API appears to be broken. In asm-powerpc/irq.h NO_IRQ is defined
> > as (0). There is no way to tell an error in the attribute from a valid
> > attribute selecting interrupt zero.
>
> No, it is not broken. Interrupt numbers in arch/powerpc are virtually
> mapped and 0 is never a legal value, thus NO_IRQ is 0. This was done in
> part because Linus wanted it that way.
>
> The hardware number can perfectly be 0 though, that's a different thing,
> and has nothing to do with what you pass to request_irq().

Is the only error return from irq_of_parse_and_map() NO_IRQ? or can we
get standard negative returns? I started tracing out the returns from
irq_of_parse_and_map() but there are a lot of them.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: IRQs in i2c-mpc.c

[Benjamin Herrenschmidt]

> Is the only error return from irq_of_parse_and_map() NO_IRQ? or can we
> get standard negative returns? I started tracing out the returns from
> irq_of_parse_and_map() but there are a lot of them.

NP_IRQ is the only error return.

Ben.

Re: IRQs in i2c-mpc.c

[Benjamin Herrenschmidt]

On Sat, 2007-11-10 at 17:18 -0600, Kumar Gala wrote:
> On Nov 10, 2007, at 5:16 PM, Jon Smirl wrote:
> 
> > On 11/10/07, Kumar Gala <galak@kernel.crashing.org> wrote:
> >> Looking at the current driver it looks like we could get ride of if
> >> check since the previous code checked the return of  
> >> platform_get_irq().
> >
> > The code was a snippet from the larger patch that is converting i2c
> > from being a platform driver to a of_platform driver.
> >
> > The question is, what to do about a missing IRQ tag in the device tree
> > or a IRQ of zero. What is an error and what should be ignored, etc.
> 
> I think the lack of an IRQ in the device tree should be an error.  If  
> the IRQ value is zero, than its zero.

virq 0 is always illegal so if platform_get_irq() returns 0, it can be
safely treated as an error or the absence of irq, on both powerpc and
x86.

Ben.