lkml.org 
[lkml]   [2012]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v5 3/4] clk: introduce the common clock framework
    On Sat, Mar 03, 2012 at 09:14:43AM -0800, Turquette, Mike wrote:
    > On Sat, Mar 3, 2012 at 5:31 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
    > > On Sat, Mar 03, 2012 at 12:29:00AM -0800, Mike Turquette wrote:
    > >> The common clock framework defines a common struct clk useful across
    > >> most platforms as well as an implementation of the clk api that drivers
    > >> can use safely for managing clocks.
    > >>
    > >> The net result is consolidation of many different struct clk definitions
    > >> and platform-specific clock framework implementations.
    > >>
    > >> This patch introduces the common struct clk, struct clk_ops and an
    > >> implementation of the well-known clock api in include/clk/clk.h.
    > >> Platforms may define their own hardware-specific clock structure and
    > >> their own clock operation callbacks, so long as it wraps an instance of
    > >> struct clk_hw.
    > >>
    > >> See Documentation/clk.txt for more details.
    > >>
    > >> This patch is based on the work of Jeremy Kerr, which in turn was based
    > >> on the work of Ben Herrenschmidt.
    > >>
    > >> +
    > >> +/**
    > >> + * struct clk_hw - handle for traversing from a struct clk to its corresponding
    > >> + * hardware-specific structure.  struct clk_hw should be declared within struct
    > >> + * clk_foo and then referenced by the struct clk instance that uses struct
    > >> + * clk_foo's clk_ops
    > >> + *
    > >> + * clk: pointer to the struct clk instance that points back to this struct
    > >> + * clk_hw instance
    > >> + */
    > >> +struct clk_hw {
    > >> +     struct clk *clk;
    > >> +};
    > >
    > > The reason for doing this is that struct clk should be an opaque cookie
    > > for both drivers and implementers of clocks. I recently had the idea whether
    > > the roles of these two structs could be swapped. So instead of the above we
    > > could do:
    > >
    > > struct clk {
    > >        struct clk_hw *hw;
    > > }
    >
    > Firstly, struct clk is an opaque cookie for both drivers and
    > implementers of clocks with this patchset.
    >
    > Secondly, struct clk does indeed have a pointer to struct clk_hw.
    > Refer to include/linux/clk-private.h in this patch.
    >
    > The reference is cyclical. A reference to struct clk can navigate to
    > struct clk_foo via container_of (usually something like "#define
    > to_clk_foo(_hw) container_of(_hw, struct clk_foo, hw)" where struct
    > clk's pointer to it's .hw member is passed into one of the struct
    > clk_ops callbacks.
    >
    > Likewise if struct clk_foo needs the struct clk ptr for any reason
    > then it can get it from foo->hw->clk.
    >
    > I believe this patch already does what you suggest, but I might be
    > missing your point.

    In include/linux/clk-private.h you expose struct clk outside the core.
    This has to be done to make static initializers possible. There is a big
    warning in this file that it must not be included from files implementing
    struct clk_ops. You can simply avoid this warning by declaring struct clk
    with only a single member:

    include/linux/clk.h:

    struct clk {
    struct clk_internal *internal;
    };
    This way everybody knows struct clk (thus can embed it in their static
    initializers), but doesn't know anything about the internal members. Now
    in drivers/clk/clk.c you declare struct clk_internal exactly like struct
    clk was declared before:

    struct clk_internal {
    const char *name;
    const struct clk_ops *ops;
    struct clk_hw *hw;
    struct clk *parent;
    char **parent_names;
    struct clk **parents;
    u8 num_parents;
    unsigned long rate;
    unsigned long flags;
    unsigned int enable_count;
    unsigned int prepare_count;
    struct hlist_head children;
    struct hlist_node child_node;
    unsigned int notifier_count;
    #ifdef CONFIG_COMMON_CLK_DEBUG
    struct dentry *dentry;
    #endif
    };
    An instance of struct clk_internal will be allocated in
    __clk_init/clk_register. Now the private data stays completely inside
    the core and noone can abuse it.

    With this __clk_init could be something like:

    struct clk_initializer {
    const char *name;
    const struct clk_ops *ops;
    char **parent_names;
    u8 num_parents;
    unsigned long flags;
    struct clk *clk;
    };
    void __clk_init(struct device *dev, struct clk_initializer *init);

    I hope I made my intention a bit clearer.

    Sascha

    --
    Pengutronix e.K. | |
    Industrial Linux Solutions | http://www.pengutronix.de/ |
    Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
    Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2012-03-04 12:55    [from the cache]
    ©2003-2014 Jasper Spaans. Advertise on this site