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

[Heiko Schocher]

Hello Lukasz,

On 28.08.2012 12:40, Lukasz Majewski wrote:
> 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).

Ok.

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

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany