2019-07-15 12:05:21

by Kamil Konieczny

Subject: [PATCH v2 1/4] opp: core: add regulators enable and disable

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]>
--
Changes in v2:

- move regulator enable and disable into loop

---
drivers/opp/core.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0e7703fe733f..069c5cf8827e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
goto free_regulators;
}

+ ret = regulator_enable(reg);
+ if (ret < 0)
+ goto disable;
+
opp_table->regulators[i] = reg;
}

@@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,

return opp_table;

+disable:
+ regulator_put(reg);
+ --i;
+
free_regulators:
- while (i != 0)
- regulator_put(opp_table->regulators[--i]);
+ for (; i >= 0; --i) {
+ regulator_disable(opp_table->regulators[i]);
+ regulator_put(opp_table->regulators[i]);
+ }

kfree(opp_table->regulators);
opp_table->regulators = NULL;
@@ -1610,8 +1620,10 @@ 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--)
+ for (i = opp_table->regulator_count - 1; i >= 0; i--) {
+ regulator_disable(opp_table->regulators[i]);
regulator_put(opp_table->regulators[i]);
+ }

_free_set_opp_data(opp_table);

--
2.22.0

2019-07-16 04:00:44

by Chanwoo Choi

Subject: Re: [PATCH v2 1/4] opp: core: add regulators enable and disable

Hi Kamil,

On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> 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().

IMHO, it is not proper to mention the specific driver name.
If you explain the reason why enable the regulator before using it,
it is enough description.

>
> Signed-off-by: Kamil Konieczny <[email protected]>
> --
> Changes in v2:
>
> - move regulator enable and disable into loop
>
> ---
> drivers/opp/core.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..069c5cf8827e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> goto free_regulators;
> }
>
> + ret = regulator_enable(reg);
> + if (ret < 0)
> + goto disable;
> +
> opp_table->regulators[i] = reg;
> }
>
> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>
> return opp_table;
>
> +disable:
> + regulator_put(reg);
> + --i;
> +
> free_regulators:
> - while (i != 0)
> - regulator_put(opp_table->regulators[--i]);
> + for (; i >= 0; --i) {
> + regulator_disable(opp_table->regulators[i]);
> + regulator_put(opp_table->regulators[i]);
> + }
>
> kfree(opp_table->regulators);
> opp_table->regulators = NULL;
> @@ -1610,8 +1620,10 @@ 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--)
> + for (i = opp_table->regulator_count - 1; i >= 0; i--) {
> + regulator_disable(opp_table->regulators[i]);
> regulator_put(opp_table->regulators[i]);
> + }
>
> _free_set_opp_data(opp_table);
>
>

I agree to enable the regulator before using it.
The bootloader might not enable the regulators
and the kernel need to enable regulator in order to increase
the reference count explicitly event if bootloader enables it.

Reviewed-by: Chanwoo Choi <[email protected]>

--
Best Regards,
Chanwoo Choi
Samsung Electronics

2019-07-16 10:07:40

by Viresh Kumar

Subject: Re: [PATCH v2 1/4] opp: core: add regulators enable and disable

On 15-07-19, 14:04, Kamil Konieczny wrote:
> 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]>
> --
> Changes in v2:
>
> - move regulator enable and disable into loop
>
> ---
> drivers/opp/core.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..069c5cf8827e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> goto free_regulators;
> }
>
> + ret = regulator_enable(reg);
> + if (ret < 0)
> + goto disable;

The name of this label is logically incorrect because we won't disable
the regulator from there but put it. Over that, I would rather prefer
to remove the label and add regulator_put() here itself.

> +
> opp_table->regulators[i] = reg;
> }
>
> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>
> return opp_table;
>
> +disable:
> + regulator_put(reg);
> + --i;
> +
> free_regulators:
> - while (i != 0)
> - regulator_put(opp_table->regulators[--i]);
> + for (; i >= 0; --i) {
> + regulator_disable(opp_table->regulators[i]);
> + regulator_put(opp_table->regulators[i]);

This is incorrect as this will now try to put/disable the regulator
which we failed to acquire. As --i happens only after the loop has run
once. You can rather do:

while (i--) {
regulator_disable(opp_table->regulators[i]);
regulator_put(opp_table->regulators[i]);
}


> + }
>
> kfree(opp_table->regulators);
> opp_table->regulators = NULL;
> @@ -1610,8 +1620,10 @@ 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--)
> + for (i = opp_table->regulator_count - 1; i >= 0; i--) {
> + regulator_disable(opp_table->regulators[i]);
> regulator_put(opp_table->regulators[i]);
> + }
>
> _free_set_opp_data(opp_table);
>
> --
> 2.22.0

--
viresh

2019-07-17 14:14:12

by Kamil Konieczny

Subject: Re: [PATCH v2 1/4] opp: core: add regulators enable and disable

On 16.07.2019 06:03, Chanwoo Choi wrote:
> Hi Kamil,
>
> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>> 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().
>
> IMHO, it is not proper to mention the specific driver name.
> If you explain the reason why enable the regulator before using it,
> it is enough description.
>
>>
>> Signed-off-by: Kamil Konieczny <[email protected]>
>> --
>> Changes in v2:
>>
>> - move regulator enable and disable into loop
>>
>> ---
>> drivers/opp/core.c | 18 +++++++++++++++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 0e7703fe733f..069c5cf8827e 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>> goto free_regulators;
>> }
>>
>> + ret = regulator_enable(reg);
>> + if (ret < 0)
>> + goto disable;
>> +
>> opp_table->regulators[i] = reg;
>> }
>>
>> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>>
>> return opp_table;
>>
>> +disable:
>> + regulator_put(reg);
>> + --i;
>> +
>> free_regulators:
>> - while (i != 0)
>> - regulator_put(opp_table->regulators[--i]);
>> + for (; i >= 0; --i) {
>> + regulator_disable(opp_table->regulators[i]);
>> + regulator_put(opp_table->regulators[i]);
>> + }
>>
>> kfree(opp_table->regulators);
>> opp_table->regulators = NULL;
>> @@ -1610,8 +1620,10 @@ 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--)
>> + for (i = opp_table->regulator_count - 1; i >= 0; i--) {
>> + regulator_disable(opp_table->regulators[i]);
>> regulator_put(opp_table->regulators[i]);
>> + }
>>
>> _free_set_opp_data(opp_table);
>>
>>
>
> I agree to enable the regulator before using it.
> The bootloader might not enable the regulators
> and the kernel need to enable regulator in order to increase
> the reference count explicitly event if bootloader enables it.
>
> Reviewed-by: Chanwoo Choi <[email protected]>

Thank you, I will change commit description and send v3.

--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland

2019-07-17 14:15:46

by Kamil Konieczny

Subject: Re: [PATCH v2 1/4] opp: core: add regulators enable and disable

On 16.07.2019 12:05, Viresh Kumar wrote:
> On 15-07-19, 14:04, Kamil Konieczny wrote:
>> 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]>
>> --
>> Changes in v2:
>>
>> - move regulator enable and disable into loop
>>
>> ---
>> drivers/opp/core.c | 18 +++++++++++++++---
>> 1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 0e7703fe733f..069c5cf8827e 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>> goto free_regulators;
>> }
>>
>> + ret = regulator_enable(reg);
>> + if (ret < 0)
>> + goto disable;
>
> The name of this label is logically incorrect because we won't disable
> the regulator from there but put it. Over that, I would rather prefer
> to remove the label and add regulator_put() here itself.

I will change this and following according to your suggestions and will send v3.

>> +
>> opp_table->regulators[i] = reg;
>> }
>>
>> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>>
>> return opp_table;
>>
>> +disable:
>> + regulator_put(reg);
>> + --i;
>> +
>> free_regulators:
>> - while (i != 0)
>> - regulator_put(opp_table->regulators[--i]);
>> + for (; i >= 0; --i) {
>> + regulator_disable(opp_table->regulators[i]);
>> + regulator_put(opp_table->regulators[i]);
>
> This is incorrect as this will now try to put/disable the regulator
> which we failed to acquire. As --i happens only after the loop has run
> once. You can rather do:
>
> while (i--) {
> regulator_disable(opp_table->regulators[i]);
> regulator_put(opp_table->regulators[i]);
> }
>
>
>> + }
>>
>> kfree(opp_table->regulators);
>> opp_table->regulators = NULL;
>> @@ -1610,8 +1620,10 @@ 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--)
>> + for (i = opp_table->regulator_count - 1; i >= 0; i--) {
>> + regulator_disable(opp_table->regulators[i]);
>> regulator_put(opp_table->regulators[i]);
>> + }
>>
>> _free_set_opp_data(opp_table);
>>
>> --
>> 2.22.0
>

--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland