| Message ID | 1350343930-15648-3-git-send-email-festevam@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
Am 16/10/2012 01:32, schrieb Fabio Estevam: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Introduce CONFIG_SYS_FSL_PMIC_I2C_LENGTH to configure the number of bytes > that are used to communicate with the PMIC via I2C. > > Instead of hardcoding the value, pass the number via a config option. > > This will be useful for adding support for PMIC MC34704 from Freescale, which > uses only one byte in its I2C protocol. > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- Hi Fabio, > Changes since v2: > - No changes. Newly introduced in this series > > drivers/misc/pmic_fsl.c | 2 +- > include/configs/mx35pdk.h | 1 + > include/configs/mx53evk.h | 1 + > include/configs/mx53loco.h | 1 + > 4 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/misc/pmic_fsl.c b/drivers/misc/pmic_fsl.c > index 0ff75ed..40c448b 100644 > --- a/drivers/misc/pmic_fsl.c > +++ b/drivers/misc/pmic_fsl.c > @@ -53,7 +53,7 @@ int pmic_init(void) > #elif defined(CONFIG_PMIC_I2C) > p->interface = PMIC_I2C; > p->hw.i2c.addr = CONFIG_SYS_FSL_PMIC_I2C_ADDR; > - p->hw.i2c.tx_num = 3; > + p->hw.i2c.tx_num = CONFIG_SYS_FSL_PMIC_I2C_LENGTH; The bad thing with it is that it seems that each board can have a different value. However, this is bound to the selected pmic and not to the board. So IMHO it should not go into the board configuration file, but in the pmic specific initialization. We share the same general code for all FSL PMIcs, so I am not sure which is the best way to do. Maybe at the pmic initialization ? I added Lucasz in CC, because he is working on the PMIC framework making it more flexible and with the possibility to have multiple PMIC (he has a board with more as one PMIC, even if they are integrated in teh same chip). Lucasz changed (not yet merged) the pmic startup passing the number of I2C bus. So with the new interface, we could have something like: pmic_init(I2C_PMIC); if (pmic_detect()) { p = pmic_get("FSL_PMIC"); The question is if it makes sense to pass also the tx_num to the init function, and moving to a pmic_init(i2cbus, txnum); or is there a more elegant way ? Best regards, Stefano
Hi Stefano, > Am 16/10/2012 01:32, schrieb Fabio Estevam: > > From: Fabio Estevam <fabio.estevam@freescale.com> > > > > Introduce CONFIG_SYS_FSL_PMIC_I2C_LENGTH to configure the number of > > bytes that are used to communicate with the PMIC via I2C. > > > > Instead of hardcoding the value, pass the number via a config > > option. > > > > This will be useful for adding support for PMIC MC34704 from > > Freescale, which uses only one byte in its I2C protocol. > > > > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > > --- > > Hi Fabio, > > > Changes since v2: > > - No changes. Newly introduced in this series > > > > drivers/misc/pmic_fsl.c | 2 +- > > include/configs/mx35pdk.h | 1 + > > include/configs/mx53evk.h | 1 + > > include/configs/mx53loco.h | 1 + > > 4 files changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/misc/pmic_fsl.c b/drivers/misc/pmic_fsl.c > > index 0ff75ed..40c448b 100644 > > --- a/drivers/misc/pmic_fsl.c > > +++ b/drivers/misc/pmic_fsl.c > > @@ -53,7 +53,7 @@ int pmic_init(void) > > #elif defined(CONFIG_PMIC_I2C) > > p->interface = PMIC_I2C; > > p->hw.i2c.addr = CONFIG_SYS_FSL_PMIC_I2C_ADDR; > > - p->hw.i2c.tx_num = 3; > > + p->hw.i2c.tx_num = CONFIG_SYS_FSL_PMIC_I2C_LENGTH; > > The bad thing with it is that it seems that each board can have a > different value. However, this is bound to the selected pmic and not > to the board. So IMHO it should not go into the board configuration > file, but in the pmic specific initialization. > > We share the same general code for all FSL PMIcs, so I am not sure > which is the best way to do. Maybe at the pmic initialization ? > > I added Lucasz in CC, because he is working on the PMIC framework > making it more flexible and with the possibility to have multiple > PMIC (he has a board with more as one PMIC, even if they are > integrated in teh same chip). > > Lucasz changed (not yet merged) the pmic startup passing the number of > I2C bus. So with the new interface, we could have something like: I'm going to post another version of the patch in a few days. > > pmic_init(I2C_PMIC); > if (pmic_detect()) { > p = pmic_get("FSL_PMIC"); > > The question is if it makes sense to pass also the tx_num to the init > function, and moving to a > pmic_init(i2cbus, txnum); I'd like to avoid passing more data than necessary for the pmic_init. I will investigate how it can be solved. > > or is there a more elegant way ? > > Best regards, > Stefano >
On Tue, Oct 16, 2012 at 11:39 AM, Stefano Babic <sbabic@denx.de> wrote: >> --- a/drivers/misc/pmic_fsl.c >> +++ b/drivers/misc/pmic_fsl.c >> @@ -53,7 +53,7 @@ int pmic_init(void) >> #elif defined(CONFIG_PMIC_I2C) >> p->interface = PMIC_I2C; >> p->hw.i2c.addr = CONFIG_SYS_FSL_PMIC_I2C_ADDR; >> - p->hw.i2c.tx_num = 3; >> + p->hw.i2c.tx_num = CONFIG_SYS_FSL_PMIC_I2C_LENGTH; > > The bad thing with it is that it seems that each board can have a > different value. However, this is bound to the selected pmic and not to > the board. So IMHO it should not go into the board configuration file, > but in the pmic specific initialization. Ok, understood. What about putting CONFIG_SYS_FSL_PMIC_I2C_LENGTH inside the PMIC include file, such as include/dialog_pmic.h include/mc13783.h include/mc13892.h ? Thanks, Fabio Estevam
Hi Fabio, > On Tue, Oct 16, 2012 at 11:39 AM, Stefano Babic <sbabic@denx.de> > wrote: > > >> --- a/drivers/misc/pmic_fsl.c > >> +++ b/drivers/misc/pmic_fsl.c > >> @@ -53,7 +53,7 @@ int pmic_init(void) > >> #elif defined(CONFIG_PMIC_I2C) > >> p->interface = PMIC_I2C; > >> p->hw.i2c.addr = CONFIG_SYS_FSL_PMIC_I2C_ADDR; > >> - p->hw.i2c.tx_num = 3; > >> + p->hw.i2c.tx_num = CONFIG_SYS_FSL_PMIC_I2C_LENGTH; > > > > The bad thing with it is that it seems that each board can have a > > different value. However, this is bound to the selected pmic and > > not to the board. So IMHO it should not go into the board > > configuration file, but in the pmic specific initialization. > > Ok, understood. > > What about putting CONFIG_SYS_FSL_PMIC_I2C_LENGTH inside the PMIC > include file, such as > include/dialog_pmic.h > include/mc13783.h > include/mc13892.h ? > I totally agree. This definition looks like PMIC device specific and shall be defined at proper *.h file (as you proposed). Moreover the p->hw.i2c.tx_num = CONFIG_SYS_FSL_PMIC_I2C_LENGTH; shall be assigned at per pmic instantiation: int pmic_init(bus) { ... p->hw.i2c.tx_num = CONFIG_SYS_FSL_PMIC_I2C_LENGTH; ... } This should solve your problem.
Am 22/10/2012 17:22, schrieb Fabio Estevam: > On Tue, Oct 16, 2012 at 11:39 AM, Stefano Babic <sbabic@denx.de> wrote: > >>> --- a/drivers/misc/pmic_fsl.c >>> +++ b/drivers/misc/pmic_fsl.c >>> @@ -53,7 +53,7 @@ int pmic_init(void) >>> #elif defined(CONFIG_PMIC_I2C) >>> p->interface = PMIC_I2C; >>> p->hw.i2c.addr = CONFIG_SYS_FSL_PMIC_I2C_ADDR; >>> - p->hw.i2c.tx_num = 3; >>> + p->hw.i2c.tx_num = CONFIG_SYS_FSL_PMIC_I2C_LENGTH; >> >> The bad thing with it is that it seems that each board can have a >> different value. However, this is bound to the selected pmic and not to >> the board. So IMHO it should not go into the board configuration file, >> but in the pmic specific initialization. > > Ok, understood. > > What about putting CONFIG_SYS_FSL_PMIC_I2C_LENGTH inside the PMIC > include file, such as > include/dialog_pmic.h > include/mc13783.h > include/mc13892.h ? Apart changing the name (the prefix CONFIG_SYS_ should be not used), I think it is a praticable solution. Best regards, Stefano Babic
diff --git a/drivers/misc/pmic_fsl.c b/drivers/misc/pmic_fsl.c index 0ff75ed..40c448b 100644 --- a/drivers/misc/pmic_fsl.c +++ b/drivers/misc/pmic_fsl.c @@ -53,7 +53,7 @@ int pmic_init(void) #elif defined(CONFIG_PMIC_I2C) p->interface = PMIC_I2C; p->hw.i2c.addr = CONFIG_SYS_FSL_PMIC_I2C_ADDR; - p->hw.i2c.tx_num = 3; + p->hw.i2c.tx_num = CONFIG_SYS_FSL_PMIC_I2C_LENGTH; p->bus = I2C_PMIC; #else #error "You must select CONFIG_PMIC_SPI or CONFIG_PMIC_I2C" diff --git a/include/configs/mx35pdk.h b/include/configs/mx35pdk.h index 69bd654..3998d76 100644 --- a/include/configs/mx35pdk.h +++ b/include/configs/mx35pdk.h @@ -69,6 +69,7 @@ #define CONFIG_PMIC_I2C #define CONFIG_PMIC_FSL #define CONFIG_SYS_FSL_PMIC_I2C_ADDR 0x08 +#define CONFIG_SYS_FSL_PMIC_I2C_LENGTH 3 #define CONFIG_RTC_MC13XXX /* diff --git a/include/configs/mx53evk.h b/include/configs/mx53evk.h index 832050e..f7b11c0 100644 --- a/include/configs/mx53evk.h +++ b/include/configs/mx53evk.h @@ -59,6 +59,7 @@ #define CONFIG_PMIC_I2C #define CONFIG_PMIC_FSL #define CONFIG_SYS_FSL_PMIC_I2C_ADDR 8 +#define CONFIG_SYS_FSL_PMIC_I2C_LENGTH 3 #define CONFIG_RTC_MC13XXX /* MMC Configs */ diff --git a/include/configs/mx53loco.h b/include/configs/mx53loco.h index 6a6aaa1..fd454d5 100644 --- a/include/configs/mx53loco.h +++ b/include/configs/mx53loco.h @@ -96,6 +96,7 @@ #define CONFIG_PMIC_FSL #define CONFIG_SYS_DIALOG_PMIC_I2C_ADDR 0x48 #define CONFIG_SYS_FSL_PMIC_I2C_ADDR 0x8 +#define CONFIG_SYS_FSL_PMIC_I2C_LENGTH 3 /* allow to overwrite serial and ethaddr */ #define CONFIG_ENV_OVERWRITE