[U-Boot] [PATCH v2 1/3] dm: i2c: Add a flag that needs to generate a stop bit

[U-Boot] [PATCH v2 1/3] dm: i2c: Add a flag that needs to generate a stop bit

[Chuanhua Han]

Usually the i2c bus needs to write the address of the register before
reading the internal register data of the device (ignoring the
transmission of the slave address).

Generally, the stop signal is not needed before the register is read,
but there is a special chip that needs this stop signal (such as
pcf2127), this patch adds a flag that needs to generate a stop
bit to determine whether the i2c host needs to generate a stop signal
before reading the register data.

Signed-off-by: Biwen Li <biwen.li@nxp.com>
Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
---
Changes in v2:
	- Split the original patch into 3 patches
	- Add detailed description information for each patch

 drivers/i2c/i2c-uclass.c | 2 ++
 include/i2c.h            | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
index e47abf1833..18f7364d72 100644
--- a/drivers/i2c/i2c-uclass.c
+++ b/drivers/i2c/i2c-uclass.c
@@ -141,6 +141,8 @@ int dm_i2c_read(struct udevice *dev, uint offset, uint8_t *buffer, int len)
 	if (len) {
 		ptr->addr = chip->chip_addr;
 		ptr->flags = chip->flags & DM_I2C_CHIP_10BIT ? I2C_M_TEN : 0;
+		ptr->flags |= chip->flags & DM_I2C_CHIP_RD_NEED_STOP_BIT ?
+			      I2C_M_RD_NEED_STOP_BIT : 0;
 		ptr->flags |= I2C_M_RD;
 		ptr->len = len;
 		ptr->buf = buffer;
diff --git a/include/i2c.h b/include/i2c.h
index a5c760c711..beaa028349 100644
--- a/include/i2c.h
+++ b/include/i2c.h
@@ -28,6 +28,7 @@ enum dm_i2c_chip_flags {
 	DM_I2C_CHIP_10BIT	= 1 << 0, /* Use 10-bit addressing */
 	DM_I2C_CHIP_RD_ADDRESS	= 1 << 1, /* Send address for each read byte */
 	DM_I2C_CHIP_WR_ADDRESS	= 1 << 2, /* Send address for each write byte */
+	DM_I2C_CHIP_RD_NEED_STOP_BIT    = 1 << 3, /* Need generate stop bit */
 };
 
 struct udevice;
@@ -87,6 +88,7 @@ enum dm_i2c_msg_flags {
 	I2C_M_IGNORE_NAK	= 0x1000, /* continue after NAK */
 	I2C_M_NO_RD_ACK		= 0x0800, /* skip the Ack bit on reads */
 	I2C_M_RECV_LEN		= 0x0400, /* length is first received byte */
+	I2C_M_RD_NEED_STOP_BIT  = 0x0002, /* need generate stop bit */
 };
 
 /**
-- 
2.17.1

[U-Boot] [PATCH v2 2/3] i2c: mxc_i2c: The i2c controller generates a stop signal before reading the register data

[Chuanhua Han]

This patch enables the i2c controller to generate a stop signal before
reading the slave device's internal register after setting the register
address (need to determine if the signal is needed according to the
message flag).

Signed-off-by: Biwen Li <biwen.li@nxp.com>
Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
---
Changes in v2: 
	- Split the original patch into 3 patches
	- Add detailed description information for each patch

 drivers/i2c/mxc_i2c.c | 70 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 62 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index 23119cce65..f3811eb0c5 100644
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -961,6 +961,8 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
 	int reg_shift = i2c_bus->driver_data & I2C_QUIRK_FLAG ?
 		VF610_I2C_REGSHIFT : IMX_I2C_REGSHIFT;
 	int read_mode;
+	bool quirk = i2c_bus->driver_data & I2C_QUIRK_FLAG ? true : false;
+	unsigned int temp;
 
 	/* 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
@@ -975,6 +977,7 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
 	read_mode = -1; /* So it's always different on the first message */
 	for (; nmsgs > 0; nmsgs--, msg++) {
 		const int msg_is_read = !!(msg->flags & I2C_M_RD);
+		bool next_is_read = nmsgs > 1 && (msg[1].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');
@@ -982,13 +985,16 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
 		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));
+				if (!(msg[1].flags & I2C_M_RD_NEED_STOP_BIT)) {
+					debug("i2c_xfer: [RSTART]\n");
+					ret = readb(base + (I2CR << reg_shift));
+					ret |= I2CR_RSTA;
+					writeb(ret, base + (I2CR << reg_shift));
+				}
 			}
 			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");
@@ -998,16 +1004,64 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
 			read_mode = msg_is_read;
 		}
 
-		if (msg->flags & I2C_M_RD)
+		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;
+			continue;
+		}
 
+		 /* Write message */
+		ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
+				     msg->len);
 		if (ret < 0)
 			break;
+
+		if (!next_is_read)
+			continue;
+
+		/* Read message following write message */
+		if (msg[1].flags & I2C_M_RD_NEED_STOP_BIT) {
+			/* Generate a stop bit */
+			i2c_imx_stop(i2c_bus);
+			/* Reset i2c slave */
+			i2c_force_reset_slave();
+
+			/* Enable I2C controller */
+			if (quirk)
+				ret = readb(base + (I2CR << reg_shift))
+					& I2CR_IDIS;
+			else
+				ret = !(readb(base + (I2CR << reg_shift))
+						& I2CR_IEN);
+			if (ret) {
+				writeb(I2CR_IEN, base + (I2CR << reg_shift));
+				/* Wait for controller to be stable */
+				udelay(50);
+			}
+
+			/* Clear interrupt bit */
+			writeb(I2SR_IIF_CLEAR, base + (I2SR << reg_shift));
+			ret = wait_for_sr_state(i2c_bus, ST_BUS_IDLE);
+			if (ret < 0)
+				return ret;
+
+			/* Start I2C transaction */
+			temp = readb(base + (I2CR << reg_shift));
+			temp |= I2CR_MSTA;
+			writeb(temp, base + (I2CR << reg_shift));
+
+			ret = wait_for_sr_state(i2c_bus, ST_BUS_BUSY);
+			if (ret < 0)
+				return ret;
+
+			/* Enter transfer mode */
+			temp |= I2CR_MTX | I2CR_TX_NO_AK;
+			writeb(temp, base + (I2CR << reg_shift));
+			udelay(50);
+		}
 	}
 
 	if (ret)
-- 
2.17.1

[U-Boot] [PATCH v2 2/3] i2c: mxc_i2c: The i2c controller generates a stop signal before reading the register data

[Lukasz Majewski]

Hi Chuanhua,

> This patch enables the i2c controller to generate a stop signal before
> reading the slave device's internal register after setting the
> register address (need to determine if the signal is needed according
> to the message flag).
> 
> Signed-off-by: Biwen Li <biwen.li@nxp.com>
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
> Changes in v2: 
> 	- Split the original patch into 3 patches
> 	- Add detailed description information for each patch
> 
>  drivers/i2c/mxc_i2c.c | 70

I must admit that I'm a bit puzzled - in the patch 1/3 you added the
U-Boot wide I2C_M_RD_NEED_STOP_BIT flag to indicate the need to generate
a stop bit.

Iin this patch you change the respective mxc_i2c.c
driver (which is NXP SoC specific). Shouldn't those changes go in to a
"generic" i2c code (or better to the pcf2127 driver) ?

What would happen if somebody use this device (pcf2127.c) on board
running e.g. TI processor? (like Beagle Bone Black). Would the pcf2127
work properly in that case?

> ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 62
> insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> index 23119cce65..f3811eb0c5 100644
> --- a/drivers/i2c/mxc_i2c.c
> +++ b/drivers/i2c/mxc_i2c.c
> @@ -961,6 +961,8 @@ static int mxc_i2c_xfer(struct udevice *bus,
> struct i2c_msg *msg, int nmsgs) int reg_shift = i2c_bus->driver_data
> & I2C_QUIRK_FLAG ? VF610_I2C_REGSHIFT : IMX_I2C_REGSHIFT;
>  	int read_mode;
> +	bool quirk = i2c_bus->driver_data & I2C_QUIRK_FLAG ? true :
> false;
> +	unsigned int temp;
>  
>  	/* 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 @@ -975,6 +977,7 @@ static int mxc_i2c_xfer(struct udevice
> *bus, struct i2c_msg *msg, int nmsgs) read_mode = -1; /* So it's
> always different on the first message */ for (; nmsgs > 0; nmsgs--,
> msg++) { const int msg_is_read = !!(msg->flags & I2C_M_RD);
> +		bool next_is_read = nmsgs > 1 && (msg[1].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');
> @@ -982,13 +985,16 @@ static int mxc_i2c_xfer(struct udevice *bus,
> struct i2c_msg *msg, int nmsgs) 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));
> +				if (!(msg[1].flags &
> I2C_M_RD_NEED_STOP_BIT)) {
> +					debug("i2c_xfer:
> [RSTART]\n");
> +					ret = readb(base + (I2CR <<
> reg_shift));
> +					ret |= I2CR_RSTA;
> +					writeb(ret, base + (I2CR <<
> reg_shift));
> +				}
>  			}
>  			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");
> @@ -998,16 +1004,64 @@ static int mxc_i2c_xfer(struct udevice *bus,
> struct i2c_msg *msg, int nmsgs) read_mode = msg_is_read;
>  		}
>  
> -		if (msg->flags & I2C_M_RD)
> +		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;
> +			continue;
> +		}
>  
> +		 /* Write message */
> +		ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
> +				     msg->len);
>  		if (ret < 0)
>  			break;
> +
> +		if (!next_is_read)
> +			continue;
> +
> +		/* Read message following write message */
> +		if (msg[1].flags & I2C_M_RD_NEED_STOP_BIT) {
> +			/* Generate a stop bit */
> +			i2c_imx_stop(i2c_bus);
> +			/* Reset i2c slave */
> +			i2c_force_reset_slave();
> +
> +			/* Enable I2C controller */
> +			if (quirk)
> +				ret = readb(base + (I2CR <<
> reg_shift))
> +					& I2CR_IDIS;
> +			else
> +				ret = !(readb(base + (I2CR <<
> reg_shift))
> +						& I2CR_IEN);
> +			if (ret) {
> +				writeb(I2CR_IEN, base + (I2CR <<
> reg_shift));
> +				/* Wait for controller to be stable
> */
> +				udelay(50);
> +			}
> +
> +			/* Clear interrupt bit */
> +			writeb(I2SR_IIF_CLEAR, base + (I2SR <<
> reg_shift));
> +			ret = wait_for_sr_state(i2c_bus,
> ST_BUS_IDLE);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* Start I2C transaction */
> +			temp = readb(base + (I2CR << reg_shift));
> +			temp |= I2CR_MSTA;
> +			writeb(temp, base + (I2CR << reg_shift));
> +
> +			ret = wait_for_sr_state(i2c_bus,
> ST_BUS_BUSY);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* Enter transfer mode */
> +			temp |= I2CR_MTX | I2CR_TX_NO_AK;
> +			writeb(temp, base + (I2CR << reg_shift));
> +			udelay(50);
> +		}
>  	}
>  
>  	if (ret)




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190524/3653fdf0/attachment.sig>

[U-Boot] [PATCH v2 3/3] rtc: pcf2127: Fixed bug with rtc settings and getting error time

[Chuanhua Han]

The previous pcf2127 RTC chip could not read and set the correct time.
When reading the data of internal registers, the read address was the
value of register plus 1. This is because this chip requires the host
to send a stop signal after setting the register address and before
reading the register data.

This patch sets the flag that the stop signal is needed and fixes the
bug of the original read and write time.

Signed-off-by: Biwen Li <biwen.li@nxp.com>
Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
---
Changes in v2: 
	- Split the original patch into 3 patches
	- Add detailed description information for each patch

 drivers/rtc/pcf2127.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c
index dcf0340b4d..010c45ecbf 100644
--- a/drivers/rtc/pcf2127.c
+++ b/drivers/rtc/pcf2127.c
@@ -24,12 +24,9 @@
 
 static int pcf2127_rtc_set(struct udevice *dev, const struct rtc_time *tm)
 {
-	uchar buf[8];
+	uchar buf[7] = {0};
 	int i = 0, ret;
 
-	/* start register address */
-	buf[i++] = PCF2127_REG_SC;
-
 	/* hours, minutes and seconds */
 	buf[i++] = bin2bcd(tm->tm_sec);
 	buf[i++] = bin2bcd(tm->tm_min);
@@ -44,7 +41,7 @@ static int pcf2127_rtc_set(struct udevice *dev, const struct rtc_time *tm)
 	buf[i++] = bin2bcd(tm->tm_year % 100);
 
 	/* write register's data */
-	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
+	ret = dm_i2c_write(dev, PCF2127_REG_SC, buf, i);
 
 	return ret;
 }
@@ -54,9 +51,6 @@ static int pcf2127_rtc_get(struct udevice *dev, struct rtc_time *tm)
 	int ret = 0;
 	uchar buf[10] = { PCF2127_REG_CTRL1 };
 
-	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
-	if (ret < 0)
-		return ret;
 	ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
 	if (ret < 0)
 		return ret;
@@ -90,6 +84,13 @@ static int pcf2127_rtc_reset(struct udevice *dev)
 	return 0;
 }
 
+static int pcf2127_probe(struct udevice *dev)
+{
+	i2c_set_chip_flags(dev, DM_I2C_CHIP_RD_NEED_STOP_BIT);
+
+	return 0;
+}
+
 static const struct rtc_ops pcf2127_rtc_ops = {
 	.get = pcf2127_rtc_get,
 	.set = pcf2127_rtc_set,
@@ -104,6 +105,7 @@ static const struct udevice_id pcf2127_rtc_ids[] = {
 U_BOOT_DRIVER(rtc_pcf2127) = {
 	.name	= "rtc-pcf2127",
 	.id	= UCLASS_RTC,
+	.probe	= pcf2127_probe,
 	.of_match = pcf2127_rtc_ids,
 	.ops	= &pcf2127_rtc_ops,
 };
-- 
2.17.1

[U-Boot] [PATCH v2 3/3] rtc: pcf2127: Fixed bug with rtc settings and getting error time

[Simon Glass]

Hi,

On Fri, 24 May 2019 at 03:23, Chuanhua Han <chuanhua.han@nxp.com> wrote:
>
> The previous pcf2127 RTC chip could not read and set the correct time.
> When reading the data of internal registers, the read address was the
> value of register plus 1. This is because this chip requires the host
> to send a stop signal after setting the register address and before
> reading the register data.
>
> This patch sets the flag that the stop signal is needed and fixes the
> bug of the original read and write time.
>
> Signed-off-by: Biwen Li <biwen.li@nxp.com>
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
> Changes in v2:
>         - Split the original patch into 3 patches
>         - Add detailed description information for each patch
>
>  drivers/rtc/pcf2127.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)

Can you modify this to use i2c_xfer() so that separate transactions are done?

Regards,
SImon

[U-Boot] [EXT] Re: [PATCH v2 3/3] rtc: pcf2127: Fixed bug with rtc settings and getting error time

[Chuanhua Han]

Hi,

> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: 2019年6月23日 3:10
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: Heiko Schocher <hs@denx.de>; U-Boot Mailing List <u-boot@lists.denx.de>;
> Biwen Li <biwen.li@nxp.com>; Lukasz Majewski <lukma@denx.de>
> Subject: [EXT] Re: [PATCH v2 3/3] rtc: pcf2127: Fixed bug with rtc settings and
> getting error time
> 
> Caution: EXT Email
> 
> Hi,
> 
> On Fri, 24 May 2019 at 03:23, Chuanhua Han <chuanhua.han@nxp.com>
> wrote:
> >
> > The previous pcf2127 RTC chip could not read and set the correct time.
> > When reading the data of internal registers, the read address was the
> > value of register plus 1. This is because this chip requires the host
> > to send a stop signal after setting the register address and before
> > reading the register data.
> >
> > This patch sets the flag that the stop signal is needed and fixes the
> > bug of the original read and write time.
> >
> > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > ---
> > Changes in v2:
> >         - Split the original patch into 3 patches
> >         - Add detailed description information for each patch
> >
> >  drivers/rtc/pcf2127.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> Can you modify this to use i2c_xfer() so that separate transactions are done?
> 
I have sent the fourth edition you see the new patchwork:
http://patchwork.ozlabs.org/project/uboot/list/?series=114242

> Regards,
> SImon

[U-Boot] [PATCH v2 1/3] dm: i2c: Add a flag that needs to generate a stop bit

[Simon Glass]

Hi,

On Fri, 24 May 2019 at 03:23, Chuanhua Han <chuanhua.han@nxp.com> wrote:
>
> Usually the i2c bus needs to write the address of the register before
> reading the internal register data of the device (ignoring the
> transmission of the slave address).
>
> Generally, the stop signal is not needed before the register is read,
> but there is a special chip that needs this stop signal (such as
> pcf2127), this patch adds a flag that needs to generate a stop
> bit to determine whether the i2c host needs to generate a stop signal
> before reading the register data.

Does this mean that the chip does not comply with the i2c protocol?

Is it possible instead to so a write and then a read?

See dm_i2c_xfer().


>
> Signed-off-by: Biwen Li <biwen.li@nxp.com>
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
> Changes in v2:
>         - Split the original patch into 3 patches
>         - Add detailed description information for each patch
>
>  drivers/i2c/i2c-uclass.c | 2 ++
>  include/i2c.h            | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> index e47abf1833..18f7364d72 100644
> --- a/drivers/i2c/i2c-uclass.c
> +++ b/drivers/i2c/i2c-uclass.c
> @@ -141,6 +141,8 @@ int dm_i2c_read(struct udevice *dev, uint offset, uint8_t *buffer, int len)
>         if (len) {
>                 ptr->addr = chip->chip_addr;
>                 ptr->flags = chip->flags & DM_I2C_CHIP_10BIT ? I2C_M_TEN : 0;
> +               ptr->flags |= chip->flags & DM_I2C_CHIP_RD_NEED_STOP_BIT ?
> +                             I2C_M_RD_NEED_STOP_BIT : 0;
>                 ptr->flags |= I2C_M_RD;
>                 ptr->len = len;
>                 ptr->buf = buffer;
> diff --git a/include/i2c.h b/include/i2c.h
> index a5c760c711..beaa028349 100644
> --- a/include/i2c.h
> +++ b/include/i2c.h
> @@ -28,6 +28,7 @@ enum dm_i2c_chip_flags {
>         DM_I2C_CHIP_10BIT       = 1 << 0, /* Use 10-bit addressing */
>         DM_I2C_CHIP_RD_ADDRESS  = 1 << 1, /* Send address for each read byte */
>         DM_I2C_CHIP_WR_ADDRESS  = 1 << 2, /* Send address for each write byte */
> +       DM_I2C_CHIP_RD_NEED_STOP_BIT    = 1 << 3, /* Need generate stop bit */
>  };
>
>  struct udevice;
> @@ -87,6 +88,7 @@ enum dm_i2c_msg_flags {
>         I2C_M_IGNORE_NAK        = 0x1000, /* continue after NAK */
>         I2C_M_NO_RD_ACK         = 0x0800, /* skip the Ack bit on reads */
>         I2C_M_RECV_LEN          = 0x0400, /* length is first received byte */
> +       I2C_M_RD_NEED_STOP_BIT  = 0x0002, /* need generate stop bit */
>  };
>
>  /**
> --
> 2.17.1
>

[U-Boot] [EXT] Re: [PATCH v2 1/3] dm: i2c: Add a flag that needs to generate a stop bit

[Chuanhua Han]

> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: 2019年6月23日 3:10
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: Heiko Schocher <hs@denx.de>; U-Boot Mailing List <u-boot@lists.denx.de>;
> Biwen Li <biwen.li@nxp.com>; Lukasz Majewski <lukma@denx.de>
> Subject: [EXT] Re: [PATCH v2 1/3] dm: i2c: Add a flag that needs to generate a
> stop bit
> 
> Caution: EXT Email
> 
> Hi,
> 
> On Fri, 24 May 2019 at 03:23, Chuanhua Han <chuanhua.han@nxp.com> wrote:
> >
> > Usually the i2c bus needs to write the address of the register before
> > reading the internal register data of the device (ignoring the
> > transmission of the slave address).
> >
> > Generally, the stop signal is not needed before the register is read,
> > but there is a special chip that needs this stop signal (such as
> > pcf2127), this patch adds a flag that needs to generate a stop bit to
> > determine whether the i2c host needs to generate a stop signal before
> > reading the register data.
> 
> Does this mean that the chip does not comply with the i2c protocol?
> 
> Is it possible instead to so a write and then a read?
> 
> See dm_i2c_xfer().
Please see the fourth version of the patch, use the dm_i2c_xfer () function to do and I use a flag to limit the dm_i2c_read function
It is the same to transmit only one msg. Since the core layer already provides the dm_i2c_read function, it is appropriate to use this function.
Only condition flag is required to limit
> 
> 
> >
> > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > ---
> > Changes in v2:
> >         - Split the original patch into 3 patches
> >         - Add detailed description information for each patch
> >
> >  drivers/i2c/i2c-uclass.c | 2 ++
> >  include/i2c.h            | 2 ++
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c index
> > e47abf1833..18f7364d72 100644
> > --- a/drivers/i2c/i2c-uclass.c
> > +++ b/drivers/i2c/i2c-uclass.c
> > @@ -141,6 +141,8 @@ int dm_i2c_read(struct udevice *dev, uint offset,
> uint8_t *buffer, int len)
> >         if (len) {
> >                 ptr->addr = chip->chip_addr;
> >                 ptr->flags = chip->flags & DM_I2C_CHIP_10BIT ?
> > I2C_M_TEN : 0;
> > +               ptr->flags |= chip->flags &
> DM_I2C_CHIP_RD_NEED_STOP_BIT ?
> > +                             I2C_M_RD_NEED_STOP_BIT : 0;
> >                 ptr->flags |= I2C_M_RD;
> >                 ptr->len = len;
> >                 ptr->buf = buffer;
> > diff --git a/include/i2c.h b/include/i2c.h index
> > a5c760c711..beaa028349 100644
> > --- a/include/i2c.h
> > +++ b/include/i2c.h
> > @@ -28,6 +28,7 @@ enum dm_i2c_chip_flags {
> >         DM_I2C_CHIP_10BIT       = 1 << 0, /* Use 10-bit addressing */
> >         DM_I2C_CHIP_RD_ADDRESS  = 1 << 1, /* Send address for each
> read byte */
> >         DM_I2C_CHIP_WR_ADDRESS  = 1 << 2, /* Send address for each
> > write byte */
> > +       DM_I2C_CHIP_RD_NEED_STOP_BIT    = 1 << 3, /* Need generate
> stop bit */
> >  };
> >
> >  struct udevice;
> > @@ -87,6 +88,7 @@ enum dm_i2c_msg_flags {
> >         I2C_M_IGNORE_NAK        = 0x1000, /* continue after NAK */
> >         I2C_M_NO_RD_ACK         = 0x0800, /* skip the Ack bit on
> reads */
> >         I2C_M_RECV_LEN          = 0x0400, /* length is first received
> byte */
> > +       I2C_M_RD_NEED_STOP_BIT  = 0x0002, /* need generate stop bit
> */
> >  };
> >
> >  /**
> > --
> > 2.17.1
> >

[U-Boot] [EXT] Re: [PATCH v2 1/3] dm: i2c: Add a flag that needs to generate a stop bit

[Simon Glass]

Hi Chuanhua,

On Sun, 23 Jun 2019 at 22:46, Chuanhua Han <chuanhua.han@nxp.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Simon Glass <sjg@chromium.org>
> > Sent: 2019年6月23日 3:10
> > To: Chuanhua Han <chuanhua.han@nxp.com>
> > Cc: Heiko Schocher <hs@denx.de>; U-Boot Mailing List <u-boot@lists.denx.de>;
> > Biwen Li <biwen.li@nxp.com>; Lukasz Majewski <lukma@denx.de>
> > Subject: [EXT] Re: [PATCH v2 1/3] dm: i2c: Add a flag that needs to generate a
> > stop bit
> >
> > Caution: EXT Email
> >
> > Hi,
> >
> > On Fri, 24 May 2019 at 03:23, Chuanhua Han <chuanhua.han@nxp.com> wrote:
> > >
> > > Usually the i2c bus needs to write the address of the register before
> > > reading the internal register data of the device (ignoring the
> > > transmission of the slave address).
> > >
> > > Generally, the stop signal is not needed before the register is read,
> > > but there is a special chip that needs this stop signal (such as
> > > pcf2127), this patch adds a flag that needs to generate a stop bit to
> > > determine whether the i2c host needs to generate a stop signal before
> > > reading the register data.
> >
> > Does this mean that the chip does not comply with the i2c protocol?
> >
> > Is it possible instead to so a write and then a read?
> >
> > See dm_i2c_xfer().
> Please see the fourth version of the patch, use the dm_i2c_xfer () function to do and I use a flag to limit the dm_i2c_read function
> It is the same to transmit only one msg. Since the core layer already provides the dm_i2c_read function, it is appropriate to use this function.
> Only condition flag is required to limit

I cannot find that patch. On another thread you pointed me to a v4
patch but when I looked it was not v4.

Can you please link to the v4 patch in patchwork?

I found a v2 that I will reply to in any case.

Regards,
Simon