lkml.org 
[lkml]   [2005]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH][I2C] Marvell mv64xxx i2c driver
    On Tue, 08 Feb 2005 17:32:11 -0700, Mark A. Greer <mgreer@mvista.com> wrote:
    > Bartlomiej Zolnierkiewicz wrote:
    >
    > >Hi,
    > >
    > >just a minor thing
    > >
    > >
    > >
    > >>+static int __devinit
    > >>+mv64xxx_i2c_init(void)
    > >>+{
    > >>+ return driver_register(&mv64xxx_i2c_driver);
    > >>+}
    > >>
    > >>
    > >
    > >__init
    > >
    > >
    > >
    > >>+static void __devexit
    > >>+mv64xxx_i2c_exit(void)
    > >>+{
    > >>+ driver_unregister(&mv64xxx_i2c_driver);
    > >>+ return;
    > >>+}
    > >>
    > >>
    > >
    > >__exit
    > >
    > >these functions relate to module not device
    > >
    >
    > Gahhh. Thanks Bartlomiej.
    >
    > Below is yet another replacement patch

    Thanks, I see that you did global replacement of __devinit
    by __init and __devexit by __exit - it seems correct *only* if:
    - there can be only one i2c controller in the system
    - there can be only one host bridge in the system
    - i2c core calls ->probe only once during driver init
    and ->remove only once during driver exit

    If all conditions are really true some comment about
    this in the code would still be be nice.

    While at it more silly, minor nitpicking ;)

    > +static void
    > +mv64xxx_i2c_wait_for_completion(struct mv64xxx_i2c_data *drv_data)
    > +{
    > + long flags, time_left;

    'flags' are of 'unsigned long' not 'long' type

    > + char abort = 0;
    > +
    > + time_left = wait_event_interruptible_timeout(drv_data->waitq,
    > + !drv_data->block, msecs_to_jiffies(drv_data->adapter.timeout));
    > +
    > + spin_lock_irqsave(&drv_data->lock, flags);
    > + if (!time_left) { /* Timed out */
    > + drv_data->rc = -ETIMEDOUT;
    > + abort = 1;
    > + } else if (time_left < 0) { /* Interrupted/Error */
    > + drv_data->rc = time_left; /* errno value */
    > + abort = 1;
    > + }
    > +
    > + if (abort && drv_data->block) {
    > + drv_data->state = MV64XXX_I2C_STATE_ABORTING;
    > + spin_unlock_irqrestore(&drv_data->lock, flags);
    > +
    > + time_left = wait_event_timeout(drv_data->waitq,
    > + !drv_data->block,
    > + msecs_to_jiffies(drv_data->adapter.timeout));
    > +
    > + if (time_left <= 0) {
    > + drv_data->state = MV64XXX_I2C_STATE_IDLE;
    > + dev_err(&drv_data->adapter.dev,
    > + "mv64xxx: I2C bus locked\n");
    > + }
    > + } else
    > + spin_unlock_irqrestore(&drv_data->lock, flags);
    > +
    > + return;

    there is no need for explicit return in void functions

    [ These two comments also apply to other code in the driver. ]

    Thanks,
    Bartlomiej
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-03-22 14:10    [W:0.028 / U:30.116 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site