linux-kernel - Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20140723070949.GB15759@ulmo.nvidia.com>
Date:	Wed, 23 Jul 2014 09:09:50 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Tuomas Tynkkynen <ttynkkynen@...dia.com>
Cc:	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-pm@...r.kernel.org,
	Stephen Warren <swarren@...dotorg.org>,
	Peter De Schrijver <pdeschrijver@...dia.com>,
	Prashant Gaikwad <pgaikwad@...dia.com>,
	Mike Turquette <mturquette@...aro.org>,
	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	Paul Walmsley <pwalmsley@...dia.com>,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v2 14/16] cpufreq: Add cpufreq driver for Tegra124

On Mon, Jul 21, 2014 at 06:39:00PM +0300, Tuomas Tynkkynen wrote:
[...]
> diff --git a/drivers/cpufreq/tegra124-cpufreq.c b/drivers/cpufreq/tegra124-cpufreq.c
[...]
> +static int tegra124_cpu_switch_to_dfll(void)
> +{
> +	struct clk *original_cpu_clk_parent;

Maybe just "parent"?

> +	unsigned long rate;
> +	struct dev_pm_opp *opp;
> +	int ret;
> +
> +	rate = clk_get_rate(cpu_clk);
> +	opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate);
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
> +
> +	ret = clk_set_rate(dfll_clk, rate);
> +	if (ret)
> +		return ret;
> +
> +	original_cpu_clk_parent = clk_get_parent(cpu_clk);
> +	clk_set_parent(cpu_clk, pllp_clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_prepare_enable(dfll_clk);
> +	if (ret)
> +		goto out_switch_to_original_parent;

This could simply be "out" or "err" or anything else shorter than the
above.

> +
> +	clk_set_parent(cpu_clk, dfll_clk);
> +
> +	return 0;
> +
> +out_switch_to_original_parent:
> +	clk_set_parent(cpu_clk, original_cpu_clk_parent);
> +
> +	return ret;
> +}
> +
> +static struct platform_device_info cpufreq_cpu0_devinfo = {
> +	.name = "cpufreq-cpu0",
> +};
> +
> +static int tegra124_cpufreq_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	cpu_dev = get_cpu_device(0);
> +	if (!cpu_dev)
> +		return -ENODEV;
> +
> +	cpu_clk = of_clk_get_by_name(cpu_dev->of_node, "cpu_g");
> +	if (IS_ERR(cpu_clk))
> +		return PTR_ERR(cpu_clk);
> +
> +	dfll_clk = of_clk_get_by_name(cpu_dev->of_node, "dfll");
> +	if (IS_ERR(dfll_clk)) {
> +		ret = PTR_ERR(dfll_clk);
> +		goto out_put_cpu_clk;
> +	}
> +
> +	pllx_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_x");
> +	if (IS_ERR(pllx_clk)) {
> +		ret = PTR_ERR(pllx_clk);
> +		goto out_put_dfll_clk;
> +	}
> +
> +	pllp_clk = of_clk_get_by_name(cpu_dev->of_node, "pll_p");
> +	if (IS_ERR(pllp_clk)) {
> +		ret = PTR_ERR(pllp_clk);
> +		goto out_put_pllx_clk;
> +	}

Can the above not be devm_clk_get(cpu_dev, "...") so that you can remove
all the clk_put() calls in the cleanup code below?

> +
> +	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
> +	if (ret)
> +		goto out_put_pllp_clk;
> +
> +	ret = tegra124_cpu_switch_to_dfll();
> +	if (ret)
> +		goto out_free_table;
> +
> +	platform_device_register_full(&cpufreq_cpu0_devinfo);

Should the cpufreq_cpu0_devinfo device perhaps be a child of pdev?

> +
> +	return 0;
> +
> +out_free_table:
> +	dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table);
> +out_put_pllp_clk:
> +	clk_put(pllp_clk);
> +out_put_pllx_clk:
> +	clk_put(pllx_clk);
> +out_put_dfll_clk:
> +	clk_put(dfll_clk);
> +out_put_cpu_clk:
> +	clk_put(cpu_clk);
> +
> +	return ret;
> +}
> +
> +static struct platform_driver tegra124_cpufreq_platdrv = {
> +	.driver = {
> +		.name	= "cpufreq-tegra124",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe		= tegra124_cpufreq_probe,

Note that simply leaving out .remove() here doesn't guarantee that the
driver won't be unloaded. Also building it into the kernel doesn't
prevent that. You can still unbind the driver via sysfs. So you'd need
to add a .suppress_bind_attrs = true above.

But is there even a reason why we need that? Couldn't we make the
driver's .remove() undo what .probe() did so that the driver can be
unloaded?

Otherwise it probably makes more sense not to use a driver (and dummy
device) at all as Viresh already mentioned.

> +};
> +
> +static const struct of_device_id soc_of_matches[] = {
> +	{ .compatible = "nvidia,tegra124", },
> +	{}
> +};
> +
> +static int __init tegra_cpufreq_init(void)
> +{
> +	int ret;
> +	struct platform_device *pdev;
> +
> +	if (!of_find_matching_node(NULL, soc_of_matches))
> +		return -ENODEV;

I think this could be of_machine_is_compatible() since there's only a
single entry in the match table. If there's a good chance that we may
end up with more entries, perhaps now would be a good time to add an
of_match_machine() function?

> +
> +	ret = platform_driver_register(&tegra124_cpufreq_platdrv);
> +	if (ret)
> +		return ret;
> +
> +	pdev = platform_device_register_simple("cpufreq-tegra124", -1, NULL, 0);
> +	if (IS_ERR(pdev)) {
> +		platform_driver_unregister(&tegra124_cpufreq_platdrv);
> +		return PTR_ERR(pdev);
> +	}
> +
> +	return 0;
> +}
> +
> +MODULE_AUTHOR("Tuomas Tynkkynen <ttynkkynen@...dia.com>");
> +MODULE_DESCRIPTION("cpufreq driver for nVIDIA Tegra124");

We use "NVIDIA" everywhere nowadays.

> +MODULE_LICENSE("GPLv2");

The correct license string is "GPL v2".

> +module_init(tegra_cpufreq_init);

The placement of this is unusual. It should go immediately below the
tegra_cpufreq_init() function.

Thierry

Content of type "application/pgp-signature" skipped