Re: [PATCH 2/2] regulator: add mxs regulator driver

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]

On Mon, Sep 29, 2014 at 08:38:51AM +0200, Stefan Wahren wrote:

> I'm searching for a good regulator implementation example.

> Does it apply to ti-abb-regulator.c and twl-regulator.c?

Possibly.  But bear in mind that it's important to understand the
hardware you're trying to support.

> > This really needs a comment to explain what on earth is going on here -
> > the whole thing with writing the same thing twice with two delays is
> > more than a little odd.  It looks like the driver is trying to busy wait
> > in cases where the change happens quickly but the comments about "fast"
> > and "normal" mode make this unclear.

> The regulator driver polls for the DC_OK bit in the power status register.

> Quote for reference manual (p. 935): "High when switching DC-DC
> converter control loop has stabilized after a voltage target change."

> The two loops comes from the different regulator modes
> (REGULATOR_MODE_FAST, REGULATOR_MODE_NORMAL).

> In REGULATOR_MODE_FAST the voltage steping is disabled and changing
> voltage should be fast. In REGULATOR_MODE_NORMAL voltage steping is
> enabled and it's take a while for reaching the target voltage.

I don't think you've fully understood what the different modes mean
here, that's not normally how a buck convertor works.  The different
modes would typically control the ability of the regulator to respond
quickly to changes in load without drifting off regulation, fast mode
makes the regulator less efficient but more responsive to load changes
(probably marginally with modern regulators).  It should have relatively
little to do with the ability to ramp the voltage and certainly not on
the scale there.

> Do you see more a problem with the two different loops or the redundant
> register write?

Both.  The code right now just looks really obscure.

> if (readl(sreg->base_addr) & sreg->mode_mask)
>     return REGULATOR_MODE_FAST;

> return REGULATOR_MODE_NORMAL;

> Better?

Yes.

> > Why is this not looked up from the data structure like the rest of the
> > data?

> I was a little bit confused why there wasn't a mode_mask in the struct
> regulator_desc. I will do it like the ti-abb-regulator in the matching
> table.

There's no helpers for setting modes.

> >> +	regulator_unregister(rdev);
> >> +	iounmap(power_addr);
> >> +	iounmap(base_addr);

> > Use devm_ versions of functions.

> As a result the remove function for the drivers becomes unnecessary. Am
> i right?

Yes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]