Re: [PATCH] Topcliff PHUB: Generate PacketHub driver

[Yong Wang <yong.y.wang@linux.intel.com>]

On Tue, Jun 08, 2010 at 02:00:55PM +0900, Masayuki Ohtak wrote:
> Hi Alan
> 
> We are now updating our Phub driver.
> 
> I have a questions for your comment.
> 
> (1)
> >> +#ifdef PCH_CAN_PCLK_50MHZ
> >> + /* save clk cfg register */
> >> + g_pch_phub_reg.clkcfg_reg = PCH_READ_REG(CLKCFG_REG_OFFSET);
> >> +#endif
> > 
> > This makes it hard to build one kernel for everything
> We couldn't understand your intention.
> Does the above mean we must not use "#ifdef PCH_CAN_PCLK_50MHZ"	in source code ?
> 

I think what Alan means is that you will have to build two kernel images
for two possible configurations of how your hardware is going to be used
if you write code this way. One is the case when CAN clock runs at 50MHz
and you will build a kernel image with PCH_CAN_PCLK_50MHZ defined. The
other is when CAN clock runs at other speed and you need to build
another kernel image with PCH_CAN_PCLK_50MHZ undefined. It would be much
better if one kernel image could run on all configurations.

Why is 50MHz so special, btw? Don't you need to save and restore the
clock config register when CAN clock runs at other speed?

> 
> >> +#define PCH_READ_REG(a) ioread32((pch_phub_base_address + a))
> 
> >> +
> 
> >> +#define PCH_WRITE_REG(a, b) iowrite32(a, (pch_phub_base_address + b))
> 
> > 
> 
> > These on the other hand do - but not where they are now
> 
> > 
> 
> > iowrite32((a), pcb_phub_base_address + (b))
> 
> > 
> 
> > (so that if a or b are expressions they are evaluated first)
> 
> Modify like below.
> 
> #define PCH_READ_REG(a) ioread32(pch_phub_base_address + (a))
> 
> #define PCH_WRITE_REG(a, b) iowrite32(a, pch_phub_base_address + (b))
> 

Note that 'a' is supposed to be surrounded by brackets, too.

#define PCH_WRITE_REG(a, b)	iowrite32((a), pcb_phub_base_address + (b))

Thanks
-Yong