i2c: mxs: Rework the PIO mode operation

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.

                                xlen, i, msg->len,

Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Marek,

> Hi Torsten,
> 
> 
> 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,

The patch looks OK. I will test it when I'm back in the office. But this is 
not before August 12.

Best regards,
Torsten Fleischer

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html