| Message ID | 1342475161-20402-1-git-send-email-robherring2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 >
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 >>
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 >
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
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.
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 > > > > >
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 --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