Messages in this thread | | | Date | Fri, 7 Sep 2012 11:28:34 +0200 | From | Davide Ciminaghi <> | Subject | Re: [PATCH RFC] sta2x11 common clock framework implementation |
| |
On Thu, Sep 06, 2012 at 04:32:14PM -0700, Mike Turquette wrote: > Quoting Davide Ciminaghi (2012-08-27 08:03:40) > > On Thu, Aug 23, 2012 at 06:47:19PM -0700, Mike Turquette wrote: > > > Yikes. Is this code copied from a legacy clock framework > > > implementation? Since we have the nice struct clk_hw abstraction now I > > > think it is worth considering breaking up struct sta2x11_clk_data into > > > separate structs, one for each clock types. This lets you get rid of > > > the union and the .type member while keeping things nice and tidy and > > > reducing the number of run-time checks. > > > > > > > mmmm, not sure I have fully understood your comment here. > > My basic idea was avoiding a huge list of clk_register_xxxyyy() calls by > > using an array with one entry per clock and a for cycle. We then walk through > > the array and call an init() function which does a runtime initialization of > > the relevant table entry (by adding data such as pci base addresses, which are > > unknown at compile time). Finally a registration function is invoked, which > > actually registers each clock. > > We have one registration function per clock type, and the .type field is > > needed to invoke the right registration function for each entry. > > Then of course we need just one struct type to get an array. I > > solved this by declaring a struct sta2x11_clk_data with some common fields > > and an args union containing "private" data for each clock type. > > Intermediate register functions (do_register_xxxyyy()) are needed > > because the clk_register_xxxyyy() functions have different prototypes for > > each xxxyyy. > > > > I did not read the code carefully enough the first time through and I > think I misunderstood the purpose of the struct. > ok.
> > Would it be ok to have something like this: > > > > > > struct sta2x11_clk_data { > > const char *basename; > > unsigned int reg_offset; > > struct clk *(*register)(struct sta2x11_clk_data *, const char *, int); > > void (*init)(struct sta2x11_clk_data *, struct sta2x11_clk_reg_data *); > > unsigned long flags; > > void *priv; > > }; > > > > ? > > > > with .priv pointing to a different struct for each clock type (for instance > > struct fixed_rate_root_priv_data for a fixed rate clock), as you suggested. > > > > Note that the .type field has also been replaced by a .register function > > pointer. This would let us avoid the regfuncs[] table and make things more > > symmetric (initialization would just work like registration). Well, I was > > actually planning to use the .type field for disabling unimplemented clocks > > on some of the supported boards (by setting their .type to "none"), but we > > could do this by setting the init and/or register pointers to NULL, so that the > > relevant array entry is skipped. > > This new approach would require separate tables for each clock's private data, > > instead of a single table containing everything is needed for registration, > > but if it is ok for you, I'm fine with it. > > > > That is up to you. I'm OK with your original implementation, or the new > suggested one.
Well, what I proposed could be somehow better, but the first version has the advantage of being ready and tested :-) . I have to think about this a little bit.
> Sorry for the noise.
ok no problem.
> The other review comments from me still stand (especially spin_lock versus > spin_lock_irqsave). Will you be resubmitting the patch with the minor fixes?
yes of course. While waiting for your comments, I have been working on a device tree implementation for our boards, so I will probably also submit an initial devicetree support for clocks. I also need some patches of mine on sta2x11-mfd to be accepted before the clock infrastructure can compile on linux-next, I hope that will happen shortly (by the way, that's the main reason why the patch was RFC only).
Thanks again and regards Davide
| |