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.