On 01/09, Mike Turquette wrote:
> If we're going to use these wrappers, why make it regmap specific? The
> struct clk_desc patches[1][2] can achieve this, but in a more generic
> way.
>
I think you're suggesting a way to avoid adding a
clk_register_regmap() function? But won't we need to write the
same code:
if (dev && dev_get_regmap(dev, NULL))
[clk_type]->regmap = dev_get_regmap(dev, NULL);
else if (dev && dev->parent)
[clk_type]->regmap = dev_get_regmap(dev->parent, NULL);
everytime we want to assign the regmap pointer to a different clock type?
A macro might work for this little snippet, but it wouldn't have
any type safety.
> >
> >
> > 2) Interfaces: Add a void *data in struct clk_hw that can point to
> > whatever I want and still have the same clk_regmap_register() and
> > devm_clk_regmap_register()
> >
> > Example:
> >
> > struct clk_hw {
> > struct clk *clk;
> > const struct clk_init_data *init;
> > void *data;
> > };
> >
> > struct clk_regmap {
> > struct regmap *regmap;
> > unsigned int enable_reg;
> > unsigned int enable_mask;
> > bool enable_is_inverted;
> > };
> >
> > struct clk_branch {
> > u32 hwcg_reg;
> > u32 halt_reg;
> > u8 hwcg_bit;
> > u8 halt_bit;
> > u8 halt_check;
> >
> > struct clk_hw;
> > };
> >
> > static struct clk_branch gsbi1_uart_clk = {
> > .halt_reg = 0x2fcc,
> > .halt_bit = 10,
> > .hw = {
> > .data = &(struct clk_regmap){
> > .enable_reg = 0x29d4,
> > .enable_mask = BIT(9),
> > };
> > .init = &(struct clk_init_data){
> > .name = "gsbi1_uart_clk",
> > .parent_names = (const char *[]){
> > "gsbi1_uart_src",
> > },
> > .num_parents = 1,
> > .ops = &clk_branch_ops,
> > .flags = CLK_SET_RATE_PARENT,
> > },
> > },
> > };
> >
> > I guess option 2 is less likely given your comment about clk_hw being
> > nothing more than a traversal mechanism.
>
> Instead of private data, how about a .register() callback function that
> can point to anything you like? The clk_desc patches implement this and
> it would suffice for registering regmap ops or anything else, without
> polluting struct clk_hw.
>
> [1] http://www.spinics.net/lists/linux-omap/msg101822.html
> [2] http://www.spinics.net/lists/linux-omap/msg101698.html
>
> So you could statically define gsbi1_uart_clk with:
>
> static struct clk_branch_desc gsbi1_uart_clk_desc = {
> .halt_reg = 0x2fcc,
> .halt_bit = 10,
> .enable_reg = 0x29d4,
> .enable_mask = BIT(9),
> .desc = {
> .name = "gsbi1_uart_clk",
> .parent_names = (const char *[]){
> "gsbi1_uart_src",
> },
> .num_parents = 1,
> .ops = &clk_branch_ops,
> .flags = CLK_SET_RATE_PARENT,
> },
> };
>
> And then register it with:
>
> clk_register_desc(NULL, &gsbi1_uart_clk_desc.desc);
>
> This is very analogous to the way that you use use &gsbi1_uart_clk.hw
> but it is more generic and also doesn't pollute clk_hw any further. I
> also think your static data is quite a bit prettier using this method.
>
> Thoughts?
Is the plan to allocate a struct clk_branch at runtime and then
copy all the fields over one by one? I'm trying to avoid that
because it takes more time and more runtime memory. If I had to
go the descriptor route I would probably avoid copying any fields
and just point to the descriptor from struct clk_branch, i.e.
struct clk_branch {
struct clk_branch_desc *desc;
struct clk_hw;
};
but that still seems wasteful to allocate a bunch of little
pointer wrappers when I could have just embedded the clk_hw
struct inside the clk_branch struct from the start.
It feels another key point is being missed though. The regmap
pointer and the enable_reg/enable_mask is embedded in clk_hw to
allow the same code to be used by different types of surrounding
structs. Each struct: clk_pll, clk_rcg, and clk_branch in this
series use the regmap interface to enable/disable the clock and
they can easily do so by passing something that's always
available from struct clk_hw (be it via a wrapper struct, private
data member, or addition of new fields to clk_hw). If the regmap
members move into each specific type of clock we can't just pass
a single pointer to the enable/disable regmap functions anymore.
This is the reason why I suggested a driver data pointer or
container struct so that everything regmap related is contained
within one type.
--
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-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
