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).
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