[U-Boot] [PATCH] 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 p.marczak@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com --- drivers/power/fuel_gauge/fg_max17042.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/power/fuel_gauge/fg_max17042.c b/drivers/power/fuel_gauge/fg_max17042.c index c285747..2cbf8cf 100644 --- a/drivers/power/fuel_gauge/fg_max17042.c +++ b/drivers/power/fuel_gauge/fg_max17042.c @@ -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);
Dear Przemyslaw Marczak,
On 11/12/13 00:19, Przemyslaw Marczak wrote:
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 p.marczak@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com
drivers/power/fuel_gauge/fg_max17042.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/power/fuel_gauge/fg_max17042.c b/drivers/power/fuel_gauge/fg_max17042.c index c285747..2cbf8cf 100644 --- a/drivers/power/fuel_gauge/fg_max17042.c +++ b/drivers/power/fuel_gauge/fg_max17042.c @@ -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.
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;
*(data + i) = (u16) dat;
please remove space between ) and dat.
}
return ret;
then it can be return 0; and initial value ( = 0) is unnecessary.
} @@ -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) ?
por_fuelgauge_init(p);ret |= pmic_reg_read(p, MAX17042_VERSION, &val);
Thanks, Minkyu Kang.
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 p.marczak@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: Minkyu Kang mk7.kang@samsung.com
--- 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(-)
diff --git a/drivers/power/fuel_gauge/fg_max17042.c b/drivers/power/fuel_gauge/fg_max17042.c index c285747..154ca6a 100644 --- a/drivers/power/fuel_gauge/fg_max17042.c +++ b/drivers/power/fuel_gauge/fg_max17042.c @@ -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 at 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);
Hello,
On 12/30/2013 11:24 AM, Przemyslaw Marczak wrote:
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 p.marczak@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: Minkyu Kang mk7.kang@samsung.com
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,
Hello Minkyu,
On 12/30/2013 11:31 AM, Przemyslaw Marczak wrote:
Hello,
On 12/30/2013 11:24 AM, Przemyslaw Marczak wrote:
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 p.marczak@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: Minkyu Kang mk7.kang@samsung.com
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,
Could you review this patch, please? Is it possible to apply this before the release? It is useful fix.
Thank you,
On 13/01/14 18:27, Przemyslaw Marczak wrote:
Hello Minkyu,
On 12/30/2013 11:31 AM, Przemyslaw Marczak wrote:
Hello,
On 12/30/2013 11:24 AM, Przemyslaw Marczak wrote:
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 p.marczak@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: Minkyu Kang mk7.kang@samsung.com
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,
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.
Hello,
On 01/14/2014 01:24 AM, Minkyu Kang wrote:
On 13/01/14 18:27, Przemyslaw Marczak wrote:
Hello Minkyu,
On 12/30/2013 11:31 AM, Przemyslaw Marczak wrote:
Hello,
On 12/30/2013 11:24 AM, Przemyslaw Marczak wrote:
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 p.marczak@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: Minkyu Kang mk7.kang@samsung.com
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,
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.
Ok, thank you,
On Mon, Dec 30, 2013 at 11:24:32AM +0100, Przemyslaw Marczak wrote:
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 p.marczak@samsung.com Cc: Lukasz Majewski l.majewski@samsung.com Cc: Minkyu Kang mk7.kang@samsung.com
Applied to u-boot/master, thanks!
participants (3)
-
Minkyu Kang -
Przemyslaw Marczak -
Tom Rini