[U-Boot] [PATCH V4 5/6] exynos: Add a common DT based PMIC driver initialization

[Minkyu Kang <mk7.kang@samsung.com>]

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.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
>>>
>>> +#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.

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

>>> +{
>>> +	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.

> 
>>> +	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?

Thanks,
Minkyu Kang.