-----Original Message----- From: Lei Wen [mailto:leiwen@marvell.com] Sent: Thursday, March 17, 2011 12:15 PM To: Heiko Schocher; Wolfgang Denk; Prafulla Wadaskar; u- boot@lists.denx.de; Marek Vasut; Ashish Karkare; Prabhanjan Sarnaik; adrian.wenl@gmail.com Subject: [PATCH V3 2/5] mv_i2c: use structure to replace the direclty define
Signed-off-by: Lei Wen leiwen@marvell.com
Changelog: V3: clean code sytle issue
arch/arm/cpu/pxa/cpu.c | 11 +++ arch/arm/include/asm/arch-pxa/pxa-regs.h | 56 ------------- board/innokom/innokom.c | 9 +-- drivers/i2c/mv_i2c.c | 133 ++++++++++++++---------
drivers/i2c/mv_i2c.h | 83 +++++++++++++++++++ include/configs/innokom.h | 1 + include/configs/xm250.h | 1 + 7 files changed, 161 insertions(+), 133 deletions(-) create mode 100644 drivers/i2c/mv_i2c.h
...snip...
diff --git a/drivers/i2c/mv_i2c.c b/drivers/i2c/mv_i2c.c index 09756a4..152ea43 100644 --- a/drivers/i2c/mv_i2c.c +++ b/drivers/i2c/mv_i2c.c @@ -8,6 +8,9 @@
- (C) Copyright 2003 Pengutronix e.K.
- Robert Schwebel r.schwebel@pengutronix.de
- (C) Copyright 2011 Marvell Inc.
- Lei Wen leiwen@marvell.com
- See file CREDITS for list of people who contributed to this
- project.
@@ -34,44 +37,16 @@ #include <asm/io.h>
#ifdef CONFIG_HARD_I2C
-/*
- CONFIG_SYS_I2C_SPEED
- I2C_PXA_SLAVE_ADDR
- */
-#include <asm/arch/hardware.h> -#include <asm/arch/pxa-regs.h> #include <i2c.h>
-#if (CONFIG_SYS_I2C_SPEED == 400000) -#define I2C_ICR_INIT (ICR_FM | ICR_BEIE | ICR_IRFIE | ICR_ITEIE | ICR_GCD \
| ICR_SCLE)-#else -#define I2C_ICR_INIT (ICR_BEIE | ICR_IRFIE | ICR_ITEIE | ICR_GCD | ICR_SCLE) -#endif
-#define I2C_ISR_INIT 0x7FF +#include "mv_i2c.h"
#ifdef DEBUG_I2C #define PRINTD(x) printf x #else #define PRINTD(x) #endif
-/* Shall the current transfer have a start/stop condition? */ -#define I2C_COND_NORMAL 0 -#define I2C_COND_START 1 -#define I2C_COND_STOP 2
-/* Shall the current transfer be ack/nacked or being waited for it? */ -#define I2C_ACKNAK_WAITACK 1 -#define I2C_ACKNAK_SENDACK 2 -#define I2C_ACKNAK_SENDNAK 4
-/* Specify who shall transfer the data (master or slave) */ -#define I2C_READ 0 -#define I2C_WRITE 1 +#define PXAI2C_AND(reg, val) writel(readl(reg) & val, reg) +#define PXAI2C_OR(reg, val) writel(readl(reg) | val, reg)
With reference to the file name MVI2C_ is better macro name to be used here. Applicable for s/pxa/mv/g in the entire file too
/* All transfers are described by this data structure */ struct i2c_msg { @@ -81,27 +56,37 @@ struct i2c_msg { u8 data; };
+struct pxa_i2c {
- u32 ibmr;
- u32 pad0;
- u32 idbr;
- u32 pad1;
- u32 icr;
- u32 pad2;
- u32 isr;
- u32 pad3;
- u32 isar;
+};
Please document the long names of these register with their offsets.
+static struct pxa_i2c *base = (struct pxa_i2c *)CONFIG_PXA_I2C_REG;
...snip...
if (msg->acknack == I2C_ACKNAK_SENDACK)
writel(readl(ICR) & ~ICR_ACKNAK, ICR);writel(readl(ICR) & ~ICR_ALDIE, ICR);writel(readl(ICR) | ICR_TB, ICR);
PXAI2C_AND(&base->icr, ~ICR_ACKNAK);PXAI2C_AND(&base->icr, ~ICR_ALDIE);PXAI2C_OR(&base->icr, ICR_TB);
What are benefits of those macros? To me this looks ugly and looses readability.
...snip...
diff --git a/drivers/i2c/mv_i2c.h b/drivers/i2c/mv_i2c.h new file mode 100644 index 0000000..41af0d9 --- /dev/null +++ b/drivers/i2c/mv_i2c.h @@ -0,0 +1,83 @@ +/*
- (C) Copyright 2011
- Marvell Inc, <www.marvell.com>
- See file CREDITS for list of people who contributed to this
- project.
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; either version 2 of
- the License, or (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 59 Temple Place, Suite 330, Boston,
- MA 02111-1307 USA
- */
+#ifndef _MV_I2C_H_ +#define _MV_I2C_H_ +extern void i2c_clk_enable(void);
+/* Shall the current transfer have a start/stop condition? */ +#define I2C_COND_NORMAL 0 +#define I2C_COND_START 1 +#define I2C_COND_STOP 2
How about prefixing the macros in this file as MVI2C instead of I2C?
...snip...
+#endif diff --git a/include/configs/innokom.h b/include/configs/innokom.h index 0ea73c9..1ddee03 100644 --- a/include/configs/innokom.h +++ b/include/configs/innokom.h @@ -141,6 +141,7 @@
- I2C bus
*/ #define CONFIG_I2C_MV 1 +#define CONFIG_PXA_I2C_REG 0x40301680
Please define i2c base address in armada100.h and in pantheon.h and use it in driver, avoid this definition here.
Regards.. Prafulla . .