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.

quoted

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