[lkml]   [2012]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] clk: Use a separate struct for holding init data.
    On 20120501-21:42, Saravana Kannan wrote:
    > On 05/01/2012 07:04 PM, Mike Turquette wrote:
    > >1) I'm surprised that you abandoned the approach of exposing the
    > >less-private members of struct clk via struct clk_hw. Your original
    > >patch did just that, but did not account for static initialization.
    > >This patch seems to have gone in the opposite direction and only
    > >accounts for static initialization.
    > I think there might be some misunderstanding on what can/can't be
    > done with my patch. Or may be I'm not understanding your question.

    I believe I am reading the code correctly. I'll try reiterating below.

    > I used to expose the "shared" info through clk_hw. I just put them
    > in a struct and make clk_hw point to it. This would allow for easily
    > marking this shared info as __init data.

    So platforms must choose between marking clk->hw->init as __initdata, OR
    they can keep it around and reference it later from clock code that
    includes clk-provder.h. Is this a fair statement?

    > It would have a been a pain
    > to do (or not even possible) if I had put the fields directly into
    > clk_hw.

    Not true. Combining Sascha's original static initializer idea with your
    struct clk_hw patch would be easy. A separate struct for static init
    would be marked as __initdata. When passed into a registration function
    that data would be copied to fields in struct clk_hw. You've done this
    exact same thing, with the exception of copying the data to struct clk

    It is not a big deal, and I'm fine with the direction this patch has
    taken, but I wanted to point it out. If you look at Russell's and
    Sascha's replies in this thread you'll see that they regard
    clk->hw->init as purely __initdata, going so far as to mark it NULL in
    clk_register. This sort of change at the framework level would
    eliminate the ability for your clock code to reference these members

    > I'm not sure why you say my patch only accounts for static
    > initialization. The entire clk specific struct (say, struct
    > fixed_clk), the clk_init_data can be dynamically allocated and
    > registered using clk_register.

    This was a miscommunication on my part and can be disregarded. Of
    course your patch allows for dynamic registration.

    My point was that of the two previous approaches on this list (Sascha's
    static initializers and your own struct clk_hw modifications), this
    patch only represents the functionality of the former. I should have
    been more clear.

    > >2) I did make a modification to your patch where I kept the
    > >DEFINE_CLK_* macros and continued to declare __clk_init in
    > >clk-private.h. I do want to get rid of both of these in the future but
    > >currently my platform relies on static initialization before the
    > >allocator is available. Please let me know if this causes a problem for
    > >you.
    > I definitely had your requirements in mind too when I made the changes.
    > You really shouldn't need __clk_init. That's why I added
    > __clk_register.

    I completely missed __clk_register. I'm not sure I see the point of it
    however. struct clk is still exposed to folks that are using this new
    function; compared to simply calling __clk_init it has added a few loads
    & stores. Regardless these interfaces will hopefully die off completely
    once OMAP's clock code uses an initcall.

    > >Platform folks should rebase on top of this if needed. This should be
    > >the last change to the driver/platform-facing API before 3.5.
    > I really wish we discussed your changes before it was made, pulled
    > in and announced since clk_init isn't really needed. But since you
    > just added more APIs and didn't remove the ones I added, I guess
    > it's not very bad.

    I probably announced it too soon, but everything you added to the API is
    still there, just with some extra stuff that should go away in the

    > Since people were already frustrated with the API change I made at
    > this point, can we recommend people to not use __clk_init() when
    > sending patches for your clk-next? And make it static after the next
    > kernel release?

    Yes, I completely agree. It would be good to get rid of __clk_register
    and __clk_init down the road. Marking the interfaces as deprecated is
    one solution; however I agree with your suggestion and just catching it
    in review is probably the best route.


     \ /
      Last update: 2012-05-02 21:41    [W:0.029 / U:14.916 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site