[U-Boot] [PATCH v2 1/3] arm: odroid: pmic77686: allow bucket voltage settings

[Przemyslaw Marczak]

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 <jh80.chung at samsung.com> 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 <suriyan.r at gmail.com>
>>> ---
>>>   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