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

[Suriyan Ramasami]

Hello Przemyslaw Marczak,

On Mon, Oct 27, 2014 at 4:08 AM, Przemyslaw Marczak
<p.marczak at samsung.com> wrote:
> 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.
>

OK, Shall use negative errno values.

>
>>>> +     }
>>>> +
>>>> +     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.

OK, shall do so.

>>>
>>>
>>>> +     }
>>>> +
>>>> +     adr = max77686_buck_addr[buck] + 1;
>>>> +     hex = max77686_buck_volt2hex(buck, uV);
>>>> +
>
>
> if (hex < 0)
>         return hex;
>
>

OK, will do so.

>>>> +     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
>>>>
>>>
>

Thanks!
- Suriyan

> Best regards,
> --
> Przemyslaw Marczak
> Samsung R&D Institute Poland
> Samsung Electronics
> p.marczak at samsung.com