> -----Original Message----- > From: Heiko Schocher [mailto:h...@denx.de] > Sent: Wednesday, March 23, 2011 2:23 PM > To: Lei Wen > Cc: Prafulla Wadaskar; Lei Wen; 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 > > 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).
Hi Lei I can understand your dependency. Even I am fine with that, my intention was if we can make it better why not :-) > > 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. This is a good approach indeed. If you (Lei) agree, I will pull the multibus/multiadapter patches on u-boot-marvell.git which will be a good starting reference for your work. If not, you may continue as you requested. BTW: in his patch, he has renamed and moved boards/Marvell/common/i2c.c to mv_i2c.c. I will be happy to see and test if the patch series by Heiko gets mainlined. Regards.. Prafulla . . _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
