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

[Heiko Schocher]

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!

> 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.

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

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