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

[Kumar Gala]

On Apr 25, 2006, at 8:22 AM, Jean Delvare wrote:

> 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.

Here's the definintion of CSR_MBB:

Bus busy. Indicates the status of the bus. When a START condition is  
detected, MBB is set. If a STOP condition
is detected, it is cleared.

What I meant is that I have a I2C slave device which is hanging up  
the bus on some transactions.  In those cases we will end up in one  
of the error conditions CSR_MCF, or CSR_MAL.  If I don't reset the  
controller all future transactions regardless of which device they  
are to fail.

> > * 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.

Sorry about that, figured they fell in a catch all.

The first is an attempt to reduce the errors related to the buggy  
slave device.

The second (writeccr(i2c, 0)) is just ensure the controller is in a  
known state when we startup.

The

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/