From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Wed, 27 Aug 2014 11:56:41 +0200 Subject: [U-Boot] [PATCH v2 13/40] i2c: Add high-level API In-Reply-To: <20140827085057.GJ15640@ulmo> References: <1409067268-956-1-git-send-email-thierry.reding@gmail.com> <1409067268-956-14-git-send-email-thierry.reding@gmail.com> <53FD6AEF.1000109@denx.de> <20140827062124.GE15640@ulmo> <53FD83CE.3050105@denx.de> <20140827085057.GJ15640@ulmo> Message-ID: <53FDAB59.50800@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>> >>>>> 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