[U-Boot] [PATCH 1/6] i2c: rcar_i2c: Fix u32 cast warnings

[U-Boot] [PATCH 1/6] i2c: rcar_i2c: Fix u32 cast warnings

[Ismael Luceno Cortes]

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---
 drivers/i2c/rcar_i2c.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index 8d87c73713..bd7c37a207 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -34,9 +34,9 @@
 #define RCAR_I2C_ICMSR_MNR		BIT(6)
 #define RCAR_I2C_ICMSR_MAL		BIT(5)
 #define RCAR_I2C_ICMSR_MST		BIT(4)
-#define RCAR_I2C_ICMSR_MDE		BIT(3)
+#define RCAR_I2C_ICMSR_MDE		((u32)BIT(3))
 #define RCAR_I2C_ICMSR_MDT		BIT(2)
-#define RCAR_I2C_ICMSR_MDR		BIT(1)
+#define RCAR_I2C_ICMSR_MDR		((u32)BIT(1))
 #define RCAR_I2C_ICMSR_MAT		BIT(0)
 #define RCAR_I2C_ICSIER			0x10
 #define RCAR_I2C_ICMIER			0x14
-- 
2.19.1

[U-Boot] [PATCH 2/6] i2c: rcar_i2c: Add comments about registers & values

[Ismael Luceno Cortes]

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---
 drivers/i2c/rcar_i2c.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index bd7c37a207..7212587234 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -18,33 +18,33 @@
 #include <asm/io.h>
 #include <wait_bit.h>
 
-#define RCAR_I2C_ICSCR			0x00
-#define RCAR_I2C_ICMCR			0x04
-#define RCAR_I2C_ICMCR_MDBS		BIT(7)
-#define RCAR_I2C_ICMCR_FSCL		BIT(6)
-#define RCAR_I2C_ICMCR_FSDA		BIT(5)
-#define RCAR_I2C_ICMCR_OBPC		BIT(4)
-#define RCAR_I2C_ICMCR_MIE		BIT(3)
+#define RCAR_I2C_ICSCR			0x00   /* slave ctrl */
+#define RCAR_I2C_ICMCR			0x04   /* master ctrl */
+#define RCAR_I2C_ICMCR_MDBS		BIT(7) /* non-fifo mode switch */
+#define RCAR_I2C_ICMCR_FSCL		BIT(6) /* override SCL pin */
+#define RCAR_I2C_ICMCR_FSDA		BIT(5) /* override SDA pin */
+#define RCAR_I2C_ICMCR_OBPC		BIT(4) /* override pins */
+#define RCAR_I2C_ICMCR_MIE		BIT(3) /* master if enable */
 #define RCAR_I2C_ICMCR_TSBE		BIT(2)
-#define RCAR_I2C_ICMCR_FSB		BIT(1)
-#define RCAR_I2C_ICMCR_ESG		BIT(0)
-#define RCAR_I2C_ICSSR			0x08
-#define RCAR_I2C_ICMSR			0x0c
+#define RCAR_I2C_ICMCR_FSB		BIT(1) /* force stop bit */
+#define RCAR_I2C_ICMCR_ESG		BIT(0) /* enable start bit gen */
+#define RCAR_I2C_ICSSR			0x08   /* slave status */
+#define RCAR_I2C_ICMSR			0x0c   /* master status */
 #define RCAR_I2C_ICMSR_MASK		0x7f
-#define RCAR_I2C_ICMSR_MNR		BIT(6)
-#define RCAR_I2C_ICMSR_MAL		BIT(5)
-#define RCAR_I2C_ICMSR_MST		BIT(4)
+#define RCAR_I2C_ICMSR_MNR		BIT(6) /* Nack */
+#define RCAR_I2C_ICMSR_MAL		BIT(5) /* Arbitration lost */
+#define RCAR_I2C_ICMSR_MST		BIT(4) /* Stop */
 #define RCAR_I2C_ICMSR_MDE		((u32)BIT(3))
 #define RCAR_I2C_ICMSR_MDT		BIT(2)
 #define RCAR_I2C_ICMSR_MDR		((u32)BIT(1))
 #define RCAR_I2C_ICMSR_MAT		BIT(0)
-#define RCAR_I2C_ICSIER			0x10
-#define RCAR_I2C_ICMIER			0x14
-#define RCAR_I2C_ICCCR			0x18
+#define RCAR_I2C_ICSIER			0x10   /* slave irq enable */
+#define RCAR_I2C_ICMIER			0x14   /* master irq enable */
+#define RCAR_I2C_ICCCR			0x18   /* clock dividers */
 #define RCAR_I2C_ICCCR_SCGD_OFF		3
-#define RCAR_I2C_ICSAR			0x1c
-#define RCAR_I2C_ICMAR			0x20
-#define RCAR_I2C_ICRXD_ICTXD		0x24
+#define RCAR_I2C_ICSAR			0x1c   /* slave address */
+#define RCAR_I2C_ICMAR			0x20   /* master address */
+#define RCAR_I2C_ICRXD_ICTXD		0x24   /* data port */
 
 struct rcar_i2c_priv {
 	void __iomem		*base;
-- 
2.19.1

[U-Boot] [PATCH 3/6] i2c: rcar_i2c: Fix sending of slave addresses

[Ismael Luceno Cortes]

Do the reset before clearing the MSR, otherwise it may result in a read
or write operation instead if the start condition is repeated.

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---
 drivers/i2c/rcar_i2c.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index 7212587234..8cdd37c006 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -120,9 +120,11 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
 	}
 
 	writel((chip << 1) | read, priv->base + RCAR_I2C_ICMAR);
-	writel(0, priv->base + RCAR_I2C_ICMSR);
+	/* Reset */
 	writel(RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE | RCAR_I2C_ICMCR_ESG,
 	       priv->base + RCAR_I2C_ICMCR);
+	/* Clear Status */
+	writel(0, priv->base + RCAR_I2C_ICMSR);
 
 	ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMSR, mask,
 				true, 100, true);
-- 
2.19.1

[U-Boot] [PATCH 4/6] i2c: rcar_i2c: Don't mask errors with EREMOTEIO at rcar_i2c_xfer

[Ismael Luceno Cortes]

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---
 drivers/i2c/rcar_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index 8cdd37c006..b4cc0c55b1 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -207,7 +207,7 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
 			ret = rcar_i2c_write_common(dev, msg);
 
 		if (ret)
-			return -EREMOTEIO;
+			return ret;
 	}
 
 	return ret;
-- 
2.19.1

[U-Boot] [PATCH 5/6] i2c: rcar_i2c: Set the slave address from rcar_i2c_xfer

[Ismael Luceno Cortes]

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---
 drivers/i2c/rcar_i2c.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index b4cc0c55b1..5e04b68d95 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -144,10 +144,6 @@ static int rcar_i2c_read_common(struct udevice *dev, struct i2c_msg *msg)
 	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
 	int i, ret = -EREMOTEIO;
 
-	ret = rcar_i2c_set_addr(dev, msg->addr, 1);
-	if (ret)
-		return ret;
-
 	for (i = 0; i < msg->len; i++) {
 		if (msg->len - 1 == i)
 			icmcr |= RCAR_I2C_ICMCR_FSB;
@@ -174,10 +170,6 @@ static int rcar_i2c_write_common(struct udevice *dev, struct i2c_msg *msg)
 	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
 	int i, ret = -EREMOTEIO;
 
-	ret = rcar_i2c_set_addr(dev, msg->addr, 0);
-	if (ret)
-		return ret;
-
 	for (i = 0; i < msg->len; i++) {
 		writel(msg->buf[i], priv->base + RCAR_I2C_ICRXD_ICTXD);
 		writel(icmcr, priv->base + RCAR_I2C_ICMCR);
@@ -201,6 +193,10 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
 	int ret;
 
 	for (; nmsgs > 0; nmsgs--, msg++) {
+		ret = rcar_i2c_set_addr(dev, msg->addr, 1);
+		if (ret)
+			return -EREMOTEIO;
+
 		if (msg->flags & I2C_M_RD)
 			ret = rcar_i2c_read_common(dev, msg);
 		else
@@ -210,7 +206,7 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
 			return ret;
 	}
 
-	return ret;
+	return 0;
 }
 
 static int rcar_i2c_probe_chip(struct udevice *dev, uint addr, uint flags)
-- 
2.19.1

[U-Boot] [PATCH 6/6] i2c: rcar_i2c: Move FSDA check to rcar_i2c_recover

[Ismael Luceno Cortes]

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---
 drivers/i2c/rcar_i2c.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index 5e04b68d95..b829e8b20d 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -68,7 +68,7 @@ static int rcar_i2c_finish(struct udevice *dev)
 	return ret;
 }
 
-static void rcar_i2c_recover(struct udevice *dev)
+static int rcar_i2c_recover(struct udevice *dev)
 {
 	struct rcar_i2c_priv *priv = dev_get_priv(dev);
 	u32 mcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_OBPC;
@@ -93,6 +93,9 @@ static void rcar_i2c_recover(struct udevice *dev)
 	udelay(5);
 	writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR);
 	udelay(5);
+
+	u32 val = readl(priv->base + RCAR_I2C_ICMSR);
+	return val & RCAR_I2C_ICMCR_FSDA;
 }
 
 static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
@@ -108,12 +111,11 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
 	writel(0, priv->base + RCAR_I2C_ICMSR);
 	writel(priv->icccr, priv->base + RCAR_I2C_ICCCR);
 
+	/* Wait for the bus */
 	ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR,
 				RCAR_I2C_ICMCR_FSDA, false, 2, true);
 	if (ret) {
-		rcar_i2c_recover(dev);
-		val = readl(priv->base + RCAR_I2C_ICMSR);
-		if (val & RCAR_I2C_ICMCR_FSDA) {
+		if (rcar_i2c_recover(dev)) {
 			dev_err(dev, "Bus busy, aborting\n");
 			return ret;
 		}
-- 
2.19.1

[U-Boot] [PATCH 1/6] i2c: rcar_i2c: Fix u32 cast warnings

[Marek Vasut]

On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote:

Hello Ismael,

the patch is missing commit message, please fix globally.

I sent this patch [1] two days ago, I believe it fixes the same issue,
can you rebase the series on top of it ?

[1] http://patchwork.ozlabs.org/patch/1050650/

> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
> ---
>  drivers/i2c/rcar_i2c.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> index 8d87c73713..bd7c37a207 100644
> --- a/drivers/i2c/rcar_i2c.c
> +++ b/drivers/i2c/rcar_i2c.c
> @@ -34,9 +34,9 @@
>  #define RCAR_I2C_ICMSR_MNR		BIT(6)
>  #define RCAR_I2C_ICMSR_MAL		BIT(5)
>  #define RCAR_I2C_ICMSR_MST		BIT(4)
> -#define RCAR_I2C_ICMSR_MDE		BIT(3)
> +#define RCAR_I2C_ICMSR_MDE		((u32)BIT(3))
>  #define RCAR_I2C_ICMSR_MDT		BIT(2)
> -#define RCAR_I2C_ICMSR_MDR		BIT(1)
> +#define RCAR_I2C_ICMSR_MDR		((u32)BIT(1))
>  #define RCAR_I2C_ICMSR_MAT		BIT(0)
>  #define RCAR_I2C_ICSIER			0x10
>  #define RCAR_I2C_ICMIER			0x14
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 5/6] i2c: rcar_i2c: Set the slave address from rcar_i2c_xfer

[Marek Vasut]

On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote:
> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
> ---
>  drivers/i2c/rcar_i2c.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> index b4cc0c55b1..5e04b68d95 100644
> --- a/drivers/i2c/rcar_i2c.c
> +++ b/drivers/i2c/rcar_i2c.c
> @@ -144,10 +144,6 @@ static int rcar_i2c_read_common(struct udevice *dev, struct i2c_msg *msg)
>  	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
>  	int i, ret = -EREMOTEIO;
>  
> -	ret = rcar_i2c_set_addr(dev, msg->addr, 1);
> -	if (ret)
> -		return ret;
> -
>  	for (i = 0; i < msg->len; i++) {
>  		if (msg->len - 1 == i)
>  			icmcr |= RCAR_I2C_ICMCR_FSB;
> @@ -174,10 +170,6 @@ static int rcar_i2c_write_common(struct udevice *dev, struct i2c_msg *msg)
>  	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
>  	int i, ret = -EREMOTEIO;
>  
> -	ret = rcar_i2c_set_addr(dev, msg->addr, 0);
> -	if (ret)
> -		return ret;
> -
>  	for (i = 0; i < msg->len; i++) {
>  		writel(msg->buf[i], priv->base + RCAR_I2C_ICRXD_ICTXD);
>  		writel(icmcr, priv->base + RCAR_I2C_ICMCR);
> @@ -201,6 +193,10 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
>  	int ret;
>  
>  	for (; nmsgs > 0; nmsgs--, msg++) {
> +		ret = rcar_i2c_set_addr(dev, msg->addr, 1);
> +		if (ret)
> +			return -EREMOTEIO;

Shouldn't you return ret here ? It seems like you fixed similar issue in
4/6.

> +
>  		if (msg->flags & I2C_M_RD)
>  			ret = rcar_i2c_read_common(dev, msg);
>  		else
> @@ -210,7 +206,7 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
>  			return ret;
>  	}
>  
> -	return ret;
> +	return 0;

Is this change valid ?

>  }
>  
>  static int rcar_i2c_probe_chip(struct udevice *dev, uint addr, uint flags)
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 6/6] i2c: rcar_i2c: Move FSDA check to rcar_i2c_recover

[Marek Vasut]

On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote:

Why is this change needed ? A commit message explaining it would help a lot.

> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
> ---
>  drivers/i2c/rcar_i2c.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> index 5e04b68d95..b829e8b20d 100644
> --- a/drivers/i2c/rcar_i2c.c
> +++ b/drivers/i2c/rcar_i2c.c
> @@ -68,7 +68,7 @@ static int rcar_i2c_finish(struct udevice *dev)
>  	return ret;
>  }
>  
> -static void rcar_i2c_recover(struct udevice *dev)
> +static int rcar_i2c_recover(struct udevice *dev)
>  {
>  	struct rcar_i2c_priv *priv = dev_get_priv(dev);
>  	u32 mcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_OBPC;
> @@ -93,6 +93,9 @@ static void rcar_i2c_recover(struct udevice *dev)
>  	udelay(5);
>  	writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR);
>  	udelay(5);
> +
> +	u32 val = readl(priv->base + RCAR_I2C_ICMSR);
> +	return val & RCAR_I2C_ICMCR_FSDA;

Declare all your variables at the beginning of the function, I believe
the compiler would even complain about this.

>  }
>  
>  static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
> @@ -108,12 +111,11 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
>  	writel(0, priv->base + RCAR_I2C_ICMSR);
>  	writel(priv->icccr, priv->base + RCAR_I2C_ICCCR);
>  
> +	/* Wait for the bus */
>  	ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR,
>  				RCAR_I2C_ICMCR_FSDA, false, 2, true);
>  	if (ret) {
> -		rcar_i2c_recover(dev);
> -		val = readl(priv->base + RCAR_I2C_ICMSR);
> -		if (val & RCAR_I2C_ICMCR_FSDA) {
> +		if (rcar_i2c_recover(dev)) {
>  			dev_err(dev, "Bus busy, aborting\n");
>  			return ret;
>  		}
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 6/6] i2c: rcar_i2c: Move FSDA check to rcar_i2c_recover

[Ismael Luceno Cortes]

On 04/Mar/2019 20:47, Marek Vasut wrote:
> On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote:
> 
> Why is this change needed ? A commit message explaining it would help a lot.

It's cosmetic. Any call to the recover function would need to do the
same check afterwards, so it' sensible to make it part of the function.

> > Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
> > ---
> >  drivers/i2c/rcar_i2c.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> > index 5e04b68d95..b829e8b20d 100644
> > --- a/drivers/i2c/rcar_i2c.c
> > +++ b/drivers/i2c/rcar_i2c.c
> > @@ -68,7 +68,7 @@ static int rcar_i2c_finish(struct udevice *dev)
> >  	return ret;
> >  }
> >  
> > -static void rcar_i2c_recover(struct udevice *dev)
> > +static int rcar_i2c_recover(struct udevice *dev)
> >  {
> >  	struct rcar_i2c_priv *priv = dev_get_priv(dev);
> >  	u32 mcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_OBPC;
> > @@ -93,6 +93,9 @@ static void rcar_i2c_recover(struct udevice *dev)
> >  	udelay(5);
> >  	writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR);
> >  	udelay(5);
> > +
> > +	u32 val = readl(priv->base + RCAR_I2C_ICMSR);
> > +	return val & RCAR_I2C_ICMCR_FSDA;
> 
> Declare all your variables at the beginning of the function, I believe
> the compiler would even complain about this.

The default mode is -std=gnu11 since GCC 5, and the Makefile defines
CSTD_FLAG to that too.

> >  }
> >  
> >  static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
> > @@ -108,12 +111,11 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
> >  	writel(0, priv->base + RCAR_I2C_ICMSR);
> >  	writel(priv->icccr, priv->base + RCAR_I2C_ICCCR);
> >  
> > +	/* Wait for the bus */
> >  	ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR,
> >  				RCAR_I2C_ICMCR_FSDA, false, 2, true);
> >  	if (ret) {
> > -		rcar_i2c_recover(dev);
> > -		val = readl(priv->base + RCAR_I2C_ICMSR);
> > -		if (val & RCAR_I2C_ICMCR_FSDA) {
> > +		if (rcar_i2c_recover(dev)) {
> >  			dev_err(dev, "Bus busy, aborting\n");
> >  			return ret;
> >  		}

[U-Boot] [PATCH 1/6] i2c: rcar_i2c: Fix u32 cast warnings

[Ismael Luceno Cortes]

On 04/Mar/2019 20:45, Marek Vasut wrote:
> On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote:
> 
> Hello Ismael,
> 
> the patch is missing commit message, please fix globally.
> 
> I sent this patch [1] two days ago, I believe it fixes the same issue,
> can you rebase the series on top of it ?
> 
> [1] http://patchwork.ozlabs.org/patch/1050650/

I'll rebase.

BTW, if the ICFBSCR only needs to be written to once, it would make
sense to do it at the end of rcar_i2c_set_speed.

[U-Boot] [PATCH 5/6] i2c: rcar_i2c: Set the slave address from rcar_i2c_xfer

[Ismael Luceno Cortes]

On 04/Mar/2019 20:46, Marek Vasut wrote:
> On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote:
> > Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
> > ---
> >  drivers/i2c/rcar_i2c.c | 14 +++++---------
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> > index b4cc0c55b1..5e04b68d95 100644
> > --- a/drivers/i2c/rcar_i2c.c
> > +++ b/drivers/i2c/rcar_i2c.c
> > @@ -144,10 +144,6 @@ static int rcar_i2c_read_common(struct udevice *dev, struct i2c_msg *msg)
> >  	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
> >  	int i, ret = -EREMOTEIO;
> >  
> > -	ret = rcar_i2c_set_addr(dev, msg->addr, 1);
> > -	if (ret)
> > -		return ret;
> > -
> >  	for (i = 0; i < msg->len; i++) {
> >  		if (msg->len - 1 == i)
> >  			icmcr |= RCAR_I2C_ICMCR_FSB;
> > @@ -174,10 +170,6 @@ static int rcar_i2c_write_common(struct udevice *dev, struct i2c_msg *msg)
> >  	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
> >  	int i, ret = -EREMOTEIO;
> >  
> > -	ret = rcar_i2c_set_addr(dev, msg->addr, 0);
> > -	if (ret)
> > -		return ret;
> > -
> >  	for (i = 0; i < msg->len; i++) {
> >  		writel(msg->buf[i], priv->base + RCAR_I2C_ICRXD_ICTXD);
> >  		writel(icmcr, priv->base + RCAR_I2C_ICMCR);
> > @@ -201,6 +193,10 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
> >  	int ret;
> >  
> >  	for (; nmsgs > 0; nmsgs--, msg++) {
> > +		ret = rcar_i2c_set_addr(dev, msg->addr, 1);
> > +		if (ret)
> > +			return -EREMOTEIO;
> 
> Shouldn't you return ret here ? It seems like you fixed similar issue in
> 4/6.

Correct. Thanks.

> >  		if (msg->flags & I2C_M_RD)
> >  			ret = rcar_i2c_read_common(dev, msg);
> >  		else
> > @@ -210,7 +206,7 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
> >  			return ret;
> >  	}
> >  
> > -	return ret;
> > +	return 0;
> 
> Is this change valid ?

Yes, ret is always checked, so it can only be 0.

[U-Boot] [PATCH 5/6] i2c: rcar_i2c: Set the slave address from rcar_i2c_xfer

[Marek Vasut]

On 3/5/19 11:46 AM, Ismael Luceno Cortes wrote:
> On 04/Mar/2019 20:46, Marek Vasut wrote:
>> On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote:
>>> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
>>> ---
>>>  drivers/i2c/rcar_i2c.c | 14 +++++---------
>>>  1 file changed, 5 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
>>> index b4cc0c55b1..5e04b68d95 100644
>>> --- a/drivers/i2c/rcar_i2c.c
>>> +++ b/drivers/i2c/rcar_i2c.c
>>> @@ -144,10 +144,6 @@ static int rcar_i2c_read_common(struct udevice *dev, struct i2c_msg *msg)
>>>  	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
>>>  	int i, ret = -EREMOTEIO;
>>>  
>>> -	ret = rcar_i2c_set_addr(dev, msg->addr, 1);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>>  	for (i = 0; i < msg->len; i++) {
>>>  		if (msg->len - 1 == i)
>>>  			icmcr |= RCAR_I2C_ICMCR_FSB;
>>> @@ -174,10 +170,6 @@ static int rcar_i2c_write_common(struct udevice *dev, struct i2c_msg *msg)
>>>  	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
>>>  	int i, ret = -EREMOTEIO;
>>>  
>>> -	ret = rcar_i2c_set_addr(dev, msg->addr, 0);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>>  	for (i = 0; i < msg->len; i++) {
>>>  		writel(msg->buf[i], priv->base + RCAR_I2C_ICRXD_ICTXD);
>>>  		writel(icmcr, priv->base + RCAR_I2C_ICMCR);
>>> @@ -201,6 +193,10 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
>>>  	int ret;
>>>  
>>>  	for (; nmsgs > 0; nmsgs--, msg++) {
>>> +		ret = rcar_i2c_set_addr(dev, msg->addr, 1);
>>> +		if (ret)
>>> +			return -EREMOTEIO;
>>
>> Shouldn't you return ret here ? It seems like you fixed similar issue in
>> 4/6.
> 
> Correct. Thanks.
> 
>>>  		if (msg->flags & I2C_M_RD)
>>>  			ret = rcar_i2c_read_common(dev, msg);
>>>  		else
>>> @@ -210,7 +206,7 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
>>>  			return ret;
>>>  	}
>>>  
>>> -	return ret;
>>> +	return 0;
>>
>> Is this change valid ?
> 
> Yes, ret is always checked, so it can only be 0.

OK

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 6/6] i2c: rcar_i2c: Move FSDA check to rcar_i2c_recover

[Marek Vasut]

On 3/5/19 11:03 AM, Ismael Luceno Cortes wrote:
> On 04/Mar/2019 20:47, Marek Vasut wrote:
>> On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote:
>>
>> Why is this change needed ? A commit message explaining it would help a lot.
> 
> It's cosmetic. Any call to the recover function would need to do the
> same check afterwards, so it' sensible to make it part of the function.

This needs to be in the commit message then.

>>> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
>>> ---
>>>  drivers/i2c/rcar_i2c.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
>>> index 5e04b68d95..b829e8b20d 100644
>>> --- a/drivers/i2c/rcar_i2c.c
>>> +++ b/drivers/i2c/rcar_i2c.c
>>> @@ -68,7 +68,7 @@ static int rcar_i2c_finish(struct udevice *dev)
>>>  	return ret;
>>>  }
>>>  
>>> -static void rcar_i2c_recover(struct udevice *dev)
>>> +static int rcar_i2c_recover(struct udevice *dev)
>>>  {
>>>  	struct rcar_i2c_priv *priv = dev_get_priv(dev);
>>>  	u32 mcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_OBPC;
>>> @@ -93,6 +93,9 @@ static void rcar_i2c_recover(struct udevice *dev)
>>>  	udelay(5);
>>>  	writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR);
>>>  	udelay(5);
>>> +
>>> +	u32 val = readl(priv->base + RCAR_I2C_ICMSR);
>>> +	return val & RCAR_I2C_ICMCR_FSDA;
>>
>> Declare all your variables at the beginning of the function, I believe
>> the compiler would even complain about this.
> 
> The default mode is -std=gnu11 since GCC 5, and the Makefile defines
> CSTD_FLAG to that too.
> 
>>>  }
>>>  
>>>  static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
>>> @@ -108,12 +111,11 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
>>>  	writel(0, priv->base + RCAR_I2C_ICMSR);
>>>  	writel(priv->icccr, priv->base + RCAR_I2C_ICCCR);
>>>  
>>> +	/* Wait for the bus */
>>>  	ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR,
>>>  				RCAR_I2C_ICMCR_FSDA, false, 2, true);
>>>  	if (ret) {
>>> -		rcar_i2c_recover(dev);
>>> -		val = readl(priv->base + RCAR_I2C_ICMSR);
>>> -		if (val & RCAR_I2C_ICMCR_FSDA) {
>>> +		if (rcar_i2c_recover(dev)) {
>>>  			dev_err(dev, "Bus busy, aborting\n");
>>>  			return ret;
>>>  		}


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 1/6] i2c: rcar_i2c: Fix u32 cast warnings

[Marek Vasut]

On 3/5/19 11:23 AM, Ismael Luceno Cortes wrote:
> On 04/Mar/2019 20:45, Marek Vasut wrote:
>> On 3/4/19 3:19 PM, Ismael Luceno Cortes wrote:
>>
>> Hello Ismael,
>>
>> the patch is missing commit message, please fix globally.
>>
>> I sent this patch [1] two days ago, I believe it fixes the same issue,
>> can you rebase the series on top of it ?
>>
>> [1] http://patchwork.ozlabs.org/patch/1050650/
> 
> I'll rebase.
> 
> BTW, if the ICFBSCR only needs to be written to once, it would make
> sense to do it at the end of rcar_i2c_set_speed.

Can you prepare a patch please ?

-- 
Best regards,
Marek Vasut