[V2] opp: Don't ignore clk_get() errors other than -ENOENT

Message ID 61854037cd4d3ac367cfda3f02fd1557b1b3bb8b.1612153322.git.viresh.kumar@linaro.org
State New
Headers show
Series
  • [V2] opp: Don't ignore clk_get() errors other than -ENOENT
Related show

Commit Message

Viresh Kumar Feb. 1, 2021, 4:22 a.m.
Not all devices that need to use OPP core need to have clocks, a missing
clock is fine in which case -ENOENT shall be returned by clk_get().

Anything else is an error and must be handled properly.

Reported-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
V2:
- s/ENODEV/ENOENT
- Use dev_err_probe()

Stephen, is the understanding correct that -ENOENT is the only error
returned for missing clocks ?

 drivers/opp/core.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af

Comments

Dmitry Osipenko Feb. 1, 2021, 7:54 p.m. | #1
01.02.2021 07:22, Viresh Kumar пишет:
> Not all devices that need to use OPP core need to have clocks, a missing

> clock is fine in which case -ENOENT shall be returned by clk_get().

> 

> Anything else is an error and must be handled properly.

> 

> Reported-by: Dmitry Osipenko <digetx@gmail.com>

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

> V2:

> - s/ENODEV/ENOENT

> - Use dev_err_probe()

> 

> Stephen, is the understanding correct that -ENOENT is the only error

> returned for missing clocks ?

> 

>  drivers/opp/core.c | 17 ++++++++++-------

>  1 file changed, 10 insertions(+), 7 deletions(-)

> 

> diff --git a/drivers/opp/core.c b/drivers/opp/core.c

> index a518173fd64a..0beb3ee79523 100644

> --- a/drivers/opp/core.c

> +++ b/drivers/opp/core.c

> @@ -1252,6 +1252,8 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,

>  					       struct opp_table *opp_table,

>  					       bool getclk)

>  {

> +	int ret;

> +

>  	/*

>  	 * Return early if we don't need to get clk or we have already tried it

>  	 * earlier.

> @@ -1261,18 +1263,19 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,

>  

>  	/* Find clk for the device */

>  	opp_table->clk = clk_get(dev, NULL);

> -	if (IS_ERR(opp_table->clk)) {

> -		int ret = PTR_ERR(opp_table->clk);

>  

> -		if (ret == -EPROBE_DEFER) {

> -			dev_pm_opp_put_opp_table(opp_table);

> -			return ERR_PTR(ret);

> -		}

> +	ret = PTR_ERR_OR_ZERO(opp_table->clk);

> +	if (!ret)

> +		return opp_table;

>  

> +	if (ret == -ENOENT) {

>  		dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);

> +		return opp_table;

>  	}

>  

> -	return opp_table;

> +	dev_pm_opp_put_opp_table(opp_table);

> +

> +	return ERR_PTR(dev_err_probe(dev, ret, "Couldn't find clock\n"));

>  }

>  

>  /*

> 


Thanks, looks okay.

Although, I think the previous variant of coding style was a bit more appropriate, i.e. like this:

dev_err_probe(dev, "%s: Couldn't find clock: %d\n", __func__, ret);

return ERR_PTR(ret);
Viresh Kumar Feb. 2, 2021, 4:35 a.m. | #2
On 01-02-21, 22:54, Dmitry Osipenko wrote:
> Thanks, looks okay.

> 

> Although, I think the previous variant of coding style was a bit more appropriate, i.e. like this:

> 

> dev_err_probe(dev, "%s: Couldn't find clock: %d\n", __func__, ret);

> 

> return ERR_PTR(ret);


Okay will move to that.

-- 
viresh
Stephen Boyd Feb. 11, 2021, 7:19 p.m. | #3
Quoting Viresh Kumar (2021-01-31 20:22:58)
> Not all devices that need to use OPP core need to have clocks, a missing

> clock is fine in which case -ENOENT shall be returned by clk_get().

> 

> Anything else is an error and must be handled properly.

> 

> Reported-by: Dmitry Osipenko <digetx@gmail.com>

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

> V2:

> - s/ENODEV/ENOENT

> - Use dev_err_probe()

> 

> Stephen, is the understanding correct that -ENOENT is the only error

> returned for missing clocks ?


Yeah pretty much. See clk_get_optional().

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index a518173fd64a..0beb3ee79523 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1252,6 +1252,8 @@  static struct opp_table *_update_opp_table_clk(struct device *dev,
 					       struct opp_table *opp_table,
 					       bool getclk)
 {
+	int ret;
+
 	/*
 	 * Return early if we don't need to get clk or we have already tried it
 	 * earlier.
@@ -1261,18 +1263,19 @@  static struct opp_table *_update_opp_table_clk(struct device *dev,
 
 	/* Find clk for the device */
 	opp_table->clk = clk_get(dev, NULL);
-	if (IS_ERR(opp_table->clk)) {
-		int ret = PTR_ERR(opp_table->clk);
 
-		if (ret == -EPROBE_DEFER) {
-			dev_pm_opp_put_opp_table(opp_table);
-			return ERR_PTR(ret);
-		}
+	ret = PTR_ERR_OR_ZERO(opp_table->clk);
+	if (!ret)
+		return opp_table;
 
+	if (ret == -ENOENT) {
 		dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
+		return opp_table;
 	}
 
-	return opp_table;
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return ERR_PTR(dev_err_probe(dev, ret, "Couldn't find clock\n"));
 }
 
 /*