From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Fri, 13 Jan 2012 08:34:18 +0100 Subject: [U-Boot] [PATCH 7/7] tegra: Enable I2C on Seaboard In-Reply-To: References: <1324923111-340-1-git-send-email-sjg@chromium.org> <1324923111-340-8-git-send-email-sjg@chromium.org> <4F0B600C.50507@nvidia.com> <74CDBE0F657A3D45AFBB94109FB122FF17801D1D4A@HQMAIL01.nvidia.com> Message-ID: <4F0FDE7A.3070705@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, Simon Glass wrote: > +U-Boot, which I seemed to have dropped by mistake > > Hi Stephen, > > On Thu, Jan 12, 2012 at 10:59 AM, Stephen Warren wrote: >> Simon Glass wrote at Wednesday, January 11, 2012 8:00 PM: >>> On Mon, Jan 9, 2012 at 1:45 PM, Stephen Warren wrote: >>>> On 12/26/2011 11:11 AM, Simon Glass wrote: >>>>> This enables I2C on Seaboard. >>>> ... >>>>> diff --git a/include/configs/seaboard.h b/include/configs/seaboard.h >>>> ... >>>>> +#define CONFIG_SYS_I2C_INIT_BOARD >>>> I don't think that option is correct for Seaboard; the description in >>>> the README indicates this causes a function named i2c_init_board() to be >>>> called from boards/xxx/board.c, which is supposed to use GPIOs to unhang >>>> the I2C bus. However, this raises a couple of issues: >>>> >>>> 1) Patch 5 in this series calls i2c_init_board() ifdef CONFIG_TEGRA2_I2C >>>> rather than depending on this CONFIG option. >>>> >>>> 2) Tegra's i2c_init_board() doesn't appear to be anything to do with bus >>>> unhanging, but is instead regular I2C initialization. Perhaps the >>>> function should be renamed? >>> I have had a bit of a look at this. From what I can see the problem is >>> that i2c_init() is passed a bus speed, but this is just >>> CONFIG_SYS_I2C_SPEED. We want to control the speed individually for >>> each port. Yes would could use i2c_init() and ignore the speed, but >>> that seems odd also. At least with the way it is we don't ignore the >>> setting. >>> >>> We also don't define CONFIG_HARD_I2C which again seems odd, but I >>> suppose is for the same reason (we don't want to call i2c_init()). >> Hmm. It sounds like we should replace CONFIG_TEGRA2_I2C with >> CONFIG_HARD_I2C given the README description of the latter? > > Well we could, but we would need to ignore the speed argument. Do you > think that is better? Without defining CONFIG_HARD_i2C, i2c_init() never gets called ... naming CONFIG_TEGRA2_I2C is absolutely OK for enabling the tegra i2c driver. >>> Also U-Boot tends to call i2c_init() before every use of i2c, whereas >>> we just want to init once and be done with it. >>> >>> I think the function name is correct, but perhaps I should change the >>> #ifdef you mention in 1 above to CONFIG_SYS_I2C_INIT_BOARD. But for >>> i2c to function on Tegra boards, this must be defined, so in fact this >>> might be counterproductive. So perhaps a check that it is defined is >>> best? >> But README explicitly says that i2c_init_board() is for bus unhanging >> which isn't what the Tegra implementation does. How can the function >> name be correct given that? >> > Well we should rename the function IMO. It seems to me that with a a > name like that it is for initing i2c on the board. Yes, the name is missleading. On the other hand is a deblocking also some sort of "init" ... but I see no problem in using the i2c_init_board() for doing board specific i2c inits too ... maybe the README should be adapted ... but this problem results, that you don't want to use i2c_init() ... >> Don't we just want to make i2c_init() use a global/static variable to >> determine whether the device has already been initialized, and defer all >> the init until first usage or something like that? That seems in line >> with U-Boot's desire not to initialize drivers until they're actually >> used. Yep, that would be inline with that ... but how would you use "i2c reset" command? That only calls i2c_init() ... so, you never can reset/reinit your i2c bus controller. > Actually that might work - if we keep i2c_init() a no-op, and wait > until we get a request for a bus before we look up the fdt and init > that port. But I suspect we might need to init port 0 immediately > since there is no explicit call to i2c_set_bus_num() for that. It's a > little wonky whatever we do. What do you think? Yep, the clean way would be to use the multibus/multiadapter branch: http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2_20111112 but as I said in a previous EMail ... there is some work to do, before it is mainline acceptable ... [...] bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany