Re: [PATCH 5/5] ARM: dts: Add initial regulator mode on exynos Peach boards

[Mark Brown <broonie@kernel.org>]

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

On Thu, Oct 09, 2014 at 04:27:37PM +0200, Javier Martinez Canillas wrote:

> I see, I thought that an operating mode could be anything that alter the
> regulator behavior either during runtime or when the system is suspended.
> But under your definition, it is true that most max77802 regulators have

It's not just me, it's the code and all the users and documentation...

> only two modes: ON and OFF (and some of them have a third Low Power mode).

...but let's be clear, only "on" (normal) and low power are modes here.
Like I keep saying please think things through - if modes also include
enable control why would they be treated separately in the API?

> I think though that a generic way to configure this enable control feature
> is needed. Maybe adding a new pair of .{get,set}_suspend_control function
> pointers to struct regulator_ops and an .initial_suspend_ctrl field to the
> struct regulation_constraints?

> That way the core could parse a generic DT property and call the function
> handlers but each driver can document in their own DT bindings what their
> control values are and how those affects the regulators during suspend?

That maps poorly onto a lot of devices which have control schemes which
are more complex than this, for example placing regulators into groups
which are then controlled en masse or with internal sequencing options.
There's also the general taste thing with an API that basically just
consists of passing a random value through - there's a lack of
generality there, it wouldn't be possible to write a generic user of the
API which is a bit of a warning sign.

If you just care about the specific "this pin controls enable in sleep
state" I'd suggest making an interface that very specifically does that.

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