Dear Sebastian:
> -----Original Message-----
> From: Sebastian Andrzej Siewior [mailto:[email protected]]
> Sent: Thursday, January 10, 2013 5:30 AM
> To: Fangxiaozhi (Franko)
> Cc: Linlei (Lei Lin); Huqiao (C); [email protected]
> Subject: Re: [PATCH 1/1]linux-usb:optimize to match the Huawei USB storage
> devices and support new switch command
>
> 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.
>
You mean that we have to write as follows?
+ memset(bcbw, 0, sizeof(struct bulk_cb_wrap));
+ bcbw->Signature = cpu_to_le32(US_BULK_CB_SIGN);
+ bcbw->Length = sizeof(rewind_cmd);
Right?
> > > > + 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
Best Regards,
Franko Fang
N�����r��y����b�X��ǧv�^�){.n�+����{������^n�r���z���h�����&�� �G���h�(�階�ݢj"���m������z�ޖ���f���h���~�m�
