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

[Prafulla Wadaskar]

> -----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.
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
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.
3. all I2C driver files should be in drivers/i2c/
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)
5. Flags must be provided for i2c_read/write APIs to have precise control to execute I2C_START/I2C_STOP sequence in the call.

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?

Regards..
Prafulla . .