Re: [PATCH 1/2] clk: Make clk API return per-user struct clk instances

2014-10-07 Thread Tomeu Vizoso 
On 4 October 2014 01:15, Stephen Boyd sb...@codeaurora.org wrote:
 On 10/02, Tomeu Vizoso wrote:
 +
  #if defined(CONFIG_OF)  defined(CONFIG_COMMON_CLK)

 These ifdefs look useless.

  struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec);
  struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
  void of_clk_lock(void);
  void of_clk_unlock(void);
  #endif
 +
 +#if defined(CONFIG_COMMON_CLK)

 So we shouldn't need this one either.

Actually, i had to put it back so clkdev.c builds on
!CONFIG_COMMON_CLK. Do you have another idea on how to deal with this?

Sorry, but I forgot to CC you on v2 (just sent):
https://lkml.org/lkml/2014/10/7/430

Thanks,

Tomeu
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] clk: Make clk API return per-user struct clk instances

2014-10-07 Thread Stephen Boyd 

On 10/07/2014 08:28 AM, Tomeu Vizoso wrote:

On 4 October 2014 01:15, Stephen Boyd sb...@codeaurora.org wrote:

On 10/02, Tomeu Vizoso wrote:

+
  #if defined(CONFIG_OF)  defined(CONFIG_COMMON_CLK)

These ifdefs look useless.


  struct clk *of_clk_get_by_clkspec(struct of_phandle_args *clkspec);
  struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec);
  void of_clk_lock(void);
  void of_clk_unlock(void);
  #endif
+
+#if defined(CONFIG_COMMON_CLK)

So we shouldn't need this one either.

Actually, i had to put it back so clkdev.c builds on
!CONFIG_COMMON_CLK. Do you have another idea on how to deal with this?


What's the error? Probably need to add a forward declaration for struct 
clk, struct of_phandle_args, and struct clk_hw.


--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] clk: Make clk API return per-user struct clk instances

2014-10-06 Thread Tomeu Vizoso 
On 4 October 2014 01:15, Stephen Boyd sb...@codeaurora.org wrote:
 On 10/02, Tomeu Vizoso wrote:
 Moves clock state to struct clk_core, but takes care to change as little API 
 as
 possible.

 struct clk_hw still has a pointer to a struct clk, which is the
 implementation's per-user clk instance, for backwards compatibility.

 The struct clk that clk_get_parent() returns isn't owned by the caller, but 
 by
 the clock implementation, so the former shouldn't call clk_put() on it.

 Because some boards in mach-omap2 still register clocks statically, their 
 clock
 registration had to be updated to take into account that the clock 
 information
 is stored in struct clk_core now.

 Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com
 ---


 We should s/provider/core/ when we're dealing with clk_core
 structures in the function signature. The providers are hardware
 drivers and the core structures are for the framework, not the
 same. Furthermore, the provider drivers should only be dealing
 with clk_hw structures. The only place that clk_core should be in
 clk-provider.h is in struct clk_hw because there's no way to get
 around it.

 This way, provider drivers should only be including
 clk-provider.h because they only care about dealing with struct
 clk_hw. Consumers should only be including linux/clk.h where they
 only know about struct clk as an opaque pointer. Once we get OMAP
 to stop using clk-private.h we can kill off that header entirely
 (I see there are some other bogus users of that header outside of
 OMAP that we should nuke). Then the framework can include
 clk-provider.h and clk.h to map between the hw cookie and the
 consumer cookie.

Agreed.

 This is the end goal. I understand that the provider API is sort
 of a mess with us allowing drivers to use the underscore and
 non-underscore functions and the mixture of struct clk and struct
 ckl_hw throughout.

  struct clk_hw -- struct clk_core  struct clk
\- struct clk
|- struct clk

Agree this is how it should look like at some point, but for now I
need a reference to struct clk from struct clk_hw, so providers can
keep using the existing API. This reference would be removed once they
move to the new clk_hw-based API.

  providers
  -
  struct clk_hw {
 struct clk_core *
 ...
  };

  consumers
  -

  struct clk;

  hidden in core framework
  
  struct clk {
 struct clk_core *;
 ...
  }

  struct clk_core {
 struct clk_hw *;
 ...
  }



 diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
 index 4eeb8de..b216b13 100644
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -37,6 +37,13 @@ static HLIST_HEAD(clk_root_list);
  static HLIST_HEAD(clk_orphan_list);
  static LIST_HEAD(clk_notifier_list);

 +static void clk_provider_put(struct clk_core *clk);

 Does this need to be forward declared?

No, removed.

 +static long clk_provider_get_accuracy(struct clk_core *clk);
 +static bool clk_provider_is_prepared(struct clk_core *clk);
 +static bool clk_provider_is_enabled(struct clk_core *clk);
 +static long clk_provider_round_rate(struct clk_core *clk, unsigned long 
 rate);
 @@ -356,7 +363,7 @@ out:
   *
   * Caller must hold prepare_lock.
   */
 -static void clk_debug_unregister(struct clk *clk)
 +static void clk_debug_unregister(struct clk_core *clk)
  {
   debugfs_remove_recursive(clk-dentry);
  }
 @@ -366,8 +373,8 @@ struct dentry *clk_debugfs_add_file(struct clk *clk, 
 char *name, umode_t mode,

 We should pass struct clk_hw here instead of struct clk. Let's do
 it soon before we get any users.

Sounds good to me.

  {
   struct dentry *d = NULL;

 - if (clk-dentry)
 - d = debugfs_create_file(name, mode, clk-dentry, data, fops);
 + if (clk-core-dentry)
 + d = debugfs_create_file(name, mode, clk-core-dentry, data, 
 fops);

   return d;
  }
 @@ -545,53 +553,67 @@ late_initcall_sync(clk_disable_unused);

  const char *__clk_get_name(struct clk *clk)
  {
 - return !clk ? NULL : clk-name;
 + return !clk ? NULL : clk-core-name;
  }
  EXPORT_SYMBOL_GPL(__clk_get_name);

  struct clk_hw *__clk_get_hw(struct clk *clk)
  {
 - return !clk ? NULL : clk-hw;
 + return !clk ? NULL : clk-core-hw;
  }
  EXPORT_SYMBOL_GPL(__clk_get_hw);

  u8 __clk_get_num_parents(struct clk *clk)
  {
 - return !clk ? 0 : clk-num_parents;
 + return !clk ? 0 : clk-core-num_parents;
  }
  EXPORT_SYMBOL_GPL(__clk_get_num_parents);

  struct clk *__clk_get_parent(struct clk *clk)
  {
 - return !clk ? NULL : clk-parent;
 + /* TODO: Create a per-user clk and change callers to call clk_put */

 More like replace all callers with a function that returns their
 parent's hw pointer.

Sounds good, but I thought about it and have chosen to just remove the comment.

 struct clk_hw *clk_provider_get_parent(struct clk_hw *hw)


 + return

Re: [PATCH 1/2] clk: Make clk API return per-user struct clk instances

2014-10-06 Thread Stephen Boyd 

On 10/06/2014 10:14 AM, Tomeu Vizoso wrote:

This is the end goal. I understand that the provider API is sort
of a mess with us allowing drivers to use the underscore and
non-underscore functions and the mixture of struct clk and struct
ckl_hw throughout.

  struct clk_hw -- struct clk_core  struct clk
\- struct clk
|- struct clk

Agree this is how it should look like at some point, but for now I
need a reference to struct clk from struct clk_hw, so providers can
keep using the existing API. This reference would be removed once they
move to the new clk_hw-based API.


Ok sounds like we're on the same page.


+struct clk *__clk_create_clk(struct clk_core *clk_core, const char *dev_id,
+  const char *con_id);
+#endif
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index da4bda8..fe3712f 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -168,14 +172,27 @@ static struct clk_lookup *clk_find(const char *dev_id, 
const char *con_id)
  struct clk *clk_get_sys(const char *dev_id, const char *con_id)
  {
   struct clk_lookup *cl;
+ struct clk *clk = NULL;

   mutex_lock(clocks_mutex);
   cl = clk_find(dev_id, con_id);
- if (cl  !__clk_get(cl-clk))
- cl = NULL;
+ if (cl) {
+#if defined(CONFIG_COMMON_CLK)
+ clk = __clk_create_clk(cl-clk-core, dev_id, con_id);
+ if (clk  !__clk_get(clk)) {
+ kfree(clk);

This looks weird. It makes me suspect we've failed to reference
count something properly. Can we avoid this?

Can you extend on this? But I see how the behaviour doesn't match the
previous one because cl should be NULLed when __clk_get fails. I have
fixed this.


It triggers my that's not symmetric filter because it requires the 
caller to free something allocated by the callee. Do we still need 
__clk_get() to be called in the common clock path? Why not just do the 
stuff we do in __clk_get() in __clk_create_clk()? Then if that fails we 
can return an error pointer indicating some sort of failure (-ENOENT?) 
and we don't need to do any sort of cleanup otherwise.


--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] clk: Make clk API return per-user struct clk instances

2014-10-03 Thread Stephen Boyd 
On 10/02, Tomeu Vizoso wrote:
 Moves clock state to struct clk_core, but takes care to change as little API 
 as
 possible.
 
 struct clk_hw still has a pointer to a struct clk, which is the
 implementation's per-user clk instance, for backwards compatibility.
 
 The struct clk that clk_get_parent() returns isn't owned by the caller, but by
 the clock implementation, so the former shouldn't call clk_put() on it.
 
 Because some boards in mach-omap2 still register clocks statically, their 
 clock
 registration had to be updated to take into account that the clock information
 is stored in struct clk_core now.
 
 Signed-off-by: Tomeu Vizoso tomeu.viz...@collabora.com
 ---


We should s/provider/core/ when we're dealing with clk_core
structures in the function signature. The providers are hardware
drivers and the core structures are for the framework, not the
same. Furthermore, the provider drivers should only be dealing
with clk_hw structures. The only place that clk_core should be in
clk-provider.h is in struct clk_hw because there's no way to get
around it.

This way, provider drivers should only be including
clk-provider.h because they only care about dealing with struct
clk_hw. Consumers should only be including linux/clk.h where they
only know about struct clk as an opaque pointer. Once we get OMAP
to stop using clk-private.h we can kill off that header entirely
(I see there are some other bogus users of that header outside of
OMAP that we should nuke). Then the framework can include
clk-provider.h and clk.h to map between the hw cookie and the
consumer cookie.

This is the end goal. I understand that the provider API is sort
of a mess with us allowing drivers to use the underscore and
non-underscore functions and the mixture of struct clk and struct
ckl_hw throughout.

 struct clk_hw -- struct clk_core  struct clk
   \- struct clk
   |- struct clk

 providers
 -
 struct clk_hw {
struct clk_core *
...
 };

 consumers
 -

 struct clk;

 hidden in core framework
 
 struct clk {
struct clk_core *;
...
 }

 struct clk_core {
struct clk_hw *;
...
 }


 
 diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
 index 4eeb8de..b216b13 100644
 --- a/drivers/clk/clk.c
 +++ b/drivers/clk/clk.c
 @@ -37,6 +37,13 @@ static HLIST_HEAD(clk_root_list);
  static HLIST_HEAD(clk_orphan_list);
  static LIST_HEAD(clk_notifier_list);
  
 +static void clk_provider_put(struct clk_core *clk);

Does this need to be forward declared?

 +static long clk_provider_get_accuracy(struct clk_core *clk);
 +static bool clk_provider_is_prepared(struct clk_core *clk);
 +static bool clk_provider_is_enabled(struct clk_core *clk);
 +static long clk_provider_round_rate(struct clk_core *clk, unsigned long 
 rate);
 @@ -356,7 +363,7 @@ out:
   *
   * Caller must hold prepare_lock.
   */
 -static void clk_debug_unregister(struct clk *clk)
 +static void clk_debug_unregister(struct clk_core *clk)
  {
   debugfs_remove_recursive(clk-dentry);
  }
 @@ -366,8 +373,8 @@ struct dentry *clk_debugfs_add_file(struct clk *clk, char 
 *name, umode_t mode,

We should pass struct clk_hw here instead of struct clk. Let's do
it soon before we get any users.

  {
   struct dentry *d = NULL;
  
 - if (clk-dentry)
 - d = debugfs_create_file(name, mode, clk-dentry, data, fops);
 + if (clk-core-dentry)
 + d = debugfs_create_file(name, mode, clk-core-dentry, data, 
 fops);
  
   return d;
  }
 @@ -545,53 +553,67 @@ late_initcall_sync(clk_disable_unused);
  
  const char *__clk_get_name(struct clk *clk)
  {
 - return !clk ? NULL : clk-name;
 + return !clk ? NULL : clk-core-name;
  }
  EXPORT_SYMBOL_GPL(__clk_get_name);
  
  struct clk_hw *__clk_get_hw(struct clk *clk)
  {
 - return !clk ? NULL : clk-hw;
 + return !clk ? NULL : clk-core-hw;
  }
  EXPORT_SYMBOL_GPL(__clk_get_hw);
  
  u8 __clk_get_num_parents(struct clk *clk)
  {
 - return !clk ? 0 : clk-num_parents;
 + return !clk ? 0 : clk-core-num_parents;
  }
  EXPORT_SYMBOL_GPL(__clk_get_num_parents);
  
  struct clk *__clk_get_parent(struct clk *clk)
  {
 - return !clk ? NULL : clk-parent;
 + /* TODO: Create a per-user clk and change callers to call clk_put */

More like replace all callers with a function that returns their
parent's hw pointer.

struct clk_hw *clk_provider_get_parent(struct clk_hw *hw)


 + return !clk ? NULL : clk-core-parent-hw-clk;
  }
  EXPORT_SYMBOL_GPL(__clk_get_parent);
  
 -struct clk *clk_get_parent_by_index(struct clk *clk, u8 index)
 +static struct clk_core *clk_provider_get_parent_by_index(struct clk_core 
 *clk,
 +  u8 index)
  {
   if (!clk || index = clk-num_parents)
   return NULL;
   else if (!clk-parents)
 - return __clk_lookup(clk-parent_names[index]);
 +