[U-Boot] [PATCH v6] socfpga: Adding Scan Manager driver

[Chin Liang See]

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 <clsee at altera.com>
> > Cc: Dinh Nguyen <dinguyen at altera.com>
> > Cc: Wolfgang Denk <wd at denx.de>
> > CC: Pavel Machek <pavel at denx.de>
> > Cc: Tom Rini <trini at ti.com>
> > Cc: Albert Aribaud <albert.u.boot at 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