From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Mon, 29 Oct 2012 10:44:27 +0100 Subject: [U-Boot] [PATCH 0/3] Bring in new I2C framework In-Reply-To: References: <1350927621-12481-1-git-send-email-hs@denx.de> <508A2420.2000207@denx.de> Message-ID: <508E4FFB.2010008@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 Simon, On 26.10.2012 18:08, Simon Glass wrote: > On Thu, Oct 25, 2012 at 10:48 PM, Heiko Schocher wrote: >> Hello Simon, >> >> >> On 25.10.2012 23:37, Simon Glass wrote: >>> >>> On Mon, Oct 22, 2012 at 10:40 AM, Heiko Schocher 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