diff mbox

[PATCHv2,2/4] cpufreq: cpufreq-dt: extend with platform_data

Message ID 1410350908-11316-3-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Thomas Petazzoni Sept. 10, 2014, 12:08 p.m. UTC 
This commit extends the cpufreq-dt driver to take a platform_data
structure. This structure is for now used to tell the cpufreq-dt
driver the layout of the clocks on the platform, i.e whether all CPUs
share the same clock or whether each CPU has a separate clock.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++--
 include/linux/cpufreq-dt.h   | 22 ++++++++++++++++++++++
 2 files changed, 37 insertions(+), 2 deletions(-)
 create mode 100644 include/linux/cpufreq-dt.h

Comments

Viresh Kumar Sept. 10, 2014, 12:22 p.m. UTC | #1 
On 10 September 2014 17:38, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> +       dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);

I don't think this is right. What if platform device's platform data
is freed later?
That's why its always better to duplicate that structure instead of playing with
pointers.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thomas Petazzoni Sept. 10, 2014, 12:28 p.m. UTC | #2 
Dear Viresh Kumar,

On Wed, 10 Sep 2014 17:52:59 +0530, Viresh Kumar wrote:
> On 10 September 2014 17:38, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > +       dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);
> 
> I don't think this is right. What if platform device's platform data
> is freed later?
> That's why its always better to duplicate that structure instead of playing with
> pointers.

Isn't the piece of code registering the platform_device supposed to
make sure that platform_data doesn't disappear? At least, in PATCH 3/4,
I'm using platform_device_register_data(), which does a kmemdup() of
the custom data being passed before assigning the struct
device->platform_data field.

But if you like, I can add one more memory copy :)

Thomas
Viresh Kumar Sept. 10, 2014, 12:32 p.m. UTC | #3 
On 10 September 2014 17:58, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Viresh Kumar,
>
> On Wed, 10 Sep 2014 17:52:59 +0530, Viresh Kumar wrote:
>> On 10 September 2014 17:38, Thomas Petazzoni
>> <thomas.petazzoni@free-electrons.com> wrote:
>> > +       dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);
>>
>> I don't think this is right. What if platform device's platform data
>> is freed later?
>> That's why its always better to duplicate that structure instead of playing with
>> pointers.
>
> Isn't the piece of code registering the platform_device supposed to
> make sure that platform_data doesn't disappear? At least, in PATCH 3/4,

I don't know. I remember this from the days when I used to write individual
drivers for SPEAr platform... Its been some time now that I have seen this :)

> I'm using platform_device_register_data(), which does a kmemdup() of
> the custom data being passed before assigning the struct
> device->platform_data field.

Atleast in your case it isn't required to copy anymore but this driver can be
used by others which may not guarantee that..

> But if you like, I can add one more memory copy :)

Its not what I like, as wasting memory isn't sensible at all.. But about what's
the right thing to do to make this code un-breakable..

@Arnd: Any inputs?
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Sept. 10, 2014, 12:36 p.m. UTC | #4 
On Wednesday 10 September 2014 18:02:59 Viresh Kumar wrote:
> > I'm using platform_device_register_data(), which does a kmemdup() of
> > the custom data being passed before assigning the struct
> > device->platform_data field.
> 
> Atleast in your case it isn't required to copy anymore but this driver can be
> used by others which may not guarantee that..
> 
> > But if you like, I can add one more memory copy 
> 
> Its not what I like, as wasting memory isn't sensible at all.. But about what's
> the right thing to do to make this code un-breakable..
> 
> @Arnd: Any inputs?

platform_device_register_data() is the right way to register a platform
device if one needs to do that, and it will always copy the data,
so that should be used, and the driver should rely on the data not
getting freed under it.

Statically defining platform_device instances is generally a bad idea,
and we should not do that, but if someone does it is up to them to
make sure the platform_data is not marked as __init.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index e002650..987bbfa 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -18,6 +18,7 @@ 
 #include <linux/cpu.h>
 #include <linux/cpu_cooling.h>
 #include <linux/cpufreq.h>
+#include <linux/cpufreq-dt.h>
 #include <linux/cpumask.h>
 #include <linux/err.h>
 #include <linux/module.h>
@@ -178,6 +179,7 @@  try_again:
 
 static int cpufreq_init(struct cpufreq_policy *policy)
 {
+	struct cpufreq_dt_platform_data *pd;
 	struct cpufreq_frequency_table *freq_table;
 	struct thermal_cooling_device *cdev;
 	struct device_node *np;
@@ -266,9 +268,18 @@  static int cpufreq_init(struct cpufreq_policy *policy)
 	policy->driver_data = priv;
 
 	policy->clk = cpu_clk;
-	ret = cpufreq_generic_init(policy, freq_table, transition_latency);
-	if (ret)
+	ret = cpufreq_table_validate_and_show(policy, freq_table);
+	if (ret) {
+		dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__,
+			ret);
 		goto out_cooling_unregister;
+	}
+
+	policy->cpuinfo.transition_latency = transition_latency;
+
+	pd = cpufreq_get_driver_data();
+	if (pd && !pd->independent_clocks)
+		cpumask_setall(policy->cpus);
 
 	return 0;
 
@@ -334,6 +345,8 @@  static int dt_cpufreq_probe(struct platform_device *pdev)
 	if (!IS_ERR(cpu_reg))
 		regulator_put(cpu_reg);
 
+	dt_cpufreq_driver.driver_data = dev_get_platdata(&pdev->dev);
+
 	ret = cpufreq_register_driver(&dt_cpufreq_driver);
 	if (ret)
 		dev_err(cpu_dev, "failed register driver: %d\n", ret);
diff --git a/include/linux/cpufreq-dt.h b/include/linux/cpufreq-dt.h
new file mode 100644
index 0000000..0414009
--- /dev/null
+++ b/include/linux/cpufreq-dt.h
@@ -0,0 +1,22 @@ 
+/*
+ * Copyright (C) 2014 Marvell
+ * Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __CPUFREQ_DT_H__
+#define __CPUFREQ_DT_H__
+
+struct cpufreq_dt_platform_data {
+	/*
+	 * True when each CPU has its own clock to control its
+	 * frequency, false when all CPUs are controlled by a single
+	 * clock.
+	 */
+	bool independent_clocks;
+};
+
+#endif /* __CPUFREQ_DT_H__ */