[U-Boot] [PATCH V3 1/5] pxa: move i2c driver to the common place

[Prafulla Wadaskar]

> -----Original Message-----
> From: Heiko Schocher [mailto:hs at denx.de]
> Sent: Wednesday, March 23, 2011 2:23 PM
> To: Lei Wen
> Cc: Prafulla Wadaskar; Lei Wen; Wolfgang Denk; u-boot at 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 <hs at denx.de> wrote:
> >> Hello Prafulla,
> >>
> >> Prafulla Wadaskar wrote:
> >>>> -----Original Message-----
> >>>> From: Lei Wen [mailto:adrian.wenl at gmail.com]
> >>>> Sent: Tuesday, March 22, 2011 6:14 PM
> >>>> To: Prafulla Wadaskar
> >>>> Cc: Lei Wen; Heiko Schocher; Wolfgang Denk; u-boot at 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 . .