Messages in this thread | | | Date | Tue, 8 Jun 2010 15:20:49 +0800 | From | Yong Wang <> | Subject | Re: [PATCH] Topcliff PHUB: Generate PacketHub driver |
| |
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
| |