Dear Sebastian: > -----Original Message----- > From: Sebastian Andrzej Siewior [mailto:sebastian@xxxxxxxxxxxxx] > Sent: Thursday, January 10, 2013 5:30 AM > To: Fangxiaozhi (Franko) > Cc: Linlei (Lei Lin); Huqiao (C); linux-usb@xxxxxxxxxxxxxxx > 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��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
