linux-kernel - Re: [PATCH V2] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140601190141.GM5099@sirena.org.uk>
Date:	Sun, 1 Jun 2014 20:01:41 +0100
From:	Mark Brown <broonie@...nel.org>
To:	"Opensource [James Seong-Won Ban]" <james.ban.opensource@...semi.com>
Cc:	Liam Girdwood <lgirdwood@...il.com>,
	Support Opensource <support.opensource@...semi.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Guennadi Liakhovetski <g.liakhovetski@....de>,
	David Dajun Chen <david.chen@...semi.com>
Subject: Re: [PATCH V2] regulator: DA9211 : new regulator driver

On Mon, May 19, 2014 at 02:45:51AM +0100, Opensource [James Seong-Won 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.

I'm still really not convinced that the whole handling of the A and B
settings is doing what it's supposed to be doing.  Like I said before I
know another one of your drivers is doing something similar (and not
sharing the code!) but still...

It looks like the driver is basically written so that register set A
must be used when Linux is running and set B must be used in suspend
mode but not everything seems joined up and consistent.

> Signed-off-by: Opensource [James Seong-Won Ban] <james.ban.opensource@...semi.com>

Please just use your name in signoffs, the "Opensource []" garbage that
your mail system inserts isn't helping.

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

> +	/*
> +	 * There are two voltage register set A & B for voltage ramping but
> +	 * either one of then can be active therefore we first determine
> +	 * the active register set.
> +	 */
> +	ret = regmap_read(chip->regmap, info->conf.reg, &data);
> +	if (ret < 0)
> +		return ret;

This appears to be picking the active register set...

> +	/*
> +	 * Regulator register set A/B is not selected through GPIO therefore
> +	 * we use default register set A for voltage ramping.
> +	 */
> +	if (regulator->reg_rselect == DA9211_RSEL_NO_GPIO) {
> +		/* Select register set A */
> +		ret = regmap_update_bits(chip->regmap, info->conf.reg,
> +					info->conf.sel_mask,
> +					DA9211_VBUCKA_SEL_A);
> +		if (ret < 0)
> +			return ret;
> +
> +		/* Set the voltage */
> +		return regmap_update_bits(chip->regmap, info->volt.reg_a,
> +					 info->volt.v_mask, selector);
> +	}

This forces to register set A.

> +	/*
> +	 * Here regulator register set A/B is selected through GPIO.
> +	 * Therefore we first determine the selected register set A/B and
> +	 * then set the desired voltage for that register set A/B.
> +	 */
> +	ret = regmap_read(chip->regmap, info->conf.reg, &data);
> +	if (ret < 0)
> +		return ret;

We don't appear to be controlling this GPIO - this then assumes that the
GPIO can only be used for selecting suspend mode but doesn't impose any
restrictions on this.

> +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;
> +
> +	/* Select register set B for suspend voltage ramping. */
> +	if (regulator->reg_rselect == DA9211_RSEL_NO_GPIO) {
> +		ret = regmap_update_bits(chip->regmap, info->conf.reg,
> +					info->conf.sel_mask,
> +					DA9211_VBUCKA_SEL_B);
> +		if (ret < 0)
> +			return ret;
> +	}

This appears to immediately change the active register set to set B
which as I said previously is broken; we may not be suspended yet and
clearly we haven't actually applied the desired setting yet either.

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