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,
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).
> > 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
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);
,which can be defined on different platforms.
What is your opinion about that?
--
Best regards,
Lukasz Majewski
Samsung Poland R&D Center | Linux Platform Group
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
