From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Mon, 27 Oct 2014 12:08:03 +0100 Subject: [U-Boot] [PATCH v2 1/3] arm: odroid: pmic77686: allow bucket voltage settings In-Reply-To: References: <1413827523-8341-1-git-send-email-suriyan.r@gmail.com> <5449F71D.9010101@samsung.com> Message-ID: <544E2793.8090804@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 Suriyan, On 10/24/2014 05:53 PM, Suriyan Ramasami wrote: > Hello Jaehoon Chung, > > > On Thu, Oct 23, 2014 at 11:52 PM, Jaehoon Chung wrote: >> Hi. >> >> On 10/21/2014 02:52 AM, Suriyan Ramasami wrote: >>> Allow to set the bucket voltage for the max77686. >>> This will be used to reset the SMC LAN9730 ethernet on the odroids. >>> >>> Signed-off-by: Suriyan Ramasami >>> --- >>> drivers/power/pmic/pmic_max77686.c | 48 +++++++++++++++++++++++++++++++++++++- >>> include/power/max77686_pmic.h | 3 +++ >>> 2 files changed, 50 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/power/pmic/pmic_max77686.c b/drivers/power/pmic/pmic_max77686.c >>> index df1fd91..1aeadb4 100644 >>> --- a/drivers/power/pmic/pmic_max77686.c >>> +++ b/drivers/power/pmic/pmic_max77686.c >>> @@ -42,6 +42,25 @@ static unsigned int max77686_ldo_volt2hex(int ldo, ulong uV) >>> return 0; >>> } >>> >>> +static unsigned int max77686_buck_volt2hex(int buck, ulong uV) >>> +{ >>> + unsigned int hex = 0; >>> + >>> + if (buck < 5 || buck > 9) { >>> + debug("%s: buck %d is not supported\n", __func__, buck); >>> + return 0; > > Here, I should return MAX77686_BUCK_VOLT_MAX_HEX + 1 instead of 0 to > signal an error condition, as 0 is a valid non error value. > He 'hex' value has at most 1 byte of len, so you can return the 'int' value and use the negative errno numbers - and there is no value conflicts. >>> + } >>> + >>> + hex = (uV - 750000) / 50000; >>> + >>> + if (hex >= 0 && hex <= MAX77686_BUCK_VOLT_MAX_HEX) >>> + return hex; >>> + >>> + debug("%s: %ld is wrong voltage value for BUCK%d\n", >>> + __func__, uV, buck); >>> + return MAX77686_BUCK_VOLT_MAX_HEX + 1; >>> +} >>> + >>> int max77686_set_ldo_voltage(struct pmic *p, int ldo, ulong uV) >>> { >>> unsigned int val, ret, hex, adr; >>> @@ -68,6 +87,33 @@ int max77686_set_ldo_voltage(struct pmic *p, int ldo, ulong uV) >>> return ret; >>> } >>> >>> +int max77686_set_buck_voltage(struct pmic *p, int buck, ulong uV) >>> +{ >>> + unsigned int val, ret, hex, adr; >>> + >>> + if (buck < 5 && buck > 9) { >>> + printf("%s: %d is an unsupported bucket number\n", >>> + __func__, buck); >>> + return -1; >> >> How about using error number instead of "-1"? > > Could you please elaborate on this? This function always returns -1 on > failure just like the function max77686_set_ldo_voltate() which I have > tried to mimic as much as I can. > I am guessing that you want me to return -EINVAL in this case? Please > let me know, and I am OK to change it, just that this function will > deviate from the rest of the functions in this file. > Yes, this will be better. >> >>> + } >>> + >>> + adr = max77686_buck_addr[buck] + 1; >>> + hex = max77686_buck_volt2hex(buck, uV); >>> + if (hex < 0) return hex; >>> + if (hex == MAX77686_BUCK_VOLT_MAX_HEX + 1) >>> + return -1; >> >> Ditto. > > I believe, I return -EINVAL here, but please look at my reasoning above. > >> >>> + >>> + ret = pmic_reg_read(p, adr, &val); >>> + if (ret) >>> + return ret; >>> + >>> + val &= ~MAX77686_BUCK_VOLT_MASK; >>> + val |= hex; >>> + ret |= pmic_reg_write(p, adr, val); >> >> ret |= pmic_reg_write(p, addr, val | hex); ? >> > > OK, will change that. Again, this was done to be as close to > max77686_set_ldo_voltate() > > Thanks and Regards! > - Suriyan > >> Best Regards, >> Jaehoon Chung >>> + >>> + return ret; >>> +} >>> + >>> int max77686_set_ldo_mode(struct pmic *p, int ldo, char opmode) >>> { >>> unsigned int val, ret, adr, mode; >>> @@ -157,7 +203,7 @@ int max77686_set_buck_mode(struct pmic *p, int buck, char opmode) >>> /* mode */ >>> switch (opmode) { >>> case OPMODE_OFF: >>> - mode = MAX77686_BUCK_MODE_OFF; >>> + mode = MAX77686_BUCK_MODE_OFF << mode_shift; >>> break; >>> case OPMODE_STANDBY: >>> switch (buck) { >>> diff --git a/include/power/max77686_pmic.h b/include/power/max77686_pmic.h >>> index c2a772a..b0e4255 100644 >>> --- a/include/power/max77686_pmic.h >>> +++ b/include/power/max77686_pmic.h >>> @@ -150,6 +150,7 @@ enum { >>> >>> int max77686_set_ldo_voltage(struct pmic *p, int ldo, ulong uV); >>> int max77686_set_ldo_mode(struct pmic *p, int ldo, char opmode); >>> +int max77686_set_buck_voltage(struct pmic *p, int buck, ulong uV); >>> int max77686_set_buck_mode(struct pmic *p, int buck, char opmode); >>> >>> #define MAX77686_LDO_VOLT_MAX_HEX 0x3f >>> @@ -159,6 +160,8 @@ int max77686_set_buck_mode(struct pmic *p, int buck, char opmode); >>> #define MAX77686_LDO_MODE_STANDBY (0x01 << 0x06) >>> #define MAX77686_LDO_MODE_LPM (0x02 << 0x06) >>> #define MAX77686_LDO_MODE_ON (0x03 << 0x06) >>> +#define MAX77686_BUCK_VOLT_MAX_HEX 0x3f >>> +#define MAX77686_BUCK_VOLT_MASK 0x3f >>> #define MAX77686_BUCK_MODE_MASK 0x03 >>> #define MAX77686_BUCK_MODE_SHIFT_1 0x00 >>> #define MAX77686_BUCK_MODE_SHIFT_2 0x04 >>> >> Best regards, -- Przemyslaw Marczak Samsung R&D Institute Poland Samsung Electronics p.marczak at samsung.com