From mboxrd@z Thu Jan 1 00:00:00 1970 From: ksi at koi8.net Date: Tue, 16 Dec 2008 17:00:59 -0800 (PST) Subject: [U-Boot] [PATCH v3] i2c: merge all i2c_reg_read() and i2c_reg_write() into inline functions In-Reply-To: <49483DD7.5080908@freescale.com> References: <1228325310-19275-1-git-send-email-timur@freescale.com> <20081215224707.D70FD832E8A1@gemini.denx.de> <4946EB64.5060900@freescale.com> <4947C6E5.3070403@freescale.com> <4947F8B4.8070804@freescale.com> <20081216204954.CF8DA832E8A1@gemini.denx.de> <49483DD7.5080908@freescale.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, 16 Dec 2008, Timur Tabi wrote: > Wolfgang Denk wrote: > >> Hm... what exactly is broken with the concept of having a "current >> device" or a "current bus"? We use it elasewhere, too (like for >> selection IDE or S-ATA devices and such), and so far I am not aware >> of fundamental issues because of that. > > I think it's a kludge because you have to set the current device before > you > can access it. It seems ridiculous that you have to do this: > > i2c_set_bus_num(x) > i2c_write(...) > > when you could do this: > > i2c_write(x, ...) Only if you have more than one I2C bus that 95% of supported boards don't have (95% is a wild guess; in reality, methinks, it is even closer to 99%.) That means you should only do this for less than 5% of existing boards. Adding a parameter would require you to make changes for 100% of boards. common/cmd_i2c.c is a single file, common for all platforms so it is a special case. >>> Sounds to me like you haven't really looked at the U-Boot code. > There are >>> plenty of places where one function does I2C operations, then calls >>> another >>> function that does its own. >> >> Really? Where exactly does such a thing happen? > > Well, I can't find a real example, but here's something close. Take a > look > at function pib_init() in mpc8568mds.c. This function needs to talk to > the > 2nd I2C bus. It has no idea who called it, so it has no idea what the > current I2C bus is. Therefore, it has to save and restore it. > > Now consider the code that calls pib_init(). Technically, this code > cannot > know that pib_init() does I2C operations. So it doesn't know that > pib_init() > could change the current I2C bus. So it wouldn't know to save and > restore > what it needs. > > This situation could happen anywhere. No, that is not an issue. That pib_init() does not happen out of a blue, it is YOU who call it in $(BOARD).c file so you MUST know what you are doing. >> I tend to call this a bug that needs to be fixed, if there is ssuch >> code. > > It's not a bug. Function X does I2C operations, and function Y also > does I2C > operations, but on a different device. If function X calls function Y, > then > Y needs to save and restore the current bus number. You will never get > rid > of this requirement as long as you have the concept of a current I2C bus > number. > > So you're still going to need i2c_get_bus_num() and i2c_set_bus_num(), > and > you're still going to have code like this: > > bus = i2c_get_bus_num(); > i2c_set_bus_num(x); > i2c_write(...) > i2c_set_bus_num(bus); > >> This statement is probably correct: I don't share your point of view >> here, either. > > In that case, I have no interest in working on this new I2C design. You > guys > obviously have everything under control, so good luck. > > -- > Timur Tabi > Linux Kernel Developer @ Freescale > --- ****************************************************************** * KSI at home KOI8 Net < > The impossible we do immediately. * * Las Vegas NV, USA < > Miracles require 24-hour notice. * ******************************************************************