| Message ID | 1386688742-17901-1-git-send-email-p.marczak@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
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.
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);
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(-)