[U-Boot] [PATCH v4 1/2] common: usb_storage: Make common function for usb_stor_read/usb_stor_write
Rajesh Bhagat
rajesh.bhagat at nxp.com
Thu Jun 9 17:48:39 CEST 2016
> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Thursday, June 09, 2016 8:21 PM
> To: Rajesh Bhagat <rajesh.bhagat at nxp.com>; u-boot at lists.denx.de
> Cc: sjg at chromium.org; york sun <york.sun at nxp.com>; Sriram Dash
> <sriram.dash at nxp.com>
> 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 <rajesh.bhagat at 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.
>
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
More information about the U-Boot
mailing list