Re: [PATCH V2] regulator: DA9211 : new regulator driver

[Mark Brown]

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@xxxxxxxxxxx>

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.

Attachment: signature.asc
Description: Digital signature