Re: [PATCH v4] opp: Use clk_get_optional() to avoid leaving opp_table->clk as an error pointer

On 27-07-26, 22:33, Praveen Talari wrote:
> Suggested-by: Sebastian Reichel <[email protected]>
> Reviewed-by: Sebastian Reichel <[email protected]>
> Signed-off-by: Praveen Talari <[email protected]>
> ---

You added ^^ and ..

> Changes in v4:
> - Rebased on
>   https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/log/?h=cpufreq/arm/linux-next
> - Removed ret variable.
> - Link to v3:
>   https://lore.kernel.org/all/[email protected]
> 
> Changes in v3:
> - Added OPP framework maintainers
> - Link to v2:
>   https://patch.msgid.link/[email protected]
> 
> Changes in v2:
> - Switched from guarding clk_round_rate() against error pointers to
>   fixing the root cause in OPP core: use clk_get_optional() instead
>   of clk_get() in _update_opp_table_clk(), so opp_table->clk is left
>   as NULL (not ERR_PTR(-ENOENT)) when a device has no Linux clock,
>   per Sebastian Reichel's review suggestion.
> - Dropped the drivers/clk/clk.c change entirely.
> - Link to v1:
>   https://patch.msgid.link/[email protected]
> 
> To: [email protected]
> To: Viresh Kumar <[email protected]>
> To: Nishanth Menon <[email protected]>
> To: Stephen Boyd <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---

and this ^^

Only one line with `---` is enough and you can add non-commit part
after that.

>  drivers/opp/core.c | 54 ++++++++++++++++++++++++------------------------------
>  1 file changed, 24 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index ab0b0a2f85a1..f2b2ffdb410d 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1582,8 +1582,6 @@ 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 done it
>  	 * earlier.
> @@ -1592,39 +1590,35 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
>  	    opp_table->clks)
>  		return opp_table;
>  
> -	/* Find clk for the device */
> -	opp_table->clk = clk_get(dev, NULL);
> +	/*
> +	 * There are few platforms which don't want the OPP core to manage
> +	 * device's clock settings. In such cases neither the platform
> +	 * provides the clks explicitly to us, nor the DT contains a valid
> +	 * clk entry. The OPP nodes in DT may still contain "opp-hz" property
> +	 * though, which we need to parse and allow the platform to find an
> +	 * OPP based on freq later on.
> +	 *
> +	 * This is a simple solution to take care of such corner cases, i.e.
> +	 * make the clk_count 1, which lets us allocate space for frequency
> +	 * in opp->rates and also parse the entries in DT. Use
> +	 * clk_get_optional() instead of clk_get() so opp_table->clk stays
> +	 * NULL for such devices, instead of holding an ERR_PTR(-ENOENT) that
> +	 * consumers must remember to special-case.
> +	 */
> +	opp_table->clk = clk_get_optional(dev, NULL);
>  
> -	ret = PTR_ERR_OR_ZERO(opp_table->clk);
> -	if (!ret) {
> -		opp_table->config_clks = _opp_config_clk_single;
> -		opp_table->clk_count = 1;
> -		return opp_table;
> +	if (IS_ERR(opp_table->clk)) {
> +		dev_pm_opp_put_opp_table(opp_table);
> +		dev_err_probe(dev, PTR_ERR(opp_table->clk), "Couldn't find clock\n");
> +		return ERR_CAST(opp_table->clk);
>  	}
>  
> -	if (ret == -ENOENT) {
> -		/*
> -		 * There are few platforms which don't want the OPP core to
> -		 * manage device's clock settings. In such cases neither the
> -		 * platform provides the clks explicitly to us, nor the DT
> -		 * contains a valid clk entry. The OPP nodes in DT may still
> -		 * contain "opp-hz" property though, which we need to parse and
> -		 * allow the platform to find an OPP based on freq later on.
> -		 *
> -		 * This is a simple solution to take care of such corner cases,
> -		 * i.e. make the clk_count 1, which lets us allocate space for
> -		 * frequency in opp->rates and also parse the entries in DT.
> -		 */
> -		opp_table->clk_count = 1;
> -
> -		dev_dbg(dev, "%s: Couldn't find clock: %d\n", __func__, ret);
> -		return opp_table;
> -	}
> +	if (opp_table->clk)
> +		opp_table->config_clks = _opp_config_clk_single;
>  
> -	dev_pm_opp_put_opp_table(opp_table);
> -	dev_err_probe(dev, ret, "Couldn't find clock\n");
> +	opp_table->clk_count = 1;
>  
> -	return ERR_PTR(ret);
> +	return opp_table;
>  }

Applied. Thanks.

-- 
viresh