[PATCH][I2C] Marvell mv64xxx i2c driver

[Mark A. Greer]

Marvell makes a line of host bridge for PPC and MIPS systems. On those
bridges is an i2c controller. This patch adds the driver for that i2c
controller.

Please apply.

Depends on patch submitted by Jean Delvare:
http://archives.andrew.net.au/lm-sensors/msg29405.html

Signed-off-by: Mark A. Greer <mgr...@mvista.com>

[Note: This patch is a *replacement* patch]
--

[Bartlomiej Zolnierkiewicz]

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

> +module_init(mv64xxx_i2c_init);
> +module_exit(mv64xxx_i2c_exit);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Mark A. Greer]

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
--

Marvell makes a line of host bridge for PPC and MIPS systems. On those
bridges is an i2c controller. This patch adds the driver for that i2c
controller.

Please apply.

Depends on patch submitted by Jean Delvare:
http://archives.andrew.net.au/lm-sensors/msg29405.html

Signed-off-by: Mark A. Greer <mgr...@mvista.com>

--

[Bartlomiej Zolnierkiewicz]

On Tue, 08 Feb 2005 17:32:11 -0700, Mark A. Greer <mgr...@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

[Mark A. Greer]

Bartlomiej Zolnierkiewicz wrote:

>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.
>

You're right. The 'dev' is back except on the module_init/exit routines.

>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
>

Fixed.

>there is no need for explicit return in void functions
>

I can't find any definitive policy on this. I kind of like the explicit
return, I don't know why. I've had others make the same comment,
though, so I'll remove them since it obviously bothers people.

Attached is a replacement patch.

Thanks again, Bartlomiej.

Mark

[Greg KH]

On Wed, Feb 09, 2005 at 02:33:59PM -0700, Mark A. Greer wrote:
>
> I can't find any definitive policy on this. I kind of like the explicit
> return, I don't know why. I've had others make the same comment,
> though, so I'll remove them since it obviously bothers people.
>
> Attached is a replacement patch.
>
> Thanks again, Bartlomiej.
>
> Mark
> --
> Marvell makes a line of host bridge for PPC and MIPS systems. On those
> bridges is an i2c controller. This patch adds the driver for that i2c
> controller.
>
> Please apply.
>
> Depends on patch submitted by Jean Delvare:
> http://archives.andrew.net.au/lm-sensors/msg29405.html
>
> Signed-off-by: Mark A. Greer <mgr...@mvista.com>

Applied, thanks.

greg k-h