Re: [Patch v3 2/7] Regulator: DA9055 Regulator driver

[Ashish Jangam <ashish.jangam@kpitcummins.com>]

On Sat, 2012-10-27 at 22:59 +0100, Mark Brown wrote:
> On Thu, Oct 11, 2012 at 03:39:24PM +0530, Ashish Jangam wrote:
> 
> > This is the Regulator patch for the DA9055 PMIC and has got dependency on
> > the DA9055 MFD core.
> 
> Always submit patches with subject lines appropriate for the subsystem,
> this helps get your patch noticed.  People do things like search their
> mailboxes for subsystem prefixes when looking for things they need to
> review.
In subject line apart from "regulator" I will introduce "next" too. 
> 
> > This patch support all of the DA9055 regulators. The output voltages are
> > fully programmable through I2C interface only. The platform data with regulation
> > constraints is passed down from the board to the regulator.
> > 
> > +	switch (mode) {
> > +	case REGULATOR_MODE_FAST:
> > +		val = DA9055_BUCK_MODE_SYNC << info->mode.shift;
> > +		break;
> > +	case REGULATOR_MODE_NORMAL:
> > +		val = DA9055_BUCK_MODE_AUTO << info->mode.shift;
> > +		break;
> > +	case REGULATOR_MODE_IDLE:
> > +	case REGULATOR_MODE_STANDBY:
> > +		val = DA9055_BUCK_MODE_SLEEP << info->mode.shift;
> > +		break;
> 
> _IDLE and _STANDBY should have different effects if they're both
> implemented; pick one.  From the rest of the code it looks like it
> should be _STANDBY.
Yes, _STANDBY should be picked only.  
> 
> > +	switch (mode) {
> > +	case REGULATOR_MODE_NORMAL:
> > +	case REGULATOR_MODE_FAST:
> > +		val = DA9055_LDO_MODE_SYNC;
> > +		break;
> > +	case REGULATOR_MODE_IDLE:
> > +	case REGULATOR_MODE_STANDBY:
> > +		val = DA9055_LDO_MODE_SLEEP;
> > +	}
> 
> Similarly here.  You're also missing a break;
Ok, will fix this.
> 
> > +	/* Get the voltage for the activer register set A/B */
> > +	if (ret == DA9055_REGUALTOR_SET_A)
> > +		ret = da9055_reg_read(regulator->da9055, volt.reg_a);
> > +	else
> > +		ret = da9055_reg_read(regulator->da9055, volt.reg_b);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	sel = ((ret & volt.v_mask) - volt.v_offset);
> 
> Why not just use the register values directly and refuse to write ones
> That are too low?  This would simplify things a little as you'd only
> need to check 
If I understood it correctly, v_offset should be used to check register
values as seen in the below sample code snippet
sel = ret & mask;
if (sel <= v_offset)
	return 0;
else
	return sel
> 
> > +	/* Set the voltage */
> > +	if (ret == DA9055_REGUALTOR_SET_A)
> > +		return da9055_regulator_set_voltage_bits(rdev, info->volt.reg_a,
> > +							 selector);
> > +
> > +	return da9055_regulator_set_voltage_bits(rdev, info->volt.reg_b,
> > +						 selector);
> 
> This is confusingly written - it should be either a switch or an if/else
> really.
if/else seems to be sensible here.
> 
> > +	/* Select register set B for suspend voltage ramping. */
> > +	ret = da9055_reg_update(regulator->da9055, info->conf.reg,
> > +				info->conf.sel_mask, DA9055_SEL_REG_B);
> > +	if (ret < 0)
> > +		return ret;
> 
> This doesn't seem like it plays nicely with the GPIO selection in normal
> set_voltage() - does it need to check to see if register set B might be
> used in normal operation and refuse to run if it could?
Thanks for catching this. Need to add condition to check if GPIO is
selected. 
> 
> > +	for (i = 0; i < ARRAY_SIZE(da9055_regulator_info); i++) {
> > +		info = &da9055_regulator_info[i];
> > +		if (info->reg_desc.id == id)
> > +			return info;
> > +		}
> > +
> 
> The indentation here is *very* messed up.  I'd suggest not omitting any
> braces.
Ok, will fix indentation and put braces around if condition.