On Mon, Mar 11, 2013 at 01:28:09PM -0500, Josh Cartwright wrote:
> On Mon, Mar 11, 2013 at 10:15:04AM +0100, Steffen Trumtrar wrote:
> > Signed-off-by: Steffen Trumtrar <s.trumt...@pengutronix.de>
> > ---
> > arch/arm/mach-zynq/clk-zynq7000.c | 33 ++++++++++++++++++++++++++++++++-
> > 1 file changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/mach-zynq/clk-zynq7000.c
> > b/arch/arm/mach-zynq/clk-zynq7000.c
> > index 5a8a12a..0d3c3a8 100644
> > --- a/arch/arm/mach-zynq/clk-zynq7000.c
> > +++ b/arch/arm/mach-zynq/clk-zynq7000.c
> > @@ -33,10 +33,21 @@ enum zynq_clks {
> > cpu_clk, cpu_6x4x, cpu_3x2x, cpu_2x, cpu_1x, clks_max
> > };
> >
> > +enum zynq_pll_type {
> > + ZYNQ_PLL_ARM,
> > + ZYNQ_PLL_DDR,
> > + ZYNQ_PLL_IO,
> > +};
> > +
> > +#define PLL_ARM_LOCK (1 << 0)
> > +#define PLL_DDR_LOCK (1 << 1)
> > +#define PLL_IO_LOCK (1 << 2)
>
> Having both an enum and the #define's seem like an unnecessary
> indirection. I'd suggest just:
>
> enum zynq_pll_lockbit {
> PLL_ARM_LOCK = (1 << 0),
> PLL_DDR_LOCK = (1 << 1),
> PLL_IO_LOCK = (1 << 2),
> };
>
> struct zynq_pll_clk {
> /* ... */
> enum zynq_pll_lockbit lockbit;
> };
>
> static inline struct clk *zynq_pll_clk(enum zynq_pll_lockbit lockbit,
> const char *name,
> void __iomem *pll_ctrl)
> {
> /* ... */
> pll->lockbit = lockbit;
> /* ... */
> }
>
> > +
> > static struct clk *clks[clks_max];
> >
> > struct zynq_pll_clk {
> > struct clk clk;
> > + u32 pll_lock;
> > void __iomem *pll_ctrl;
> > };
> >
> > @@ -51,11 +62,19 @@ static unsigned long zynq_pll_recalc_rate(struct clk
> > *clk,
> > return parent_rate * PLL_CTRL_FDIV(readl(pll->pll_ctrl));
> > }
> >
> > +static int zynq_pll_enable(struct clk *clk)
> > +{
> > + return 0;
> > +}
> > +
> > static struct clk_ops zynq_pll_clk_ops = {
> > .recalc_rate = zynq_pll_recalc_rate,
> > + .enable = zynq_pll_enable,
> > };
> >
> > -static inline struct clk *zynq_pll_clk(const char *name, void __iomem
> > *pll_ctrl)
> > +static inline struct clk *zynq_pll_clk(enum zynq_pll_type type,
> > + const char *name,
> > + void __iomem *pll_ctrl)
> > {
> > static const char *pll_parent = "ps_clk";
> > struct zynq_pll_clk *pll;
> > @@ -68,6 +87,18 @@ static inline struct clk *zynq_pll_clk(const char *name,
> > void __iomem *pll_ctrl)
> > pll->clk.parent_names = &pll_parent;
> > pll->clk.num_parents = 1;
> >
> > + switch(type) {
> > + case ZYNQ_PLL_ARM:
> > + pll->pll_lock = PLL_ARM_LOCK;
> > + break;
> > + case ZYNQ_PLL_DDR:
> > + pll->pll_lock = PLL_DDR_LOCK;
> > + break;
> > + case ZYNQ_PLL_IO:
> > + pll->pll_lock = PLL_IO_LOCK;
>
> Actually, maybe I've gotten a little ahead of myself...you add bits for
> the lock, but you never use it! So, what's the point! (If it's to be
> used in the future, it'd be nice to see that in the commit description).
I will remove this from this series. This only makes sense, when
the enable function is filled.
str
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
