lkml.org 
[lkml]   [2006]   [Apr]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] I2C-MPC: Fix up error handling
    Date

    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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-04-25 16:39    [W:9.108 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site