Re: [PATCH v4 2/3] regulator MAX8998/LP3974: Support DVS-GPIO.

[MyungJoo Ham <myungjoo.ham@samsung.com>]

On Tue, Jan 4, 2011 at 4:49 PM, Lukasz Majewski <l.majewski@samsung.com> wrote:
> On Tue, 04 Jan 2011 14:17:40 +0900
> MyungJoo Ham <myungjoo.ham@samsung.com> wrote:
>
> Hi all,
>
> I've posted some comments below:
>
> if (gpio_is_valid(pdata->buck2_set3)) {
>> -                     if (max8998->buck2_vol[0] == i) {
>> -                             max8998->buck2_idx = 0;
>> -                             buck2_gpio_set(pdata->buck2_set3, 0);
>> -                     } else {
>> -                             max8998->buck2_idx = 1;
>> -                             ret =
>> max8998_get_voltage_register(rdev, &reg, -
>> &shift,
>> -
>> &mask);
>> -                             ret = max8998_write_reg(i2c, reg, i);
>> -                             max8998->buck2_vol[1] = i;
>> -                             buck2_gpio_set(pdata->buck2_set3, 1);
>> +
>> +                     /* check if requested voltage */
>> +                     /* value is already defined */
>> +                     for (j = 0; j <
>> ARRAY_SIZE(max8998->buck2_vol); j++) {
>> +                             if (max8998->buck2_vol[j] == i) {
>> +                                     max8998->buck2_idx = j;
>> +
>> buck2_gpio_set(pdata->buck2_set3, j);
>> +                                     goto buck2_exit;
>> +                             }
>>                       }
>> +
>> +                     if (pdata->buck_voltage_lock)
>> +                             return -EINVAL;
>> +
>> +                     max8998_get_voltage_register(rdev,
>> +                                     &reg, &shift, &mask);
>> +                     ret = max8998_write_reg(i2c, reg, i);
>> +                     max8998->buck2_vol[max8998->buck2_idx] = i;
>> +
>> buck2_gpio_set(pdata->buck2_set3,max8998->buck2_idx); +buck2_exit:
>> dev_dbg(max8998->dev, "%s: SET3:%d\n", i2c->name,
> gpio_get_value(pdata->buck2_set3));
>
> Maybe only the matter of taste. The "for" loop for only two elements?

It was only for the look; to appear more symmetric with buck1.

>
>> + * @buck_voltage_lock: Do NOT change the values of the following six
>> + *   registers set by buck?_voltage?. The voltage of BUCK1/2 cannot
>> + *   be other than the preset values.
>> + * @buck1_voltage1: BUCK1 DVS mode 1 voltage register
>> + * @buck1_voltage2: BUCK1 DVS mode 2 voltage register
>> + * @buck1_voltage3: BUCK1 DVS mode 3 voltage register
>> + * @buck1_voltage4: BUCK1 DVS mode 4 voltage register
>> + * @buck2_voltage1: BUCK2 DVS mode 1 voltage register
>> + * @buck2_voltage2: BUCK2 DVS mode 2 voltage register
>
> Is it desirable to define all four for BUCK1 and two for BUCK2 DVS
> voltages in platform code?

In case a system with 4 buck1 preset voltages and 2 buck2 preset
voltages, it is desirable. :)

>
> Why the "general purpose" slots approach for user changeable/definable
> voltages (as it was done previously) have been replaced? Is the current
> approach faster?

As long as "buck_voltage_lock" is not set, user may use voltages not
defined as a preset (buckx_voltagex).

If buck_voltage_lock is true, any voltage not predefined is
rejected.However, if not, undefined voltages are accepted and from the
point where an undefined voltage is submitted, presets are not
guaranteed to be kept (any of preset values may be overwritten.)

>
>
> --
> Best regards,
>
> Lukasz Majewski
>
> Samsung Poland R&D Center
> Platform Group
>

Thanks.

-- 
MyungJoo Ham (함명주), Ph.D.
Mobile Software Platform Lab,
Digital Media and Communications (DMC) Business
Samsung Electronics
cell: 82-10-6714-2858