Re: [U-Boot] [PATCH v2 2/3] pmic:pfuze implement regulator mode set
Hi, Przemyslaw
On 1/15/2015 10:58 PM, Przemyslaw Marczak wrote:
Hello Peng,
On 01/15/2015 11:18 AM, Peng Fan wrote:
This patch is to implement pfuze_mode_init and pfuze_regulator_mode_set
function, and add prototype in header file.
pfuze_mode_init is to set switching mode for all buck regulators to
improve system efficiency.
pfuze_regulator_mode_set is to set the regulator's mode according input
parameter.
Mode:
OFF: The regulator is switched off and the output voltage is discharged.
PFM: In this mode, the regulator is always in PFM mode, which
is useful at light loads for optimized efficiency.
PWM: In this mode, the regulator is always in PWM mode operation
regardless of load conditions.
APS: In this mode, the regulator moves automatically between
pulse skipping mode and PWM mode depending on load conditions.
Signed-off-by: Peng Fan
---
Changes v2:
implement one function mode for one regulator.
board/freescale/common/pfuze.c | 43
++
board/freescale/common/pfuze.h | 2 ++
2 files changed, 45 insertions(+)
diff --git a/board/freescale/common/pfuze.c
b/board/freescale/common/pfuze.c
index 2cd1794..f2fffc1 100644
--- a/board/freescale/common/pfuze.c
+++ b/board/freescale/common/pfuze.c
@@ -8,6 +8,49 @@
#include
#include
+/* Set one switch regulator's mode */
This is not exactly what I mean.
Actually by doing this, we have pmic_reg_write() function with just
the other name.
+int pfuze_regulator_mode_set(struct pmic *p, u32 regulator, u32 mode)
+{
+return pmic_reg_write(p, regulator, mode);
+}
The 'u32 regulator' suggests to pass the regulator number rather than
defined mode register address - little unclear.
And the "pfuze.." function name is general and should be implemented
by pfuze driver, which could be 'temporary' implemented in:
drivers/power/pmic/pmic_pfuze100.c
And this driver should take care of the PFUZE id and write the given
mode to proper pmic register address.
I mean 'temporary' because, soon we will move to the next pmic
framework, finally I hope to send it at the end of the next week.
+
+/* Set all switch regulators' working mode */
So I think, that the below function without the loop could be
implemented in the pfuze driver file.
+int pfuze_mode_init(struct pmic *p, u32 mode)
Starting with some like this:
int pfuze_sw_regulator_mode_set(struct pmic *p, u32 ??switch_num??,
u32 mode)
+{
+unsigned char offset, i, switch_num;
+u32 id, ret;
+
+pmic_reg_read(p, PFUZE100_DEVICEID, &id);
+id = id & 0xf;
+
+if (id == 0) {
+switch_num = 6;
+offset = PFUZE100_SW1CMODE;
+} else if (id == 1) {
+switch_num = 4;
+offset = PFUZE100_SW2MODE;
+} else {
+printf("Not supported, id=%d\n", id);
+return -1;
+}
+
Ending with something like this:
ret = pmic_reg_write(p, offset + switch_num * 7, mode);
if (ret ...
...
}
And this code below can stay in the 'common.c' code, with small changes:
ret = pfuze_sw_regulator_mode_set(p, 1, mode);
+ret = pfuze_regulator_mode_set(p, PFUZE100_SW1ABMODE, mode);
+if (ret < 0) {
+printf("Set SW1AB mode error!\n");
+return ret;
+}
+
+for (i = 0; i < switch_num - 1; i++) {
ret = pfuze_sw_regulator_mode_set(p, i, mode);
+ret = pfuze_regulator_mode_set(p, offset + i * 7, mode);
+if (ret < 0) {
+printf("Set switch%x mode error!\n", offset + i * 7);
+return ret;
+}
+}
+
+return ret;
+}
+
struct pmic *pfuze_common_init(unsigned char i2cbus)
{
struct pmic *p;
diff --git a/board/freescale/common/pfuze.h
b/board/freescale/common/pfuze.h
index 7a4126c..7c316c6 100644
--- a/board/freescale/common/pfuze.h
+++ b/board/freescale/common/pfuze.h
@@ -8,5 +8,7 @@
#define __PFUZE_BOARD_HELPER__
struct pmic *pfuze_common_init(unsigned char i2cbus);
+int pfuze_mode_init(struct pmic *p, u32 mode);
+int pfuze_regulator_mode_set(struct pmic *p, u32 regulator, u32 mode);
And this:
int pfuze_sw_regulator_mode_set(...);
should go here:
include/power/pfuze100_pmic.h
#endif
I suggested the 'sw' in the function name for the switching regulator
type, so then we can pass just switching regulator number as an
argument: e.g. 1,2,3...
And maybe in the future you will need ldo or some else regulator type,
so adding 'pfuze_ldo_regulator_mode_set' function, will keep the code
consistence.
I would like to keep the driver specific code in driver file and
common code in the common file.
Thanks for your review.
Get it. I think this first version is fine for me now, this new feature
can be done in future patch.
I think it is reasonable. If you now prefer to keep the first version
of this patch set, then you have still my ACK for it:).
Best regards,
Regards,
Peng.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/li
Re: [U-Boot] [PATCH v2 2/3] pmic:pfuze implement regulator mode set
Hello Peng,
On 01/15/2015 11:18 AM, Peng Fan wrote:
This patch is to implement pfuze_mode_init and pfuze_regulator_mode_set
function, and add prototype in header file.
pfuze_mode_init is to set switching mode for all buck regulators to
improve system efficiency.
pfuze_regulator_mode_set is to set the regulator's mode according input
parameter.
Mode:
OFF: The regulator is switched off and the output voltage is discharged.
PFM: In this mode, the regulator is always in PFM mode, which
is useful at light loads for optimized efficiency.
PWM: In this mode, the regulator is always in PWM mode operation
regardless of load conditions.
APS: In this mode, the regulator moves automatically between
pulse skipping mode and PWM mode depending on load conditions.
Signed-off-by: Peng Fan
---
Changes v2:
implement one function mode for one regulator.
board/freescale/common/pfuze.c | 43 ++
board/freescale/common/pfuze.h | 2 ++
2 files changed, 45 insertions(+)
diff --git a/board/freescale/common/pfuze.c b/board/freescale/common/pfuze.c
index 2cd1794..f2fffc1 100644
--- a/board/freescale/common/pfuze.c
+++ b/board/freescale/common/pfuze.c
@@ -8,6 +8,49 @@
#include
#include
+/* Set one switch regulator's mode */
This is not exactly what I mean.
Actually by doing this, we have pmic_reg_write() function with just the
other name.
+int pfuze_regulator_mode_set(struct pmic *p, u32 regulator, u32 mode)
+{
+ return pmic_reg_write(p, regulator, mode);
+}
The 'u32 regulator' suggests to pass the regulator number rather than
defined mode register address - little unclear.
And the "pfuze.." function name is general and should be implemented by
pfuze driver, which could be 'temporary' implemented in:
drivers/power/pmic/pmic_pfuze100.c
And this driver should take care of the PFUZE id and write the given
mode to proper pmic register address.
I mean 'temporary' because, soon we will move to the next pmic
framework, finally I hope to send it at the end of the next week.
+
+/* Set all switch regulators' working mode */
So I think, that the below function without the loop could be
implemented in the pfuze driver file.
+int pfuze_mode_init(struct pmic *p, u32 mode)
Starting with some like this:
int pfuze_sw_regulator_mode_set(struct pmic *p, u32 ??switch_num??, u32
mode)
+{
+ unsigned char offset, i, switch_num;
+ u32 id, ret;
+
+ pmic_reg_read(p, PFUZE100_DEVICEID, &id);
+ id = id & 0xf;
+
+ if (id == 0) {
+ switch_num = 6;
+ offset = PFUZE100_SW1CMODE;
+ } else if (id == 1) {
+ switch_num = 4;
+ offset = PFUZE100_SW2MODE;
+ } else {
+ printf("Not supported, id=%d\n", id);
+ return -1;
+ }
+
Ending with something like this:
ret = pmic_reg_write(p, offset + switch_num * 7, mode);
if (ret ...
...
}
And this code below can stay in the 'common.c' code, with small changes:
ret = pfuze_sw_regulator_mode_set(p, 1, mode);
+ ret = pfuze_regulator_mode_set(p, PFUZE100_SW1ABMODE, mode);
+ if (ret < 0) {
+ printf("Set SW1AB mode error!\n");
+ return ret;
+ }
+
+ for (i = 0; i < switch_num - 1; i++) {
ret = pfuze_sw_regulator_mode_set(p, i, mode);
+ ret = pfuze_regulator_mode_set(p, offset + i * 7, mode);
+ if (ret < 0) {
+ printf("Set switch%x mode error!\n", offset + i * 7);
+ return ret;
+ }
+ }
+
+ return ret;
+}
+
struct pmic *pfuze_common_init(unsigned char i2cbus)
{
struct pmic *p;
diff --git a/board/freescale/common/pfuze.h b/board/freescale/common/pfuze.h
index 7a4126c..7c316c6 100644
--- a/board/freescale/common/pfuze.h
+++ b/board/freescale/common/pfuze.h
@@ -8,5 +8,7 @@
#define __PFUZE_BOARD_HELPER__
struct pmic *pfuze_common_init(unsigned char i2cbus);
+int pfuze_mode_init(struct pmic *p, u32 mode);
+int pfuze_regulator_mode_set(struct pmic *p, u32 regulator, u32 mode);
And this:
int pfuze_sw_regulator_mode_set(...);
should go here:
include/power/pfuze100_pmic.h
#endif
I suggested the 'sw' in the function name for the switching regulator
type, so then we can pass just switching regulator number as an
argument: e.g. 1,2,3...
And maybe in the future you will need ldo or some else regulator type,
so adding 'pfuze_ldo_regulator_mode_set' function, will keep the code
consistence.
I would like to keep the driver specific code in driver file and common
code in the common file.
I think it is reasonable. If you now prefer to keep the first version of
this patch set, then you have still my ACK for it:).
Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marc...@samsung.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://l
