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

[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 <ismael.luceno at silicon-gears.com>
> ---
> 
> 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