Hi Pavel, On Mon, 2014-03-03 at 21:41 +0100, ZY - pavel wrote: > Hi! > > > Scan Manager driver will be called to configure the IOCSR > > scan chain. This configuration will setup the IO buffer settings > > > > Signed-off-by: Chin Liang See <cl...@altera.com> > > Cc: Dinh Nguyen <dingu...@altera.com> > > Cc: Wolfgang Denk <w...@denx.de> > > CC: Pavel Machek <pa...@denx.de> > > Cc: Tom Rini <tr...@ti.com> > > Cc: Albert Aribaud <albert.u.b...@aribaud.net> > > > +static inline uint32_t scan_chain_engine_is_idle(uint32_t max_iter) > > +{ > > + uint32_t scanmgr_status; > > + > > + scanmgr_status = readl(&scan_manager_base->stat); > > + > > + /* Poll the engine until the scan engine is inactive */ > > + while (SCANMGR_STAT_ACTIVE_GET(scanmgr_status) || > > + (SCANMGR_STAT_WFIFOCNT_GET(scanmgr_status) > 0)) { > > + max_iter--; > > + if (max_iter > 0) > > + scanmgr_status = readl(&scan_manager_base->stat); > > + else > > + return SCAN_MGR_STATUS_ACTIVE; > > + } > > + return SCAN_MGR_STATUS_IDLE; > > +} > > The function is named _is_idle, but returns 1 if it is _active_. I'd > get rid of SCAN_MGR_STATUS_* defines (they make it harder to read) and > reverse the logic...
This seems is controversial function :) Yup, I already fixed that by removed the status variable. > > > +/* Program HPS IO Scan Chain */ > > +uint32_t scan_mgr_io_scan_chain_prg( > > + uint32_t io_scan_chain_id, > > + uint32_t io_scan_chain_len_in_bits, > > + const uint32_t *iocsr_scan_chain) > > +{ > ... > > + /* > > + * Check if the scan chain engine is inactive and the > > + * WFIFO is empty before enabling the IO scan chain > > + */ > > + if (scan_chain_engine_is_idle(SCAN_MAX_DELAY) != SCAN_MGR_STATUS_IDLE) > > + return 1; > > Maybe it would be better to return <0 numbers on error? > I changed that to if (!scan_chain_engine_is_idle(SCAN_MAX_DELAY)) It should much better now > Should the function be static? Then it could have shorter name... > Its already static and fit into single line now. > > + /* Disable IO Scan chain when configuration done*/ > > I'd put space before end of comment. > > > +/* > > + * Program HPS IO Scan Chain > > + * io_scan_chain_id - IO scan chain ID > > + * io_scan_chain_len_in_bits - IO scan chain length in bits > > + * iocsr_scan_chain - IO scan chain table > > + */ > > +uint32_t scan_mgr_io_scan_chain_prg( > > + uint32_t io_scan_chain_id, > > + uint32_t io_scan_chain_len_in_bits, > > + const uint32_t *iocsr_scan_chain); > > ...and you can probably drop the prototype... Removed. Thanks Chin Liang > > Thanks, > Pavel _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot