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!
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.
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:
1. Add common definition of I2C_X (as I've proposed) to<i2c.h>
2. 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 samsung
specific.
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!
bye,
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
