Re: [PATCH RFC] sta2x11 common clock framework implementation

[Davide Ciminaghi <ciminaghi@gnudd.com>]

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