From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Tue, 28 Aug 2012 13:29:00 +0200 Subject: [U-Boot] [PATCH 1/2] i2c:soft:multi: Support for multiple soft I2C buses In-Reply-To: <20120828124002.2629e017@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> Message-ID: <503CAB7C.2080906@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 12:40, Lukasz Majewski wrote: > Hi Heiko, > >>> #if defined(CONFIG_I2C_MULTI_BUS) >>> static unsigned int i2c_bus_num __attribute__ ((section >>> (".data"))) = 0; +const char *soft_i2c_name[CONFIG_SYS_MAX_I2C_BUS] >>> = { >>> + NULL, >>> + NULL, >>> + NULL, >>> + NULL, >>> + "soft_i2c_4", >>> + "soft_i2c_5", >>> + NULL, >>> +}; >> >> For what do you need "soft_i2c_name"? I see no usage of this in your >> patchset? Also why the "NULL" for 0-3 and 6 positions? >> >> And what is, if CONFIG_SYS_MAX_I2C_BUS is< 7 ? > > Indeed this can be removed - it is not needed (for now). Ok. >>> +#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 > +}; > > 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. bye, Heiko -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany