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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <20110104084904.5765cd9e@lmajewski.digital.local>
Date:	Tue, 04 Jan 2011 08:49:04 +0100
From:	Lukasz Majewski <l.majewski@...sung.com>
To:	MyungJoo Ham <myungjoo.ham@...sung.com>
Cc:	linux-kernel@...r.kernel.org, Samuel Ortiz <sameo@...ux.intel.com>,
	Liam Girdwood <lrg@...mlogic.co.uk>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	Alessandro Zummo <a.zummo@...ertech.it>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Joonyoung Shim <jy0922.shim@...sung.com>,
	myungjoo.ham@...il.com
Subject: Re: [PATCH v4 2/3] regulator MAX8998/LP3974: Support DVS-GPIO.

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

> + * @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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/