On 17/05/2021 10:47, Dmitry Osipenko wrote:
> 17.05.2021 14:43, 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.
>
> ...
>>> +EXPORT_SYMBOL_GPL(devm_tegra_core_dev_init_opp_table);
>>> diff --git a/include/soc/tegra/common.h b/include/soc/tegra/common.h
>>> index 98027a76ce3d..e8eab13aa199 100644
>>> --- a/include/soc/tegra/common.h
>>> +++ b/include/soc/tegra/common.h
>>> @@ -6,6 +6,36 @@
>>> #ifndef __SOC_TEGRA_COMMON_H__
>>> #define __SOC_TEGRA_COMMON_H__
>>>
>>> +#include <linux/errno.h>
>>> +#include <linux/types.h>
>>> +
>>> +struct device;
>>> +
>>> +/**
>>> + * Tegra SoC core device OPP table configuration
>>> + *
>>> + * @init_state: pre-initialize OPP state of a device
>>> + */
>>> +struct tegra_core_opp_params {
>>> + bool init_state;
>>> +};
>>> +
>>> +#ifdef CONFIG_ARCH_TEGRA
>>> bool soc_is_tegra(void);
>>> +int devm_tegra_core_dev_init_opp_table(struct device *dev,
>>> + struct tegra_core_opp_params *params);
>>> +#else
>>> +static inline bool soc_is_tegra(void)
>>
>> This looks unrelated. Please make it a separate patch.
>
> The missing stub for soc_is_tegra() popped up multiple times before.
> Hence it didn't look like a bad idea to me to add stub for it since this
> patch touches code around it.
>
> I'll factor it out into a separate patch in v2.
Thanks!
Best regards,
Krzysztof
