Re: [PATCH] i2c: mxs: Rework the PIO mode operation

[Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>]

Hi Torsten,

> Hi Marek,
> 
> > Thinking about this, did you correctly configure the EEPROM write sector
> > size? I think it's 32 bytes for this device.
> 
> The EEPROM is correctly configured, i.e. page size is set to 32.
> 
> I made a test by writing one byte (0x01) to the address 0x0100 of the
> EEPROM. The EEPROM's I2C bus address is 0x54.
> The test has only one transfer with a message length of 3 bytes.
> 
> I figured out that the value written to the MXS_I2C_DATA register should be
> 0x010001a8. But instead of that a value of 0x00000000 is written to the
> register.
> 
> I think that the reason for that can be found in the following code
> section:
> 
> if (i + 1 == msg->len) {
> 	/* This is the last transfer. */
> 	start |= flags;
> 	start &= ~MXS_I2C_CTRL0_RETAIN_CLOCK;
> 	xlen = (i + 2) % 4;
> 	data >>= (4 - xlen) * 8;
> } else if ((i & 3) == 2) {
> 	/* Regular transfer. */
> 	xlen = 4;
> } else {
> 	/* Just stuff data. */
> 	continue;
> }
> 
> After the last byte is stuffed into the variable data (i = 2) xlen is
> calculated (with xlen = (i + 2) % 4) to zero. Thus the following shift
> operation (data >>= (4 - xlen) * 8) shifts all the bits out of data that
> already contains 0x010001a8.

You're right, thanks for finding this! Does the following fix look OK ? I added 
some more comments and "decompressed" the code some more.

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index fb6f110..ee17b6a 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -366,7 +366,7 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
        struct mxs_i2c_dev *i2c = i2c_get_adapdata(adap);
        uint32_t addr_data = msg->addr << 1;
        uint32_t data = 0;
-       int i, ret, xlen = 0;
+       int i, ret, xlen = 0, xmit = 0;
        uint32_t start;
 
        /* Mute IRQs coming from this block. */
@@ -463,20 +463,42 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter 
*adap,
                        data >>= 8;
                        data |= (msg->buf[i] << 24);
 
+                       xmit = 0;
+
+                       /* This is the last transfer of the message. */
                        if (i + 1 == msg->len) {
-                               /* This is the last transfer. */
+                               /* Add optional STOP flag. */
                                start |= flags;
+                               /* Remove RETAIN_CLOCK bit. */
                                start &= ~MXS_I2C_CTRL0_RETAIN_CLOCK;
-                               xlen = (i + 2) % 4;
-                               data >>= (4 - xlen) * 8;
-                       } else if ((i & 3) == 2) {
-                               /* Regular transfer. */
-                               xlen = 4;
-                       } else {
-                               /* Just stuff data. */
-                               continue;
+                               xmit = 1;
                        }
 
+                       /* Four bytes are ready in the "data" variable. */
+                       if ((i & 3) == 2)
+                               xmit = 1;
+
+                       /* Nothing interesting happened, continue stuffing. */
+                       if (!xmit)
+                               continue;
+
+                       /*
+                        * Compute the size of the transfer and shift the
+                        * data accordingly.
+                        *
+                        * i = (4k + 0) .... xlen = 2
+                        * i = (4k + 1) .... xlen = 3
+                        * i = (4k + 2) .... xlen = 4
+                        * i = (4k + 3) .... xlen = 1
+                        */
+
+                       if ((i % 4) == 3)
+                               xlen = 1;
+                       else
+                               xlen = (i % 4) + 2;
+
+                       data >>= (4 - xlen) * 8;
+
                        dev_dbg(i2c->dev,
                                "PIO: len=%i pos=%i total=%i [W%s%s%s]\n",
                                xlen, i, msg->len,

Best regards,
Marek Vasut