-----Original Message----- From: Heiko Schocher [mailto:hs@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 hs@denx.de wrote:
Hello Prafulla,
Prafulla Wadaskar wrote:
-----Original Message----- From: Lei Wen [mailto:adrian.wenl@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
- 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!
- Secondly all these drivers have some common code, mostly
i2c_read, i2c_write, i2c_probe, etc..
- 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.
- 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.
- 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.
- 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.
- all I2C driver files should be in drivers/i2c/
Yep.
- 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?
- 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:
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 . .