From: ShiHao <i.shihao.999@gmail.com>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: skhan@linuxfoundation.org, linux-usb@vger.kernel.org,
i.shihao.999@gmail.com, gregkh@linuxfoundation.org,
stern@rowland.harvard.edu
Subject: Re: [PATCH] usb: Implement proper subclass protocol translation
Date: Sat, 11 Oct 2025 15:07:23 +0530 [thread overview]
Message-ID: <aOolUwtQHX7JUASe@fedora> (raw)
In-Reply-To: <2d7cd968-3365-4f9f-aa88-d953809bf0ce@rowland.harvard.edu>
On Fri, Oct 10, 2025 at 11:07:03AM -0400, Alan Stern wrote:
> The spacing in this text is strange -- double spaces in random places,
> no space following the first '.' in the last paragraph, and an extra
> space before the second '.'. Also, strange capitalization of the word
> "It" in two places.
>
Hi Alan,
thanks for the review and suggestions . I have updated the commit
message body removed double spaces and over cappitalization of i
> > - /* FIXME: we must do the protocol translation here */
> > + /* Protocol translation per scsi opcode group */
>
> This comment should be different. The "per scsi opcode group" will be
> mentioned in another comment a few lines later; it's not needed here as
> well.
also added new comment for the protocol translation
as you said to make it different from earlier as per op
code comment.
> > if (us->subclass == USB_SC_RBC || us->subclass == USB_SC_SCSI ||
> > - us->subclass == USB_SC_CYP_ATACB)
> > - srb->cmd_len = 6;
> > - else
> > + us->subclass == USB_SC_CYP_ATACB) {
>
> Please don't change the existing indentation.
I changed the pervious indentaion as greg k-h told me to
do so he said to check checkpatch.pl script errors which said
alignment should match open parenthesis thats why i placed it
under the open parenthesis and it does not have any checkpatch.pl
error so far . I have checked it with the argument --strict too .
Changing the line from open parenthesis triggers checkpatch.pl
error.
> > + else
> > + srb->cmd_len = 6;
> > + } else {
>
> And you could add a comment here explaining that the other protocols use
> a fixed value for the command length.
>
> > srb->cmd_len = 12;
> > + }
Thanks for the suggestion of this new comment i have added it
in the second version please check the above changes in the updated
version and if there is anything wrong please let me know i am ready
to make any further changes and again thanks for you review and suggestions
on this matter . Thanks for giving your precious time to this matter . Thank you
so much.
prev parent reply other threads:[~2025-10-11 9:37 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-10 11:30 [PATCH] usb: Implement proper subclass protocol translation Shi Hao
2025-10-10 12:29 ` Greg KH
2025-10-10 15:07 ` Alan Stern
2025-10-11 9:37 ` ShiHao [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aOolUwtQHX7JUASe@fedora \
--to=i.shihao.999@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-usb@vger.kernel.org \
--cc=skhan@linuxfoundation.org \
--cc=stern@rowland.harvard.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).