Hello ksi,
k...@koi8.net wrote:
> On Fri, 13 Feb 2009, Heiko Schocher wrote:
>> k...@koi8.net wrote:
>>> Here is the second attempt for initial portion of multibus/multiadapter
>>> I2C support.
>>>
>> Can you please send your patches with some better commit messages.
>> You only send your Signed-off-by, without any explanation. Please
>> change this.
>
> There is not much sense in extensive commit messages in this case, IMHO. It
> is not a bug fix or added feature at one particular place; it is a major
> rework. The only message I can give is something like "Changes for
> multiadapter/multibus I2C support..."
>
> I'll add it to the second attempt that I will make later today.
>
>>> This includes a set of common files, all drivers in drivers/i2c and all
>>> boards affected by these changes (config files, board files, and lib_xx
>>> files.)
>>>
>>> There is an illustrative example of multiadapter multibus I2C config in
>>> MPC8548CDS.h config file (#if 0'd.) Definitions in that example are
>>> bogus so please don't expect it to work. It will compile though...
>>>
>>> This set also includes big rework for soft_i2c.c that makes it template
>>> version that allows up to 4 bitbanged adapters. This number can be
>>>
>> Didn;t you try my suggestion? This is a really big define monster now,
>> which I think, we can avoid, and without to change nearly all lines of
>> the existing driver.
>
> We can not avoid it. At least I can not see an easy way to do this. SOFT_I2C
Yes, we can. Look again deeper in my approach, this _is_ an easy way!
I also looked again in your changes in the fsl_i2c.c driver, and we
can make this also simplier, by using cur_adap_nr->hwadapnr!
We have not to define for both hardware adapter each function in
i2c_adap_t! For example:
You did:
static void fsl_i2c1_init(int speed, int slaveadd)
{
__i2c_init(0, speed, slaveadd);
}
instead we only need
i2c_init(cur_adap_nr->hwadapnr, speed, slaveadd);
with
i2c_adap_t fsl_i2c_adap[] = {
{
.init = i2c_init,
[...]
.hwadapnr = 0,
.name = FSL_NAME(CONFIG_SYS_FSL_I2C_OFFSET)
},
#ifdef CONFIG_SYS_FSL_I2C2_OFFSET
{
.init = i2c_init,
[...]
.hwadapnr = 1,
.name = FSL_NAME(CONFIG_SYS_FSL_I2C2_OFFSET)
},
#endif
Please change this driver also!
If I think more, we never even need to change the function parameters
like you did for example for i2c_int ()! We can use at the beginning
of every function who go in i2c_adap_t, the "cur_adap_nr->hwadapnr"
and make the settings we need for this function... and wow we saved
one function parameter.
> is special. Those multiple e.g. MXC or OMAP3 adapters can be parameterized
> because different channels do only differ in their base address that can be
> made into a parameter. Software I2C is totally different because it has
Why is this different? If you change a base or the way to the pins?
> totally different functions for different channels, there is nothing we can
Think about my explanation to the soft_i2c.c driver in previous EMail and
above function.
It also works!
> make into a parameter. All those I2C_SDA etc. are NOT DEFINES, they are
> MACROS. Every function for every channel is built of those macros that can
I know this in your approach, but we _don;t_ need this. We simply can make
one "common" board function and switch in this function dependent on the
"cur_adap_nr->hwadapnr" to the particular GPIO pin functions, wherever the
are!
> be absolutely different for each channel. They define NOT some PARAMETERS
> but function TEXT that will be compiled into executable code.
And this additional TEXT I save too!
I think, you think too much in C++?
> This is how it is done in Linux kernel (see e.g. drivers/hwmon/lm93.c.)
Thats no argument.
bye
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
