Re: [PATCH 3/3] regulator: qcom-rpm: Regulator driver for the Qualcomm RPM

[Mark Brown <broonie@kernel.org>]

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

On Thu, May 29, 2014 at 02:03:40PM -0700, Bjorn Andersson wrote:

Please fix your mailer to word wrap at less than 80 columns so quoted
text is legible.

> The hardware in this case is a "pmic" shared by all cpus in the system, so when
> we set the voltage, state or load of a regulator we merely case a vote. For
> voltage and state this is not an issue, but for load the value that's
> interesting for the "pmic" is the sum of the votes; i.e. the sum of the loads
> from all systems on a single regulator.

> What the code does here is to follow the hack found at codeaurora, that upon
> get_optimum_mode we guess that the client will call get_optimum_mode followed
> my set_mode. We keep the track of what load was last requested and use that in
> our vote.

No, this is awful and there's no way in hell that stuff like this should
be implemented in a driver since there's clearly nothing at all hardware
specific about it.  The load tracking needs to be implemented in the
framework if it's going to be implemented, and passing it up through the
chain is obviously going to need some conversion and accounting for
hardware conversion losses which doesn't seem to be happening here.

I'm still unclear on what the summed current is going to be used for,
though...

> >> +     if (vreg->parts->ip.mask) {
> >> +             initdata->constraints.valid_ops_mask |= REGULATOR_CHANGE_DRMS;
> >> +             initdata->constraints.valid_ops_mask |= REGULATOR_CHANGE_MODE;
> >> +             initdata->constraints.valid_modes_mask |= REGULATOR_MODE_NORMAL;
> >> +             initdata->constraints.valid_modes_mask |= REGULATOR_MODE_IDLE;

> > No, this is just plain broken.  Constraints are set by the *board*, you
> > don't know if these settings are safe on any given board.

> I can see that these are coming from board files, but I didn't find any example
> of how these are supposed to be set when using DT only.

> What's happening here is that given what compatible you use, different "parts"
> will be selected and based on that this code will or will not be executed;
> hence this is defined by what compatible you're using.

> But this is of course not obvious, so unless I've missed something I can clean
> this up slightly and make the connection to compatible more obvious. Okay?

No, it's still just plain broken.  You've no idea if the settings you're
providing work or not on a given system (or set of drivers for that
matter) - mode configuration really is a part of the system integration
not an unchanging part of the PMIC.  This code appears to assume that
every client driver (plus passives) is going to accurately supply load
information which just isn't realistic except in very controlled cases
for a specific system.

The reason it's not supported in DT at the minute is that the definition
of modes is not at all clear in a generic fashion, plus of course the
fact that we have no users in mainline for dynamic mode setting.  Most
PMICs these days are smart enough to do this autonomously anyway so it's
not clear that this is something that it's worth spending time on.

Please look for the prior threads on this.

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