From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rajesh Bhagat Date: Thu, 9 Jun 2016 15:48:39 +0000 Subject: [U-Boot] [PATCH v4 1/2] common: usb_storage: Make common function for usb_stor_read/usb_stor_write In-Reply-To: <57598244.8040700@denx.de> References: <1465453952-22586-1-git-send-email-rajesh.bhagat@nxp.com> <1465453952-22586-2-git-send-email-rajesh.bhagat@nxp.com> <57598244.8040700@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de > -----Original Message----- > From: Marek Vasut [mailto:marex at denx.de] > Sent: Thursday, June 09, 2016 8:21 PM > To: Rajesh Bhagat ; u-boot at lists.denx.de > Cc: sjg at chromium.org; york sun ; Sriram Dash > > Subject: Re: [PATCH v4 1/2] common: usb_storage: Make common function for > usb_stor_read/usb_stor_write > > 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 > > --- > > 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. > Ok, I will change it into a function in v5. > > + > > #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. > My apologies, I took it from usb_stor_write function and din't noticed the coding style issue in the existing code. Will correct it in v5. > > + /* 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. > I understand your concern, but my intent was to keep the exiting prototypes unchanged, may be difference of thought process. Will take care in v5. Best Regards, Rajesh Bhagat > > + 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