[U-Boot] [PATCH] 7/12 Multiadapter/multibus I2C, drivers part 4
ksi at koi8.net
ksi at koi8.net
Mon Feb 16 07:35:29 CET 2009
On Sun, 15 Feb 2009, Heiko Schocher wrote:
> Hello ksi,
>
> ksi at koi8.net wrote:
> > On Sat, 14 Feb 2009, Heiko Schocher wrote:
> >
> >
> >> Hello ksi,
> >>
> >> ksi at koi8.net wrote:
> >>
> >>> On Fri, 13 Feb 2009, Heiko Schocher wrote:
> >>>
> >>>> ksi at koi8.net wrote:
> >>>>
> >>>>> Signed-off-by: Sergey Kubushyn <ksi at koi8.net>
> >>>>> ---
> >>>>> diff -purN u-boot-i2c.orig/drivers/i2c/soft_i2c.c u-boot-i2c/drivers/i2c/soft_i2c.c
> >>>>> --- u-boot-i2c.orig/drivers/i2c/soft_i2c.c 2009-02-12 10:43:41.000000000 -0800
> >>>>> +++ u-boot-i2c/drivers/i2c/soft_i2c.c 2009-02-12 10:46:00.000000000 -0800
> >>>>> @@ -1,4 +1,8 @@
> >>>>> /*
> >>>>> + * Copyright (c) 2009 Sergey Kubushyn <ksi at koi8.net>
> >>>>> + *
> >>>>> + * Changes for multibus/multiadapter I2C support.
> >>>>> + *
> >>>>> * (C) Copyright 2001, 2002
> >>>>> * Wolfgang Denk, DENX Software Engineering, wd at denx.de.
> >>>>>
> >>>> [...]
> >>>>
> >>>> The following patch is based on your patches without 7/12 and
> >>>> adds multibus support for the soft_i2c driver without doing such
> >>>> a big change as you did. Maybe it is not yet perfect, because
> >>>> it is just a fast try, but I think we should go this way. What
> >>>> do you/others think?
> >>>>
> >>> The reason behind this patch is making SEVERAL different SOFT_I2C ADAPTERS
> >>> available. Not BUSSES but separate PHYSICAL I2C ADAPTERS made of different
> >>> pin pairs from different chips.
> >>>
> >> This you can also do with "my" suggestion ...
> >>
> >>
> >>> OK, please explain how are you going to make different functions for
> >>> different adapters? Let's say you want to use 2 on-SoC GPIO pins for
> >>>
> >> You can do now the following for example in your
> >> include/configs/MPC8548CDS.h example:
> >>
> >> you only have to define
> >>
> >> #define I2C_SDA(bit) (printf("hwadap: %d sda1: %d", cur_adap_nr->hwadapnr, bit))
> >>
> >> if this is a real driver you can make a function in your board code
> >> say (just a fast thought):
> >>
> >> void i2c_soft_sda (int bit)
> >> {
> >> switch(cur_adap_nr->hwadapnr) {
> >> case 0:
> >> /* adapter specfic code 0 */
> >> break;
> >> case 1:
> >> /* adapter specfic code 1 */
> >> break;
> >> [...]
> >> }
> >> }
> >>
> >> and define in config file
> >>
> >> #define I2C_SDA(bit) i2c_soft_sda (bit)
> >>
> >
> > That means you have to make changes in two places instead of one -- config
> > file AND $(BOARD).c. Also you use functions instead of macros and you can
> >
>
> Yes. But that was just a thought, it should be possible to do this also
> in macros.
Not if you want to make functions in $(BOARD).c.
> > NOT make them inline because they come from a separate object file. This
> > essentially defeats the very purpose of that common soft_i2c.c driver. If
> > you want to make functions for bitbanged I2C into the $(BOARD).c there is no
> > reason to have them as a base for that driver. It is much more logical to do
> >
>
> Maybe more logical, but not needed.
>
> > everything in reverse, i.e. instead of having soft_i2c.c as a bona fide
> > drivers and those I2C_SDA and friends as its building blocks make those
> > i2c_soft_sda() etc. in each and every $(BOARD).c into primary entities and
> > build the actual driver in the $(BOARD).c itself. Just convert that
> > soft_i2c.c into a header file with macros for real functions (soft_i2c_read
> > etc.) and instantiate them in the $(BOARD).c.
> >
> > The only problem with that is it breaks uniformity and makes another mess.
> >
>
> Just, if we do this, but we don;t need to do it so.
>
> > The whole idea was to bring _ALL_ I2C drivers to a single place and make
> > them totally transparent and uniform. Something like e.g. Linux VFS.
> >
> > And remember, the devil is in details. How are you going to assign
> > (initialize) that innocent looking "cur_adap_nr->hwadapnr"? How are you
> > going to work on an adapter other that "current" in a situation when you can
> > NOT change "current" adapter (e.g. perform all I2C layer initialization
> > while still running from flash?) Remember, this is plain C and there is no
> >
>
> Yes, good point. But do we need more then one i2c adapter when running
> from flash? I see only one reason to use i2c when running from flash:
> accessing SPD EEprom ... and this "bus" could always be the first
> hw adapter. All other accesses to i2c should be moved to run when
> we are in ram.
You can have, e.g. TWO SPD EEPROMs on different busses. And please remember
that infamous "640K ought to be enough for anybody..."
> > "this" pointer... And that is just a tip of an iceberg...
> >
> > And the million dollar question -- what is the potential gain?
> >
>
> I want to avoid such a big change in soft_i2c.c. Also if you have
> 4 bitbang instances with your version you have 3 times more code.
There is almost NO change in soft_i2c.c. It is simply templetized and 4
#ifdef'ed instances are added. If you only have one bitbanged adapter in the
system it generates _EXACTLY_ the same source code as original one did after
CPP pass.
> But if others are on your side, I have no problem with your approach.
I don't think there is a viable reason to object unless somebody came up
with something better.
> >
> >>> adapter #0, 2 GPIOs from a PCI-PCI bridge for adapter #1, and 2 pins from
> >>> some chip sitting behind that bridge for adapter #2 if all those pin sets
> >>> are accessed totally different. I won't even start about using pins from
> >>> different chips for SDA and SCL (let's say you only have one GPIO available
> >>> on your SoC and another one on PCI Bridge.)
> >>>
> >>> What your patch creates is just aliases to the SAME physical adapter.
> >>>
> >> No, it is not! I only use the same functions, but in the board
> >> specific code it is possible to made a switch and access
> >> the Pins where ever they are.
> >>
> >
> > You are adding unnecessary complexity to the code. And you break uniformity.
> >
>
> not really.
>
> > Those defines in config/soft_i2c.c make real inline functions at _COMPILE_
> > time. Your approach shifts it to _LINK_ time. It also makes those drivers
> > come from 3 places ($(BOARD).c, include/configs/$(BOARD).h, and soft_i2c.c)
> >
>
> Thats not really a problem.
It is. The more files you spread the same code between the more places to
fix if something's changed.
There is another reason why those functions were made into macros instead of
functions. I'll come to that in my reply to your next email.
---
******************************************************************
* KSI at home KOI8 Net < > The impossible we do immediately. *
* Las Vegas NV, USA < > Miracles require 24-hour notice. *
******************************************************************
More information about the U-Boot
mailing list