Hi Heiko, On Thu, Jan 12, 2012 at 11:12 PM, Heiko Schocher <h...@denx.de> wrote: > Hello Stephen, > > Stephen Warren wrote: >> Simon Glass wrote ednesday, January 11, 2012 9:18 PM: >>> On Mon, Jan 9, 2012 at 2:07 PM, Stephen Warren <swar...@nvidia.com> wrote: >>>> On 12/26/2011 11:11 AM, Simon Glass wrote: >>>>> From: Yen Lin <ye...@nvidia.com> >>>>> >>>>> Add basic i2c driver for Tegra2 with 8- and 16-bit address support. >>>>> The driver supports building both with and without CONFIG_OF_CONTROL. >>>>> >>>>> Without CONFIG_OF_CONTROL a number of CONFIG options must be supplied >>>>> in the board config header file: >>>>> >>>>> I2CSPEED_KHZ - speed to run I2C bus at (typically 100000) >>>>> CONFIG_I2Cx_PIN_MUX - pin mux setting for each port (P, 1, 2, 3) >>>>> (typically this will be 0 to bring the port out the common >>>>> pins) >> ... >> >>>> ... >>>>> diff --git a/drivers/i2c/tegra2_i2c.c b/drivers/i2c/tegra2_i2c.c >>>> ... >>>>> +struct i2c_bus i2c_controllers[CONFIG_SYS_MAX_I2C_BUS]; >>>> What if there are I2C bus extenders/muxes/... such that there are more >>>> I2C buses in the system than Tegra I2C controllers? I'd rather see an >>>> explicit TEGRA_I2C_NUM_CONTROLLERS define used throughout this patch. >>> We don't actually support CONFIG_I2C_MUX, so I can't see how that >>> could happen. Can you please explain a bit more? >> >> We may not support it now, but I see no reason we won't in the future. >> If we confuse the two defines now, it'll make it harder to allow muxes >> in the future. The fix is simply using the correct define name within >> the I2C driver isn't it; pretty simple. > > If we really have this case, we *must* get the multibus/multiadapter > branch from here: > > http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2_20111112 > > to uboot mainline! With that approach we could handle this case > in a clean way ... This branch is not in sync with current TOT, and > it has to be tested again, as I didn't found time for this in the > last year :-( Also, as its base is >2 years old, there are a lot > of checkpatch errors in this patchserie, which have to be cleaned > up ... hope I get some time for it ... help is welcome. >
Hmm quite a few patches. I see this email from 2.5 years ago indicating that it was ready but needed more testing: http://lists.denx.de/pipermail/u-boot/2009-July/056726.html 2a083b7 i2c: get rid of HARD_i2C ccb680c i2c: add dynamic bus allocation b68480a i2c, u8500: added new multibus/multiadapter support 692f037 i2c, marvell: added new multibus/multiadapter support 3b0258c i2c, spear: added new multibus/multiadapter support a06deff i2c, at91rm9200: added new multibus/multiadapter support 0758f6b i2c, kirkwood: added new multibus/multiadapter support 7919863 i2c, mv: added new multibus/multiadapter support b164243 i2c, mpc824x: added new multibus/multiadapter support a5f8bd1 i2c, tsi108: added new multibus/multiadapter support 9db40d8 i2c, s3c44b0: added new multibus/multiadapter support 67b95ef i2c, s3c24x0: added new multibus/multiadapter support 6b2c3f5 i2c, omap24xx: added new multibus/multiadapter support 1f8ffed i2c, omap1510: added new multibus/multiadapter support c41757c i2c, mxc: added new multibus/multiadapter support 5c22159 i2c, davinci: added new multibus/multiadapter support 3dd28ab i2c, bfin: added new multibus/multiadapter support 3d1c119 i2c, mpc8220: added new multibus/multiadapter support 8f4d062 i2c, mpc512x: added new multibus/multiadapter support 1bd40d4 i2c, mpc5xxx: added new multibus/multiadapter support 6225ccb i2c, 8xx: added new multibus/multiadapter support d94a632 i2c, ppc4xx_i2c: added new multibus/multiadapter support c34a705 i2c, mpc8260_i2c: added new multibus/multiadapter support 832b409 i2c, fsl_i2c: switch to new multibus/multiadapter support 06de378 i2c, soft-i2c: switch to new multibus/multiadapter support e47115f i2c, common: common changes for multibus/multiadapter support d7a70d7 i2c: add i2c_core and prepare for new multibus support 489 files changed, 8512 insertions(+), 7063 deletions(-) checkpatch.pl found 295 error(s), 653 warning(s) I have had a look through the patches. I agree that i2c needs a clean up and this seems to fit the bill. How can we get this committed? The first two commits aren't too hard to bring over but it doesn't actually build. I could perhaps have a look at getting it to build correctly with and without CONFIG_SYS_I2C, and see if I can get the latter working on Tegra. Then perhaps we could add the new support as just another option so we have CONFIG_HARD_I2C, CONFIG_SOFT_I2C and CONFIG_SYS_I2C. That might be a start, and drivers can move over to the new system. What do you think? >>>>> +int i2c_init_board(void) >>>>> +{ > [...] >>>> Surely that function needs to actually do something, at least set up the >>>> clocks so that the (user?) requested rate is honored, or print an error >>>> message if you're only allowed to use the hard-coded bus rate. >>> See my other message. I suppose we could reinit, but we really don't >>> want to honour the speed, since the fdt speed setting is then lost and >>> irrecoverable. For now it feels like we should ignore it. >> >> Hmm. I suspect the answer here is roughly to override the following in >> cmd_i2c.c: >> >> /* TODO: Implement architecture-specific get/set functions */ >> unsigned int __def_i2c_get_bus_speed(void) >> { >> return CONFIG_SYS_I2C_SPEED; >> } >> unsigned int i2c_get_bus_speed(void) >> __attribute__((weak, alias("__def_i2c_get_bus_speed"))); >> >> int __def_i2c_set_bus_speed(unsigned int speed) >> { >> if (speed != CONFIG_SYS_I2C_SPEED) >> return -1; >> >> return 0; >> } >> int i2c_set_bus_speed(unsigned int) >> __attribute__((weak, alias("__def_i2c_set_bus_speed"))); >> >> To actually read/write the rate in use by the driver. > > Yep, for this reason this functions are weak, and you can define > a driver specific function which returns the current used settings > in the driver. > > Code should compile, if tegra2 not define CONFIG_SYS_I2C_SPEED, > as a "default" value for CONFIG_SYS_I2C_SPEED is defined in > include/i2c.h ... > >> Then, fix do_i2c_reset() to use i2c_get_bus_speed() so it interacts >> correctly with those functions. > > Yep, I think, you are right here, that should be fixed. As the > default function for i2c_get_bus_speed returns CONFIG_SYS_I2C_SPEED, > that should be OK. Yes ok I will take a look. Regards, Simon > >> There may be other places that need to be updates to use those function >> instead of hard-coding CONFIG_SYS_I2C_SPEED too. Perhaps we could even >> get away without defining CONFIG_SYS_I2C_SPEED for Tegra since it's >> meaningless. Instead, ifdef those default function definitions above >> based on whether CONFIG_SYS_I2C_SPEED is defined or not. >> > > bye, > Heiko > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
