keep the CC list please.
On Wed, Jan 09, 2013 at 07:28:43AM +0000, Fangxiaozhi (Franko) wrote:
> > > +/* This function will send
> > > + * a scsi switch command called rewind' to huawei dongle.
> > > + * When the dongle receives this command at the first time,
> > > + * it will reboot immediately,
> > > + * after rebooted, it will ignore this command and do nothing,
> > > + * if it receives this command again.
> > > + * So it is unnecessary to read its response. */
> >
> > This is not how a proper multi line comment looks like. The line break in
> > the
> > middle of the sentence does not look good IMHO.
>
> -------Sorry, but if not do that, the comment is longer than 80 characters in
> one line.
Don't cross 80 lines but you don't need a line break after "send" and
"immediately," if you look at your initial patch.
> > > +static int usb_stor_huawei_scsi_init(struct us_data *us) {
> > > + int result = 0;
> > > + int act_len = 0;
> > > + struct bulk_cb_wrap *bcbw = (struct bulk_cb_wrap *) us->iobuf;
> > > + char rewind_cmd[] = {0x11, 0x06, 0x20, 0x00, 0x00, 0x01, 0x01, 0x00,
> > > + 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
> > > +
> > > + memset(bcbw, 0, sizeof(struct bulk_cb_wrap));
> -----set the whole bcbw to be 0.
>
> > > + bcbw->Signature = cpu_to_le32(US_BULK_CB_SIGN);
> > > + bcbw->Tag = 0;
> > > + bcbw->DataTransferLength = 0;
> > > + bcbw->Flags = bcbw->Lun = 0;
> > > + bcbw->Length = sizeof(rewind_cmd);
> ------initialize every elements of the struct bulk_cb_wrap.
> >
> > I asked earlier and I ask again: why memset to zero followed by init to
> > zero.
> > Could we stick to one thing?
> -----So these codes maybe seem to be redundant, but I think it can make the
> codes to more clear.
It is point less. Looking at drivers/usb/storage/transport.c at other users
and nobody is doing it. I don't see the point in assigning a values a few
times to zero. Please remove the "= 0" assignments.
> > > + memcpy(bcbw->CDB, rewind_cmd, sizeof(rewind_cmd));
> > > +
> > > + result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe, bcbw,
> > > + US_BULK_CB_WRAP_LEN, &act_len);
> >
> > This looks like it could work. Was it really tested before sending this
> > time? :P
> -----Yes, it is tested OK before our submitting.
okay then.
> >
> > > + US_DEBUGP("transfer actual length=%d, result=%d\n", act_len, result);
> > > + return result;
> > > +}
> > > +
> > > +/* usb_stor_huawei_dongles_pid: try to find the supported Huawei USB
> > > +dongles
> > > + * In Huawei, they assign the following product IDs
> > > + * for all of their mobile broadband dongles,
> > > + * including the new dongles in the future.
> > > + * So if the product ID is not included in this list,
> > > + * it means it is not Huawei's mobile broadband dongles.
> > > + */
> >
> > Not a proper multiple line comment. Kernel doc format is different btw. and
> > is
> > described in Documentation/kernel-doc-nano-HOWTO.txt
> >
> ------Do you mean to write the comment like that:
>
> Example kernel-doc function comment:
>
> /**
> * foobar() - short function description of foobar
> * @arg1: Describe the first argument to foobar.
> * @arg2: Describe the second argument to foobar.
> * One can provide multiple line descriptions
> * for arguments.
> *
> * A longer description, with more discussion of the function foobar()
> * that might be useful to those using or modifying it. Begins with
> * empty comment line, and may include additional embedded empty
> * comment lines.
> *
> * The longer description can have multiple paragraphs.
> *
> * Return: Describe the return value of foobar.
> */
Yes, something like that. A short comment would work, too. However since you
added the function name in top of your comment you should stick to a standard
form.
> > > +static int usb_stor_huawei_dongles_pid(struct us_data *us) {
> > > + struct usb_interface_descriptor *idesc;
> > > + int idProduct;
> > > +
> > > + idesc = &us->pusb_intf->cur_altsetting->desc;
> > > + idProduct = us->pusb_dev->descriptor.idProduct;
> > > + /* The first port is CDROM,
> > > + * means the dongle in the single port mode,
> > > + * and a switch command is required to be sent. */
> > > + if (idesc && idesc->bInterfaceNumber == 0) {
> > > + if ((idProduct == 0x1001)
> > > + || (idProduct == 0x1003)
> > > + || (idProduct == 0x1004)
> > > + || (idProduct >= 0x1401 && idProduct < 0x1501)
> > > + || (idProduct > 0x1504 && idProduct <= 0x1600)
> >
> > why not >= 1505 and <= 1500 instead of the < and > operators? It would look
> > better. Do you exclude them on purpose or by accident?
> ------Well, I think ">= 1505 and <= 1500" is the same as " idProduct > 0x1504
> and idProduct < 0x1501", they both include 1500 and 1505.
Yes, it does but it reads better if the operators are the same and not diffent
for no reason.
> > On a second look, why not do this instead:
> >
> > switch (idProduct)
> > case 0x1001:
> > case 0x1401 .. 0x1500
> > return 1;
> > default:
> > return 0;
> >
> > This reads way way beter.
> -------Yes, maybe, but "case 0x1401 .. 0x1500" is the standard style in GNU C?
So what is the problem? It looks better, doesn't it? There are other users in
kernel for instance bvec_alloc_bs() fs/bio.c
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
