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.opensou...@diasemi.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.
signature.asc
Description: Digital signature
