Re: [PATCH][I2C] Marvell mv64xxx i2c driver
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 <[EMAIL PROTECTED]> Applied, thanks. greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
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 [EMAIL PROTECTED] Applied, thanks. greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
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)
+{
+ longflags, 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
--
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 <[EMAIL PROTECTED]>
--
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-09 14:32:24 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-09 14:32:24 -07:00
@@ -486,4 +486,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate "Marvell mv64xxx I2C Controller"
+ depends on I2C && MV64X60 && EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-02-09 14:32:24 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-02-09 14:32:24 -07:00
@@ -21,6 +21,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-02-09 14:32:24 -07:00
@@ -0,0 +1,596 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer <[EMAIL PROTECTED]>
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* Register defines */
+#defineMV64XXX_I2C_REG_SLAVE_ADDR 0x00
+#defineMV64XXX_I2C_REG_DATA0x04
+#defineMV64XXX_I2C_REG_CONTROL 0x08
+#defineMV64XXX_I2C_REG_STATUS 0x0c
+#defineMV64XXX_I2C_REG_BAUD0x0c
+#defineMV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10
+#defineMV64XXX_I2C_REG_SOFT_RESET 0x1c
+
+#defineMV64XXX_I2C_REG_CONTROL_ACK 0x0004
+#defineMV64XXX_I2C_REG_CONTROL_IFLG0x0008
+#defineMV64XXX_I2C_REG_CONTROL_STOP0x0010
+#defineMV64XXX_I2C_REG_CONTROL_START 0x0020
+#defineMV64XXX_I2C_REG_CONTROL_TWSIEN 0x0040
+#defineMV64XXX_I2C_REG_CONTROL_INTEN 0x0080
+
+/* Ctlr status values */
+#defineMV64XXX_I2C_STATUS_BUS_ERR 0x00
+#defineMV64XXX_I2C_STATUS_MAST_START 0x08
+#defineMV64XXX_I2C_STATUS_MAST_REPEAT_START0x10
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20
+#defineMV64XXX_I2C_STATUS_MAST_WR_ACK 0x28
+#defineMV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30
+#defineMV64XXX_I2C_STATUS_MAST_LOST_ARB
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
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)
+{
+ longflags, 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
--
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 [EMAIL PROTECTED]
--
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-09 14:32:24 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-09 14:32:24 -07:00
@@ -486,4 +486,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate Marvell mv64xxx I2C Controller
+ depends on I2C MV64X60 EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-02-09 14:32:24 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-02-09 14:32:24 -07:00
@@ -21,6 +21,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-02-09 14:32:24 -07:00
@@ -0,0 +1,596 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer [EMAIL PROTECTED]
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed as is without any warranty of any kind, whether express
+ * or implied.
+ */
+#include linux/kernel.h
+#include linux/module.h
+#include linux/spinlock.h
+#include linux/i2c.h
+#include linux/interrupt.h
+#include linux/mv643xx.h
+#include asm/io.h
+
+/* Register defines */
+#defineMV64XXX_I2C_REG_SLAVE_ADDR 0x00
+#defineMV64XXX_I2C_REG_DATA0x04
+#defineMV64XXX_I2C_REG_CONTROL 0x08
+#defineMV64XXX_I2C_REG_STATUS 0x0c
+#defineMV64XXX_I2C_REG_BAUD0x0c
+#defineMV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10
+#defineMV64XXX_I2C_REG_SOFT_RESET 0x1c
+
+#defineMV64XXX_I2C_REG_CONTROL_ACK 0x0004
+#defineMV64XXX_I2C_REG_CONTROL_IFLG0x0008
+#defineMV64XXX_I2C_REG_CONTROL_STOP0x0010
+#defineMV64XXX_I2C_REG_CONTROL_START 0x0020
+#defineMV64XXX_I2C_REG_CONTROL_TWSIEN 0x0040
+#defineMV64XXX_I2C_REG_CONTROL_INTEN 0x0080
+
+/* Ctlr status values */
+#defineMV64XXX_I2C_STATUS_BUS_ERR 0x00
+#defineMV64XXX_I2C_STATUS_MAST_START 0x08
+#defineMV64XXX_I2C_STATUS_MAST_REPEAT_START0x10
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20
+#defineMV64XXX_I2C_STATUS_MAST_WR_ACK 0x28
+#define
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Tue, 08 Feb 2005 17:32:11 -0700, Mark A. Greer <[EMAIL PROTECTED]> wrote:
> Bartlomiej Zolnierkiewicz wrote:
>
> >Hi,
> >
> >just a minor thing
> >
> >
> >
> >>+static int __devinit
> >>+mv64xxx_i2c_init(void)
> >>+{
> >>+ return driver_register(_i2c_driver);
> >>+}
> >>
> >>
> >
> >__init
> >
> >
> >
> >>+static void __devexit
> >>+mv64xxx_i2c_exit(void)
> >>+{
> >>+ driver_unregister(_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)
> +{
> + longflags, time_left;
'flags' are of 'unsigned long' not 'long' type
> + charabort = 0;
> +
> + time_left = wait_event_interruptible_timeout(drv_data->waitq,
> + !drv_data->block,
> msecs_to_jiffies(drv_data->adapter.timeout));
> +
> + spin_lock_irqsave(_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(_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(_data->adapter.dev,
> + "mv64xxx: I2C bus locked\n");
> + }
> + } else
> + spin_unlock_irqrestore(_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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Bartlomiej Zolnierkiewicz wrote:
Hi,
just a minor thing
+static int __devinit
+mv64xxx_i2c_init(void)
+{
+ return driver_register(_i2c_driver);
+}
__init
+static void __devexit
+mv64xxx_i2c_exit(void)
+{
+ driver_unregister(_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 <[EMAIL PROTECTED]>
--
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-08 17:28:52 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-08 17:28:52 -07:00
@@ -486,4 +486,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate "Marvell mv64xxx I2C Controller"
+ depends on I2C && MV64X60 && EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-02-08 17:28:52 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-02-08 17:28:52 -07:00
@@ -21,6 +21,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-02-08 17:28:52 -07:00
@@ -0,0 +1,606 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer <[EMAIL PROTECTED]>
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* Register defines */
+#defineMV64XXX_I2C_REG_SLAVE_ADDR 0x00
+#defineMV64XXX_I2C_REG_DATA0x04
+#defineMV64XXX_I2C_REG_CONTROL 0x08
+#defineMV64XXX_I2C_REG_STATUS 0x0c
+#defineMV64XXX_I2C_REG_BAUD0x0c
+#defineMV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10
+#defineMV64XXX_I2C_REG_SOFT_RESET 0x1c
+
+#defineMV64XXX_I2C_REG_CONTROL_ACK 0x0004
+#defineMV64XXX_I2C_REG_CONTROL_IFLG0x0008
+#defineMV64XXX_I2C_REG_CONTROL_STOP0x0010
+#defineMV64XXX_I2C_REG_CONTROL_START 0x0020
+#defineMV64XXX_I2C_REG_CONTROL_TWSIEN 0x0040
+#defineMV64XXX_I2C_REG_CONTROL_INTEN 0x0080
+
+/* Ctlr status values */
+#defineMV64XXX_I2C_STATUS_BUS_ERR 0x00
+#defineMV64XXX_I2C_STATUS_MAST_START 0x08
+#defineMV64XXX_I2C_STATUS_MAST_REPEAT_START0x10
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20
+#defineMV64XXX_I2C_STATUS_MAST_WR_ACK 0x28
+#defineMV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30
+#defineMV64XXX_I2C_STATUS_MAST_LOST_ARB0x38
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK 0x48
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_ACK 0x50
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK 0x58
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK 0xd0
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK0xd8
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK 0xe0
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK0xe8
+#defineMV64XXX_I2C_STATUS_NO_STATUS0xf8
+
+/* Driver states */
+enum {
+
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Hi,
just a minor thing
> +static int __devinit
> +mv64xxx_i2c_init(void)
> +{
> + return driver_register(_i2c_driver);
> +}
__init
> +static void __devexit
> +mv64xxx_i2c_exit(void)
> +{
> + driver_unregister(_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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
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 [EMAIL PROTECTED]
--
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-08 17:28:52 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-08 17:28:52 -07:00
@@ -486,4 +486,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate Marvell mv64xxx I2C Controller
+ depends on I2C MV64X60 EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-02-08 17:28:52 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-02-08 17:28:52 -07:00
@@ -21,6 +21,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-02-08 17:28:52 -07:00
@@ -0,0 +1,606 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer [EMAIL PROTECTED]
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed as is without any warranty of any kind, whether express
+ * or implied.
+ */
+#include linux/kernel.h
+#include linux/module.h
+#include linux/spinlock.h
+#include linux/i2c.h
+#include linux/interrupt.h
+#include linux/mv643xx.h
+#include asm/io.h
+
+/* Register defines */
+#defineMV64XXX_I2C_REG_SLAVE_ADDR 0x00
+#defineMV64XXX_I2C_REG_DATA0x04
+#defineMV64XXX_I2C_REG_CONTROL 0x08
+#defineMV64XXX_I2C_REG_STATUS 0x0c
+#defineMV64XXX_I2C_REG_BAUD0x0c
+#defineMV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10
+#defineMV64XXX_I2C_REG_SOFT_RESET 0x1c
+
+#defineMV64XXX_I2C_REG_CONTROL_ACK 0x0004
+#defineMV64XXX_I2C_REG_CONTROL_IFLG0x0008
+#defineMV64XXX_I2C_REG_CONTROL_STOP0x0010
+#defineMV64XXX_I2C_REG_CONTROL_START 0x0020
+#defineMV64XXX_I2C_REG_CONTROL_TWSIEN 0x0040
+#defineMV64XXX_I2C_REG_CONTROL_INTEN 0x0080
+
+/* Ctlr status values */
+#defineMV64XXX_I2C_STATUS_BUS_ERR 0x00
+#defineMV64XXX_I2C_STATUS_MAST_START 0x08
+#defineMV64XXX_I2C_STATUS_MAST_REPEAT_START0x10
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20
+#defineMV64XXX_I2C_STATUS_MAST_WR_ACK 0x28
+#defineMV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30
+#defineMV64XXX_I2C_STATUS_MAST_LOST_ARB0x38
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK 0x48
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_ACK 0x50
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK 0x58
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK 0xd0
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK0xd8
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK 0xe0
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK0xe8
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Tue, 08 Feb 2005 17:32:11 -0700, Mark A. Greer [EMAIL PROTECTED] 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)
+{
+ longflags, time_left;
'flags' are of 'unsigned long' not 'long' type
+ charabort = 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Hi all, Quoting myself: > I am as surprised as you are to see this here. I2C_ALGO_AU1550 should > really be made a different value. There is also a problem with > I2C_ALGO_PCA and I2C_ALGO_SIBYTE having the same value, which was > already reported to Greg some days ago if memory serves. I think I > will send a patch against 2.6.10-rc3 to Linus this evening, which > fixes the broken algo IDs. That way Mark can keep the algorithm ID he > is using right now, and each algorithm will get its own, unique ID, as > should be. I've changed my mind since. There isn't anything critical here and pushing random patches to Linus right before he releases 2.6.11 wouldn't have been particularly subtle. So I instead sent a patch to Greg, as can be seen here: http://archives.andrew.net.au/lm-sensors/msg29405.html So Mark, providing Greg accepts this patch, you can assume that I2C_ALGO_MV64XXX is already properly defined in Greg's tree (where your own patch will end up) so you don't need to add it yourself. You will still have to define I2C_HW_MV64XXX though. Thanks, -- Jean Delvare - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Hi all, Quoting myself: I am as surprised as you are to see this here. I2C_ALGO_AU1550 should really be made a different value. There is also a problem with I2C_ALGO_PCA and I2C_ALGO_SIBYTE having the same value, which was already reported to Greg some days ago if memory serves. I think I will send a patch against 2.6.10-rc3 to Linus this evening, which fixes the broken algo IDs. That way Mark can keep the algorithm ID he is using right now, and each algorithm will get its own, unique ID, as should be. I've changed my mind since. There isn't anything critical here and pushing random patches to Linus right before he releases 2.6.11 wouldn't have been particularly subtle. So I instead sent a patch to Greg, as can be seen here: http://archives.andrew.net.au/lm-sensors/msg29405.html So Mark, providing Greg accepts this patch, you can assume that I2C_ALGO_MV64XXX is already properly defined in Greg's tree (where your own patch will end up) so you don't need to add it yourself. You will still have to define I2C_HW_MV64XXX though. Thanks, -- Jean Delvare - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Hi Mark, Alexey, > > >+ /* 0x17 - USB */ > > >+ /* 0x18 - Virtual buses */ > > >+#define I2C_ALGO_MV64XXX 0x19 /* Marvell mv64xxx i2c ctlr > > >*/ > > > > While I searched for typos and you're fixing them, au1550 owned 0x17. > > 2.6.11-rc3 says: > > > > #define I2C_ALGO_AU1550 0x17 /* Au1550 PSC algorithm */ > > > > So, I'd remove first two comments. > > I added the comments b/c of this email from Jean Delvare, > http://www.uwsg.iu.edu/hypermail/linux/kernel/0501.3/0977.html. The > relevant part being: > > "0x17 is reserved within the legacy i2c project for an USB algorithm, > and 0x18 for virtual busses. Could you please use 0x19 instead, > so as to avoid future collisions?" > > It looks like I2C_ALGO_AU1550 was just added so my guess is Jean is > correct and I2C_ALGO_AU1550 should be made 0x1a (or I move mine back > one). Would someone confirm that 0x17 is used by legacy i2c stuffs? > I don't really know where to look. If so, I can easily make a patch > moving it back. I am as surprised as you are to see this here. I2C_ALGO_AU1550 should really be made a different value. There is also a problem with I2C_ALGO_PCA and I2C_ALGO_SIBYTE having the same value, which was already reported to Greg some days ago if memory serves. I think I will send a patch against 2.6.10-rc3 to Linus this evening, which fixes the broken algo IDs. That way Mark can keep the algorithm ID he is using right now, and each algorithm will get its own, unique ID, as should be. You can see the i2c-id.h list from the legacy i2c project here: http://www2.lm-sensors.nu/~lm78/cvs/i2c/kernel/i2c-id.h I am indeed trying to keep it in sync with the one in Linux 2.6 (and with the one in Linux 2.4 at times), in the hope it'll avoid confusion and increase portability. Thanks, -- Jean Delvare - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Hi Mark, Alexey, + /* 0x17 - USB */ + /* 0x18 - Virtual buses */ +#define I2C_ALGO_MV64XXX 0x19 /* Marvell mv64xxx i2c ctlr */ While I searched for typos and you're fixing them, au1550 owned 0x17. 2.6.11-rc3 says: #define I2C_ALGO_AU1550 0x17 /* Au1550 PSC algorithm */ So, I'd remove first two comments. I added the comments b/c of this email from Jean Delvare, http://www.uwsg.iu.edu/hypermail/linux/kernel/0501.3/0977.html. The relevant part being: 0x17 is reserved within the legacy i2c project for an USB algorithm, and 0x18 for virtual busses. Could you please use 0x19 instead, so as to avoid future collisions? It looks like I2C_ALGO_AU1550 was just added so my guess is Jean is correct and I2C_ALGO_AU1550 should be made 0x1a (or I move mine back one). Would someone confirm that 0x17 is used by legacy i2c stuffs? I don't really know where to look. If so, I can easily make a patch moving it back. I am as surprised as you are to see this here. I2C_ALGO_AU1550 should really be made a different value. There is also a problem with I2C_ALGO_PCA and I2C_ALGO_SIBYTE having the same value, which was already reported to Greg some days ago if memory serves. I think I will send a patch against 2.6.10-rc3 to Linus this evening, which fixes the broken algo IDs. That way Mark can keep the algorithm ID he is using right now, and each algorithm will get its own, unique ID, as should be. You can see the i2c-id.h list from the legacy i2c project here: http://www2.lm-sensors.nu/~lm78/cvs/i2c/kernel/i2c-id.h I am indeed trying to keep it in sync with the one in Linux 2.6 (and with the one in Linux 2.4 at times), in the hope it'll avoid confusion and increase portability. Thanks, -- Jean Delvare - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Alexey Dobriyan wrote: On Thursday 03 February 2005 21:12, Mark A. Greer wrote: + mv64xxx_i2c_fsm(drv_data, status); It can set drv_data->rc to -ENODEV or -EIO. In both cases ->action goes to MV64XXX_I2C_ACTION_SEND_STOP and mv64xxx_i2c_do_action() will writel() something. Is it correct to _not_ check ->rc here? I think so. It still needs to go into do_action even when rc != 0 (in which case it'll do a STOP condition). Ok. Thanks for the explanation. Agree, ->rc should be left as is. Okay. --- a/include/linux/i2c-id.h +++ b/include/linux/i2c-id.h + /* 0x17 - USB */ + /* 0x18 - Virtual buses */ +#define I2C_ALGO_MV64XXX 0x19 /* Marvell mv64xxx i2c ctlr */ While I searched for typos and you're fixing them, au1550 owned 0x17. 2.6.11-rc3 says: #define I2C_ALGO_AU1550 0x17 /* Au1550 PSC algorithm */ So, I'd remove first two comments. I added the comments b/c of this email from Jean Delvare, http://www.uwsg.iu.edu/hypermail/linux/kernel/0501.3/0977.html. The relevant part being: "0x17 is reserved within the legacy i2c project for an USB algorithm, and 0x18 for virtual busses. Could you please use 0x19 instead, so as to avoid future collisions?" It looks like I2C_ALGO_AU1550 was just added so my guess is Jean is correct and I2C_ALGO_AU1550 should be made 0x1a (or I move mine back one). Would someone confirm that 0x17 is used by legacy i2c stuffs? I don't really know where to look. If so, I can easily make a patch moving it back. Oh, and the last note: current sparse and gcc 4 don't produce any warnings. Cool! Mark - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Thursday 03 February 2005 21:12, Mark A. Greer wrote: > > >+ mv64xxx_i2c_fsm(drv_data, status); > > > >It can set drv_data->rc to -ENODEV or -EIO. In both cases ->action goes to > >MV64XXX_I2C_ACTION_SEND_STOP and mv64xxx_i2c_do_action() will writel() > >something. Is it correct to _not_ check ->rc here? > > I think so. It still needs to go into do_action even when rc != 0 (in > which case it'll do a STOP condition). Ok. Thanks for the explanation. Agree, ->rc should be left as is. > This patch is a replacement patch that should address your concerns > except maybe the mv64xxx_i2c_data.rc one. > --- a/include/linux/i2c-id.h > +++ b/include/linux/i2c-id.h > + /* 0x17 - USB */ > + /* 0x18 - Virtual buses */ > +#define I2C_ALGO_MV64XXX 0x19 /* Marvell mv64xxx i2c ctlr */ While I searched for typos and you're fixing them, au1550 owned 0x17. 2.6.11-rc3 says: #define I2C_ALGO_AU1550 0x17 /* Au1550 PSC algorithm */ So, I'd remove first two comments. Oh, and the last note: current sparse and gcc 4 don't produce any warnings. Alexey - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Hi, Alexey.
--
Alexey Dobriyan wrote:
On Wednesday 02 February 2005 19:26, Mark A. Greer wrote:
How's this (a complete replacement for previous patch)?
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges
Dot at the end. Should have noticed it earlier...
Gahh, I noticed that too a while back. I thought I'd fixed it... Done.
+ if (drv_data->state == MV64XXX_I2C_STATE_IDLE) {
+ drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
+ drv_data->state = MV64XXX_I2C_STATE_IDLE;
drv_data->state is already MV64XXX_I2C_STATE_IDLE. Gcc will probably
optimize
this line away, but...
Done away with.
+ }
+ else {
It should be "} else {" according to CodingStyle.
Done.
+static int
+mv64xxx_i2c_intr(int irq, void *dev_id, struct pt_regs *regs)
+ while (readl(drv_data->reg_base + MV64XXX_I2C_REG_CONTROL) &
+ MV64XXX_I2C_REG_CONTROL_IFLG) {
+ status = readl(drv_data->reg_base + MV64XXX_I2C_REG_STATUS);
+ mv64xxx_i2c_fsm(drv_data, status);
It can set drv_data->rc to -ENODEV or -EIO. In both cases ->action goes to
MV64XXX_I2C_ACTION_SEND_STOP and mv64xxx_i2c_do_action() will writel()
something. Is it correct to _not_ check ->rc here?
I think so. It still needs to go into do_action even when rc != 0 (in
which case it'll do a STOP condition).
If it isn't then would it be
better to make all functions that set it return -E___ instead and drop
struct mv64xxx_i2c_data.rc altogether?
I may not be understanding what you mean but I think I need something
like mv64xxx_i2c_data.rc or a plain old global to hang onto the return
code. I need that so when the wait_event_interruptible_timeout()
returns, that thread can find out what happened while it was blocked.
This is what I mean:
- calling thread enters i2c_xfer
- eventually the initial i2c action is executed and the thread blocks in
wait_event_interruptible_timeout()
- other processes run
- several interrupts happen, last one causing an error which is stored
in mv64xxx_i2c_data.rc
- do_action issues a stop on the i2c bus
- thread is unblocked and now has to dig out the rc from mv64xxx_i2c_data.rc
Or am I not understanding what you mean?
+
+ if (!time_left <= 0) {
Confusing. You meant "if (time_left)" or "if (time_left > 0)"?
No, I'm blind. I meant "if (time_left <=0)". Should be fixed now.
Good catch.
+static struct i2c_algorithm mv64xxx_i2c_algo = {
+ .name = MV64XXX_I2C_CTLR_NAME "algorithm",
MV64XXX_I2C_CTLR_NAME doesn't end with space. " algorithm" here.
Space added.
+ dev_err(dev, "mv64xxx: Can't register intr handler "
+ "irq: %d\\n", drv_data->irq);
You snipped s# \\n # \n # suggestion in my previous email. ;-)
Ah, got it this time. :)
This patch is a replacement patch that should address your concerns
except maybe the mv64xxx_i2c_data.rc one.
Signed-off-by: Mark A. Greer <[EMAIL PROTECTED]>
--
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-03 12:10:35 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-03 12:10:35 -07:00
@@ -476,4 +476,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate "Marvell mv64xxx I2C Controller"
+ depends on I2C && MV64X60 && EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-02-03 12:10:35 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-02-03 12:10:35 -07:00
@@ -20,6 +20,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-02-03 12:10:35 -07:00
@@ -0,0 +1,606 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer <[EMAIL PROTECTED]>
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Hi, Alexey.
--
Alexey Dobriyan wrote:
On Wednesday 02 February 2005 19:26, Mark A. Greer wrote:
How's this (a complete replacement for previous patch)?
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges
Dot at the end. Should have noticed it earlier...
Gahh, I noticed that too a while back. I thought I'd fixed it... Done.
+ if (drv_data-state == MV64XXX_I2C_STATE_IDLE) {
+ drv_data-action = MV64XXX_I2C_ACTION_SEND_STOP;
+ drv_data-state = MV64XXX_I2C_STATE_IDLE;
drv_data-state is already MV64XXX_I2C_STATE_IDLE. Gcc will probably
optimize
this line away, but...
Done away with.
+ }
+ else {
It should be } else { according to CodingStyle.
Done.
+static int
+mv64xxx_i2c_intr(int irq, void *dev_id, struct pt_regs *regs)
+ while (readl(drv_data-reg_base + MV64XXX_I2C_REG_CONTROL)
+ MV64XXX_I2C_REG_CONTROL_IFLG) {
+ status = readl(drv_data-reg_base + MV64XXX_I2C_REG_STATUS);
+ mv64xxx_i2c_fsm(drv_data, status);
It can set drv_data-rc to -ENODEV or -EIO. In both cases -action goes to
MV64XXX_I2C_ACTION_SEND_STOP and mv64xxx_i2c_do_action() will writel()
something. Is it correct to _not_ check -rc here?
I think so. It still needs to go into do_action even when rc != 0 (in
which case it'll do a STOP condition).
If it isn't then would it be
better to make all functions that set it return -E___ instead and drop
struct mv64xxx_i2c_data.rc altogether?
I may not be understanding what you mean but I think I need something
like mv64xxx_i2c_data.rc or a plain old global to hang onto the return
code. I need that so when the wait_event_interruptible_timeout()
returns, that thread can find out what happened while it was blocked.
This is what I mean:
- calling thread enters i2c_xfer
- eventually the initial i2c action is executed and the thread blocks in
wait_event_interruptible_timeout()
- other processes run
- several interrupts happen, last one causing an error which is stored
in mv64xxx_i2c_data.rc
- do_action issues a stop on the i2c bus
- thread is unblocked and now has to dig out the rc from mv64xxx_i2c_data.rc
Or am I not understanding what you mean?
+
+ if (!time_left = 0) {
Confusing. You meant if (time_left) or if (time_left 0)?
No, I'm blind. I meant if (time_left =0). Should be fixed now.
Good catch.
+static struct i2c_algorithm mv64xxx_i2c_algo = {
+ .name = MV64XXX_I2C_CTLR_NAME algorithm,
MV64XXX_I2C_CTLR_NAME doesn't end with space. algorithm here.
Space added.
+ dev_err(dev, mv64xxx: Can't register intr handler
+ irq: %d\\n, drv_data-irq);
You snipped s# \\n # \n # suggestion in my previous email. ;-)
Ah, got it this time. :)
This patch is a replacement patch that should address your concerns
except maybe the mv64xxx_i2c_data.rc one.
Signed-off-by: Mark A. Greer [EMAIL PROTECTED]
--
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-03 12:10:35 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-03 12:10:35 -07:00
@@ -476,4 +476,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate Marvell mv64xxx I2C Controller
+ depends on I2C MV64X60 EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges.
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-02-03 12:10:35 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-02-03 12:10:35 -07:00
@@ -20,6 +20,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-02-03 12:10:35 -07:00
@@ -0,0 +1,606 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer [EMAIL PROTECTED]
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed as is without any warranty of any kind, whether express
+ * or implied.
+ */
+#include
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Thursday 03 February 2005 21:12, Mark A. Greer wrote: + mv64xxx_i2c_fsm(drv_data, status); It can set drv_data-rc to -ENODEV or -EIO. In both cases -action goes to MV64XXX_I2C_ACTION_SEND_STOP and mv64xxx_i2c_do_action() will writel() something. Is it correct to _not_ check -rc here? I think so. It still needs to go into do_action even when rc != 0 (in which case it'll do a STOP condition). Ok. Thanks for the explanation. Agree, -rc should be left as is. This patch is a replacement patch that should address your concerns except maybe the mv64xxx_i2c_data.rc one. --- a/include/linux/i2c-id.h +++ b/include/linux/i2c-id.h + /* 0x17 - USB */ + /* 0x18 - Virtual buses */ +#define I2C_ALGO_MV64XXX 0x19 /* Marvell mv64xxx i2c ctlr */ While I searched for typos and you're fixing them, au1550 owned 0x17. 2.6.11-rc3 says: #define I2C_ALGO_AU1550 0x17 /* Au1550 PSC algorithm */ So, I'd remove first two comments. Oh, and the last note: current sparse and gcc 4 don't produce any warnings. Alexey - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Alexey Dobriyan wrote: On Thursday 03 February 2005 21:12, Mark A. Greer wrote: + mv64xxx_i2c_fsm(drv_data, status); It can set drv_data-rc to -ENODEV or -EIO. In both cases -action goes to MV64XXX_I2C_ACTION_SEND_STOP and mv64xxx_i2c_do_action() will writel() something. Is it correct to _not_ check -rc here? I think so. It still needs to go into do_action even when rc != 0 (in which case it'll do a STOP condition). Ok. Thanks for the explanation. Agree, -rc should be left as is. Okay. --- a/include/linux/i2c-id.h +++ b/include/linux/i2c-id.h + /* 0x17 - USB */ + /* 0x18 - Virtual buses */ +#define I2C_ALGO_MV64XXX 0x19 /* Marvell mv64xxx i2c ctlr */ While I searched for typos and you're fixing them, au1550 owned 0x17. 2.6.11-rc3 says: #define I2C_ALGO_AU1550 0x17 /* Au1550 PSC algorithm */ So, I'd remove first two comments. I added the comments b/c of this email from Jean Delvare, http://www.uwsg.iu.edu/hypermail/linux/kernel/0501.3/0977.html. The relevant part being: 0x17 is reserved within the legacy i2c project for an USB algorithm, and 0x18 for virtual busses. Could you please use 0x19 instead, so as to avoid future collisions? It looks like I2C_ALGO_AU1550 was just added so my guess is Jean is correct and I2C_ALGO_AU1550 should be made 0x1a (or I move mine back one). Would someone confirm that 0x17 is used by legacy i2c stuffs? I don't really know where to look. If so, I can easily make a patch moving it back. Oh, and the last note: current sparse and gcc 4 don't produce any warnings. Cool! Mark - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Alexey Dobriyan wrote:
On Tue, 01 Feb 2005 10:54:32 -0700, Mark A. Greer wrote:
+struct mv64xxx_i2c_data {
+ void *reg_base;
ioremap() returns "void __iomem *".
Okay.
+static void __devexit
+mv64xxx_i2c_unmap_regs(struct mv64xxx_i2c_data *drv_data)
+{
+ if (!drv_data->reg_base) {
+ iounmap(drv_data->reg_base);
A typo? You're unmapping known to be invalid address.
Yes, a typo. Good catch.
+ drv_data->reg_base = 0;
Use NULL because drv_data->reg_base is a pointer.
Done.
+ if ((pd->id == 0) && pdata) {
Rewrite this as:
if ((pd->id != 0) || !pdata)
return -ENODEV;
and save one level of indentation below.
Done.
Done.
Alexey
P. S.: struct mv64xxx_i2c_data revisited...
+ uint state;
+ ulong reg_base_p;
Silly request, but... Maybe this should be changed to plain old "unsigned
int"
and "unsigned long". Please. I just don't understand why people use "uint",
"u_int", "uInt", "UINT", "uINT", "uint_t" which are always typedef'ed to
"unsigned int".
Thanks Alexey & Greg. How's this (a complete replacement for previous
patch)?
Signed-off-by: Mark A. Greer <[EMAIL PROTECTED]>
--
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-02 10:24:30 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-02 10:24:30 -07:00
@@ -476,4 +476,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate "Marvell mv64xxx I2C Controller"
+ depends on I2C && MV64X60 && EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-02-02 10:24:30 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-02-02 10:24:30 -07:00
@@ -20,6 +20,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-02-02 10:24:30 -07:00
@@ -0,0 +1,615 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer <[EMAIL PROTECTED]>
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* Register defines */
+#defineMV64XXX_I2C_REG_SLAVE_ADDR 0x00
+#defineMV64XXX_I2C_REG_DATA0x04
+#defineMV64XXX_I2C_REG_CONTROL 0x08
+#defineMV64XXX_I2C_REG_STATUS 0x0c
+#defineMV64XXX_I2C_REG_BAUD0x0c
+#defineMV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10
+#defineMV64XXX_I2C_REG_SOFT_RESET 0x1c
+
+#defineMV64XXX_I2C_REG_CONTROL_ACK 0x0004
+#defineMV64XXX_I2C_REG_CONTROL_IFLG0x0008
+#defineMV64XXX_I2C_REG_CONTROL_STOP0x0010
+#defineMV64XXX_I2C_REG_CONTROL_START 0x0020
+#defineMV64XXX_I2C_REG_CONTROL_TWSIEN 0x0040
+#defineMV64XXX_I2C_REG_CONTROL_INTEN 0x0080
+
+/* Ctlr status values */
+#defineMV64XXX_I2C_STATUS_BUS_ERR 0x00
+#defineMV64XXX_I2C_STATUS_MAST_START 0x08
+#defineMV64XXX_I2C_STATUS_MAST_REPEAT_START0x10
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20
+#defineMV64XXX_I2C_STATUS_MAST_WR_ACK 0x28
+#defineMV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30
+#defineMV64XXX_I2C_STATUS_MAST_LOST_ARB0x38
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK 0x48
+#define
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Alexey Dobriyan wrote:
On Tue, 01 Feb 2005 10:54:32 -0700, Mark A. Greer wrote:
+struct mv64xxx_i2c_data {
+ void *reg_base;
ioremap() returns void __iomem *.
Okay.
+static void __devexit
+mv64xxx_i2c_unmap_regs(struct mv64xxx_i2c_data *drv_data)
+{
+ if (!drv_data-reg_base) {
+ iounmap(drv_data-reg_base);
A typo? You're unmapping known to be invalid address.
Yes, a typo. Good catch.
+ drv_data-reg_base = 0;
Use NULL because drv_data-reg_base is a pointer.
Done.
+ if ((pd-id == 0) pdata) {
Rewrite this as:
if ((pd-id != 0) || !pdata)
return -ENODEV;
and save one level of indentation below.
Done.
snip of xxx_probe modifications
Done.
Alexey
P. S.: struct mv64xxx_i2c_data revisited...
+ uint state;
+ ulong reg_base_p;
Silly request, but... Maybe this should be changed to plain old unsigned
int
and unsigned long. Please. I just don't understand why people use uint,
u_int, uInt, UINT, uINT, uint_t which are always typedef'ed to
unsigned int.
Thanks Alexey Greg. How's this (a complete replacement for previous
patch)?
Signed-off-by: Mark A. Greer [EMAIL PROTECTED]
--
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-02 10:24:30 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-02 10:24:30 -07:00
@@ -476,4 +476,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate Marvell mv64xxx I2C Controller
+ depends on I2C MV64X60 EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-02-02 10:24:30 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-02-02 10:24:30 -07:00
@@ -20,6 +20,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-02-02 10:24:30 -07:00
@@ -0,0 +1,615 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer [EMAIL PROTECTED]
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed as is without any warranty of any kind, whether express
+ * or implied.
+ */
+#include linux/kernel.h
+#include linux/module.h
+#include linux/spinlock.h
+#include linux/i2c.h
+#include linux/interrupt.h
+#include linux/mv643xx.h
+#include asm/io.h
+
+/* Register defines */
+#defineMV64XXX_I2C_REG_SLAVE_ADDR 0x00
+#defineMV64XXX_I2C_REG_DATA0x04
+#defineMV64XXX_I2C_REG_CONTROL 0x08
+#defineMV64XXX_I2C_REG_STATUS 0x0c
+#defineMV64XXX_I2C_REG_BAUD0x0c
+#defineMV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10
+#defineMV64XXX_I2C_REG_SOFT_RESET 0x1c
+
+#defineMV64XXX_I2C_REG_CONTROL_ACK 0x0004
+#defineMV64XXX_I2C_REG_CONTROL_IFLG0x0008
+#defineMV64XXX_I2C_REG_CONTROL_STOP0x0010
+#defineMV64XXX_I2C_REG_CONTROL_START 0x0020
+#defineMV64XXX_I2C_REG_CONTROL_TWSIEN 0x0040
+#defineMV64XXX_I2C_REG_CONTROL_INTEN 0x0080
+
+/* Ctlr status values */
+#defineMV64XXX_I2C_STATUS_BUS_ERR 0x00
+#defineMV64XXX_I2C_STATUS_MAST_START 0x08
+#defineMV64XXX_I2C_STATUS_MAST_REPEAT_START0x10
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20
+#defineMV64XXX_I2C_STATUS_MAST_WR_ACK 0x28
+#defineMV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30
+#defineMV64XXX_I2C_STATUS_MAST_LOST_ARB0x38
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40
+#define
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Wed, Feb 02, 2005 at 03:15:14AM +0200, Alexey Dobriyan wrote: > P. S.: struct mv64xxx_i2c_data revisited... > > > + uintstate; > > + ulong reg_base_p; > > Silly request, but... Maybe this should be changed to plain old "unsigned int" > and "unsigned long". Please. I just don't understand why people use "uint", > "u_int", "uInt", "UINT", "uINT", "uint_t" which are always typedef'ed to > "unsigned int". Not a silly request at all. Please use the u32, u64 and so on values instead. That way we know what you mean, and it's portable. thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Greg KH wrote:
How about a whole new patch that I could apply? That would be better :)
Oops, sorry. :)
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-01 10:45:00 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-01 10:45:00 -07:00
@@ -476,4 +476,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate "Marvell mv64xxx I2C Controller"
+ depends on I2C && MV64X60 && EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-02-01 10:45:00 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-02-01 10:45:00 -07:00
@@ -20,6 +20,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-02-01 10:45:00 -07:00
@@ -0,0 +1,614 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer <[EMAIL PROTECTED]>
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* Register defines */
+#defineMV64XXX_I2C_REG_SLAVE_ADDR 0x00
+#defineMV64XXX_I2C_REG_DATA0x04
+#defineMV64XXX_I2C_REG_CONTROL 0x08
+#defineMV64XXX_I2C_REG_STATUS 0x0c
+#defineMV64XXX_I2C_REG_BAUD0x0c
+#defineMV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10
+#defineMV64XXX_I2C_REG_SOFT_RESET 0x1c
+
+#defineMV64XXX_I2C_REG_CONTROL_ACK 0x0004
+#defineMV64XXX_I2C_REG_CONTROL_IFLG0x0008
+#defineMV64XXX_I2C_REG_CONTROL_STOP0x0010
+#defineMV64XXX_I2C_REG_CONTROL_START 0x0020
+#defineMV64XXX_I2C_REG_CONTROL_TWSIEN 0x0040
+#defineMV64XXX_I2C_REG_CONTROL_INTEN 0x0080
+
+/* Ctlr status values */
+#defineMV64XXX_I2C_STATUS_BUS_ERR 0x00
+#defineMV64XXX_I2C_STATUS_MAST_START 0x08
+#defineMV64XXX_I2C_STATUS_MAST_REPEAT_START0x10
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20
+#defineMV64XXX_I2C_STATUS_MAST_WR_ACK 0x28
+#defineMV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30
+#defineMV64XXX_I2C_STATUS_MAST_LOST_ARB0x38
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK 0x48
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_ACK 0x50
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK 0x58
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK 0xd0
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK0xd8
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK 0xe0
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK0xe8
+#defineMV64XXX_I2C_STATUS_NO_STATUS0xf8
+
+/* Driver states */
+enum {
+ MV64XXX_I2C_STATE_INVALID,
+ MV64XXX_I2C_STATE_IDLE,
+ MV64XXX_I2C_STATE_WAITING_FOR_START_COND,
+ MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA,
+ MV64XXX_I2C_STATE_ABORTING,
+};
+
+/* Driver actions */
+enum {
+ MV64XXX_I2C_ACTION_INVALID,
+ MV64XXX_I2C_ACTION_CONTINUE,
+ MV64XXX_I2C_ACTION_SEND_START,
+ MV64XXX_I2C_ACTION_SEND_ADDR_1,
+ MV64XXX_I2C_ACTION_SEND_ADDR_2,
+ MV64XXX_I2C_ACTION_SEND_DATA,
+
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Greg KH wrote:
How about a whole new patch that I could apply? That would be better :)
Oops, sorry. :)
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-02-01 10:45:00 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-02-01 10:45:00 -07:00
@@ -476,4 +476,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate Marvell mv64xxx I2C Controller
+ depends on I2C MV64X60 EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-02-01 10:45:00 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-02-01 10:45:00 -07:00
@@ -20,6 +20,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-02-01 10:45:00 -07:00
@@ -0,0 +1,614 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer [EMAIL PROTECTED]
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed as is without any warranty of any kind, whether express
+ * or implied.
+ */
+#include linux/kernel.h
+#include linux/module.h
+#include linux/spinlock.h
+#include linux/i2c.h
+#include linux/interrupt.h
+#include linux/mv643xx.h
+#include asm/io.h
+
+/* Register defines */
+#defineMV64XXX_I2C_REG_SLAVE_ADDR 0x00
+#defineMV64XXX_I2C_REG_DATA0x04
+#defineMV64XXX_I2C_REG_CONTROL 0x08
+#defineMV64XXX_I2C_REG_STATUS 0x0c
+#defineMV64XXX_I2C_REG_BAUD0x0c
+#defineMV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10
+#defineMV64XXX_I2C_REG_SOFT_RESET 0x1c
+
+#defineMV64XXX_I2C_REG_CONTROL_ACK 0x0004
+#defineMV64XXX_I2C_REG_CONTROL_IFLG0x0008
+#defineMV64XXX_I2C_REG_CONTROL_STOP0x0010
+#defineMV64XXX_I2C_REG_CONTROL_START 0x0020
+#defineMV64XXX_I2C_REG_CONTROL_TWSIEN 0x0040
+#defineMV64XXX_I2C_REG_CONTROL_INTEN 0x0080
+
+/* Ctlr status values */
+#defineMV64XXX_I2C_STATUS_BUS_ERR 0x00
+#defineMV64XXX_I2C_STATUS_MAST_START 0x08
+#defineMV64XXX_I2C_STATUS_MAST_REPEAT_START0x10
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20
+#defineMV64XXX_I2C_STATUS_MAST_WR_ACK 0x28
+#defineMV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30
+#defineMV64XXX_I2C_STATUS_MAST_LOST_ARB0x38
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK 0x48
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_ACK 0x50
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK 0x58
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK 0xd0
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK0xd8
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK 0xe0
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK0xe8
+#defineMV64XXX_I2C_STATUS_NO_STATUS0xf8
+
+/* Driver states */
+enum {
+ MV64XXX_I2C_STATE_INVALID,
+ MV64XXX_I2C_STATE_IDLE,
+ MV64XXX_I2C_STATE_WAITING_FOR_START_COND,
+ MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA,
+ MV64XXX_I2C_STATE_ABORTING,
+};
+
+/* Driver actions */
+enum {
+ MV64XXX_I2C_ACTION_INVALID,
+ MV64XXX_I2C_ACTION_CONTINUE,
+ MV64XXX_I2C_ACTION_SEND_START,
+ MV64XXX_I2C_ACTION_SEND_ADDR_1,
+
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Wed, Feb 02, 2005 at 03:15:14AM +0200, Alexey Dobriyan wrote: P. S.: struct mv64xxx_i2c_data revisited... + uintstate; + ulong reg_base_p; Silly request, but... Maybe this should be changed to plain old unsigned int and unsigned long. Please. I just don't understand why people use uint, u_int, uInt, UINT, uINT, uint_t which are always typedef'ed to unsigned int. Not a silly request at all. Please use the u32, u64 and so on values instead. That way we know what you mean, and it's portable. thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Mon, Jan 31, 2005 at 11:41:28AM -0700, Mark A. Greer wrote:
> Greg KH wrote:
>
> >On Tue, Jan 25, 2005 at 06:26:45PM -0700, Mark A. Greer wrote:
> >
> >
> >>+static inline void
> >>+mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
> >>
> >>
> >
> >This is a much too big of a function to be "inline". Please change it.
> >Same for your other inline functions, that's not really needed, right?
> >
> >
> >
> >>+{
> >>+ pr_debug("mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n",
> >>+ drv_data->state, status);
> >>
> >>
> >
> >Please use the dev_* calls instead. It gives you an accurate
> >description of the specific device that emits the messages. Also use it
> >for all of the printk() calls in the driver too.
> >
> >thanks,
> >
> >greg k-h
> >
>
> Certainly. I already posted this [incremental] patch based on your
> previous comments,
> http://www.ussg.iu.edu/hypermail/linux/kernel/0501.3/0941.html. Is that
> better?
How about a whole new patch that I could apply? That would be better :)
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Greg KH wrote:
On Tue, Jan 25, 2005 at 06:26:45PM -0700, Mark A. Greer wrote:
+static inline void
+mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
This is a much too big of a function to be "inline". Please change it.
Same for your other inline functions, that's not really needed, right?
+{
+ pr_debug("mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n",
+ drv_data->state, status);
Please use the dev_* calls instead. It gives you an accurate
description of the specific device that emits the messages. Also use it
for all of the printk() calls in the driver too.
thanks,
greg k-h
Certainly. I already posted this [incremental] patch based on your
previous comments,
http://www.ussg.iu.edu/hypermail/linux/kernel/0501.3/0941.html. Is that
better?
Mark
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Tue, Jan 25, 2005 at 06:26:45PM -0700, Mark A. Greer wrote:
> +static inline void
> +mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
This is a much too big of a function to be "inline". Please change it.
Same for your other inline functions, that's not really needed, right?
> +{
> + pr_debug("mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n",
> + drv_data->state, status);
Please use the dev_* calls instead. It gives you an accurate
description of the specific device that emits the messages. Also use it
for all of the printk() calls in the driver too.
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Tue, Jan 25, 2005 at 06:26:45PM -0700, Mark A. Greer wrote:
+static inline void
+mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
This is a much too big of a function to be inline. Please change it.
Same for your other inline functions, that's not really needed, right?
+{
+ pr_debug(mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n,
+ drv_data-state, status);
Please use the dev_* calls instead. It gives you an accurate
description of the specific device that emits the messages. Also use it
for all of the printk() calls in the driver too.
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Greg KH wrote:
On Tue, Jan 25, 2005 at 06:26:45PM -0700, Mark A. Greer wrote:
+static inline void
+mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
This is a much too big of a function to be inline. Please change it.
Same for your other inline functions, that's not really needed, right?
+{
+ pr_debug(mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n,
+ drv_data-state, status);
Please use the dev_* calls instead. It gives you an accurate
description of the specific device that emits the messages. Also use it
for all of the printk() calls in the driver too.
thanks,
greg k-h
Certainly. I already posted this [incremental] patch based on your
previous comments,
http://www.ussg.iu.edu/hypermail/linux/kernel/0501.3/0941.html. Is that
better?
Mark
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Mon, Jan 31, 2005 at 11:41:28AM -0700, Mark A. Greer wrote:
Greg KH wrote:
On Tue, Jan 25, 2005 at 06:26:45PM -0700, Mark A. Greer wrote:
+static inline void
+mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
This is a much too big of a function to be inline. Please change it.
Same for your other inline functions, that's not really needed, right?
+{
+ pr_debug(mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n,
+ drv_data-state, status);
Please use the dev_* calls instead. It gives you an accurate
description of the specific device that emits the messages. Also use it
for all of the printk() calls in the driver too.
thanks,
greg k-h
Certainly. I already posted this [incremental] patch based on your
previous comments,
http://www.ussg.iu.edu/hypermail/linux/kernel/0501.3/0941.html. Is that
better?
How about a whole new patch that I could apply? That would be better :)
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Hi 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 let me know if you see any problems with this patch. I do not feel qualified for a full review of this code. However, I noticed the following minor issues: > +config I2C_MV64XXX > + tristate "Marvell mv64xxx I2C Controller" > + depends on I2C && MV64X60 && EXPERIMENTAL? > diff -Nru a/include/linux/i2c-id.h b/include/linux/i2c-id.h > --- a/include/linux/i2c-id.h 2005-01-25 18:15:24 -07:00 > +++ b/include/linux/i2c-id.h 2005-01-25 18:15:24 -07:00 > @@ -200,6 +200,7 @@ > > #define I2C_ALGO_SIBYTE 0x15 /* Broadcom SiByte SOCs */ > #define I2C_ALGO_SGI 0x16/* SGI algorithm*/ > +#define I2C_ALGO_MV64XXX 0x17 /* Marvell mv64xxx i2c ctlr */ 0x17 is reserved within the legacy i2c project for an USB algorithm, and 0x18 for virtual busses. Could you please use 0x19 instead, so as to avoid future collisions? > -#define MV64340_I2C_SLAVE_ADDR 0xc000 > -#define MV64340_I2C_EXTENDED_SLAVE_ADDR 0xc010 > -#define MV64340_I2C_DATA0xc004 > -#define MV64340_I2C_CONTROL 0xc008 > -#define MV64340_I2C_STATUS_BAUDE_RATE 0xc00C > -#define MV64340_I2C_SOFT_RESET 0xc01c > +#define MV64XXX_I2C_CTLR_NAME > "mv64xxx i2c" > +#define MV64XXX_I2C_OFFSET 0xc000 > +#define MV64XXX_I2C_REG_BLOCK_SIZE 0x0020 You have a tab instead of space before MV64XXX_I2C_CTLR_NAME, it seems. Also, you want to align the numerical values using only tabs, no space. Thanks, -- Jean Delvare http://khali.linux-fr.org/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Jean Delvare wrote: Hi Mark, Thanks for the commenting, Jean. +config I2C_MV64XXX + tristate "Marvell mv64xxx I2C Controller" + depends on I2C && MV64X60 && EXPERIMENTAL? Yes, I guess that's the correct thing to do. I'll add that. diff -Nru a/include/linux/i2c-id.h b/include/linux/i2c-id.h --- a/include/linux/i2c-id.h2005-01-25 18:15:24 -07:00 +++ b/include/linux/i2c-id.h2005-01-25 18:15:24 -07:00 @@ -200,6 +200,7 @@ #define I2C_ALGO_SIBYTE 0x15 /* Broadcom SiByte SOCs */ #define I2C_ALGO_SGI 0x16/* SGI algorithm*/ +#define I2C_ALGO_MV64XXX 0x17 /* Marvell mv64xxx i2c ctlr */ 0x17 is reserved within the legacy i2c project for an USB algorithm, and 0x18 for virtual busses. Could you please use 0x19 instead, so as to avoid future collisions? Absolutely. I was unaware of the other uses. -#define MV64340_I2C_SOFT_RESET 0xc01c +#define MV64XXX_I2C_CTLR_NAME "mv64xxx i2c" +#define MV64XXX_I2C_OFFSET 0xc000 +#define MV64XXX_I2C_REG_BLOCK_SIZE0x0020 You have a tab instead of space before MV64XXX_I2C_CTLR_NAME, it seems. Also, you want to align the numerical values using only tabs, no space. Oops. I'll fix that too and repost later today. Mark - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Jean Delvare wrote:
+config I2C_MV64XXX
+ tristate "Marvell mv64xxx I2C Controller"
+ depends on I2C && MV64X60
&& EXPERIMENTAL?
Done.
+#define I2C_ALGO_MV64XXX 0x17 /* Marvell mv64xxx i2c ctlr */
0x17 is reserved within the legacy i2c project for an USB algorithm,
and 0x18 for virtual busses. Could you please use 0x19 instead,
so as to avoid future collisions?
Done.
+#define MV64XXX_I2C_CTLR_NAME "mv64xxx i2c"
+#define MV64XXX_I2C_OFFSET 0xc000
+#define MV64XXX_I2C_REG_BLOCK_SIZE0x0020
You have a tab instead of space before MV64XXX_I2C_CTLR_NAME, it seems.
Also, you want to align the numerical values using only tabs, no space.
Actually, I did use tabs but what was already there used spaces. So the
patch won't mess up the alignment, I changed mine to spaces too.
The attached patch is a complete replacement for the one posted earlier.
Signed-off-by: Mark A. Greer <[EMAIL PROTECTED]>
--
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-01-26 14:49:22 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-01-26 14:49:22 -07:00
@@ -476,4 +476,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate "Marvell mv64xxx I2C Controller"
+ depends on I2C && MV64X60 && EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-01-26 14:49:22 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-01-26 14:49:22 -07:00
@@ -20,6 +20,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-01-26 14:49:22 -07:00
@@ -0,0 +1,550 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer <[EMAIL PROTECTED]>
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "i2c-mv64xxx.h"
+
+/*
+ *
+ *
+ * Finite State Machine & Interrupt Routines
+ *
+ *
+ */
+static inline void
+mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
+{
+ pr_debug("mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n",
+ drv_data->state, status);
+
+ /*
+* If state is idle, then this is likely the remnants of an old
+* operation that driver has given up on or the user has killed.
+* If so, issue the stop condition and go to idle.
+*/
+ if (drv_data->state == MV64XXX_I2C_STATE_IDLE) {
+ drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
+ drv_data->state = MV64XXX_I2C_STATE_IDLE;
+ pr_debug("mv64xxx_i2c_fsm: EXIT--Entered when in IDLE state\n");
+ return;
+ }
+
+ if (drv_data->state == MV64XXX_I2C_STATE_ABORTING) {
+ drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
+ drv_data->state = MV64XXX_I2C_STATE_IDLE;
+ pr_debug("mv64xxx_i2c_fsm: EXIT--Aborting\n");
+ return;
+ }
+
+ /* The status from the ctlr [mostly] tells us what to do next */
+ switch (status) {
+ /* Start condition interrupt */
+ case MV64XXX_I2C_STATUS_MAST_START: /* 0x08 */
+ case MV64XXX_I2C_STATUS_MAST_REPEAT_START: /* 0x10 */
+ drv_data->action = MV64XXX_I2C_ACTION_SEND_ADDR_1;
+ drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK;
+ break;
+
+ /* Performing a write */
+ case MV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK: /* 0x18 */
+ if (drv_data->msg->flags & I2C_M_TEN) {
+ drv_data->action =
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Wed, Jan 26, 2005 at 02:56:55PM -0700, Mark A. Greer wrote:
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "i2c-mv64xxx.h"
Please put +static inline void
> +mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
> +{
> + pr_debug("mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n",
> + drv_data->state, status);
You have a lot of pr_debug and other printk() for stuff in this driver.
Please use dev_dbg(), dev_err() and friends instead. That way you get a
consistant message, that points to the exact device that is causing the
message.
> +static inline void
> +mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
> + struct i2c_msg *msg)
You have some big inline functions here. Should they really be inline?
We aren't really worried about speed here, right? Size is probably a
bigger issue.
> diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.h
> b/drivers/i2c/busses/i2c-mv64xxx.h
> --- /dev/null Wed Dec 31 16:00:00 196900
> +++ b/drivers/i2c/busses/i2c-mv64xxx.h2005-01-26 14:49:22 -07:00
Is this header file really needed? Does any other file other than this
single driver ever include it? If not, please just put it into the
driver itself.
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Greg KH wrote:
Please put
Done.
You have a lot of pr_debug and other printk() for stuff in this driver.
Please use dev_dbg(), dev_err() and friends instead. That way you get a
consistant message, that points to the exact device that is causing the
message.
Cool. Done.
You have some big inline functions here. Should they really be inline?
We aren't really worried about speed here, right? Size is probably a
bigger issue.
No, no, and done.
Is this header file really needed? Does any other file other than this
single driver ever include it? If not, please just put it into the
driver itself.
No, no, and done.
Included is an *incremental* patch that I hope addresses your concerns.
Thanks Greg.
Mark
--
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- a/drivers/i2c/busses/i2c-mv64xxx.c 2005-01-26 16:52:56 -07:00
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-01-26 16:52:56 -07:00
@@ -11,21 +11,94 @@
* is licensed "as is" without any warranty of any kind, whether express
* or implied.
*/
-#include
#include
#include
-#include
-#include
-#include
-#include
#include
-#include
-#include
#include
#include
-#include
#include
-#include "i2c-mv64xxx.h"
+#include
+
+/* Register defines */
+#defineMV64XXX_I2C_REG_SLAVE_ADDR 0x00
+#defineMV64XXX_I2C_REG_DATA0x04
+#defineMV64XXX_I2C_REG_CONTROL 0x08
+#defineMV64XXX_I2C_REG_STATUS 0x0c
+#defineMV64XXX_I2C_REG_BAUD0x0c
+#defineMV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10
+#defineMV64XXX_I2C_REG_SOFT_RESET 0x1c
+
+#defineMV64XXX_I2C_REG_CONTROL_ACK 0x0004
+#defineMV64XXX_I2C_REG_CONTROL_IFLG0x0008
+#defineMV64XXX_I2C_REG_CONTROL_STOP0x0010
+#defineMV64XXX_I2C_REG_CONTROL_START 0x0020
+#defineMV64XXX_I2C_REG_CONTROL_TWSIEN 0x0040
+#defineMV64XXX_I2C_REG_CONTROL_INTEN 0x0080
+
+/* Ctlr status values */
+#defineMV64XXX_I2C_STATUS_BUS_ERR 0x00
+#defineMV64XXX_I2C_STATUS_MAST_START 0x08
+#defineMV64XXX_I2C_STATUS_MAST_REPEAT_START0x10
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20
+#defineMV64XXX_I2C_STATUS_MAST_WR_ACK 0x28
+#defineMV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30
+#defineMV64XXX_I2C_STATUS_MAST_LOST_ARB0x38
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK 0x48
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_ACK 0x50
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK 0x58
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK 0xd0
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK0xd8
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK 0xe0
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK0xe8
+#defineMV64XXX_I2C_STATUS_NO_STATUS0xf8
+
+/* Driver states */
+enum {
+ MV64XXX_I2C_STATE_INVALID,
+ MV64XXX_I2C_STATE_IDLE,
+ MV64XXX_I2C_STATE_WAITING_FOR_START_COND,
+ MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA,
+ MV64XXX_I2C_STATE_ABORTING,
+};
+
+/* Driver actions */
+enum {
+ MV64XXX_I2C_ACTION_INVALID,
+ MV64XXX_I2C_ACTION_CONTINUE,
+ MV64XXX_I2C_ACTION_SEND_START,
+ MV64XXX_I2C_ACTION_SEND_ADDR_1,
+ MV64XXX_I2C_ACTION_SEND_ADDR_2,
+ MV64XXX_I2C_ACTION_SEND_DATA,
+ MV64XXX_I2C_ACTION_RCV_DATA,
+ MV64XXX_I2C_ACTION_RCV_DATA_STOP,
+ MV64XXX_I2C_ACTION_SEND_STOP,
+};
+
+struct mv64xxx_i2c_data {
+ int irq;
+ uintstate;
+ uintaction;
+ u32 cntl_bits;
+ void*reg_base;
+ ulong reg_base_p;
+ u32 addr1;
+ u32 addr2;
+ uintbytes_left;
+ uintbyte_posn;
+ uintblock;
+ int rc;
+ u32 freq_m;
+ u32 freq_n;
+ wait_queue_head_t waitq;
+ spinlock_t lock;
+ struct i2c_msg *msg;
+ struct i2c_adapter adapter;
+};
/*
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Greg KH wrote:
Please put asm/ after linux/
Done.
You have a lot of pr_debug and other printk() for stuff in this driver.
Please use dev_dbg(), dev_err() and friends instead. That way you get a
consistant message, that points to the exact device that is causing the
message.
Cool. Done.
You have some big inline functions here. Should they really be inline?
We aren't really worried about speed here, right? Size is probably a
bigger issue.
No, no, and done.
Is this header file really needed? Does any other file other than this
single driver ever include it? If not, please just put it into the
driver itself.
No, no, and done.
Included is an *incremental* patch that I hope addresses your concerns.
Thanks Greg.
Mark
--
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- a/drivers/i2c/busses/i2c-mv64xxx.c 2005-01-26 16:52:56 -07:00
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-01-26 16:52:56 -07:00
@@ -11,21 +11,94 @@
* is licensed as is without any warranty of any kind, whether express
* or implied.
*/
-#include linux/config.h
#include linux/kernel.h
#include linux/module.h
-#include linux/sched.h
-#include linux/init.h
-#include linux/pci.h
-#include linux/wait.h
#include linux/spinlock.h
-#include asm/io.h
-#include asm/ocp.h
#include linux/i2c.h
#include linux/interrupt.h
-#include linux/delay.h
#include linux/mv643xx.h
-#include i2c-mv64xxx.h
+#include asm/io.h
+
+/* Register defines */
+#defineMV64XXX_I2C_REG_SLAVE_ADDR 0x00
+#defineMV64XXX_I2C_REG_DATA0x04
+#defineMV64XXX_I2C_REG_CONTROL 0x08
+#defineMV64XXX_I2C_REG_STATUS 0x0c
+#defineMV64XXX_I2C_REG_BAUD0x0c
+#defineMV64XXX_I2C_REG_EXT_SLAVE_ADDR 0x10
+#defineMV64XXX_I2C_REG_SOFT_RESET 0x1c
+
+#defineMV64XXX_I2C_REG_CONTROL_ACK 0x0004
+#defineMV64XXX_I2C_REG_CONTROL_IFLG0x0008
+#defineMV64XXX_I2C_REG_CONTROL_STOP0x0010
+#defineMV64XXX_I2C_REG_CONTROL_START 0x0020
+#defineMV64XXX_I2C_REG_CONTROL_TWSIEN 0x0040
+#defineMV64XXX_I2C_REG_CONTROL_INTEN 0x0080
+
+/* Ctlr status values */
+#defineMV64XXX_I2C_STATUS_BUS_ERR 0x00
+#defineMV64XXX_I2C_STATUS_MAST_START 0x08
+#defineMV64XXX_I2C_STATUS_MAST_REPEAT_START0x10
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_ACK 0x18
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_NO_ACK 0x20
+#defineMV64XXX_I2C_STATUS_MAST_WR_ACK 0x28
+#defineMV64XXX_I2C_STATUS_MAST_WR_NO_ACK 0x30
+#defineMV64XXX_I2C_STATUS_MAST_LOST_ARB0x38
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_ACK 0x40
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_NO_ACK 0x48
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_ACK 0x50
+#defineMV64XXX_I2C_STATUS_MAST_RD_DATA_NO_ACK 0x58
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK 0xd0
+#defineMV64XXX_I2C_STATUS_MAST_WR_ADDR_2_NO_ACK0xd8
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_ACK 0xe0
+#defineMV64XXX_I2C_STATUS_MAST_RD_ADDR_2_NO_ACK0xe8
+#defineMV64XXX_I2C_STATUS_NO_STATUS0xf8
+
+/* Driver states */
+enum {
+ MV64XXX_I2C_STATE_INVALID,
+ MV64XXX_I2C_STATE_IDLE,
+ MV64XXX_I2C_STATE_WAITING_FOR_START_COND,
+ MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK,
+ MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA,
+ MV64XXX_I2C_STATE_ABORTING,
+};
+
+/* Driver actions */
+enum {
+ MV64XXX_I2C_ACTION_INVALID,
+ MV64XXX_I2C_ACTION_CONTINUE,
+ MV64XXX_I2C_ACTION_SEND_START,
+ MV64XXX_I2C_ACTION_SEND_ADDR_1,
+ MV64XXX_I2C_ACTION_SEND_ADDR_2,
+ MV64XXX_I2C_ACTION_SEND_DATA,
+ MV64XXX_I2C_ACTION_RCV_DATA,
+ MV64XXX_I2C_ACTION_RCV_DATA_STOP,
+ MV64XXX_I2C_ACTION_SEND_STOP,
+};
+
+struct mv64xxx_i2c_data {
+ int irq;
+ uintstate;
+ uintaction;
+ u32 cntl_bits;
+ void*reg_base;
+ ulong reg_base_p;
+ u32 addr1;
+ u32 addr2;
+ uintbytes_left;
+ uintbyte_posn;
+ uintblock;
+ int rc;
+ u32 freq_m;
+ u32 freq_n;
+ wait_queue_head_t
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
On Wed, Jan 26, 2005 at 02:56:55PM -0700, Mark A. Greer wrote:
+#include linux/config.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/sched.h
+#include linux/init.h
+#include linux/pci.h
+#include linux/wait.h
+#include linux/spinlock.h
+#include asm/io.h
+#include asm/ocp.h
+#include linux/i2c.h
+#include linux/interrupt.h
+#include linux/delay.h
+#include linux/mv643xx.h
+#include i2c-mv64xxx.h
Please put asm/ after linux/
+static inline void
+mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
+{
+ pr_debug(mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n,
+ drv_data-state, status);
You have a lot of pr_debug and other printk() for stuff in this driver.
Please use dev_dbg(), dev_err() and friends instead. That way you get a
consistant message, that points to the exact device that is causing the
message.
+static inline void
+mv64xxx_i2c_prepare_for_io(struct mv64xxx_i2c_data *drv_data,
+ struct i2c_msg *msg)
You have some big inline functions here. Should they really be inline?
We aren't really worried about speed here, right? Size is probably a
bigger issue.
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.h
b/drivers/i2c/busses/i2c-mv64xxx.h
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.h2005-01-26 14:49:22 -07:00
Is this header file really needed? Does any other file other than this
single driver ever include it? If not, please just put it into the
driver itself.
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Jean Delvare wrote:
+config I2C_MV64XXX
+ tristate Marvell mv64xxx I2C Controller
+ depends on I2C MV64X60
EXPERIMENTAL?
Done.
+#define I2C_ALGO_MV64XXX 0x17 /* Marvell mv64xxx i2c ctlr */
0x17 is reserved within the legacy i2c project for an USB algorithm,
and 0x18 for virtual busses. Could you please use 0x19 instead,
so as to avoid future collisions?
Done.
+#define MV64XXX_I2C_CTLR_NAME mv64xxx i2c
+#define MV64XXX_I2C_OFFSET 0xc000
+#define MV64XXX_I2C_REG_BLOCK_SIZE0x0020
You have a tab instead of space before MV64XXX_I2C_CTLR_NAME, it seems.
Also, you want to align the numerical values using only tabs, no space.
Actually, I did use tabs but what was already there used spaces. So the
patch won't mess up the alignment, I changed mine to spaces too.
The attached patch is a complete replacement for the one posted earlier.
Signed-off-by: Mark A. Greer [EMAIL PROTECTED]
--
diff -Nru a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
--- a/drivers/i2c/busses/Kconfig2005-01-26 14:49:22 -07:00
+++ b/drivers/i2c/busses/Kconfig2005-01-26 14:49:22 -07:00
@@ -476,4 +476,14 @@
This driver can also be built as a module. If so, the module
will be called i2c-pca-isa.
+config I2C_MV64XXX
+ tristate Marvell mv64xxx I2C Controller
+ depends on I2C MV64X60 EXPERIMENTAL
+ help
+ If you say yes to this option, support will be included for the
+ built-in I2C interface on the Marvell 64xxx line of host bridges
+
+ This driver can also be built as a module. If so, the module
+ will be called i2c-mv64xxx.
+
endmenu
diff -Nru a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
--- a/drivers/i2c/busses/Makefile 2005-01-26 14:49:22 -07:00
+++ b/drivers/i2c/busses/Makefile 2005-01-26 14:49:22 -07:00
@@ -20,6 +20,7 @@
obj-$(CONFIG_I2C_IXP4XX) += i2c-ixp4xx.o
obj-$(CONFIG_I2C_KEYWEST) += i2c-keywest.o
obj-$(CONFIG_I2C_MPC) += i2c-mpc.o
+obj-$(CONFIG_I2C_MV64XXX) += i2c-mv64xxx.o
obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o
obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o
obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o
diff -Nru a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
--- /dev/null Wed Dec 31 16:00:00 196900
+++ b/drivers/i2c/busses/i2c-mv64xxx.c 2005-01-26 14:49:22 -07:00
@@ -0,0 +1,550 @@
+/*
+ * drivers/i2c/busses/i2c-mv64xxx.c
+ *
+ * Driver for the i2c controller on the Marvell line of host bridges for MIPS
+ * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
+ *
+ * Author: Mark A. Greer [EMAIL PROTECTED]
+ *
+ * 2005 (c) MontaVista, Software, Inc. This file is licensed under
+ * the terms of the GNU General Public License version 2. This program
+ * is licensed as is without any warranty of any kind, whether express
+ * or implied.
+ */
+#include linux/config.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/sched.h
+#include linux/init.h
+#include linux/pci.h
+#include linux/wait.h
+#include linux/spinlock.h
+#include asm/io.h
+#include asm/ocp.h
+#include linux/i2c.h
+#include linux/interrupt.h
+#include linux/delay.h
+#include linux/mv643xx.h
+#include i2c-mv64xxx.h
+
+/*
+ *
+ *
+ * Finite State Machine Interrupt Routines
+ *
+ *
+ */
+static inline void
+mv64xxx_i2c_fsm(struct mv64xxx_i2c_data *drv_data, u32 status)
+{
+ pr_debug(mv64xxx_i2c_fsm: ENTER--state: %d, status: 0x%x\n,
+ drv_data-state, status);
+
+ /*
+* If state is idle, then this is likely the remnants of an old
+* operation that driver has given up on or the user has killed.
+* If so, issue the stop condition and go to idle.
+*/
+ if (drv_data-state == MV64XXX_I2C_STATE_IDLE) {
+ drv_data-action = MV64XXX_I2C_ACTION_SEND_STOP;
+ drv_data-state = MV64XXX_I2C_STATE_IDLE;
+ pr_debug(mv64xxx_i2c_fsm: EXIT--Entered when in IDLE state\n);
+ return;
+ }
+
+ if (drv_data-state == MV64XXX_I2C_STATE_ABORTING) {
+ drv_data-action = MV64XXX_I2C_ACTION_SEND_STOP;
+ drv_data-state = MV64XXX_I2C_STATE_IDLE;
+ pr_debug(mv64xxx_i2c_fsm: EXIT--Aborting\n);
+ return;
+ }
+
+ /* The status from the ctlr [mostly] tells us what to do next */
+ switch (status) {
+ /* Start condition interrupt */
+ case MV64XXX_I2C_STATUS_MAST_START: /* 0x08 */
+ case MV64XXX_I2C_STATUS_MAST_REPEAT_START: /* 0x10 */
+ drv_data-action = MV64XXX_I2C_ACTION_SEND_ADDR_1;
+ drv_data-state = MV64XXX_I2C_STATE_WAITING_FOR_ADDR_1_ACK;
+ break;
+
+ /* Performing a write */
+ case
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Jean Delvare wrote: Hi Mark, Thanks for the commenting, Jean. snip +config I2C_MV64XXX + tristate Marvell mv64xxx I2C Controller + depends on I2C MV64X60 EXPERIMENTAL? Yes, I guess that's the correct thing to do. I'll add that. diff -Nru a/include/linux/i2c-id.h b/include/linux/i2c-id.h --- a/include/linux/i2c-id.h2005-01-25 18:15:24 -07:00 +++ b/include/linux/i2c-id.h2005-01-25 18:15:24 -07:00 @@ -200,6 +200,7 @@ #define I2C_ALGO_SIBYTE 0x15 /* Broadcom SiByte SOCs */ #define I2C_ALGO_SGI 0x16/* SGI algorithm*/ +#define I2C_ALGO_MV64XXX 0x17 /* Marvell mv64xxx i2c ctlr */ 0x17 is reserved within the legacy i2c project for an USB algorithm, and 0x18 for virtual busses. Could you please use 0x19 instead, so as to avoid future collisions? Absolutely. I was unaware of the other uses. -#define MV64340_I2C_SOFT_RESET 0xc01c +#define MV64XXX_I2C_CTLR_NAME mv64xxx i2c +#define MV64XXX_I2C_OFFSET 0xc000 +#define MV64XXX_I2C_REG_BLOCK_SIZE0x0020 You have a tab instead of space before MV64XXX_I2C_CTLR_NAME, it seems. Also, you want to align the numerical values using only tabs, no space. Oops. I'll fix that too and repost later today. Mark - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH][I2C] Marvell mv64xxx i2c driver
Hi 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 let me know if you see any problems with this patch. I do not feel qualified for a full review of this code. However, I noticed the following minor issues: +config I2C_MV64XXX + tristate Marvell mv64xxx I2C Controller + depends on I2C MV64X60 EXPERIMENTAL? diff -Nru a/include/linux/i2c-id.h b/include/linux/i2c-id.h --- a/include/linux/i2c-id.h 2005-01-25 18:15:24 -07:00 +++ b/include/linux/i2c-id.h 2005-01-25 18:15:24 -07:00 @@ -200,6 +200,7 @@ #define I2C_ALGO_SIBYTE 0x15 /* Broadcom SiByte SOCs */ #define I2C_ALGO_SGI 0x16/* SGI algorithm*/ +#define I2C_ALGO_MV64XXX 0x17 /* Marvell mv64xxx i2c ctlr */ 0x17 is reserved within the legacy i2c project for an USB algorithm, and 0x18 for virtual busses. Could you please use 0x19 instead, so as to avoid future collisions? -#define MV64340_I2C_SLAVE_ADDR 0xc000 -#define MV64340_I2C_EXTENDED_SLAVE_ADDR 0xc010 -#define MV64340_I2C_DATA0xc004 -#define MV64340_I2C_CONTROL 0xc008 -#define MV64340_I2C_STATUS_BAUDE_RATE 0xc00C -#define MV64340_I2C_SOFT_RESET 0xc01c +#define MV64XXX_I2C_CTLR_NAME mv64xxx i2c +#define MV64XXX_I2C_OFFSET 0xc000 +#define MV64XXX_I2C_REG_BLOCK_SIZE 0x0020 You have a tab instead of space before MV64XXX_I2C_CTLR_NAME, it seems. Also, you want to align the numerical values using only tabs, no space. Thanks, -- Jean Delvare http://khali.linux-fr.org/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
