Re: [PATCH v3 RESEND] regulator: Add S5M8767A regulator driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Fri, Dec 30, 2011 at 02:50:59PM +0900, Sangbeom Kim wrote:
> On Thu, Dec 29, 2011 at 07:58 PM, Mark Brown wrote:

> > > +	ret = s5m8767_get_register(rdev, &reg);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return s5m_reg_update(s5m8767->iodev, reg, ~pattern, mask);

> > The above looks like it's going to update the same registers as the
> > regular enable and disable which isn't right, this function is for
> > updating suspend mode configuration.

> What's exact meaning of updating suspend mode configuration?
> As I know, The purpose of set_suspend_enable is that
> enabling voltage output during suspend mode.
> So, I implement same register enable and disable.

If there's only the one set of registers you should just not implement
this function.  Some regulators have separate registers for configuring
the state they go into when suspended so they need a separate call to
configure that mode.

> > > +	do {
> > > +		ret = s5m_reg_read(s5m8767->iodev, reg, &val);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		s5m_reg_update(s5m8767->iodev, reg,
> > > +				((val & 0x3f) | 0x40), 0xff);
> > > +		reg++;
> > > +		if ((reg == S5M8767_REG_LDO9CTRL) ||
> > > +			(reg == S5M8767_REG_LDO11CTRL) ||
> > > +			(reg == S5M8767_REG_LDO13CTRL) ||
> > > +			(reg == S5M8767_REG_LDO17CTRL))
> > > +			reg++;
> > > +	} while (reg <= S5M8767_REG_LDO16CTRL);

> > What does this do?

> This is SMDK4412 specific code.
> I will delete it for the general purpose.

OK.  Is it something that's missing from the framework or was it just
done this way for a quick test?