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

[Lei Wen]

Hi Heiko,

On Wed, Mar 23, 2011 at 4:53 PM, Heiko Schocher <hs at denx.de> wrote:
> 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).

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