From mboxrd@z Thu Jan  1 00:00:00 1970
From: Minkyu Kang <mk7.kang@samsung.com>
Date: Wed, 11 Dec 2013 11:27:54 +0900
Subject: [U-Boot] [PATCH] fuelgauge: max17042: fix i2c read issue which
 causes infinity loop.
In-Reply-To: <1386688742-17901-1-git-send-email-p.marczak@samsung.com>
References: <1386688742-17901-1-git-send-email-p.marczak@samsung.com>
Message-ID: <52A7CDAA.2030305@samsung.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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.