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

[Lukasz Majewski]

Hi Heiko,

> >   #if defined(CONFIG_I2C_MULTI_BUS)
> >   static unsigned int i2c_bus_num __attribute__ ((section
> > (".data"))) = 0; +const char *soft_i2c_name[CONFIG_SYS_MAX_I2C_BUS]
> > = {
> > +	NULL,
> > +	NULL,
> > +	NULL,
> > +	NULL,
> > +	"soft_i2c_4",
> > +	"soft_i2c_5",
> > +	NULL,
> > +};  
> 
> For what do you need "soft_i2c_name"? I see no usage of this in your
> patchset? Also why the "NULL" for 0-3 and 6 positions?
> 
> And what is, if CONFIG_SYS_MAX_I2C_BUS is < 7 ?

Indeed this can be removed - it is not needed (for now).

> > +#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()).

What here I'm trying to avoid is the code duplication for each board
(e.g. Samsung's GONI, Universal, Trats, Origen ... etc).
I can agree, that now only I2C_{4|5} are defined (since for now Samsung
is using I2C_4 and I2C_5).

But other cases can be also defined.

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
+};

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.

For second approach used I need to duplicate the code for other targets
(goni, universal, origen) when needed and I cannot avoid that someone
else will define other names -> like #define MINE_I2C_X on
his/her ./include/configs/{board}.h





-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group