lkml.org 
[lkml]   [2011]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/11] MFD: DA9052 MFD core module v1
    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.


    \
     
     \ /
      Last update: 2011-06-29 05:29    [W:0.028 / U:96.532 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site