Hi Minkyu Kang,
Thanks for reviewing the patch, Please check my comments inline.
On 1/7/14, Minkyu Kang mk7.kang@samsung.com 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 vbendeb@chromium.org Signed-off-by: Leela Krishna Amudala l.krishna@samsung.com Reviewed-by: Doug Anderson dianders@google.com Acked-by: Simon Glass sjg@chromium.org
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);elseret = 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 U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot