From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Fri, 16 Jan 2015 09:59:40 +0800 Subject: [U-Boot] [PATCH v2 2/3] pmic:pfuze implement regulator mode set In-Reply-To: <54B7D58C.4060406@samsung.com> References: <1421317114-15858-1-git-send-email-Peng.Fan@freescale.com> <1421317114-15858-3-git-send-email-Peng.Fan@freescale.com> <54B7D58C.4060406@samsung.com> Message-ID: <54B8708C.3070400@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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.