Hi,
On 1/16/14, Minkyu Kang <mk7.k...@samsung.com> wrote:
> On 16/01/14 13:05, Leela Krishna Amudala wrote:
>> Hi Minkyu Kang,
>>
>> Thanks for reviewing the patch, Please check my comments inline.
>>
>> On 1/7/14, Minkyu Kang <mk7.k...@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 <vben...@chromium.org>
>>>> Signed-off-by: Leela Krishna Amudala <l.kris...@samsung.com>
>>>> Reviewed-by: Doug Anderson <diand...@google.com>
>>>> Acked-by: Simon Glass <s...@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
>>>>
>>>> +#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.
>
> So, it's a wrong architecture.
> It (find the number of registers) is a PMIC specific feature.
>
Okay, Then in that case I'll move this function to a new file or some
suitable location and call it here
or if you have any suggestions please tell me.
>>
>>>> +
>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>> +
>>>> +static unsigned pmic_number_of_regs(enum fdt_compat_id pmic_compat)
>
> As I said, this function is not a common feature.
>
Okay, will move it to suitable location.
>>>> +{
>>>> + 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].
>
> maybe your code will work correctly
> but, the point is pmic_ops[-1] is not always right.
> please think about it.
>
> If I modify your code, then I'll return error immediately in while loop.
>
Okay, I'll change this.
>>
>>>> + 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.
>
> It means you want to support S2MPS11 only. right?
> Then why you make a common file?
>
I'm trying to make it as a common stuff so started with S2MPS11.
Okay, I'll use u32 to make it compatible with other PMIC chips.
Thanks,
Leela Krishna.
> Thanks,
> Minkyu Kang.
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
