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

2019-03-06 Thread Ismael Luceno Cortes 
On 05/Mar/2019 19:32, Marek Vasut wrote:
> On 3/5/19 12:23 PM, Ismael Luceno Cortes wrote:
> > Cosmetic change.  Any call to the recover function would need to do the
> > same check afterwards, so it's sensible to make it part of the function.
> > 
> > Signed-off-by: Ismael Luceno 
> > ---
> > 
> > Notes:
> > Changes since v1:
> > - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC 
> > support")
> > - Explained the change
> > - Replaced C99-style variable declaration
> > 
> >  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 4556b115bd..21bcf09101 100644
> > --- a/drivers/i2c/rcar_i2c.c
> > +++ b/drivers/i2c/rcar_i2c.c
> > @@ -77,12 +77,13 @@ 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;
> > u32 mcra = mcr | RCAR_I2C_ICMCR_FSDA;
> > int i;
> > +   u32 ret;
> 
> Can you change this to "reg" or something like that ? $ret is usually a
> signed int return value, so this could be confusing.

Ok.
 
> >  
> > /* Send 9 SCL pulses */
> > for (i = 0; i < 9; i++) {
> > @@ -102,6 +103,9 @@ static void rcar_i2c_recover(struct udevice *dev)
> > udelay(5);
> > writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR);
> > udelay(5);
> > +
> > +   ret = readl(priv->base + RCAR_I2C_ICMSR);
> > +   return ret & RCAR_I2C_ICMCR_FSDA;
> 
> What about doing this instead
> 
> reg = readl...
> if (reg & ...FSDA)
>   return -EBUSY;
> 
> return 0;
> 
> That way, the code will use standard errno.h return values.

Makes sense.
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

2019-03-05 Thread Marek Vasut 
On 3/5/19 12:23 PM, Ismael Luceno Cortes wrote:
> Cosmetic change.  Any call to the recover function would need to do the
> same check afterwards, so it's sensible to make it part of the function.
> 
> Signed-off-by: Ismael Luceno 
> ---
> 
> Notes:
> Changes since v1:
> - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")
> - Explained the change
> - Replaced C99-style variable declaration
> 
>  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 4556b115bd..21bcf09101 100644
> --- a/drivers/i2c/rcar_i2c.c
> +++ b/drivers/i2c/rcar_i2c.c
> @@ -77,12 +77,13 @@ 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;
>   u32 mcra = mcr | RCAR_I2C_ICMCR_FSDA;
>   int i;
> + u32 ret;

Can you change this to "reg" or something like that ? $ret is usually a
signed int return value, so this could be confusing.

>  
>   /* Send 9 SCL pulses */
>   for (i = 0; i < 9; i++) {
> @@ -102,6 +103,9 @@ static void rcar_i2c_recover(struct udevice *dev)
>   udelay(5);
>   writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR);
>   udelay(5);
> +
> + ret = readl(priv->base + RCAR_I2C_ICMSR);
> + return ret & RCAR_I2C_ICMCR_FSDA;

What about doing this instead

reg = readl...
if (reg & ...FSDA)
return -EBUSY;

return 0;

That way, the code will use standard errno.h return values.

>  }
>  
>  static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
> @@ -121,9 +125,7 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 
> chip, u8 read)
>   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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

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

2019-03-05 Thread Ismael Luceno Cortes 
Cosmetic change.  Any call to the recover function would need to do the
same check afterwards, so it's sensible to make it part of the function.

Signed-off-by: Ismael Luceno 
---

Notes:
Changes since v1:
- Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")
- Explained the change
- Replaced C99-style variable declaration

 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 4556b115bd..21bcf09101 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -77,12 +77,13 @@ 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;
u32 mcra = mcr | RCAR_I2C_ICMCR_FSDA;
int i;
+   u32 ret;
 
/* Send 9 SCL pulses */
for (i = 0; i < 9; i++) {
@@ -102,6 +103,9 @@ static void rcar_i2c_recover(struct udevice *dev)
udelay(5);
writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR);
udelay(5);
+
+   ret = readl(priv->base + RCAR_I2C_ICMSR);
+   return ret & RCAR_I2C_ICMCR_FSDA;
 }
 
 static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
@@ -121,9 +125,7 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, 
u8 read)
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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot