Re: [PATCH 1/3] opp: core: add regulators enable and disable
On 11.07.2019 08:22, Viresh Kumar wrote:
> On 08-07-19, 16:11, k.koniec...@partner.samsung.com wrote:
>> From: Kamil Konieczny
>>
>> 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
>> ---
>> 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(_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 Institute Poland
Re: [PATCH 1/3] opp: core: add regulators enable and disable
On 08-07-19, 16:11, k.koniec...@partner.samsung.com wrote:
> From: Kamil Konieczny
>
> 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
> ---
> 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(_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
Re: [PATCH 1/3] opp: core: add regulators enable and disable
On Mon, 8 Jul 2019 at 16:12, wrote: > > From: Kamil Konieczny > > 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 > --- > drivers/opp/core.c | 13 + > 1 file changed, 13 insertions(+) Acked-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH 1/3] opp: core: add regulators enable and disable
On 10.07.2019 12:43, Kamil Konieczny wrote:
> On 09.07.2019 07:40, Viresh Kumar wrote:
>> On 08-07-19, 16:11, k.koniec...@partner.samsung.com wrote:
>>> From: Kamil Konieczny
>>>
>>> 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
>>> ---
>>> 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 Institute Poland
Re: [PATCH 1/3] opp: core: add regulators enable and disable
On 09.07.2019 07:40, Viresh Kumar wrote:
> On 08-07-19, 16:11, k.koniec...@partner.samsung.com wrote:
>> From: Kamil Konieczny
>>
>> 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
>> ---
>> 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 Institute Poland
Re: [PATCH 1/3] opp: core: add regulators enable and disable
On 10.07.2019 12:16, Kamil Konieczny wrote:
>
>
> On 09.07.2019 07:40, Viresh Kumar wrote:
>> On 08-07-19, 16:11, k.koniec...@partner.samsung.com wrote:
>>> From: Kamil Konieczny
>>>
>>> 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
>>> ---
>>> 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 Institute Poland
Re: [PATCH 1/3] opp: core: add regulators enable and disable
On 09.07.2019 07:40, Viresh Kumar wrote:
> On 08-07-19, 16:11, k.koniec...@partner.samsung.com wrote:
>> From: Kamil Konieczny
>>
>> 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
>> ---
>> 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 Institute Poland
Re: [PATCH 1/3] opp: core: add regulators enable and disable
On 08-07-19, 16:11, k.koniec...@partner.samsung.com wrote:
> From: Kamil Konieczny
>
> 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
> ---
> 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
[PATCH 1/3] opp: core: add regulators enable and disable
From: Kamil Konieczny
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
---
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(_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
