[U-Boot] [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[U-Boot] [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[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).
However, in the current i2c general code, the dm_i2c_read api encapsulates
two messages, the first time is to set the register address message,
the second time is a message to read the register data, so that no stop
signal is generated.

This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET flag for specific
i2c chips, so if the i2c slave requires a stop signal, chips driver can
set this flag, then call the dm_i2c_write to set the register address
(a stop signal is generated after this API call), then call dm_i2c_read
to read the register data.

Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
---
Changes in v3:
	- Use the new flag DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET	

Changes in v2: 
        - Split the original patch into 3 patches
        - Add detailed description information for each patch

 drivers/i2c/i2c-uclass.c | 6 ++++--
 include/i2c.h            | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
index e47abf1833..9804b5e8c7 100644
--- a/drivers/i2c/i2c-uclass.c
+++ b/drivers/i2c/i2c-uclass.c
@@ -135,8 +135,10 @@ int dm_i2c_read(struct udevice *dev, uint offset, uint8_t *buffer, int len)
 	if (chip->flags & DM_I2C_CHIP_RD_ADDRESS)
 		return i2c_read_bytewise(dev, offset, buffer, len);
 	ptr = msg;
-	if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
-		ptr++;
+	if (!(chip->flags & DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET)) {
+		if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
+			ptr++;
+	}
 
 	if (len) {
 		ptr->addr = chip->chip_addr;
diff --git a/include/i2c.h b/include/i2c.h
index a5c760c711..3123cbf280 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_NO_I2C_SETUP_OFFSET  = 1 << 3, /* No i2c_setup_offset*/
 };
 
 struct udevice;
-- 
2.17.1

[U-Boot] [PATCH 2/2] 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 DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET flag in order to
generate a stop signal 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 v3:
	- When the rtc time is obtained, the address of the set register
is separated from the read data.

Changes in v2: 
        - Split the original patch into 3 patches
        - Add detailed description information for each patch

 drivers/rtc/pcf2127.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c
index dcf0340b4d..65794b98e6 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,12 @@ 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);
+	/* Set the address of the start register to be read */
+	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, NULL, 0);
 	if (ret < 0)
 		return ret;
+
+	/* Read register's data */
 	ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
 	if (ret < 0)
 		return ret;
@@ -90,6 +90,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_NO_I2C_SETUP_OFFSET);
+
+	return 0;
+}
+
 static const struct rtc_ops pcf2127_rtc_ops = {
 	.get = pcf2127_rtc_get,
 	.set = pcf2127_rtc_set,
@@ -104,6 +111,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 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[Lukasz Majewski]

On Thu, 30 May 2019 18:31:48 +0800
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). However, in the current i2c general code, the dm_i2c_read
> api encapsulates two messages, the first time is to set the register
> address message, the second time is a message to read the register
> data, so that no stop signal is generated.
> 
> This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET flag for
> specific i2c chips, so if the i2c slave requires a stop signal, chips
> driver can set this flag, then call the dm_i2c_write to set the
> register address (a stop signal is generated after this API call),
> then call dm_i2c_read to read the register data.
> 
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
> Changes in v3:
> 	- Use the new flag DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET	
> 
> Changes in v2: 
>         - Split the original patch into 3 patches
>         - Add detailed description information for each patch
> 
>  drivers/i2c/i2c-uclass.c | 6 ++++--
>  include/i2c.h            | 1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> index e47abf1833..9804b5e8c7 100644
> --- a/drivers/i2c/i2c-uclass.c
> +++ b/drivers/i2c/i2c-uclass.c
> @@ -135,8 +135,10 @@ int dm_i2c_read(struct udevice *dev, uint
> offset, uint8_t *buffer, int len) if (chip->flags &
> DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev, offset, buffer,
> len); ptr = msg;
> -	if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
> -		ptr++;
> +	if (!(chip->flags & DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET)) {
> +		if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
> +			ptr++;
> +	}
>  
>  	if (len) {
>  		ptr->addr = chip->chip_addr;
> diff --git a/include/i2c.h b/include/i2c.h
> index a5c760c711..3123cbf280 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 */

Aren't those two above flags describe exactly what you need? They send
address for each read/written byte. (or do you need to send more than a
single byte)?

>	DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET  = 1 << 3,
>       /* No i2c_setup_offset*/ };
>  
>  struct udevice;




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/20190604/7cc4da76/attachment.sig>

[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[Chuanhua Han]

> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年6月4日 6:08
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> Subject: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that not call
> i2c_setup_offset
> 
> On Thu, 30 May 2019 18:31:48 +0800
> 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). However, in the current i2c general code, the dm_i2c_read
> > api encapsulates two messages, the first time is to set the register
> > address message, the second time is a message to read the register
> > data, so that no stop signal is generated.
> >
> > This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET flag for
> > specific i2c chips, so if the i2c slave requires a stop signal, chips
> > driver can set this flag, then call the dm_i2c_write to set the
> > register address (a stop signal is generated after this API call),
> > then call dm_i2c_read to read the register data.
> >
> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > ---
> > Changes in v3:
> > 	- Use the new flag DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> >
> > Changes in v2:
> >         - Split the original patch into 3 patches
> >         - Add detailed description information for each patch
> >
> >  drivers/i2c/i2c-uclass.c | 6 ++++--
> >  include/i2c.h            | 1 +
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c index
> > e47abf1833..9804b5e8c7 100644
> > --- a/drivers/i2c/i2c-uclass.c
> > +++ b/drivers/i2c/i2c-uclass.c
> > @@ -135,8 +135,10 @@ int dm_i2c_read(struct udevice *dev, uint offset,
> > uint8_t *buffer, int len) if (chip->flags &
> > DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev, offset, buffer,
> > len); ptr = msg;
> > -	if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
> > -		ptr++;
> > +	if (!(chip->flags & DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET)) {
> > +		if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
> > +			ptr++;
> > +	}
> >
> >  	if (len) {
> >  		ptr->addr = chip->chip_addr;
> > diff --git a/include/i2c.h b/include/i2c.h index
> > a5c760c711..3123cbf280 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 */
> 
> Aren't those two above flags describe exactly what you need? They send
> address for each read/written byte. (or do you need to send more than a single
> byte)?
This is not what I need. I need to generate a stop signal after setting the register address, so I need the following flag.
> 
> >	DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET  = 1 << 3,
> >       /* No i2c_setup_offset*/ };
> >
> >  struct udevice;
> 
> 
> 
> 
> 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

[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[Lukasz Majewski]

On Tue, 4 Jun 2019 02:20:49 +0000
Chuanhua Han <chuanhua.han@nxp.com> wrote:

> > -----Original Message-----
> > From: Lukasz Majewski <lukma@denx.de>
> > Sent: 2019年6月4日 6:08
> > To: Chuanhua Han <chuanhua.han@nxp.com>
> > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> > Subject: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that
> > not call i2c_setup_offset
> > 
> > On Thu, 30 May 2019 18:31:48 +0800
> > 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). However, in the current i2c general code, the
> > > dm_i2c_read api encapsulates two messages, the first time is to
> > > set the register address message, the second time is a message to
> > > read the register data, so that no stop signal is generated.
> > >
> > > This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET flag for
> > > specific i2c chips, so if the i2c slave requires a stop signal,
> > > chips driver can set this flag, then call the dm_i2c_write to set
> > > the register address (a stop signal is generated after this API
> > > call), then call dm_i2c_read to read the register data.
> > >
> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > ---
> > > Changes in v3:
> > > 	- Use the new flag DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > >
> > > Changes in v2:
> > >         - Split the original patch into 3 patches
> > >         - Add detailed description information for each patch
> > >
> > >  drivers/i2c/i2c-uclass.c | 6 ++++--
> > >  include/i2c.h            | 1 +
> > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> > > index e47abf1833..9804b5e8c7 100644
> > > --- a/drivers/i2c/i2c-uclass.c
> > > +++ b/drivers/i2c/i2c-uclass.c
> > > @@ -135,8 +135,10 @@ int dm_i2c_read(struct udevice *dev, uint
> > > offset, uint8_t *buffer, int len) if (chip->flags &
> > > DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev, offset,
> > > buffer, len); ptr = msg;
> > > -	if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
> > > -		ptr++;
> > > +	if (!(chip->flags & DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET))
> > > {
> > > +		if (!i2c_setup_offset(chip, offset, offset_buf,
> > > ptr))
> > > +			ptr++;
> > > +	}
> > >
> > >  	if (len) {
> > >  		ptr->addr = chip->chip_addr;
> > > diff --git a/include/i2c.h b/include/i2c.h index
> > > a5c760c711..3123cbf280 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 */  
> > 
> > Aren't those two above flags describe exactly what you need? They
> > send address for each read/written byte. (or do you need to send
> > more than a single byte)?  
> This is not what I need. I need to generate a stop signal after
> setting the register address, 

Please correct me if I'm wrong, but the above flag says that you need
to send the I2C stop after you send one byte?

Would it be possible to send address with I2C stop and then read data
(single byte) with I2C stop?

> so I need the following flag.
> >   
> > >	DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET  = 1 << 3,

I do find this name as a bit misleading - is the RD_NO_I2C_SETUP_OFFSET
name reflecting the purpose (to send I2C stop after sending address)?

Maybe there would be a better name?

> > >       /* No i2c_setup_offset*/ };
> > >
> > >  struct udevice;  
> > 
> > 
> > 
> > 
> > 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  




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/20190604/39e68170/attachment.sig>

[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[Chuanhua Han]

> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年6月4日 14:46
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that not call
> i2c_setup_offset
> 
> On Tue, 4 Jun 2019 02:20:49 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukasz Majewski <lukma@denx.de>
> > > Sent: 2019年6月4日 6:08
> > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> > > Subject: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that not
> > > call i2c_setup_offset
> > >
> > > On Thu, 30 May 2019 18:31:48 +0800
> > > 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). However, in the current i2c general code, the
> > > > dm_i2c_read api encapsulates two messages, the first time is to
> > > > set the register address message, the second time is a message to
> > > > read the register data, so that no stop signal is generated.
> > > >
> > > > This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET flag for
> > > > specific i2c chips, so if the i2c slave requires a stop signal,
> > > > chips driver can set this flag, then call the dm_i2c_write to set
> > > > the register address (a stop signal is generated after this API
> > > > call), then call dm_i2c_read to read the register data.
> > > >
> > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > ---
> > > > Changes in v3:
> > > > 	- Use the new flag DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > >
> > > > Changes in v2:
> > > >         - Split the original patch into 3 patches
> > > >         - Add detailed description information for each patch
> > > >
> > > >  drivers/i2c/i2c-uclass.c | 6 ++++--
> > > >  include/i2c.h            | 1 +
> > > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> > > > index e47abf1833..9804b5e8c7 100644
> > > > --- a/drivers/i2c/i2c-uclass.c
> > > > +++ b/drivers/i2c/i2c-uclass.c
> > > > @@ -135,8 +135,10 @@ int dm_i2c_read(struct udevice *dev, uint
> > > > offset, uint8_t *buffer, int len) if (chip->flags &
> > > > DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev, offset,
> > > > buffer, len); ptr = msg;
> > > > -	if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
> > > > -		ptr++;
> > > > +	if (!(chip->flags & DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET))
> > > > {
> > > > +		if (!i2c_setup_offset(chip, offset, offset_buf,
> > > > ptr))
> > > > +			ptr++;
> > > > +	}
> > > >
> > > >  	if (len) {
> > > >  		ptr->addr = chip->chip_addr;
> > > > diff --git a/include/i2c.h b/include/i2c.h index
> > > > a5c760c711..3123cbf280 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 */
> > >
> > > Aren't those two above flags describe exactly what you need? They
> > > send address for each read/written byte. (or do you need to send
> > > more than a single byte)?
> > This is not what I need. I need to generate a stop signal after
> > setting the register address,
> 
> Please correct me if I'm wrong, but the above flag says that you need to send
> the I2C stop after you send one byte?
> 
> Would it be possible to send address with I2C stop and then read data (single
> byte) with I2C stop?
> 
> > so I need the following flag.
> > >
> > > >	DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET  = 1 << 3,
> 
> I do find this name as a bit misleading - is the RD_NO_I2C_SETUP_OFFSET
> name reflecting the purpose (to send I2C stop after sending address)?
The purpose is to send a stop signal after setting the slave register address.
> 
> Maybe there would be a better name?
> 
> > > >       /* No i2c_setup_offset*/ };
> > > >
> > > >  struct udevice;
> > >
> > >
> > >
> > >
> > > 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
> 
> 
> 
> 
> 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

[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[Biwen Li]

Hi lukasz,chuanhua,

	1. This means that pcf2127(rtc) do not need send slave register address when calling dm_i2c_read,because pcf2127(rtc) has send slave register address when calling dm_i2c_write in drivers/rtc/pcf2127.c
	2. pcf2127's timings of reading registers,you can look at the attached file.
		2.1 i2c write register address
		2.2 i2c read registers(do not need send register address again,because it has send register address in 2.1) 

-----Original Message-----
From: Chuanhua Han 
Sent: 2019年6月4日 14:49
To: Lukasz Majewski <lukma@denx.de>
Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
Subject: RE: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset



> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年6月4日 14:46
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that 
> not call i2c_setup_offset
> 
> On Tue, 4 Jun 2019 02:20:49 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukasz Majewski <lukma@denx.de>
> > > Sent: 2019年6月4日 6:08
> > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> > > Subject: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that 
> > > not call i2c_setup_offset
> > >
> > > On Thu, 30 May 2019 18:31:48 +0800 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). However, in the current i2c general code, the 
> > > > dm_i2c_read api encapsulates two messages, the first time is to 
> > > > set the register address message, the second time is a message 
> > > > to read the register data, so that no stop signal is generated.
> > > >
> > > > This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET flag for 
> > > > specific i2c chips, so if the i2c slave requires a stop signal, 
> > > > chips driver can set this flag, then call the dm_i2c_write to 
> > > > set the register address (a stop signal is generated after this 
> > > > API call), then call dm_i2c_read to read the register data.
> > > >
> > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > ---
> > > > Changes in v3:
> > > > 	- Use the new flag DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > >
> > > > Changes in v2:
> > > >         - Split the original patch into 3 patches
> > > >         - Add detailed description information for each patch
> > > >
> > > >  drivers/i2c/i2c-uclass.c | 6 ++++--
> > > >  include/i2c.h            | 1 +
> > > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c 
> > > > index e47abf1833..9804b5e8c7 100644
> > > > --- a/drivers/i2c/i2c-uclass.c
> > > > +++ b/drivers/i2c/i2c-uclass.c
> > > > @@ -135,8 +135,10 @@ int dm_i2c_read(struct udevice *dev, uint 
> > > > offset, uint8_t *buffer, int len) if (chip->flags &
> > > > DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev, offset, 
> > > > buffer, len); ptr = msg;
> > > > -	if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
> > > > -		ptr++;
> > > > +	if (!(chip->flags & DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET))
> > > > {
> > > > +		if (!i2c_setup_offset(chip, offset, offset_buf,
> > > > ptr))
> > > > +			ptr++;
> > > > +	}
> > > >
> > > >  	if (len) {
> > > >  		ptr->addr = chip->chip_addr;
> > > > diff --git a/include/i2c.h b/include/i2c.h index
> > > > a5c760c711..3123cbf280 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 */
> > >
> > > Aren't those two above flags describe exactly what you need? They 
> > > send address for each read/written byte. (or do you need to send 
> > > more than a single byte)?
> > This is not what I need. I need to generate a stop signal after 
> > setting the register address,
> 
> Please correct me if I'm wrong, but the above flag says that you need 
> to send the I2C stop after you send one byte?
> 
> Would it be possible to send address with I2C stop and then read data 
> (single
> byte) with I2C stop?
> 
> > so I need the following flag.
> > >
> > > >	DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET  = 1 << 3,
> 
> I do find this name as a bit misleading - is the 
> RD_NO_I2C_SETUP_OFFSET name reflecting the purpose (to send I2C stop after sending address)?
The purpose is to send a stop signal after setting the slave register address.
> 
> Maybe there would be a better name?
> 
> > > >       /* No i2c_setup_offset*/ };
> > > >
> > > >  struct udevice;
> > >
> > >
> > >
> > >
> > > 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
> 
> 
> 
> 
> 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: how_to_read_pcf2127_s_registers_with_i2c(with_intructions).jpg
Type: image/jpeg
Size: 112619 bytes
Desc: how_to_read_pcf2127_s_registers_with_i2c(with_intructions).jpg
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190604/14b76888/attachment-0001.jpg>

[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[Lukasz Majewski]

On Tue, 4 Jun 2019 06:48:59 +0000
Chuanhua Han <chuanhua.han@nxp.com> wrote:

> > -----Original Message-----
> > From: Lukasz Majewski <lukma@denx.de>
> > Sent: 2019年6月4日 14:46
> > To: Chuanhua Han <chuanhua.han@nxp.com>
> > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> > Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag
> > that not call i2c_setup_offset
> > 
> > On Tue, 4 Jun 2019 02:20:49 +0000
> > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Lukasz Majewski <lukma@denx.de>
> > > > Sent: 2019年6月4日 6:08
> > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > <biwen.li@nxp.com> Subject: [EXT] Re: [U-Boot] [PATCH 1/2] dm:
> > > > i2c: Add a flag that not call i2c_setup_offset
> > > >
> > > > On Thu, 30 May 2019 18:31:48 +0800
> > > > 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). However, in the current i2c general
> > > > > code, the dm_i2c_read api encapsulates two messages, the
> > > > > first time is to set the register address message, the second
> > > > > time is a message to read the register data, so that no stop
> > > > > signal is generated.
> > > > >
> > > > > This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET flag
> > > > > for specific i2c chips, so if the i2c slave requires a stop
> > > > > signal, chips driver can set this flag, then call the
> > > > > dm_i2c_write to set the register address (a stop signal is
> > > > > generated after this API call), then call dm_i2c_read to read
> > > > > the register data.
> > > > >
> > > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > ---
> > > > > Changes in v3:
> > > > > 	- Use the new flag DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > > >
> > > > > Changes in v2:
> > > > >         - Split the original patch into 3 patches
> > > > >         - Add detailed description information for each patch
> > > > >
> > > > >  drivers/i2c/i2c-uclass.c | 6 ++++--
> > > > >  include/i2c.h            | 1 +
> > > > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/i2c/i2c-uclass.c
> > > > > b/drivers/i2c/i2c-uclass.c index e47abf1833..9804b5e8c7 100644
> > > > > --- a/drivers/i2c/i2c-uclass.c
> > > > > +++ b/drivers/i2c/i2c-uclass.c
> > > > > @@ -135,8 +135,10 @@ int dm_i2c_read(struct udevice *dev, uint
> > > > > offset, uint8_t *buffer, int len) if (chip->flags &
> > > > > DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev, offset,
> > > > > buffer, len); ptr = msg;
> > > > > -	if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
> > > > > -		ptr++;
> > > > > +	if (!(chip->flags &
> > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET)) {
> > > > > +		if (!i2c_setup_offset(chip, offset,
> > > > > offset_buf, ptr))
> > > > > +			ptr++;
> > > > > +	}
> > > > >
> > > > >  	if (len) {
> > > > >  		ptr->addr = chip->chip_addr;
> > > > > diff --git a/include/i2c.h b/include/i2c.h index
> > > > > a5c760c711..3123cbf280 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 */  
> > > >
> > > > Aren't those two above flags describe exactly what you need?
> > > > They send address for each read/written byte. (or do you need
> > > > to send more than a single byte)?  
> > > This is not what I need. I need to generate a stop signal after
> > > setting the register address,  
> > 
> > Please correct me if I'm wrong, but the above flag says that you
> > need to send the I2C stop after you send one byte?
> > 
> > Would it be possible to send address with I2C stop and then read
> > data (single byte) with I2C stop?
> >   
> > > so I need the following flag.  
> > > >  
> > > > >	DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET  = 1 << 3,  
> > 
> > I do find this name as a bit misleading - is the
> > RD_NO_I2C_SETUP_OFFSET name reflecting the purpose (to send I2C
> > stop after sending address)?  
> The purpose is to send a stop signal after setting the slave register
> address.

What I'm trying to point out is that the above description
doesn't match the flag name. Maybe there is a better, more descriptive
name?

Also - what is byte length of send address to this device? One byte ?

> > 
> > Maybe there would be a better name?
> >   
> > > > >       /* No i2c_setup_offset*/ };
> > > > >
> > > > >  struct udevice;  
> > > >
> > > >
> > > >
> > > >
> > > > 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  
> > 
> > 
> > 
> > 
> > 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  




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/20190604/f4212b77/attachment.sig>

[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[Chuanhua Han]

> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年6月4日 15:56
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that not call
> i2c_setup_offset
> 
> On Tue, 4 Jun 2019 06:48:59 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukasz Majewski <lukma@denx.de>
> > > Sent: 2019年6月4日 14:46
> > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> > > Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that
> > > not call i2c_setup_offset
> > >
> > > On Tue, 4 Jun 2019 02:20:49 +0000
> > > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Lukasz Majewski <lukma@denx.de>
> > > > > Sent: 2019年6月4日 6:08
> > > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > > <biwen.li@nxp.com> Subject: [EXT] Re: [U-Boot] [PATCH 1/2] dm:
> > > > > i2c: Add a flag that not call i2c_setup_offset
> > > > >
> > > > > On Thu, 30 May 2019 18:31:48 +0800 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). However, in the current i2c general
> > > > > > code, the dm_i2c_read api encapsulates two messages, the first
> > > > > > time is to set the register address message, the second time
> > > > > > is a message to read the register data, so that no stop signal
> > > > > > is generated.
> > > > > >
> > > > > > This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET flag
> > > > > > for specific i2c chips, so if the i2c slave requires a stop
> > > > > > signal, chips driver can set this flag, then call the
> > > > > > dm_i2c_write to set the register address (a stop signal is
> > > > > > generated after this API call), then call dm_i2c_read to read
> > > > > > the register data.
> > > > > >
> > > > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > > ---
> > > > > > Changes in v3:
> > > > > > 	- Use the new flag DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > > > >
> > > > > > Changes in v2:
> > > > > >         - Split the original patch into 3 patches
> > > > > >         - Add detailed description information for each patch
> > > > > >
> > > > > >  drivers/i2c/i2c-uclass.c | 6 ++++--
> > > > > >  include/i2c.h            | 1 +
> > > > > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/i2c/i2c-uclass.c
> > > > > > b/drivers/i2c/i2c-uclass.c index e47abf1833..9804b5e8c7 100644
> > > > > > --- a/drivers/i2c/i2c-uclass.c
> > > > > > +++ b/drivers/i2c/i2c-uclass.c
> > > > > > @@ -135,8 +135,10 @@ int dm_i2c_read(struct udevice *dev, uint
> > > > > > offset, uint8_t *buffer, int len) if (chip->flags &
> > > > > > DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev, offset,
> > > > > > buffer, len); ptr = msg;
> > > > > > -	if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
> > > > > > -		ptr++;
> > > > > > +	if (!(chip->flags &
> > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET)) {
> > > > > > +		if (!i2c_setup_offset(chip, offset,
> > > > > > offset_buf, ptr))
> > > > > > +			ptr++;
> > > > > > +	}
> > > > > >
> > > > > >  	if (len) {
> > > > > >  		ptr->addr = chip->chip_addr; diff --git a/include/i2c.h
> > > > > > b/include/i2c.h index
> > > > > > a5c760c711..3123cbf280 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 */
> > > > >
> > > > > Aren't those two above flags describe exactly what you need?
> > > > > They send address for each read/written byte. (or do you need to
> > > > > send more than a single byte)?
> > > > This is not what I need. I need to generate a stop signal after
> > > > setting the register address,
> > >
> > > Please correct me if I'm wrong, but the above flag says that you
> > > need to send the I2C stop after you send one byte?
> > >
> > > Would it be possible to send address with I2C stop and then read
> > > data (single byte) with I2C stop?
> > >
> > > > so I need the following flag.
> > > > >
> > > > > >	DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET  = 1 << 3,
> > >
> > > I do find this name as a bit misleading - is the
> > > RD_NO_I2C_SETUP_OFFSET name reflecting the purpose (to send I2C stop
> > > after sending address)?
> > The purpose is to send a stop signal after setting the slave register
> > address.
> 
> What I'm trying to point out is that the above description doesn't match the
> flag name. Maybe there is a better, more descriptive name?
Because the stop signal is generated by not calling the i2c_setup_offset function, this flag is used.
> 
> Also - what is byte length of send address to this device? One byte ?
Yes,one byte
> 
> > >
> > > Maybe there would be a better name?
> > >
> > > > > >       /* No i2c_setup_offset*/ };
> > > > > >
> > > > > >  struct udevice;
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > 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
> > >
> > >
> > >
> > >
> > > 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
> 
> 
> 
> 
> 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

[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[Lukasz Majewski]

On Tue, 4 Jun 2019 07:59:44 +0000
Chuanhua Han <chuanhua.han@nxp.com> wrote:

> > -----Original Message-----
> > From: Lukasz Majewski <lukma@denx.de>
> > Sent: 2019年6月4日 15:56
> > To: Chuanhua Han <chuanhua.han@nxp.com>
> > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> > Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag
> > that not call i2c_setup_offset
> > 
> > On Tue, 4 Jun 2019 06:48:59 +0000
> > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Lukasz Majewski <lukma@denx.de>
> > > > Sent: 2019年6月4日 14:46
> > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > <biwen.li@nxp.com> Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2]
> > > > dm: i2c: Add a flag that not call i2c_setup_offset
> > > >
> > > > On Tue, 4 Jun 2019 02:20:49 +0000
> > > > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> > > >  
> > > > > > -----Original Message-----
> > > > > > From: Lukasz Majewski <lukma@denx.de>
> > > > > > Sent: 2019年6月4日 6:08
> > > > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > > > <biwen.li@nxp.com> Subject: [EXT] Re: [U-Boot] [PATCH 1/2]
> > > > > > dm: i2c: Add a flag that not call i2c_setup_offset
> > > > > >
> > > > > > On Thu, 30 May 2019 18:31:48 +0800 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). However, in the
> > > > > > > current i2c general code, the dm_i2c_read api
> > > > > > > encapsulates two messages, the first time is to set the
> > > > > > > register address message, the second time is a message to
> > > > > > > read the register data, so that no stop signal is
> > > > > > > generated.
> > > > > > >
> > > > > > > This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > > > > > flag for specific i2c chips, so if the i2c slave requires
> > > > > > > a stop signal, chips driver can set this flag, then call
> > > > > > > the dm_i2c_write to set the register address (a stop
> > > > > > > signal is generated after this API call), then call
> > > > > > > dm_i2c_read to read the register data.
> > > > > > >
> > > > > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > > > ---
> > > > > > > Changes in v3:
> > > > > > > 	- Use the new flag
> > > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > > > > >
> > > > > > > Changes in v2:
> > > > > > >         - Split the original patch into 3 patches
> > > > > > >         - Add detailed description information for each
> > > > > > > patch
> > > > > > >
> > > > > > >  drivers/i2c/i2c-uclass.c | 6 ++++--
> > > > > > >  include/i2c.h            | 1 +
> > > > > > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/i2c/i2c-uclass.c
> > > > > > > b/drivers/i2c/i2c-uclass.c index e47abf1833..9804b5e8c7
> > > > > > > 100644 --- a/drivers/i2c/i2c-uclass.c
> > > > > > > +++ b/drivers/i2c/i2c-uclass.c
> > > > > > > @@ -135,8 +135,10 @@ int dm_i2c_read(struct udevice *dev,
> > > > > > > uint offset, uint8_t *buffer, int len) if (chip->flags &
> > > > > > > DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev,
> > > > > > > offset, buffer, len); ptr = msg;
> > > > > > > -	if (!i2c_setup_offset(chip, offset, offset_buf,
> > > > > > > ptr))
> > > > > > > -		ptr++;
> > > > > > > +	if (!(chip->flags &
> > > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET)) {
> > > > > > > +		if (!i2c_setup_offset(chip, offset,
> > > > > > > offset_buf, ptr))
> > > > > > > +			ptr++;
> > > > > > > +	}
> > > > > > >
> > > > > > >  	if (len) {
> > > > > > >  		ptr->addr = chip->chip_addr; diff --git
> > > > > > > a/include/i2c.h b/include/i2c.h index
> > > > > > > a5c760c711..3123cbf280 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 */  
> > > > > >
> > > > > > Aren't those two above flags describe exactly what you need?
> > > > > > They send address for each read/written byte. (or do you
> > > > > > need to send more than a single byte)?  
> > > > > This is not what I need. I need to generate a stop signal
> > > > > after setting the register address,  
> > > >
> > > > Please correct me if I'm wrong, but the above flag says that you
> > > > need to send the I2C stop after you send one byte?
> > > >
> > > > Would it be possible to send address with I2C stop and then read
> > > > data (single byte) with I2C stop?
> > > >  
> > > > > so I need the following flag.  
> > > > > >  
> > > > > > >	DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET  = 1 << 3,  
> > > >
> > > > I do find this name as a bit misleading - is the
> > > > RD_NO_I2C_SETUP_OFFSET name reflecting the purpose (to send I2C
> > > > stop after sending address)?  
> > > The purpose is to send a stop signal after setting the slave
> > > register address.  
> > 
> > What I'm trying to point out is that the above description doesn't
> > match the flag name. Maybe there is a better, more descriptive
> > name?  
> Because the stop signal is generated by not calling the
> i2c_setup_offset function, this flag is used.

But the name of the flag shall be not based on particular function
name. It shall be a generic description - like presented above:

DM_I2C_CHIP_WR_ADDRESS = 1 << 2,
/* Send address for each write byte */

With such generic names - there is no problem if we refactor the code
and change the function name.

I've looked closer into those two flags:
 git grep -n "DM_I2C_CHIP_WR_ADDRESS"

drivers/rtc/ds1307.c:319: 
drivers/rtc/isl1208.c:173:
drivers/rtc/rv3029.c:469:

It looks like those above rtc devices use it - so maybe the issue
described for pcf2127 is also present on those devices (so those flags
have been introduced) ?

> > 
> > Also - what is byte length of send address to this device? One
> > byte ?  
> Yes,one byte
> >   
> > > >
> > > > Maybe there would be a better name?
> > > >  
> > > > > > >       /* No i2c_setup_offset*/ };
> > > > > > >
> > > > > > >  struct udevice;  
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > 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  
> > > >
> > > >
> > > >
> > > >
> > > > 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  
> > 
> > 
> > 
> > 
> > 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  




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/20190604/0651467d/attachment.sig>

[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[Chuanhua Han]

> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年6月4日 16:19
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that not call
> i2c_setup_offset
> 
> On Tue, 4 Jun 2019 07:59:44 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukasz Majewski <lukma@denx.de>
> > > Sent: 2019年6月4日 15:56
> > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> > > Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that
> > > not call i2c_setup_offset
> > >
> > > On Tue, 4 Jun 2019 06:48:59 +0000
> > > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Lukasz Majewski <lukma@denx.de>
> > > > > Sent: 2019年6月4日 14:46
> > > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > > <biwen.li@nxp.com> Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2]
> > > > > dm: i2c: Add a flag that not call i2c_setup_offset
> > > > >
> > > > > On Tue, 4 Jun 2019 02:20:49 +0000 Chuanhua Han
> > > > > <chuanhua.han@nxp.com> wrote:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Lukasz Majewski <lukma@denx.de>
> > > > > > > Sent: 2019年6月4日 6:08
> > > > > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > > > > <biwen.li@nxp.com> Subject: [EXT] Re: [U-Boot] [PATCH 1/2]
> > > > > > > dm: i2c: Add a flag that not call i2c_setup_offset
> > > > > > >
> > > > > > > On Thu, 30 May 2019 18:31:48 +0800 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). However, in the
> > > > > > > > current i2c general code, the dm_i2c_read api encapsulates
> > > > > > > > two messages, the first time is to set the register
> > > > > > > > address message, the second time is a message to read the
> > > > > > > > register data, so that no stop signal is generated.
> > > > > > > >
> > > > > > > > This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > > > > > > flag for specific i2c chips, so if the i2c slave requires
> > > > > > > > a stop signal, chips driver can set this flag, then call
> > > > > > > > the dm_i2c_write to set the register address (a stop
> > > > > > > > signal is generated after this API call), then call
> > > > > > > > dm_i2c_read to read the register data.
> > > > > > > >
> > > > > > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > > > > ---
> > > > > > > > Changes in v3:
> > > > > > > > 	- Use the new flag
> > > > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > > > > > >
> > > > > > > > Changes in v2:
> > > > > > > >         - Split the original patch into 3 patches
> > > > > > > >         - Add detailed description information for each
> > > > > > > > patch
> > > > > > > >
> > > > > > > >  drivers/i2c/i2c-uclass.c | 6 ++++--
> > > > > > > >  include/i2c.h            | 1 +
> > > > > > > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/i2c/i2c-uclass.c
> > > > > > > > b/drivers/i2c/i2c-uclass.c index e47abf1833..9804b5e8c7
> > > > > > > > 100644 --- a/drivers/i2c/i2c-uclass.c
> > > > > > > > +++ b/drivers/i2c/i2c-uclass.c
> > > > > > > > @@ -135,8 +135,10 @@ int dm_i2c_read(struct udevice *dev,
> > > > > > > > uint offset, uint8_t *buffer, int len) if (chip->flags &
> > > > > > > > DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev,
> > > > > > > > offset, buffer, len); ptr = msg;
> > > > > > > > -	if (!i2c_setup_offset(chip, offset, offset_buf,
> > > > > > > > ptr))
> > > > > > > > -		ptr++;
> > > > > > > > +	if (!(chip->flags &
> > > > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET)) {
> > > > > > > > +		if (!i2c_setup_offset(chip, offset,
> > > > > > > > offset_buf, ptr))
> > > > > > > > +			ptr++;
> > > > > > > > +	}
> > > > > > > >
> > > > > > > >  	if (len) {
> > > > > > > >  		ptr->addr = chip->chip_addr; diff --git a/include/i2c.h
> > > > > > > > b/include/i2c.h index
> > > > > > > > a5c760c711..3123cbf280 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 */
> > > > > > >
> > > > > > > Aren't those two above flags describe exactly what you need?
> > > > > > > They send address for each read/written byte. (or do you
> > > > > > > need to send more than a single byte)?
> > > > > > This is not what I need. I need to generate a stop signal
> > > > > > after setting the register address,
> > > > >
> > > > > Please correct me if I'm wrong, but the above flag says that you
> > > > > need to send the I2C stop after you send one byte?
> > > > >
> > > > > Would it be possible to send address with I2C stop and then read
> > > > > data (single byte) with I2C stop?
> > > > >
> > > > > > so I need the following flag.
> > > > > > >
> > > > > > > >	DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET  = 1 << 3,
> > > > >
> > > > > I do find this name as a bit misleading - is the
> > > > > RD_NO_I2C_SETUP_OFFSET name reflecting the purpose (to send I2C
> > > > > stop after sending address)?
> > > > The purpose is to send a stop signal after setting the slave
> > > > register address.
> > >
> > > What I'm trying to point out is that the above description doesn't
> > > match the flag name. Maybe there is a better, more descriptive name?
> > Because the stop signal is generated by not calling the
> > i2c_setup_offset function, this flag is used.
> 
> But the name of the flag shall be not based on particular function name. It
> shall be a generic description - like presented above:
> 
> DM_I2C_CHIP_WR_ADDRESS = 1 << 2,
> /* Send address for each write byte */
> 
> With such generic names - there is no problem if we refactor the code and
> change the function name.
> 
> I've looked closer into those two flags:
>  git grep -n "DM_I2C_CHIP_WR_ADDRESS"
> 
> drivers/rtc/ds1307.c:319:
> drivers/rtc/isl1208.c:173:
> drivers/rtc/rv3029.c:469:
> 
> It looks like those above rtc devices use it - so maybe the issue described for
> pcf2127 is also present on those devices (so those flags have been
> introduced) ?
In this way, although each byte will send the slave address, this does not correspond to the timing of pcf2127. 
Also, there is no stop signal after setting the register address.
The chip also doesn't work.
> 
> > >
> > > Also - what is byte length of send address to this device? One byte
> > > ?
> > Yes,one byte
> > >
> > > > >
> > > > > Maybe there would be a better name?
> > > > >
> > > > > > > >       /* No i2c_setup_offset*/ };
> > > > > > > >
> > > > > > > >  struct udevice;
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > 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
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > 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
> > >
> > >
> > >
> > >
> > > 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
> 
> 
> 
> 
> 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

[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[Lukasz Majewski]

Hi Chuanhua,

> > -----Original Message-----
> > From: Lukasz Majewski <lukma@denx.de>
> > Sent: 2019年6月4日 16:19
> > To: Chuanhua Han <chuanhua.han@nxp.com>
> > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> > Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag
> > that not call i2c_setup_offset
> > 
> > On Tue, 4 Jun 2019 07:59:44 +0000
> > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Lukasz Majewski <lukma@denx.de>
> > > > Sent: 2019年6月4日 15:56
> > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > <biwen.li@nxp.com> Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2]
> > > > dm: i2c: Add a flag that not call i2c_setup_offset
> > > >
> > > > On Tue, 4 Jun 2019 06:48:59 +0000
> > > > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> > > >  
> > > > > > -----Original Message-----
> > > > > > From: Lukasz Majewski <lukma@denx.de>
> > > > > > Sent: 2019年6月4日 14:46
> > > > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > > > <biwen.li@nxp.com> Subject: Re: [EXT] Re: [U-Boot] [PATCH
> > > > > > 1/2] dm: i2c: Add a flag that not call i2c_setup_offset
> > > > > >
> > > > > > On Tue, 4 Jun 2019 02:20:49 +0000 Chuanhua Han
> > > > > > <chuanhua.han@nxp.com> wrote:
> > > > > >  
> > > > > > > > -----Original Message-----
> > > > > > > > From: Lukasz Majewski <lukma@denx.de>
> > > > > > > > Sent: 2019年6月4日 6:08
> > > > > > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > > > > > <biwen.li@nxp.com> Subject: [EXT] Re: [U-Boot] [PATCH
> > > > > > > > 1/2] dm: i2c: Add a flag that not call i2c_setup_offset
> > > > > > > >
> > > > > > > > On Thu, 30 May 2019 18:31:48 +0800 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). However, in
> > > > > > > > > the current i2c general code, the dm_i2c_read api
> > > > > > > > > encapsulates two messages, the first time is to set
> > > > > > > > > the register address message, the second time is a
> > > > > > > > > message to read the register data, so that no stop
> > > > > > > > > signal is generated.
> > > > > > > > >
> > > > > > > > > This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > > > > > > > flag for specific i2c chips, so if the i2c slave
> > > > > > > > > requires a stop signal, chips driver can set this
> > > > > > > > > flag, then call the dm_i2c_write to set the register
> > > > > > > > > address (a stop signal is generated after this API
> > > > > > > > > call), then call dm_i2c_read to read the register
> > > > > > > > > data.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > > > > > ---
> > > > > > > > > Changes in v3:
> > > > > > > > > 	- Use the new flag
> > > > > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > > > > > > >
> > > > > > > > > Changes in v2:
> > > > > > > > >         - Split the original patch into 3 patches
> > > > > > > > >         - Add detailed description information for
> > > > > > > > > each patch
> > > > > > > > >
> > > > > > > > >  drivers/i2c/i2c-uclass.c | 6 ++++--
> > > > > > > > >  include/i2c.h            | 1 +
> > > > > > > > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/i2c/i2c-uclass.c
> > > > > > > > > b/drivers/i2c/i2c-uclass.c index
> > > > > > > > > e47abf1833..9804b5e8c7 100644 ---
> > > > > > > > > a/drivers/i2c/i2c-uclass.c +++
> > > > > > > > > b/drivers/i2c/i2c-uclass.c @@ -135,8 +135,10 @@ int
> > > > > > > > > dm_i2c_read(struct udevice *dev, uint offset, uint8_t
> > > > > > > > > *buffer, int len) if (chip->flags &
> > > > > > > > > DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev,
> > > > > > > > > offset, buffer, len); ptr = msg;
> > > > > > > > > -	if (!i2c_setup_offset(chip, offset,
> > > > > > > > > offset_buf, ptr))
> > > > > > > > > -		ptr++;
> > > > > > > > > +	if (!(chip->flags &
> > > > > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET)) {
> > > > > > > > > +		if (!i2c_setup_offset(chip, offset,
> > > > > > > > > offset_buf, ptr))
> > > > > > > > > +			ptr++;
> > > > > > > > > +	}
> > > > > > > > >
> > > > > > > > >  	if (len) {
> > > > > > > > >  		ptr->addr = chip->chip_addr; diff
> > > > > > > > > --git a/include/i2c.h b/include/i2c.h index
> > > > > > > > > a5c760c711..3123cbf280 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 */  
> > > > > > > >
> > > > > > > > Aren't those two above flags describe exactly what you
> > > > > > > > need? They send address for each read/written byte. (or
> > > > > > > > do you need to send more than a single byte)?  
> > > > > > > This is not what I need. I need to generate a stop signal
> > > > > > > after setting the register address,  
> > > > > >
> > > > > > Please correct me if I'm wrong, but the above flag says
> > > > > > that you need to send the I2C stop after you send one byte?
> > > > > >
> > > > > > Would it be possible to send address with I2C stop and then
> > > > > > read data (single byte) with I2C stop?
> > > > > >  
> > > > > > > so I need the following flag.  
> > > > > > > >  
> > > > > > > > >	DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET  = 1 << 3,  
> > > > > >
> > > > > > I do find this name as a bit misleading - is the
> > > > > > RD_NO_I2C_SETUP_OFFSET name reflecting the purpose (to send
> > > > > > I2C stop after sending address)?  
> > > > > The purpose is to send a stop signal after setting the slave
> > > > > register address.  
> > > >
> > > > What I'm trying to point out is that the above description
> > > > doesn't match the flag name. Maybe there is a better, more
> > > > descriptive name?  
> > > Because the stop signal is generated by not calling the
> > > i2c_setup_offset function, this flag is used.  
> > 
> > But the name of the flag shall be not based on particular function
> > name. It shall be a generic description - like presented above:
> > 
> > DM_I2C_CHIP_WR_ADDRESS = 1 << 2,
> > /* Send address for each write byte */
> > 
> > With such generic names - there is no problem if we refactor the
> > code and change the function name.
> > 
> > I've looked closer into those two flags:
> >  git grep -n "DM_I2C_CHIP_WR_ADDRESS"
> > 
> > drivers/rtc/ds1307.c:319:
> > drivers/rtc/isl1208.c:173:
> > drivers/rtc/rv3029.c:469:
> > 
> > It looks like those above rtc devices use it - so maybe the issue
> > described for pcf2127 is also present on those devices (so those
> > flags have been introduced) ?  
> In this way, although each byte will send the slave address, this
> does not correspond to the timing of pcf2127. Also, there is no stop
> signal after setting the register address. The chip also doesn't work.

I don't mind to add new flag - It is OK.

Please only name it properly - like DM_I2C_CHIP_ADDR_STOP - to be
generic enough so other IC's with similar issue can re-use it.

> >   
> > > >
> > > > Also - what is byte length of send address to this device? One
> > > > byte ?  
> > > Yes,one byte  
> > > >  
> > > > > >
> > > > > > Maybe there would be a better name?
> > > > > >  
> > > > > > > > >       /* No i2c_setup_offset*/ };
> > > > > > > > >
> > > > > > > > >  struct udevice;  
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > 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  
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > 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  
> > > >
> > > >
> > > >
> > > >
> > > > 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  
> > 
> > 
> > 
> > 
> > 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  




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/20190617/5d4d3e39/attachment.sig>

[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that not call i2c_setup_offset

[Chuanhua Han]

> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年6月4日 16:19
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that not call
> i2c_setup_offset
> 
> On Tue, 4 Jun 2019 07:59:44 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukasz Majewski <lukma@denx.de>
> > > Sent: 2019年6月4日 15:56
> > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>
> > > Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2] dm: i2c: Add a flag that
> > > not call i2c_setup_offset
> > >
> > > On Tue, 4 Jun 2019 06:48:59 +0000
> > > Chuanhua Han <chuanhua.han@nxp.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Lukasz Majewski <lukma@denx.de>
> > > > > Sent: 2019年6月4日 14:46
> > > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > > <biwen.li@nxp.com> Subject: Re: [EXT] Re: [U-Boot] [PATCH 1/2]
> > > > > dm: i2c: Add a flag that not call i2c_setup_offset
> > > > >
> > > > > On Tue, 4 Jun 2019 02:20:49 +0000 Chuanhua Han
> > > > > <chuanhua.han@nxp.com> wrote:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Lukasz Majewski <lukma@denx.de>
> > > > > > > Sent: 2019年6月4日 6:08
> > > > > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > > > > <biwen.li@nxp.com> Subject: [EXT] Re: [U-Boot] [PATCH 1/2]
> > > > > > > dm: i2c: Add a flag that not call i2c_setup_offset
> > > > > > >
> > > > > > > On Thu, 30 May 2019 18:31:48 +0800 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). However, in the
> > > > > > > > current i2c general code, the dm_i2c_read api encapsulates
> > > > > > > > two messages, the first time is to set the register
> > > > > > > > address message, the second time is a message to read the
> > > > > > > > register data, so that no stop signal is generated.
> > > > > > > >
> > > > > > > > This patch uses the DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > > > > > > flag for specific i2c chips, so if the i2c slave requires
> > > > > > > > a stop signal, chips driver can set this flag, then call
> > > > > > > > the dm_i2c_write to set the register address (a stop
> > > > > > > > signal is generated after this API call), then call
> > > > > > > > dm_i2c_read to read the register data.
> > > > > > > >
> > > > > > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > > > > ---
> > > > > > > > Changes in v3:
> > > > > > > > 	- Use the new flag
> > > > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET
> > > > > > > >
> > > > > > > > Changes in v2:
> > > > > > > >         - Split the original patch into 3 patches
> > > > > > > >         - Add detailed description information for each
> > > > > > > > patch
> > > > > > > >
> > > > > > > >  drivers/i2c/i2c-uclass.c | 6 ++++--
> > > > > > > >  include/i2c.h            | 1 +
> > > > > > > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/i2c/i2c-uclass.c
> > > > > > > > b/drivers/i2c/i2c-uclass.c index e47abf1833..9804b5e8c7
> > > > > > > > 100644 --- a/drivers/i2c/i2c-uclass.c
> > > > > > > > +++ b/drivers/i2c/i2c-uclass.c
> > > > > > > > @@ -135,8 +135,10 @@ int dm_i2c_read(struct udevice *dev,
> > > > > > > > uint offset, uint8_t *buffer, int len) if (chip->flags &
> > > > > > > > DM_I2C_CHIP_RD_ADDRESS) return i2c_read_bytewise(dev,
> > > > > > > > offset, buffer, len); ptr = msg;
> > > > > > > > -	if (!i2c_setup_offset(chip, offset, offset_buf,
> > > > > > > > ptr))
> > > > > > > > -		ptr++;
> > > > > > > > +	if (!(chip->flags &
> > > > > > > > DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET)) {
> > > > > > > > +		if (!i2c_setup_offset(chip, offset,
> > > > > > > > offset_buf, ptr))
> > > > > > > > +			ptr++;
> > > > > > > > +	}
> > > > > > > >
> > > > > > > >  	if (len) {
> > > > > > > >  		ptr->addr = chip->chip_addr; diff --git a/include/i2c.h
> > > > > > > > b/include/i2c.h index
> > > > > > > > a5c760c711..3123cbf280 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 */
> > > > > > >
> > > > > > > Aren't those two above flags describe exactly what you need?
> > > > > > > They send address for each read/written byte. (or do you
> > > > > > > need to send more than a single byte)?
> > > > > > This is not what I need. I need to generate a stop signal
> > > > > > after setting the register address,
> > > > >
> > > > > Please correct me if I'm wrong, but the above flag says that you
> > > > > need to send the I2C stop after you send one byte?
> > > > >
> > > > > Would it be possible to send address with I2C stop and then read
> > > > > data (single byte) with I2C stop?
> > > > >
> > > > > > so I need the following flag.
> > > > > > >
> > > > > > > >	DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET  = 1 << 3,
> > > > >
> > > > > I do find this name as a bit misleading - is the
> > > > > RD_NO_I2C_SETUP_OFFSET name reflecting the purpose (to send I2C
> > > > > stop after sending address)?
> > > > The purpose is to send a stop signal after setting the slave
> > > > register address.
> > >
> > > What I'm trying to point out is that the above description doesn't
> > > match the flag name. Maybe there is a better, more descriptive name?
> > Because the stop signal is generated by not calling the
> > i2c_setup_offset function, this flag is used.
> 
> But the name of the flag shall be not based on particular function name. It
> shall be a generic description - like presented above:
> 
> DM_I2C_CHIP_WR_ADDRESS = 1 << 2,
> /* Send address for each write byte */
> 
> With such generic names - there is no problem if we refactor the code and
> change the function name.
> 
> I've looked closer into those two flags:
>  git grep -n "DM_I2C_CHIP_WR_ADDRESS"
> 
> drivers/rtc/ds1307.c:319:
> drivers/rtc/isl1208.c:173:
> drivers/rtc/rv3029.c:469:
> 
> It looks like those above rtc devices use it - so maybe the issue described for
> pcf2127 is also present on those devices (so those flags have been
> introduced) ?
> 
> > >
> > > Also - what is byte length of send address to this device? One byte
> > > ?
> > Yes,one byte
> > >
> > > > >
> > > > > Maybe there would be a better name?
> > > > >
> > > > > > > >       /* No i2c_setup_offset*/ };
> > > > > > > >
> > > > > > > >  struct udevice;
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > 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
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > 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
> > >
> > >
> > >
> > >
> > > 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
> 
> 
> 
> 
> 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
Hi,
I have already replied to your question, are there any other questions? I look forward to your reply, please review! Thank you