Re: [PATCH] i2c-ibm_iic driver - new patch

Hi Sean,

On Mon, 07 Jan 2008 21:03:12 -0500 Sean MacLennan [off-list ref] wrote:

Please don't post patches as attachments.

+static int __devinit iic_probe(struct of_device *ofdev,
+							   const struct of_device_id *match)

Indenting could be better.

+{

+	if (!(dev = kzalloc(sizeof(*dev), GFP_KERNEL))) {

Please split the assignments from the tests.  Here and elsewhere.

+		printk(KERN_CRIT "ibm-iic: failed to allocate device data\n");

I am not sure that these messages are necessary and, even if so, not KERN_CRIT.

+	if(iic_force_poll)

Space after "if"

+	if (dev->irq != NO_IRQ) {

	.
	.

+	}
+
+	if (dev->irq == NO_IRQ)

	else instead?

+		printk(KERN_WARNING "ibm-iic%d: using polling mode\n",
+			   dev->idx);
+static int __devexit iic_remove(struct of_device *ofdev)
+{
+	struct ibm_iic_private* dev = (struct ibm_iic_private*)dev_get_drvdata(&ofdev->dev);

Unnecessary cast.

+	if (i2c_del_adapter(&dev->adap)){
+		printk(KERN_CRIT "ibm-iic%d: failed to delete i2c adapter :(\n",
+			dev->idx);

This is not a KERN_CRIT situation ...

+		/* That's *very* bad, just shutdown IRQ ... */
+		if (dev->irq >= 0){

What is that testing? For NO_IRQ as below?

+		    iic_interrupt_mode(dev, 0);
+		    free_irq(dev->irq, dev);
+		    dev->irq = -1;

NO_IRQ?

+		}
+	} else {
+		if (dev->irq != NO_IRQ){
+		    iic_interrupt_mode(dev, 0);
+		    free_irq(dev->irq, dev);
+		}
+		iounmap(dev->vaddr);
+		kfree(dev);

Should these last two be after the below brace?

+	}
+
+	return 0;
+}
+
+
+static struct of_device_id ibm_iic_match[] =

This should be const.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/