> -----Original Message-----
> From: Marek Vasut [mailto:ma...@denx.de]
> Sent: Monday, June 13, 2016 7:00 PM
> To: Rajesh Bhagat <rajesh.bha...@nxp.com>; u-boot@lists.denx.de
> Cc: s...@chromium.org; york sun <york....@nxp.com>; Sriram Dash
> <sriram.d...@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.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")
>
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
