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 Redingtreding@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.
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.
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.
Also, if we directly talk to an adapter instead, then the bulk of what i2c_set_bus_num() does isn't even required. It would require that 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=18d673660...
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=14d6b227d689825025f9dfc98...
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 slaves are connected to (either a HW controller directly or a mux connected to a HW controller).
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.
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.
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 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.
Thierry