* [U-Boot] [PATCH V3 1/6] exynos: Use common pmic_reg_update() definition
2013-11-12 10:04 [U-Boot] [PATCH V3 0/6] SMDK5420: Add S2MPS11 pmic support to SMDK5420 Leela Krishna Amudala
@ 2013-11-12 10:04 ` Leela Krishna Amudala
2013-12-05 5:50 ` Minkyu Kang
2013-11-12 10:04 ` [U-Boot] [PATCH V3 2/6] power: Explicitly select pmic device's bus Leela Krishna Amudala
` (4 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Leela Krishna Amudala @ 2013-11-12 10:04 UTC (permalink / raw)
To: u-boot
This function is used by different Exynos platforms, put it in the
common file.
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>
Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
board/samsung/common/board.c | 19 -------------------
drivers/power/power_core.c | 19 +++++++++++++++++++
include/power/pmic.h | 1 +
3 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/board/samsung/common/board.c b/board/samsung/common/board.c
index 2512a59..1959c4e 100644
--- a/board/samsung/common/board.c
+++ b/board/samsung/common/board.c
@@ -156,25 +156,6 @@ static int board_init_cros_ec_devices(const void *blob)
#if defined(CONFIG_POWER)
#ifdef CONFIG_POWER_MAX77686
-static int pmic_reg_update(struct pmic *p, int reg, uint regval)
-{
- u32 val;
- int ret = 0;
-
- ret = pmic_reg_read(p, reg, &val);
- if (ret) {
- debug("%s: PMIC %d register read failed\n", __func__, reg);
- return -1;
- }
- val |= regval;
- ret = pmic_reg_write(p, reg, val);
- if (ret) {
- debug("%s: PMIC %d register write failed\n", __func__, reg);
- return -1;
- }
- return 0;
-}
-
static int max77686_init(void)
{
struct pmic *p;
diff --git a/drivers/power/power_core.c b/drivers/power/power_core.c
index 29ccc83..e715435 100644
--- a/drivers/power/power_core.c
+++ b/drivers/power/power_core.c
@@ -208,6 +208,25 @@ int do_pmic(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
return CMD_RET_SUCCESS;
}
+int pmic_reg_update(struct pmic *p, int reg, uint regval)
+{
+ u32 val;
+ int ret = 0;
+
+ ret = pmic_reg_read(p, reg, &val);
+ if (ret) {
+ debug("%s: PMIC %d register read failed\n", __func__, reg);
+ return -1;
+ }
+ val |= regval;
+ ret = pmic_reg_write(p, reg, val);
+ if (ret) {
+ debug("%s: PMIC %d register write failed\n", __func__, reg);
+ return -1;
+ }
+ return 0;
+}
+
U_BOOT_CMD(
pmic, CONFIG_SYS_MAXARGS, 1, do_pmic,
"PMIC",
diff --git a/include/power/pmic.h b/include/power/pmic.h
index 0e7aa31..d17dbdc 100644
--- a/include/power/pmic.h
+++ b/include/power/pmic.h
@@ -83,6 +83,7 @@ int pmic_probe(struct pmic *p);
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, uint regval);
#define pmic_i2c_addr (p->hw.i2c.addr)
#define pmic_i2c_tx_num (p->hw.i2c.tx_num)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [U-Boot] [PATCH V3 1/6] exynos: Use common pmic_reg_update() definition
2013-11-12 10:04 ` [U-Boot] [PATCH V3 1/6] exynos: Use common pmic_reg_update() definition Leela Krishna Amudala
@ 2013-12-05 5:50 ` Minkyu Kang
2014-01-02 23:36 ` Leela Krishna Amudala
0 siblings, 1 reply; 12+ messages in thread
From: Minkyu Kang @ 2013-12-05 5:50 UTC (permalink / raw)
To: u-boot
Dear Leela Krishna Amudala,
On 12/11/13 19:04, Leela Krishna Amudala wrote:
> This function is used by different Exynos platforms, put it in the
> common file.
>
> 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>
> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
> Acked-by: Simon Glass <sjg@chromium.org>
> ---
> board/samsung/common/board.c | 19 -------------------
> drivers/power/power_core.c | 19 +++++++++++++++++++
> include/power/pmic.h | 1 +
> 3 files changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/board/samsung/common/board.c b/board/samsung/common/board.c
> index 2512a59..1959c4e 100644
> --- a/board/samsung/common/board.c
> +++ b/board/samsung/common/board.c
> @@ -156,25 +156,6 @@ static int board_init_cros_ec_devices(const void *blob)
>
> #if defined(CONFIG_POWER)
> #ifdef CONFIG_POWER_MAX77686
> -static int pmic_reg_update(struct pmic *p, int reg, uint regval)
> -{
> - u32 val;
> - int ret = 0;
> -
> - ret = pmic_reg_read(p, reg, &val);
> - if (ret) {
> - debug("%s: PMIC %d register read failed\n", __func__, reg);
> - return -1;
> - }
> - val |= regval;
> - ret = pmic_reg_write(p, reg, val);
> - if (ret) {
> - debug("%s: PMIC %d register write failed\n", __func__, reg);
> - return -1;
> - }
> - return 0;
> -}
> -
> static int max77686_init(void)
> {
> struct pmic *p;
> diff --git a/drivers/power/power_core.c b/drivers/power/power_core.c
> index 29ccc83..e715435 100644
> --- a/drivers/power/power_core.c
> +++ b/drivers/power/power_core.c
> @@ -208,6 +208,25 @@ int do_pmic(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> return CMD_RET_SUCCESS;
> }
>
> +int pmic_reg_update(struct pmic *p, int reg, uint regval)
> +{
> + u32 val;
> + int ret = 0;
= 0 is unnecessary.
> +
> + ret = pmic_reg_read(p, reg, &val);
> + if (ret) {
> + debug("%s: PMIC %d register read failed\n", __func__, reg);
> + return -1;
> + }
please add blank line.
> + val |= regval;
> + ret = pmic_reg_write(p, reg, val);
> + if (ret) {
> + debug("%s: PMIC %d register write failed\n", __func__, reg);
> + return -1;
> + }
> + return 0;
> +}
> +
> U_BOOT_CMD(
> pmic, CONFIG_SYS_MAXARGS, 1, do_pmic,
> "PMIC",
> diff --git a/include/power/pmic.h b/include/power/pmic.h
> index 0e7aa31..d17dbdc 100644
> --- a/include/power/pmic.h
> +++ b/include/power/pmic.h
> @@ -83,6 +83,7 @@ int pmic_probe(struct pmic *p);
> 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, uint regval);
uint regval -> u32 val, for keep naming convention with other functions.
>
> #define pmic_i2c_addr (p->hw.i2c.addr)
> #define pmic_i2c_tx_num (p->hw.i2c.tx_num)
>
Thanks,
Minkyu Kang.
^ permalink raw reply [flat|nested] 12+ messages in thread* [U-Boot] [PATCH V3 1/6] exynos: Use common pmic_reg_update() definition
2013-12-05 5:50 ` Minkyu Kang
@ 2014-01-02 23:36 ` Leela Krishna Amudala
0 siblings, 0 replies; 12+ messages in thread
From: Leela Krishna Amudala @ 2014-01-02 23:36 UTC (permalink / raw)
To: u-boot
Hi Minkyu Kang,
Sorry for late response.
Please find my comments below.
On Thu, Dec 5, 2013 at 2:50 PM, Minkyu Kang <mk7.kang@samsung.com> wrote:
> Dear Leela Krishna Amudala,
>
> On 12/11/13 19:04, Leela Krishna Amudala wrote:
>> This function is used by different Exynos platforms, put it in the
>> common file.
>>
>> 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>
>> Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
>> Acked-by: Simon Glass <sjg@chromium.org>
>> ---
>> board/samsung/common/board.c | 19 -------------------
>> drivers/power/power_core.c | 19 +++++++++++++++++++
>> include/power/pmic.h | 1 +
>> 3 files changed, 20 insertions(+), 19 deletions(-)
>>
>> diff --git a/board/samsung/common/board.c b/board/samsung/common/board.c
>> index 2512a59..1959c4e 100644
>> --- a/board/samsung/common/board.c
>> +++ b/board/samsung/common/board.c
>> @@ -156,25 +156,6 @@ static int board_init_cros_ec_devices(const void *blob)
>>
>> #if defined(CONFIG_POWER)
>> #ifdef CONFIG_POWER_MAX77686
>> -static int pmic_reg_update(struct pmic *p, int reg, uint regval)
>> -{
>> - u32 val;
>> - int ret = 0;
>> -
>> - ret = pmic_reg_read(p, reg, &val);
>> - if (ret) {
>> - debug("%s: PMIC %d register read failed\n", __func__, reg);
>> - return -1;
>> - }
>> - val |= regval;
>> - ret = pmic_reg_write(p, reg, val);
>> - if (ret) {
>> - debug("%s: PMIC %d register write failed\n", __func__, reg);
>> - return -1;
>> - }
>> - return 0;
>> -}
>> -
>> static int max77686_init(void)
>> {
>> struct pmic *p;
>> diff --git a/drivers/power/power_core.c b/drivers/power/power_core.c
>> index 29ccc83..e715435 100644
>> --- a/drivers/power/power_core.c
>> +++ b/drivers/power/power_core.c
>> @@ -208,6 +208,25 @@ int do_pmic(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>> return CMD_RET_SUCCESS;
>> }
>>
>> +int pmic_reg_update(struct pmic *p, int reg, uint regval)
>> +{
>> + u32 val;
>> + int ret = 0;
>
> = 0 is unnecessary.
>
Okay, will remove it.
>> +
>> + ret = pmic_reg_read(p, reg, &val);
>> + if (ret) {
>> + debug("%s: PMIC %d register read failed\n", __func__, reg);
>> + return -1;
>> + }
>
> please add blank line.
>
Okay, will add it.
>> + val |= regval;
>> + ret = pmic_reg_write(p, reg, val);
>> + if (ret) {
>> + debug("%s: PMIC %d register write failed\n", __func__, reg);
>> + return -1;
>> + }
>> + return 0;
>> +}
>> +
>> U_BOOT_CMD(
>> pmic, CONFIG_SYS_MAXARGS, 1, do_pmic,
>> "PMIC",
>> diff --git a/include/power/pmic.h b/include/power/pmic.h
>> index 0e7aa31..d17dbdc 100644
>> --- a/include/power/pmic.h
>> +++ b/include/power/pmic.h
>> @@ -83,6 +83,7 @@ int pmic_probe(struct pmic *p);
>> 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, uint regval);
>
> uint regval -> u32 val, for keep naming convention with other functions.
>
Okay, will change it.
Best Wishes,
Leela Krishna.
>>
>> #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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH V3 2/6] power: Explicitly select pmic device's bus
2013-11-12 10:04 [U-Boot] [PATCH V3 0/6] SMDK5420: Add S2MPS11 pmic support to SMDK5420 Leela Krishna Amudala
2013-11-12 10:04 ` [U-Boot] [PATCH V3 1/6] exynos: Use common pmic_reg_update() definition Leela Krishna Amudala
@ 2013-11-12 10:04 ` Leela Krishna Amudala
2013-12-05 5:50 ` Minkyu Kang
2013-11-12 10:04 ` [U-Boot] [PATCH V3 3/6] FDT: Exynos5420: Add compatible srings for PMIC Leela Krishna Amudala
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Leela Krishna Amudala @ 2013-11-12 10:04 UTC (permalink / raw)
To: u-boot
The current pmic i2c code assumes the current i2c bus is
the same as the pmic device's bus. There is nothing ensuring
that to be true. Therefore, select the proper bus before performing
a transaction.
Signed-off-by: Aaron Durbin <adurbin@chromium.org>
Signed-off-by: Simon Glass <sjg@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>
---
drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++----
1 file changed, 57 insertions(+), 5 deletions(-)
diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
index ac76870..3cafa4d 100644
--- a/drivers/power/power_i2c.c
+++ b/drivers/power/power_i2c.c
@@ -16,9 +16,45 @@
#include <i2c.h>
#include <compiler.h>
+static int pmic_select(struct pmic *p)
+{
+ int ret, old_bus;
+
+ old_bus = i2c_get_bus_num();
+ if (old_bus != p->bus) {
+ debug("%s: Select bus %d\n", __func__, p->bus);
+ ret = i2c_set_bus_num(p->bus);
+ if (ret) {
+ debug("%s: Cannot select pmic %s, err %d\n",
+ __func__, p->name, ret);
+ return -1;
+ }
+ }
+
+ return old_bus;
+}
+
+static int pmic_deselect(int old_bus)
+{
+ int ret;
+
+ if (old_bus != i2c_get_bus_num()) {
+ ret = i2c_set_bus_num(old_bus);
+ debug("%s: Select bus %d\n", __func__, old_bus);
+ if (ret) {
+ debug("%s: Cannot restore i2c bus, err %d\n",
+ __func__, ret);
+ return -1;
+ }
+ }
+
+ return 0;
+}
+
int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
{
unsigned char buf[4] = { 0 };
+ int ret, old_bus;
if (check_reg(p, reg))
return -1;
@@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
return -1;
}
- if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
+ old_bus = pmic_select(p);
+ if (old_bus < 0)
return -1;
- return 0;
+ ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
+ pmic_deselect(old_bus);
+ return ret;
}
int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
{
unsigned char buf[4] = { 0 };
u32 ret_val = 0;
+ int ret, old_bus;
if (check_reg(p, reg))
return -1;
- if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
+ old_bus = pmic_select(p);
+ if (old_bus < 0)
return -1;
+ ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
+ pmic_deselect(old_bus);
+ if (ret)
+ return ret;
+
switch (pmic_i2c_tx_num) {
case 3:
if (p->sensor_byte_order == PMIC_SENSOR_BYTE_ORDER_BIG)
@@ -98,9 +144,15 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
int pmic_probe(struct pmic *p)
{
- i2c_set_bus_num(p->bus);
+ int ret, old_bus;
+
+ old_bus = pmic_select(p);
+ if (old_bus < 0)
+ return -1;
debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
- if (i2c_probe(pmic_i2c_addr)) {
+ ret = i2c_probe(pmic_i2c_addr);
+ pmic_deselect(old_bus);
+ if (ret) {
printf("Can't find PMIC:%s\n", p->name);
return -1;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [U-Boot] [PATCH V3 2/6] power: Explicitly select pmic device's bus
2013-11-12 10:04 ` [U-Boot] [PATCH V3 2/6] power: Explicitly select pmic device's bus Leela Krishna Amudala
@ 2013-12-05 5:50 ` Minkyu Kang
2014-01-02 23:37 ` Leela Krishna Amudala
0 siblings, 1 reply; 12+ messages in thread
From: Minkyu Kang @ 2013-12-05 5:50 UTC (permalink / raw)
To: u-boot
Dear Leela Krishna Amudala,
On 12/11/13 19:04, Leela Krishna Amudala wrote:
> The current pmic i2c code assumes the current i2c bus is
> the same as the pmic device's bus. There is nothing ensuring
> that to be true. Therefore, select the proper bus before performing
> a transaction.
>
> Signed-off-by: Aaron Durbin <adurbin@chromium.org>
> Signed-off-by: Simon Glass <sjg@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>
> ---
> drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
> index ac76870..3cafa4d 100644
> --- a/drivers/power/power_i2c.c
> +++ b/drivers/power/power_i2c.c
> @@ -16,9 +16,45 @@
> #include <i2c.h>
> #include <compiler.h>
>
> +static int pmic_select(struct pmic *p)
> +{
> + int ret, old_bus;
I think, old prefix is meaningless.
please fix it globally.
> +
> + old_bus = i2c_get_bus_num();
> + if (old_bus != p->bus) {
How about return immediately if get a bus.
if (old_bus == p->bus)
return old_bus;
> + debug("%s: Select bus %d\n", __func__, p->bus);
> + ret = i2c_set_bus_num(p->bus);
> + if (ret) {
> + debug("%s: Cannot select pmic %s, err %d\n",
> + __func__, p->name, ret);
> + return -1;
> + }
> + }
> +
> + return old_bus;
> +}
> +
> +static int pmic_deselect(int old_bus)
in your patch, you never check a return value.
then is it void function or wrong usage?
And I think pmic_deselect function is almost same with pmic_select.
If you change the parameter for pmic_select to "int bus" then,
What is different with pmic_select?
for example.
bus = pmic_select(p->bus);
/* do something */
pmic_deselect(bus);
is same with.
bus = pmic_select(p->bus);
/* do something */
pmic_select(bus);
How do you think?
> +{
> + int ret;
> +
> + if (old_bus != i2c_get_bus_num()) {
> + ret = i2c_set_bus_num(old_bus);
> + debug("%s: Select bus %d\n", __func__, old_bus);
> + if (ret) {
> + debug("%s: Cannot restore i2c bus, err %d\n",
> + __func__, ret);
> + return -1;
> + }
> + }
> +
> + return 0;
> +}
> +
> int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
> {
> unsigned char buf[4] = { 0 };
> + int ret, old_bus;
>
> if (check_reg(p, reg))
> return -1;
> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
> return -1;
> }
>
> - if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
> + old_bus = pmic_select(p);
> + if (old_bus < 0)
> return -1;
>
> - return 0;
> + ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
> + pmic_deselect(old_bus);
please add blank line.
> + return ret;
> }
>
> int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
> {
> unsigned char buf[4] = { 0 };
> u32 ret_val = 0;
> + int ret, old_bus;
>
> if (check_reg(p, reg))
> return -1;
>
> - if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
> + old_bus = pmic_select(p);
> + if (old_bus < 0)
> return -1;
>
> + ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
> + pmic_deselect(old_bus);
> + if (ret)
> + return ret;
> +
> switch (pmic_i2c_tx_num) {
> case 3:
> if (p->sensor_byte_order == PMIC_SENSOR_BYTE_ORDER_BIG)
> @@ -98,9 +144,15 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>
> int pmic_probe(struct pmic *p)
> {
> - i2c_set_bus_num(p->bus);
> + int ret, old_bus;
> +
> + old_bus = pmic_select(p);
> + if (old_bus < 0)
> + return -1;
please add blank line.
> debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
> - if (i2c_probe(pmic_i2c_addr)) {
> + ret = i2c_probe(pmic_i2c_addr);
> + pmic_deselect(old_bus);
> + if (ret) {
> printf("Can't find PMIC:%s\n", p->name);
> return -1;
> }
>
Thanks,
Minkyu Kang.
^ permalink raw reply [flat|nested] 12+ messages in thread* [U-Boot] [PATCH V3 2/6] power: Explicitly select pmic device's bus
2013-12-05 5:50 ` Minkyu Kang
@ 2014-01-02 23:37 ` Leela Krishna Amudala
2014-01-03 1:11 ` Minkyu Kang
0 siblings, 1 reply; 12+ messages in thread
From: Leela Krishna Amudala @ 2014-01-02 23:37 UTC (permalink / raw)
To: u-boot
Hi Minkyu Kang,
On Thu, Dec 5, 2013 at 2:50 PM, Minkyu Kang <mk7.kang@samsung.com> wrote:
> Dear Leela Krishna Amudala,
>
> On 12/11/13 19:04, Leela Krishna Amudala wrote:
>> The current pmic i2c code assumes the current i2c bus is
>> the same as the pmic device's bus. There is nothing ensuring
>> that to be true. Therefore, select the proper bus before performing
>> a transaction.
>>
>> Signed-off-by: Aaron Durbin <adurbin@chromium.org>
>> Signed-off-by: Simon Glass <sjg@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>
>> ---
>> drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 57 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
>> index ac76870..3cafa4d 100644
>> --- a/drivers/power/power_i2c.c
>> +++ b/drivers/power/power_i2c.c
>> @@ -16,9 +16,45 @@
>> #include <i2c.h>
>> #include <compiler.h>
>>
>> +static int pmic_select(struct pmic *p)
>> +{
>> + int ret, old_bus;
>
> I think, old prefix is meaningless.
> please fix it globally.
>
I think, it is necessary to differentiate with the current bus.
Please see my below commets for clear picture.
>> +
>> + old_bus = i2c_get_bus_num();
>> + if (old_bus != p->bus) {
>
> How about return immediately if get a bus.
>
> if (old_bus == p->bus)
> return old_bus;
>
The current code is also doing the same but in other way.
If old_bus != p->bus then set the new bus otherwise no code to execute
and return old_bus.
This is same as what you suggested.
>> + debug("%s: Select bus %d\n", __func__, p->bus);
>> + ret = i2c_set_bus_num(p->bus);
>> + if (ret) {
>> + debug("%s: Cannot select pmic %s, err %d\n",
>> + __func__, p->name, ret);
>> + return -1;
>> + }
>> + }
>> +
>> + return old_bus;
>> +}
>> +
>> +static int pmic_deselect(int old_bus)
>
> in your patch, you never check a return value.
> then is it void function or wrong usage?
>
Okay, I'll change the function return type.
> And I think pmic_deselect function is almost same with pmic_select.
> If you change the parameter for pmic_select to "int bus" then,
> What is different with pmic_select?
>
> for example.
>
> bus = pmic_select(p->bus);
> /* do something */
> pmic_deselect(bus);
>
> is same with.
>
> bus = pmic_select(p->bus);
> /* do something */
> pmic_select(bus);
>
> How do you think?
>
Yes, pmic_deselect is almost same as pmic_select but there is a minor
difference.
pmic_select() is used to set new bus and this function returns the old
bus number. we will hold this old_bus number and once we are done with
our work we have to restore the old_bus so we are passing old_bus to
pmic_deselect()
Now, pmic_deselect() takes old_bus as argument and trying to set it.
This function won't return any bus number.
Hope this explanation helps.
>> +{
>> + int ret;
>> +
>> + if (old_bus != i2c_get_bus_num()) {
>> + ret = i2c_set_bus_num(old_bus);
>> + debug("%s: Select bus %d\n", __func__, old_bus);
>> + if (ret) {
>> + debug("%s: Cannot restore i2c bus, err %d\n",
>> + __func__, ret);
>> + return -1;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>> {
>> unsigned char buf[4] = { 0 };
>> + int ret, old_bus;
>>
>> if (check_reg(p, reg))
>> return -1;
>> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>> return -1;
>> }
>>
>> - if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>> + old_bus = pmic_select(p);
>> + if (old_bus < 0)
>> return -1;
>>
>> - return 0;
>> + ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
>> + pmic_deselect(old_bus);
>
> please add blank line.
>
Okay, will do it.
>> + return ret;
>> }
>>
>> int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>> {
>> unsigned char buf[4] = { 0 };
>> u32 ret_val = 0;
>> + int ret, old_bus;
>>
>> if (check_reg(p, reg))
>> return -1;
>>
>> - if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>> + old_bus = pmic_select(p);
>> + if (old_bus < 0)
>> return -1;
>>
>> + ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
>> + pmic_deselect(old_bus);
>> + if (ret)
>> + return ret;
>> +
>> switch (pmic_i2c_tx_num) {
>> case 3:
>> if (p->sensor_byte_order == PMIC_SENSOR_BYTE_ORDER_BIG)
>> @@ -98,9 +144,15 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>>
>> int pmic_probe(struct pmic *p)
>> {
>> - i2c_set_bus_num(p->bus);
>> + int ret, old_bus;
>> +
>> + old_bus = pmic_select(p);
>> + if (old_bus < 0)
>> + return -1;
>
> please add blank line.
>
Okay, will do it.
Best Wishes,
Leela Krishna.
>> debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
>> - if (i2c_probe(pmic_i2c_addr)) {
>> + ret = i2c_probe(pmic_i2c_addr);
>> + pmic_deselect(old_bus);
>> + if (ret) {
>> printf("Can't find PMIC:%s\n", p->name);
>> return -1;
>> }
>>
>
> Thanks,
> Minkyu Kang.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
^ permalink raw reply [flat|nested] 12+ messages in thread* [U-Boot] [PATCH V3 2/6] power: Explicitly select pmic device's bus
2014-01-02 23:37 ` Leela Krishna Amudala
@ 2014-01-03 1:11 ` Minkyu Kang
0 siblings, 0 replies; 12+ messages in thread
From: Minkyu Kang @ 2014-01-03 1:11 UTC (permalink / raw)
To: u-boot
On 03/01/14 08:37, Leela Krishna Amudala wrote:
> Hi Minkyu Kang,
>
> On Thu, Dec 5, 2013 at 2:50 PM, Minkyu Kang <mk7.kang@samsung.com> wrote:
>> Dear Leela Krishna Amudala,
>>
>> On 12/11/13 19:04, Leela Krishna Amudala wrote:
>>> The current pmic i2c code assumes the current i2c bus is
>>> the same as the pmic device's bus. There is nothing ensuring
>>> that to be true. Therefore, select the proper bus before performing
>>> a transaction.
>>>
>>> Signed-off-by: Aaron Durbin <adurbin@chromium.org>
>>> Signed-off-by: Simon Glass <sjg@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>
>>> ---
>>> drivers/power/power_i2c.c | 62 +++++++++++++++++++++++++++++++++++++++++----
>>> 1 file changed, 57 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/power/power_i2c.c b/drivers/power/power_i2c.c
>>> index ac76870..3cafa4d 100644
>>> --- a/drivers/power/power_i2c.c
>>> +++ b/drivers/power/power_i2c.c
>>> @@ -16,9 +16,45 @@
>>> #include <i2c.h>
>>> #include <compiler.h>
>>>
>>> +static int pmic_select(struct pmic *p)
>>> +{
>>> + int ret, old_bus;
>>
>> I think, old prefix is meaningless.
>> please fix it globally.
>>
>
> I think, it is necessary to differentiate with the current bus.
> Please see my below commets for clear picture.
there's no current bus variable.
also, we knew that p->bus is current bus.
>
>>> +
>>> + old_bus = i2c_get_bus_num();
>>> + if (old_bus != p->bus) {
>>
>> How about return immediately if get a bus.
>>
>> if (old_bus == p->bus)
>> return old_bus;
>>
>
> The current code is also doing the same but in other way.
> If old_bus != p->bus then set the new bus otherwise no code to execute
> and return old_bus.
> This is same as what you suggested.
I know.
I'm talking about code quality.
please think, which one is better.
>
>>> + debug("%s: Select bus %d\n", __func__, p->bus);
>>> + ret = i2c_set_bus_num(p->bus);
>>> + if (ret) {
>>> + debug("%s: Cannot select pmic %s, err %d\n",
>>> + __func__, p->name, ret);
>>> + return -1;
>>> + }
>>> + }
>>> +
>>> + return old_bus;
>>> +}
>>> +
>>> +static int pmic_deselect(int old_bus)
>>
>> in your patch, you never check a return value.
>> then is it void function or wrong usage?
>>
>
> Okay, I'll change the function return type.
>
>> And I think pmic_deselect function is almost same with pmic_select.
>> If you change the parameter for pmic_select to "int bus" then,
>> What is different with pmic_select?
>>
>> for example.
>>
>> bus = pmic_select(p->bus);
>> /* do something */
>> pmic_deselect(bus);
>>
>> is same with.
>>
>> bus = pmic_select(p->bus);
>> /* do something */
>> pmic_select(bus);
>>
>> How do you think?
>>
>
> Yes, pmic_deselect is almost same as pmic_select but there is a minor
> difference.
>
> pmic_select() is used to set new bus and this function returns the old
> bus number. we will hold this old_bus number and once we are done with
> our work we have to restore the old_bus so we are passing old_bus to
> pmic_deselect()
>
> Now, pmic_deselect() takes old_bus as argument and trying to set it.
> This function won't return any bus number.
>
> Hope this explanation helps.
I know.
the focus is that almost codes were duplicated.
My suggestion is one of example for reducing code duplication.
Please think about it.
>
>>> +{
>>> + int ret;
>>> +
>>> + if (old_bus != i2c_get_bus_num()) {
>>> + ret = i2c_set_bus_num(old_bus);
>>> + debug("%s: Select bus %d\n", __func__, old_bus);
>>> + if (ret) {
>>> + debug("%s: Cannot restore i2c bus, err %d\n",
>>> + __func__, ret);
>>> + return -1;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>>> {
>>> unsigned char buf[4] = { 0 };
>>> + int ret, old_bus;
>>>
>>> if (check_reg(p, reg))
>>> return -1;
>>> @@ -52,23 +88,33 @@ int pmic_reg_write(struct pmic *p, u32 reg, u32 val)
>>> return -1;
>>> }
>>>
>>> - if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>>> + old_bus = pmic_select(p);
>>> + if (old_bus < 0)
>>> return -1;
>>>
>>> - return 0;
>>> + ret = i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
>>> + pmic_deselect(old_bus);
>>
>> please add blank line.
>>
>
> Okay, will do it.
>
>>> + return ret;
>>> }
>>>
>>> int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>>> {
>>> unsigned char buf[4] = { 0 };
>>> u32 ret_val = 0;
>>> + int ret, old_bus;
>>>
>>> if (check_reg(p, reg))
>>> return -1;
>>>
>>> - if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>>> + old_bus = pmic_select(p);
>>> + if (old_bus < 0)
>>> return -1;
>>>
>>> + ret = i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num);
>>> + pmic_deselect(old_bus);
>>> + if (ret)
>>> + return ret;
>>> +
>>> switch (pmic_i2c_tx_num) {
>>> case 3:
>>> if (p->sensor_byte_order == PMIC_SENSOR_BYTE_ORDER_BIG)
>>> @@ -98,9 +144,15 @@ int pmic_reg_read(struct pmic *p, u32 reg, u32 *val)
>>>
>>> int pmic_probe(struct pmic *p)
>>> {
>>> - i2c_set_bus_num(p->bus);
>>> + int ret, old_bus;
>>> +
>>> + old_bus = pmic_select(p);
>>> + if (old_bus < 0)
>>> + return -1;
>>
>> please add blank line.
>>
>
> Okay, will do it.
>
> Best Wishes,
> Leela Krishna.
>
>>> debug("Bus: %d PMIC:%s probed!\n", p->bus, p->name);
>>> - if (i2c_probe(pmic_i2c_addr)) {
>>> + ret = i2c_probe(pmic_i2c_addr);
>>> + pmic_deselect(old_bus);
>>> + if (ret) {
>>> printf("Can't find PMIC:%s\n", p->name);
>>> return -1;
>>> }
>>>
>>
>> Thanks,
>> Minkyu Kang.
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
Thanks,
Minkyu Kang.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [U-Boot] [PATCH V3 3/6] FDT: Exynos5420: Add compatible srings for PMIC
2013-11-12 10:04 [U-Boot] [PATCH V3 0/6] SMDK5420: Add S2MPS11 pmic support to SMDK5420 Leela Krishna Amudala
2013-11-12 10:04 ` [U-Boot] [PATCH V3 1/6] exynos: Use common pmic_reg_update() definition Leela Krishna Amudala
2013-11-12 10:04 ` [U-Boot] [PATCH V3 2/6] power: Explicitly select pmic device's bus Leela Krishna Amudala
@ 2013-11-12 10:04 ` Leela Krishna Amudala
2013-11-12 10:04 ` [U-Boot] [PATCH V3 4/6] SMDK5420: S2MPS11: Adds the register settings for S2MPS11 Leela Krishna Amudala
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Leela Krishna Amudala @ 2013-11-12 10:04 UTC (permalink / raw)
To: u-boot
Add required compatible strings for PMIC S2MPS11
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
include/fdtdec.h | 1 +
lib/fdtdec.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/include/fdtdec.h b/include/fdtdec.h
index 6bf83bf..6290078 100644
--- a/include/fdtdec.h
+++ b/include/fdtdec.h
@@ -85,6 +85,7 @@ enum fdt_compat_id {
COMPAT_INFINEON_SLB9635_TPM, /* Infineon SLB9635 TPM */
COMPAT_INFINEON_SLB9645_TPM, /* Infineon SLB9645 TPM */
COMPAT_SAMSUNG_EXYNOS5_I2C, /* Exynos5 High Speed I2C Controller */
+ COMPAT_SAMSUNG_S2MPS11_PMIC, /* S2MPS11 PMIC */
COMPAT_COUNT,
};
diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index dc35856..0ea1c08 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -58,6 +58,7 @@ static const char * const compat_names[COMPAT_COUNT] = {
COMPAT(INFINEON_SLB9635_TPM, "infineon,slb9635-tpm"),
COMPAT(INFINEON_SLB9645_TPM, "infineon,slb9645-tpm"),
COMPAT(SAMSUNG_EXYNOS5_I2C, "samsung,exynos5-hsi2c"),
+ COMPAT(SAMSUNG_S2MPS11_PMIC, "samsung,s2mps11-pmic"),
};
const char *fdtdec_get_compatible(enum fdt_compat_id id)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [U-Boot] [PATCH V3 4/6] SMDK5420: S2MPS11: Adds the register settings for S2MPS11
2013-11-12 10:04 [U-Boot] [PATCH V3 0/6] SMDK5420: Add S2MPS11 pmic support to SMDK5420 Leela Krishna Amudala
` (2 preceding siblings ...)
2013-11-12 10:04 ` [U-Boot] [PATCH V3 3/6] FDT: Exynos5420: Add compatible srings for PMIC Leela Krishna Amudala
@ 2013-11-12 10:04 ` Leela Krishna Amudala
2013-11-12 10:04 ` [U-Boot] [PATCH V3 5/6] exynos: Add a common DT based PMIC driver initialization Leela Krishna Amudala
2013-11-12 10:04 ` [U-Boot] [PATCH V3 6/6] config: SMDK5420: Enable S2MPS11 pmic Leela Krishna Amudala
5 siblings, 0 replies; 12+ messages in thread
From: Leela Krishna Amudala @ 2013-11-12 10:04 UTC (permalink / raw)
To: u-boot
Adds the register settings, addresses and voltages associated with S2MPS11
Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
Reviewed-by: Vadim Bendebury <vbendeb@google.com>
Reviewed-by: Lukasz Majewski <l.majewski@samsung.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
include/power/s2mps11_pmic.h | 141 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 141 insertions(+)
create mode 100644 include/power/s2mps11_pmic.h
diff --git a/include/power/s2mps11_pmic.h b/include/power/s2mps11_pmic.h
new file mode 100644
index 0000000..20c781d
--- /dev/null
+++ b/include/power/s2mps11_pmic.h
@@ -0,0 +1,141 @@
+/*
+ * s2mps11_pmic.h
+ *
+ * Copyright (c) 2012 Samsung Electronics Co., Ltd
+ * http://www.samsung.com
+ *
+ * SPDX-License-Identifier: GPL-2.0+
+ *
+ */
+#ifndef __S2MPS11_H
+#define __S2MPS11_H
+
+/* S2MPS11 registers */
+enum s2mps11_reg {
+ S2MPS11_REG_ID,
+ S2MPS11_REG_INT1,
+ S2MPS11_REG_INT2,
+ S2MPS11_REG_INT3,
+ S2MPS11_REG_INT1M,
+ S2MPS11_REG_INT2M,
+ S2MPS11_REG_INT3M,
+ S2MPS11_REG_ST1,
+ S2MPS11_REG_ST2,
+ S2MPS11_REG_OFFSRC,
+ S2MPS11_REG_PWRONSRC,
+ S2MPS11_REG_RTC_CTRL,
+ S2MPS11_REG_CTRL1,
+ S2MPS11_REG_ETC_TEST,
+ S2MPS11_REG_RSVD3,
+ S2MPS11_REG_BU_CHG,
+ S2MPS11_REG_RAMP,
+ S2MPS11_REG_RAMP_BUCK,
+ S2MPS11_REG_LDO1_8,
+ S2MPS11_REG_LDO9_16,
+ S2MPS11_REG_LDO17_24,
+ S2MPS11_REG_LDO25_32,
+ S2MPS11_REG_LDO33_38,
+ S2MPS11_REG_LDO1_8_1,
+ S2MPS11_REG_LDO9_16_1,
+ S2MPS11_REG_LDO17_24_1,
+ S2MPS11_REG_LDO25_32_1,
+ S2MPS11_REG_LDO33_38_1,
+ S2MPS11_REG_OTP_ADRL,
+ S2MPS11_REG_OTP_ADRH,
+ S2MPS11_REG_OTP_DATA,
+ S2MPS11_REG_MON1SEL,
+ S2MPS11_REG_MON2SEL,
+ S2MPS11_REG_LEE,
+ S2MPS11_REG_RSVD_NO,
+ S2MPS11_REG_UVLO,
+ S2MPS11_REG_LEE_NO,
+ S2MPS11_REG_B1CTRL1,
+ S2MPS11_REG_B1CTRL2,
+ S2MPS11_REG_B2CTRL1,
+ S2MPS11_REG_B2CTRL2,
+ S2MPS11_REG_B3CTRL1,
+ S2MPS11_REG_B3CTRL2,
+ S2MPS11_REG_B4CTRL1,
+ S2MPS11_REG_B4CTRL2,
+ S2MPS11_REG_B5CTRL1,
+ S2MPS11_REG_BUCK5_SW,
+ S2MPS11_REG_B5CTRL2,
+ S2MPS11_REG_B5CTRL3,
+ S2MPS11_REG_B5CTRL4,
+ S2MPS11_REG_B5CTRL5,
+ S2MPS11_REG_B6CTRL1,
+ S2MPS11_REG_B6CTRL2,
+ S2MPS11_REG_B7CTRL1,
+ S2MPS11_REG_B7CTRL2,
+ S2MPS11_REG_B8CTRL1,
+ S2MPS11_REG_B8CTRL2,
+ S2MPS11_REG_B9CTRL1,
+ S2MPS11_REG_B9CTRL2,
+ S2MPS11_REG_B10CTRL1,
+ S2MPS11_REG_B10CTRL2,
+ S2MPS11_REG_L1CTRL,
+ S2MPS11_REG_L2CTRL,
+ S2MPS11_REG_L3CTRL,
+ S2MPS11_REG_L4CTRL,
+ S2MPS11_REG_L5CTRL,
+ S2MPS11_REG_L6CTRL,
+ S2MPS11_REG_L7CTRL,
+ S2MPS11_REG_L8CTRL,
+ S2MPS11_REG_L9CTRL,
+ S2MPS11_REG_L10CTRL,
+ S2MPS11_REG_L11CTRL,
+ S2MPS11_REG_L12CTRL,
+ S2MPS11_REG_L13CTRL,
+ S2MPS11_REG_L14CTRL,
+ S2MPS11_REG_L15CTRL,
+ S2MPS11_REG_L16CTRL,
+ S2MPS11_REG_L17CTRL,
+ S2MPS11_REG_L18CTRL,
+ S2MPS11_REG_L19CTRL,
+ S2MPS11_REG_L20CTRL,
+ S2MPS11_REG_L21CTRL,
+ S2MPS11_REG_L22CTRL,
+ S2MPS11_REG_L23CTRL,
+ S2MPS11_REG_L24CTRL,
+ S2MPS11_REG_L25CTRL,
+ S2MPS11_REG_L26CTRL,
+ S2MPS11_REG_L27CTRL,
+ S2MPS11_REG_L28CTRL,
+ S2MPS11_REG_L29CTRL,
+ S2MPS11_REG_L30CTRL,
+ S2MPS11_REG_L31CTRL,
+ S2MPS11_REG_L32CTRL,
+ S2MPS11_REG_L33CTRL,
+ S2MPS11_REG_L34CTRL,
+ S2MPS11_REG_L35CTRL,
+ S2MPS11_REG_L36CTRL,
+ S2MPS11_REG_L37CTRL,
+ S2MPS11_REG_L38CTRL,
+
+ S2MPS11_NUM_OF_REGS,
+};
+
+/* I2C device address for pmic S2MPS11 */
+#define S2MPS11_I2C_ADDR (0xCC >> 1)
+#define S2MPS11_BUS_NUM 4
+
+/* Value to set voltage as 1V */
+#define S2MPS11_BUCK_CTRL2_1V 0x40
+/* Value to set voltage as 1.2V */
+#define S2MPS11_BUCK_CTRL2_1_2V 0x60
+/* Value to set voltage as 1.2625V */
+#define S2MPS11_BUCK_CTRL2_1_2625V 0x6A
+
+/* Buck register addresses */
+#define S2MPS11_BUCK1_CTRL2 0x26
+#define S2MPS11_BUCK2_CTRL2 0x28
+#define S2MPS11_BUCK3_CTRL2 0x2a
+#define S2MPS11_BUCK4_CTRL2 0x2c
+#define S2MPS11_BUCK6_CTRL2 0x34
+#define S2MPS11_LDO22_CTRL 0x52
+
+#define S2MPS11_DEVICE_NAME "S2MPS11_PMIC"
+
+#define S2MPS11_RTC_CTRL_32KHZ_CP_EN (1 << 1)
+#define S2MPS11_RTC_CTRL_JIT (1 << 4)
+#endif /* __LINUX_MFD_S2MPS11_H */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [U-Boot] [PATCH V3 5/6] exynos: Add a common DT based PMIC driver initialization
2013-11-12 10:04 [U-Boot] [PATCH V3 0/6] SMDK5420: Add S2MPS11 pmic support to SMDK5420 Leela Krishna Amudala
` (3 preceding siblings ...)
2013-11-12 10:04 ` [U-Boot] [PATCH V3 4/6] SMDK5420: S2MPS11: Adds the register settings for S2MPS11 Leela Krishna Amudala
@ 2013-11-12 10:04 ` Leela Krishna Amudala
2013-11-12 10:04 ` [U-Boot] [PATCH V3 6/6] config: SMDK5420: Enable S2MPS11 pmic Leela Krishna Amudala
5 siblings, 0 replies; 12+ messages in thread
From: Leela Krishna Amudala @ 2013-11-12 10:04 UTC (permalink / raw)
To: u-boot
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 1959c4e..cc83724 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>
DECLARE_GLOBAL_DATA_PTR;
@@ -154,6 +155,25 @@ static int board_init_cros_ec_devices(const void *blob)
}
#endif
+#ifdef CONFIG_POWER_S2MPS11
+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)
@@ -259,6 +279,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 11b3d03..ec0a992 100644
--- a/drivers/power/pmic/Makefile
+++ b/drivers/power/pmic/Makefile
@@ -15,6 +15,7 @@ COBJS-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o
COBJS-$(CONFIG_POWER_MAX77686) += pmic_max77686.o
COBJS-$(CONFIG_POWER_TPS65217) += pmic_tps65217.o
COBJS-$(CONFIG_POWER_TPS65910) += pmic_tps65910.o
+COBJS-$(CONFIG_POWER) += pmic_common.o
COBJS := $(COBJS-y)
SRCS := $(COBJS:.o=.c)
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.
+ *
+ * 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>
+
+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);
+ 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 e715435..6a6769f 100644
--- a/drivers/power/power_core.c
+++ b/drivers/power/power_core.c
@@ -227,6 +227,20 @@ int pmic_reg_update(struct pmic *p, int reg, uint regval)
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 d17dbdc..f7d3cf4 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, uint regval);
+/*
+ * 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;
+};
+
+/**
+ * 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)
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* [U-Boot] [PATCH V3 6/6] config: SMDK5420: Enable S2MPS11 pmic
2013-11-12 10:04 [U-Boot] [PATCH V3 0/6] SMDK5420: Add S2MPS11 pmic support to SMDK5420 Leela Krishna Amudala
` (4 preceding siblings ...)
2013-11-12 10:04 ` [U-Boot] [PATCH V3 5/6] exynos: Add a common DT based PMIC driver initialization Leela Krishna Amudala
@ 2013-11-12 10:04 ` Leela Krishna Amudala
5 siblings, 0 replies; 12+ messages in thread
From: Leela Krishna Amudala @ 2013-11-12 10:04 UTC (permalink / raw)
To: u-boot
configure S2MPS11 pmic on SMDK5420
Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com>
Acked-by: Simon Glass <sjg@chromium.org>
---
include/configs/smdk5420.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/configs/smdk5420.h b/include/configs/smdk5420.h
index 447f8e5..46aeec0 100644
--- a/include/configs/smdk5420.h
+++ b/include/configs/smdk5420.h
@@ -53,4 +53,8 @@
#define CONFIG_MAX_I2C_NUM 11
+#define CONFIG_POWER
+#define CONFIG_POWER_I2C
+#define CONFIG_POWER_S2MPS11
+
#endif /* __CONFIG_5420_H */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 12+ messages in thread