From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Tue, 28 Aug 2012 14:25:43 +0200 Subject: [U-Boot] [PATCH 1/2] i2c:soft:multi: Support for multiple soft I2C buses In-Reply-To: <20120828141226.42183f6c@amdc308.digital.local> References: <1346142797-17645-1-git-send-email-l.majewski@samsung.com> <1346142797-17645-2-git-send-email-l.majewski@samsung.com> <503C88C7.9010707@denx.de> <20120828124002.2629e017@amdc308.digital.local> <503CAB7C.2080906@denx.de> <20120828141226.42183f6c@amdc308.digital.local> Message-ID: <503CB8C7.8010607@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Lukasz, On 28.08.2012 14:12, Lukasz Majewski wrote: > Hi Heiko, > > >>>>> +#if defined(CONFIG_I2C_MULTI_BUS) >>>>> +/* Handle multiple I2C buses instances */ >>>>> +int get_multi_scl_pin(void) >>>>> +{ >>>>> + switch (I2C_GET_BUS()) { >>>>> + case I2C_4: >>>>> + return CONFIG_SOFT_I2C_I2C4_SCL; >>>>> + case I2C_5: >>>>> + return CONFIG_SOFT_I2C_I2C5_SCL; >>>>> + }; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +int get_multi_sda_pin(void) >>>>> +{ >>>>> + switch (I2C_GET_BUS()) { >>>>> + case I2C_4: >>>>> + return CONFIG_SOFT_I2C_I2C4_SDA; >>>>> + case I2C_5: >>>>> + return CONFIG_SOFT_I2C_I2C5_SDA; >>>>> + }; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +int multi_i2c_init(void) >>>>> +{ >>>>> + return 0; >>>>> +} >>>>> +#endif /* CONFIG_I2C_MULTI_BUS */ >>>> >>>> Again, what is with busnr = 0-3 or 6? >>>> >>>> This is not needed in the i2c soft file. You can define this >>>> functions board specific ... so, no change in the driver is >>>> needed ... please move this to board specific code. >>> >>> Please consider, that get_multi_{sda|scl}_pin can be used by other >>> boards. Those are written in a generic way (by calling >>> I2C_GET_BUS()). >> >> Got this, but why do you index them with 4 and 5 and not with 0 and 1? >> What is, if another board uses 0 and 1, so this would introduce >> the defines CONFIG_SOFT_I2C_I2C0_SDA and CONFIG_SOFT_I2C_I2C1_SDA >> in the "common" get_multi_sda_pin(), which leads in compilererror for >> your board(s) ... your proposed get_multi_sda_pin() is currently >> samsung specific ... >> >>> What here I'm trying to avoid is the code duplication for each board >>> (e.g. Samsung's GONI, Universal, Trats, Origen ... etc). >> >> If they use all the same function, they should end in >> a ../samsung/common/common.c because, currently your functions are >> samsung specific. >> >> common is (from my point of view) that we add in the board config >> file: >> >> +#define CONFIG_SOFT_I2C_GPIO_SCL get_multi_scl_pin() >> +#define CONFIG_SOFT_I2C_GPIO_SDA get_multi_sda_pin() >> >>> I can agree, that now only I2C_{4|5} are defined (since for now >>> Samsung is using I2C_4 and I2C_5). >> >> and thats samsung specific ... because other boards maybe start >> with I2C_0 ... and this case is not respected in your patch. >> >>> But other cases can be also defined. >> >> Yep, and break compiling your board, as this defines are not >> specified. >> >>> What I see even more important is a definition of (at): >>> >>> +#if (defined(CONFIG_SOFT_I2C)&& defined(CONFIG_I2C_MULTI_BUS)) >>> +enum { >>> + I2C_0, >>> + I2C_1, >>> + I2C_2, >>> + I2C_3, >>> + I2C_4, >>> + I2C_5, >>> + I2C_6 >>> +}; >>> > > I would like to propose that, I will rename the I2C_4 -> I2C_0 and > I2C_5 -> I2C_1, Yep! > then we can define at : > > +#if (defined(CONFIG_SOFT_I2C)&& defined(CONFIG_I2C_MULTI_BUS)) > +enum { > + I2C_0, > + I2C_1, > +}; > > And this would facilitate handling of SOFT_I2C numbering across > relevant subsystems (e.g. PMICs and other). Ok. >>> since this will organize the order of multiple (soft) I2C devices. >>> >>> Imagine that 2 PMICs are on the board (I2C_4 and I2C_5). I need to >>> distinct those (when calling I2C_SET|GET_BUS) >>> And then support for another I2C device (e.g. I2C_2) at other >>> subsystem is provided. >>> Then I can: >>> 1. Add common definition of I2C_X (as I've proposed) to >>> 2. Add #define I2C_X on the ./include/configs/{e.g. trats}.h board. >> >> Why add "#define I2C_X" in ./include/configs/{e.g. trats}.h ? >> I don?t understand this ... and you do not this in your >> patchserie! >> >>> For second approach used I need to duplicate the code for other >>> targets (goni, universal, origen) when needed and I cannot avoid >>> that someone >> >> or make a ../samsung/common/common.c until they are samsung >> specific. >> >>> else will define other names -> like #define MINE_I2C_X on >>> his/her ./include/configs/{board}.h >> >> Ok, but if you use I2C_4 and I2C_5, you must also define the I2C_0, >> I2C_1, I2C_2 and I2C_3 cases in the "get_multi_*" functions, as other >> boards would start with I2C_0 ... >> >> ... and add a documentation in README for this ... >> >> but I mislike to introduce such a lot of defines ... instead of >> defining get_multi_*() board/manufacturer/soc specific ... Maybe >> there is a board with 10 i2c soft busses, so we must define in all >> boards using soft multibus this 20 >> (CONFIG_SOFT_I2C_I2C*_SCL/SDA)defines ... or at least define them if >> not defined in include/i2c.h ... bad. >> > > I will move the "get_multi_*" functions to ../samsung/common/common.c Good. > However, I think, that it would be good to add following declarations to > : > > extern int get_multi_scl_pin(void); > extern int get_multi_sda_pin(void); > extern int multi_i2c_init(void); In the case CONFIG_I2C_MULTI_BUS is defined. > ,which can be defined on different platforms. > > What is your opinion about that? I agree with this! bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany