Hello Thierry,
Am 27.08.2014 10:51, schrieb Thierry Reding:
On Wed, Aug 27, 2014 at 09:07:58AM +0200, Heiko Schocher wrote:
Hello Thierry,
Am 27.08.2014 08:21, schrieb Thierry Reding:
On Wed, Aug 27, 2014 at 07:21:51AM +0200, Heiko Schocher wrote:
Hello Thierry,
Am 26.08.2014 17:34, schrieb Thierry Reding:
From: Thierry Reding<tred...@nvidia.com>
This API operates on I2C adapters or I2C clients (a new type of object
which is a bad idea ...
that refers to a particular slave connected to an adapter). This is
useful to avoid having to call i2c_set_bus_num() whenever a device is
being accessed.
But thats the supose of i2c_set_bus_num()! ... if you use an i2c bus,
you must check before every access, if you are on it, if not, you must
switch back to it...
That's not what code does today. Everybody calls i2c_set_bus_num() once
and then does a bunch of transactions on that bus. Given that U-Boot
Yes, sadly. This has historical reasons ...
doesn't run multithreaded this works. If you're really concerned about
Yes, U-Boot is singlethread only.
this being a problem, then it should be solved at a different level. It
could be part of i2c_client for example, so that i2c_client_read() and
i2c_client_write() would always perform this step. Burdening users with
Exactly, thats right, and this is a goal from the CONFIG_SYS_I2C API!
But why do you introduce i2c_client_read/write and do not add this step
to i2c_read/write?
- convert all i2c drivers, which are not yet converted to CONFIG_SYS_I2C
(get also rid od CONFIG_HARD_I2C)
- add busnumber to i2c_read/write API and make i2c_set_bus_num() static ...
and fix all i2c_read/write() calls in U-Boot code ...
I don't think adding a bus number as parameter is useful. Why not just
use the I2C adapter directly? That way we don't have to keep looking it
up in an array every time.
You again just talk from i2c_adapter ... why you ignore i2c muxes?
A bus is not only an i2c adapter ...
Currently we have two "versions" of i2c_adapter:
In a system without muxes, you can say: i2c bus == i2c adapter
but in a system with muxes we have: i2c bus == i2c_bus_hose
i2c commands use also a "bus number" starting from 0 ... the bus number
has historical reasons... we could change this ...
But if we introduce a new API, please with mux functionallity ...
Hmm.. thinking about it ... if you want to introduce a new API, please
start with using the DM for I2C!
I know, this is a big change and a lot of work ... thats the reason
why we are not at this point ... nobody volunteered to go forward, and
I did not found time to do it ...
I suppose that would be one possibility to do it. But I consider
i2c_client more of a convenience around the lower-level i2c_read() and
i2c_write(). The idea is that users set up an I2C client once and then
refer to the client, which will automatically use the correct adapter
and slave address rather than having that duplicated in every driver.
Hmm... Ok, if we want to have a i2c_client struct instead an int ...
What do others think?
But, if we(you ;-) touch this, please start with using the DM for I2C!
This is also a step which should be done, and I do not want to have another
API, if somedays we find time to switch to DM!
this isn't going to work (in a multithreaded environment the switch to a
different mux could happen between the call to i2c_set_bus_num() and the
bus access).
In fact I think this would even have to be solved at the controller
level if you want to make sure that client transactions are atomic.
As U-Boot is single threaded all i2c_read/writes are atomic.
In which case you don't have to call i2c_set_bus_num() for every access,
only whenever you don't know exactly where you're coming from. Functions
that perform a sequence of accesses only need to set it once.
Yes ... which really is a pro for having i2c_set_bus_num() not static ...
Also, if we directly talk to an adapter instead, then the bulk of what
You ignore again muxes ...
i2c_set_bus_num() does isn't even required. It would require that
What does i2c_set_bus_num() ?
- first check, if current bus = new bus, and if it is initialized
if so -> all is good, return!
Thats the main case ... is this so expensive? This checks should
be always done (except we do a bulk of i2c accesses, yes). I think,
this has also to be done somewhere with your approach ... or?
What is done in the case, we switch to another bus:
- If we have no muxes, save new bus for further use
- look if new bus is initialized, if not initialize it
All this is hidden from the user ... and actions, which must be done,
whatever API you define ...
If I understand you correct, your big change is not using "int bus"
instead introducing i2c_client ... and define some functions, which
use this struct as parameter ...
Why not defining:
[1]:
int i2c_bus_read(int bus, uint8_t chip,
unsigned int address, size_t alen, void *buffer,
size_t size)
{
i2c_set_bus_num(bus);
return i2c_read(chip, address, alen, buffer, size);
}
int i2c_bus_write(int bus, uint8_t chip,
unsigned int address, size_t alen, void *buffer,
size_t size)
{
i2c_set_bus_num(bus);
return i2c_write(chip, address, alen, buffer, size);
}
and you never have to call something like i2c_init(), as
i2c_set_bus_num() does this all for you! You only have to use in your
driver i2c_bus_read/write ... without taking any attention ...
looking deeper in your approach:
+int i2c_client_init(struct i2c_client *client, struct i2c_adapter *adapter,
+ uint8_t address)
+{
+ client->adapter = adapter;
+ client->address = address;
+
+ return 0;
+}
Where is here called adapter->init() ? Nor you check, if the i2c
adapter is intialized ... if more than one driver uses this function,
each intializes the hw? Or currently none :-(
adapters are made aware of a hierarchy if there are muxes, but I think
that's worthwhile to do in any case. Also if ever I2C muxing needs to
gain device tree support having the muxes set up dynamically will be
pretty much a prerequisite.
This is collected in i2c_set_bus_num() ... before, every "user" did
this on his own ... if you are on the bus you want to access, the
overhead is not so big, see:
http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/i2c_core.c;h=18d6736601c161f45cb7d81b5eae53bdeaaf6b0b;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l278
278 int i2c_set_bus_num(unsigned int bus)
279 {
280 int max;
281
282 if ((bus == I2C_BUS)&& (I2C_ADAP->init_done> 0))
283 return 0;
And you must be aware of i2c muxes! You directly use the read/write
functions from the i2c adapter, but what is if you have i2c muxes?
That's complexity that users shouldn't have to worry about. They should
Exactly!
simply access an adapter and the adapter (or rather the core) should
take care of setting up any muxes correctly.
Yes!
I think you mix here i2c adapter with bus. An "U-Boot i2c adapter" is a
hw adapter (or special case soft i2c adapter). An "i2c bus" is a hw adapter
maybe with m muxes, and each bus has exactly one way through the
i2c muxes, see for an example the README:
http://git.denx.de/?p=u-boot.git;a=blob;f=README;h=14d6b227d689825025f9dfc98fb305021882446d;hb=7bee1c91a94db19bd26f92cc67be35d3592c6429#l2349
So the only thing a User must know when he wants to use an i2c bus is
his number. The switching to this i2c adapter, initializing it and maybe
set i2c muxes does the i2c subsystem ...
The above doesn't preclude an I2C adapter representing one of the ports
of a mux. That way you can still talk to an adapter rather than having
to refer to the bus by number. Adapter would become a little more
abstract than it is now, since it would be simply an output that I2C
Oh!
slaves are connected to (either a HW controller directly or a mux
connected to a HW controller).
Thats sounds for me, that you should use DM ... I do not know, what
your plans are!
Maybe there is on one i2c adapter a i2c mux with 4 ports. On one is
an eeprom, on the other is a PMIC ... your code in patch
"power: Add AMS AS3722 PMIC support" does access with your functions
the PMIC ... what is, if between this accesses someone accesses the eeprom?
If he switches the mux, you never switch back!
Your code did not check this!
Like I said, a lot of code in U-Boot doesn't check this. And quite
With using i2c_set_bus_num() you have not to check this! You only have
to call i2c_set_bus_num() before calling i2c_read/write ... and yes,
that would be nice, if we just pass the bus number to i2c_read/write()
and drop the i2c_set_bus_num() call all over the code ...
Patches welcome!
How about a slightly different proposal: introduce a new level of
abstraction (like i2c_client) and start using it in new I2C slave
drivers. At the same time existing drivers could be converted one at a
time without having the big flag date when i2c_read() and i2c_write()
are switched over all at once.
Sound like use/introduce DM for I2C!
When that new level of abstraction is used, we can hide all the
details behind that and the implementation no longer influences any of
the drivers. Then we could transparently rework adapters and muxes to
our heart's content without needing to update users of the high-level
API.
Ok, with some functions like in [1], maybe you introduce i2c_client,
and use this instead "int bus" ...
frankly as long as this isn't handled in the core I don't think people
will get it right.
Yes, full ack, which is the goal from CONFIG_SYS_I2C. If all i2c
driver are converted to it, we can make i2c_set_bus_num() static, and
add to the i2c API the bus number as a function parameter!
Why is i2c_set_bus_num() such a problem?
Because it's completely confusing. And it's exposing an implementation
detail to users instead of handling it transparently in the core.
Yes! Full Ack ... but I do not accept a new API for that! Please
fix the i2c_read/write() functions!
Doing this kind of conversion is a nightmare. We'd be changing an API
Full Ack.
that has around 600 occurrences in U-Boot, all of which need to be
changed *at once* to avoid build breakage. At the same time we need to
make sure any patches in development use the same API, which means that
they can't even be build-tested until the the API has been changed.
Transitioning step by step is a lot less complicated.
Ok, it was worth a try ;-)
So what do you think about defining functions like in [1] ?
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
