From: Kamil Konieczny <[email protected]>
Add enable regulators to dev_pm_opp_set_regulators() and disable
regulators to dev_pm_opp_put_regulators(). This prepares for
converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
Signed-off-by: Kamil Konieczny <[email protected]>
---
drivers/opp/core.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0e7703fe733f..947cac452854 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
if (ret)
goto free_regulators;
+ for (i = 0; i < opp_table->regulator_count; i++) {
+ ret = regulator_enable(opp_table->regulators[i]);
+ if (ret < 0)
+ goto disable;
+ }
+
return opp_table;
+disable:
+ while (i != 0)
+ regulator_disable(opp_table->regulators[--i]);
+
+ i = opp_table->regulator_count;
free_regulators:
while (i != 0)
regulator_put(opp_table->regulators[--i]);
@@ -1609,6 +1620,8 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
/* Make sure there are no concurrent readers while updating opp_table */
WARN_ON(!list_empty(&opp_table->opp_list));
+ for (i = opp_table->regulator_count - 1; i >= 0; i--)
+ regulator_disable(opp_table->regulators[i]);
for (i = opp_table->regulator_count - 1; i >= 0; i--)
regulator_put(opp_table->regulators[i]);
--
2.22.0
On 08-07-19, 16:11, [email protected] wrote:
> From: Kamil Konieczny <[email protected]>
>
> Add enable regulators to dev_pm_opp_set_regulators() and disable
> regulators to dev_pm_opp_put_regulators(). This prepares for
> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>
> Signed-off-by: Kamil Konieczny <[email protected]>
> ---
> drivers/opp/core.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..947cac452854 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> if (ret)
> goto free_regulators;
>
> + for (i = 0; i < opp_table->regulator_count; i++) {
> + ret = regulator_enable(opp_table->regulators[i]);
> + if (ret < 0)
> + goto disable;
> + }
I am wondering on why is this really required as this isn't done for
any other platform, probably because the regulators are enabled by
bootloader and are always on.
--
viresh
On 09.07.2019 07:40, Viresh Kumar wrote:
> On 08-07-19, 16:11, [email protected] wrote:
>> From: Kamil Konieczny <[email protected]>
>>
>> Add enable regulators to dev_pm_opp_set_regulators() and disable
>> regulators to dev_pm_opp_put_regulators(). This prepares for
>> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>>
>> Signed-off-by: Kamil Konieczny <[email protected]>
>> ---
>> drivers/opp/core.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 0e7703fe733f..947cac452854 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>> if (ret)
>> goto free_regulators;
>>
>> + for (i = 0; i < opp_table->regulator_count; i++) {
>> + ret = regulator_enable(opp_table->regulators[i]);
>> + if (ret < 0)
>> + goto disable;
>> + }
>
> I am wondering on why is this really required as this isn't done for
> any other platform, probably because the regulators are enabled by
> bootloader and are always on.
It was not enabled for historical reasons, from design point regualtors
should be enabled before use.
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
On 10.07.2019 12:16, Kamil Konieczny wrote:
>
>
> On 09.07.2019 07:40, Viresh Kumar wrote:
>> On 08-07-19, 16:11, [email protected] wrote:
>>> From: Kamil Konieczny <[email protected]>
>>>
>>> Add enable regulators to dev_pm_opp_set_regulators() and disable
>>> regulators to dev_pm_opp_put_regulators(). This prepares for
>>> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>>>
>>> Signed-off-by: Kamil Konieczny <[email protected]>
>>> ---
>>> drivers/opp/core.c | 13 +++++++++++++
>>> 1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>>> index 0e7703fe733f..947cac452854 100644
>>> --- a/drivers/opp/core.c
>>> +++ b/drivers/opp/core.c
>>> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>>> if (ret)
>>> goto free_regulators;
>>>
>>> + for (i = 0; i < opp_table->regulator_count; i++) {
>>> + ret = regulator_enable(opp_table->regulators[i]);
>>> + if (ret < 0)
>>> + goto disable;
>>> + }
>>
>> I am wondering on why is this really required as this isn't done for
>> any other platform, probably because the regulators are enabled by
>> bootloader and are always on.
>
> It was not enabled for historical reasons, from design point regualtors
> should be enabled before use.
On Exynos platform devfreq driver (exynos-bus) always enabled them,
so I wanted to preserve the current behaviour.
I've also checked the change with cpufreq-dt driver and it doesn't cause
issues.
Do you find this change acceptable?
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
On 09.07.2019 07:40, Viresh Kumar wrote:
> On 08-07-19, 16:11, [email protected] wrote:
>> From: Kamil Konieczny <[email protected]>
>>
>> Add enable regulators to dev_pm_opp_set_regulators() and disable
>> regulators to dev_pm_opp_put_regulators(). This prepares for
>> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>>
>> Signed-off-by: Kamil Konieczny <[email protected]>
>> ---
>> drivers/opp/core.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 0e7703fe733f..947cac452854 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>> if (ret)
>> goto free_regulators;
>>
>> + for (i = 0; i < opp_table->regulator_count; i++) {
>> + ret = regulator_enable(opp_table->regulators[i]);
>> + if (ret < 0)
>> + goto disable;
>> + }
>
> I am wondering on why is this really required as this isn't done for
> any other platform, probably because the regulators are enabled by
> bootloader and are always on.
It is not ABI break, it should work with existing DTBs
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
On 10.07.2019 12:43, Kamil Konieczny wrote:
> On 09.07.2019 07:40, Viresh Kumar wrote:
>> On 08-07-19, 16:11, [email protected] wrote:
>>> From: Kamil Konieczny <[email protected]>
>>>
>>> Add enable regulators to dev_pm_opp_set_regulators() and disable
>>> regulators to dev_pm_opp_put_regulators(). This prepares for
>>> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>>>
>>> Signed-off-by: Kamil Konieczny <[email protected]>
>>> ---
>>> drivers/opp/core.c | 13 +++++++++++++
>>> 1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>>> index 0e7703fe733f..947cac452854 100644
>>> --- a/drivers/opp/core.c
>>> +++ b/drivers/opp/core.c
>>> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>>> if (ret)
>>> goto free_regulators;
>>>
>>> + for (i = 0; i < opp_table->regulator_count; i++) {
>>> + ret = regulator_enable(opp_table->regulators[i]);
>>> + if (ret < 0)
>>> + goto disable;
>>> + }
>>
>> I am wondering on why is this really required as this isn't done for
>> any other platform, probably because the regulators are enabled by
>> bootloader and are always on.
>
> It is not ABI break, it should work with existing DTBs
Sorry, this answer should go to question by Krzysztof
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
On Mon, 8 Jul 2019 at 16:12, <[email protected]> wrote:
>
> From: Kamil Konieczny <[email protected]>
>
> Add enable regulators to dev_pm_opp_set_regulators() and disable
> regulators to dev_pm_opp_put_regulators(). This prepares for
> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>
> Signed-off-by: Kamil Konieczny <[email protected]>
> ---
> drivers/opp/core.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 08-07-19, 16:11, [email protected] wrote:
> From: Kamil Konieczny <[email protected]>
>
> Add enable regulators to dev_pm_opp_set_regulators() and disable
> regulators to dev_pm_opp_put_regulators(). This prepares for
> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>
> Signed-off-by: Kamil Konieczny <[email protected]>
> ---
> drivers/opp/core.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..947cac452854 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> if (ret)
> goto free_regulators;
>
> + for (i = 0; i < opp_table->regulator_count; i++) {
> + ret = regulator_enable(opp_table->regulators[i]);
> + if (ret < 0)
> + goto disable;
> + }
What about doing this in the same loop of regulator_get_optional() ?
> +
> return opp_table;
>
> +disable:
> + while (i != 0)
> + regulator_disable(opp_table->regulators[--i]);
> +
> + i = opp_table->regulator_count;
You also need to call _free_set_opp_data() here.
> free_regulators:
> while (i != 0)
> regulator_put(opp_table->regulators[--i]);
> @@ -1609,6 +1620,8 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
>
> /* Make sure there are no concurrent readers while updating opp_table */
> WARN_ON(!list_empty(&opp_table->opp_list));
Preserve the blank line here.
> + for (i = opp_table->regulator_count - 1; i >= 0; i--)
> + regulator_disable(opp_table->regulators[i]);
>
> for (i = opp_table->regulator_count - 1; i >= 0; i--)
> regulator_put(opp_table->regulators[i]);
Only single loop should be sufficient for this.
> --
> 2.22.0
--
viresh
On 11.07.2019 08:22, Viresh Kumar wrote:
> On 08-07-19, 16:11, [email protected] wrote:
>> From: Kamil Konieczny <[email protected]>
>>
>> Add enable regulators to dev_pm_opp_set_regulators() and disable
>> regulators to dev_pm_opp_put_regulators(). This prepares for
>> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>>
>> Signed-off-by: Kamil Konieczny <[email protected]>
>> ---
>> drivers/opp/core.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 0e7703fe733f..947cac452854 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1580,8 +1580,19 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>> if (ret)
>> goto free_regulators;
>>
>> + for (i = 0; i < opp_table->regulator_count; i++) {
>> + ret = regulator_enable(opp_table->regulators[i]);
>> + if (ret < 0)
>> + goto disable;
>> + }
>
> What about doing this in the same loop of regulator_get_optional() ?
yes, this is good, it will simplify code
>> +
>> return opp_table;
>>
>> +disable:
>> + while (i != 0)
>> + regulator_disable(opp_table->regulators[--i]);
>> +
>> + i = opp_table->regulator_count;
>
> You also need to call _free_set_opp_data() here.
good catch
if I move enable in loop above, then this will not be needed
>> free_regulators:
>> while (i != 0)
>> regulator_put(opp_table->regulators[--i]);
>> @@ -1609,6 +1620,8 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
>>
>> /* Make sure there are no concurrent readers while updating opp_table */
>> WARN_ON(!list_empty(&opp_table->opp_list));
>
> Preserve the blank line here.
>
>> + for (i = opp_table->regulator_count - 1; i >= 0; i--)
>> + regulator_disable(opp_table->regulators[i]);
>>
>> for (i = opp_table->regulator_count - 1; i >= 0; i--)
>> regulator_put(opp_table->regulators[i]);
>
> Only single loop should be sufficient for this.
you are right, I will do this in single loop
I will send v2
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland