Hello Lei, Lei Wen wrote: > Hi Heiko, > > On Wed, Mar 23, 2011 at 4:22 PM, Heiko Schocher <h...@denx.de> wrote: >> Hello Prafulla, >> >> Prafulla Wadaskar wrote: >>>> -----Original Message----- >>>> From: Lei Wen [mailto:adrian.w...@gmail.com] >>>> Sent: Tuesday, March 22, 2011 6:14 PM >>>> To: Prafulla Wadaskar >>>> Cc: Lei Wen; Heiko Schocher; Wolfgang Denk; u-boot@lists.denx.de; Marek >>>> Vasut; Ashish Karkare; Prabhanjan Sarnaik; Yu Tang >>>> Subject: Re: [PATCH V3 1/5] pxa: move i2c driver to the common place >>>> >>> ...snip... >>>>>> drivers/i2c/Makefile | 1 + >>>>>> drivers/i2c/mv_i2c.c | 452 >>>>>> +++++++++++++++++++++++++++++++++++++++++++ >>>>>> include/configs/innokom.h | 1 + >>>>>> include/configs/xm250.h | 1 + >>>>>> 6 files changed, 455 insertions(+), 470 deletions(-) >>>>>> delete mode 100644 arch/arm/cpu/pxa/i2c.c >>>>>> create mode 100644 drivers/i2c/mv_i2c.c >>>>> ...snip... >>>>> >>>>>> -#endif /* CONFIG_HARD_I2C */ >>>>>> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile >>>>>> index 052fe36..00a12cc 100644 >>>>>> --- a/drivers/i2c/Makefile >>>>>> +++ b/drivers/i2c/Makefile >>>>>> @@ -29,6 +29,7 @@ COBJS-$(CONFIG_BFIN_TWI_I2C) += bfin-twi_i2c.o >>>>>> COBJS-$(CONFIG_DRIVER_DAVINCI_I2C) += davinci_i2c.o >>>>>> COBJS-$(CONFIG_FSL_I2C) += fsl_i2c.o >>>>>> COBJS-$(CONFIG_I2C_MVTWSI) += mvtwsi.o >>>>>> +COBJS-$(CONFIG_I2C_MV) += mv_i2c.o >>>>> Mvtwsi and mv_i2c are two i2c drivers for Marvell. >>>>> Can you merge these two? >>>> As I explain to you before. Although kirkwood and pxa series are both >>>> the product >>>> of Marvell, but it don't necessary means that they must have the same >>>> controller >>>> for both product line. For the i2c part, they just use two different >>>> controller. >>>> So why you keep request merge those two? Do you mean you want to >>>> create a unique I2C >>>> framework for whole i2c drivers in drivers/i2c? >>> Hi Lei >>> >>> 1. Most of i2c drivers supported in u-boot are either in SoC specific >>> folder or in drivers/i2c folder, there is no as such thumb rule here. >> New drivers should go to drivers/i2c ! >> The existing (old) drivers are just not moved ... >> patches welcome! >> >>> 2. Secondly all these drivers have some common code, mostly i2c_read, >>> i2c_write, i2c_probe, etc.. >>> 3. Specific to Marvell, we already have mvtwsi.c that supports Kirkwood and >>> Orion5X SoCs. Whereas you are adding new mvi2c.c that will support >>> armada100, pantheon apart from pxa. >>> 4. What about if we need to support some new Marvell SoC with different i2C >>> controller? Do we add one more driver? >>> >>> I would love if some one creates drivers/i2c/i2c_core.c??? not necessarily >>> you ;-) >>> >>> Here is what I would like to suggest. >>> 1. cmd_i2c mostly interfaced with i2c_probe, i2c_read, i2c_write, >>> i2c_get_bun_num, i2c_set_bus_num, those should go in drivers/i2c_core.c >> Yep, but see below comment. >> >>> 2. APIs like i2c_start, i2c_stop, i2c_send, i2c_recv, i2c_reset are more >>> I2C controller specific and those will be different implementation on >>> different SoCs, those can go in SoC specific i2c driver file. >> Yep. >> >>> 3. all I2C driver files should be in drivers/i2c/ >> Yep. >> >>> 4. i2c_read/write API need to be redefined since those are not generic to >>> be used to access any I2C peripheral( most of the device don't need address >>> to be programmed) >> With which devices do you have problems? You can set with >> "i2c mw chip address.0 ..." an addresslen = 0 ... or? >> >>> 5. Flags must be provided for i2c_read/write APIs to have precise control >>> to execute I2C_START/I2C_STOP sequence in the call. >> If needed, yes. >> >>> Since you are the one starting with re-using pxa driver for armada100 and >>> Pantheon SoC, why don't you split it into i2c_core.c and i2c_pxa.c? then >>> add i2c_armada100.c and i2c_pantheon.c? >>> Others can migrate in the similar way. (even mvtwsi,c) >>> >>> Hi Heiko >>> What do you think on this? >> I made such a i2c_core.c file in the multibus/multiadapter branch >> for the i2c subsystem, see here: >> >> http://git.denx.de/?p=u-boot/u-boot-i2c.git;a=shortlog;h=refs/heads/multibus_v2 >> >> (also you can grep in u-boot ML for discussions about) >> >> Actual state: >> - arm boards: i2c driver tested on suen3 (kirkwood based board) >> - powerpc boards: i2c driver tested on mpc82xx, 83xx, 8xx boards >> - all others just coded not tested ... >> (A result of lacking hw and/or time) >> >> ToDo: >> - rebase against current head >> (Sorry, didn;t found time to rebase it since Oktober 2010) >> - Update README >> - porting arrived new i2c drivers, boards since Oktober 2010 >> to this new i2c approach >> - testing, testing, testing ... Testers welcome! >> >> I prefer to integrate this to mainline, before we do above steps >> (4?) and 5. As Lei mentioned, if a soc/board has different i2c >> controllers and more than one bus we *need* this approach, >> so it is not worthwhile to introduce a i2c_core file only ... >> instead we should forwarding this branch to mainline? >> >> Patches are welcome ;-) >> >> I am afraid, this would get such a big cut as the arm relocation >> changes ... and it affects all archs. > > It is certainly a big change for introduce the i2c-core framework. :) > > Also my incoming mmc/sd enabling patch for pantheon and armada100 > is also based on this i2c enabling patch, as I need the i2c to turn on the > repsonding pmic power connection. > > While we could get a i2c working pantheon, armada100, and other pxa > series platform now with this patch set. So what about Could we merge > this first, and > gradually change to the i2c framework, test and make it mature.
I am fine with that, but please address the other comments from Prafulla and Wolfgang (rename defines, use standard accessors). If you plan to investigate time for the multibus/multiadapter i2c branch, let me know, maybe I can rebase this branch before you use it. > Prafulla, > What do you think for this proposal? > > Thanks, > Lei 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