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

[Heiko Schocher <hs@denx.de>]

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 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).

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.

> Prafulla,
> What do you think for this proposal?
> 
> Thanks,
> Lei

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany