On Tue, Jun 28, 2011 at 07:46:49PM +0530, ashishj3 wrote: > +static int da9052_add_subdevs(struct da9052 *da9052) > +{ > + struct da9052_pdata *pdata = da9052->dev->platform_data; > + int ret; > + > + static struct mfd_cell __initdata da9052_subdev_info[] = { > + {"da9052-onkey", .resources = &da9052_onkey_resource, > + .num_resources = 1},
It seems a bit odd that this is embedded into the function? > + {"da9052-gpio", .resources = NULL, .num_resources = 0}, No need to explicitly initialize static data to zero. > +int da9052_device_init(struct da9052 *da9052) > +{ > + struct da9052_pdata *pdata = da9052->dev->platform_data; > + int ret; > + > + mutex_init(&da9052->io_lock); > + mutex_init(&da9052->auxadc_lock); > + pdata->init(da9052); This will crash if no init() function is provided which seems wrong, especially when I'd expect people wouldn't have any need to use such a callback normally. > + > + ret = da9052_add_subdevs(da9052); > + if (ret != 0) > + return ret; > + > + ret = da9052_irq_init(da9052, pdata); > + if (ret != 0) > + return ret; > + > + return 0; > +} This doesn't remove things it added when it failed. > + for (raddr = reg ; raddr < reg + bytes; raddr++) { > + raddr = (raddr << 1); > + > + spi_message_init(&message); > + memset(&xfer, 0, sizeof(xfer)); > + > + xfer.len = 2; > + xfer.tx_buf = da9052->spi_tx_buf; > + xfer.rx_buf = da9052->spi_rx_buf; > + > + da9052->spi_tx_buf[0] = raddr; > + da9052->spi_tx_buf[1] = *val++; > + > + spi_message_add_tail(&xfer, &message); > + > + spi_sync(da9052->spi_dev, &message); > + } This looks like an open coded spi_write(). > + for (raddr = reg ; raddr < reg + bytes; raddr++) { > + reg = ((raddr << 1) | da9052->rw_pol); > + > + spi_message_init(&message); > + memset(&xfer, 0, sizeof(xfer)); > + > + xfer.len = 2; > + xfer.tx_buf = da9052->spi_tx_buf; > + xfer.rx_buf = da9052->spi_rx_buf; > + > + da9052->spi_tx_buf[0] = raddr; > + da9052->spi_tx_buf[1] = 0xff; > + > + da9052->spi_rx_buf[0] = 0; > + da9052->spi_rx_buf[1] = 0; > + > + spi_message_add_tail(&xfer, &message); > + > + ret = spi_sync(da9052->spi_dev, &message); > + > + if (ret == 0) { > + *val = da9052->spi_rx_buf[1]; > + val++; > + return 0; > + } This looks like an open coded spi_write_then_read(), or even better spi_w8r8(). > + da9052_spi->spi_tx_buf = kmalloc(2, GFP_KERNEL | GFP_DMA); > + if (!da9052_spi->spi_tx_buf) { > + ret = -ENOMEM; > + goto err_spi_rx_buf; > + } It would be better to just allocate the array as part of the structure, a separate allocation just uses more memory for both the pointer and the blocks that are used for the allocation. > +static struct spi_driver da9052_spi_driver = { > + .probe = da9052_spi_probe, > + .remove = __devexit_p(da9052_spi_remove), > + . driver = { > + .name = "da9052_spi", Why the _spi? > index 0000000..c005a28 > --- /dev/null > +++ b/include/linux/mfd/da9052/da9052.h > +static const int32_t tbat_lookup[255] = { This shouldn't be in a header file. If it needs to be shared between multiple modules define it in one place and add the prototype in the header file. _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev