Hi Minkyu Kang, Thanks for reviewing the patch, Please check my comments inline.
On 1/7/14, Minkyu Kang <[email protected]> wrote: > On 06/01/14 20:49, Leela Krishna Amudala wrote: >> Most of i2c PMIC drivers follow the same initialization sequence, >> let's generalize it in a common file. >> >> The initialization function finds the PMIC in the device tree, and if >> found - registers it in the list of known PMICs and initializes it, >> iterating through the table of settings supplied by the caller. >> >> Signed-off-by: Vadim Bendebury <[email protected]> >> Signed-off-by: Leela Krishna Amudala <[email protected]> >> Reviewed-by: Doug Anderson <[email protected]> >> Acked-by: Simon Glass <[email protected]> >> --- >> board/samsung/common/board.c | 22 +++++++++ >> drivers/power/pmic/Makefile | 1 + >> drivers/power/pmic/pmic_common.c | 97 >> ++++++++++++++++++++++++++++++++++++++ >> drivers/power/power_core.c | 14 ++++++ >> include/power/pmic.h | 34 +++++++++++++ >> 5 files changed, 168 insertions(+) >> create mode 100644 drivers/power/pmic/pmic_common.c >> >> diff --git a/board/samsung/common/board.c b/board/samsung/common/board.c >> index b3b84c1..d23a7a6 100644 >> --- a/board/samsung/common/board.c >> +++ b/board/samsung/common/board.c >> @@ -23,6 +23,7 @@ >> #include <power/pmic.h> >> #include <asm/arch/sromc.h> >> #include <power/max77686_pmic.h> >> +#include <power/s2mps11_pmic.h> > > Do we need to include those two header files (max77686 and s2mps11) > together? > Okay, I'll make it conditional inclusion >> >> DECLARE_GLOBAL_DATA_PTR; >> >> @@ -160,6 +161,25 @@ static int board_init_cros_ec_devices(const void >> *blob) >> } >> #endif >> >> +#ifdef CONFIG_POWER_S2MPS11 > > please move this block into "#if defined(CONFIG_POWER)". > Okay, will move it >> +int board_init_s2mps11(void) >> +{ >> + const struct pmic_init_ops pmic_ops[] = { >> + {PMIC_REG_WRITE, S2MPS11_BUCK1_CTRL2, S2MPS11_BUCK_CTRL2_1V}, >> + {PMIC_REG_WRITE, S2MPS11_BUCK2_CTRL2, >> + S2MPS11_BUCK_CTRL2_1_2625V}, >> + {PMIC_REG_WRITE, S2MPS11_BUCK3_CTRL2, S2MPS11_BUCK_CTRL2_1V}, >> + {PMIC_REG_WRITE, S2MPS11_BUCK4_CTRL2, S2MPS11_BUCK_CTRL2_1V}, >> + {PMIC_REG_WRITE, S2MPS11_BUCK6_CTRL2, S2MPS11_BUCK_CTRL2_1V}, >> + {PMIC_REG_UPDATE, S2MPS11_REG_RTC_CTRL, >> + S2MPS11_RTC_CTRL_32KHZ_CP_EN | S2MPS11_RTC_CTRL_JIT}, >> + {PMIC_REG_BAIL} >> + }; >> + >> + return pmic_common_init(COMPAT_SAMSUNG_S2MPS11_PMIC, pmic_ops); >> +} >> +#endif >> + >> #if defined(CONFIG_POWER) >> #ifdef CONFIG_POWER_MAX77686 >> static int max77686_init(void) >> @@ -263,6 +283,8 @@ int power_init_board(void) >> >> #ifdef CONFIG_POWER_MAX77686 >> ret = max77686_init(); >> +#elif defined(CONFIG_POWER_S2MPS11) >> + ret = board_init_s2mps11(); >> #endif >> >> return ret; >> diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile >> index 0b45ffa..5e236a3 100644 >> --- a/drivers/power/pmic/Makefile >> +++ b/drivers/power/pmic/Makefile >> @@ -11,3 +11,4 @@ obj-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o >> obj-$(CONFIG_POWER_MAX77686) += pmic_max77686.o >> obj-$(CONFIG_POWER_TPS65217) += pmic_tps65217.o >> obj-$(CONFIG_POWER_TPS65910) += pmic_tps65910.o >> +obj-$(CONFIG_POWER) += pmic_common.o >> diff --git a/drivers/power/pmic/pmic_common.c >> b/drivers/power/pmic/pmic_common.c >> new file mode 100644 >> index 0000000..3117ae2 >> --- /dev/null >> +++ b/drivers/power/pmic/pmic_common.c >> @@ -0,0 +1,97 @@ >> +/* >> + * Copyright (c) 2013 The Chromium OS Authors. All rights reserved. > > Please write on the author of this file. > Okay, will do that >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> +#include <common.h> >> +#include <fdtdec.h> >> +#include <errno.h> >> +#include <power/pmic.h> >> +#include <power/s2mps11_pmic.h> >> +#include <power/max77686_pmic.h> > > Why common driver need specific pmic's header file? > It is wrong architecture. > Here, in this file we are using PMIC compact ID to find the number of registers in a PMIC (now currently have support for only S2MPS11 and other PMICs info may added in future), so we need those headers. >> + >> +DECLARE_GLOBAL_DATA_PTR; >> + >> +static unsigned pmic_number_of_regs(enum fdt_compat_id pmic_compat) >> +{ >> + switch (pmic_compat) { >> + case COMPAT_SAMSUNG_S2MPS11_PMIC: >> + return S2MPS11_NUM_OF_REGS; >> + default: >> + break; >> + } >> + return 0; >> +} >> + >> +int pmic_common_init(enum fdt_compat_id pmic_compat, >> + const struct pmic_init_ops *pmic_ops) >> +{ >> + const void *blob = gd->fdt_blob; >> + struct pmic *p; >> + int node, parent, ret; >> + unsigned number_of_regs = pmic_number_of_regs(pmic_compat); >> + const char *pmic_name, *comma; >> + >> + if (!number_of_regs) { >> + printf("%s: %s - not a supported PMIC\n", >> + __func__, fdtdec_get_compatible(pmic_compat)); >> + return -1; >> + } >> + >> + node = fdtdec_next_compatible(blob, 0, pmic_compat); >> + if (node < 0) { >> + debug("PMIC: Error %s. No node for %s in device tree\n", >> + fdt_strerror(node), fdtdec_get_compatible(pmic_compat)); >> + return node; >> + } >> + >> + pmic_name = fdtdec_get_compatible(pmic_compat); >> + comma = strchr(pmic_name, ','); >> + if (comma) >> + pmic_name = comma + 1; >> + >> + p = pmic_alloc(); >> + >> + if (!p) { >> + printf("%s: POWER allocation error!\n", __func__); >> + return -ENOMEM; >> + } >> + parent = fdt_parent_offset(blob, node); >> + if (parent < 0) { >> + debug("%s: Cannot find node parent\n", __func__); >> + return -1; >> + } >> + >> + p->bus = i2c_get_bus_num_fdt(parent); >> + if (p->bus < 0) { >> + debug("%s: Cannot find I2C bus\n", __func__); >> + return -1; >> + } >> + p->hw.i2c.addr = fdtdec_get_int(blob, node, "reg", 9); >> + >> + p->name = pmic_name; >> + p->interface = PMIC_I2C; >> + p->hw.i2c.tx_num = 1; >> + p->number_of_regs = number_of_regs; >> + p->compat_id = pmic_compat; >> + >> + ret = 0; >> + while ((pmic_ops->reg_op != PMIC_REG_BAIL) && !ret) { >> + if (pmic_ops->reg_op == PMIC_REG_WRITE) >> + ret = pmic_reg_write(p, >> + pmic_ops->reg_addr, >> + pmic_ops->reg_value); >> + else >> + ret = pmic_reg_update(p, >> + pmic_ops->reg_addr, >> + pmic_ops->reg_value); >> + pmic_ops++; >> + } >> + >> + if (ret) >> + printf("%s: Failed accessing reg 0x%x of %s\n", >> + __func__, pmic_ops[-1].reg_addr, p->name); > > pmic_ops[-1].reg_addr, is it right? > Yes, this is right because if you see the above while() loop we are incrementing the pmic_ops pointer after reg_write/reg_update and if any of them returns error value ,while() loop will break and the pmic_ops pointing to the next instance address. so to print the values in the previous instance pointer we are using pmic_ops[-1]. >> + else >> + printf("PMIC %s initialized\n", p->name); >> + return ret; >> +} >> diff --git a/drivers/power/power_core.c b/drivers/power/power_core.c >> index a1c4fd0..f40be31 100644 >> --- a/drivers/power/power_core.c >> +++ b/drivers/power/power_core.c >> @@ -228,6 +228,20 @@ int pmic_reg_update(struct pmic *p, int reg, u32 >> val) >> return 0; >> } >> >> +struct pmic *pmic_get_by_id(enum fdt_compat_id pmic_compat) >> +{ >> + struct pmic *p; >> + >> + list_for_each_entry(p, &pmic_list, list) { >> + if (p->compat_id == pmic_compat) { >> + debug("%s: pmic %s -> 0x%p\n", __func__, p->name, p); >> + return p; >> + } >> + } >> + >> + return NULL; >> +} >> + >> U_BOOT_CMD( >> pmic, CONFIG_SYS_MAXARGS, 1, do_pmic, >> "PMIC", >> diff --git a/include/power/pmic.h b/include/power/pmic.h >> index e0b9139..e0982e3 100644 >> --- a/include/power/pmic.h >> +++ b/include/power/pmic.h >> @@ -12,6 +12,7 @@ >> #include <linux/list.h> >> #include <i2c.h> >> #include <power/power_chrg.h> >> +#include <fdtdec.h> >> >> enum { PMIC_I2C, PMIC_SPI, PMIC_NONE}; >> enum { I2C_PMIC, I2C_NUM, }; >> @@ -72,6 +73,7 @@ struct pmic { >> >> struct pmic *parent; >> struct list_head list; >> + enum fdt_compat_id compat_id; >> }; >> >> int pmic_init(unsigned char bus); >> @@ -84,6 +86,38 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val); >> int pmic_reg_write(struct pmic *p, u32 reg, u32 val); >> int pmic_set_output(struct pmic *p, u32 reg, int ldo, int on); >> int pmic_reg_update(struct pmic *p, int reg, u32 val); >> +/* >> + * Find registered PMIC based on its compatibility ID. >> + * >> + * @param pmic_compat compatibility ID of the PMIC to search for. >> + * @return pointer to the relevant 'struct pmic' on success or NULL >> + */ >> +struct pmic *pmic_get_by_id(enum fdt_compat_id pmic_compat); >> + >> +enum pmic_reg_op { PMIC_REG_BAIL, PMIC_REG_WRITE, PMIC_REG_UPDATE }; >> +struct pmic_init_ops { >> + enum pmic_reg_op reg_op; >> + u8 reg_addr; >> + u8 reg_value; > > u8? why? > according to pmic.h, all of pmic operations are allowed u32. > Currently I don't have S2MPS11 data sheet with me, but I remember that it has 1 byte size registers. so the structure declared like that. Best Wishes, Leela krishna. >> +}; >> + >> +/** >> + * Common function used to intialize an i2c based PMIC. >> + * >> + * This function finds the PMIC in the device tree based on its >> compatibility >> + * ID. If found, the struct pmic is allocated, initialized and >> registered. >> + * >> + * Then the table of initialization settings is scanned and the PMIC >> registers >> + * are set as dictated by the table contents, >> + * >> + * @param pmic_compat compatibility ID f the PMIC to be initialized. >> + * @param pmic_ops a pointer to the table containing PMIC >> initialization >> + * settings. The last entry contains reg_op >> + * of PMIC_REG_BAIL. >> + * @return zero on success, nonzero on failure >> + */ >> +int pmic_common_init(enum fdt_compat_id pmic_compat, >> + const struct pmic_init_ops *pmic_ops); >> >> #define pmic_i2c_addr (p->hw.i2c.addr) >> #define pmic_i2c_tx_num (p->hw.i2c.tx_num) >> > > Thanks, > Minkyu Kang. > _______________________________________________ > U-Boot mailing list > [email protected] > http://lists.denx.de/mailman/listinfo/u-boot > _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
