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

login
register
mail settings
Submitter Simon Glass
Date April 2, 2013, 12:04 a.m.
Message ID <1364861055-21670-4-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/232849/
State New
Delegated to: Minkyu Kang
Headers show

Comments

Simon Glass - April 2, 2013, 12:04 a.m.
From: Aaron Durbin <adurbin@chromium.org>

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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 drivers/power/power_i2c.c | 4 ++++
 1 file changed, 4 insertions(+)
Rajeshwari Birje - April 2, 2013, 5:18 a.m.
Hi Simon,

Just had one following comment

On Tue, Apr 2, 2013 at 5:34 AM, Simon Glass <sjg@chromium.org> wrote:
> From: Aaron Durbin <adurbin@chromium.org>
>
> 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>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  drivers/power/power_i2c.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
> index 3e5a784..ec9701e 100644
> --- a/drivers/power/power_i2c.c
> +++ b/drivers/power/power_i2c.c
> @@ -39,6 +39,8 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>         if (check_reg(p, reg))
>                 return -1;
>
> +       I2C_SET_BUS(p->bus);
> +

Do we need to set I2C bus for each register read and write?
>         switch (pmic_i2c_tx_num) {
>         case 3:
>                 if (p->sensor_byte_order == PMIC_SENSOR_BYTE_ORDER_BIG) {
> @@ -82,6 +84,8 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>         if (check_reg(p, reg))
>                 return -1;
>
> +       I2C_SET_BUS(p->bus);
> +
>         if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>                 return -1;
>
> --
> 1.8.1.3
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Simon Glass - May 11, 2013, 6:55 p.m.
Hi Rajeshwari,

On Mon, Apr 1, 2013 at 11:18 PM, Rajeshwari Birje
<rajeshwari.birje@gmail.com> wrote:
> Hi Simon,
>
> Just had one following comment
>
> On Tue, Apr 2, 2013 at 5:34 AM, Simon Glass <sjg@chromium.org> wrote:
>> From: Aaron Durbin <adurbin@chromium.org>
>>
>> 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>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> ---
>>  drivers/power/power_i2c.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
>> index 3e5a784..ec9701e 100644
>> --- a/drivers/power/power_i2c.c
>> +++ b/drivers/power/power_i2c.c
>> @@ -39,6 +39,8 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>>         if (check_reg(p, reg))
>>                 return -1;
>>
>> +       I2C_SET_BUS(p->bus);
>> +
>
> Do we need to set I2C bus for each register read and write?

It could potentially be done at a higher level, but it is tricky,
since the pmic is supposed to know its bus number. Nothing else should
need to know.

It's just setting a variable so does not take long. I think this sort
of thing will get cleaned up now that Heiko has posted the i2c
clean-up series (which I am going to take a look at).

Regards,
Simon


>>         switch (pmic_i2c_tx_num) {
>>         case 3:
>>                 if (p->sensor_byte_order == PMIC_SENSOR_BYTE_ORDER_BIG) {
>> @@ -82,6 +84,8 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>>         if (check_reg(p, reg))
>>                 return -1;
>>
>> +       I2C_SET_BUS(p->bus);
>> +
>>         if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>>                 return -1;
>>
>> --
>> 1.8.1.3
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
> --
> Regards,
> Rajeshwari Shinde

Patch

diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
index 3e5a784..ec9701e 100644
--- a/drivers/power/power_i2c.c
+++ b/drivers/power/power_i2c.c
@@ -39,6 +39,8 @@  int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
 	if (check_reg(p, reg))
 		return -1;
 
+	I2C_SET_BUS(p->bus);
+
 	switch (pmic_i2c_tx_num) {
 	case 3:
 		if (p->sensor_byte_order == PMIC_SENSOR_BYTE_ORDER_BIG) {
@@ -82,6 +84,8 @@  int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
 	if (check_reg(p, reg))
 		return -1;
 
+	I2C_SET_BUS(p->bus);
+
 	if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
 		return -1;