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