Hello Simon,
On 26.10.2012 18:08, Simon Glass wrote:
On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocher<h...@denx.de> wrote:
Hello Simon,
On 25.10.2012 23:37, Simon Glass wrote:
On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher<h...@denx.de> wrote:
[...]
2. The init is a bit odd, in that we can call init() repeatedly.
Perhaps that function should be renamed to reset, and the init should
be used for calling just once at the start?
What do you mean here exactly? I couldn´t parse this ...
Well there is start-of-day setup, which I think should be called init.
This is done once on boot for each i2c adapter.
Hmm... I am not sure if this is only done on boot, because we should
"close" or "deinit" an adapter if not used any more in U-Boot as the
U-Boot principle says:
http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast
So I want to add in future some "deinit" to every adapter, and
call it from i2c_set_bus() when switching to another i2c adapter ...
And then there is the i2c_init() which seems to be called whenever we
feel like it - e.g. to change speed. I suggest that we use init() to
mean start-of-day init and reset(), or similar, to mean any post-init.
I am not suggest that for this series, just as a future effort.
Yes, that should be changed. We do not need an init() in the i2c
API, as i2c_set_bus_num() do this for us (and later also the deinit())
We just need a set/get_speed() and a deblock()/reset() ?
Maybe a step in the API cleanup?
3. Rather than each device having to call i2c_get_bus_num() to find
out what bus is referred to, why not just pass this information in? In
fact the adapter pointer can serve for this.
Not the "struct i2c_adapter" must passed, but the "struct
i2c_bus"!
And each device should know, which i2c bus it uses, or? So at
the end we should have something like i2c_read(struct i2c_bus *bus, ...)
calls ... and the i2c core can detect, if this bus is the
current, if so go on, if not switch to this bus. So at the
end i2c_set_bus_num would be go static ...
4. So perhaps the i2c read/write functions should change from:
int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len)
to:
int i2c_read((struct i2c_adapter *adapter, uint addr, int alen, uchar
*buffer, int len)
Yep, exactly, see comments to point 3 ...
That would be the best (and I think in previous discussions I defined
this as one goal ...), but this would be (another) big change,
because this is an *API* change, with maybe a lot of config file
changes ...
Let us bring in the new i2c framework with all i2c drivers converted,
and then do the next step ... maybe one step more, if mareks device
model is ready, we can switch easy to DM ... and maybe get this API
change for free ...
Yes. I certainly understand the need to fit in with what is already
there, and avoid a massive API change, which can be performed as a
follow-on patch anyway. With your info above I will adjust the tegra
driver to work with this and test it.
Ok, great! So I post v2 patches after you tested ...
And Yes, we should do this API change, but I tend to do this step after
the new i2c framework is stable and all i2c drivers are converted to it ...
Later, I wonder whether the concept of a 'current' i2c bus should be
maintained by the command line interpreter, rather than the i2c
system. Because to be honest, most of the drivers I see have to save
the current bus number, set the current bus, do the operation, then
set the bus back how they found it (to preserve whatever the user
things is the current bus).
Yes, suboptimal ... but this is independent from the new i2c framework
patches!
It is possible (with old/new i2c bus framework) to introduce a
"current commandline i2c bus", and then, before calling i2c_read/write
from the commandline, call a i2c_set_bus_num() ... then we can get rid
of this store/restore current i2c bus ... waiting for patches ;-)
OK.
Granted there is overhead with i2c muxes, but the i2c core can
remember the state of these muxes and doesn't have to switch things
until there has been a change since the last transaction.
This exactly do the i2c_set_bus_num() now!
Great.
This last suggestion can be dealt with later, but I thought I would bring
it up.
I am happy about every comment! :-)
Thanks,
Simon
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
