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

[ksi at koi8.net]

On Sat, 21 Feb 2009, Heiko Schocher wrote:

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

So that is those defines that look scary, right? As I already said we could
have them replaced with just repeated text.

It is quite a strange position - make everything ugly, make it bloated,
violate every taboo just because something look scary.

And why is it just that soft_i2c.c, there are other multiadapter drivers
(fsl_i2c.c, mxc_i2c.c, omap*.) Should we also apply those kludges to them?

> [...]
> 
> > 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?

No, I don't. That scary looking underlined construction IS your cur_ptr. And
the variable itself is an integer, bus number, that doesn't have to be
adjusted when the code is relocated and i2c_get_bus_num() just returns this
integer. And it is file scope.

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

My actual implementation.

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

OK, I will not do a lengthy analysis again, I do not have more time to
waste. With your cur_ptr you introduce a whole bunch of problems, violate
rules of good engineering and do not provide _ANY_ benefits.

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

You do NOT save any SourceCode. And there is no reason to save SourceCode
even if you did.

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

Eh, how are you going to pass that adap_no to those macros? Or have I
underestimated the extent of that horror and you want to access that
external global variable in each that macro? An you want THAT in config
files?

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

There in no such thing as "slightly pregnant" :) You should also change
other multiadapter drivers for consistency. And please explain why we should
have separate structures for each adapter that hold exactly the same
pointers to the same function?

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

Where are the benefits? It does NOT make _ANY_ good, just little harm. And
"it is not that much harm" does _NOT_ justify something if there is no some
benefit that is worth that additional harm.

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

It is NOT.

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

Yep. You make the code bloated, more difficult to comprehense for board
designer and this is OK because some part of code that that designer would
probably never ever look at looks scary to you? Perfect logic...

> > 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 is really bad. And there is no way for a complier to detect it because
that code is absolutely legal as far as compiler concerned.

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

No you don't. You should also add that global variable, you should rewrite
couple of functions, you should take care of adjusting that cur_pointer
after relocation so it is still pointing to a proper place, you should
change those I2C_xxx macros in _ALL_ config files because you either allow
for multiadapter and then your configuration always uses that pointer or you
do NOT allow it at all if you are not using that pointer to have a
consistent code and so on...

And where is the benefit for all that mess?

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

You can _NOT_ change soft_i2c.c code without changing the code! My patch did
_NOT_ change that code, it is yours that does !!!

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

You _DO_ change. That is me who don't. I'm just multiplying that file, not
changing anything in it.

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

OK, I rest my case... Sorry guys I simply don't have any more time to waste,
almost two weeks is more than enough. And I have another, more urgent job
waiting.

Please count me off.

---
******************************************************************
*  KSI at home    KOI8 Net  < >  The impossible we do immediately.  *
*  Las Vegas   NV, USA   < >  Miracles require 24-hour notice.   *
******************************************************************