[U-Boot] [PATCH v4 1/2] common: usb_storage: Make common function for usb_stor_read/usb_stor_write

[Marek Vasut <marex@denx.de>]

On 06/09/2016 08:32 AM, Rajesh Bhagat wrote:
> Performs code cleanup by making common function for usb_stor_read/
> usb_stor_write. Currently only difference in these fucntions is call
> to usb_read_10/usb_write_10 scsi commands.
> 
> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
> ---
> Changes in v4: 
>  - Adds code to make common function for read/write
> 
>  common/usb_storage.c | 132 ++++++++++++++++++---------------------------------
>  1 file changed, 46 insertions(+), 86 deletions(-)
> 
> diff --git a/common/usb_storage.c b/common/usb_storage.c
> index 7e6e52d..5efdc90 100644
> --- a/common/usb_storage.c
> +++ b/common/usb_storage.c
> @@ -1104,12 +1104,20 @@ static void usb_bin_fixup(struct usb_device_descriptor descriptor,
>  }
>  #endif /* CONFIG_USB_BIN_FIXUP */
>  
> +#define USB_STOR_OPERATION	is_write ? "write" : "read"
> +#define USB_STOR_OPERATION_FUNC(srb, ss, start, smallblks)		 \
> +				is_write ?				 \
> +				usb_write_10(srb, ss, start, smallblks) :\
> +				usb_read_10(srb, ss, start, smallblks)

Turn this into a function. The macro is obviously missing even the most
basic parenthesis, but that's beyond the point.

> +
>  #ifdef CONFIG_BLK
> -static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
> -				   lbaint_t blkcnt, void *buffer)
> +static unsigned long usb_stor_read_write(struct udevice *dev, lbaint_t blknr,
> +					 lbaint_t blkcnt, const void *buffer,
> +					 bool is_write)
>  #else
> -static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
> -				   lbaint_t blkcnt, void *buffer)
> +static unsigned long usb_stor_read_write(struct blk_desc *block_dev,
> +					 lbaint_t blknr, lbaint_t blkcnt,
> +					 const void *buffer, bool is_write)
>  #endif
>  {
>  	lbaint_t start, blks;
> @@ -1129,9 +1137,9 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
>  #ifdef CONFIG_BLK
>  	block_dev = dev_get_uclass_platdata(dev);
>  	udev = dev_get_parent_priv(dev_get_parent(dev));
> -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> +	debug("\nusb_%s: udev %d\n", USB_STOR_OPERATION, block_dev->devnum);
>  #else
> -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> +	debug("\nusb_%s: udev %d\n", USB_STOR_OPERATION, block_dev->devnum);
>  	udev = usb_dev_desc[block_dev->devnum].priv;
>  	if (!udev) {
>  		debug("%s: No device\n", __func__);
> @@ -1146,11 +1154,14 @@ static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
>  	start = blknr;
>  	blks = blkcnt;
>  
> -	debug("\nusb_read: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
> -	      PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr);
> +	debug("\nusb_%s: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
> +	      PRIxPTR "\n", USB_STOR_OPERATION, block_dev->devnum, start, blks,
> +	      buf_addr);
>  
>  	do {
> -		/* XXX need some comment here */

Multi-line comment goes this way:
/*
 * foo
 * bar
 */
It's in the coding style document too, please read it.

> +		/* If read/write fails retry for max retry count else
> +		 * return with number of blocks written successfully.
> +		 */
>  		retry = 2;
>  		srb->pdata = (unsigned char *)buf_addr;
>  		if (blks > USB_MAX_XFER_BLK)
> @@ -1162,8 +1173,8 @@ retry_it:
>  			usb_show_progress();
>  		srb->datalen = block_dev->blksz * smallblks;
>  		srb->pdata = (unsigned char *)buf_addr;
> -		if (usb_read_10(srb, ss, start, smallblks)) {
> -			debug("Read ERROR\n");
> +		if (USB_STOR_OPERATION_FUNC(srb, ss, start, smallblks)) {
> +			debug("%s ERROR\n", USB_STOR_OPERATION);
>  			usb_request_sense(srb, ss);
>  			if (retry--)
>  				goto retry_it;
> @@ -1176,9 +1187,9 @@ retry_it:
>  	} while (blks != 0);
>  	ss->flags &= ~USB_READY;
>  
> -	debug("usb_read: end startblk " LBAF
> +	debug("usb_%s: end startblk " LBAF
>  	      ", blccnt %x buffer %" PRIxPTR "\n",
> -	      start, smallblks, buf_addr);
> +	      USB_STOR_OPERATION, start, smallblks, buf_addr);
>  
>  	usb_disable_asynch(0); /* asynch transfer allowed */
>  	if (blkcnt >= USB_MAX_XFER_BLK)
> @@ -1186,6 +1197,24 @@ retry_it:
>  	return blkcnt;
>  }
>  
> +
> +#ifdef CONFIG_BLK
> +static unsigned long usb_stor_read(struct udevice *dev, lbaint_t blknr,
> +				   lbaint_t blkcnt, void *buffer)
> +#else
> +static unsigned long usb_stor_read(struct blk_desc *block_dev, lbaint_t blknr,
> +				   lbaint_t blkcnt, void *buffer)
> +#endif
> +{
> +	return usb_stor_read_write(
> +#ifdef CONFIG_BLK

Rename the variable to be always dev and drop this ifdef ... please,
review the patches before submitting to remove such stupid omissions
from them, it's wasting both my time and yours.

> +				   dev,
> +#else
> +				   block_dev,
> +#endif
> +				   blknr, blkcnt, buffer, false);
> +}
> +
>  #ifdef CONFIG_BLK
>  static unsigned long usb_stor_write(struct udevice *dev, lbaint_t blknr,
>  				    lbaint_t blkcnt, const void *buffer)
> @@ -1194,82 +1223,13 @@ static unsigned long usb_stor_write(struct blk_desc *block_dev, lbaint_t blknr,
>  				    lbaint_t blkcnt, const void *buffer)
>  #endif
>  {
> -	lbaint_t start, blks;
> -	uintptr_t buf_addr;
> -	unsigned short smallblks;
> -	struct usb_device *udev;
> -	struct us_data *ss;
> -	int retry;
> -	ccb *srb = &usb_ccb;
> +	return usb_stor_read_write(
>  #ifdef CONFIG_BLK
> -	struct blk_desc *block_dev;
> -#endif
> -
> -	if (blkcnt == 0)
> -		return 0;
> -
> -	/* Setup  device */
> -#ifdef CONFIG_BLK
> -	block_dev = dev_get_uclass_platdata(dev);
> -	udev = dev_get_parent_priv(dev_get_parent(dev));
> -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> +				   dev,
>  #else
> -	debug("\nusb_read: udev %d\n", block_dev->devnum);
> -	udev = usb_dev_desc[block_dev->devnum].priv;
> -	if (!udev) {
> -		debug("%s: No device\n", __func__);
> -		return 0;
> -	}
> +				   block_dev,
>  #endif
> -	ss = (struct us_data *)udev->privptr;
> -
> -	usb_disable_asynch(1); /* asynch transfer not allowed */
> -
> -	srb->lun = block_dev->lun;
> -	buf_addr = (uintptr_t)buffer;
> -	start = blknr;
> -	blks = blkcnt;
> -
> -	debug("\nusb_write: dev %d startblk " LBAF ", blccnt " LBAF " buffer %"
> -	      PRIxPTR "\n", block_dev->devnum, start, blks, buf_addr);
> -
> -	do {
> -		/* If write fails retry for max retry count else
> -		 * return with number of blocks written successfully.
> -		 */
> -		retry = 2;
> -		srb->pdata = (unsigned char *)buf_addr;
> -		if (blks > USB_MAX_XFER_BLK)
> -			smallblks = USB_MAX_XFER_BLK;
> -		else
> -			smallblks = (unsigned short) blks;
> -retry_it:
> -		if (smallblks == USB_MAX_XFER_BLK)
> -			usb_show_progress();
> -		srb->datalen = block_dev->blksz * smallblks;
> -		srb->pdata = (unsigned char *)buf_addr;
> -		if (usb_write_10(srb, ss, start, smallblks)) {
> -			debug("Write ERROR\n");
> -			usb_request_sense(srb, ss);
> -			if (retry--)
> -				goto retry_it;
> -			blkcnt -= blks;
> -			break;
> -		}
> -		start += smallblks;
> -		blks -= smallblks;
> -		buf_addr += srb->datalen;
> -	} while (blks != 0);
> -	ss->flags &= ~USB_READY;
> -
> -	debug("usb_write: end startblk " LBAF ", blccnt %x buffer %"
> -	      PRIxPTR "\n", start, smallblks, buf_addr);
> -
> -	usb_disable_asynch(0); /* asynch transfer allowed */
> -	if (blkcnt >= USB_MAX_XFER_BLK)
> -		debug("\n");
> -	return blkcnt;
> -
> +				   blknr, blkcnt, buffer, true);
>  }
>  
>  /* Probe to see if a new device is actually a Storage device */
> 


-- 
Best regards,
Marek Vasut