[U-Boot] [PATCH 1/2] i2c:soft:multi: Support for multiple soft I2C buses

[Lukasz Majewski]

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