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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
