On 05/11/2011 10:03 AM, Jason Liu wrote: > This patch add dialog pmic(DA9053) driver with I2C interface support > In order to not duplicate code and according to the discussion on the > mail-list, change fsl_pmic.c to spi_i2c_pmic.c.Actaully,spi_i2c_pmic.c > is just a wrapper for PMIC communication when SPI or I2C is used. > > Signed-off-by: Jason Liu <jason....@linaro.org> > Cc: sba...@denx.de <sba...@denx.de> > Cc: Detlev Zundel <d...@denx.de>
Hi Jason, > --- a/drivers/misc/fsl_pmic.c > +++ b/drivers/misc/spi_i2c_pmic.c > @@ -22,13 +22,16 @@ > > #include <config.h> > #include <common.h> > +#include <i2c.h> > #include <asm/errno.h> > #include <linux/types.h> > +#if defined(CONFIG_FSL_PMIC) > #include <fsl_pmic.h> > +#endif > > -static int check_param(u32 reg, u32 write) > +static int check_param(u32 reg, u32 write, u32 max_reg) > { > - if (reg > 63 || write > 1) { > + if (reg > max_reg || write > 1) { > printf("<reg num> = %d is invalid. Should be less then 63\n", > reg); > return -1; > @@ -37,15 +40,13 @@ static int check_param(u32 reg, u32 write) > return 0; > } > > -#ifdef CONFIG_FSL_PMIC_I2C > -#include <i2c.h> > - > -u32 pmic_reg(u32 reg, u32 val, u32 write) > +#if defined(CONFIG_FSL_PMIC_I2C) > +u32 fsl_pmic_reg(u32 reg, u32 val, u32 write) > { > - unsigned char buf[4] = { 0 }; > u32 ret_val = 0; > + unsigned char buf[4] = { 0 }; > > - if (check_param(reg, write)) > + if (check_param(reg, write, 63)) > return -1; > > if (write) { > @@ -62,7 +63,44 @@ u32 pmic_reg(u32 reg, u32 val, u32 write) > > return ret_val; > } > -#else /* SPI interface */ > +#endif > + > +#if defined(CONFIG_DIALOG_PMIC_I2C) > +u32 dlg_pmic_reg(u32 reg, u32 val, u32 write) > +{ > + u32 ret_val = 0; > + unsigned char buf[1] = { 0 }; > + > + if (check_param(reg, write, 142)) > + return -1; > + > + if (write) { > + buf[0] = (val) & 0xff; > + if (i2c_write(CONFIG_SYS_DIALOG_PMIC_I2C_ADDR, reg, 1, buf, 1)) > + return -1; > + } else { > + if (i2c_read(CONFIG_SYS_DIALOG_PMIC_I2C_ADDR, reg, 1, buf, 1)) > + return -1; > + ret_val = buf[0]; > + } > + > + return ret_val; This is not what I meant. You have duplicated the code, instead of merge the two functions together. And I think the switch is wrong. This file provides a general access to PMIc using SPI/I2C. There should not be #ifdef related to a single PMIC. Instead of that, You need additional CONFIG_ to select how to access the PMIC (8 bit or 32 bit). IMHO we can rid of the check_param() function, as this is a constraint to have an implementation independent from a single PMIC. I would prefer something like this: u32 pmic_reg(u32 reg, u32 val, u32 write) { ........ #ifdef CONFIG_SYS_PMIC_8BIT <read / write 8 bit register> #else <actual implementation for fsl PMICs> #endif > +#if defined(CONFIG_FSL_PMIC_I2C) || defined(CONFIG_DIALOG_PMIC_I2C) Same comments apply here. We should select only between I2C and SPI, not the chip. > +#if defined(CONFIG_FSL_PMIC) > +#define PMIC_NAME "Freescale PMIC (Atlas)" > +#elif defined(CONFIG_DIALOG_PMIC) > +#define PMIC_NAME "Dialog PMIC (DA905x)" > +#else > +#error "Unkown PMIC name" > +#endif Instead of that, we can set a general name or put the PMIC_NAME inside the specific PMIC header file. Best regards, Stefano -- ===================================================================== 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: off...@denx.de ===================================================================== _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot