Hi Cory, late review, sorry about that... On Mon, Dec 14, 2009 at 05:38:55PM -0800, Cory Maccarrone wrote: > This change introduces a driver for the HTC PLD chip found > on some smartphones, such as the HTC Wizard and HTC Herald. > It works through the I2C bus and acts as a GPIO extender. > Specifically: > > * it can have several sub-devices, each with its own I2C > address > * Each sub-device provides 8 output and 8 input pins > * The chip attaches to one GPIO to signal when any of the > input GPIOs change -- at which point all chips must be > scanned for changes > > This driver implements the GPIOs throught the kernel's > GPIO and IRQ framework. This allows any GPIO-servicing > drivers to operate on htcpld pins, such as the gpio-keys > and gpio-leds drivers. The driver looks fine to me, but I get 23 checkpatch warnings and 5 errors for it. Could you please fix them, keeping in mind that I'm fine with printk/dev_* strings being more than 80 chars. A couple of additional comments: > diff --git a/drivers/mfd/htc-i2cpld.c b/drivers/mfd/htc-i2cpld.c > new file mode 100644 > index 0000000..23338ff > --- /dev/null > +++ b/drivers/mfd/htc-i2cpld.c > @@ -0,0 +1,591 @@ > +/* > + * htc-i2cpld.c > + * Chip driver for a ?cpld? found on omap850 HTC devices like the ?cpld? ?? > +static irqreturn_t htcpld_handler(int irq, void *dev) > +{ > + struct htcpld_data *htcpld = dev; > + unsigned int i; > + unsigned long flags; > + int irqpin; > + struct irq_desc *desc; > + > + if (!htcpld) { > + printk(KERN_INFO "htcpld is null\n"); Please use pr_* instead. It seems you're using it already, so let's be consistent. > +static int __devinit htcpld_core_probe(struct platform_device *pdev) > +{ This routine is fairly big, and could be more readable by calling some subroutines. The chips initialisation part could be extracted out for example. > + struct htcpld_data *htcpld; > + struct device *dev = &pdev->dev; > + struct htcpld_core_platform_data *pdata; > + struct resource *res; > + int i, ret = 0; > + unsigned int irq, irq_end; > + > + if (!dev) > + return -ENODEV; > + > + pdata = dev->platform_data; > + if (!pdata) { > + printk(KERN_ERR "Platform data not found for htcpld core!\n"); > + return -ENXIO; > + } > + > + htcpld = kzalloc(sizeof(struct htcpld_data), GFP_KERNEL); > + if (!htcpld) > + return -ENOMEM; > + > + /* Find chained irq */ > + ret = -EINVAL; > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > + if (res) > + htcpld->chained_irq = res->start; > + > + platform_set_drvdata(pdev, htcpld); > + > + htcpld->int_reset_gpio_hi = pdata->int_reset_gpio_hi; > + htcpld->int_reset_gpio_lo = pdata->int_reset_gpio_lo; > + > + /* Setup each chip's output GPIOs */ > + htcpld->nchips = pdata->num_chip; > + htcpld->chip = kzalloc(sizeof(struct htcpld_chip) * htcpld->nchips, GFP_KERNEL); > + if (!htcpld->chip) { > + ret = -ENOMEM; > + goto fail; > + } > + > + for (i = 0; i < htcpld->nchips; i++) { > + struct i2c_adapter *adapter; > + struct i2c_client *client; > + struct i2c_board_info info; > + struct gpio_chip *chip; > + int ret; > + > + /* Setup the HTCPLD chips */ > + htcpld->chip[i].reset = pdata->chip[i].reset; > + htcpld->chip[i].cache_out = pdata->chip[i].reset; > + htcpld->chip[1].cache_in = 0; Shouldnt it be: htcpld->chip[i].cache_in = 0; instead ? > + htcpld->chip[i].dev = dev; > + htcpld->chip[i].irq_start = pdata->chip[i].irq_base; > + htcpld->chip[i].nirqs = pdata->chip[i].num_irqs; > + > + INIT_WORK(&(htcpld->chip[i].set_val_work), &htcpld_chip_set_ni); > + spin_lock_init(&(htcpld->chip[i].lock)); > + > + /* Setup the IRQs */ > + if (htcpld->chained_irq) { > + int error = 0, flags; > + > + /* Setup irq handlers */ > + irq_end = htcpld->chip[i].irq_start + htcpld->chip[i].nirqs; > + for (irq = htcpld->chip[i].irq_start; irq < irq_end; irq++) { > + set_irq_chip(irq, &htcpld_muxed_chip); > + set_irq_chip_data(irq, &htcpld->chip[i]); > + set_irq_handler(irq, handle_simple_irq); > + set_irq_flags(irq, IRQF_VALID | IRQF_PROBE); > + } > + > + flags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING | IRQF_SAMPLE_RANDOM; > + error = request_threaded_irq( > + htcpld->chained_irq, > + NULL, > + htcpld_handler, > + flags, > + pdev->name, > + htcpld); You could have a nicer indentation here: error = request_threaded_irq(htcpld->chained_irq, NULL, htcpld_handler, flags, pdev->name, htcpld); Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html