| Message ID | 1293626331-18644-9-git-send-email-r64343@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On 12/29/2010 01:38 PM, Jason Liu wrote: > This patch add I2C interface for fsl_pmic driver support > > Signed-off-by: Jason Liu <r64343@freescale.com> > > --- > Changes for v2: > - Address the comments from Stefano, > - factor out the param_check in pmic_reg for both spi/i2c > --- > drivers/misc/fsl_pmic.c | 52 ++++++++++++++++++++++++++++++++++++++++++---- > 1 files changed, 47 insertions(+), 5 deletions(-) > Hi Jason, > + > + if (init_done == 0) { > + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); > + init_done = 1; > + } As I can see, i2c_init is called during initialization in arch/arm/lib/board.c. Why do we need to call it again ? > + 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]; > + } I am wondering if it works. The line with ret_val is never reached. Do you tested it ? Best regards, Stefano Babic
Hi, Stefano, 2010/12/31 Stefano Babic <sbabic@denx.de>: > On 12/29/2010 01:38 PM, Jason Liu wrote: >> This patch add I2C interface for fsl_pmic driver support >> >> Signed-off-by: Jason Liu <r64343@freescale.com> >> >> --- >> Changes for v2: >> - Address the comments from Stefano, >> - factor out the param_check in pmic_reg for both spi/i2c >> --- >> drivers/misc/fsl_pmic.c | 52 ++++++++++++++++++++++++++++++++++++++++++---- >> 1 files changed, 47 insertions(+), 5 deletions(-) >> > > Hi Jason, > >> + >> + if (init_done == 0) { >> + i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE); >> + init_done = 1; >> + } > > As I can see, i2c_init is called during initialization in > arch/arm/lib/board.c. Why do we need to call it again ? Yes, I think, I can remove it. > >> + 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]; >> + } > > I am wondering if it works. The line with ret_val is never reached. Do > you tested it ? I have tested before. This patch of code is definitely wrong. I will fix it. Thanks for review. Happy New Year! > > 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..b58854d 100644 --- a/drivers/misc/fsl_pmic.c +++ b/drivers/misc/fsl_pmic.c @@ -22,11 +22,55 @@ #include <config.h> #include <common.h> -#include <spi.h> #include <asm/errno.h> #include <linux/types.h> #include <fsl_pmic.h> +static 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; +} + +#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 (check_param(reg, write)) + 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) @@ -55,11 +99,8 @@ u32 pmic_reg(u32 reg, u32 val, u32 write) return -1; } - if (reg > 63 || write > 1) { - printf("<reg num> = %d is invalid. Should be less then 63\n", - reg); + if (check_param(reg, write)) return -1; - } if (spi_claim_bus(slave)) return -1; @@ -87,6 +128,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> --- Changes for v2: - Address the comments from Stefano, - factor out the param_check in pmic_reg for both spi/i2c --- drivers/misc/fsl_pmic.c | 52 ++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 47 insertions(+), 5 deletions(-)