Hi Heiko,
Hello Lukasz,
On 28.08.2012 14:12, Lukasz Majewski wrote:
Hi Heiko,
+#if defined(CONFIG_I2C_MULTI_BUS) +/* Handle multiple I2C buses instances */ +int get_multi_scl_pin(void) +{
- switch (I2C_GET_BUS()) {
- case I2C_4:
return CONFIG_SOFT_I2C_I2C4_SCL;- case I2C_5:
return CONFIG_SOFT_I2C_I2C5_SCL;- };
- return 0;
+}
+int get_multi_sda_pin(void) +{
- switch (I2C_GET_BUS()) {
- case I2C_4:
return CONFIG_SOFT_I2C_I2C4_SDA;- case I2C_5:
return CONFIG_SOFT_I2C_I2C5_SDA;- };
- return 0;
+}
+int multi_i2c_init(void) +{
- return 0;
+} +#endif /* CONFIG_I2C_MULTI_BUS */
Again, what is with busnr = 0-3 or 6?
This is not needed in the i2c soft file. You can define this functions board specific ... so, no change in the driver is needed ... please move this to board specific code.
Please consider, that get_multi_{sda|scl}_pin can be used by other boards. Those are written in a generic way (by calling I2C_GET_BUS()).
Got this, but why do you index them with 4 and 5 and not with 0 and 1? What is, if another board uses 0 and 1, so this would introduce the defines CONFIG_SOFT_I2C_I2C0_SDA and CONFIG_SOFT_I2C_I2C1_SDA in the "common" get_multi_sda_pin(), which leads in compilererror for your board(s) ... your proposed get_multi_sda_pin() is currently samsung specific ...
What here I'm trying to avoid is the code duplication for each board (e.g. Samsung's GONI, Universal, Trats, Origen ... etc).
If they use all the same function, they should end in a ../samsung/common/common.c because, currently your functions are samsung specific.
common is (from my point of view) that we add in the board config file:
+#define CONFIG_SOFT_I2C_GPIO_SCL get_multi_scl_pin() +#define CONFIG_SOFT_I2C_GPIO_SDA get_multi_sda_pin()
I can agree, that now only I2C_{4|5} are defined (since for now Samsung is using I2C_4 and I2C_5).
and thats samsung specific ... because other boards maybe start with I2C_0 ... and this case is not respected in your patch.
But other cases can be also defined.
Yep, and break compiling your board, as this defines are not specified.
What I see even more important is a definition of (at<i2c.h>):
+#if (defined(CONFIG_SOFT_I2C)&& defined(CONFIG_I2C_MULTI_BUS)) +enum {
- I2C_0,
- I2C_1,
- I2C_2,
- I2C_3,
- I2C_4,
- I2C_5,
- I2C_6
+};
I would like to propose that, I will rename the I2C_4 -> I2C_0 and I2C_5 -> I2C_1,
Yep!
Ok, so we have agreed.
then we can define at<i2c.h> :
+#if (defined(CONFIG_SOFT_I2C)&& defined(CONFIG_I2C_MULTI_BUS)) +enum {
- I2C_0,
- I2C_1,
+};
And this would facilitate handling of SOFT_I2C numbering across relevant subsystems (e.g. PMICs and other).
Ok.
Nice,
since this will organize the order of multiple (soft) I2C devices.
Imagine that 2 PMICs are on the board (I2C_4 and I2C_5). I need to distinct those (when calling I2C_SET|GET_BUS) And then support for another I2C device (e.g. I2C_2) at other subsystem is provided. Then I can:
- Add common definition of I2C_X (as I've proposed) to<i2c.h>
- Add #define I2C_X on the ./include/configs/{e.g. trats}.h
board.
Why add "#define I2C_X" in ./include/configs/{e.g.trats}.h ? I don´t understand this ... and you do not this in your patchserie!
For second approach used I need to duplicate the code for other targets (goni, universal, origen) when needed and I cannot avoid that someone
or make a ../samsung/common/common.c until they are samsungspecific.
else will define other names -> like #define MINE_I2C_X on his/her ./include/configs/{board}.h
Ok, but if you use I2C_4 and I2C_5, you must also define the I2C_0, I2C_1, I2C_2 and I2C_3 cases in the "get_multi_*" functions, as other boards would start with I2C_0 ...
... and add a documentation in README for this ...
but I mislike to introduce such a lot of defines ... instead of defining get_multi_*() board/manufacturer/soc specific ... Maybe there is a board with 10 i2c soft busses, so we must define in all boards using soft multibus this 20 (CONFIG_SOFT_I2C_I2C*_SCL/SDA)defines ... or at least define them if not defined in include/i2c.h ... bad.
I will move the "get_multi_*" functions to ../samsung/common/common.c
Good.
However, I think, that it would be good to add following declarations to <i2c.h>:
extern int get_multi_scl_pin(void); extern int get_multi_sda_pin(void); extern int multi_i2c_init(void);
In the case CONFIG_I2C_MULTI_BUS is defined.
,which can be defined on different platforms.
What is your opinion about that?
I agree with this!
Ok, I need this.