From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chin Liang See <clsee@altera.com>
Date: Fri, 21 Feb 2014 09:26:10 -0600
Subject: [U-Boot] [PATCH v3] socfpga: Adding Scan Manager driver
In-Reply-To: <20140221150746.GA6037@amd.pavel.ucw.cz>
References: <1392994382-3882-1-git-send-email-clsee@altera.com>
	<20140221150746.GA6037@amd.pavel.ucw.cz>
Message-ID: <1392996370.3602.7.camel@clsee-VirtualBox>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Pavel,

Nice to hear from you again :)

On Fri, 2014-02-21 at 16:07 +0100, ZY - pavel wrote:
> Hi!
> 
> > +	/*
> > +	 * Check if the scan chain engine is inactive and the
> > +	 * WFIFO is empty before enabling the IO scan chain
> > +	 */
> > +	if (SCAN_MGR_IO_SCAN_ENGINE_STATUS_IDLE
> > +		!= scan_mgr_io_scan_chain_engine_is_idle(
> > +		MAX_WAITING_DELAY_IO_SCAN_ENGINE)) {
> > +		return 1;
> > +	}
> 
> Hmm.. function named "_is_idle" maybe should just return 0 or 1?

Yup we can simplify that. Probably just remain for readability. Hope its
ok for you.

> 
> > +		/*
> > +		 * Check if the scan chain engine has completed the
> > +		 * IO scan chain data shifting
> > +		 */
> > +		if (SCAN_MGR_IO_SCAN_ENGINE_STATUS_IDLE
> > +			!= scan_mgr_io_scan_chain_engine_is_idle(
> > +			MAX_WAITING_DELAY_IO_SCAN_ENGINE)) {
> > +			/* Disable IO Scan chain when error detected */
> > +			clrbits_le32(&scan_manager_base->en,
> > +				1 << io_scan_chain_id);
> > +			return 1;
> > +		}
> > +	}
> 
> "goto error" would help avoid code duplication.

Good suggestion here. Will change that.

> 
> > +struct socfpga_scan_manager {
> > +	u32	stat;
> > +	u32	en;
> > +	u32	padding[2];
> > +	u32	fifosinglebyte;
> > +	u32	fifodoublebyte;
> > +	u32	fifoquadbyte;
> > +};
> 
> some underscores should be added here.

Sure, for better readability.

Thanks!

Chin Liang

> 
> Thanks,
> 									Pavel