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

[Lukasz Majewski]

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:
> >>> 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!

Ok, I need this.


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group