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

[Dmitry Osipenko]

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.

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.

...
>> +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.