[PATCH 1/2] regulators: Add definition of regulator_set_voltage_time() for !CONFIG_REGULATOR
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;
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.
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.
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
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.
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..
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.
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 ..
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.
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.
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.
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.
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.
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?
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.
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
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?
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.
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.
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"?
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..
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.
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..
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.
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..
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.
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 :(
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.
linaro-kernel@lists.linaro.org
-
Mark Brown -
Rafael J. Wysocki -
Viresh Kumar