From mboxrd@z Thu Jan  1 00:00:00 1970
From: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>
Date: Fri, 27 Sep 2013 13:44:14 +0900
Subject: [U-Boot] [PATCH] i2c: Add support for Renesas rcar
In-Reply-To: <5244FD93.2060709@denx.de>
References: <1380237685-1741-1-git-send-email-nobuhiro.iwamatsu.yj@renesas.com>
	<5244FD93.2060709@denx.de>
Message-ID: <52450D1E.8040000@renesas.com>
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

Hi, Heiko.

Thank you for your review.

(2013/09/27 12:37), Heiko Schocher wrote:
> Hello Nobuhiro,
>
> Am 27.09.2013 01:21, schrieb Nobuhiro Iwamatsu:
>> This supports i2c controller for Renesas rcar.
>>
>> Signed-off-by: Hisashi Nakamura<hisashi.nakamura.ak@renesas.com>
>> Signed-off-by: Nobuhiro Iwamatsu<nobuhiro.iwamatsu.yj@renesas.com>
>> ---
>> drivers/i2c/Makefile | 1 +
>> drivers/i2c/rcar_i2c.c | 289 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 290 insertions(+)
>> create mode 100644 drivers/i2c/rcar_i2c.c
>
> Thanks! Patch looks good to me, just some nitpicking comments:
>
>> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
>> index 37ccbd1..f7cbd62 100644
>> --- a/drivers/i2c/Makefile
>> +++ b/drivers/i2c/Makefile
>> @@ -26,6 +26,7 @@ COBJS-$(CONFIG_TSI108_I2C) += tsi108_i2c.o
>> COBJS-$(CONFIG_U8500_I2C) += u8500_i2c.o
>> COBJS-$(CONFIG_SH_I2C) += sh_i2c.o
>> COBJS-$(CONFIG_SH_SH7734_I2C) += sh_sh7734_i2c.o
>> +COBJS-$(CONFIG_SYS_I2C_RCAR) += rcar_i2c.o
>
> Please keep this list sorted ...
>

OK, I will fix.

>> COBJS-$(CONFIG_SYS_I2C) += i2c_core.o
>> COBJS-$(CONFIG_SYS_I2C_FSL) += fsl_i2c.o
>> COBJS-$(CONFIG_SYS_I2C_FTI2C010) += fti2c010.o
>> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
>> new file mode 100644
>> index 0000000..92f0700
>> --- /dev/null
>> +++ b/drivers/i2c/rcar_i2c.c
>> @@ -0,0 +1,289 @@
> [...]
>> +static u8
>> +rcar_i2c_raw_read(struct rcar_i2c *dev, u8 chip, uint addr)
>> +{
>> + u8 ret;
>> +
>> + rcar_i2c_raw_rw_common(dev, chip, addr);
>> +
>> + /* set slave address, receive */
>> + writel((chip<< 1) | 1,&dev->icmar);
> ^ ^
> space please, please fix globally
>
> Hmm.. checkpatch says for your patch:
>
> total: 0 errors, 0 warnings, 0 checks, 296 lines checked
>
> NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE USLEEP_RANGE
>
> mbox has no obvious style problems and is ready for submission.
>
> Seems checpatch did not check this ...
>

this is my mistake. I will fix and recheck.
>> + /* start master receive */
>> + writel(MCR_MDBS | MCR_MIE | MCR_ESG,&dev->icmcr);
>> +
>> + while ((readl(&dev->icmsr)& (MSR_MAT | MSR_MDE))
>> + != (MSR_MAT | MSR_MDE))
>> + udelay(10);
>> +
>> + /* clear ESG */
>> + writel(MCR_MDBS | MCR_MIE,&dev->icmcr);
>> + /* prepare stop condition */
>> + writel(MCR_MDBS | MCR_MIE | MCR_FSB,&dev->icmcr);
>> + /* start SCLclk */
>> + writel(~(MSR_MAT | MSR_MDR),&dev->icmsr);
>> +
>> + while (!(readl(&dev->icmsr)& MSR_MDR))
>> + udelay(10);
>> +
>> + /* get receive data */
>> + ret = (u8)readl(&dev->icrxdtxd);
>> + /* start SCLclk */
>> + writel(~MSR_MDR,&dev->icmsr);
>> +
>> + rcar_i2c_raw_rw_finish(dev);
>> +
>> + return ret;
>> +}
>> +
>> +
>
> Please only one empty line.
>
> [...]

OK, I will remove this line.

>
> bye,
> Heiko

Best regards,
   Nobuhiro