Messages in this thread | | | Date | Sun, 1 Jun 2014 20:01:41 +0100 | From | Mark Brown <> | 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@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. [unhandled content-type:application/pgp-signature] | |