linux-kernel - Re: [PATCH V3] regulator: DA9211 : new regulator driver

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] [day] [month] [year] [list]

Message-ID: <20140627150057.GO23300@sirena.org.uk>
Date:	Fri, 27 Jun 2014 16:00:57 +0100
From:	Mark Brown <broonie@...nel.org>
To:	James Ban <james.ban.opensource@...semi.com>
Cc:	Liam Girdwood <lgirdwood@...il.com>,
	Support Opensource <support.opensource@...semi.com>,
	LKML <linux-kernel@...r.kernel.org>,
	David Dajun Chen <david.chen@...semi.com>
Subject: Re: [PATCH V3] regulator: DA9211 : new regulator driver

On Mon, Jun 02, 2014 at 08:17:03AM +0100, James Ban wrote:

> This is the driver for the Dialog DA9211 Multi-phase 12A DC-DC Buck
> Converter regulator. It communicates via an I2C bus to the device.

This *still* seems to have all the same problems with inconsistent
handling of the A and B register sets 

> +	/* Make sure to exit from suspend mode on enable */
> +	if (regulator->reg_rselect == DA9211_RSEL_NO_GPIO) {
> +		ret = regmap_update_bits(chip->regmap, info->conf.reg,
> +					info->conf.sel_mask,
> +					DA9211_VBUCKA_SEL_A);
> +		if (ret < 0)
> +			return ret;
> +	}

Here it forces set A if and only if in no GPIO mode but...

> +static int da9211_regulator_set_suspend_voltage(struct regulator_dev *rdev,
> +						int uV)
> +{
> +	struct da9211_regulator *regulator = rdev_get_drvdata(rdev);
> +	struct da9211_regulator_info *info = regulator->info;
> +	struct da9211 *chip = regulator->da9211;
> +	int ret;
> +
> +	ret = regulator_map_voltage_linear(rdev, uV, uV);
> +	if (ret < 0)
> +		return ret;
> +
> +	return regmap_update_bits(chip->regmap, info->volt.reg_b,
> +				 info->volt.v_mask, ret);
> +}

...this unconditionally uses set B and...

> +static int da9211_suspend_enable(struct regulator_dev *rdev)
> +{
> +	struct da9211_regulator *regulator = rdev_get_drvdata(rdev);
> +	struct da9211_regulator_info *info = regulator->info;
> +	struct da9211 *chip = regulator->da9211;
> +
> +	/* Select register set B for voltage ramping. */
> +	if (regulator->reg_rselect == DA9211_RSEL_NO_GPIO)
> +		return regmap_update_bits(chip->regmap, info->conf.reg,
> +					info->conf.sel_mask,
> +					DA9211_VBUCKA_SEL_B);
> +	else
> +		return 0;
> +}

...this uses B if not using a GPIO and otherwise reports success without
actually doing anything (which seems buggy).  These behaviours aren't
compatible with each other, set_suspend_voltage() will break if set B is
selected by GPIO for example.

Please either go through and try to make these things all consistent or
(probably better) strip out the suspend and probably GPIO handling for
now, get the bulk of the driver merged, and then come back and try to
deal with those tricky bits.

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)