| Message ID | 1346142797-17645-2-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Heiko Schocher |
| Headers | show |
Hello Lukasz, On 28.08.2012 10:33, Lukasz Majewski wrote: > Support for multiple soft I2C buses at soft_i2c.c > > This approach defines get_multi_{sda|scl}_pin functions to switch > between multiple "soft" I2C buses. > > Up to CONFIG_SYS_MAX_I2C_BUS devices can be utilized. > Common definition of I2C_X I2C buses is provided. > > TEST HW: > Samsung's Exynos4210 evt.0.1 - Trats development board > > Signed-off-by: Lukasz Majewski<l.majewski@samsung.com> > Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com> > --- > drivers/i2c/soft_i2c.c | 41 +++++++++++++++++++++++++++++++++++++++++ > include/i2c.h | 17 +++++++++++++++++ > 2 files changed, 58 insertions(+), 0 deletions(-) > > diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c > index 36c6114..7901f04 100644 > --- a/drivers/i2c/soft_i2c.c > +++ b/drivers/i2c/soft_i2c.c > @@ -127,6 +127,15 @@ DECLARE_GLOBAL_DATA_PTR; > > #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 ? > #endif /* CONFIG_I2C_MULTI_BUS */ > > /*----------------------------------------------------------------------- > @@ -482,3 +491,35 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) > send_stop(); > return(failures); > } > + > +#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. > diff --git a/include/i2c.h b/include/i2c.h > index 1f35acf..d563d62 100644 > --- a/include/i2c.h > +++ b/include/i2c.h > @@ -250,4 +250,21 @@ static inline void I2C_SET_BUS(unsigned int bus) > i2c_set_bus_num(bus); > } > > +/* Multi I2C busses handling */ > +#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 > +}; > + > +extern const char *soft_i2c_name[CONFIG_SYS_MAX_I2C_BUS]; > +extern int get_multi_scl_pin(void); > +extern int get_multi_sda_pin(void); > +extern int multi_i2c_init(void); > +#endif > #endif /* _I2C_H_ */ bye, Heiko
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). > > +#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()). What here I'm trying to avoid is the code duplication for each board (e.g. Samsung's GONI, Universal, Trats, Origen ... etc). I can agree, that now only I2C_{4|5} are defined (since for now Samsung is using I2C_4 and I2C_5). But other cases can be also defined. What I see even more important is a definition of (at <i2c.h>): +#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 <i2c.h> 2. Add #define I2C_X on the ./include/configs/{e.g. trats}.h board. For second approach used I need to duplicate the code for other targets (goni, universal, origen) when needed and I cannot avoid that someone else will define other names -> like #define MINE_I2C_X on his/her ./include/configs/{board}.h
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<i2c.h>): > > +#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<i2c.h> > 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
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<i2c.h>): > > > > +#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, then we can define at <i2c.h> : +#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). > > 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<i2c.h> > > 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 However, I think, that it would be good to add following declarations to <i2c.h>: extern int get_multi_scl_pin(void); extern int get_multi_sda_pin(void); extern int multi_i2c_init(void); ,which can be defined on different platforms. What is your opinion about that?
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<i2c.h>): >>> >>> +#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<i2c.h> : > > +#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<i2c.h> >>> 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 > <i2c.h>: > > 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
Hi Heiko, > 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<i2c.h>): > >>> > >>> +#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! Ok, so we have agreed. > > > then we can define at<i2c.h> : > > > > +#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. Nice, > > >>> 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<i2c.h> > >>> 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 <i2c.h>: > > > > 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! Ok, I need this.
diff --git a/drivers/i2c/soft_i2c.c b/drivers/i2c/soft_i2c.c index 36c6114..7901f04 100644 --- a/drivers/i2c/soft_i2c.c +++ b/drivers/i2c/soft_i2c.c @@ -127,6 +127,15 @@ DECLARE_GLOBAL_DATA_PTR; #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, +}; #endif /* CONFIG_I2C_MULTI_BUS */ /*----------------------------------------------------------------------- @@ -482,3 +491,35 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) send_stop(); return(failures); } + +#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 */ diff --git a/include/i2c.h b/include/i2c.h index 1f35acf..d563d62 100644 --- a/include/i2c.h +++ b/include/i2c.h @@ -250,4 +250,21 @@ static inline void I2C_SET_BUS(unsigned int bus) i2c_set_bus_num(bus); } +/* Multi I2C busses handling */ +#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 +}; + +extern const char *soft_i2c_name[CONFIG_SYS_MAX_I2C_BUS]; +extern int get_multi_scl_pin(void); +extern int get_multi_sda_pin(void); +extern int multi_i2c_init(void); +#endif #endif /* _I2C_H_ */
Support for multiple soft I2C buses at soft_i2c.c This approach defines get_multi_{sda|scl}_pin functions to switch between multiple "soft" I2C buses. Up to CONFIG_SYS_MAX_I2C_BUS devices can be utilized. Common definition of I2C_X I2C buses is provided. TEST HW: Samsung's Exynos4210 evt.0.1 - Trats development board Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> --- drivers/i2c/soft_i2c.c | 41 +++++++++++++++++++++++++++++++++++++++++ include/i2c.h | 17 +++++++++++++++++ 2 files changed, 58 insertions(+), 0 deletions(-)