On Wed, Aug 27, 2014 at 11:56:41AM +0200, Heiko Schocher wrote:
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 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.
You again just talk from i2c_adapter ... why you ignore i2c muxes? A bus is not only an i2c adapter ...
I know. I keep saying i2c_adapter because in the rough sketch that I have in mind for how this could work eventually, an mux is just another special kind of 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 can look into it, but it sounds like a task way beyond what I have time for right now.
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 ...
Or if said functionality is encapsulated within adapters then they can be done as necessary with the same effect.
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?
It's currently done as part of i2c_adapter_get() which as you already point out only work when muxes aren't used. But the main issue about the current approach is that we have these global indices and pass them around in global state and need to be careful to always set the current bus where appropriate.
If we simply pass adapters around (provided they handle muxes correctly) we can simply address each slave as an (adapter, slave address) pair, rather than repeating the same arguments over and over and calling very low-level bus management functions.
But we already agree that this isn't optimal. The disagreement seems to be mostly about the implementation details.
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 ...
Correct.
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 ...
Yes, that sounds like a useful change in itself. But it still requires users to explicitly manage the bus and chip numbers. The goal of the i2c_client API is to standardize on how to talk to clients. Essentially the goal would be for a driver to get a hold of a struct i2c_client via some standard interface (perhaps with DM this could be done completely outside of the driver itself) and then talks to the device via this standard object. Currently drivers either need to hardcode the bus number and slave addresses, or they need to store them in some custom structure. i2c_client gives uniformity to that.
There are really two parts of a puzzle here. One is the client-side API that drivers for slave devices use and the other is the I2C controller drivers that provide the struct i2c_adapter.
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 :-(
It's done as part of the i2c_adapter_get(). It takes an index (which is roughly the bus number) initializes the corresponding adapter (if found) and returns a pointer to it.
To handle muxes I think this could work by adding hooks that could be called before doing transactions to make sure that the correct muxing is set up.
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
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!
Yes, perhaps DM is part of the solution. But I don't have any concrete plans. I'd rather tackle this step by step rather than as one monolithic patch series.
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" ...
This would probably work fine to approach the problem from two sides. An API like the i2c_client could be used to abstract away all the implementation details on the controller/mux side and then more work could be done to convert the adapter side of things to whatever works best.
Thierry