tegra: i2c: Add function to know about current bus

drivers/i2c/i2c_core.c line 14

Quoted 10 lines · Simon Glass

(just for illustration, please don't merge)

This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra i2c driver to support this.

drivers/i2c/i2c_core.c +3 -0

 
+#ifdef CONFIG_TEGRA_I2C
+extern struct i2c_adapter tegra_i2c_adap[];
+#endif

I'm not sure why that's needed if the config files have to put the adpater list into a #define:

Quoted 1 line

include/configs/seaboard.h +0 -0

include/configs/seaboard.h line 66

Quoted 3 lines

include/configs/seaboard.h +3 -0

+#define CONFIG_SYS_I2C
+#define CONFIG_SYS_I2C_ADAPTERS	{&tegra_i2c_adap[0]}
+#define CONFIG_SYS_NUM_I2C_ADAPTERS	TEGRA_I2C_NUM_CONTROLLERS

But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init functions (which presumably would be called from board code or as a result of DT parsing) dynamically register themselves?

Aside from that, this looks OK to me at a quick glance.

Hello Stephen,

Quoted 18 lines · Stephen Warren

drivers/i2c/i2c_core.c line 14

Quoted 11 lines · Simon Glass

(just for illustration, please don't merge)

This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra
i2c driver to support this.

drivers/i2c/i2c_core.c +3 -0

 
+#ifdef CONFIG_TEGRA_I2C
+extern struct i2c_adapter tegra_i2c_adap[];
+#endif

I'm not sure why that's needed if the config files have to put the adpater list into a #define:

Quoted 1 line

include/configs/seaboard.h +0 -0

Quoted 7 lines

include/configs/seaboard.h line 66

Quoted 3 lines

include/configs/seaboard.h +3 -0

+#define CONFIG_SYS_I2C
+#define CONFIG_SYS_I2C_ADAPTERS    {&tegra_i2c_adap[0]}
+#define CONFIG_SYS_NUM_I2C_ADAPTERS    TEGRA_I2C_NUM_CONTROLLERS

But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init functions (which presumably would be called from board code or as a result of DT parsing) dynamically register themselves?

This would not work, if we are running for example from flash on powerpc before relocation. There some boards need to read RAM infos from an SPD Eeprom through i2c ... so we must have very early i2c bus ready and usable ...

It maybe a good plus to add i2c busses/adapters dynamically after relocation is done and we have enough RAM ... I tried such an approach here:

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commit;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4

but this is an old state (only rebased a long time), but it is maybe a good base for doing such an feature if it is needed ...

Quoted 40 lines · Heiko Schocher

Hello Stephen,

Quoted 18 lines · Stephen Warren

drivers/i2c/i2c_core.c line 14

Quoted 11 lines · Simon Glass

(just for illustration, please don't merge)

This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra
i2c driver to support this.

drivers/i2c/i2c_core.c +3 -0

 
+#ifdef CONFIG_TEGRA_I2C
+extern struct i2c_adapter tegra_i2c_adap[];
+#endif

I'm not sure why that's needed if the config files have to put the adpater list into a #define:

Quoted 1 line

include/configs/seaboard.h +0 -0

Quoted 7 lines

include/configs/seaboard.h line 66

Quoted 3 lines

include/configs/seaboard.h +3 -0

+#define CONFIG_SYS_I2C
+#define CONFIG_SYS_I2C_ADAPTERS    {&tegra_i2c_adap[0]}
+#define CONFIG_SYS_NUM_I2C_ADAPTERS    TEGRA_I2C_NUM_CONTROLLERS

But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init functions (which presumably would be called from board code or as a result of DT parsing) dynamically register themselves?

This would not work, if we are running for example from flash on powerpc before relocation. There some boards need to read RAM infos from an SPD Eeprom through i2c ... so we must have very early i2c bus ready and usable ...

It maybe a good plus to add i2c busses/adapters dynamically after relocation is done and we have enough RAM ... I tried such an approach here:

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commit;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4

Quoted 3 lines

but this is an old state (only rebased a long time), but it is maybe a good base for doing such an feature if it is needed ...

I mainly ask because Simon is pushing to have Tegra's U-Boot completely driven by device tree. If we need to hard-code the list of enabled I2C adapters in the U-Boot config file, and don't also support dynamically added I2C drivers, then that will be incompatible with device tree.

Hi Stephen,

Quoted 5 lines · Stephen Warren

Quoted 4 lines · Heiko Schocher

Hello Stephen,

Quoted 1 line · Stephen Warren

On 10/30/2012 11:28 AM, Simon Glass wrote:

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

include/configs/seaboard.h +0 -0

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

include/configs/seaboard.h line 66

Quoted 3 lines

include/configs/seaboard.h +3 -0

+#define CONFIG_SYS_I2C
+#define CONFIG_SYS_I2C_ADAPTERS    {&tegra_i2c_adap[0]}
+#define CONFIG_SYS_NUM_I2C_ADAPTERS    TEGRA_I2C_NUM_CONTROLLERS

But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init functions (which presumably would be called from board code or as a result of DT parsing) dynamically register themselves?

Quoted 4 lines

I mainly ask because Simon is pushing to have Tegra's U-Boot completely driven by device tree. If we need to hard-code the list of enabled I2C adapters in the U-Boot config file, and don't also support dynamically added I2C drivers, then that will be incompatible with device tree.

Mostly, although with the serial console (which had a similar problem) we just decoded the information onto the stack as needed. It was inefficient, but worked fine for a small number of operations. We might be able to do better with pre-relocation malloc() soon.

Quoted 8 lines · Simon Glass

Hi Stephen,

Quoted 5 lines · Stephen Warren

Quoted 4 lines · Heiko Schocher

Hello Stephen,

Quoted 1 line · Stephen Warren

On 10/30/2012 11:28 AM, Simon Glass wrote:

...

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

include/configs/seaboard.h +0 -0

...

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

include/configs/seaboard.h line 66

Quoted 3 lines

include/configs/seaboard.h +3 -0

+#define CONFIG_SYS_I2C
+#define CONFIG_SYS_I2C_ADAPTERS    {&tegra_i2c_adap[0]}
+#define CONFIG_SYS_NUM_I2C_ADAPTERS    TEGRA_I2C_NUM_CONTROLLERS

But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init functions (which presumably would be called from board code or as a result of DT parsing) dynamically register themselves?

...

Quoted 9 lines

Quoted 4 lines

I mainly ask because Simon is pushing to have Tegra's U-Boot completely driven by device tree. If we need to hard-code the list of enabled I2C adapters in the U-Boot config file, and don't also support dynamically added I2C drivers, then that will be incompatible with device tree.

Mostly, although with the serial console (which had a similar problem) we just decoded the information onto the stack as needed. It was inefficient, but worked fine for a small number of operations. We might be able to do better with pre-relocation malloc() soon.

It's more complex than that.

If we use DT, we must be able to:

a) Configure exactly which I2C instances the board uses from DT.
b) Provide configuration (e.g. max clock rate) for those instances from DT.

If all in-use I2C adapters must be specified statically in CONFIG_SYS_I2C_ADAPTERS, then since a board's DT could enable any arbitrary subset of Tegra's I2C adapters, then we must always set CONFIG_SYS_I2C_ADAPTERS to the list of all Tegra's I2C adapters. If we put some subset into CONFIG_SYS_I2C_ADAPTERS, then we're pre-defining the maximal set of I2C adapters that a board can enable, which means DT isn't specifying that, but rather the board config file is, and hence it's pointless to even use DT for this purpose.

Now, most boards won't use all I2C adapters, so presumably the Tegra I2C init routine will look for status="disabled" (or the inverse) in DT, and only initialize if the DT node for the adapter is present and enabled.

However, this will leave entries in CONFIG_SYS_I2C_ADAPTERS that are not enabled, which will presumably influence the I2C bus numbering in the

U-Boot shell; there will be I2C busses that exist but cannot be used. Is this what we want? Perhaps it is in fact a good idea to always make the

U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps when some ports aren't enabled? That would be a departure from the way USB is handled today though.

Hello Stephen,

Quoted 39 lines · Stephen Warren

Quoted 9 lines · Simon Glass

Hi Stephen,

Quoted 5 lines · Stephen Warren

Quoted 4 lines · Heiko Schocher

Hello Stephen,

Quoted 1 line · Stephen Warren

On 10/30/2012 11:28 AM, Simon Glass wrote:

...

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

include/configs/seaboard.h +0 -0

...

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

include/configs/seaboard.h

Quoted 3 lines

include/configs/seaboard.h +3 -0

+#define CONFIG_SYS_I2C
+#define CONFIG_SYS_I2C_ADAPTERS    {&tegra_i2c_adap[0]}
+#define CONFIG_SYS_NUM_I2C_ADAPTERS    TEGRA_I2C_NUM_CONTROLLERS

But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init functions (which presumably would be called from board code or as a result of DT parsing) dynamically register themselves?

...

Quoted 9 lines

Quoted 4 lines

I mainly ask because Simon is pushing to have Tegra's U-Boot completely driven by device tree. If we need to hard-code the list of enabled I2C adapters in the U-Boot config file, and don't also support dynamically added I2C drivers, then that will be incompatible with device tree.

Mostly, although with the serial console (which had a similar problem) we just decoded the information onto the stack as needed. It was inefficient, but worked fine for a small number of operations. We might be able to do better with pre-relocation malloc() soon.

It's more complex than that.

If we use DT, we must be able to:

a) Configure exactly which I2C instances the board uses from DT.
b) Provide configuration (e.g. max clock rate) for those instances from DT.

Quoted 8 lines

If all in-use I2C adapters must be specified statically in CONFIG_SYS_I2C_ADAPTERS, then since a board's DT could enable any arbitrary subset of Tegra's I2C adapters, then we must always set CONFIG_SYS_I2C_ADAPTERS to the list of all Tegra's I2C adapters. If we put some subset into CONFIG_SYS_I2C_ADAPTERS, then we're pre-defining the maximal set of I2C adapters that a board can enable, which means DT isn't specifying that, but rather the board config file is, and hence it's pointless to even use DT for this purpose.

My current approach needs static specific adapters, yes. But I see not the problem, if we define all tegra adapters per default and ...

Quoted 3 lines

Now, most boards won't use all I2C adapters, so presumably the Tegra I2C init routine will look for status="disabled" (or the inverse) in DT, and only initialize if the DT node for the adapter is present and enabled.

... and it should here be possible to add the used "i2c busses"! dynamically from the info in the DT, or?

We use in U-Boot not direct the i2c adapters, instead i2c busses ... so if we define all 4 tegra i2c adapters per default, but using on one board only adapter 1 and 3 we have two i2c busses:
0 (= adapter 1) and
1 (= adapter 3) ...
no gaps between ...)

Quoted 2 lines

However, this will leave entries in CONFIG_SYS_I2C_ADAPTERS that are not enabled, which will presumably influence the I2C bus numbering in the

No, why? You can add all i2c adapters to the CONFIG_SYS_I2C_ADAPTERS define, without really use them in CONFIG_SYS_I2C_BUSSES. And it

should be possible (not yet coded, but tried in an older version) to add i2c busses after relocation, or while interpret DT ...

something like I did in
http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13

function:
int i2c_add_one_bus(char *buf)

-> while interpreting DT i2c info for above board will result in calling:

   i2c_add_one_bus("tegra-i2c-1");
   i2c_add_one_bus("tegra-i2c-3");

and results in two new i2c busses 0 and 1 ...
Maybe this is a way to go?

Quoted 2 lines

U-Boot shell; there will be I2C busses that exist but cannot be used. Is this what we want? Perhaps it is in fact a good idea to always make the

Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have some i2c adapters which are defined but (maybe) not used ...

Quoted 3 lines

U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps when some ports aren't enabled? That would be a departure from the way USB is handled today though.

Hmm.. but is this possible? If we have a board with 2 (or more) different i2c adapters (which is now possible with the new framework), for example 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra adapters are the first 4 i2c busses, so we cannot longer say the two soft i2c adapters are starting from 0 too (and vice versa) ...

Quoted 54 lines · Heiko Schocher

Hello Stephen,

Quoted 39 lines · Stephen Warren

Quoted 9 lines · Simon Glass

Hi Stephen,

Quoted 5 lines · Stephen Warren

Quoted 4 lines · Heiko Schocher

Hello Stephen,

Quoted 1 line · Stephen Warren

On 10/30/2012 11:28 AM, Simon Glass wrote:

...

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

include/configs/seaboard.h +0 -0

...

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

include/configs/seaboard.h

Quoted 3 lines

include/configs/seaboard.h +3 -0

+#define CONFIG_SYS_I2C
+#define CONFIG_SYS_I2C_ADAPTERS    {&tegra_i2c_adap[0]}
+#define CONFIG_SYS_NUM_I2C_ADAPTERS    TEGRA_I2C_NUM_CONTROLLERS

But, why is CONFIG_SYS_I2C_ADAPTERS needed; can't the adapter init functions (which presumably would be called from board code or as a result of DT parsing) dynamically register themselves?

...

Quoted 9 lines

Quoted 4 lines

I mainly ask because Simon is pushing to have Tegra's U-Boot completely driven by device tree. If we need to hard-code the list of enabled I2C adapters in the U-Boot config file, and don't also support dynamically added I2C drivers, then that will be incompatible with device tree.

Mostly, although with the serial console (which had a similar problem) we just decoded the information onto the stack as needed. It was inefficient, but worked fine for a small number of operations. We might be able to do better with pre-relocation malloc() soon.

It's more complex than that.

If we use DT, we must be able to:

a) Configure exactly which I2C instances the board uses from DT.
b) Provide configuration (e.g. max clock rate) for those instances from DT.

Quoted 8 lines

If all in-use I2C adapters must be specified statically in CONFIG_SYS_I2C_ADAPTERS, then since a board's DT could enable any arbitrary subset of Tegra's I2C adapters, then we must always set CONFIG_SYS_I2C_ADAPTERS to the list of all Tegra's I2C adapters. If we put some subset into CONFIG_SYS_I2C_ADAPTERS, then we're pre-defining the maximal set of I2C adapters that a board can enable, which means DT isn't specifying that, but rather the board config file is, and hence it's pointless to even use DT for this purpose.

My current approach needs static specific adapters, yes. But I see not the problem, if we define all tegra adapters per default and ...

Quoted 6 lines

Quoted 3 lines

Now, most boards won't use all I2C adapters, so presumably the Tegra I2C init routine will look for status="disabled" (or the inverse) in DT, and only initialize if the DT node for the adapter is present and enabled.

... and it should here be possible to add the used "i2c busses"! dynamically from the info in the DT, or?

Does the I2C adapter registration function call an I2C core function to dynamically register the actual buses that exist on the system? If so, then everything seems fine.

However, your later responses re: CONFIG_SYS_I2C_BUSSES then would confuse me...

Quoted 7 lines

We use in U-Boot not direct the i2c adapters, instead i2c busses ... so if we define all 4 tegra i2c adapters per default, but using on one board only adapter 1 and 3 we have two i2c busses:
0 (= adapter 1) and
1 (= adapter 3) ...
no gaps between ...)

Quoted 5 lines

Quoted 2 lines

However, this will leave entries in CONFIG_SYS_I2C_ADAPTERS that are not enabled, which will presumably influence the I2C bus numbering in the

No, why? You can add all i2c adapters to the CONFIG_SYS_I2C_ADAPTERS define, without really use them in CONFIG_SYS_I2C_BUSSES. And it

Oh, so CONFIG_SYS_I2C_ADAPTERS defines which adapters get initialized, and CONFIG_SYS_I2C_BUSSES statically defines which I2C buses exist, so CONFIG_SYS_I2C_BUSSES could end up not defining a bus that uses a particular adapter, or could end up defining multiple buses that use a particular bus (in the presence of a mux). That all seems fine, but...

However, CONFIG_SYS_I2C_BUSSES sounds like a config variable defined at compile time, not something dynamic. If that's true, then I'll just re-state my previous comments but say "CONFIG_SYS_I2C_BUSSES" anywhere I said "CONFIG_SYS_I2C_ADAPTERS"...

Quoted 5 lines

should be possible (not yet coded, but tried in an older version) to add i2c busses after relocation, or while interpret DT ...

something like I did in
http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13

I guess my question is: Why even have a CONFIG_SYS_I2C_ADAPTERS or CONFIG_SYS_I2C_BUSSES variable at all? Surely if we intend to make this dynamic in the long run we should just make it dynamic from the start as part of the API rework. If we don't, we'll just have to do another pass over all the drivers converting them to dynamic registration later anyway.

I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each driver registers an arbitrary number of adapters and/or buses during its initialization.

We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; just have each driver define a structure that gets put into a special linker section, so that the I2C core can iterate over all linked drivers at boot time and call their initialization functions.

Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically.

BTW, isn't buses spelled "buses" not "busses"? Thunderbird's spell-checker certainly thinks so!

Quoted 3 lines

function:
int i2c_add_one_bus(char *buf)

Quoted 6 lines

-> while interpreting DT i2c info for above board will result in calling:

   i2c_add_one_bus("tegra-i2c-1");
   i2c_add_one_bus("tegra-i2c-3");

and results in two new i2c busses 0 and 1 ...
Maybe this is a way to go?

Yes, that sounds like the right kind of direction.

Quoted 11 lines

Quoted 3 lines

U-Boot shell; there will be I2C busses that exist but cannot be used. Is this what we want? Perhaps it is in fact a good idea to always make the

Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have some i2c adapters which are defined but (maybe) not used ...

Quoted 3 lines

U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps when some ports aren't enabled? That would be a departure from the way USB is handled today though.

Quoted 5 lines

Hmm.. but is this possible? If we have a board with 2 (or more) different i2c adapters (which is now possible with the new framework), for example 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra adapters are the first 4 i2c busses, so we cannot longer say the two soft i2c adapters are starting from 0 too (and vice versa) ...

I was talking about the I2C HW in the SoC itself. There isn't any concept of an expected I2C bus/adapter ID for anything outside the SoC.

Hi,

Quoted 8 lines · Stephen Warren

Quoted 7 lines · Heiko Schocher

Hello Stephen,

Quoted 4 lines · Stephen Warren

Quoted 3 lines · Simon Glass

Hi Stephen,

On Wed, Oct 31, 2012 at 8:41 AM, Stephen

Quoted 34 lines

Quoted 5 lines

should be possible (not yet coded, but tried in an older version) to add i2c busses after relocation, or while interpret DT ...

something like I did in
http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13

I guess my question is: Why even have a CONFIG_SYS_I2C_ADAPTERS or CONFIG_SYS_I2C_BUSSES variable at all? Surely if we intend to make this dynamic in the long run we should just make it dynamic from the start as part of the API rework. If we don't, we'll just have to do another pass over all the drivers converting them to dynamic registration later anyway.

I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each driver registers an arbitrary number of adapters and/or buses during its initialization.

We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; just have each driver define a structure that gets put into a special linker section, so that the I2C core can iterate over all linked drivers at boot time and call their initialization functions.

Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically.

BTW, isn't buses spelled "buses" not "busses"? Thunderbird's spell-checker certainly thinks so!

Quoted 7 lines

function:
int i2c_add_one_bus(char *buf)

-> while interpreting DT i2c info for above board will result in calling:

    i2c_add_one_bus("tegra-i2c-1");
    i2c_add_one_bus("tegra-i2c-3");

Quoted 4 lines

Quoted 2 lines

and results in two new i2c busses 0 and 1 ...
Maybe this is a way to go?

Yes, that sounds like the right kind of direction.

Well it certainly is useful to have dynamic I2C, but it might be tricky until we have a pre-reloc malloc()?

We don't need to do everything at once. It would be nice to get the adapter parameter in there though, to avoid two steps of driver

change. But I would encourage you Heiko to get this tidied up a bit and merged, since it is already a big step forward.

Quoted 16 lines

Quoted 15 lines

Quoted 2 lines

U-Boot shell; there will be I2C busses that exist but cannot be used. Is this what we want? Perhaps it is in fact a good idea to always make the

Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have some i2c adapters which are defined but (maybe) not used ...

Quoted 3 lines

U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps when some ports aren't enabled? That would be a departure from the way USB is handled today though.

Hmm.. but is this possible? If we have a board with 2 (or more) different i2c adapters (which is now possible with the new framework), for example 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra adapters are the first 4 i2c busses, so we cannot longer say the two soft i2c adapters are starting from 0 too (and vice versa) ...

Quoted 2 lines

I was talking about the I2C HW in the SoC itself. There isn't any concept of an expected I2C bus/adapter ID for anything outside the SoC.

Well the device tree should take care of this (in the alias section). The numbering of i2c ports should be done that way. See Tegra USB for how this works, as an example (board_usb_init() calls fdtdec_find_aliases_for_id(), then add_port() to add each of the ports it finds).

Hello Simon,

Quoted 17 lines · Simon Glass

Hi,

Quoted 12 lines · Stephen Warren

Quoted 10 lines · Heiko Schocher

Hello Stephen,

Quoted 6 lines · Stephen Warren

Quoted 4 lines · Simon Glass

Hi Stephen,

On Wed, Oct 31, 2012 at 8:41 AM, Stephen

[...]

Quoted 42 lines

Quoted 42 lines

Quoted 6 lines

should be possible (not yet coded, but tried in an older version) to add i2c busses after relocation, or while interpret DT ...

something like I did in

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13

I guess my question is: Why even have a CONFIG_SYS_I2C_ADAPTERS or CONFIG_SYS_I2C_BUSSES variable at all? Surely if we intend to make this dynamic in the long run we should just make it dynamic from the start as part of the API rework. If we don't, we'll just have to do another pass over all the drivers converting them to dynamic registration later anyway.

I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each driver registers an arbitrary number of adapters and/or buses during its initialization.

We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; just have each driver define a structure that gets put into a special linker section, so that the I2C core can iterate over all linked drivers at boot time and call their initialization functions.

Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically.

BTW, isn't buses spelled "buses" not "busses"? Thunderbird's spell-checker certainly thinks so!

Quoted 10 lines

function:
int i2c_add_one_bus(char *buf)

-> while interpreting DT i2c info for above board will result in calling:

    i2c_add_one_bus("tegra-i2c-1");
    i2c_add_one_bus("tegra-i2c-3");

and results in two new i2c busses 0 and 1 ...
Maybe this is a way to go?

Quoted 5 lines

Quoted 1 line

Yes, that sounds like the right kind of direction.

Well it certainly is useful to have dynamic I2C, but it might be tricky until we have a pre-reloc malloc()?

Yep ... not only this, we maybe have not enough "RAM" for it, because RAM is not initialized and we are using some SRAM, cache, ... for stack ...

(/dreaming on
Sometimes I think, we should switch completely to boot basically with SPL for all boards (in SPL we do not need mutliadapter/mutlibus support), so we could start U-Boot directly relocated in RAM from SPL with an arch independent board.c and all RAM availiable, which would solve some problems ...
/dreaming off)

Quoted 2 lines

We don't need to do everything at once. It would be nice to get the adapter parameter in there though, to avoid two steps of driver

did this, see:

http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2_20121030

Quoted 2 lines

change. But I would encourage you Heiko to get this tidied up a bit and merged, since it is already a big step forward.

Ok, we should vote here, if we

a) go ahead with this patchset
b) wait for DM is ready
c) rework this patchserie here completely new (dynamic registration) as stephen suggested

Quoted 28 lines

Quoted 24 lines

Quoted 16 lines

Quoted 5 lines

U-Boot shell; there will be I2C busses that exist but cannot be used. Is
this what we want? Perhaps it is in fact a good idea to always make the

Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have some i2c adapters which are defined but (maybe) not used ...

Quoted 4 lines

U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps
when some ports aren't enabled? That would be a departure from the way USB is handled today though.

Quoted 6 lines

Hmm.. but is this possible? If we have a board with 2 (or more) different
i2c adapters (which is now possible with the new framework), for example 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra adapters are the first 4 i2c busses, so we cannot longer say the two soft i2c adapters are starting from 0 too (and vice versa) ...

Quoted 2 lines

I was talking about the I2C HW in the SoC itself. There isn't any concept of an expected I2C bus/adapter ID for anything outside the SoC.

Quoted 5 lines

Well the device tree should take care of this (in the alias section). The numbering of i2c ports should be done that way. See Tegra USB for how this works, as an example (board_usb_init() calls fdtdec_find_aliases_for_id(), then add_port() to add each of the ports it finds).

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

Quoted 9 lines · Heiko Schocher

Hello Stephen,

Quoted 6 lines · Stephen Warren

Quoted 5 lines · Simon Glass

Hi Stephen,

Quoted 1 line · Stephen Warren

On 10/31/2012 12:00 AM, Heiko Schocher wrote:

Quoted 12 lines

Quoted 8 lines

If all in-use I2C adapters must be specified statically in CONFIG_SYS_I2C_ADAPTERS, then since a board's DT could enable any arbitrary subset of Tegra's I2C adapters, then we must always set CONFIG_SYS_I2C_ADAPTERS to the list of all Tegra's I2C adapters. If we put some subset into CONFIG_SYS_I2C_ADAPTERS, then we're pre-defining the maximal set of I2C adapters that a board can enable, which means DT isn't specifying that, but rather the board config file is, and hence it's pointless to even use DT for this purpose.

My current approach needs static specific adapters, yes. But I see not the problem, if we define all tegra adapters per default and ...

Quoted 6 lines

Quoted 3 lines

Now, most boards won't use all I2C adapters, so presumably the Tegra I2C init routine will look for status="disabled" (or the inverse) in DT, and only initialize if the DT node for the adapter is present and enabled.

... and it should here be possible to add the used "i2c busses"! dynamically from the info in the DT, or?

Does the I2C adapter registration function call an I2C core function to dynamically register the actual buses that exist on the system? If so, then everything seems fine.

However, your later responses re: CONFIG_SYS_I2C_BUSSES then would confuse me...

Quoted 7 lines

We use in U-Boot not direct the i2c adapters, instead i2c busses ... so if we define all 4 tegra i2c adapters per default, but using on one board only adapter 1 and 3 we have two i2c busses:
0 (= adapter 1) and
1 (= adapter 3) ...
no gaps between ...)

Quoted 5 lines

Quoted 2 lines

However, this will leave entries in CONFIG_SYS_I2C_ADAPTERS that are not enabled, which will presumably influence the I2C bus numbering in the

No, why? You can add all i2c adapters to the CONFIG_SYS_I2C_ADAPTERS define, without really use them in CONFIG_SYS_I2C_BUSSES. And it

Oh, so CONFIG_SYS_I2C_ADAPTERS defines which adapters get initialized,

and CONFIG_SYS_I2C_BUSSES statically defines which I2C buses exist, so CONFIG_SYS_I2C_BUSSES could end up not defining a bus that uses a particular adapter, or could end up defining multiple buses that use a particular bus (in the presence of a mux). That all seems fine, but...

However, CONFIG_SYS_I2C_BUSSES sounds like a config variable defined at compile time, not something dynamic. If that's true, then I'll just re-state my previous comments but say "CONFIG_SYS_I2C_BUSSES" anywhere I said "CONFIG_SYS_I2C_ADAPTERS"...

Quoted 5 lines

should be possible (not yet coded, but tried in an older version) to add i2c busses after relocation, or while interpret DT ...

something like I did in
http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=commitdiff;h=ccb680c8cf9002232bc459e4003e3b47db5e1bf4#patch13

I guess my question is: Why even have a CONFIG_SYS_I2C_ADAPTERS or CONFIG_SYS_I2C_BUSSES variable at all? Surely if we intend to make this dynamic in the long run we should just make it dynamic from the start as part of the API rework. If we don't, we'll just have to do another pass over all the drivers converting them to dynamic registration later anyway.

I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each driver registers an arbitrary number of adapters and/or buses during its initialization.

We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; just have each driver define a structure that gets put into a special linker section, so that the I2C core can iterate over all linked drivers at boot time and call their initialization functions.

Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically.

BTW, isn't buses spelled "buses" not "busses"? Thunderbird's spell-checker certainly thinks so!

Quoted 3 lines

function:
int i2c_add_one_bus(char *buf)

Quoted 6 lines

-> while interpreting DT i2c info for above board will result in calling:

    i2c_add_one_bus("tegra-i2c-1");
    i2c_add_one_bus("tegra-i2c-3");

and results in two new i2c busses 0 and 1 ...
Maybe this is a way to go?

Yes, that sounds like the right kind of direction.

Quoted 10 lines

Quoted 2 lines

U-Boot shell; there will be I2C busses that exist but cannot be used. Is this what we want? Perhaps it is in fact a good idea to always make the

Now, this is wrong! You mix here "i2c bus" with "i2c adapter"! We have some i2c adapters which are defined but (maybe) not used ...

Quoted 3 lines

U-Boot shell's I2C bus IDs be the same as the HW's, and hence leave gaps when some ports aren't enabled? That would be a departure from the way USB is handled today though.

Quoted 5 lines

Hmm.. but is this possible? If we have a board with 2 (or more) different i2c adapters (which is now possible with the new framework), for example 2 i2c soft adapters + 4 tegra i2c adapter ... if we say the i2c tegra adapters are the first 4 i2c busses, so we cannot longer say the two soft i2c adapters are starting from 0 too (and vice versa) ...

I was talking about the I2C HW in the SoC itself. There isn't any concept of an expected I2C bus/adapter ID for anything outside the SoC.

On 01.11.2012 18:03, Stephen Warren wrote:

Quoted 3 lines

I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each driver registers an arbitrary number of adapters and/or buses during its initialization.

Why should an i2c driver register buses? That is board specific.

Quoted 5 lines

We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; just have each driver define a structure that gets put into a special linker section, so that the I2C core can iterate over all linked drivers at boot time and call their initialization functions.

Quoted 2 lines

Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically.

How do we this, for example on some powerpc boards, which boot from NOR flash and read a SPD EEprom data for RAM initialization (Note: we have here not a lot of stack, nor RAM)? Maybe we should wait with this hole patchserie until DM is ready? (And maybe rework it completly?)

Quoted 1 line · Heiko Schocher

On 01.11.2012 18:03, Stephen Warren wrote:

...

Quoted 5 lines

Quoted 3 lines

I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each driver registers an arbitrary number of adapters and/or buses during its initialization.

Why should an i2c driver register buses? That is board specific.

I don't entirely agree here. Certainly the information is board-specific, but I don't believe that precludes bus registration occurring from the I2C adapter drivers themselves, based on information passed from a board file or device tree.

If a particular adapter is instantiated by the board, then there is clearly an I2C bus attached to that adapter. Hence, it's quite reasonable for the adapter itself to register the bus directly attached to it.

Following on from there, if there's an I2C bus mux attached to some I2C bus, then there are clearly I2C buses downstream from the bus mux, and the bus mux driver knows exactly how many there are, and can register those buses.

This can continue through a whole tree of I2C adapters/muxes/buses.

The above model fits into device tree very well; in that case, you only find out about the existence of downstream muxes etc. once you've initialized the I2C adapter for the parent bus.

Equally, I think this model can work very well for manually coded board files; the board file can directly instantiate all top-level I2C adapters, and pass information to the I2C driver for those adapters indicating which I2C devices are attached to the buses. Then, if one of those devices happens to be an I2C bus mux, it can instantiate further I2C buses, which in turn instantiate more devices based on the parameters passed to the I2C bus mux driver.

Quoted 8 lines

Quoted 7 lines

We could probably even get away without CONFIG_SYS_I2C_DRIVERS at all; just have each driver define a structure that gets put into a special linker section, so that the I2C core can iterate over all linked drivers at boot time and call their initialization functions.

Even if we don't get rid of CONFIG_SYS_I2C_ADAPTERS, I really think we should get rid of CONFIG_SYS_I2C_BUSSES and do that dynamically.

Quoted 4 lines

How do we this, for example on some powerpc boards, which boot from NOR flash and read a SPD EEprom data for RAM initialization (Note: we have here not a lot of stack, nor RAM)? Maybe we should wait with this hole patchserie until DM is ready? (And maybe rework it completly?)

I'd expect SPD initialization to be a special case in a U-Boot SPL; isn't that its purpose?

Quoted 8 lines

Equally, I think this model can work very well for manually coded board files; the board file can directly instantiate all top-level I2C adapters, and pass information to the I2C driver for those adapters indicating which I2C devices are attached to the buses. Then, if one of those devices happens to be an I2C bus mux, it can instantiate further I2C buses, which in turn instantiate more devices based on the parameters passed to the I2C bus mux driver.

Yep, possible ... but another approach ...

Hello Stephen,

Quoted 15 lines · Stephen Warren

Quoted 1 line · Heiko Schocher

On 01.11.2012 18:03, Stephen Warren wrote:

...

Quoted 6 lines

Quoted 4 lines

I'd suggest having a CONFIG_SYS_I2C_DRIVERS variable at most, and each driver registers an arbitrary number of adapters and/or buses during its
initialization.

Why should an i2c driver register buses? That is board specific.

I don't entirely agree here. Certainly the information is board-specific, but I don't believe that precludes bus registration occurring from the I2C adapter drivers themselves, based on information passed from a board file or device tree.

Quoted 4 lines

If a particular adapter is instantiated by the board, then there is clearly an I2C bus attached to that adapter. Hence, it's quite reasonable for the adapter itself to register the bus directly attached to it.

But some i2c drivers have more than one instance... would you for all boards register all possible instances of a driver? ...

Quoted 4 lines

Following on from there, if there's an I2C bus mux attached to some I2C bus, then there are clearly I2C buses downstream from the bus mux, and the bus mux driver knows exactly how many there are, and can register those buses.

Here again, why register all possible buses? We are just a bootloader ...

(just for illustration, please don't merge)

This enables CONFIG_SYS_I2C on Tegra, updating existing boards and the Tegra i2c driver to support this.

Signed-off-by Simon Glass<[email protected]>

---
  drivers/i2c/i2c_core.c      |    3 ++
  drivers/i2c/tegra_i2c.c     |   64 +++++++++++++++++--------------------------
  include/configs/seaboard.h  |    4 ++-
  include/configs/trimslice.h |    4 ++-
  include/configs/whistler.h  |    4 ++-
  5 files changed, 37 insertions(+), 42 deletions(-)

drivers/i2c/i2c_core.c +3 -0

index 5a738d5..44f1297 100644
@@ -9,6 +9,9 @@
  #ifdef CONFIG_SYS_I2C_SOFT
  extern struct i2c_adapter soft_i2c_adap[];
  #endif
+#ifdef CONFIG_TEGRA_I2C
+extern struct i2c_adapter tegra_i2c_adap[];
+#endif
 
  struct i2c_adapter *i2c_adap[CONFIG_SYS_NUM_I2C_ADAPTERS] =
  			CONFIG_SYS_I2C_ADAPTERS;

drivers/i2c/tegra_i2c.c +10 -39

index 3e157f4..b92bcf1 100644
@@ -35,8 +35,6 @@
 
  DECLARE_GLOBAL_DATA_PTR;
 
-static unsigned int i2c_bus_num;
-
  /* Information about i2c controller */
  struct i2c_bus {
  	int			id;

@@ -327,21 +325,11 @@ static struct i2c_bus *tegra_i2c_get_bus(unsigned int bus_num)
  	return bus;
  }
 
-unsigned int i2c_get_bus_speed(void)
+static unsigned int tegra_i2c_set_bus_speed(unsigned int speed)
  {
  	struct i2c_bus *bus;
 
-	bus = tegra_i2c_get_bus(i2c_bus_num);
-	if (!bus)
-		return 0;
-	return bus->speed;
-}
-
-int i2c_set_bus_speed(unsigned int speed)
-{
-	struct i2c_bus *bus;
-
-	bus = tegra_i2c_get_bus(i2c_bus_num);
+	bus = tegra_i2c_get_bus(i2c_get_bus_num());
  	if (!bus)
  		return 0;
  	bus->speed = speed;

@@ -450,7 +438,7 @@ void i2c_init_board(void)
  		return;
  }
 
-void i2c_init(int speed, int slaveaddr)
+static void tegra_i2c_init(int speed, int slaveaddr)
  {
  	/* This will override the speed selected in the fdt for that port */
  	debug("i2c_init(speed=%u, slaveaddr=0x%x)\n", speed, slaveaddr);

@@ -500,7 +488,7 @@ int i2c_read_data(struct i2c_bus *bus, uchar chip, uchar *buffer, int len)
  }
 
  /* Probe to see if a chip is present. */
-int i2c_probe(uchar chip)
+static int tegra_i2c_probe(uchar chip)
  {
  	struct i2c_bus *bus;
  	int rc;

@@ -526,7 +514,8 @@ static int i2c_addr_ok(const uint addr, const int alen)
  }
 
  /* Read bytes */
-int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
+static int tegra_i2c_read(uchar chip, uint addr, int alen, uchar *buffer,
+			  int len)
  {
  	struct i2c_bus *bus;
  	uint offset;

@@ -534,7 +523,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
 
  	debug("i2c_read: chip=0x%x, addr=0x%x, len=0x%x\n",
  				chip, addr, len);
-	bus = tegra_i2c_get_bus(i2c_bus_num);
+	bus = tegra_i2c_get_bus(i2c_get_bus_num());
  	if (!bus)
  		return 1;
  	if (!i2c_addr_ok(addr, alen)) {

@@ -564,7 +553,8 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
  }
 
  /* Write bytes */
-int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
+static int tegra_i2c_write(uchar chip, uint addr, int alen, uchar *buffer,
+			   int len)
  {
  	struct i2c_bus *bus;
  	uint offset;

@@ -572,7 +562,7 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
 
  	debug("i2c_write: chip=0x%x, addr=0x%x, len=0x%x\n",
  				chip, addr, len);
-	bus = tegra_i2c_get_bus(i2c_bus_num);
+	bus = tegra_i2c_get_bus(i2c_get_bus_num());
  	if (!bus)
  		return 1;
  	if (!i2c_addr_ok(addr, alen)) {

@@ -593,25 +583,6 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len)
  	return 0;
  }
 
-#if defined(CONFIG_I2C_MULTI_BUS)
-/*
- * Functions for multiple I2C bus handling
- */
-unsigned int i2c_get_bus_num(void)
-{
-	return i2c_bus_num;
-}
-
-int i2c_set_bus_num(unsigned int bus)
-{
-	if (bus>= TEGRA_I2C_NUM_CONTROLLERS || !i2c_controllers[bus].inited)
-		return -1;
-	i2c_bus_num = bus;
-
-	return 0;
-}
-#endif
-
  int tegra_i2c_get_dvc_bus_num(void)
  {
  	int i;

@@ -625,3 +596,18 @@ int tegra_i2c_get_dvc_bus_num(void)
 
  	return -1;
  }

drivers/i2c/tegra_i2c.c +5 -0

+		.name		= "tegra-i2c",
+		.init_done	= 0,
+		.hwadapnr	= 0,
+	}
+};

include/configs/seaboard.h +0 -0

index 0727a4c..61aca66 100644

include/configs/seaboard.h +25 -0

+struct i2c_adapter tegra_i2c_adap[] = {
+	{
+		.init		= tegra_i2c_init,
+		.probe		= tegra_i2c_probe,
+		.read		= tegra_i2c_read,
+		.write		= tegra_i2c_write,
+		.set_bus_speed	= tegra_i2c_set_bus_speed,
+		.speed		= 100000,
+		.slaveaddr	= 0,
+		.name		= "tegra-i2c",
+		.init_done	= 0,
+		.hwadapnr	= 0,
+	},
+     {
+		.init		= tegra_i2c_init,
+		.probe		= tegra_i2c_probe,
+		.read		= tegra_i2c_read,
+		.write		= tegra_i2c_write,
+		.set_bus_speed	= tegra_i2c_set_bus_speed,
+		.speed		= 100000,
+		.slaveaddr	= 0,
 
+		.name		= "tegra-i2c-2",
+		.init_done	= 0,
+		.hwadapnr	= 1,
+      },

+};

Rather than using a variable in various places, add a single function, tegra_i2c_get_bus(), which returns a pointer to information about a bus.

This will make it easier to move to the new i2c framework.

Signed-off-by Simon Glass<[email protected]>

---
  drivers/i2c/tegra_i2c.c |   78 ++++++++++++++++++++++++++++++++++++----------
  1 files changed, 61 insertions(+), 17 deletions(-)

drivers/i2c/tegra_i2c.c +0 -0

index efc77fa..3e157f4 100644

drivers/i2c/tegra_i2c.c +13 -0

@@ -302,18 +304,48 @@ static int tegra_i2c_read_data(u32 addr, u8 *data, u32 len)
  #error "Please enable device tree support to use this driver"
  #endif
 
+/**
+ * Check that a bus number is valid and return a pointer to it
+ *
+ * @param bus_num	Bus number to check / return
+ * @return pointer to bus, if valid, else NULL
+ */
+static struct i2c_bus *tegra_i2c_get_bus(unsigned int bus_num)
+{
+	struct i2c_bus *bus;
+
+	if (bus_num>= TEGRA_I2C_NUM_CONTROLLERS) {
+		debug("%s: Invalid bus number %u\n", __func__, bus_num);
+		return NULL;

Hello Simon,

Quoted 5 lines · Simon Glass

Rather than using a variable in various places, add a single function, tegra_i2c_get_bus(), which returns a pointer to information about a bus.

Quoted 7 lines

This will make it easier to move to the new i2c framework.

Signed-off-by Simon Glass<[email protected]>

---
  drivers/i2c/tegra_i2c.c |   78
++++++++++++++++++++++++++++++++++++----------
  1 files changed, 61 insertions(+), 17 deletions(-)

First question, did you tried the new i2c framework on some HW? Works it for you?

Quoted 6 lines

drivers/i2c/tegra_i2c.c +0 -0

index efc77fa..3e157f4 100644

[...]

Quoted 20 lines

drivers/i2c/tegra_i2c.c +14 -0

@@ -302,18 +304,48 @@ static int tegra_i2c_read_data(u32 addr, u8 *data,
 u32 len)
  #error "Please enable device tree support to use this driver"
  #endif
 
+/**
+ * Check that a bus number is valid and return a pointer to it
+ *
+ * @param bus_num      Bus number to check / return
+ * @return pointer to bus, if valid, else NULL
+ */
+static struct i2c_bus *tegra_i2c_get_bus(unsigned int bus_num)
+{
+       struct i2c_bus *bus;
+
+       if (bus_num>= TEGRA_I2C_NUM_CONTROLLERS) {
+               debug("%s: Invalid bus number %u\n", __func__, bus_num);
+               return NULL;
+       }

drivers/i2c/tegra_i2c.c static int tegra_i2c_read_data(u32 addr, u8 *data, · line 331

Quoted 10 lines

drivers/i2c/tegra_i2c.c +9 -0

+       bus =&i2c_controllers[bus_num];
 
+       if (!bus->inited) {
+               debug("%s: Bus %u not available\n", __func__, bus_num);
+               return NULL;
+       }
+
+       return bus;
+}
+

I added, as Stephen and you suggested, in the "struct i2c_adapter" to the driver callbacks a new parameter "struct i2c_adapter *adap":

struct i2c_adapter {
void (*init)(struct i2c_adapter *adap, int speed,
int slaveaddr);
int (*probe)(struct i2c_adapter *adap, uint8_t chip);
int (*read)(struct i2c_adapter *adap, uint8_t chip,
uint addr, int alen, uint8_t *buffer, int len);
int (*write)(struct i2c_adapter *adap, uint8_t chip,
uint addr, int alen, uint8_t *buffer, int len);
uint (*set_bus_speed)(struct i2c_adapter *adap,
uint speed);
[...]

so the probe/read/write and set_bus_speed() functions gets now a "struct i2c_adapter" pointer ... i2c driver internally, we need only the "struct i2c_adapter", which the i2c core passes to the i2c driver. So this function would look like now:

Quoted 1 line

drivers/i2c/tegra_i2c.c +1 -0

+static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap)

drivers/i2c/tegra_i2c.c static int tegra_i2c_read_data(u32 addr, u8 *data, · line 334

Quoted 3 lines

drivers/i2c/tegra_i2c.c +2 -0

+{
+     struct i2c_bus *bus;

^^^^^^^^^^^^^^
maybe a rename to "i2c_tegra_driver" would be good?

drivers/i2c/tegra_i2c.c static int tegra_i2c_read_data(u32 addr, u8 *data, u32 len) · line 328

Quoted 9 lines

drivers/i2c/tegra_i2c.c +8 -0

+     bus =&i2c_controllers[adap->hwadapnr];
 
+     if (!bus->inited) {
+             debug("%s: Bus %u not available\n", __func__, bus_num);
+             return NULL;
+     }
+
+     return bus;
+}

With this, we can get rid of i2c_bus_num in this driver.

Also the probe/read/write and set_bus_speed() functions needs to be adapted. I can do this for you, and add this patchset to my v2 post, if it is ok for you?

drivers/i2c/tegra_i2c.c static int tegra_i2c_read_data(u32 addr, u8 *data, u32 len) · line 330

Quoted 2 lines

drivers/i2c/tegra_i2c.c +0 -0

  unsigned int i2c_get_bus_speed(void)

This function is not needed for the new framework! [...]

Quoted 19 lines

drivers/i2c/tegra_i2c.c +6 -4

  int i2c_set_bus_speed(unsigned int speed)
  {
-       struct i2c_bus *i2c_bus;
+       struct i2c_bus *bus;
 
-       i2c_bus =&i2c_controllers[i2c_bus_num];
 
-       i2c_bus->speed = speed;
-       i2c_init_controller(i2c_bus);
+       bus = tegra_i2c_get_bus(i2c_bus_num);
+       if (!bus)
+               return 0;
+       bus->speed = speed;
+       i2c_init_controller(bus);
 
        return 0;
  }

@@ -426,7 +458,7 @@ void i2c_init(int speed, int slaveaddr)

[...]

Rest looks good to me.

Thanks!

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

Hi Heiko,

Quoted 18 lines · Heiko Schocher

Hello Simon,

Quoted 12 lines · Simon Glass

Rather than using a variable in various places, add a single function, tegra_i2c_get_bus(), which returns a pointer to information about a bus.

This will make it easier to move to the new i2c framework.

Signed-off-by Simon Glass<[email protected]>

---
   drivers/i2c/tegra_i2c.c |   78
++++++++++++++++++++++++++++++++++++----------
   1 files changed, 61 insertions(+), 17 deletions(-)

Quoted 2 lines

First question, did you tried the new i2c framework on some HW? Works it for you?

Yes, it works fine. Tried on a seaboard.

Quoted 4 lines

Quoted 4 lines

drivers/i2c/tegra_i2c.c +0 -0

index efc77fa..3e157f4 100644

Quoted 6 lines

so the probe/read/write and set_bus_speed() functions gets now a "struct i2c_adapter" pointer ... i2c driver internally, we need only the "struct i2c_adapter", which the i2c core passes to the i2c driver. So this function would look like now:

Quoted 1 line

+static struct i2c_bus *tegra_i2c_get_bus(struct i2c_adapter *adap)

Sounds good. Are you going to send a new patch?

Quoted 16 lines

drivers/i2c/tegra_i2c.c

Quoted 2 lines

drivers/i2c/tegra_i2c.c +2 -0

+{
+     struct i2c_bus *bus;

^^^^^^^^^^^^^^
maybe a rename to "i2c_tegra_driver" would be good?

drivers/i2c/tegra_i2c.c static int tegra_i2c_read_data(u32 addr, u8 *data, u32 len) · line 328

Quoted 9 lines

drivers/i2c/tegra_i2c.c +8 -0

+     bus =&i2c_controllers[adap->hwadapnr];
 
+     if (!bus->inited) {
+             debug("%s: Bus %u not available\n", __func__, bus_num);
+             return NULL;
+     }
+
+     return bus;
+}

With this, we can get rid of i2c_bus_num in this driver.

Quoted 3 lines

Also the probe/read/write and set_bus_speed() functions needs to be adapted. I can do this for you, and add this patchset to my v2 post, if it is ok for you?

Of course.

Quoted 5 lines

Quoted 1 line

unsigned int i2c_get_bus_speed(void)

This function is not needed for the new framework! [...]

Yes, I thought I removed it in 2/2, perhaps not.

Quoted 22 lines

Quoted 19 lines

   int i2c_set_bus_speed(unsigned int speed)
   {

drivers/i2c/tegra_i2c.c +6 -4

-       struct i2c_bus *i2c_bus;
+       struct i2c_bus *bus;
 
-       i2c_bus =&i2c_controllers[i2c_bus_num];
 
-       i2c_bus->speed = speed;
-       i2c_init_controller(i2c_bus);
+       bus = tegra_i2c_get_bus(i2c_bus_num);
+       if (!bus)
+               return 0;
+       bus->speed = speed;
+       i2c_init_controller(bus);
 
         return 0;
   }

@@ -426,7 +458,7 @@ void i2c_init(int speed, int slaveaddr)

[...]

Quoted 3 lines

Rest looks good to me.

Thanks!

Will await your further patch, and I am ready to test. Thanks.