diff mbox series

[3/4] opp: Reused enabled flag and remove regulator_enabled

Message ID 359b588928b7e58b009f786b17ddc088c6a7d18b.1597292833.git.viresh.kumar@linaro.org
State New
Delegated to: viresh kumar
Headers show
Series [V2,1/4] opp: Enable resources again if they were disabled earlier | expand

Commit Message

Viresh Kumar Aug. 13, 2020, 4:29 a.m. UTC
The common "enabled" flag can be used here instead of
"regulator_enabled" now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/opp/core.c | 13 +++----------
 drivers/opp/opp.h  |  2 --
 2 files changed, 3 insertions(+), 12 deletions(-)

Comments

Stephen Boyd Aug. 15, 2020, 7:55 a.m. UTC | #1
Quoting Viresh Kumar (2020-08-12 21:29:00)
> The common "enabled" flag can be used here instead of
> "regulator_enabled" now.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Why not put this before the other patch? And mention that it will be
reused in another place soon? Then the previous patch won't look like
we're adding a variable to the struct when it is only used inside a
single function. Or at least squash it with the previous patch.

> ---
>  drivers/opp/core.c | 13 +++----------
>  drivers/opp/opp.h  |  2 --
>  2 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index e8882e7fd8a5..5f5da257f58a 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
>          * Enable the regulator after setting its voltages, otherwise it breaks
>          * some boot-enabled regulators.
>          */
> -       if (unlikely(!opp_table->regulator_enabled)) {
> +       if (unlikely(!opp_table->enabled)) {
>                 ret = regulator_enable(reg);
>                 if (ret < 0)
>                         dev_warn(dev, "Failed to enable regulator: %d", ret);
> -               else
> -                       opp_table->regulator_enabled = true;

A quick glance makes this look unsafe now because we're only checking
'enabled' and not actually setting it when this function is called. I
have to go back to the previous patch to understand where enabled is now
set to confirm that it is OK. If it was all one patch all the context
would be here.
Rajendra Nayak Aug. 18, 2020, 7:10 a.m. UTC | #2
On 8/13/2020 9:59 AM, Viresh Kumar wrote:
> The common "enabled" flag can be used here instead of
> "regulator_enabled" now.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/opp/core.c | 13 +++----------
>   drivers/opp/opp.h  |  2 --
>   2 files changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index e8882e7fd8a5..5f5da257f58a 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
>   	 * Enable the regulator after setting its voltages, otherwise it breaks
>   	 * some boot-enabled regulators.
>   	 */
> -	if (unlikely(!opp_table->regulator_enabled)) {
> +	if (unlikely(!opp_table->enabled)) {
>   		ret = regulator_enable(reg);
>   		if (ret < 0)
>   			dev_warn(dev, "Failed to enable regulator: %d", ret);
> -		else
> -			opp_table->regulator_enabled = true;
>   	}
>   
>   	return 0;
> @@ -905,10 +903,7 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
>   		if (ret)
>   			goto put_opp_table;
>   
> -		if (opp_table->regulator_enabled) {
> -			regulator_disable(opp_table->regulators[0]);
> -			opp_table->regulator_enabled = false;
> -		}
> +		regulator_disable(opp_table->regulators[0]);

unconditionally calling regulator_disable() here based on the common
'enabled' flag results in a crash on platforms without regulators
associated with the opp table.

>   
>   		ret = _set_required_opps(dev, opp_table, NULL);
>   		if (!ret)
> @@ -1795,11 +1790,9 @@ 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));
>   
> -	if (opp_table->regulator_enabled) {
> +	if (opp_table->enabled) {
>   		for (i = opp_table->regulator_count - 1; i >= 0; i--)
>   			regulator_disable(opp_table->regulators[i]);
> -
> -		opp_table->regulator_enabled = false;
>   	}
>   
>   	for (i = opp_table->regulator_count - 1; i >= 0; i--)
> diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
> index bd35802acc6e..0c3de3f6db5c 100644
> --- a/drivers/opp/opp.h
> +++ b/drivers/opp/opp.h
> @@ -147,7 +147,6 @@ enum opp_table_access {
>    * @clk: Device's clock handle
>    * @regulators: Supply regulators
>    * @regulator_count: Number of power supply regulators. Its value can be -1
> - * @regulator_enabled: Set to true if regulators were previously enabled.
>    * (uninitialized), 0 (no opp-microvolt property) or > 0 (has opp-microvolt
>    * property).
>    * @paths: Interconnect path handles
> @@ -196,7 +195,6 @@ struct opp_table {
>   	struct clk *clk;
>   	struct regulator **regulators;
>   	int regulator_count;
> -	bool regulator_enabled;
>   	struct icc_path **paths;
>   	unsigned int path_count;
>   	bool enabled;
>
Viresh Kumar Aug. 20, 2020, 7:19 a.m. UTC | #3
On 15-08-20, 00:55, Stephen Boyd wrote:
> Quoting Viresh Kumar (2020-08-12 21:29:00)
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index e8882e7fd8a5..5f5da257f58a 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -703,12 +703,10 @@ static int _generic_set_opp_regulator(struct opp_table *opp_table,
> >          * Enable the regulator after setting its voltages, otherwise it breaks
> >          * some boot-enabled regulators.
> >          */
> > -       if (unlikely(!opp_table->regulator_enabled)) {
> > +       if (unlikely(!opp_table->enabled)) {
> >                 ret = regulator_enable(reg);
> >                 if (ret < 0)
> >                         dev_warn(dev, "Failed to enable regulator: %d", ret);
> > -               else
> > -                       opp_table->regulator_enabled = true;
> 
> A quick glance makes this look unsafe now because we're only checking
> 'enabled' and not actually setting it when this function is called. I
> have to go back to the previous patch to understand where enabled is now
> set to confirm that it is OK. If it was all one patch all the context
> would be here.

The only case where things can go crazy are the cases where (for
example) clk_set_rate() fails, or something like that which would be a
bug and it shouldn't bother in the normal working of this code.
diff mbox series

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index e8882e7fd8a5..5f5da257f58a 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -703,12 +703,10 @@  static int _generic_set_opp_regulator(struct opp_table *opp_table,
 	 * Enable the regulator after setting its voltages, otherwise it breaks
 	 * some boot-enabled regulators.
 	 */
-	if (unlikely(!opp_table->regulator_enabled)) {
+	if (unlikely(!opp_table->enabled)) {
 		ret = regulator_enable(reg);
 		if (ret < 0)
 			dev_warn(dev, "Failed to enable regulator: %d", ret);
-		else
-			opp_table->regulator_enabled = true;
 	}
 
 	return 0;
@@ -905,10 +903,7 @@  int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
 		if (ret)
 			goto put_opp_table;
 
-		if (opp_table->regulator_enabled) {
-			regulator_disable(opp_table->regulators[0]);
-			opp_table->regulator_enabled = false;
-		}
+		regulator_disable(opp_table->regulators[0]);
 
 		ret = _set_required_opps(dev, opp_table, NULL);
 		if (!ret)
@@ -1795,11 +1790,9 @@  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));
 
-	if (opp_table->regulator_enabled) {
+	if (opp_table->enabled) {
 		for (i = opp_table->regulator_count - 1; i >= 0; i--)
 			regulator_disable(opp_table->regulators[i]);
-
-		opp_table->regulator_enabled = false;
 	}
 
 	for (i = opp_table->regulator_count - 1; i >= 0; i--)
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index bd35802acc6e..0c3de3f6db5c 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -147,7 +147,6 @@  enum opp_table_access {
  * @clk: Device's clock handle
  * @regulators: Supply regulators
  * @regulator_count: Number of power supply regulators. Its value can be -1
- * @regulator_enabled: Set to true if regulators were previously enabled.
  * (uninitialized), 0 (no opp-microvolt property) or > 0 (has opp-microvolt
  * property).
  * @paths: Interconnect path handles
@@ -196,7 +195,6 @@  struct opp_table {
 	struct clk *clk;
 	struct regulator **regulators;
 	int regulator_count;
-	bool regulator_enabled;
 	struct icc_path **paths;
 	unsigned int path_count;
 	bool enabled;