[U-Boot] [PATCH 1/2] i2c: mxc_i2c: Document how non-DM functions work

[U-Boot] [PATCH 1/2] i2c: mxc_i2c: Document how non-DM functions work

[Trent Piepho]

It is not very clear how these work in relation to the exact I2C xfers
they produce.  In paticular, the address length is somewhat overloaded
in the read method.  Clearly document the existing behavior.  Maybe this
will help the next person who needs to work on this driver and not break
non-DM boards.

Cc: Nandor Han <nandor.han@ge.com>
Cc: Heiko Schocher <hs@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Breno Matheus Lima <brenomatheus@gmail.com>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 drivers/i2c/mxc_i2c.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index 5420afbc8e..f17a47f600 100644
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -540,6 +540,25 @@ static int i2c_read_data(struct mxc_i2c_bus *i2c_bus, uchar chip, uchar *buf,
 #ifndef CONFIG_DM_I2C
 /*
  * Read data from I2C device
+ *
+ * The transactions use the syntax defined in the Linux kernel I2C docs.
+ *
+ * If alen is > 0, then this function will send a transaction of the form:
+ *     S Chip Wr [A] Addr [A] S Chip Rd [A] [data] A ... NA P
+ * This is a normal I2C register read: writing the register address, then doing
+ * a repeated start and reading the data.
+ *
+ * If alen == 0, then we get this transaction:
+ *     S Chip Wr [A] S Chip Rd [A] [data] A ... NA P
+ * This is somewhat unusual, though valid, transaction.  It addresses the chip
+ * in write mode, but doesn't actually write any register address or data, then
+ * does a repeated start and reads data.
+ *
+ * If alen < 0, then we get this transaction:
+ *     S Chip Rd [A] [data] A ... NA P
+ * The chip is addressed in read mode and then data is read.  No register
+ * address is written first.  This is perfectly valid on most devices and
+ * required on some (usually those that don't act like an array of registers).
  */
 static int bus_i2c_read(struct mxc_i2c_bus *i2c_bus, u8 chip, u32 addr,
 			int alen, u8 *buf, int len)
@@ -574,6 +593,20 @@ static int bus_i2c_read(struct mxc_i2c_bus *i2c_bus, u8 chip, u32 addr,
 
 /*
  * Write data to I2C device
+ *
+ * If alen > 0, we get this transaction:
+ *    S Chip Wr [A] addr [A] data [A] ... [A] P
+ * An ordinary write register command.
+ *
+ * If alen == 0, then we get this:
+ *    S Chip Wr [A] data [A] ... [A] P
+ * This is a simple I2C write.
+ *
+ * If alen < 0, then we get this:
+ *    S data [A] ... [A] P
+ * This is most likely NOT something that should be used.  It doesn't send the
+ * chip address first, so in effect, the first byte of data will be used as the
+ * address.
  */
 static int bus_i2c_write(struct mxc_i2c_bus *i2c_bus, u8 chip, u32 addr,
 			 int alen, const u8 *buf, int len)
@@ -881,6 +914,7 @@ static int mxc_i2c_probe(struct udevice *bus)
 	return 0;
 }
 
+/* Sends: S Addr Wr [A|NA] P */
 static int mxc_i2c_probe_chip(struct udevice *bus, u32 chip_addr,
 			      u32 chip_flags)
 {
-- 
2.14.5

[U-Boot] [PATCH 2/2] i2c: mxc_i2c: Fix read and read->write xfers in DM mode

[Trent Piepho]

This is an old driver that supports both device mapped and non-mapped
mode, and covers a wide range of hardware.  It's hard to change without
risking breaking something.  I have to tried to be exceedingly detailed
in this patch, so please excuse the length of the commit essay that
follows.

In device mapped mode the I2C xfer function does not handle plain read,
and some other, transfers correctly.

What it can't handle are transactions that:
    Start with a read, or,
    Have a write followed by a read, or,
    Have more than one read in a row.

The common I2C/SMBUS read register and write register transactions
always start with a write, followed by a write or a read, and then end.
These work, so the bug is not apparent for most I2C slaves that only use
these common xfer forms.

The existing xfer loop initializes by sending the chip address in write
mode after it deals with bus arbitration and master setup.  When
processing each message, if the next message will be a read, it sends a
repeated start followed by the chip address in read mode after the
current message.

Obviously, this does not work if the first message is a read, as the
chip is always addressed in write mode initially by i2c_init_transfer().

A write following a read does not work because the repeated start is
only sent when the next message is a read.  There is no logic to send it
when the current message is a read and next is write.  It should be sent
every time the bus changes direction.

The ability to use a plain read was added to this driver in
commit 2feec4eafd40 ("imx: mxc_i2c: tweak the i2c transfer method"),
but this applied only the non-DM code path.

This patch fixes the DM code path.  The xfer function will call
i2c_init_transfer() with an alen of -1 to avoid sending the chip
address.  The same way the non-DM code achieves this.  The xfer
function's message loop will send the address and mode before each
message if the bus changes direction, and on the first message.

When reading data, the master hardware is one byte ahead of what we
receive.  I.e., reading a byte from the data register returns a byte
*already received* by the master, and causes the master to start the RX
of the *next* byte.  Therefor, before we read the final byte of a
message, we must tell the master what to do next.  I add a "last" flag
to i2c_read_data() to tell it if the message is to be followed by a stop
or a repeated start.  When last == true it acts exactly as before.

The non-DM code can only create an xfer where the read, if any, is the
final message of the xfer.  And so the only callsite of i2c_read_data()
in the non-DM code has the "last" parameter as true.  Therefore, this
change has no effect on the non-DM code.  As all other changes are in
the DM xfer function, which is not even compiled in non-DM code, I am
confident that this patch has no effect on boards not using I2C_DM.
This greatly reduces the change of hardware that could be affected.

For DM boards, I have verified every transaction the "i2c" command can
create on a scope and they are all exactly as they are supposed to be.
I also tested write->read->write, which isn't possible with the i2c
command, and it works as well.  I didn't fix multiple reads in a row, as
it's a lot more invasive and obviously no one has every wanted them
since they've never worked.  It didn't seem like the extra complexity
was justified to support something no one uses.

Cc: Nandor Han <nandor.han@ge.com>
Cc: Heiko Schocher <hs@denx.de>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Breno Matheus Lima <brenomatheus@gmail.com>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 drivers/i2c/mxc_i2c.c | 95 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 64 insertions(+), 31 deletions(-)

diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index f17a47f600..23119cce65 100644
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -482,8 +482,13 @@ static int i2c_write_data(struct mxc_i2c_bus *i2c_bus, u8 chip, const u8 *buf,
 	return ret;
 }
 
+/* Will generate a STOP after the last byte if "last" is true, i.e. this is the
+ * final message of a transaction.  If not, it switches the bus back to TX mode
+ * and does not send a STOP, leaving the bus in a state where a repeated start
+ * and address can be sent for another message.
+ */
 static int i2c_read_data(struct mxc_i2c_bus *i2c_bus, uchar chip, uchar *buf,
-			 int len)
+			 int len, bool last)
 {
 	int ret;
 	unsigned int temp;
@@ -513,17 +518,31 @@ static int i2c_read_data(struct mxc_i2c_bus *i2c_bus, uchar chip, uchar *buf,
 			return ret;
 		}
 
-		/*
-		 * It must generate STOP before read I2DR to prevent
-		 * controller from generating another clock cycle
-		 */
 		if (i == (len - 1)) {
-			i2c_imx_stop(i2c_bus);
+			/* Final byte has already been received by master!  When
+			 * we read it from I2DR, the master will start another
+			 * cycle.  We must program it first to send a STOP or
+			 * switch to TX to avoid this.
+			 */
+			if (last) {
+				i2c_imx_stop(i2c_bus);
+			} else {
+				/* Final read, no stop, switch back to tx */
+				temp = readb(base + (I2CR << reg_shift));
+				temp |= I2CR_MTX | I2CR_TX_NO_AK;
+				writeb(temp, base + (I2CR << reg_shift));
+			}
 		} else if (i == (len - 2)) {
+			/* Master has already recevied penultimate byte.  When
+			 * we read it from I2DR, master will start RX of final
+			 * byte.  We must set TX_NO_AK now so it does not ACK
+			 * that final byte.
+			 */
 			temp = readb(base + (I2CR << reg_shift));
 			temp |= I2CR_TX_NO_AK;
 			writeb(temp, base + (I2CR << reg_shift));
 		}
+
 		writeb(I2SR_IIF_CLEAR, base + (I2SR << reg_shift));
 		buf[i] = readb(base + (I2DR << reg_shift));
 	}
@@ -533,7 +552,9 @@ static int i2c_read_data(struct mxc_i2c_bus *i2c_bus, uchar chip, uchar *buf,
 		debug(" 0x%02x", buf[ret]);
 	debug("\n");
 
-	i2c_imx_stop(i2c_bus);
+	/* It is not clear to me that this is necessary */
+	if (last)
+		i2c_imx_stop(i2c_bus);
 	return 0;
 }
 
@@ -585,7 +606,7 @@ static int bus_i2c_read(struct mxc_i2c_bus *i2c_bus, u8 chip, u32 addr,
 		return ret;
 	}
 
-	ret = i2c_read_data(i2c_bus, chip, buf, len);
+	ret = i2c_read_data(i2c_bus, chip, buf, len, true);
 
 	i2c_imx_stop(i2c_bus);
 	return ret;
@@ -939,42 +960,54 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
 	ulong base = i2c_bus->base;
 	int reg_shift = i2c_bus->driver_data & I2C_QUIRK_FLAG ?
 		VF610_I2C_REGSHIFT : IMX_I2C_REGSHIFT;
+	int read_mode;
 
-	/*
-	 * Here the 3rd parameter addr and the 4th one alen are set to 0,
-	 * because here we only want to send out chip address. The register
-	 * address is wrapped in msg.
+	/* Here address len is set to -1 to not send any address at first.
+	 * Otherwise i2c_init_transfer will send the chip address with write
+	 * mode set.  This is wrong if the 1st message is read.
 	 */
-	ret = i2c_init_transfer(i2c_bus, msg->addr, 0, 0);
+	ret = i2c_init_transfer(i2c_bus, msg->addr, 0, -1);
 	if (ret < 0) {
 		debug("i2c_init_transfer error: %d\n", ret);
 		return ret;
 	}
 
+	read_mode = -1; /* So it's always different on the first message */
 	for (; nmsgs > 0; nmsgs--, msg++) {
-		bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD);
-		debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len);
-		if (msg->flags & I2C_M_RD)
-			ret = i2c_read_data(i2c_bus, msg->addr, msg->buf,
-					    msg->len);
-		else {
-			ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
-					     msg->len);
-			if (ret)
-				break;
-			if (next_is_read) {
-				/* Reuse ret */
+		const int msg_is_read = !!(msg->flags & I2C_M_RD);
+
+		debug("i2c_xfer: chip=0x%x, len=0x%x, dir=%c\n", msg->addr,
+		      msg->len, msg_is_read ? 'R' : 'W');
+
+		if (msg_is_read != read_mode) {
+			/* Send repeated start if not 1st message */
+			if (read_mode != -1) {
+				debug("i2c_xfer: [RSTART]\n");
 				ret = readb(base + (I2CR << reg_shift));
 				ret |= I2CR_RSTA;
 				writeb(ret, base + (I2CR << reg_shift));
-
-				ret = tx_byte(i2c_bus, (msg->addr << 1) | 1);
-				if (ret < 0) {
-					i2c_imx_stop(i2c_bus);
-					break;
-				}
 			}
+			debug("i2c_xfer: [ADDR %02x | %c]\n", msg->addr,
+			      msg_is_read ? 'R' : 'W');
+			ret = tx_byte(i2c_bus, (msg->addr << 1) | msg_is_read);
+			if (ret < 0) {
+				debug("i2c_xfer: [STOP]\n");
+				i2c_imx_stop(i2c_bus);
+				break;
+			}
+			read_mode = msg_is_read;
 		}
+
+		if (msg->flags & I2C_M_RD)
+			ret = i2c_read_data(i2c_bus, msg->addr, msg->buf,
+					    msg->len, nmsgs == 1 ||
+						      (msg->flags & I2C_M_STOP));
+		else
+			ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
+					     msg->len);
+
+		if (ret < 0)
+			break;
 	}
 
 	if (ret)
-- 
2.14.5

[U-Boot] [PATCH 1/2] i2c: mxc_i2c: Document how non-DM functions work

[Heiko Schocher]

Hello Trent,

Am 16.04.2019 um 00:02 schrieb Trent Piepho:
> It is not very clear how these work in relation to the exact I2C xfers
> they produce.  In paticular, the address length is somewhat overloaded
> in the read method.  Clearly document the existing behavior.  Maybe this
> will help the next person who needs to work on this driver and not break
> non-DM boards.
> 
> Cc: Nandor Han <nandor.han@ge.com>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Breno Matheus Lima <brenomatheus@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>
> ---
>   drivers/i2c/mxc_i2c.c | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)

Thanks, but your patch has a lot of checkpatch errors:

total: 34 errors, 0 warnings, 0 checks, 52 lines checked

please fix this and send a v2.

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH 2/2] i2c: mxc_i2c: Fix read and read->write xfers in DM mode

[Heiko Schocher]

Hello Trent,

Am 16.04.2019 um 00:02 schrieb Trent Piepho:
> This is an old driver that supports both device mapped and non-mapped
> mode, and covers a wide range of hardware.  It's hard to change without
> risking breaking something.  I have to tried to be exceedingly detailed
> in this patch, so please excuse the length of the commit essay that
> follows.
> 
> In device mapped mode the I2C xfer function does not handle plain read,
> and some other, transfers correctly.
> 
> What it can't handle are transactions that:
>      Start with a read, or,
>      Have a write followed by a read, or,
>      Have more than one read in a row.
> 
> The common I2C/SMBUS read register and write register transactions
> always start with a write, followed by a write or a read, and then end.
> These work, so the bug is not apparent for most I2C slaves that only use
> these common xfer forms.
> 
> The existing xfer loop initializes by sending the chip address in write
> mode after it deals with bus arbitration and master setup.  When
> processing each message, if the next message will be a read, it sends a
> repeated start followed by the chip address in read mode after the
> current message.
> 
> Obviously, this does not work if the first message is a read, as the
> chip is always addressed in write mode initially by i2c_init_transfer().
> 
> A write following a read does not work because the repeated start is
> only sent when the next message is a read.  There is no logic to send it
> when the current message is a read and next is write.  It should be sent
> every time the bus changes direction.
> 
> The ability to use a plain read was added to this driver in
> commit 2feec4eafd40 ("imx: mxc_i2c: tweak the i2c transfer method"),
> but this applied only the non-DM code path.
> 
> This patch fixes the DM code path.  The xfer function will call
> i2c_init_transfer() with an alen of -1 to avoid sending the chip
> address.  The same way the non-DM code achieves this.  The xfer
> function's message loop will send the address and mode before each
> message if the bus changes direction, and on the first message.
> 
> When reading data, the master hardware is one byte ahead of what we
> receive.  I.e., reading a byte from the data register returns a byte
> *already received* by the master, and causes the master to start the RX
> of the *next* byte.  Therefor, before we read the final byte of a
> message, we must tell the master what to do next.  I add a "last" flag
> to i2c_read_data() to tell it if the message is to be followed by a stop
> or a repeated start.  When last == true it acts exactly as before.
> 
> The non-DM code can only create an xfer where the read, if any, is the
> final message of the xfer.  And so the only callsite of i2c_read_data()
> in the non-DM code has the "last" parameter as true.  Therefore, this
> change has no effect on the non-DM code.  As all other changes are in
> the DM xfer function, which is not even compiled in non-DM code, I am
> confident that this patch has no effect on boards not using I2C_DM.
> This greatly reduces the change of hardware that could be affected.
> 
> For DM boards, I have verified every transaction the "i2c" command can
> create on a scope and they are all exactly as they are supposed to be.
> I also tested write->read->write, which isn't possible with the i2c
> command, and it works as well.  I didn't fix multiple reads in a row, as
> it's a lot more invasive and obviously no one has every wanted them
> since they've never worked.  It didn't seem like the extra complexity
> was justified to support something no one uses.
> 
> Cc: Nandor Han <nandor.han@ge.com>
> Cc: Heiko Schocher <hs@denx.de>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Breno Matheus Lima <brenomatheus@gmail.com>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>
> ---
>   drivers/i2c/mxc_i2c.c | 95 ++++++++++++++++++++++++++++++++++-----------------
>   1 file changed, 64 insertions(+), 31 deletions(-)

Thanks for this work!

Your patch has

total: 64 errors, 2 warnings, 0 checks, 145 lines checked

Please fix and send a v2.

It would be good to become here some Tested by tags ...

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> index f17a47f600..23119cce65 100644
> --- a/drivers/i2c/mxc_i2c.c
> +++ b/drivers/i2c/mxc_i2c.c
> @@ -482,8 +482,13 @@ static int i2c_write_data(struct mxc_i2c_bus *i2c_bus, u8 chip, const u8 *buf,
>   	return ret;
>   }
>   
> +/* Will generate a STOP after the last byte if "last" is true, i.e. this is the
> + * final message of a transaction.  If not, it switches the bus back to TX mode
> + * and does not send a STOP, leaving the bus in a state where a repeated start
> + * and address can be sent for another message.
> + */
>   static int i2c_read_data(struct mxc_i2c_bus *i2c_bus, uchar chip, uchar *buf,
> -			 int len)
> +			 int len, bool last)
>   {
>   	int ret;
>   	unsigned int temp;
> @@ -513,17 +518,31 @@ static int i2c_read_data(struct mxc_i2c_bus *i2c_bus, uchar chip, uchar *buf,
>   			return ret;
>   		}
>   
> -		/*
> -		 * It must generate STOP before read I2DR to prevent
> -		 * controller from generating another clock cycle
> -		 */
>   		if (i == (len - 1)) {
> -			i2c_imx_stop(i2c_bus);
> +			/* Final byte has already been received by master!  When
> +			 * we read it from I2DR, the master will start another
> +			 * cycle.  We must program it first to send a STOP or
> +			 * switch to TX to avoid this.
> +			 */
> +			if (last) {
> +				i2c_imx_stop(i2c_bus);
> +			} else {
> +				/* Final read, no stop, switch back to tx */
> +				temp = readb(base + (I2CR << reg_shift));
> +				temp |= I2CR_MTX | I2CR_TX_NO_AK;
> +				writeb(temp, base + (I2CR << reg_shift));
> +			}
>   		} else if (i == (len - 2)) {
> +			/* Master has already recevied penultimate byte.  When
> +			 * we read it from I2DR, master will start RX of final
> +			 * byte.  We must set TX_NO_AK now so it does not ACK
> +			 * that final byte.
> +			 */
>   			temp = readb(base + (I2CR << reg_shift));
>   			temp |= I2CR_TX_NO_AK;
>   			writeb(temp, base + (I2CR << reg_shift));
>   		}
> +
>   		writeb(I2SR_IIF_CLEAR, base + (I2SR << reg_shift));
>   		buf[i] = readb(base + (I2DR << reg_shift));
>   	}
> @@ -533,7 +552,9 @@ static int i2c_read_data(struct mxc_i2c_bus *i2c_bus, uchar chip, uchar *buf,
>   		debug(" 0x%02x", buf[ret]);
>   	debug("\n");
>   
> -	i2c_imx_stop(i2c_bus);
> +	/* It is not clear to me that this is necessary */
> +	if (last)
> +		i2c_imx_stop(i2c_bus);
>   	return 0;
>   }
>   
> @@ -585,7 +606,7 @@ static int bus_i2c_read(struct mxc_i2c_bus *i2c_bus, u8 chip, u32 addr,
>   		return ret;
>   	}
>   
> -	ret = i2c_read_data(i2c_bus, chip, buf, len);
> +	ret = i2c_read_data(i2c_bus, chip, buf, len, true);
>   
>   	i2c_imx_stop(i2c_bus);
>   	return ret;
> @@ -939,42 +960,54 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
>   	ulong base = i2c_bus->base;
>   	int reg_shift = i2c_bus->driver_data & I2C_QUIRK_FLAG ?
>   		VF610_I2C_REGSHIFT : IMX_I2C_REGSHIFT;
> +	int read_mode;
>   
> -	/*
> -	 * Here the 3rd parameter addr and the 4th one alen are set to 0,
> -	 * because here we only want to send out chip address. The register
> -	 * address is wrapped in msg.
> +	/* Here address len is set to -1 to not send any address at first.
> +	 * Otherwise i2c_init_transfer will send the chip address with write
> +	 * mode set.  This is wrong if the 1st message is read.
>   	 */
> -	ret = i2c_init_transfer(i2c_bus, msg->addr, 0, 0);
> +	ret = i2c_init_transfer(i2c_bus, msg->addr, 0, -1);
>   	if (ret < 0) {
>   		debug("i2c_init_transfer error: %d\n", ret);
>   		return ret;
>   	}
>   
> +	read_mode = -1; /* So it's always different on the first message */
>   	for (; nmsgs > 0; nmsgs--, msg++) {
> -		bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD);
> -		debug("i2c_xfer: chip=0x%x, len=0x%x\n", msg->addr, msg->len);
> -		if (msg->flags & I2C_M_RD)
> -			ret = i2c_read_data(i2c_bus, msg->addr, msg->buf,
> -					    msg->len);
> -		else {
> -			ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
> -					     msg->len);
> -			if (ret)
> -				break;
> -			if (next_is_read) {
> -				/* Reuse ret */
> +		const int msg_is_read = !!(msg->flags & I2C_M_RD);
> +
> +		debug("i2c_xfer: chip=0x%x, len=0x%x, dir=%c\n", msg->addr,
> +		      msg->len, msg_is_read ? 'R' : 'W');
> +
> +		if (msg_is_read != read_mode) {
> +			/* Send repeated start if not 1st message */
> +			if (read_mode != -1) {
> +				debug("i2c_xfer: [RSTART]\n");
>   				ret = readb(base + (I2CR << reg_shift));
>   				ret |= I2CR_RSTA;
>   				writeb(ret, base + (I2CR << reg_shift));
> -
> -				ret = tx_byte(i2c_bus, (msg->addr << 1) | 1);
> -				if (ret < 0) {
> -					i2c_imx_stop(i2c_bus);
> -					break;
> -				}
>   			}
> +			debug("i2c_xfer: [ADDR %02x | %c]\n", msg->addr,
> +			      msg_is_read ? 'R' : 'W');
> +			ret = tx_byte(i2c_bus, (msg->addr << 1) | msg_is_read);
> +			if (ret < 0) {
> +				debug("i2c_xfer: [STOP]\n");
> +				i2c_imx_stop(i2c_bus);
> +				break;
> +			}
> +			read_mode = msg_is_read;
>   		}
> +
> +		if (msg->flags & I2C_M_RD)
> +			ret = i2c_read_data(i2c_bus, msg->addr, msg->buf,
> +					    msg->len, nmsgs == 1 ||
> +						      (msg->flags & I2C_M_STOP));
> +		else
> +			ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
> +					     msg->len);
> +
> +		if (ret < 0)
> +			break;
>   	}
>   
>   	if (ret)
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH 2/2] i2c: mxc_i2c: Fix read and read->write xfers in DM mode

[Trent Piepho]

On Tue, 2019-04-30 at 06:34 +0200, Heiko Schocher wrote:
> Hello Trent,
> 
> Am 16.04.2019 um 00:02 schrieb Trent Piepho:
> > This is an old driver that supports both device mapped and non-mapped
> > mode, and covers a wide range of hardware.  It's hard to change without
> > risking breaking something.  I have to tried to be exceedingly detailed
> > in this patch, so please excuse the length of the commit essay that
> > follows.
> > 
> Thanks for this work!
> 
> Your patch has
> 
> total: 64 errors, 2 warnings, 0 checks, 145 lines checked
> 
> Please fix and send a v2.
> 
> It would be good to become here some Tested by tags ...
> 
> Reviewed-by: Heiko Schocher <hs@denx.de>

My patches are fine before I send them.  They are getting DOS line
endings added in transit.  It seems Microsoft has recently changed
their mail server used for office365 to automatically add DOS line
endings to all text emails.  It didn't use to do this.  It seems the
only way to avoid this is to use base64 encoding with git send-email. 
It works went I send a test message to myself and export from
Evolution.  No way to test if the list software can handle it without
re-sending.

The patches should be automatically fixed by git am, as it will strip
DOS endings by default.

[U-Boot] [PATCH 2/2] i2c: mxc_i2c: Fix read and read->write xfers in DM mode

[Heiko Schocher]

Hello Trent,

Am 30.04.2019 um 18:04 schrieb Trent Piepho:
> On Tue, 2019-04-30 at 06:34 +0200, Heiko Schocher wrote:
>> Hello Trent,
>>
>> Am 16.04.2019 um 00:02 schrieb Trent Piepho:
>>> This is an old driver that supports both device mapped and non-mapped
>>> mode, and covers a wide range of hardware.  It's hard to change without
>>> risking breaking something.  I have to tried to be exceedingly detailed
>>> in this patch, so please excuse the length of the commit essay that
>>> follows.
>>>
>> Thanks for this work!
>>
>> Your patch has
>>
>> total: 64 errors, 2 warnings, 0 checks, 145 lines checked
>>
>> Please fix and send a v2.
>>
>> It would be good to become here some Tested by tags ...
>>
>> Reviewed-by: Heiko Schocher <hs@denx.de>
> 
> My patches are fine before I send them.  They are getting DOS line
> endings added in transit.  It seems Microsoft has recently changed
> their mail server used for office365 to automatically add DOS line
> endings to all text emails.  It didn't use to do this.  It seems the
> only way to avoid this is to use base64 encoding with git send-email.
> It works went I send a test message to myself and export from
> Evolution.  No way to test if the list software can handle it without
> re-sending.
> 
> The patches should be automatically fixed by git am, as it will strip
> DOS endings by default.

Unfortunately, your v2 version has the same problem :-(

I pushed them now to:
https://github.com/hsdenx/u-boot-i2c/commits/master

as I started a travis build, see:
https://travis-ci.org/hsdenx/u-boot-i2c/builds/527155944

Can you please check, if your patches are OK?

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH 2/2] i2c: mxc_i2c: Fix read and read->write xfers in DM mode

[Trent Piepho]

On Thu, 2019-05-02 at 07:23 +0200, Heiko Schocher wrote:
> Am 30.04.2019 um 18:04 schrieb Trent Piepho:
> > On Tue, 2019-04-30 at 06:34 +0200, Heiko Schocher wrote:
> > > Am 16.04.2019 um 00:02 schrieb Trent Piepho:
> > > > This is an old driver that supports both device mapped and non-mapped
> > > > mode, and covers a wide range of hardware.  It's hard to change without
> > > > risking breaking something.  I have to tried to be exceedingly detailed
> > > > in this patch, so please excuse the length of the commit essay that
> > > > follows.
> > > > 
> > > 
> > 
> > The patches should be automatically fixed by git am, as it will strip
> > DOS endings by default.
> 
> Unfortunately, your v2 version has the same problem :-(
> 
> Can you please check, if your patches are OK?

They look correct, thanks for applying them.

I'm not sure where the mangling is happening.  I assure you the patches
 I'm sending are correct.  They arrive at both my work and gmail
account unmangled.  Yet somehow the patchwork mbox file has DOS line
endings added to the diff portion but not the commit message.  I wonder
if the flaw is somewhere in in the denx list server or in patchwork?

Of course, if no one else's patches get mangled, it's probably
something about microsoft's email server that is the problem.