Re: [PATCH] West Bridge Astoria Driver 2.6.35, minor block and device driver updates

[Greg KH <greg@kroah.com>]

On Thu, Sep 02, 2010 at 03:49:54PM -0700, David Cross wrote:
> This patch contains minor updates to the west bridge block and device
> drivers, including the addition of a common lock and a minor update to
> remove the blk_fs_request call based on a kernel update. These changes
> could possibly be separated, but are very minor in scope and as such
> have been submitted together.

Again, one patch per logical change please.  That's the way patches for
the kernel work.  So can you please separate these and resend them?


> Signed-off-by: David Cross <david.cross@cypress.com>
> 
> diff -uprN -X linux-next-vanilla/Documentation/dontdiff linux-next-vanilla/drivers/staging/westbridge/astoria/block/cyasblkdev_block.c linux-next-incl-sdk/drivers/staging/westbridge/astoria/block/cyasblkdev_block.c
> --- linux-next-vanilla/drivers/staging/westbridge/astoria/block/cyasblkdev_block.c	2010-08-31 19:32:51.000000000 -0700
> +++ linux-next-incl-sdk/drivers/staging/westbridge/astoria/block/cyasblkdev_block.c	2010-08-25 18:29:44.000000000 -0700
> @@ -389,7 +389,7 @@ int cyasblkdev_media_changed(struct gend
>  	struct cyasblkdev_blk_data *bd;
>  
>  	#ifndef WESTBRIDGE_NDEBUG
> -	cy_as_hal_print_message("cyasblkdev_media_changed() is called\n");
> +	cy_as_hal_print_message(KERN_INFO"cyasblkdev_media_changed() is called\n");

Ick.  How about a ' ' after KERN_INFO?  And do you really always want to
do this?  You can enable tracing to get this type of info.

>  	#endif

The whole NDEBUG stuff should be removed from the .c files and put in
the .h file if it's really needed.

> @@ -493,7 +493,7 @@ static void cyasblkdev_issuecallback(
>  {
>  	int retry_cnt = 0;
>  	DBGPRN_FUNC_NAME;
> -
> +	
>  	if (status != CY_AS_ERROR_SUCCESS) {
>  		#ifndef WESTBRIDGE_NDEBUG
>  		cy_as_hal_print_message(

You added a line of trailing spaces for no good reason :(


> @@ -509,7 +509,10 @@ static void cyasblkdev_issuecallback(
>  		__func__, (unsigned int) gl_bd->queue.req, status,
>  		(unsigned int) blk_rq_sectors(gl_bd->queue.req)) ;
>  	#endif
> -
> +	
> +	if(rq_data_dir(gl_bd->queue.req) != READ) {
> +		cy_as_release_common_lock();
> +	}

No need for the extra {}, please take a look at
Documentation/CodingStyle for how to do things for the kernel.  Also a
space is needed after the 'if'.

Please run your patches through the scripts/checkpatch.pl tool before
sending them to me.  Yes, there's lots of checkpatch issues in the
driver already, but don't add new ones now please.

Care to redo this?

thanks,

greg k-h