Re: [PATCH 7/9] ARM: zynq: clk: add pll type

From: Steffen Trumtrar <s.trumtrar@pengutronix.de>
To: Josh Cartwright <joshc@eso.teric.us>
Cc: barebox@lists.infradead.org
Subject: Re: [PATCH 7/9] ARM: zynq: clk: add pll type
Date: Mon, 11 Mar 2013 19:59:52 +0100	[thread overview]
Message-ID: <20130311185952.GB22366@pengutronix.de> (raw)
In-Reply-To: <20130311182809.GN16050@kryptos>

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.trumtrar@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