Patchwork [U-Boot,2/6] power: Explicitly select pmic device's bus

login
register
mail settings
Submitter Leela Krishna Amudala
Date Oct. 1, 2013, 2:32 p.m.
Message ID <1380637926-26242-3-git-send-email-l.krishna@samsung.com>
Download mbox | patch
Permalink /patch/279480/
State Changes Requested
Delegated to: Minkyu Kang
Headers show

Comments

Leela Krishna Amudala - Oct. 1, 2013, 2:32 p.m.
The current pmic i2c code assumes the current i2c bus is
the same as the pmic device's bus. There is nothing ensuring
that to be true. Therefore, select the proper bus before performing
a transaction.

Signed-off-by: Aaron Durbin <adurbin@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
Reviewed-by: Doug Anderson <dianders@google.com>
---
 drivers/power/power_i2c.c |   62 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 57 insertions(+), 5 deletions(-)
Łukasz Majewski - Oct. 2, 2013, 3:11 p.m.
Hi Leela,

> The current pmic i2c code assumes the current i2c bus is
> the same as the pmic device's bus. There is nothing ensuring
> that to be true. Therefore, select the proper bus before performing
> a transaction.
> 
> Signed-off-by: Aaron Durbin <adurbin@chromium.org>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> Reviewed-by: Doug Anderson <dianders@google.com>
> ---
>  drivers/power/power_i2c.c |   62
> +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57
> insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
> index 47c606f..c22e01f 100644
> --- a/drivers/power/power_i2c.c
> +++ b/drivers/power/power_i2c.c
> @@ -16,9 +16,45 @@
>  #include <i2c.h>
>  #include <compiler.h>
>  
> +static int pmic_select(struct pmic *p)
> +{
> +	int ret, old_bus;
> +
> +	old_bus = i2c_get_bus_num();
> +	if (old_bus != p->bus) {
> +		debug("%s: Select bus %d\n", __func__, p->bus);
> +		ret = i2c_set_bus_num(p->bus);
> +		if (ret) {
> +			debug("%s: Cannot select pmic %s, err %d\n",
> +			      __func__, p->name, ret);
> +			return -1;
> +		}
> +	}
> +
> +	return old_bus;
> +}
> +
> +static int pmic_deselect(int old_bus)
> +{
> +	int ret;
> +
> +	if (old_bus != i2c_get_bus_num()) {
> +		ret = i2c_set_bus_num(old_bus);
> +		debug("%s: Select bus %d\n", __func__, old_bus);
> +		if (ret) {
> +			debug("%s: Cannot restore i2c bus, err %d\n",
> +			      __func__, ret);
> +			return -1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>  {
>  	unsigned char buf[4] = { 0 };
> +	int ret, old_bus;
>  
>  	if (check_reg(p, reg))
>  		return -1;
> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32
> val) return -1;
>  	}
>  
> -	if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
> +	old_bus = pmic_select(p);
> +	if (old_bus < 0)
>  		return -1;
>  
> -	return 0;
> +	ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);

I'm wondering if setting the bus before each, single i2c write (when we
usually perform several writes to one device) will not be an overkill
(we search the ll_entry_count linker list each time to find max i2c
adapter names) ?

The i2c_set_bus_num() is now done at pmic_probe(), but this also
introduces overkill for "probing" each device when we want to read from
it.

As a side note - I would appreciate if you would add Stefano Babic and
me on the Cc (as we are listed at e.g. power_core.c).


> +	pmic_deselect(old_bus);
> +	return ret;
>  }
>  
>  int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>  {
>  	unsigned char buf[4] = { 0 };
>  	u32 ret_val = 0;
> +	int ret, old_bus;
>  
>  	if (check_reg(p, reg))
>  		return -1;
>  
> -	if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
> +	old_bus = pmic_select(p);
> +	if (old_bus < 0)
>  		return -1;
>  
> +	ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
> +	pmic_deselect(old_bus);
> +	if (ret)
> +		return ret;
> +
>  	switch (pmic_i2c_tx_num) {
>  	case 3:
>  		if (p->sensor_byte_order ==
> PMIC_SENSOR_BYTE_ORDER_BIG) @@ -117,9 +163,15 @@ int
> pmic_reg_update(struct pmic *p, int reg, uint regval) 
>  int pmic_probe(struct pmic *p)
>  {
> -	i2c_set_bus_num(p->bus);
> +	int ret, old_bus;
> +
> +	old_bus = pmic_select(p);
> +	if (old_bus < 0)
> +		return -1;
>  	debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
> -	if (i2c_probe(pmic_i2c_addr)) {
> +	ret = i2c_probe(pmic_i2c_addr);
> +	pmic_deselect(old_bus);
> +	if (ret) {
>  		printf("Can't find PMIC:%s\n", p->name);
>  		return -1;
>  	}
Leela Krishna Amudala - Oct. 3, 2013, 8:41 a.m.
Hello Lukasz,

Thanks for reviewing the patch.

On Wed, Oct 2, 2013 at 8:41 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Hi Leela,
>
>> The current pmic i2c code assumes the current i2c bus is
>> the same as the pmic device's bus. There is nothing ensuring
>> that to be true. Therefore, select the proper bus before performing
>> a transaction.
>>
>> Signed-off-by: Aaron Durbin <adurbin@chromium.org>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
>> Reviewed-by: Doug Anderson <dianders@google.com>
>> ---
>>  drivers/power/power_i2c.c |   62
>> +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57
>> insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
>> index 47c606f..c22e01f 100644
>> --- a/drivers/power/power_i2c.c
>> +++ b/drivers/power/power_i2c.c
>> @@ -16,9 +16,45 @@
>>  #include <i2c.h>
>>  #include <compiler.h>
>>
>> +static int pmic_select(struct pmic *p)
>> +{
>> +     int ret, old_bus;
>> +
>> +     old_bus = i2c_get_bus_num();
>> +     if (old_bus != p->bus) {
>> +             debug("%s: Select bus %d\n", __func__, p->bus);
>> +             ret = i2c_set_bus_num(p->bus);
>> +             if (ret) {
>> +                     debug("%s: Cannot select pmic %s, err %d\n",
>> +                           __func__, p->name, ret);
>> +                     return -1;
>> +             }
>> +     }
>> +
>> +     return old_bus;
>> +}
>> +
>> +static int pmic_deselect(int old_bus)
>> +{
>> +     int ret;
>> +
>> +     if (old_bus != i2c_get_bus_num()) {
>> +             ret = i2c_set_bus_num(old_bus);
>> +             debug("%s: Select bus %d\n", __func__, old_bus);
>> +             if (ret) {
>> +                     debug("%s: Cannot restore i2c bus, err %d\n",
>> +                           __func__, ret);
>> +                     return -1;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>  int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>>  {
>>       unsigned char buf[4] = { 0 };
>> +     int ret, old_bus;
>>
>>       if (check_reg(p, reg))
>>               return -1;
>> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32
>> val) return -1;
>>       }
>>
>> -     if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>> +     old_bus = pmic_select(p);
>> +     if (old_bus < 0)
>>               return -1;
>>
>> -     return 0;
>> +     ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
>
> I'm wondering if setting the bus before each, single i2c write (when we
> usually perform several writes to one device) will not be an overkill
> (we search the ll_entry_count linker list each time to find max i2c
> adapter names) ?
>

Here we are not setting the bus before each i2c write, If you look at the
pmic_select() function code it shows like
static int pmic_select(struct pmic *p)
{
    /......./
    old_bus = i2c_get_bus_num();
    if (old_bus != p->bus) {
    /...../
    }
    return old_bus;
}

Here we are trying to get the bus number and if it is equal to the bus
on which we are going to write we are returning immediately which is minimal.

> The i2c_set_bus_num() is now done at pmic_probe(), but this also
> introduces overkill for "probing" each device when we want to read from
> it.
>
> As a side note - I would appreciate if you would add Stefano Babic and
> me on the Cc (as we are listed at e.g. power_core.c).
>
>

Okay, will do it.

Best Wishes,
Leela Krishna

>> +     pmic_deselect(old_bus);
>> +     return ret;
>>  }
>>
>>  int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>>  {
>>       unsigned char buf[4] = { 0 };
>>       u32 ret_val = 0;
>> +     int ret, old_bus;
>>
>>       if (check_reg(p, reg))
>>               return -1;
>>
>> -     if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>> +     old_bus = pmic_select(p);
>> +     if (old_bus < 0)
>>               return -1;
>>
>> +     ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
>> +     pmic_deselect(old_bus);
>> +     if (ret)
>> +             return ret;
>> +
>>       switch (pmic_i2c_tx_num) {
>>       case 3:
>>               if (p->sensor_byte_order ==
>> PMIC_SENSOR_BYTE_ORDER_BIG) @@ -117,9 +163,15 @@ int
>> pmic_reg_update(struct pmic *p, int reg, uint regval)
>>  int pmic_probe(struct pmic *p)
>>  {
>> -     i2c_set_bus_num(p->bus);
>> +     int ret, old_bus;
>> +
>> +     old_bus = pmic_select(p);
>> +     if (old_bus < 0)
>> +             return -1;
>>       debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
>> -     if (i2c_probe(pmic_i2c_addr)) {
>> +     ret = i2c_probe(pmic_i2c_addr);
>> +     pmic_deselect(old_bus);
>> +     if (ret) {
>>               printf("Can't find PMIC:%s\n", p->name);
>>               return -1;
>>       }
>
>
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Łukasz Majewski - Oct. 3, 2013, 9:44 a.m.
Hi Leela,

> Hello Lukasz,
> 
> Thanks for reviewing the patch.
> 
> On Wed, Oct 2, 2013 at 8:41 PM, Lukasz Majewski
> <l.majewski@samsung.com> wrote:
> > Hi Leela,
> >
> >> The current pmic i2c code assumes the current i2c bus is
> >> the same as the pmic device's bus. There is nothing ensuring
> >> that to be true. Therefore, select the proper bus before performing
> >> a transaction.
> >>
> >> Signed-off-by: Aaron Durbin <adurbin@chromium.org>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
> >> Reviewed-by: Doug Anderson <dianders@google.com>
> >> ---
> >>  drivers/power/power_i2c.c |   62
> >> +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57
> >> insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
> >> index 47c606f..c22e01f 100644
> >> --- a/drivers/power/power_i2c.c
> >> +++ b/drivers/power/power_i2c.c
> >> @@ -16,9 +16,45 @@
> >>  #include <i2c.h>
> >>  #include <compiler.h>
> >>
> >> +static int pmic_select(struct pmic *p)
> >> +{
> >> +     int ret, old_bus;
> >> +
> >> +     old_bus = i2c_get_bus_num();
> >> +     if (old_bus != p->bus) {
> >> +             debug("%s: Select bus %d\n", __func__, p->bus);
> >> +             ret = i2c_set_bus_num(p->bus);
> >> +             if (ret) {
> >> +                     debug("%s: Cannot select pmic %s, err %d\n",
> >> +                           __func__, p->name, ret);
> >> +                     return -1;
> >> +             }
> >> +     }
> >> +
> >> +     return old_bus;
> >> +}
> >> +
> >> +static int pmic_deselect(int old_bus)
> >> +{
> >> +     int ret;
> >> +
> >> +     if (old_bus != i2c_get_bus_num()) {
> >> +             ret = i2c_set_bus_num(old_bus);
> >> +             debug("%s: Select bus %d\n", __func__, old_bus);
> >> +             if (ret) {
> >> +                     debug("%s: Cannot restore i2c bus, err %d\n",
> >> +                           __func__, ret);
> >> +                     return -1;
> >> +             }
> >> +     }
> >> +
> >> +     return 0;
> >> +}
> >> +
> >>  int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
> >>  {
> >>       unsigned char buf[4] = { 0 };
> >> +     int ret, old_bus;
> >>
> >>       if (check_reg(p, reg))
> >>               return -1;
> >> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32
> >> val) return -1;
> >>       }
> >>
> >> -     if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
> >> +     old_bus = pmic_select(p);
> >> +     if (old_bus < 0)
> >>               return -1;
> >>
> >> -     return 0;
> >> +     ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
> >
> > I'm wondering if setting the bus before each, single i2c write
> > (when we usually perform several writes to one device) will not be
> > an overkill (we search the ll_entry_count linker list each time to
> > find max i2c adapter names) ?
> >
> 
> Here we are not setting the bus before each i2c write, If you look at
> the pmic_select() function code it shows like
> static int pmic_select(struct pmic *p)
> {
>     /......./
>     old_bus = i2c_get_bus_num();
>     if (old_bus != p->bus) {
>     /...../
>     }
>     return old_bus;
> }
> 
> Here we are trying to get the bus number and if it is equal to the bus
> on which we are going to write we are returning immediately which is
> minimal.

Thanks for explanation.

> 
> > The i2c_set_bus_num() is now done at pmic_probe(), but this also
> > introduces overkill for "probing" each device when we want to read
> > from it.
> >
> > As a side note - I would appreciate if you would add Stefano Babic
> > and me on the Cc (as we are listed at e.g. power_core.c).
> >
> >
> 
> Okay, will do it.
> 
> Best Wishes,
> Leela Krishna
> 
> >> +     pmic_deselect(old_bus);
> >> +     return ret;
> >>  }
> >>
> >>  int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
> >>  {
> >>       unsigned char buf[4] = { 0 };
> >>       u32 ret_val = 0;
> >> +     int ret, old_bus;
> >>
> >>       if (check_reg(p, reg))
> >>               return -1;
> >>
> >> -     if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
> >> +     old_bus = pmic_select(p);
> >> +     if (old_bus < 0)
> >>               return -1;
> >>
> >> +     ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
> >> +     pmic_deselect(old_bus);
> >> +     if (ret)
> >> +             return ret;
> >> +
> >>       switch (pmic_i2c_tx_num) {
> >>       case 3:
> >>               if (p->sensor_byte_order ==
> >> PMIC_SENSOR_BYTE_ORDER_BIG) @@ -117,9 +163,15 @@ int
> >> pmic_reg_update(struct pmic *p, int reg, uint regval)
> >>  int pmic_probe(struct pmic *p)
> >>  {
> >> -     i2c_set_bus_num(p->bus);
> >> +     int ret, old_bus;
> >> +
> >> +     old_bus = pmic_select(p);
> >> +     if (old_bus < 0)
> >> +             return -1;
> >>       debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
> >> -     if (i2c_probe(pmic_i2c_addr)) {
> >> +     ret = i2c_probe(pmic_i2c_addr);
> >> +     pmic_deselect(old_bus);
> >> +     if (ret) {
> >>               printf("Can't find PMIC:%s\n", p->name);
> >>               return -1;
> >>       }
> >
> >
> >
> > --
> > Best regards,
> >
> > Lukasz Majewski
> >
> > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot@lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot

Patch

diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
index 47c606f..c22e01f 100644
--- a/drivers/power/power_i2c.c
+++ b/drivers/power/power_i2c.c
@@ -16,9 +16,45 @@ 
 #include <i2c.h>
 #include <compiler.h>
 
+static int pmic_select(struct pmic *p)
+{
+	int ret, old_bus;
+
+	old_bus = i2c_get_bus_num();
+	if (old_bus != p->bus) {
+		debug("%s: Select bus %d\n", __func__, p->bus);
+		ret = i2c_set_bus_num(p->bus);
+		if (ret) {
+			debug("%s: Cannot select pmic %s, err %d\n",
+			      __func__, p->name, ret);
+			return -1;
+		}
+	}
+
+	return old_bus;
+}
+
+static int pmic_deselect(int old_bus)
+{
+	int ret;
+
+	if (old_bus != i2c_get_bus_num()) {
+		ret = i2c_set_bus_num(old_bus);
+		debug("%s: Select bus %d\n", __func__, old_bus);
+		if (ret) {
+			debug("%s: Cannot restore i2c bus, err %d\n",
+			      __func__, ret);
+			return -1;
+		}
+	}
+
+	return 0;
+}
+
 int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
 {
 	unsigned char buf[4] = { 0 };
+	int ret, old_bus;
 
 	if (check_reg(p, reg))
 		return -1;
@@ -52,23 +88,33 @@  int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
 		return -1;
 	}
 
-	if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
+	old_bus = pmic_select(p);
+	if (old_bus < 0)
 		return -1;
 
-	return 0;
+	ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
+	pmic_deselect(old_bus);
+	return ret;
 }
 
 int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
 {
 	unsigned char buf[4] = { 0 };
 	u32 ret_val = 0;
+	int ret, old_bus;
 
 	if (check_reg(p, reg))
 		return -1;
 
-	if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
+	old_bus = pmic_select(p);
+	if (old_bus < 0)
 		return -1;
 
+	ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
+	pmic_deselect(old_bus);
+	if (ret)
+		return ret;
+
 	switch (pmic_i2c_tx_num) {
 	case 3:
 		if (p->sensor_byte_order == PMIC_SENSOR_BYTE_ORDER_BIG)
@@ -117,9 +163,15 @@  int pmic_reg_update(struct pmic *p, int reg, uint regval)
 
 int pmic_probe(struct pmic *p)
 {
-	i2c_set_bus_num(p->bus);
+	int ret, old_bus;
+
+	old_bus = pmic_select(p);
+	if (old_bus < 0)
+		return -1;
 	debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
-	if (i2c_probe(pmic_i2c_addr)) {
+	ret = i2c_probe(pmic_i2c_addr);
+	pmic_deselect(old_bus);
+	if (ret) {
 		printf("Can't find PMIC:%s\n", p->name);
 		return -1;
 	}