linux-kernel - Re: [PATCH 2/2] MAX8997/8966 PMIC Regulator Driver Initial Release

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: <20110121191604.GK22414@opensource.wolfsonmicro.com>
Date:	Fri, 21 Jan 2011 19:16:04 +0000
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	MyungJoo Ham <myungjoo.ham@...sung.com>
Cc:	linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
	Liam Girdwood <lrg@...mlogic.co.uk>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	kyungmin.park@...sung.com, myungjoo.ham@...il.com
Subject: Re: [PATCH 2/2] MAX8997/8966 PMIC Regulator Driver Initial Release

On Fri, Jan 14, 2011 at 03:53:38PM +0900, MyungJoo Ham wrote:
> This patch supports PMIC/Regulator part of MAX8997/MAX8966 MFD.
> In this initial release, selecting voltages or current-limit
> and switching on/off the regulators are supported.

Mostly good, a few stylistic things and this is out of date for the
current regulator API.

> +static int max8997_get_voltage(struct regulator_dev *rdev)
> +{

> +	ret = max8997_list_voltage(rdev, val);
> +	if (ret == -EINVAL) {
> +		int ldo = max8997_get_ldo(rdev);

It'd probably be easier to just define separate get_voltage() and
list_voltage() functions for these regulators.

> +static int max8997_set_voltage_ldo(struct regulator_dev *rdev,
> +               int min_uV, int max_uV)

This needs updating for the current API in -next (and 2.6.38).

> +	if ((ldo > MAX8997_BUCK7 || ldo < MAX8997_LDO1) &&
> +			ldo != MAX8997_CHARGER_CV &&
> +			ldo != MAX8997_CHARGER &&
> +			ldo != MAX8997_CHARGER_TOPOFF)
> +		return -EINVAL;
> +	if (ldo == MAX8997_BUCK6)
> +		return -EINVAL;

A switch statement for the valid regulators might be clearer.

> +static int max8997_set_voltage_buck(struct regulator_dev *rdev,
> +		int min_uV, int max_uV)

API update needed here too.

> +static int max8997_set_voltage_safeout(struct regulator_dev *rdev,
> +		int min_uV, int max_uV)
> +{

Here also.  Given that you've got a separate set_voltage() the comment
about a separate get_voltage() above seems even stronger.

> +static inline unsigned int max8997_buck_voltage(int uV)
> +{
> +	int step;
> +
> +	if (uV <= 650000)
> +		return 0x0;
> +	if (uV >= 2225000)
> +		return 0x3f;
> +
> +	step = uV - 650000;
> +	step /= 25000;
> +
> +	return step + 1;
> +}

Shouldn't this code be shared with set_voltage()?  Or...

> +		max8997_update_reg(i2c, MAX8997_REG_BUCK1DVS(i),
> +				max8997_buck_voltage(pdata->buck1_voltage[i]),
> +				0x3f);
> +		max8997_update_reg(i2c, MAX8997_REG_BUCK2DVS(i),
> +				max8997_buck_voltage(pdata->buck2_voltage[i]),
> +				0x3f);
> +		max8997_update_reg(i2c, MAX8997_REG_BUCK5DVS(i),
> +				max8997_buck_voltage(pdata->buck5_voltage[i]),
> +				0x3f);

...just call your set_voltage() operation directly for these?

> +		if (gpio_is_valid(pdata->buck125_gpios[0]) &&
> +				gpio_is_valid(pdata->buck125_gpios[1]) &&
> +				gpio_is_valid(pdata->buck125_gpios[2])) {

Strange indentation here.
--
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/