[U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4

[Heiko Schocher]

Hello ksi,

ksi at koi8.net wrote:
> On Fri, 20 Feb 2009, Heiko Schocher wrote:
>> ksi at koi8.net wrote:
>>> On Thu, 19 Feb 2009, Heiko Schocher wrote:
>>>> ksi at koi8.net wrote:
>>>>> On Wed, 18 Feb 2009, Heiko Schocher wrote:
>>>>>> ksi at koi8.net wrote:
>>>>>>> On Mon, 16 Feb 2009, Wolfgang Denk wrote:
[...]

> Why would we need _ADDITIONAL_ ifs in extremely simple macros that just
> access some memory/io location? They are so simple and small that you can
> not make them any smaller. And _ADDING_ ifs or whatever to them will _NOT_
> make them any smaller. You can _NOT_ exclude actual code from them, you can
> only _ADD_ to it. If you want e.g. I2C_SDA for 4 interfaces you will have
> _ALL_ the code of 4 separate macros (you can _NOT_ remove a single line from
> them) _PLUS_ those ifs. One more time, it goes _ON TOP_ of whatever is in
> those macros.

I know this, but as Wolfgang mentioned, we wouldn;t have such a lot of
defines in soft-i2c.c.

[...]

> Here is the path to the real action in my case:
> 
> bus_no -> adap_no -> function -> action

precisly:
i2c_adap[i2c_bus[(bus)].adapter]-> function -> action
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> Here is yours:
> 
> bus_no -> adap_no -> function -> GLOBAL(adap_no) -> hwadap_no -> action

precisly:
cur_ptr -> function -> if (cur_ptr -> hwadap_no) -> action

Hmm.. you got an idea, why i want a cur_ptr?

Just saying we don;t want in soft-i2c.c this hwadap_no

cur_ptr -> function -> action

Which _is_ faster than your actual implementation, or?

And you need not to do:

include/i2c.h:
#ifndef CONFIG_SYS_I2C_DIRECT_BUS
#define         ADAP(bus)       i2c_adap[i2c_bus[(bus)].adapter]
#else
#define         ADAP(bus)       i2c_adap[(bus)]
#endif

it is always just cur_ptr->

> Not only that makes the path longer, not only it requires a blasphemy of
> using a global variable in object methods, but it also defeats a purpose of
> having a separate object for each adapter. Why do we need those separate
> objects if they contain exactly the same poiners to the very same functions?
> 
> There is already an index into the objects array and it is already used to
> choose a proper object. Why should we use that index again in the already
> chosen object? Why go twice over the same rake? And even if we were going
> that way what do we gain by using such horrible kludges? Is there any
> benefit?

Only to save SourceCode as Wolfgang mentioned. Don;t ignore this.

>>>>> pointers to those functions to appropriate adapter struct members at
>>>>> _COMPLILE_ time. And that's all to it.
>>>>>
>>>>> In your case you stick those functions in one monstrous i2c_write() where
>>>>> you still have those N functions as "case" bodies of some switch so you
>>>> No, just the SDA,SCL accesses have such a switch.
>>> So we should make a monster with switches off of each and every send_start()
>>> and friends and pass them a value to decide which set to use?
>> It is not in send_start(), it is in every SDA,SCL fn/macro, again, I sent
>> you a soft-i2c driver proposal. If you look in this you see, no fn
>> in it is changed!
>>
>> We want this, because we don;t need to change everything in Sourcecode,
>> as Wolfgang mentioned to you.
> 
> But you _DO_ want to change something! It is impossible to change something
> without changing it!

? Look in the soft-i2c.c I made, there is _NO_ change in the code for
adding multibus (just i2c_adap_t struct added at the end). Why you claim
there is a change? Again, only the I2C_SDA,....macros have to do be changed
when using more than one driver (and this is absolute compatible with existing
boards, when using only one there is no need for using hwadap_no in it,
also for one adapter it is the same sourcecode as actual code)

I don;t say it is better than yours, but we can save SourceCode space
(soft-i2c.c).

> Furthermore, you also must change everything because you'll have _ALL_ the
> functions and templates in that same driver to take an additional argument
> and pass it all the way down to the bare metal, those accessor macros! That
> means you will have to add an access to that global variable to each and
> every I2C method (i2c_read etc.,) then change all utility functions
> (send_start etc.) to take that argument and pass it further to those macros.

Hey come on, look in my implementation of soft-i2c.c. Where did you see
this changes? I tried my soft-i2c.c on a 8xx based board with 3 soft-i2c
adapters (the last two are made only printfs, but i saw them), and no changes
in i2c-core.c ... of course i added the cur_pointer (but I can use
use your "i2c_adap[i2c_bus[(bus)].adapter]->" if you prefer this, and
in the struct i2c_adapter the hwadap_nr"

> And that will _NOT_ make resulting code any smaller because you'll still
> have all the actual guts of all those macros _PLUS_ register loads for
> arguments you are passing _PLUS_ conditional branches in each and every
> macro (and they will be multi-step if you have more than 2 adapters...)

Yep, but is it a real problem?

> I fail to see how this can be simpler, or better, or smaller, or any more
> logical than just multiplying that code ADAPTER_NUMBER times and use
> separate methods for each adapter.

Its just simpler in sourcecode.

> Then comes an issue of where those macros should come from. In existing code
> you simply define methods to set/reset/tristate particular pins for separate
> adapters in separate macros.
>
> In your case instead of N sets of such macros you must put a single set but
> each macro will have _ALL_ those macros in a switch statement with a
> separate case for each adapter. It is wrong to put a complex C code in a
> config file and it is much more prone to errors (think about all those macro
> expansion pitfalls that are aplenty) and more difficult to comprehense.

OK, somebody who uses this, must know what he does ...

> But there is not the end of a story yet... There is another thing that is
> probably overlooked. When adding an adapter to the config file one must
> define a separate set of macros for it in my case. If he forgot to define
> some macro U-Boot build will fail with "XYZ undefined" so it will be
> immediately known and very easy to fix.

Hmm.. and if he try his new adapter in may case and it didn;t work, he
must think with his brain, okay ... thats bad.

>>> That makes everything more messy and has a performance penalty -- instead of
>>> simple branch to those simple blocks you are adding at least one register
>>> load per call to load that argument.
>> Look above. If speed is here your problem, you have a lot of other
>> problems with that, when running Linux.
>>
>>> And what are the advantages?
>> No Sourcecode change is needed.
> 
> How can you change sourcecode without changing sourcecode?

Look at my code ... i only need to add the i2c_adapter struct
in soft-i2c.c ...

[...]

>> which code? The soft-i2c driver I posted here as a proposal, also how a
>> I2C_SDA macro can look, if you have more than one bitbang driver. Look
>> in the archive.
> 
> Please see above. It is not just plain ugly and violates good engineering
> basics, it has a bunch of problems. And there is absolutely no benefits.

You ignore again the benefit in not changing soft-i2c code.

> Those guys who wrote that driver as a template, generating actual code from
> a set of configuration macros did a wonderful job and I can't see any valid
> reason to reinvent the wheel.

This guy was Wolfgang, so why you ignore his arguments?

And why we are reinventing the wheel? I change nothing in the soft-i2c.c
driver, so _why_ I are reinventing the wheel?????

[...]
>>> I'm not going to change busses when I'm running from flash. But I can call
>>> functions on any adapter I want if needed.
>> Is it needed?
> 
> It might be. In my case there is nothing special required for this, this
> comes as a free extra.

No, we defined it is as _not_ required. But just to say it again, it also
works with our suggestions.

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