lkml.org 
[lkml]   [2012]   [Sep]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2012-09-07 12:02    [W:0.088 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site