[U-Boot] [PATCH V3 2/5] mv_i2c: use structure to replace the direclty define
Lei Wen
adrian.wenl at gmail.com
Tue Mar 22 13:34:21 CET 2011
Hi Prafulla,
On Tue, Mar 22, 2011 at 7:17 PM, Prafulla Wadaskar <prafulla at marvell.com> wrote:
>
>
>> -----Original Message-----
>> From: Lei Wen [mailto:leiwen at marvell.com]
>> Sent: Thursday, March 17, 2011 12:15 PM
>> To: Heiko Schocher; Wolfgang Denk; Prafulla Wadaskar; u-
>> boot at lists.denx.de; Marek Vasut; Ashish Karkare; Prabhanjan Sarnaik;
>> adrian.wenl at gmail.com
>> Subject: [PATCH V3 2/5] mv_i2c: use structure to replace the direclty
>> define
>>
>> Signed-off-by: Lei Wen <leiwen at 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 at pengutronix.de>
>> *
>> + * (C) Copyright 2011 Marvell Inc.
>> + * Lei Wen <leiwen at 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
More information about the U-Boot
mailing list