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

[Heiko Schocher]

Hello Lukasz,

Am 02.10.2013 17:11, schrieb Lukasz Majewski:
> 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 at chromium.org>
>> Signed-off-by: Simon Glass<sjg at chromium.org>
>> Signed-off-by: Leela Krishna Amudala<l.krishna at samsung.com>
>> Reviewed-by: Doug Anderson<dianders at 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) ?

Yes, maybe we could optimze this in drivers/i2c/i2c_core.c. It should be
enough to detect the max adapter once ... but it is not a "search"...
ll_entry_count() calculates the number ...

Looking in i2c_set_bus_num(), I think it can be optimized ...
lets speaking code:

diff --git a/drivers/i2c/i2c_core.c b/drivers/i2c/i2c_core.c
index d1072e8..170423a 100644
--- a/drivers/i2c/i2c_core.c
+++ b/drivers/i2c/i2c_core.c
@@ -278,20 +278,22 @@ unsigned int i2c_get_bus_num(void)
   */
  int i2c_set_bus_num(unsigned int bus)
  {
-       int max = ll_entry_count(struct i2c_adapter, i2c);
+       int max;
+
+       if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))
+               return 0;

-       if (I2C_ADAPTER(bus) >= max) {
-               printf("Error, wrong i2c adapter %d max %d possible\n",
-                      I2C_ADAPTER(bus), max);
-               return -2;
-       }
  #ifndef CONFIG_SYS_I2C_DIRECT_BUS
         if (bus >= CONFIG_SYS_NUM_I2C_BUSES)
                 return -1;
  #endif

-       if ((bus == I2C_BUS) && (I2C_ADAP->init_done > 0))
-               return 0;
+       max = ll_entry_count(struct i2c_adapter, i2c);
+       if (I2C_ADAPTER(bus) >= max) {
+               printf("Error, wrong i2c adapter %d max %d possible\n",
+                      I2C_ADAPTER(bus), max);
+               return -2;
+       }

  #ifndef CONFIG_SYS_I2C_DIRECT_BUS
         i2c_mux_disconnet_all();

So, first check, if we are on the correct bus, and return immediately!
What do you think?

Beside of that, pmic_select() does the check, if we are on the correct
bus too, and calls i2c_set_bus_num() only, if not ... so this is here
no problem ... but exactly I want to get rid of this code as it is in
pmic_select() someday, when all i2c drivers converted to the new i2c
framework. i2c_set_bus_num() should go static then in drivers/i2c/i2c_core.c
and i2c_read/write/... become a new "int bus" parameter ... but this
will be a big api change ... but will prevent exactly such code
all over the u-boot code ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany