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.bha...@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")
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.
> +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.
> +}
> +
> #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.
> 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.
> 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
