Re: [PATCH] I2C-MPC: Fix up error handling

[Jean Delvare <khali@linux-fr.org>]

Hi Kumar,

Is there a datasheet available for this chip?

> * If we have an Unfinished (MCF) or Arbitration Lost (MAL) error and
>   the bus is still busy reset the controller.  This prevents the
>   controller from getting in a hung state for transactions for other
>   devices.

What "other devices" are you talking about? If the _bus_ is busy, it
might be caused by any chip on the bus. Resetting the controller may or
may not help. But it's hard for me to say more without technical
documentation. Can you explain what the CSR_MBB bit means exactly?
Please also explain the scenario you are trying to address here.

> * Fixed up propogating the errors from i2c_wait.

Yes, I like this.

> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -115,11 +115,20 @@ static int i2c_wait(struct mpc_i2c *i2c,
>  
>  	if (!(x & CSR_MCF)) {
>  		pr_debug("I2C: unfinished\n");
> +
> +		/* reset the controller if the bus is still busy */
> +		if (x & CSR_MBB)
> +			writeccr(i2c, 0);
> +
>  		return -EIO;
>  	}
>  
>  	if (x & CSR_MAL) {
>  		pr_debug("I2C: MAL\n");
> +
> +		/* reset the controller if the bus is still busy */
> +		if (x & CSR_MBB)
> +			writeccr(i2c, 0);
>  		return -EIO;
>  	}
>  

Please try being consistent with your blank lines.

> @@ -246,8 +259,13 @@ static int mpc_xfer(struct i2c_adapter *
>  			return -EINTR;
>  		}
>  		if (time_after(jiffies, orig_jiffies + HZ)) {
> -			pr_debug("I2C: timeout\n");
> -			return -EIO;
> +			writeccr(i2c, 0);
> +
> +			/* try one more time before we error */
> +			if (readb(i2c->base + MPC_I2C_SR) & CSR_MBB) {
> +				pr_debug("I2C: timeout\n");
> +				return -EIO;
> +			}
>  		}
>  		schedule();
>  	}
> @@ -325,6 +343,7 @@ static int fsl_i2c_probe(struct platform
>  			goto fail_irq;
>  		}
>  
> +	writeccr(i2c, 0);
>  	mpc_i2c_setclock(i2c);
>  	platform_set_drvdata(pdev, i2c);

These last two changes are not mentioned in your header comment. What
are they? Why are they needed? They look like hacks to me.

Thanks,
-- 
Jean Delvare