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

[Lukasz Majewski]

On Tue, 04 Jan 2011 14:17:40 +0900
MyungJoo Ham <myungjoo.ham@xxxxxxxxxxx> 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?

> + * @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? 

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


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center
Platform Group
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/