Re: [RFC v2 3/5] clk: use struct clk only for external API

From: Tomeu Vizoso
Date: Thu Jul 10 2014 - 05:14:14 EST

On 07/09/2014 09:27 PM, Tomasz Figa wrote:
Hi Tomeu, Rabin,

Please see my comments inline.

On 03.07.2014 16:38, Tomeu Vizoso wrote:
From: Rabin Vincent <rabin.vincent@xxxxxxxxxxxxxx>

In order to provide per-user accounting, this separates the struct clk
used in the common clock framework into two structures 'struct clk_core'
and 'struct clk'. struct clk_core will be used for internal
manipulation and struct clk will be used in the clock API
implementation.

In this patch, struct clk is simply renamed to struct clk_core and a new
struct clk is implemented which simply wraps it. In the next patch, the
new struct clk will be used to implement per-user clock enable
accounting.


Hmm, shouldn't have Rabin signed off this patch as well?

Probably, but I have changed his patches substantially and I'm not really working with him on this (though I appreciate his involvement in the discussion). I'm going to put myself as Author: in the next version, and just mention that I based it off his previous work.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
---
drivers/clk/clk-devres.c | 13 +-
drivers/clk/clk.c | 539 ++++++++++++++++++++++++-------------------
drivers/clk/clk.h | 4 +
drivers/clk/clkdev.c | 90 +++++---
include/linux/clk-private.h | 38 +--
include/linux/clk-provider.h | 127 +++++-----
include/linux/clk.h | 21 +-
include/linux/clkdev.h | 24 +-
8 files changed, 494 insertions(+), 362 deletions(-)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 8f57154..5f2aba9 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -5,6 +5,7 @@
*/

#include <linux/clk.h>
+#include <linux/clk-provider.h>
#include <linux/device.h>
#include <linux/export.h>
#include <linux/gfp.h>
@@ -14,15 +15,15 @@ static void devm_clk_release(struct device *dev, void *res)
clk_put(*(struct clk **)res);
}

-struct clk *devm_clk_get(struct device *dev, const char *id)
+struct clk_core *devm_clk_provider_get(struct device *dev, const char *id)

nit: I'm not sure if name of the internal struct has been discussed
already, but clk_core sounds a bit off to me. Maybe it's just me but it
looks like a big common structure of the whole clock core not some small
per-clock structure.

As for suggestions of alternative names that come to my mind:
clk_object, clk_data, clk_entity.

Yeah, I'm not completely happy myself with clk_core or with either of those. But I like that the new API is prefixed with clk_provider_ as it clearly indicates that it's to be used by providers and not by consumers. Unfortunately, struct clk_provider would be misleading, so I'm not really sure of what would be a good name for struct clk_core.

Probably the least bad suggestion I have heard of is to have struct clk_internal and clk_internal_get_rate, instead of struct clk_core and clk_provider_get_rate.

Wonder if there are more opinions on this.

Thanks,

Tomeu

{
- struct clk **ptr, *clk;
+ struct clk_core **ptr, *clk;

ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
if (!ptr)
return ERR_PTR(-ENOMEM);

[snip]


+/*
+ * To avoid a mass-rename of all non-common clock implementations (spread out
+ * in arch-specific code), we let them use struct clk for both the internal and
+ * external view.
+ */
+#ifdef CONFIG_COMMON_CLK
+#define clk_core_t struct clk_core
+#else
+#define clk_core_t struct clk
+#endif

Hmm. I have bad feelings about this making some typedef lovers overuse
this macro to save few characters by not typing "struct" in code that
doesn't have to use this macro. But clearly I can't think of any better
solution right now, so don't consider this a showstopper.

Otherwise looks good to me.

Best regards,
Tomasz


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/