[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Heiko Schocher]

Signed-off-by: Heiko Schocher <hs@denx.de>
---
 drivers/i2c/fsl_i2c.c |   24 +++++++++++++++++++++++-
 1 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
index ce646fd..5242884 100644
--- a/drivers/i2c/fsl_i2c.c
+++ b/drivers/i2c/fsl_i2c.c
@@ -42,6 +42,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define CONFIG_SYS_SPD_BUS_NUM 0
 #endif
 static unsigned int i2c_bus_num __attribute__ ((section (".data"))) = CONFIG_SYS_SPD_BUS_NUM;
+static unsigned int i2c_bus_num_mux __attribute__ ((section ("data"))) = 0;

 static unsigned int i2c_bus_speed[2] = {CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SPEED};

@@ -369,6 +370,23 @@ i2c_probe(uchar chip)

 int i2c_set_bus_num(unsigned int bus)
 {
+#if defined(CONFIG_I2C_MUX)
+	if (bus < CONFIG_SYS_MAX_I2C_BUS) {
+		i2c_bus_num = bus;
+	} else {
+		int	ret;
+
+		ret = i2x_mux_select_mux(bus);
+		if (ret == 0) {
+			/* with CONFIG_I2C_MUX only I2C Controller 1
+			 * is usable
+			 */
+			i2c_bus_num = 0;
+			i2c_bus_num_mux = bus;
+		} else
+			return ret;
+	}
+#else
 #ifdef CONFIG_SYS_I2C2_OFFSET
 	if (bus > 1) {
 #else
@@ -378,7 +396,7 @@ int i2c_set_bus_num(unsigned int bus)
 	}

 	i2c_bus_num = bus;
-
+#endif
 	return 0;
 }

@@ -396,7 +414,11 @@ int i2c_set_bus_speed(unsigned int speed)

 unsigned int i2c_get_bus_num(void)
 {
+#if defined(CONFIG_I2C_MUX)
+	return i2c_bus_num_mux;
+#else
 	return i2c_bus_num;
+#endif
 }

 unsigned int i2c_get_bus_speed(void)
-- 
1.6.0.6

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

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Kim Phillips]

On Thu, 19 Feb 2009 17:24:09 +0100
Heiko Schocher <hs@denx.de> wrote:

> @@ -369,6 +370,23 @@ i2c_probe(uchar chip)
> 
>  int i2c_set_bus_num(unsigned int bus)
>  {
> +#if defined(CONFIG_I2C_MUX)
> +	if (bus < CONFIG_SYS_MAX_I2C_BUS) {
> +		i2c_bus_num = bus;
> +	} else {

[1]

> +		int	ret;
> +
> +		ret = i2x_mux_select_mux(bus);
> +		if (ret == 0) {
> +			/* with CONFIG_I2C_MUX only I2C Controller 1
> +			 * is usable
> +			 */
> +			i2c_bus_num = 0;
> +			i2c_bus_num_mux = bus;
> +		} else
> +			return ret;
> +	}

how about

ret = i2x_...
if (ret)
	return ret;
/* with...

> +#else
>  #ifdef CONFIG_SYS_I2C2_OFFSET
>  	if (bus > 1) {
>  #else
> @@ -378,7 +396,7 @@ int i2c_set_bus_num(unsigned int bus)
>  	}
> 
>  	i2c_bus_num = bus;
> -
> +#endif

reuse code and moved the ifdef up, then [1] would just be:

if (bus >= CONFIG_SYS_MAX_I2C_BUS) {

>  	return 0;
>  }
> 
> @@ -396,7 +414,11 @@ int i2c_set_bus_speed(unsigned int speed)
> 
>  unsigned int i2c_get_bus_num(void)
>  {
> +#if defined(CONFIG_I2C_MUX)
> +	return i2c_bus_num_mux;
> +#else
>  	return i2c_bus_num;
> +#endif
>  }

I don't get this mux variant - why aren't we reusing i2c_bus_num in the
mux case?

Kim

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Kim Phillips]

On Thu, 19 Feb 2009 17:24:09 +0100
Heiko Schocher <hs@denx.de> wrote:

> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---
>  drivers/i2c/fsl_i2c.c |   24 +++++++++++++++++++++++-
>  1 files changed, 23 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/fsl_i2c.c b/drivers/i2c/fsl_i2c.c
> index ce646fd..5242884 100644
> --- a/drivers/i2c/fsl_i2c.c
> +++ b/drivers/i2c/fsl_i2c.c
> @@ -42,6 +42,7 @@ DECLARE_GLOBAL_DATA_PTR;
>  #define CONFIG_SYS_SPD_BUS_NUM 0
>  #endif
>  static unsigned int i2c_bus_num __attribute__ ((section (".data"))) = CONFIG_SYS_SPD_BUS_NUM;
> +static unsigned int i2c_bus_num_mux __attribute__ ((section ("data"))) = 0;

also, this line gets this from the compiler:

fsl_i2c.c:45: warning: 'i2c_bus_num_mux' defined but not used

Kim

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Timur Tabi]

On Thu, Feb 19, 2009 at 10:24 AM, Heiko Schocher <hs@denx.de> wrote:
> Signed-off-by: Heiko Schocher <hs@denx.de>
> ---

Could you add a description that says what I2C mux support is?  That
would make it easier to review this patch.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Heiko Schocher]

Hello Kim,

Kim Phillips wrote:
> On Thu, 19 Feb 2009 17:24:09 +0100
> Heiko Schocher <hs@denx.de> wrote:
> 
>> @@ -369,6 +370,23 @@ i2c_probe(uchar chip)
>>
>>  int i2c_set_bus_num(unsigned int bus)
>>  {
>> +#if defined(CONFIG_I2C_MUX)
>> +	if (bus < CONFIG_SYS_MAX_I2C_BUS) {
>> +		i2c_bus_num = bus;
>> +	} else {
> 
> [1]
> 
>> +		int	ret;
>> +
>> +		ret = i2x_mux_select_mux(bus);
>> +		if (ret == 0) {
>> +			/* with CONFIG_I2C_MUX only I2C Controller 1
>> +			 * is usable
>> +			 */
>> +			i2c_bus_num = 0;
>> +			i2c_bus_num_mux = bus;
>> +		} else
>> +			return ret;
>> +	}
> 
> how about
> 
> ret = i2x_...
> if (ret)
> 	return ret;
> /* with...
> 
>> +#else
>>  #ifdef CONFIG_SYS_I2C2_OFFSET
>>  	if (bus > 1) {
>>  #else
>> @@ -378,7 +396,7 @@ int i2c_set_bus_num(unsigned int bus)
>>  	}
>>
>>  	i2c_bus_num = bus;
>> -
>> +#endif
> 
> reuse code and moved the ifdef up, then [1] would just be:

Ok.

> if (bus >= CONFIG_SYS_MAX_I2C_BUS) {
> 
>>  	return 0;
>>  }
>>
>> @@ -396,7 +414,11 @@ int i2c_set_bus_speed(unsigned int speed)
>>
>>  unsigned int i2c_get_bus_num(void)
>>  {
>> +#if defined(CONFIG_I2C_MUX)
>> +	return i2c_bus_num_mux;
>> +#else
>>  	return i2c_bus_num;
>> +#endif
>>  }
> 
> I don't get this mux variant - why aren't we reusing i2c_bus_num in the
> mux case?

Good question, have to think about it.

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

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Heiko Schocher]

Hello Kim,

Kim Phillips wrote:
> On Thu, 19 Feb 2009 17:24:09 +0100
> Heiko Schocher <hs@denx.de> wrote:
[...]
>>  unsigned int i2c_get_bus_num(void)
>>  {
>> +#if defined(CONFIG_I2C_MUX)
>> +	return i2c_bus_num_mux;
>> +#else
>>  	return i2c_bus_num;
>> +#endif
>>  }
> 
> I don't get this mux variant - why aren't we reusing i2c_bus_num in the
> mux case?

Because i2c_bus_num is used as an index which hardware i2c controller
is used (0 or 1). In CONFIG_I2C_MUX case, you have more than 2 i2c
busses -> i2c_bus_num would be greater than 1, so you must have a
variable, where you store which hardware adapter you use, and one
which stores on which i2c bus you are.

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

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Heiko Schocher]

Hello Timur,

Timur Tabi wrote:
> On Thu, Feb 19, 2009 at 10:24 AM, Heiko Schocher <hs@denx.de> wrote:
>> Signed-off-by: Heiko Schocher <hs@denx.de>
>> ---
> 
> Could you add a description that says what I2C mux support is?  That
> would make it easier to review this patch.

Ok, do this with the next version of this patches.

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

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Kim Phillips]

On Tue, 24 Feb 2009 08:53:33 +0100
Heiko Schocher <hs@denx.de> wrote:

> Hello Kim,
> 
> Kim Phillips wrote:
> > On Thu, 19 Feb 2009 17:24:09 +0100
> > Heiko Schocher <hs@denx.de> wrote:
> [...]
> >>  unsigned int i2c_get_bus_num(void)
> >>  {
> >> +#if defined(CONFIG_I2C_MUX)
> >> +	return i2c_bus_num_mux;
> >> +#else
> >>  	return i2c_bus_num;
> >> +#endif
> >>  }
> > 
> > I don't get this mux variant - why aren't we reusing i2c_bus_num in the
> > mux case?
> 
> Because i2c_bus_num is used as an index which hardware i2c controller
> is used (0 or 1). In CONFIG_I2C_MUX case, you have more than 2 i2c
> busses -> i2c_bus_num would be greater than 1, so you must have a
> variable, where you store which hardware adapter you use, and one
> which stores on which i2c bus you are.

so instead of naming it "i2c_bus_num_mux" it should be renamed
"i2c_adapter_num"?, or does i2c_get_bus_num() still imply that it will
return the /bus/ number?  Perhaps we should we have a separate function
altogether?

Kim

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Heiko Schocher]

Hello Kim,

Kim Phillips wrote:
> On Tue, 24 Feb 2009 08:53:33 +0100
> Heiko Schocher <hs@denx.de> wrote:
> 
>> Hello Kim,
>>
>> Kim Phillips wrote:
>>> On Thu, 19 Feb 2009 17:24:09 +0100
>>> Heiko Schocher <hs@denx.de> wrote:
>> [...]
>>>>  unsigned int i2c_get_bus_num(void)
>>>>  {
>>>> +#if defined(CONFIG_I2C_MUX)
>>>> +	return i2c_bus_num_mux;
>>>> +#else
>>>>  	return i2c_bus_num;
>>>> +#endif
>>>>  }
>>> I don't get this mux variant - why aren't we reusing i2c_bus_num in the
>>> mux case?
>> Because i2c_bus_num is used as an index which hardware i2c controller
>> is used (0 or 1). In CONFIG_I2C_MUX case, you have more than 2 i2c
>> busses -> i2c_bus_num would be greater than 1, so you must have a
>> variable, where you store which hardware adapter you use, and one
>> which stores on which i2c bus you are.
> 
> so instead of naming it "i2c_bus_num_mux" it should be renamed
> "i2c_adapter_num"?, or does i2c_get_bus_num() still imply that it will

No, i2c_adapter_num should be 0 or 1 for Controller 0 or 1, I think,
and i2c_bus_num_mux can be greater then 1.

If we would do a rename, we should rename "i2c_bus_num" to "i2c_adapter_num".
In case, we don;t use i2c mux, i2c_bus_num = i2c_adapter_num.
else i2c_bus_num >= i2c_adapter_num (=0 or 1)

> return the /bus/ number?  Perhaps we should we have a separate function

Yes, i2c_get_bus_num() returns the bus number.

> altogether?

We should rework this "i2c multibus" instead complete, so we can remove
all this instances from i2c_get_bus_num()/i2c_set_bus_num() in every
i2c driver ... such an attempt was in discussion, but unfortunately
failed ... but I hope I can retrigger it.

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

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Timur Tabi]

On Wed, Feb 25, 2009 at 2:00 AM, Heiko Schocher <hs@denx.de> wrote:

> We should rework this "i2c multibus" instead complete, so we can remove
> all this instances from i2c_get_bus_num()/i2c_set_bus_num() in every
> i2c driver ... such an attempt was in discussion, but unfortunately
> failed ... but I hope I can retrigger it.

I was willing to write a lot of the code to implement that change
(getting rid of i2c_xxx_bus_num), but aer Wolfgang shot it down, I
gave up.  If you do manage to turn the tide, let me know.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Kim Phillips]

On Wed, 25 Feb 2009 09:00:39 +0100
Heiko Schocher <hs@denx.de> wrote:

> > so instead of naming it "i2c_bus_num_mux" it should be renamed
> > "i2c_adapter_num"?, or does i2c_get_bus_num() still imply that it will
> 
> No, i2c_adapter_num should be 0 or 1 for Controller 0 or 1, I think,
> and i2c_bus_num_mux can be greater then 1.
> 
> If we would do a rename, we should rename "i2c_bus_num" to "i2c_adapter_num".
> In case, we don;t use i2c mux, i2c_bus_num = i2c_adapter_num.
> else i2c_bus_num >= i2c_adapter_num (=0 or 1)

sigh..that's not really that better either.

> > altogether?
> 
> We should rework this "i2c multibus" instead complete, so we can remove
> all this instances from i2c_get_bus_num()/i2c_set_bus_num() in every
> i2c driver ... such an attempt was in discussion, but unfortunately
> failed ... but I hope I can retrigger it.

ok - looking forward to it.

Thanks,

Kim

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Heiko Schocher]

Hello Timur,

Timur Tabi wrote:
> On Wed, Feb 25, 2009 at 2:00 AM, Heiko Schocher <hs@denx.de> wrote:
> 
>> We should rework this "i2c multibus" instead complete, so we can remove
>> all this instances from i2c_get_bus_num()/i2c_set_bus_num() in every
>> i2c driver ... such an attempt was in discussion, but unfortunately
>> failed ... but I hope I can retrigger it.
> 
> I was willing to write a lot of the code to implement that change
> (getting rid of i2c_xxx_bus_num), but aer Wolfgang shot it down, I
> gave up.  If you do manage to turn the tide, let me know.

I actually prepare a branch for u-boot-i2c.git based on the patches
from ksi, and another branch in this tree with suggestions I and
Wolfgang proposed. If I am ready with that, I hope this discussion
starts again ;-)

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

[U-Boot] [PATCH 4/9 v3] 83xx, i2c: add mux support for fsl_i2c

[Heiko Schocher]

Hello Kim,

Kim Phillips wrote:
> On Wed, 25 Feb 2009 09:00:39 +0100
> Heiko Schocher <hs@denx.de> wrote:
> 
>>> so instead of naming it "i2c_bus_num_mux" it should be renamed
>>> "i2c_adapter_num"?, or does i2c_get_bus_num() still imply that it will
>> No, i2c_adapter_num should be 0 or 1 for Controller 0 or 1, I think,
>> and i2c_bus_num_mux can be greater then 1.
>>
>> If we would do a rename, we should rename "i2c_bus_num" to "i2c_adapter_num".
>> In case, we don;t use i2c mux, i2c_bus_num = i2c_adapter_num.
>> else i2c_bus_num >= i2c_adapter_num (=0 or 1)
> 
> sigh..that's not really that better either.

yes, sorry.

>>> altogether?
>> We should rework this "i2c multibus" instead complete, so we can remove
>> all this instances from i2c_get_bus_num()/i2c_set_bus_num() in every
>> i2c driver ... such an attempt was in discussion, but unfortunately
>> failed ... but I hope I can retrigger it.
> 
> ok - looking forward to it.

Yep, I hope to get this running on beginning of march, and this approach,
hopefully, solves a lot of this problems.

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