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    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site