[PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

We already have dummy implementation for most of the regulators APIs for
!CONFIG_REGULATOR case and were missing it for regulator_set_voltage_time().

Found this issue while compiling cpufreq-cpu0 driver without regulators support
in kernel.

drivers/cpufreq/cpufreq-cpu0.c: In function ‘cpu0_cpufreq_probe’:
drivers/cpufreq/cpufreq-cpu0.c:186:3: error: implicit declaration of function ‘regulator_set_voltage_time’ [-Werror=implicit-function-declaration]

Fix this by adding dummy definition for regulator_set_voltage_time().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Liam/Broonie: Please see if this can go through Rafael as 2nd patch is dependent
on it.

 include/linux/regulator/consumer.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
index 1a4a8c1..0cfc286 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -397,6 +397,12 @@ static inline int regulator_set_voltage(struct regulator *regulator,
 	return 0;
 }
 
+static inline int regulator_set_voltage_time(struct regulator *regulator,
+					     int old_uV, int new_uV)
+{
+	return 0;
+}
+
 static inline int regulator_get_voltage(struct regulator *regulator)
 {
 	return -EINVAL;
-- 
2.0.0.rc2

[PATCH V2 2/2] cpufreq: cpufreq-cpu0: remove dependency on THERMAL and REGULATOR

[Viresh Kumar]

cpufreq-cpu0 uses thermal framework to register a cooling device, but doesn't
depend on it as there are dummy calls provided by thermal layer when
CONFIG_THERMAL=n. And when these calls fail, the driver is still usable.

Similar explanation is valid for regulators as well. We do have dummy calls
available for regulator APIs and the driver can work even when those calls
fail.

So, we don't really need to mention thermal and regulators as a dependency for
cpufreq-cpu0 in Kconfig. Remove it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V1-V2: Remove dependency on REGULATORs as well.

 drivers/cpufreq/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 1fbe11f..e473d65 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -185,7 +185,7 @@ config CPU_FREQ_GOV_CONSERVATIVE
 
 config GENERIC_CPUFREQ_CPU0
 	tristate "Generic CPU0 cpufreq driver"
-	depends on HAVE_CLK && REGULATOR && OF && THERMAL && CPU_THERMAL
+	depends on HAVE_CLK && OF
 	select PM_OPP
 	help
 	  This adds a generic cpufreq driver for CPU0 frequency management.
-- 
2.0.0.rc2

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Tue, May 27, 2014 at 05:37:29PM +0530, Viresh Kumar wrote:

> Liam/Broonie: Please see if this can go through Rafael as 2nd patch is dependent
> on it.

Is that likely to happen before the merge window?

> +static inline int regulator_set_voltage_time(struct regulator *regulator,
> +					     int old_uV, int new_uV)
> +{
> +	return 0;
> +}
> +

Hrm, I'd have expected this to return -EINVAL when stubbed.  I'd also
have expected regulator_set_voltage() to return -EINVAL mind you.  I
*suppose* that something that doesn't actually depend on regulator like
cpufreq might not care if the voltage really did change (I bet this was
added for cpufreq) but it's not awesome.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Rafael J. Wysocki]

On Tuesday, May 27, 2014 08:29:23 PM Mark Brown wrote:
> 
> --wJgPvdDu+gj56kJ8
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> 
> On Tue, May 27, 2014 at 05:37:29PM +0530, Viresh Kumar wrote:
> 
> > Liam/Broonie: Please see if this can go through Rafael as 2nd patch is dependent
> > on it.
> 
> Is that likely to happen before the merge window?

Depending.  If there's -rc8, then maybe.  Otherwise, nope.

Rafael

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Wed, May 28, 2014 at 01:12:57AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, May 27, 2014 08:29:23 PM Mark Brown wrote:
> > On Tue, May 27, 2014 at 05:37:29PM +0530, Viresh Kumar wrote:

> > > Liam/Broonie: Please see if this can go through Rafael as 2nd patch is dependent
> > > on it.

> > Is that likely to happen before the merge window?

> Depending.  If there's -rc8, then maybe.  Otherwise, nope.

The reason I was asking was that I'm happy to just go ahead and apply
the patch right now if you're likely to not apply the second patch until
after the merge window.  Let's just do that for now, if you decide you
will apply then please add my ack and let me know so I can drop the
duplicate.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 28 May 2014 00:59, Mark Brown <broonie@kernel.org> wrote:
> Hrm, I'd have expected this to return -EINVAL when stubbed.  I'd also
> have expected regulator_set_voltage() to return -EINVAL mind you.  I

Or ENOSYS ? I will fix both routines once you confirm..

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Wed, May 28, 2014 at 09:59:37PM +0530, Viresh Kumar wrote:
> On 28 May 2014 00:59, Mark Brown <broonie@kernel.org> wrote:

> > Hrm, I'd have expected this to return -EINVAL when stubbed.  I'd also
> > have expected regulator_set_voltage() to return -EINVAL mind you.  I

> Or ENOSYS ? I will fix both routines once you confirm..

Whatever - I don't think the particular code makes any practical
difference.  We would need to audit existing users who don't have a
REGULATOR dependency for breakage though.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 28 May 2014 23:08, Mark Brown <broonie@kernel.org> wrote:
> Whatever - I don't think the particular code makes any practical
> difference.  We would need to audit existing users who don't have a
> REGULATOR dependency for breakage though.

I tried auditing all 29 files which had this symbol: regulator_set_voltage
and couldn't find anything which might break with the proposed change.

Either these are making sure that we have a valid regulator or they have
code inside #ifdef CONFIG_REGULATOR ..

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Mon, Jun 02, 2014 at 12:50:59PM +0530, Viresh Kumar wrote:
> On 28 May 2014 23:08, Mark Brown <broonie@kernel.org> wrote:
> > Whatever - I don't think the particular code makes any practical
> > difference.  We would need to audit existing users who don't have a
> > REGULATOR dependency for breakage though.

> I tried auditing all 29 files which had this symbol: regulator_set_voltage
> and couldn't find anything which might break with the proposed change.

> Either these are making sure that we have a valid regulator or they have
> code inside #ifdef CONFIG_REGULATOR ..

When you say they check for a valid regulator how are they doing that?
The stub will come into play if there isn't a dependency on REGULATOR.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 2 June 2014 15:21, Mark Brown <broonie@kernel.org> wrote:
> When you say they check for a valid regulator how are they doing that?
> The stub will come into play if there isn't a dependency on REGULATOR.

I meant they fail and quit if regulator_get() failed and so the first parameter
to regulator_set_voltage() is guaranteed to be valid.

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Mon, Jun 02, 2014 at 03:24:05PM +0530, Viresh Kumar wrote:
> On 2 June 2014 15:21, Mark Brown <broonie@kernel.org> wrote:

> > When you say they check for a valid regulator how are they doing that?
> > The stub will come into play if there isn't a dependency on REGULATOR.

> I meant they fail and quit if regulator_get() failed and so the first parameter
> to regulator_set_voltage() is guaranteed to be valid.

No, think about what you're changing here.  You're changing the stub -
the stub has a regulator_get() which always succeeeds.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 2 June 2014 15:32, Mark Brown <broonie@kernel.org> wrote:
> No, think about what you're changing here.  You're changing the stub -
> the stub has a regulator_get() which always succeeeds.

Right, things might start to break with the change to
regulator_set_voltage()..

When I compare this to clk-APIs, the dummy implementations always
pass and so we are allowed to send NULL clk to any routine (the way
we can do it here, probably to simply code)..

Now, why do we want to return -EINVAL from set_voltage here ? Similar
routines in clk-API are returning 0 and even clk_get_rate() returns zero,
unlike in regulators, as we return -EINVAL..

Not sure which of these frameworks is doing the right thing.

What do you suggest.

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Mon, Jun 02, 2014 at 03:45:37PM +0530, Viresh Kumar wrote:
> On 2 June 2014 15:32, Mark Brown <broonie@kernel.org> wrote:
> > No, think about what you're changing here.  You're changing the stub -
> > the stub has a regulator_get() which always succeeeds.

> Now, why do we want to return -EINVAL from set_voltage here ? Similar
> routines in clk-API are returning 0 and even clk_get_rate() returns zero,
> unlike in regulators, as we return -EINVAL..

If the consumer tried to set a voltage presumably it cares if that
voltage was set - for example if your cpufreq driver tries to increase
the voltage of a core supply so that it can then raise the frequency the
user is going to be upset if the voltage was not actually raised and it
goes off and raises the clock rate causing the system to become unstable.

> Not sure which of these frameworks is doing the right thing.

I don't think the clock API should have clk_set_rate() report success if
it was ignored.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 2 June 2014 17:53, Mark Brown <broonie@kernel.org> wrote:
> If the consumer tried to set a voltage presumably it cares if that
> voltage was set - for example if your cpufreq driver tries to increase
> the voltage of a core supply so that it can then raise the frequency the
> user is going to be upset if the voltage was not actually raised and it
> goes off and raises the clock rate causing the system to become unstable.

If the driver continued despite getting regulator as NULL, it means that
regulator isn't a MUST for it. For example a CPUFreq driver may work
with or without a regulator.

Now if the dummy calls return *error* for some cases then these driver
will have to do
if(xyz)
    API-call()..

And so dummy APIs like clk_set_rate(), clk_get_rate(),
regulator_set_voltage() must return zero..

To get rid of this in drivers these dummy routines *must* behave as
they passed, if the drivers really care about them then they must
quit as soon as regulator_get() returned NULL.

This is why we have such implementations in clk framework which are
very well thought earlier.

Does this make sense?

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Mon, Jun 02, 2014 at 06:44:35PM +0530, Viresh Kumar wrote:
> On 2 June 2014 17:53, Mark Brown <broonie@kernel.org> wrote:

> > If the consumer tried to set a voltage presumably it cares if that
> > voltage was set - for example if your cpufreq driver tries to increase
> > the voltage of a core supply so that it can then raise the frequency the
> > user is going to be upset if the voltage was not actually raised and it
> > goes off and raises the clock rate causing the system to become unstable.

> If the driver continued despite getting regulator as NULL, it means that
> regulator isn't a MUST for it. For example a CPUFreq driver may work
> with or without a regulator.

No, NULL is a perfectly valid regulator - the *only* thing that a caller
should check for is IS_ERR().  You are missing the point of the stubs,
and indeed the fact that real physical supplies can have exactly the
same limitations as the dummy supplies do and therefore the presence of
a physical regulator should not in itself indcate anything about what
you can do with it.

> Now if the dummy calls return *error* for some cases then these driver
> will have to do
> if(xyz)
>     API-call()..

Consumers should be implementing error checking code regardless.  If we
don't need to implement any error checking there's rather a lot of the
kernel we can go and delete...

> And so dummy APIs like clk_set_rate(), clk_get_rate(),
> regulator_set_voltage() must return zero..

Please re-read and think about my CPUfreq example.  How do you expect
that to work sanely if we don't care if any of the operations worked?

> To get rid of this in drivers these dummy routines *must* behave as
> they passed, if the drivers really care about them then they must
> quit as soon as regulator_get() returned NULL.

> This is why we have such implementations in clk framework which are
> very well thought earlier.

> Does this make sense?

No, not at all and I don't think it applies to the clock API either -
it's got similar issues with real physical clocks not always supporting
all operations.  Consider for a moment what happens if we try to set and
then use a clock rate ona fixed clock.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 2 June 2014 20:50, Mark Brown <broonie@kernel.org> wrote:
> No, not at all and I don't think it applies to the clock API either -
> it's got similar issues with real physical clocks not always supporting
> all operations.  Consider for a moment what happens if we try to set and
> then use a clock rate ona fixed clock.

Okay, so the patch 1/3 isn't enough as we need to fix other drivers as
well which are expected to work with CONFIG_REGULATOR=n. That
would be some work and will try to send a patchset for that..

As Rafael has asked you to apply the patches, can you please apply
patch 2-3 for now?

--
viresh

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Mon, Jun 02, 2014 at 10:25:18PM +0530, Viresh Kumar wrote:

> As Rafael has asked you to apply the patches, can you please apply
> patch 2-3 for now?

Not right now, the merge window is open.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 28 May 2014 23:08, Mark Brown <broonie@kernel.org> wrote:
> Whatever - I don't think the particular code makes any practical
> difference.  We would need to audit existing users who don't have a
> REGULATOR dependency for breakage though.

Exactly what kind of drivers are we looking to fix here? These might
be the possible cases:
- We are checking 'regulator pointer' before calling and don't need to
handle anything there..
- drivers depend on CONFIG_REGULATOR and so again we don't need
to handle anything
- None of above are true and drivers aren't checking return value of
regulator_set_voltage()
OR
They are checking it and failing when it failed..

What do we want to do in these cases?

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Tue, Jun 03, 2014 at 08:10:00PM +0530, Viresh Kumar wrote:
> On 28 May 2014 23:08, Mark Brown <broonie@kernel.org> wrote:
> > Whatever - I don't think the particular code makes any practical
> > difference.  We would need to audit existing users who don't have a
> > REGULATOR dependency for breakage though.

> Exactly what kind of drivers are we looking to fix here? These might
> be the possible cases:
> - We are checking 'regulator pointer' before calling and don't need to
> handle anything there..

I'm not sure what you mean by this.

> - None of above are true and drivers aren't checking return value of
> regulator_set_voltage()
> OR
> They are checking it and failing when it failed..

> What do we want to do in these cases?

Well, we would need to look at what the drivers were doing and figure
out something sensible - it really depends why they're trying to set the
regulator and what would happen if it doesn't work.  For drivers that
ignore the return value they won't be affected anyway.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 3 June 2014 20:23, Mark Brown <broonie@kernel.org> wrote:
>> - We are checking 'regulator pointer' before calling and don't need to
>> handle anything there..
>
> I'm not sure what you mean by this.

I meant that sometimes regulator_set_voltage() is only called when pointer
to regulator is valid.

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Tue, Jun 03, 2014 at 08:52:04PM +0530, Viresh Kumar wrote:
> On 3 June 2014 20:23, Mark Brown <broonie@kernel.org> wrote:

> >> - We are checking 'regulator pointer' before calling and don't need to
> >> handle anything there..

> > I'm not sure what you mean by this.

> I meant that sometimes regulator_set_voltage() is only called when pointer
> to regulator is valid.

Could you please be more explicit about what you mean by "valid"?

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 3 June 2014 21:02, Mark Brown <broonie@kernel.org> wrote:
> Could you please be more explicit about what you mean by "valid"?

I meant Not NULL..

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Tue, Jun 03, 2014 at 09:05:56PM +0530, Viresh Kumar wrote:
> On 3 June 2014 21:02, Mark Brown <broonie@kernel.org> wrote:

> > Could you please be more explicit about what you mean by "valid"?

> I meant Not NULL..

To repeat yet again: NULL is a perfectly valid regulator, anything
checking for NULL is broken.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 3 June 2014 20:23, Mark Brown <broonie@kernel.org> wrote:
> Well, we would need to look at what the drivers were doing and figure
> out something sensible - it really depends why they're trying to set the
> regulator and what would happen if it doesn't work.

For example, few cpufreq drivers are calling it during frequency
transition and are checking return value as well.. And fail if it failed.

One way out might be checking if pointer to regulator is valid or not
and only call it if pointer is not NULL..

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Tue, Jun 03, 2014 at 08:55:25PM +0530, Viresh Kumar wrote:
> On 3 June 2014 20:23, Mark Brown <broonie@kernel.org> wrote:

> > Well, we would need to look at what the drivers were doing and figure
> > out something sensible - it really depends why they're trying to set the
> > regulator and what would happen if it doesn't work.

> For example, few cpufreq drivers are calling it during frequency
> transition and are checking return value as well.. And fail if it failed.

> One way out might be checking if pointer to regulator is valid or not
> and only call it if pointer is not NULL..

No, as I've explained repeatedly NULL is a perfectly valid regulator and
that's not going to work reliably.  As I've previously requested please
think about what happens to cpufreq if we fail to ramp voltages.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 3 June 2014 21:18, Mark Brown <broonie@kernel.org> wrote:
> No, as I've explained repeatedly NULL is a perfectly valid regulator and

Okay, its been checked at multiple places already and that's obviously
wrong then.

> that's not going to work reliably.  As I've previously requested please
> think about what happens to cpufreq if we fail to ramp voltages.

Okay, so here is the scenario:

- driver is generic (like cpufreq-cpu0) and some user platforms may have
regulator support and others might not..

- For platforms with regulators support, we _must_ check if the voltage
change is successful or not and fail if regulator_set_voltage() failed.

- But for platforms without regulators support (CONFIG_REGULATOR=n),
regulator_get() will return NULL (a valid regulator though) and
regulator_set_voltage() will fail. Because the platform doesn't care much
about regulators it must go on and change frequency as if nothing
happened.

How can we achieve both these requirements by a generic piece of
code?

The only way I could think of currently is by returning something
special like -ENOSYS from regulator_set_voltage() when
regulators aren't configured in kernel and check return value of
regulator_set_voltage() against this..

This also holds true for regulator_get_voltage() which is returning
-EINVAL currently..

Please share if you have some other solution in mind..

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Wed, Jun 04, 2014 at 11:16:37AM +0530, Viresh Kumar wrote:

> - But for platforms without regulators support (CONFIG_REGULATOR=n),
> regulator_get() will return NULL (a valid regulator though) and
> regulator_set_voltage() will fail. Because the platform doesn't care much
> about regulators it must go on and change frequency as if nothing
> happened.

No, approximately none of this is true.  CONFIG_REGULATOR tells you
nothing about the hardware, it tells you what someone selected in the
kernel config.  There is nothing stopping anyone enabling the API on any
platform, and there is nothing stopping anyone disabling the API when
building a kernel for a platform which can do something with regulators
with CONFIG_REGULATOR disabled.

Please, go and *think* about what's going on here.  I've repeatedly
asked you to consider the case where we need to raise the voltage prior
to raising the frequency for cpufreq but you've not responded to these
requests either directly or in showing any sign of having understood the
issue.

If the code fails to change the voltage it needs to handle that
(including remembering that attempts to lower the voltage fail); if the
code handles the errors sensibly I would expect that to handle
everything.

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

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Viresh Kumar]

On 4 June 2014 16:08, Mark Brown <broonie@kernel.org> wrote:
> Please, go and *think* about what's going on here.  I've repeatedly
> asked you to consider the case where we need to raise the voltage prior
> to raising the frequency for cpufreq but you've not responded to these
> requests either directly or in showing any sign of having understood the
> issue.

I replied to that only when I said:

- For platforms with regulators support, we _must_ check if the voltage
change is successful or not and fail if regulator_set_voltage() failed.

And what you wrote earlier was correct, we may end up with unstable
systems if we ramp frequencies even when changing voltage failed.

> If the code fails to change the voltage it needs to handle that
> (including remembering that attempts to lower the voltage fail); if the
> code handles the errors sensibly I would expect that to handle
> everything.

That's what I was asking, how should the code handle it? Code also
has to take care of platforms which haven't configured regulators
in kernel and want to use this generic driver.

Sorry if I still didn't answer it properly :(

Re: [PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR

[Mark Brown]

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

On Wed, Jun 04, 2014 at 04:27:48PM +0530, Viresh Kumar wrote:
> On 4 June 2014 16:08, Mark Brown <broonie@kernel.org> wrote:
> > Please, go and *think* about what's going on here.  I've repeatedly

> > If the code fails to change the voltage it needs to handle that
> > (including remembering that attempts to lower the voltage fail); if the
> > code handles the errors sensibly I would expect that to handle
> > everything.

> That's what I was asking, how should the code handle it? Code also
> has to take care of platforms which haven't configured regulators
> in kernel and want to use this generic driver.

If the code gets an error back when it tries to change the voltage then
it should assume that the attempt to change the voltage did not succeed.
Should an attempt to change the voltage not succeed it seems reasonable
to assume that the voltage did not change and is the same as before.
The obvious thing would therefore appear to be for the code to proceed
with that assumption and act accordingly.

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