Re: [PATCH v1 1/1] soc/tegra: Add devm_tegra_core_dev_init_opp_table()

[Dmitry Osipenko <digetx@gmail.com>]

17.05.2021 17:52, Krzysztof Kozlowski пишет:
>>>> +static int tegra_core_dev_init_opp_state(struct device *dev)
>>>> +{
>>>> +	struct dev_pm_opp *opp;
>>>> +	unsigned long rate;
>>>> +	struct clk *clk;
>>>> +	int err;
>>>> +
>>>> +	clk = devm_clk_get(dev, NULL);
>>>> +	if (IS_ERR(clk)) {
>>>> +		dev_err(dev, "failed to get clk: %pe\n", clk);
>>>> +		return PTR_ERR(clk);
>>>> +	}
>>>> +
>>>> +	rate = clk_get_rate(clk);
>>>> +	if (!rate) {
>>>> +		dev_err(dev, "failed to get clk rate\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	opp = dev_pm_opp_find_freq_ceil(dev, &rate);
>>>> +
>>>> +	if (opp == ERR_PTR(-ERANGE))
>>>> +		opp = dev_pm_opp_find_freq_floor(dev, &rate);
>>>> +
>>>> +	err = PTR_ERR_OR_ZERO(opp);
>>>> +	if (err) {
>>>> +		dev_err(dev, "failed to get OPP for %ld Hz: %d\n",
>>>> +			rate, err);
>>>> +		return err;
>>>> +	}
>>>> +
>>>> +	dev_pm_opp_put(opp);
>>>> +
>>>> +	/* first dummy rate-setting initializes voltage vote */
>>>> +	err = dev_pm_opp_set_rate(dev, rate);
>>>> +	if (err) {
>>>> +		dev_err(dev, "failed to initialize OPP clock: %d\n", err);
>>>> +		return err;
>>>> +	}
>>>
>>> The devm_pm_opp_set_clkname will call clk_get(), so here you should drop
>>> the clk reference at the end. Why having it twice?
>> The devm_pm_opp_set_clkname assigns clock to the OPP table.
>>
>> The devm_clk_get() is needed for the clk_get_rate(). OPP core doesn't
>> initialize voltage vote and we need this initialization for the Tegra
>> memory drivers.
> I did not get the answer to my question. Why you need to keep the clk
> reference past this point? Why you cannot drop it after getting rate?
> 
>> The reference count of the clk will be dropped automatically once device
>> driver is released. The resource-managed helper avoids the need to care
>> about the error unwinding in the code, making it clean and easy to follow.
> I am not saying there is a leak.
> 

The clk reference is not needed past this point. It doesn't hurt to have
additional reference since this allows to make code cleaner.