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]
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)

Powered by blists - more mailing lists