[U-Boot] [PATCH 1/2] dm: i2c: Add a flag that need generate stop bit

[U-Boot] [PATCH 1/2] dm: i2c: Add a flag that need generate 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).
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_ADDR_STOP 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 v4:
	- Replace DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET with
DM_I2C_CHIP_ADDR_STOP.

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_ADDR_STOP)) {
+		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_ADDR_STOP  = 1 << 3, /* Need generate stop bit */
 };
 
 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_ADDR_STOP 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 v4:
	- Replace DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET with
DM_I2C_CHIP_ADDR_STOP.

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_ADDR_STOP);
+
+	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 2/2] rtc: pcf2127: Fixed bug with rtc settings and getting error time

[Lukasz Majewski]

Hi Chuanhua,

> 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_ADDR_STOP flag in order to
> generate a stop signal and fixes the bug of the original read and
> write time.

Thank you for the patch. It now looks far more better than the previous
versions.

It is not using generic flag and can be reused by other ICs with
similar issues.

Let's wait for Heiko's opinion.

Reviewed-by: Lukasz Majewski <lukma@denx.de>

> 
> Signed-off-by: Biwen Li <biwen.li@nxp.com>
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
> Changes in v4:
> 	- Replace DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET with
> DM_I2C_CHIP_ADDR_STOP.
> 
> 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_ADDR_STOP);
> +
> +	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,
>  };




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/20190618/fdb3ff34/attachment.sig>

[U-Boot] [PATCH 1/2] dm: i2c: Add a flag that need generate stop bit

[Lukasz Majewski]

On Mon, 17 Jun 2019 21:12:40 +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_ADDR_STOP 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>

Reviewed-by: Lukasz Majewski <lukma@denx.de>

> ---
> Changes in v4:
> 	- Replace DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET with
> DM_I2C_CHIP_ADDR_STOP.
> 
> 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_ADDR_STOP)) {
> +		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_ADDR_STOP  = 1 << 3, /* Need generate stop bit */
>  };
>  
>  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/20190618/5b8b95c3/attachment.sig>

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

[Heiko Schocher]

Hello Lukasz,

Am 18.06.2019 um 10:06 schrieb Lukasz Majewski:
> Hi Chuanhua,
> 
>> 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_ADDR_STOP flag in order to
>> generate a stop signal and fixes the bug of the original read and
>> write time.
> 
> Thank you for the patch. It now looks far more better than the previous
> versions.

Yep!

> It is not using generic flag and can be reused by other ICs with
> similar issues.
> 
> Let's wait for Heiko's opinion.

I already started a travis build, see:

https://travis-ci.org/hsdenx/u-boot-i2c/builds/547057368

Thanks for your comments!

bye,
Heiko
> 
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> 
>>
>> Signed-off-by: Biwen Li <biwen.li@nxp.com>
>> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
>> ---
>> Changes in v4:
>> 	- Replace DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET with
>> DM_I2C_CHIP_ADDR_STOP.
>>
>> 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_ADDR_STOP);
>> +
>> +	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,
>>   };
> 
> 
> 
> 
> 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
> 

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

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

[Chuanhua Han]

+ Simon Glass

> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年6月18日 16:06
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: hs at denx.de; sjg at chromium.org; Biwen Li <biwen.li@nxp.com>;
> u-boot at lists.denx.de
> Subject: [EXT] Re: [PATCH 2/2] rtc: pcf2127: Fixed bug with rtc settings and
> getting error time
> 
> Hi Chuanhua,
> 
> > 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_ADDR_STOP flag in order to generate a
> > stop signal and fixes the bug of the original read and write time.
> 
> Thank you for the patch. It now looks far more better than the previous
> versions.
> 
> It is not using generic flag and can be reused by other ICs with similar issues.
> 
> Let's wait for Heiko's opinion.
> 
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> 
> >
> > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > ---
> > Changes in v4:
> > 	- Replace DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET with
> > DM_I2C_CHIP_ADDR_STOP.
> >
> > 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_ADDR_STOP);
> > +
> > +	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,
> >  };
> 
> 
> 
> 
> 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 need generate stop bit

[Chuanhua Han]

+ Simon Glass

> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年6月18日 16:07
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: hs at denx.de; sjg at chromium.org; Biwen Li <biwen.li@nxp.com>;
> u-boot at lists.denx.de
> Subject: [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that need generate stop bit
> 
> On Mon, 17 Jun 2019 21:12:40 +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_ADDR_STOP 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>
> 
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> 
> > ---
> > Changes in v4:
> > 	- Replace DM_I2C_CHIP_RD_NO_I2C_SETUP_OFFSET with
> > DM_I2C_CHIP_ADDR_STOP.
> >
> > 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_ADDR_STOP)) {
> > +		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_ADDR_STOP  = 1 << 3, /* Need generate stop bit */
> >  };
> >
> >  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 need generate stop bit

[Simon Glass]

Hi Chuanhua,

On Sun, 23 Jun 2019 at 22:48, Chuanhua Han <chuanhua.han@nxp.com> wrote:
>
> + Simon Glass
>
> > -----Original Message-----
> > From: Lukasz Majewski <lukma@denx.de>
> > Sent: 2019年6月18日 16:07
> > To: Chuanhua Han <chuanhua.han@nxp.com>
> > Cc: hs at denx.de; sjg at chromium.org; Biwen Li <biwen.li@nxp.com>;
> > u-boot at lists.denx.de
> > Subject: [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that need generate stop bit
> >
> > On Mon, 17 Jun 2019 21:12:40 +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_ADDR_STOP 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>
> >
> > Reviewed-by: Lukasz Majewski <lukma@denx.de>

I already asked why you cannot use dm_i2c_xfer() to do what you want,
but I did not see your response. Can you please send your response
again here on this thread? The dm_i2c_xfer() function is designed to
handle any sorts of quirk that are needed. I'd like to avoid quirks in
the core code if possible.

If you really want to add a quirk, then I would like to put it behind
a CONFIG_I2C_QUIRKS option, and use:

  if (IS_ENABLED(CONFIG_I2C_QUIRKS) || !(chip->flags & DM_I2C_CHIP_ADDR_STOP)) {
           if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
                   ptr++;

so we don't affect code size on other platforms. After all this
feature is just for one broken chip and should not negatively affect
everyone else that follows the spec correctly.

I am worried about all the SPL-affecting changing that add a few bytes
here and there, but really add up.

But my first preference would be to use i2c_xfer() if possible.

Regards,
Simon

[U-Boot] [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that need generate stop bit

[Chuanhua Han]

> -----Original Message-----
> From: Simon Glass <sjg@chromium.org>
> Sent: 2019年7月7日 1:17
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: Lukasz Majewski <lukma@denx.de>; hs at denx.de; Biwen Li
> <biwen.li@nxp.com>; u-boot at lists.denx.de
> Subject: Re: [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that need generate stop
> bit
> 
> Caution: EXT Email
> 
> Hi Chuanhua,
> 
> On Sun, 23 Jun 2019 at 22:48, Chuanhua Han <chuanhua.han@nxp.com>
> wrote:
> >
> > + Simon Glass
> >
> > > -----Original Message-----
> > > From: Lukasz Majewski <lukma@denx.de>
> > > Sent: 2019年6月18日 16:07
> > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > Cc: hs at denx.de; sjg at chromium.org; Biwen Li <biwen.li@nxp.com>;
> > > u-boot at lists.denx.de
> > > Subject: [EXT] Re: [PATCH 1/2] dm: i2c: Add a flag that need
> > > generate stop bit
> > >
> > > On Mon, 17 Jun 2019 21:12:40 +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_ADDR_STOP 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>
> > >
> > > Reviewed-by: Lukasz Majewski <lukma@denx.de>
> 
> I already asked why you cannot use dm_i2c_xfer() to do what you want, but I
> did not see your response. Can you please send your response again here on
> this thread? The dm_i2c_xfer() function is designed to handle any sorts of
> quirk that are needed. I'd like to avoid quirks in the core code if possible.
> 
> If you really want to add a quirk, then I would like to put it behind a
> CONFIG_I2C_QUIRKS option, and use:
> 
>   if (IS_ENABLED(CONFIG_I2C_QUIRKS) || !(chip->flags &
> DM_I2C_CHIP_ADDR_STOP)) {
>            if (!i2c_setup_offset(chip, offset, offset_buf, ptr))
>                    ptr++;
> 
> so we don't affect code size on other platforms. After all this feature is just for
> one broken chip and should not negatively affect everyone else that follows the
> spec correctly.
> 
> I am worried about all the SPL-affecting changing that add a few bytes here
> and there, but really add up.
> 
> But my first preference would be to use i2c_xfer() if possible.
OK,thanks,I will send next version!
> 
> Regards,
> Simon