From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Sun, 25 Nov 2012 19:11:51 +0100
Subject: [U-Boot] [PATCH 03/22] ARM sunxi: I2C driver
In-Reply-To: <1353843526.17518.15.camel@home.hno.se>
References: <cover.1353842684.git.henrik@henriknordstrom.net>
	<1353843526.17518.15.camel@home.hno.se>
Message-ID: <201211251911.51460.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Henrik Nordstr?m,

[...]

> +static struct i2c __attribute__ ((section(".data"))) *i2c_base =
> +	(struct i2c *)0x1c2ac00;

I dont think you need this workaround at all ... just stick it into the 
function, it's a static constant (and #define the constant please)

> +
> +void i2c_init(int speed, int slaveaddr)
> +{
> +	sunxi_gpio_set_cfgpin(SUNXI_GPB(0), 2);
> +	sunxi_gpio_set_cfgpin(SUNXI_GPB(1), 2);
> +	clock_twi_onoff(0, 1);
> +
> +	/* Enable the i2c bus */
> +	writel(TWI_CTL_BUSEN, &i2c_base->ctl);
> +
> +	/* 400KHz operation M=2, N=1, 24MHz APB clock */
> +	writel(TWI_CLK_DIV(2, 1), &i2c_base->clkr);
> +	writel(TWI_SRST_SRST, &i2c_base->reset);
> +
> +	while ((readl(&i2c_base->reset) & TWI_SRST_SRST))
> +		;
> +}
> +
> +int i2c_probe(uchar chip)
> +{

How can this even work?

> +	return -1;
> +}
> +
> +static int i2c_wait_ctl(int mask, int state)
> +{
> +	int timeout = 0x2ff;
> +	int value = state ? mask : 0;
> +
> +	debug("i2c_wait_ctl(%x == %x), ctl=%x, status=%x\n", mask, value,
> +	      i2c_base->ctl, i2c_base->status);
> +
> +	while (((readl(&i2c_base->ctl) & mask) != value) && timeout-- > 0)
> +		;


What about you change these to :

while (!--timeout) {
 if (readl...)
  break;
}

To make it more readable ?

> +
> +	debug("i2c_wait_ctl(), timeout=%d, ctl=%x, status=%x\n", timeout,
> +	      i2c_base->ctl, i2c_base->status);
> +
> +	if (timeout != 0)
> +		return 0;
> +	else
> +		return -1;
> +}
> +
> +static void i2c_clear_irq(void)
> +{
> +	writel(readl(&i2c_base->ctl) & ~TWI_CTL_INTFLG, &i2c_base->ctl);

clrbits_le32()

> +}
> +
[...]