[U-Boot] [PATCH V4 5/6] exynos: Add a common DT based PMIC driver initialization
Leela Krishna Amudala
l.krishna at samsung.com
Thu Jan 16 05:05:54 CET 2014
Hi Minkyu Kang,
Thanks for reviewing the patch, Please check my comments inline.
On 1/7/14, Minkyu Kang <mk7.kang at 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 at chromium.org>
>> Signed-off-by: Leela Krishna Amudala <l.krishna at samsung.com>
>> Reviewed-by: Doug Anderson <dianders at google.com>
>> Acked-by: Simon Glass <sjg at 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);
>> + 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
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
More information about the U-Boot
mailing list