| Message ID | CAKohpo=cBkHZnSa8ZsM=k7RRN+RZDXFUWDM82EXXBSQcM-8Vhw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 6, 2013 at 4:32 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > clk[cluster] = clk_get_sys(name, NULL); > - if (!IS_ERR(clk[cluster])) { > + if (!IS_ERR_OR_NULL(clk[cluster])) { > pr_debug("%s: clk: %p & freq table: %p, cluster: %d\n", > __func__, clk[cluster], freq_table[cluster], > cluster); You seem pretty attached to IS_ERR_OR_NULL here. Harvey
On Thu, Mar 07, 2013 at 08:32:37AM +0800, Viresh Kumar wrote: > On 5 March 2013 18:52, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > On Tue, Mar 05, 2013 at 12:52:41PM +0800, Viresh Kumar wrote: > >> + clk[cluster] = clk_get(NULL, name); > >> + if (!IS_ERR_OR_NULL(clk[cluster])) { > > > > NAK. Two reasons. > > > > 1. IS_ERR_OR_NULL. You know about this, it's been on the list several > > times. > > Hey, i had a second thought about this one and i have some other opinion > here. This is a cpufreq driver and we need clock support for sure here, we > can't work without it. And so here is the latest fixup: NAK. You just don't understand.
On 7 March 2013 08:49, Harvey Harrison <harvey.harrison@gmail.com> wrote: > On Wed, Mar 6, 2013 at 4:32 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > >> clk[cluster] = clk_get_sys(name, NULL); >> - if (!IS_ERR(clk[cluster])) { >> + if (!IS_ERR_OR_NULL(clk[cluster])) { >> pr_debug("%s: clk: %p & freq table: %p, cluster: %d\n", >> __func__, clk[cluster], freq_table[cluster], >> cluster); > > > You seem pretty attached to IS_ERR_OR_NULL here. Haha. Not really. I just wanted to get more logical conclusion out. Please check the other mail (where i would reply to Russell), which might have more discussion around this.
On 7 March 2013 08:51, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Thu, Mar 07, 2013 at 08:32:37AM +0800, Viresh Kumar wrote: >> On 5 March 2013 18:52, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: >> > On Tue, Mar 05, 2013 at 12:52:41PM +0800, Viresh Kumar wrote: >> >> + clk[cluster] = clk_get(NULL, name); >> >> + if (!IS_ERR_OR_NULL(clk[cluster])) { >> > >> > NAK. Two reasons. >> > >> > 1. IS_ERR_OR_NULL. You know about this, it's been on the list several >> > times. >> >> Hey, i had a second thought about this one and i have some other opinion >> here. This is a cpufreq driver and we need clock support for sure here, we >> can't work without it. And so here is the latest fixup: > > NAK. You just don't understand. Poor me!! I still remember the huge discussions we had during "clk: Add non CONFIG_HAVE_CLK routines" patchset. For others: https://lkml.org/lkml/2012/4/24/389 Back to the discussion, I understand that clk_get() just returns a cookie and NULL is not an error and so it shouldn't be treated specially. And that's what we do with most of our drivers as all other clk routines (clk_get[set]_rate()) have safe guards against the NULL clk, and they wouldn't complain. The special case we have in a cpufreq driver is, we can't work with zero returned from clk_get_rate()... That will make cpufreq driver work badly. And that's why i got two additions - depends on HAVE_CLK - and using IS_ERR_OR_NULL as NULL returned from clk_get is not a error but is not acceptable to the cpufreq driver. Because i now have the HAVE_CLK dependency i am not sure if we can get NULL returned from clk_get() at all. Can you get around something here? -- viresh
On Thu, Mar 07, 2013 at 10:03:28AM +0800, Viresh Kumar wrote: > On 7 March 2013 08:51, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > On Thu, Mar 07, 2013 at 08:32:37AM +0800, Viresh Kumar wrote: > >> On 5 March 2013 18:52, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > >> > On Tue, Mar 05, 2013 at 12:52:41PM +0800, Viresh Kumar wrote: > >> >> + clk[cluster] = clk_get(NULL, name); > >> >> + if (!IS_ERR_OR_NULL(clk[cluster])) { > >> > > >> > NAK. Two reasons. > >> > > >> > 1. IS_ERR_OR_NULL. You know about this, it's been on the list several > >> > times. > >> > >> Hey, i had a second thought about this one and i have some other opinion > >> here. This is a cpufreq driver and we need clock support for sure here, we > >> can't work without it. And so here is the latest fixup: > > > > NAK. You just don't understand. > > Poor me!! > > I still remember the huge discussions we had during "clk: Add non > CONFIG_HAVE_CLK routines" patchset. > > For others: https://lkml.org/lkml/2012/4/24/389 > > Back to the discussion, I understand that clk_get() just returns a cookie and > NULL is not an error and so it shouldn't be treated specially. And that's what > we do with most of our drivers as all other clk routines (clk_get[set]_rate()) > have safe guards against the NULL clk, and they wouldn't complain. > > The special case we have in a cpufreq driver is, we can't work with > zero returned > from clk_get_rate()... That will make cpufreq driver work badly. So how is this different from any other clock which may also return zero from its clk_get_rate() ? If that's the condition you want to check for, call clk_get_rate() after a successful clk_get*() and check for the condition. Don't go treating the cookie somehow specially. You're *assuming* a behaviour that is inappropriate for the side of the interface you're working with.
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index fdf54a9..87b7e48 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -9,8 +9,7 @@ config ARM_BIG_LITTLE_CPUFREQ config ARM_DT_BL_CPUFREQ tristate "Generic ARM big LITTLE CPUfreq driver probed via DT" select ARM_BIG_LITTLE_CPUFREQ - depends on OF - default n + depends on OF && HAVE_CLK help This enables the Generic CPUfreq driver for ARM big.LITTLE platform. This gets frequency tables from DT. diff --git a/drivers/cpufreq/arm_big_little.c b/drivers/cpufreq/arm_big_little.c index 2486b9a..d1fdc65 100644 --- a/drivers/cpufreq/arm_big_little.c +++ b/drivers/cpufreq/arm_big_little.c @@ -147,7 +147,7 @@ static void put_cluster_clk_and_freq_table(u32 cluster) static int get_cluster_clk_and_freq_table(u32 cluster) { - char name[9] = "cluster"; + char name[9] = "cpu-cluster"; int count; if (atomic_inc_return(&cluster_usage[cluster]) != 1) @@ -159,7 +159,7 @@ static int get_cluster_clk_and_freq_table(u32 cluster) name[7] = cluster + '0'; clk[cluster] = clk_get_sys(name, NULL); - if (!IS_ERR(clk[cluster])) { + if (!IS_ERR_OR_NULL(clk[cluster])) { pr_debug("%s: clk: %p & freq table: %p, cluster: %d\n", __func__, clk[cluster], freq_table[cluster], cluster);