Hi Heiko, On Wed, Mar 23, 2011 at 4:53 PM, Heiko Schocher <h...@denx.de> wrote: > 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).
Ok, I would post another round of patch to fix those issue Prafulla and Wolfgang metioned. > > 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. Currently, I have no bandwidth to do this... :( Maybe I would try to investigate when I have time. Best regards, Lei _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot