Hi, Stefano,
2011/5/12 Stefano Babic sbabic@denx.de:
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.hui@linaro.org Cc: sbabic@denx.de sbabic@denx.de Cc: Detlev Zundel dzu@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).
Then can we have the CONFIG_SYS_DIALOG_PMIC_I2C_ADDR or CONFIG_SYS_FSL_PMIC_I2C_ADDR in this file?
Please tell clear about your mean, don't let me guess what you mean?
IMHO we can rid of the check_param() function, as this is a constraint to have an implementation independent from a single PMIC.
If you think we don't need check_param, then It's ok. I can remove it. Since this function is added by you before.
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
Then what you prefer is function pointer or something else? Please tell clear, otherwise, it will cost another time to do it.
+#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.
Then what general name you think it's good?
I have seen the painful for restructure with the second time, another is the make imximage. Can we avoid it in the future?
And BTW, do you have any comments about the 1/3 clock patch? If you have, please tell it as early as possible and I don't want to let the patch version goes up very bigger and the time endless.
Thanks,
Jason
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: office@denx.de ===================================================================== _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot