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

[Opensource [James Seong-Won Ban]]

> -----Original Message-----
> From: Mark Brown [mailto:broo...@kernel.org]
> Sent: Monday, June 02, 2014 4:02 AM
> To: Opensource [James Seong-Won Ban]
> Cc: Liam Girdwood; Support Opensource; LKML; Guennadi Liakhovetski;
> David Dajun Chen
> 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.opensou...@diasemi.com>
> 
> Please just use your name in signoffs, the "Opensource []" garbage that your
> mail system inserts isn't helping.
it will be fixed in next PATCH.
> 
> > +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.
In this device, the values of selection register for A/B voltage are changed 
automatically  if the status of GPIO is changed.
So it is possible to know the status of GPIO through the value of register.
> 
> > +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.
Your suggestion is correct. The code for the change of the active register 
in the function should be removed. I will send PATCH again.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/