Hi Prafulla, On Tue, Mar 22, 2011 at 7:17 PM, Prafulla Wadaskar <prafu...@marvell.com> wrote: > > >> -----Original Message----- >> From: Lei Wen [mailto:lei...@marvell.com] >> Sent: Thursday, March 17, 2011 12:15 PM >> To: Heiko Schocher; Wolfgang Denk; Prafulla Wadaskar; u- >> b...@lists.denx.de; Marek Vasut; Ashish Karkare; Prabhanjan Sarnaik; >> adrian.w...@gmail.com >> Subject: [PATCH V3 2/5] mv_i2c: use structure to replace the direclty >> define >> >> Signed-off-by: Lei Wen <lei...@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.schwe...@pengutronix.de> >> * >> + * (C) Copyright 2011 Marvell Inc. >> + * Lei Wen <lei...@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.
Good point, I would do it in next post. > >> + >> +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. This intend for short the original too long code, but I don't reject to return to the readl, write one. > > ...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? I am ok with this change proposal. > > ...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. This is not armada100 nor pantheon board, but innokom board... This driver intend to be used by all Marvell pxa series. So for innokom board where should it define itself? Best regards, Lei _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
