lkml.org 
[lkml]   [2010]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC,PATCH 1/2] Add a common struct clk
    On Thu, Jun 03, 2010 at 11:21:19AM +0800, Jeremy Kerr wrote:
    > Hi Ben,
    >
    > > > And a set of clock operations (defined per type of clock):
    > > >
    > > > struct clk_operations {
    > > >
    > > > int (*enable)(struct clk *);
    > >
    > > I'd rather the enable/disable calls where simply a set
    > > and a bool on/off, very rarelyt is the enable and disable
    > > operartions different.
    >
    > I thought about merging these, but decided against it. It does work for the
    > simple case where we're setting a bit in a register:
    >
    > static int clk_foo_set_state(struct clk *_clk, int enable)
    > {
    > struct clk_foo *clk = to_clk_foo(_clk)
    > u32 reg;
    >
    > reg = raw_readl(foo->some_register);
    > if (enable)
    > reg |= FOO_ENABLE;
    > else
    > reg &= ~FOO_ENABLE;
    > raw_writel(foo->some_register, reg);
    >
    > return 0;
    > }
    >
    > However, for anything more complex than this - for example, if there's a
    > parent clock - then we start getting pretty messy:
    >
    > static int clk_foo_set_state(struct clk *_clk, int enable)
    > {
    > struct clk_foo *clk = to_clk_foo(_clk)
    > u32 reg;

    Yuck. I think this should really be handled by the base clk_enable()
    and clk_disable() calls. Roughly based on what is currently in the
    plat-samsung clock implementation:

    clk_enable(struct clk *clk)
    {
    if (clk->parent)
    clk_enable(clk->parent)
    ...
    }

    clk_disable(struct clk *clk)
    {
    ...
    if (clk->parent)
    clk_disable(clk->parent)
    }

    I think it is a really bad idea for each implementation to have to worry
    about this. It sounds like a recipie for people to get wrong, especially
    if we have a number of these implementations kicking around.

    > if (enable) {
    > int ret = clk_enable(clk->parent);
    > if (ret)
    > return ret;
    > }
    >
    > reg = raw_readl(foo->some_register);
    > if (enable)
    > reg |= FOO_ENABLE;
    > else
    > reg &= ~FOO_ENABLE;
    >
    > raw_writel(foo->some_register, reg);
    >
    > if (!enable)
    > clk_disable(clk->parent);
    >
    > return 0;
    > }
    >
    > - where most of the function becomes surrounded by "if (enable)" statements.
    >
    > I'm aware that we can turn this into a conditional call of clk_foo_enable or
    > clk_foo_disable, but then we're back to square 1. I also think that the simple
    > case is clearer (if a little more verbose) with separate functions.

    If we do decided to move the parent control functionality to the clock
    core, then I would prefer to see the change to a single enable/disable
    callback. Especially as it fits my current implementations well.

    As a note, I also left the enable callback in the 'struct clk' instead
    of in the ops, enable/disable is the most used case of these clock
    functions, and as such should probably be the easiest to get to.

    Also, wheras plat-samsung has very few sets of clk_ops sitting about,
    there are more enable/disable calls, and adding more fields to the
    clocks to deal with this would add extra space to the kernel.

    > Also, enable and disable in the external clock API have different return
    > types.

    does that really matter?

    > > an aside, you might want to just clal these clk_ops to get into the
    > > spirit of the original naming.
    >
    > Either is fine with me - looks like 'ops' is more commonly used:

    My pref. is for less typing.

    > $ git grep -E '^struct \w*operations\s*\{' include/ | wc -l
    > 30
    >
    > $ git grep -E '^struct \w*ops\s*{' include/ | wc -l
    > 138
    >
    > Cheers,
    >
    >
    > Jeremy

    --
    --
    Ben

    Q: What's a light-year?
    A: One-third less calories than a regular year.



    \
     
     \ /
      Last update: 2010-06-03 10:17    [W:2.510 / U:0.284 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site