Re: Clock framework implementation for TI H4 OMAP2420 development board

["Juha Keski-Saari" <juha.keski-saari%teleca.com@localhost>]

Matt Thomas wrote:
> It's a good start.  But it's way too omap2 specific at this point.
> 
> The implicit ordered in clock_index[] is prone to breakage.  It
> shouldn't exist but be dynamically created as clocks are made known
> to the system.
> 
> instead of parent being a pointer to another clk, it should be
> a const char * containing the name of the parent.
> 
> The hierarchy of the clocks should be hidden behind an API:
> 
> int clk_add(struct clk *);
> int clk_remove(struct clk *);
> struct clk *clk_parent(struct clk *);
> struct clk *clk_child(struct clk *);
> struct clk *clk_next(struct clk *);
> 
> How this hierarchy is maintained behind the API is up to the
> implementation.

Thank you for the review Matt, the issues raised were good food for
thought for us. We agree on most part, the coding and naming
conventions have been corrected and we found a draft division for the
data structures. We propose the following, is this similar to what you
had in mind?

omap2_clock.h:
struct omap2_clock {
        struct clock       clock_mi;
        struct omap2_clock *parent;
        uint32_t           rate;
        uint32_t           flags;
     ...
        ... Additional MD data, addresses ...
     ...
     void               (*recalc)(struct clock *);
};

clk.h:
struct clock {
        const char       *name;

        struct clock_ops *ops;
};

struct clock_ops {
        int  (*enable)(struct clock *, int);
        void (*disable)(struct clock *, int);
        int  (*set_rate)(struct clock *, uint32_t);
        int  (*get_rate)(struct clock *);
        void (*get_status)(struct clock *, struct clkstatus *);
        void (*force_onoff)(struct clock *, int);
        void (*set_level)(struct clock *, int);
};


We found the concept of the add&remove registering along with moving
more functionality to the MI layer still floating without assurance
from actual hardware configuration specifics, if it would still be best
to leave the handling of the tree to the MD layer and keep the MI as
thin as still serves the purpose. In the end there would still be an
array of configured clocks which can be parsed for correctness if the
possibility of a misconfiguration messing things up is a major issue.
In your proposal I understand the MD portion would have the
configuration which would by appropriate means be parsed by the MI
portion into a valid configuration regardless of original order.

The quality improvements from the conventions are sound, and the
improved division in the header structure is a step in the right
direction. If you still feel the hierarchy definition should be done in
a better way, maybe we can flesh that idea out a bit better so we can
both see the effect on the final framework structure

Best regards,
Juha Keski-Saari