Hi Joakim:

Am 18.02.10 18:14 schrieb(en) Joakim Tjernlund:
> > [snip]
> > > >   static void mpc_i2c_fixup(struct mpc_i2c *i2c)
> > > >   {
> > > > -   writeccr(i2c, 0);
> > > > -   udelay(30);
> > > > -   writeccr(i2c, CCR_MEN);
> > > > -   udelay(30);
> > > > -   writeccr(i2c, CCR_MSTA | CCR_MTX);
> > > > -   udelay(30);
> > > > -   writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> > > > -   udelay(30);
> > > > -   writeccr(i2c, CCR_MEN);
> > > > -   udelay(30);
> > > > +   int k;
> > > > +   u32 delay_val = 1000000 / i2c->real_clk + 1;
> > > > +
> > > > +   if (delay_val < 2)
> > > > +      delay_val = 2;
> > > > +
> > > > +   for (k = 9; k; k--) {
> > > > +      writeccr(i2c, 0);
> > > > +      writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
> > > > +      udelay(delay_val);
> > > > +      writeccr(i2c, CCR_MEN);
> > > > +      udelay(delay_val << 1);
> > > > +   }
> > > >   }
> > >
>>> I am curious, didn't old method work with by just wrapping a for(k=9; k; k--) around it? How did the wave form look?
> >
>> Sure does that work!  The waveform was somewhat "streched", mainly due to the delays between some of the writeccr() calls which don't change the sda/scl lines.  Unfortunately I didn't take shots from the scope.
> 
> Yeah, the long delays has to go. So the wave form was the same but more stretched in time? I ask because I don't understand the writeccr(i2c, CCR_MSTA | CCR_MTX); is supposed to do.

Afaict, this is really a no-op.  The '5200 user's manual says about MEN

<snip>
* 0 module is reset and disabled. This is the Power-ON reset. When low the interface is held in reset, but registers can still be accessed.
* 1 I2C module is enabled. Bit must be set before other CR bits have any effect.
</snip>

The change in the MSTA is needed -with the proper delays- as to generate the START and STOP conditions.

Unfortunately, the data sheet is not very clear (or my English too bad), but reading it *after* seeing the signals on the scope, I can at least guess what they mean :-/

Thus, the old code will probably produce SDA and SCL held high for ~90us, then a SDA/SCL low for ~30us (plus/minus the delays the hw adds internally according to the clock setting), and then a ~30us SDA/SCL high.  It is not possible to get the necessary delays from the data sheet, but as I said I empirically verified some cases to be safe.

> The old code only works when the device is stuck at the last bit. To cope with any bit (worst case is the first bit) you need 9 cycles, 8 bits + ack = 9
> 
> Just toggling the clock 9 cycles should unlock any slave stuck in a read operation. To unlock slaves stuck in a write operation you also need to generate a START in every cycle too.

Yes, of course...  this was the initial motivation of the patch, as I *have* a slave which sometimes needs more than one cycle!

> As far as I can tell your patch does all of the above so
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>

Thanks a lot again for your time, and your helpful comments!

Best, Albrecht.