Skip to contentSkip to conversation

fuelgauge: max17042: fix i2c read issue which causes infinity loop.

PATCH Superseded Przemyslaw Marczak 3 participants 8 messages Dec 10, 2013 – Jan 14, 2014

lore
Share & tools
lore

Przemyslaw Marczak

author
#0 lore raw

PATCH 1/1 fuelgauge: max17042: fix i2c read issue which causes infinity loop.

Issues:
- reading i2c data by passing u16 pointer causes errors in read data.
- max17042 status register fields have not only Power On Reset meaning so using proper mask is required.

Changes:
- read i2c data to type u32 instead of u16 - avoids buffer overflow
- compare FG status register using mask not just one bit value

Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Lukasz Majewski <[email protected]>
---
 drivers/power/fuel_gauge/fg_max17042.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/power/fuel_gauge/fg_max17042.c +6 -3
index c285747..2cbf8cf 100644
@@ -28,11 +28,14 @@ static int fg_write_regs(struct pmic *p, u8 addr, u16 *data, int num)
 
 static int fg_read_regs(struct pmic *p, u8 addr, u16 *data, int num)
 {
+	unsigned int dat = 0;
 	int ret = 0;
 	int i;
 
-	for (i = 0; i < num; i++, addr++)
-		ret |= pmic_reg_read(p, addr, (u32 *) (data + i));
+	for (i = 0; i < num; i++, addr++) {
+		ret |= pmic_reg_read(p, addr, &dat);
+		*(data + i) = (u16) dat;
+	}
 
 	return ret;
 }

@@ -178,7 +181,7 @@ static int power_check_battery(struct pmic *p, struct pmic *bat)
 	ret |= pmic_reg_read(p, MAX17042_STATUS, &val);
 	debug("fg status: 0x%x\n", val);
 
-	if (val == MAX17042_POR)
+	if (val && MAX17042_POR)
 		por_fuelgauge_init(p);
 
 	ret |= pmic_reg_read(p, MAX17042_VERSION, &val);
Signature · 1 line
1.7.9.5
#1 lore raw

PATCH 1/1 fuelgauge: max17042: fix i2c read issue which causes infinity loop.

Dear Przemyslaw Marczak,

Quoted 20 lines · Przemyslaw Marczak

Issues:
- reading i2c data by passing u16 pointer causes errors in read data.
- max17042 status register fields have not only Power On Reset meaning so using proper mask is required.

Changes:
- read i2c data to type u32 instead of u16 - avoids buffer overflow
- compare FG status register using mask not just one bit value

Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Lukasz Majewski <[email protected]>
---
 drivers/power/fuel_gauge/fg_max17042.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/power/fuel_gauge/fg_max17042.c +0 -0
index c285747..2cbf8cf 100644

drivers/power/fuel_gauge/fg_max17042.c static int fg_write_regs(struct pmic *p, u8 addr, u16 *data, int num) · line 31

Quoted 4 lines
drivers/power/fuel_gauge/fg_max17042.c +1 -0
@@ -28,11 +28,14 @@ static int fg_write_regs(struct pmic *p, u8 addr, u16 *data, int num)
 
 static int fg_read_regs(struct pmic *p, u8 addr, u16 *data, int num)
 {
+	unsigned int dat = 0;

initial value is unnecessary.

drivers/power/fuel_gauge/fg_max17042.c static int fg_write_regs(struct pmic *p, u8 addr, u16 *data, int num) · line 36

Quoted 7 lines
drivers/power/fuel_gauge/fg_max17042.c +2 -2
 	int ret = 0;
 	int i;
 
-	for (i = 0; i < num; i++, addr++)
-		ret |= pmic_reg_read(p, addr, (u32 *) (data + i));
+	for (i = 0; i < num; i++, addr++) {
+		ret |= pmic_reg_read(p, addr, &dat);

I think, need check return value.
if failed to read then you should not update data. and such a case, do we need to read end of num? why don't you return error directly?
I think this code should be..

		ret = pmic_reg_read(p, addr, &dat);
		if (ret)
			return ret;

		*(data + i) = (u16)dat;

drivers/power/fuel_gauge/fg_max17042.c static int fg_write_regs(struct pmic *p, u8 addr, u16 *data, int num) · line 37

Quoted 1 line
drivers/power/fuel_gauge/fg_max17042.c +1 -0
+		*(data + i) = (u16) dat;

please remove space between ) and dat.

drivers/power/fuel_gauge/fg_max17042.c static int fg_write_regs(struct pmic *p, u8 addr, u16 *data, int num) · line 40

Quoted 3 lines
drivers/power/fuel_gauge/fg_max17042.c +1 -0
+	}
 
 	return ret;

then it can be return 0; and initial value ( = 0) is unnecessary.

drivers/power/fuel_gauge/fg_max17042.c static int power_check_battery(struct pmic *p, struct pmic *bat) · line 184

Quoted 7 lines
drivers/power/fuel_gauge/fg_max17042.c +1 -1
 }

@@ -178,7 +181,7 @@ static int power_check_battery(struct pmic *p, struct pmic *bat)
 	ret |= pmic_reg_read(p, MAX17042_STATUS, &val);
 	debug("fg status: 0x%x\n", val);
 
-	if (val == MAX17042_POR)
+	if (val && MAX17042_POR)

if (val & MAX17042_POR) ?

drivers/power/fuel_gauge/fg_max17042.c static int power_check_battery(struct pmic *p, struct pmic *bat) · line 187

Quoted 4 lines
drivers/power/fuel_gauge/fg_max17042.c +0 -0
 		por_fuelgauge_init(p);
 
 	ret |= pmic_reg_read(p, MAX17042_VERSION, &val);

Thanks,
Minkyu Kang.

author
#2 lore raw

PATCH v2 1/1 fuelgauge: max17042: fix i2c read issue which causes infinity loop.

In reply to Przemyslaw Marczak

Issues:
- reading i2c data by passing u16 pointer causes errors in read data.
- max17042 status register fields have not only Power On Reset meaning so using proper mask is required.

Changes:
- read i2c data to type u32 instead of u16 - avoids buffer overflow
- compare FG status register using mask not just one bit value
- add checking return value to functions fg read/write
- add model lock and model check count
- add debug msg

Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Minkyu Kang <[email protected]>

---
Changes v2:
- add checking return value to functions fg read/write
- add model lock and model check count
- add status msg
- change logical AND to bitwise AND when checking status register

 drivers/power/fuel_gauge/fg_max17042.c |  120 +++++++++++++++++++++++---------
 1 file changed, 86 insertions(+), 34 deletions(-)
drivers/power/fuel_gauge/fg_max17042.c +86 -34
index c285747..154ca6a 100644
@@ -20,21 +20,30 @@ static int fg_write_regs(struct pmic *p, u8 addr, u16 *data, int num)
 	int ret = 0;
 	int i;
 
-	for (i = 0; i < num; i++, addr++)
-		ret |= pmic_reg_write(p, addr, *(data + i));
+	for (i = 0; i < num; i++, addr++) {
+		ret = pmic_reg_write(p, addr, *(data + i));
+		if (ret)
+			return ret;
+	}
 
-	return ret;
+	return 0;
 }
 
 static int fg_read_regs(struct pmic *p, u8 addr, u16 *data, int num)
 {
+	unsigned int dat;
 	int ret = 0;
 	int i;
 
-	for (i = 0; i < num; i++, addr++)
-		ret |= pmic_reg_read(p, addr, (u32 *) (data + i));
+	for (i = 0; i < num; i++, addr++) {
+		ret = pmic_reg_read(p, addr, &dat);
+		if (ret)
+			return ret;
 
-	return ret;
+		*(data + i) = (u16)dat;
+	}
+
+	return 0;
 }
 
 static int fg_write_and_verify(struct pmic *p, u8 addr, u16 data)

@@ -57,9 +66,13 @@ static int fg_write_and_verify(struct pmic *p, u8 addr, u16 data)
 static void por_fuelgauge_init(struct pmic *p)
 {
 	u16 r_data0[16], r_data1[16], r_data2[16];
-	u32 rewrite_count = 5, i = 0;
-	unsigned int val;
-	int ret = 0;
+	u32 rewrite_count = 5;
+	u32 check_count;
+	u32 lock_count;
+	u32 i = 0;
+	u32 val;
+	s32 ret = 0;
+	char *status_msg;
 
 	/* Delay 500 ms */
 	mdelay(500);

@@ -67,29 +80,55 @@ static void por_fuelgauge_init(struct pmic *p)
 	pmic_reg_write(p, MAX17042_CONFIG, 0x2310);
 
 rewrite_model:
+	check_count = 5;
+	lock_count = 5;
+
+	if (!rewrite_count--) {
+		status_msg = "init failed!";
+		goto error;
+	}
+
 	/* Unlock Model Access */
 	pmic_reg_write(p, MAX17042_MLOCKReg1, MODEL_UNLOCK1);
 	pmic_reg_write(p, MAX17042_MLOCKReg2, MODEL_UNLOCK2);
 
 	/* Write/Read/Verify the Custom Model */
-	ret |= fg_write_regs(p, MAX17042_MODEL1, cell_character0,
+	ret = fg_write_regs(p, MAX17042_MODEL1, cell_character0,
 			     ARRAY_SIZE(cell_character0));
-	ret |= fg_write_regs(p, MAX17042_MODEL2, cell_character1,
+	if (ret)
+		goto rewrite_model;
+
+	ret = fg_write_regs(p, MAX17042_MODEL2, cell_character1,
 			     ARRAY_SIZE(cell_character1));
-	ret |= fg_write_regs(p, MAX17042_MODEL3, cell_character2,
+	if (ret)
+		goto rewrite_model;
+
+	ret = fg_write_regs(p, MAX17042_MODEL3, cell_character2,
 			     ARRAY_SIZE(cell_character2));
+	if (ret)
+		goto rewrite_model;
 
-	if (ret) {
-		printf("%s: Cell parameters write failed!\n", __func__);
-		return;
+check_model:
+	if (!check_count--) {
+		if (rewrite_count)
+			goto rewrite_model;
+		else
+			status_msg = "check failed!";
+
+		goto error;
 	}
 
-	ret |= fg_read_regs(p, MAX17042_MODEL1, r_data0, ARRAY_SIZE(r_data0));
-	ret |= fg_read_regs(p, MAX17042_MODEL2, r_data1, ARRAY_SIZE(r_data1));
-	ret |= fg_read_regs(p, MAX17042_MODEL3, r_data2, ARRAY_SIZE(r_data2));
+	ret = fg_read_regs(p, MAX17042_MODEL1, r_data0, ARRAY_SIZE(r_data0));
+	if (ret)
+		goto check_model;
+
+	ret = fg_read_regs(p, MAX17042_MODEL2, r_data1, ARRAY_SIZE(r_data1));
+	if (ret)
+		goto check_model;
 
+	ret = fg_read_regs(p, MAX17042_MODEL3, r_data2, ARRAY_SIZE(r_data2));
 	if (ret)
-		printf("%s: Cell parameters read failed!\n", __func__);
+		goto check_model;
 
 	for (i = 0; i < 16; i++) {
 		if ((cell_character0[i] != r_data0[i])

@@ -98,29 +137,37 @@ rewrite_model:
 			goto rewrite_model;
 		}
 
+lock_model:
+	if (!lock_count--) {
+		if (rewrite_count)
+			goto rewrite_model;
+		else
+			status_msg = "lock failed!";
+
+		goto error;
+	}
+
 	/* Lock model access */
 	pmic_reg_write(p, MAX17042_MLOCKReg1, MODEL_LOCK1);
 	pmic_reg_write(p, MAX17042_MLOCKReg2, MODEL_LOCK2);
 
 	/* Verify the model access is locked */
-	ret |= fg_read_regs(p, MAX17042_MODEL1, r_data0, ARRAY_SIZE(r_data0));
-	ret |= fg_read_regs(p, MAX17042_MODEL2, r_data1, ARRAY_SIZE(r_data1));
-	ret |= fg_read_regs(p, MAX17042_MODEL3, r_data2, ARRAY_SIZE(r_data2));
+	ret = fg_read_regs(p, MAX17042_MODEL1, r_data0, ARRAY_SIZE(r_data0));
+	if (ret)
+		goto lock_model;
 
-	if (ret) {
-		printf("%s: Cell parameters read failed!\n", __func__);
-		return;
-	}
+	ret = fg_read_regs(p, MAX17042_MODEL2, r_data1, ARRAY_SIZE(r_data1));
+	if (ret)
+		goto lock_model;
+
+	ret = fg_read_regs(p, MAX17042_MODEL3, r_data2, ARRAY_SIZE(r_data2));
+	if (ret)
+		goto lock_model;
 
 	for (i = 0; i < ARRAY_SIZE(r_data0); i++) {
 		/* Check if model locked */
-		if (r_data0[i] || r_data1[i] || r_data2[i]) {
-			/* Rewrite model data - prevent from endless loop */
-			if (rewrite_count--) {
-				puts("FG - Lock model access failed!\n");
-				goto rewrite_model;
-			}
-		}
+		if (r_data0[i] || r_data1[i] || r_data2[i])
+			goto lock_model;
 	}
 
 	/* Write Custom Parameters */

@@ -137,6 +184,11 @@ rewrite_model:
 
 	/* Delay@least 350 ms */
 	mdelay(350);
+
+	status_msg = "OK!";
+error:
+	debug("%s: model init status: %s\n", p->name, status_msg);
+	return;
 }
 
 static int power_update_battery(struct pmic *p, struct pmic *bat)

@@ -178,7 +230,7 @@ static int power_check_battery(struct pmic *p, struct pmic *bat)
 	ret |= pmic_reg_read(p, MAX17042_STATUS, &val);
 	debug("fg status: 0x%x\n", val);
 
-	if (val == MAX17042_POR)
+	if (val & MAX17042_POR)
 		por_fuelgauge_init(p);
 
 	ret |= pmic_reg_read(p, MAX17042_VERSION, &val);
Signature · 1 line
1.7.9.5
author
#3 lore raw

PATCH v2 1/1 fuelgauge: max17042: fix i2c read issue which causes infinity loop.

Hello,

Quoted 16 lines · Przemyslaw Marczak

Issues:
- reading i2c data by passing u16 pointer causes errors in read data.
- max17042 status register fields have not only Power On Reset meaning so using proper mask is required.

Changes:
- read i2c data to type u32 instead of u16 - avoids buffer overflow
- compare FG status register using mask not just one bit value
- add checking return value to functions fg read/write
- add model lock and model check count
- add debug msg

Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Minkyu Kang <[email protected]>
Quoted 7 lines

---
Changes v2:
- add checking return value to functions fg read/write
- add model lock and model check count
- add status msg
- change logical AND to bitwise AND when checking status register

To test this patch, it is required to apply trats i2c fix by Piotr Wilczek:
"[PATCH] board:trats1:trats2: fix adapter number"
which can be found at u-boot list.
In other way FG will be not initialized.

Regards,

Signature · 4 lines
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
author
#4 lore raw

PATCH v2 1/1 fuelgauge: max17042: fix i2c read issue which causes infinity loop.

Hello Minkyu,

Quoted 27 lines · Przemyslaw Marczak

Hello,

Quoted 16 lines · Przemyslaw Marczak

Issues:
- reading i2c data by passing u16 pointer causes errors in read data.
- max17042 status register fields have not only Power On Reset meaning so using proper mask is required.

Changes:
- read i2c data to type u32 instead of u16 - avoids buffer overflow
- compare FG status register using mask not just one bit value
- add checking return value to functions fg read/write
- add model lock and model check count
- add debug msg

Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Minkyu Kang <[email protected]>
Quoted 7 lines

---
Changes v2:
- add checking return value to functions fg read/write
- add model lock and model check count
- add status msg
- change logical AND to bitwise AND when checking status register

Quoted 6 lines

To test this patch, it is required to apply trats i2c fix by Piotr Wilczek:
"[PATCH] board:trats1:trats2: fix adapter number"
which can be found at u-boot list.
In other way FG will be not initialized.

Regards,

Could you review this patch, please?
Is it possible to apply this before the release?
It is useful fix.

Thank you,

Signature · 4 lines
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
#5 lore raw

PATCH v2 1/1 fuelgauge: max17042: fix i2c read issue which causes infinity loop.

Quoted 35 lines · Przemyslaw Marczak

Hello Minkyu,

Quoted 32 lines · Przemyslaw Marczak

Hello,

Quoted 16 lines · Przemyslaw Marczak

Issues:
- reading i2c data by passing u16 pointer causes errors in read data.
- max17042 status register fields have not only Power On Reset meaning so using proper mask is required.

Changes:
- read i2c data to type u32 instead of u16 - avoids buffer overflow
- compare FG status register using mask not just one bit value
- add checking return value to functions fg read/write
- add model lock and model check count
- add debug msg

Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Minkyu Kang <[email protected]>
Quoted 7 lines

---
Changes v2:
- add checking return value to functions fg read/write
- add model lock and model check count
- add status msg
- change logical AND to bitwise AND when checking status register

To test this patch, it is required to apply trats i2c fix by Piotr Wilczek:
"[PATCH] board:trats1:trats2: fix adapter number"
which can be found at u-boot list.
In other way FG will be not initialized.

Quoted 7 lines
Quoted 1 line

Regards,

Could you review this patch, please?
Is it possible to apply this before the release?
It is useful fix.

Thank you,

This patch was delegated to Tom.

Thanks,
Minkyu Kang.

author
#6 lore raw

PATCH v2 1/1 fuelgauge: max17042: fix i2c read issue which causes infinity loop.

Hello,

Quoted 42 lines · Minkyu Kang
Quoted 41 lines · Przemyslaw Marczak

Hello Minkyu,

Quoted 27 lines · Przemyslaw Marczak

Hello,

Quoted 16 lines · Przemyslaw Marczak

Issues:
- reading i2c data by passing u16 pointer causes errors in read data.
- max17042 status register fields have not only Power On Reset meaning so using proper mask is required.

Changes:
- read i2c data to type u32 instead of u16 - avoids buffer overflow
- compare FG status register using mask not just one bit value
- add checking return value to functions fg read/write
- add model lock and model check count
- add debug msg

Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Minkyu Kang <[email protected]>
Quoted 7 lines

---
Changes v2:
- add checking return value to functions fg read/write
- add model lock and model check count
- add status msg
- change logical AND to bitwise AND when checking status register

Quoted 6 lines

To test this patch, it is required to apply trats i2c fix by Piotr Wilczek:
"[PATCH] board:trats1:trats2: fix adapter number"
which can be found at u-boot list.
In other way FG will be not initialized.

Regards,

Could you review this patch, please?
Is it possible to apply this before the release?
It is useful fix.

Quoted 7 lines
Quoted 1 line

Thank you,

This patch was delegated to Tom.

Thanks,
Minkyu Kang.

Ok, thank you,

Signature · 4 lines
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
#7 lore raw

PATCH v2 1/1 fuelgauge: max17042: fix i2c read issue which causes infinity loop.

In reply to Przemyslaw Marczak
Quoted 12 lines · Przemyslaw Marczak

Issues:
- reading i2c data by passing u16 pointer causes errors in read data.
- max17042 status register fields have not only Power On Reset meaning so using proper mask is required.

Changes:
- read i2c data to type u32 instead of u16 - avoids buffer overflow
- compare FG status register using mask not just one bit value
- add checking return value to functions fg read/write
- add model lock and model check count
- add debug msg

Quoted 3 lines
Signed-off-by Przemyslaw Marczak <[email protected]>
Cc Lukasz Majewski <[email protected]>
Cc Minkyu Kang <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140114/bd69db8d/attachment.pgp>