| Message ID | 1293024231-4897-2-git-send-email-r64343@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On 12/22/2010 02:23 PM, Jason Liu wrote: > This patch add I2C interface for fsl_pmic driver support > > +#ifdef CONFIG_FSL_PMIC_I2C > +#include <i2c.h> > +static int init_done; > + > +u32 pmic_reg(u32 reg, u32 val, u32 write) > +{ > + unsigned char buf[4] = { 0 }; > + u32 ret_val = 0; > + > + if (reg > 63 || write > 1) { > + printf("<reg num> = %d is invalid. Should be less then 63\n", > + reg); > + return -1; > + } Probably we can factorize better this function and do not duplicate the pmic_reg function for SPI and I2C. > + > + if (init_done == 0) { > + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); > + init_done = 1; > + } > + > + if (write) { > + buf[0] = (val >> 16) & 0xff; > + buf[1] = (val >> 8) & 0xff; > + buf[2] = (val) & 0xff; It seems you are converting the endianess. No chance to use an already provided accessor ? > + if (i2c_write(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) > + return -1; > + } else { > + if (i2c_read(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) { > + return -1; > + ret_val = buf[0] << 16 | buf[1] << 8 | buf[2]; Ditto. Best regards, Stefano Babic
Hi, Stefano, 2010/12/27 Stefano Babic <sbabic@denx.de>: > On 12/22/2010 02:23 PM, Jason Liu wrote: >> This patch add I2C interface for fsl_pmic driver support >> >> +#ifdef CONFIG_FSL_PMIC_I2C >> +#include <i2c.h> >> +static int init_done; >> + >> +u32 pmic_reg(u32 reg, u32 val, u32 write) >> +{ >> + unsigned char buf[4] = { 0 }; >> + u32 ret_val = 0; >> + >> + if (reg > 63 || write > 1) { >> + printf("<reg num> = %d is invalid. Should be less then 63\n", >> + reg); >> + return -1; >> + } > > Probably we can factorize better this function and do not duplicate the > pmic_reg function for SPI and I2C. OK, I will write one function to check it. > >> + >> + if (init_done == 0) { >> + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); >> + init_done = 1; >> + } >> + >> + if (write) { >> + buf[0] = (val >> 16) & 0xff; >> + buf[1] = (val >> 8) & 0xff; >> + buf[2] = (val) & 0xff; > > It seems you are converting the endianess. No chance to use an already > provided accessor ? Good point. I will do it. > >> + if (i2c_write(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) >> + return -1; >> + } else { >> + if (i2c_read(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) { >> + return -1; >> + ret_val = buf[0] << 16 | buf[1] << 8 | buf[2]; > > Ditto. OK, > > Best regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de > ===================================================================== > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
Hi, Stefano, 2010/12/27 Stefano Babic <sbabic@denx.de>: > On 12/22/2010 02:23 PM, Jason Liu wrote: >> This patch add I2C interface for fsl_pmic driver support >> >> +#ifdef CONFIG_FSL_PMIC_I2C >> +#include <i2c.h> >> +static int init_done; >> + >> +u32 pmic_reg(u32 reg, u32 val, u32 write) >> +{ >> + unsigned char buf[4] = { 0 }; >> + u32 ret_val = 0; >> + >> + if (reg > 63 || write > 1) { >> + printf("<reg num> = %d is invalid. Should be less then 63\n", >> + reg); >> + return -1; >> + } > > Probably we can factorize better this function and do not duplicate the > pmic_reg function for SPI and I2C. I want to make sure what I did is what you want. Do you mean that I need write one function like: int check_param(u32 reg, u32 write) { if (reg > 63 || write > 1) { printf("<reg num> = %d is invalid. Should be less then 63\n", reg); return -1; } return 0; } and put the function call check_param in pmic_reg function to avoid the code duplication. If something different with what I though, please correct me. Thanks, > >> + >> + if (init_done == 0) { >> + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); >> + init_done = 1; >> + } >> + >> + if (write) { >> + buf[0] = (val >> 16) & 0xff; >> + buf[1] = (val >> 8) & 0xff; >> + buf[2] = (val) & 0xff; > > It seems you are converting the endianess. No chance to use an already > provided accessor ? > >> + if (i2c_write(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) >> + return -1; >> + } else { >> + if (i2c_read(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) { >> + return -1; >> + ret_val = buf[0] << 16 | buf[1] << 8 | buf[2]; > > Ditto. > > Best regards, > Stefano Babic > > -- > ===================================================================== > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de > ===================================================================== > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
diff --git a/drivers/misc/fsl_pmic.c b/drivers/misc/fsl_pmic.c index 5ee1de1..a54f220 100644 --- a/drivers/misc/fsl_pmic.c +++ b/drivers/misc/fsl_pmic.c @@ -22,11 +22,47 @@ #include <config.h> #include <common.h> -#include <spi.h> #include <asm/errno.h> #include <linux/types.h> #include <fsl_pmic.h> +#ifdef CONFIG_FSL_PMIC_I2C +#include <i2c.h> +static int init_done; + +u32 pmic_reg(u32 reg, u32 val, u32 write) +{ + unsigned char buf[4] = { 0 }; + u32 ret_val = 0; + + if (reg > 63 || write > 1) { + printf("<reg num> = %d is invalid. Should be less then 63\n", + reg); + return -1; + } + + if (init_done == 0) { + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); + init_done = 1; + } + + if (write) { + buf[0] = (val >> 16) & 0xff; + buf[1] = (val >> 8) & 0xff; + buf[2] = (val) & 0xff; + if (i2c_write(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) + return -1; + } else { + if (i2c_read(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) { + return -1; + ret_val = buf[0] << 16 | buf[1] << 8 | buf[2]; + } + } + + return ret_val; +} +#else /* SPI interface */ +#include <spi.h> static struct spi_slave *slave; struct spi_slave *pmic_spi_probe(void) @@ -87,6 +123,7 @@ u32 pmic_reg(u32 reg, u32 val, u32 write) spi_release_bus(slave); return cpu_to_be32(pmic_rx); } +#endif void pmic_reg_write(u32 reg, u32 value) {
This patch add I2C interface for fsl_pmic driver support Signed-off-by: Jason Liu <r64343@freescale.com> --- drivers/misc/fsl_pmic.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 files changed, 38 insertions(+), 1 deletions(-)