From mboxrd@z Thu Jan  1 00:00:00 1970
From: Mike Frysinger <vapier@gentoo.org>
Date: Wed, 29 Feb 2012 22:24:34 -0500
Subject: [U-Boot] [PATCH v4] sh: i2c: Add support I2C controller of
	SH7734
In-Reply-To: <1330570723-10002-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com>
References: <1330570723-10002-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com>
Message-ID: <201202292224.38696.vapier@gentoo.org>
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

On Wednesday 29 February 2012 21:58:42 Nobuhiro Iwamatsu wrote:
> --- /dev/null
> +++ b/drivers/i2c/sh_sh7734_i2c.c
>
> +#include <common.h>
> +#include <asm/io.h>

should include i2c.h too so you know your funcs match the prototypes everyone 
else uses

> +static int check_stop(struct sh_i2c *base)
> +{
> +	int i, ret = 1;
> +
> +	for (i = 0; i < IRQ_WAIT; i++) {
> +		if (SH_I2C_ICSR_STOP & readb(&base->icsr)) {
> +			ret = 0;
> +			break;
> +		}
> +		udelay(10);
> +	}
> +
> +	clrbits_le8(&base->icsr, SH_I2C_ICSR_STOP);
> +
> +	return ret;
> +}
> +
> +static int check_tend(struct sh_i2c *base, int stop)
> +{
> +	int i, ret = 1;
> +
> +	for (i = 0; i < IRQ_WAIT; i++) {
> +		if (SH_I2C_ICSR_TEND & readb(&base->icsr)) {
> +			ret = 0;
> +			break;
> +		}
> +		udelay(10);
> +	}
> +
> +	if (stop) {
> +		u8 data;
> +
> +		clrbits_le8(&base->icsr, SH_I2C_ICSR_STOP);
> +
> +		sh_i2c_send_stop(base);
> +	}
> +
> +	clrbits_le8(&base->icsr, SH_I2C_ICSR_TEND);
> +
> +	return ret;
> +}
> +
> +static int check_tdre(struct sh_i2c *base)
> +{
> +	int i;
> +
> +	for (i = 0; i < IRQ_WAIT; i++) {
> +		if (SH_I2C_ICSR_TDRE & readb(&base->icsr))
> +			return 0;
> +		udelay(10);
> +	}
> +
> +	return 1;
> +}
> +
> +static int check_rdrf(struct sh_i2c *base)
> +{
> +	int i;
> +
> +	for (i = 0; i < IRQ_WAIT; i++) {
> +		if (SH_I2C_ICSR_RDRF & readb(&base->icsr))
> +			return 0;
> +		udelay(10);
> +	}
> +
> +	return 1;
> +}

you've got the same icsr bit polling logic here.  probably better to put it 
into a dedicated func which takes the bit flag to check and the rest will call 
that.
	static int check_icsr_bit(struct sh_i2c *base, uint bit)
	{
		int i;

		for (i = 0; i < IRQ_WAIT; i++) {
			if (bit & readb(&base->icsr))
				return 0;
			udelay(10);
		}

		return 1;
	}

	static int check_tdre(struct sh_i2c *base)
	{
		return check_icsr_bit(base, SH_I2C_ICSR_TDRE);
	}

> +static u8 i2c_raw_read(struct sh_i2c *base, u8 id, u8 reg)
> +{
> ...
> +	writeb(id << 1 | 1, &base->icdrt);

shifting and bit ops can lead to unexpected behavior ... might be better:
	writeb((id << 1) | 1, &base->icdrt);

> +/*
> + * i2c_probe: - Test if a chip answers for a given i2c address
> + *
> + * @chip:   address of the chip which is searched for
> + * @return: 0 if a chip was found, -1 otherwhise
> + */
> +int i2c_probe(u8 chip)
> +{
> +	return 0;
> +}

should implement this.  look at the Blackfin twi driver for an easy one.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120229/7c82d16a/attachment.pgp>