diff mbox

clk: fix compile for OF && !COMMON_CLK

Message ID 1342475161-20402-1-git-send-email-robherring2@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rob Herring July 16, 2012, 9:46 p.m. UTC
From: Rob Herring <rob.herring@calxeda.com>

With commit 766e6a4ec602d0c107 (clk: add DT clock binding support),
compiling with OF && !COMMON_CLK is broken.

Reported-by: Alexandre Pereira da Silva <aletes.xgr@gmail.com>
Reported-by: Prashant Gaikwad <pgaikwad@nvidia.com>
Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 drivers/clk/clkdev.c |    2 +-
 include/linux/clk.h  |    7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Mike Turquette July 17, 2012, 12:12 a.m. UTC | #1
On 20120716-16:46, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> With commit 766e6a4ec602d0c107 (clk: add DT clock binding support),
> compiling with OF && !COMMON_CLK is broken.
> 

Hi Rob,

Thanks for sending this quickly.

<snip>
> @@ -313,19 +314,19 @@ int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
>  struct device_node;
>  struct of_phandle_args;
>  
> -#ifdef CONFIG_OF
> +#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>  struct clk *of_clk_get(struct device_node *np, int index);
>  struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
>  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
>  #else
>  static inline struct clk *of_clk_get(struct device_node *np, int index)
>  {
> -	return NULL;
> +	return ERR_PTR(-EINVAL);

This change seems unrelated?

>  }
>  static inline struct clk *of_clk_get_by_name(struct device_node *np,
>  					     const char *name)
>  {
> -	return NULL;
> +	return ERR_PTR(-EINVAL);

Ditto.

Thanks,
Mike

>  }
>  #endif
>  
> -- 
> 1.7.9.5
>
Alexandre Pereira da Silva July 17, 2012, 12:43 a.m. UTC | #2
On Mon, Jul 16, 2012 at 6:46 PM, Rob Herring <robherring2@gmail.com> wrote:
> From: Rob Herring <rob.herring@calxeda.com>
>
> With commit 766e6a4ec602d0c107 (clk: add DT clock binding support),
> compiling with OF && !COMMON_CLK is broken.

Thanks, Rob.

This fixed the issue for me, but Mike's comments applies.

> Reported-by: Alexandre Pereira da Silva <aletes.xgr@gmail.com>
> Reported-by: Prashant Gaikwad <pgaikwad@nvidia.com>
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> ---
>  drivers/clk/clkdev.c |    2 +-
>  include/linux/clk.h  |    7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 20649b3..8f87b0f 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -24,7 +24,7 @@
>  static LIST_HEAD(clocks);
>  static DEFINE_MUTEX(clocks_mutex);
>
> -#ifdef CONFIG_OF
> +#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>  struct clk *of_clk_get(struct device_node *np, int index)
>  {
>         struct of_phandle_args clkspec;
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 8b70342..35f7492 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -12,6 +12,7 @@
>  #ifndef __LINUX_CLK_H
>  #define __LINUX_CLK_H
>
> +#include <linux/err.h>
>  #include <linux/kernel.h>
>  #include <linux/notifier.h>
>
> @@ -313,19 +314,19 @@ int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
>  struct device_node;
>  struct of_phandle_args;
>
> -#ifdef CONFIG_OF
> +#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>  struct clk *of_clk_get(struct device_node *np, int index);
>  struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
>  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
>  #else
>  static inline struct clk *of_clk_get(struct device_node *np, int index)
>  {
> -       return NULL;
> +       return ERR_PTR(-EINVAL);
>  }
>  static inline struct clk *of_clk_get_by_name(struct device_node *np,
>                                              const char *name)
>  {
> -       return NULL;
> +       return ERR_PTR(-EINVAL);
>  }
>  #endif
>
> --
> 1.7.9.5
>
Rob Herring July 17, 2012, 2:08 a.m. UTC | #3
On 07/16/2012 07:12 PM, Mike Turquette wrote:
> On 20120716-16:46, Rob Herring wrote:
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> With commit 766e6a4ec602d0c107 (clk: add DT clock binding support),
>> compiling with OF && !COMMON_CLK is broken.
>>
> 
> Hi Rob,
> 
> Thanks for sending this quickly.
> 
> <snip>
>> @@ -313,19 +314,19 @@ int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
>>  struct device_node;
>>  struct of_phandle_args;
>>  
>> -#ifdef CONFIG_OF
>> +#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
>>  struct clk *of_clk_get(struct device_node *np, int index);
>>  struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
>>  struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
>>  #else
>>  static inline struct clk *of_clk_get(struct device_node *np, int index)
>>  {
>> -	return NULL;
>> +	return ERR_PTR(-EINVAL);
> 
> This change seems unrelated?
> 
>>  }
>>  static inline struct clk *of_clk_get_by_name(struct device_node *np,
>>  					     const char *name)
>>  {
>> -	return NULL;
>> +	return ERR_PTR(-EINVAL);
> 
> Ditto.

Yeah, it should probably go in Shawn's fix as that is actually when it
will start breaking.

Rob

> 
> Thanks,
> Mike
> 
>>  }
>>  #endif
>>  
>> -- 
>> 1.7.9.5
>>
Prashant Gaikwad July 17, 2012, 4:58 a.m. UTC | #4
On Tuesday 17 July 2012 03:16 AM, Rob Herring wrote:
> From: Rob Herring<rob.herring@calxeda.com>
>
> With commit 766e6a4ec602d0c107 (clk: add DT clock binding support),
> compiling with OF&&  !COMMON_CLK is broken.

Thanks Rob!!

This patch fixed the build failure for Tegra.

> Reported-by: Alexandre Pereira da Silva<aletes.xgr@gmail.com>
> Reported-by: Prashant Gaikwad<pgaikwad@nvidia.com>
> Signed-off-by: Rob Herring<rob.herring@calxeda.com>
> ---
>   drivers/clk/clkdev.c |    2 +-
>   include/linux/clk.h  |    7 ++++---
>   2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 20649b3..8f87b0f 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -24,7 +24,7 @@
>   static LIST_HEAD(clocks);
>   static DEFINE_MUTEX(clocks_mutex);
>
> -#ifdef CONFIG_OF
> +#if defined(CONFIG_OF)&&  defined(CONFIG_COMMON_CLK)
>   struct clk *of_clk_get(struct device_node *np, int index)
>   {
>   	struct of_phandle_args clkspec;
> diff --git a/include/linux/clk.h b/include/linux/clk.h
> index 8b70342..35f7492 100644
> --- a/include/linux/clk.h
> +++ b/include/linux/clk.h
> @@ -12,6 +12,7 @@
>   #ifndef __LINUX_CLK_H
>   #define __LINUX_CLK_H
>
> +#include<linux/err.h>
>   #include<linux/kernel.h>
>   #include<linux/notifier.h>
>
> @@ -313,19 +314,19 @@ int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
>   struct device_node;
>   struct of_phandle_args;
>
> -#ifdef CONFIG_OF
> +#if defined(CONFIG_OF)&&  defined(CONFIG_COMMON_CLK)
>   struct clk *of_clk_get(struct device_node *np, int index);
>   struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
>   struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
>   #else
>   static inline struct clk *of_clk_get(struct device_node *np, int index)
>   {
> -	return NULL;
> +	return ERR_PTR(-EINVAL);
>   }
>   static inline struct clk *of_clk_get_by_name(struct device_node *np,
>   					     const char *name)
>   {
> -	return NULL;
> +	return ERR_PTR(-EINVAL);
>   }
>   #endif
>
Rajendra Nayak July 17, 2012, 1:19 p.m. UTC | #5
Rob, Mike,

On Tuesday 17 July 2012 07:38 AM, Rob Herring wrote:
> On 07/16/2012 07:12 PM, Mike Turquette wrote:
>> On 20120716-16:46, Rob Herring wrote:
>>> From: Rob Herring<rob.herring@calxeda.com>
>>>
>>> With commit 766e6a4ec602d0c107 (clk: add DT clock binding support),
>>> compiling with OF&&  !COMMON_CLK is broken.
>>>
>>
>> Hi Rob,
>>
>> Thanks for sending this quickly.
>>
>> <snip>
>>> @@ -313,19 +314,19 @@ int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
>>>   struct device_node;
>>>   struct of_phandle_args;
>>>
>>> -#ifdef CONFIG_OF
>>> +#if defined(CONFIG_OF)&&  defined(CONFIG_COMMON_CLK)
>>>   struct clk *of_clk_get(struct device_node *np, int index);
>>>   struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
>>>   struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
>>>   #else
>>>   static inline struct clk *of_clk_get(struct device_node *np, int index)
>>>   {
>>> -	return NULL;
>>> +	return ERR_PTR(-EINVAL);

So how is this expected to work on platforms (like OMAP) which have 
CONFIG_OF enabled but not CONFIG_COMMON_CLK?

Archit has been seeing issues with failed clk_get's in the omap dss
driver on linux-next.
The clk_gets pass a valid dev pointer and an alias/con-id.

With the $Subject patch, the of_clk_get_by_name() for our builds always
returns a ERR_PTR(-EINVAL).

Even if we do get the right of_clk_get_by_name() built in,
there is another issue on OMAP where in we have CONFIG_OF
enabled/selected by default for all OMAP2+ builds, even when we
*do not* pass a dt blob to the kernel.

So would the below code fail in such cases because it expects a
valid of_node to be populated for a device (which also has clock
information in it)? if CONFIG_OF is set.

struct clk *clk_get(struct device *dev, const char *con_id)
{
         const char *dev_id = dev ? dev_name(dev) : NULL;
         struct clk *clk;

         if (dev) {
---->
	Any reason why this isn't
	if (dev->of_node) {
---->
                 clk = of_clk_get_by_name(dev->of_node, con_id);
                 if (clk && __clk_get(clk))
                         return clk;
         }

         return clk_get_sys(dev_id, con_id);
}

regards,
Rajendra
Rajendra Nayak July 17, 2012, 1:34 p.m. UTC | #6
On Tuesday 17 July 2012 06:49 PM, Rajendra Nayak wrote:
> struct clk *clk_get(struct device *dev, const char *con_id)
> {
>          const char *dev_id = dev ? dev_name(dev) : NULL;
>          struct clk *clk;
>
>          if (dev) {
> ---->
>      Any reason why this isn't
>      if (dev->of_node) {
> ---->

Or rather,
	if (dev && dev->of_node) {
as Archit just pointed out to me.
Rob Herring July 17, 2012, 1:46 p.m. UTC | #7
On 07/17/2012 08:19 AM, Rajendra Nayak wrote:
> Rob, Mike,
> 
> On Tuesday 17 July 2012 07:38 AM, Rob Herring wrote:
>> On 07/16/2012 07:12 PM, Mike Turquette wrote:
>>> On 20120716-16:46, Rob Herring wrote:
>>>> From: Rob Herring<rob.herring@calxeda.com>
>>>>
>>>> With commit 766e6a4ec602d0c107 (clk: add DT clock binding support),
>>>> compiling with OF&&  !COMMON_CLK is broken.
>>>>
>>>
>>> Hi Rob,
>>>
>>> Thanks for sending this quickly.
>>>
>>> <snip>
>>>> @@ -313,19 +314,19 @@ int clk_add_alias(const char *alias, const
>>>> char *alias_dev_name, char *id,
>>>>   struct device_node;
>>>>   struct of_phandle_args;
>>>>
>>>> -#ifdef CONFIG_OF
>>>> +#if defined(CONFIG_OF)&&  defined(CONFIG_COMMON_CLK)
>>>>   struct clk *of_clk_get(struct device_node *np, int index);
>>>>   struct clk *of_clk_get_by_name(struct device_node *np, const char
>>>> *name);
>>>>   struct clk *of_clk_get_from_provider(struct of_phandle_args
>>>> *clkspec);
>>>>   #else
>>>>   static inline struct clk *of_clk_get(struct device_node *np, int
>>>> index)
>>>>   {
>>>> -    return NULL;
>>>> +    return ERR_PTR(-EINVAL);
> 
> So how is this expected to work on platforms (like OMAP) which have
> CONFIG_OF enabled but not CONFIG_COMMON_CLK?
> 

As I mentioned in my other reply, this really belongs with Shawn's patch
that changes the return value checking from NULL to err values.

> Archit has been seeing issues with failed clk_get's in the omap dss
> driver on linux-next.
> The clk_gets pass a valid dev pointer and an alias/con-id.
> 
> With the $Subject patch, the of_clk_get_by_name() for our builds always
> returns a ERR_PTR(-EINVAL).
> 
> Even if we do get the right of_clk_get_by_name() built in,
> there is another issue on OMAP where in we have CONFIG_OF
> enabled/selected by default for all OMAP2+ builds, even when we
> *do not* pass a dt blob to the kernel.
> 
> So would the below code fail in such cases because it expects a
> valid of_node to be populated for a device (which also has clock
> information in it)? if CONFIG_OF is set.
> 
> struct clk *clk_get(struct device *dev, const char *con_id)
> {
>         const char *dev_id = dev ? dev_name(dev) : NULL;
>         struct clk *clk;
> 
>         if (dev) {
> ---->
>     Any reason why this isn't
>     if (dev->of_node) {
> ---->

dev may be null, we could do (dev && dev->of_node) though. Either way
the error handling here needs to do the right thing.

>                 clk = of_clk_get_by_name(dev->of_node, con_id);
>                 if (clk && __clk_get(clk))
>                         return clk;
>         }
> 
>         return clk_get_sys(dev_id, con_id);
> }
> 
> regards,
> Rajendra
> 
> 
> 
> 
>
Rajendra Nayak July 17, 2012, 1:54 p.m. UTC | #8
On Tuesday 17 July 2012 07:16 PM, Rob Herring wrote:
>> So how is this expected to work on platforms (like OMAP) which have
>> >  CONFIG_OF enabled but not CONFIG_COMMON_CLK?
>> >
> As I mentioned in my other reply, this really belongs with Shawn's patch
> that changes the return value checking from NULL to err values.
>
ah, I seemed to have missed that. Just had a look at Shawn's patch and
that should work for OMAP too. thanks.
diff mbox

Patch

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 20649b3..8f87b0f 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -24,7 +24,7 @@ 
 static LIST_HEAD(clocks);
 static DEFINE_MUTEX(clocks_mutex);
 
-#ifdef CONFIG_OF
+#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
 struct clk *of_clk_get(struct device_node *np, int index)
 {
 	struct of_phandle_args clkspec;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 8b70342..35f7492 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -12,6 +12,7 @@ 
 #ifndef __LINUX_CLK_H
 #define __LINUX_CLK_H
 
+#include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/notifier.h>
 
@@ -313,19 +314,19 @@  int clk_add_alias(const char *alias, const char *alias_dev_name, char *id,
 struct device_node;
 struct of_phandle_args;
 
-#ifdef CONFIG_OF
+#if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
 struct clk *of_clk_get(struct device_node *np, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
 struct clk *of_clk_get_from_provider(struct of_phandle_args *clkspec);
 #else
 static inline struct clk *of_clk_get(struct device_node *np, int index)
 {
-	return NULL;
+	return ERR_PTR(-EINVAL);
 }
 static inline struct clk *of_clk_get_by_name(struct device_node *np,
 					     const char *name)
 {
-	return NULL;
+	return ERR_PTR(-EINVAL);
 }
 #endif