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

14 Jun 2016

...
-----Original Message-----
From: Marek Vasut [mailto:marex@denx.de]
Sent: Monday, June 13, 2016 7:00 PM
To: Rajesh Bhagat rajesh.bhagat@nxp.com; u-boot@lists.denx.de
Cc: sjg@chromium.org; york sun york.sun@nxp.com; Sriram Dash
sriram.dash@nxp.com
Subject: Re: [PATCH v5 1/2] common: usb_storage: Make common function for
usb_stor_read/usb_stor_write
On 06/13/2016 06:03 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 v5:

Converts USB_STOR_OPERATION_FUNC macro to a function
Corrects the multi line comment accroding to coding style
Renames variable to dev to remove code duplication

Changes in v4:

Adds code to make common function for read/write

common/usb_storage.c |  129
+++++++++++++++----------------------------------
 1 files changed, 40 insertions(+), 89 deletions(-)

diff --git a/common/usb_storage.c b/common/usb_storage.c index
7e6e52d..f060637 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -1104,12 +1104,22 @@ static void usb_bin_fixup(struct
usb_device_descriptor descriptor,  }  #endif /* CONFIG_USB_BIN_FIXUP
*/
+#define USB_STOR_OP_TYPE	(is_write ? "write" : "read")
Hello Marek,
...
I just noticed this gem here. It's a macro which uses a variable, but the variable is not
passed in as it's argument. Just inline this, it's not worth having it as a macro here.
Will take care in v6. As pointed in last comment, I will be using __func__ and remove this 
macro which was  just used to differentiate b/w read/write operations in debug logs.
...
...
+static int usb_stor_operation(ccb *srb, struct us_data *ss, unsigned long start,

	      unsigned short blocks, bool is_write) {


return is_write ? usb_write_10(srb, ss, start, blocks) :
usb_read_10(srb, ss, start, blocks);



usb_read_10() and usb_write_10() look exactly the same for all but the command .
You should do this unification at that level instead.
Will take care in v6. Will unify the code to usb_read_write_10 and add is_write parameter to 
process different command. And then it can be called directly instead of making a wrapper
function.
...
...
+}



#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 +1139,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_OP_TYPE, block_dev->devnum);

#else

debug("\nusb_read: udev %d\n", block_dev->devnum);


debug("\nusb_%s: udev %d\n", USB_STOR_OP_TYPE, block_dev->devnum);

You can just use "\n%s: ..." and use __func__ instead of USB_STOR_OP_TYPE here,
that'd be less cryptic than "usb_read:" and it's used all over the place already anyway.
Will take care in v6.
...
...
udev = usb_dev_desc[block_dev->devnum].priv;
   if (!udev) {
   	debug("%s: No device\n", __func__); @@ -1146,11 +1156,15 @@ 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_OP_TYPE, block_dev->devnum, start, blks,


     buf_addr);



DTTO here.
Will take care in v6.
Best Regards,
Rajesh Bhagat
...
...
do {

/* XXX need some comment here */




/*


 * 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 +1176,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(srb, ss, start, smallblks, is_write)) {


	debug("%s ERROR\n", USB_STOR_OP_TYPE);
usb_request_sense(srb, ss);
if (retry--)
	goto retry_it;



@@ -1176,9 +1190,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_OP_TYPE, start, smallblks, buf_addr);


usb_disable_asynch(0); /* asynch transfer allowed */
 if (blkcnt >= USB_MAX_XFER_BLK)


@@ -1186,90 +1200,27 @@ retry_it:
   return blkcnt;
 }
[...]
--
Best regards,
Marek Vasut

    