[OPW,kernel,OPW] staging: ft1000: function scram_start_dwnld extracted from scram_dnldr in ft1000_download.c
diff mbox

Message ID 20131018065641.GA5922@kelleynnn-HP-Compaq-8510w
State Changes Requested
Headers show

Commit Message

Kelley Nielsen Oct. 18, 2013, 6:56 a.m. UTC
function scram_dnldr is over 500 lines long, with deep indents that
trigger checkpatch warnings for too many tabs. It mainly consists of a
switch statement with long, complicated cases. The first case has been
extracted to form the helper function scram_start_dwnld. Some style
issues in the extracted lines have been corrected as well, including the
breaking of a long DEBUG (wrapper for printk) statement into two.

Signed-off-by: Kelley Nielsen <kelleynnn@gmail.com>
---
 .../staging/ft1000/ft1000-usb/ft1000_download.c    | 47 +++++++++++-----------
 1 file changed, 24 insertions(+), 23 deletions(-)

Comments

Josh Triplett Oct. 18, 2013, 3:43 p.m. UTC | #1
On Thu, Oct 17, 2013 at 11:56:41PM -0700, Kelley Nielsen wrote:
> function scram_dnldr is over 500 lines long, with deep indents that
> trigger checkpatch warnings for too many tabs. It mainly consists of a
> switch statement with long, complicated cases. The first case has been
> extracted to form the helper function scram_start_dwnld. Some style
> issues in the extracted lines have been corrected as well, including the
> breaking of a long DEBUG (wrapper for printk) statement into two.
> 
> Signed-off-by: Kelley Nielsen <kelleynnn@gmail.com>

Very nice work; cleaning this up by extracting a function makes perfect
sense.

One issue, though:

> --- a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
> +++ b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
> @@ -554,6 +554,28 @@ static u32 write_blk_fifo(struct ft1000_usb *ft1000dev, u16 **pUsFile,
>  	return Status;
>  }
>  
> +u16 scram_start_dwnld(struct ft1000_usb *ft1000dev, u16 *hshake, u32 *state)
> +{
> +	u16 status = STATUS_SUCCESS;
> +	DEBUG("FT1000:STATE_START_DWNLD\n");
> +	if (ft1000dev->usbboot)
> +		*hshake = get_handshake_usb(ft1000dev, HANDSHAKE_DSP_BL_READY);
> +	else
> +		*hshake = get_handshake(ft1000dev, HANDSHAKE_DSP_BL_READY);
> +	if (*hshake == HANDSHAKE_DSP_BL_READY) {
> +		DEBUG("scram_dnldr: handshake is HANDSHAKE_DSP_BL_READY,");
> +		DEBUG("	call put_handshake(HANDSHAKE_DRIVER_READY)\n");

Please don't break up the DEBUG line, though; you should keep
user-visible strings unbroken, even if that exceeds 80 columns.  (See
Documentation/CodingStyle, chapter 2.)

> +		put_handshake(ft1000dev, HANDSHAKE_DRIVER_READY);
> +	} else {
> +		DEBUG
> +		    ("FT1000:download:Download error: Handshake failed\n");

Likewise, don't break this line.

- Josh Triplett
Kelley Nielsen Oct. 18, 2013, 7:32 p.m. UTC | #2
Josh, thanks for your encouragement and your feedback. I knew I wouldn't 
remember everything :-)

This file would benefit from having this done several times more. The 
greatest chance for improvement, however, is with switch cases that are 
insanely long and full of coding style issues. It might make sense to fix 
the style issues after extraction, because extraction will automatically 
shorten some of the lines. Do you have any suggestions for what steps you 
would like to see done first?

(Also, Zach hopes to get some first patch projects up for btrfs, in which 
case I'll be working on those instead.)


On Friday, October 18, 2013 8:43:21 AM UTC-7, Josh Triplett wrote:
>
> On Thu, Oct 17, 2013 at 11:56:41PM -0700, Kelley Nielsen wrote: 
> > function scram_dnldr is over 500 lines long, with deep indents that 
> > trigger checkpatch warnings for too many tabs. It mainly consists of a 
> > switch statement with long, complicated cases. The first case has been 
> > extracted to form the helper function scram_start_dwnld. Some style 
> > issues in the extracted lines have been corrected as well, including the 
> > breaking of a long DEBUG (wrapper for printk) statement into two. 
> > 
> > Signed-off-by: Kelley Nielsen <kell...@gmail.com <javascript:>> 
>
> Very nice work; cleaning this up by extracting a function makes perfect 
> sense. 
>
> One issue, though: 
>
> > --- a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c 
> > +++ b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c 
> > @@ -554,6 +554,28 @@ static u32 write_blk_fifo(struct ft1000_usb 
> *ft1000dev, u16 **pUsFile, 
> >          return Status; 
> >  } 
> >   
> > +u16 scram_start_dwnld(struct ft1000_usb *ft1000dev, u16 *hshake, u32 
> *state) 
> > +{ 
> > +        u16 status = STATUS_SUCCESS; 
> > +        DEBUG("FT1000:STATE_START_DWNLD\n"); 
> > +        if (ft1000dev->usbboot) 
> > +                *hshake = get_handshake_usb(ft1000dev, 
> HANDSHAKE_DSP_BL_READY); 
> > +        else 
> > +                *hshake = get_handshake(ft1000dev, 
> HANDSHAKE_DSP_BL_READY); 
> > +        if (*hshake == HANDSHAKE_DSP_BL_READY) { 
> > +                DEBUG("scram_dnldr: handshake is 
> HANDSHAKE_DSP_BL_READY,"); 
> > +                DEBUG("        call 
> put_handshake(HANDSHAKE_DRIVER_READY)\n"); 
>
> Please don't break up the DEBUG line, though; you should keep 
> user-visible strings unbroken, even if that exceeds 80 columns.  (See 
> Documentation/CodingStyle, chapter 2.) 
>
> > +                put_handshake(ft1000dev, HANDSHAKE_DRIVER_READY); 
> > +        } else { 
> > +                DEBUG 
> > +                    ("FT1000:download:Download error: Handshake 
> failed\n"); 
>
> Likewise, don't break this line. 
>
> - Josh Triplett 
>
Josh Triplett Oct. 19, 2013, 3:29 a.m. UTC | #3
When you reply to a mail, please don't word-wrap the quoted text; as you
can see below, that wrapped the lines of code in the patch.

Sarah might be able to tell you how to make gmail not do that.

On Fri, Oct 18, 2013 at 12:32:32PM -0700, Kelley Nielsen wrote:
> Josh, thanks for your encouragement and your feedback. I knew I wouldn't 
> remember everything :-)
> 
> This file would benefit from having this done several times more. The 
> greatest chance for improvement, however, is with switch cases that are 
> insanely long and full of coding style issues. It might make sense to fix 
> the style issues after extraction, because extraction will automatically 
> shorten some of the lines. Do you have any suggestions for what steps you 
> would like to see done first?

I agree that extracting functions makes sense to do first, so that you
don't end up with gratuitous 80-column line breaks.

> (Also, Zach hopes to get some first patch projects up for btrfs, in which 
> case I'll be working on those instead.)

Sounds great.

- Josh Triplett
Sarah Sharp Oct. 28, 2013, 9:39 p.m. UTC | #4
On Fri, Oct 18, 2013 at 08:29:49PM -0700, Josh Triplett wrote:
> When you reply to a mail, please don't word-wrap the quoted text; as you
> can see below, that wrapped the lines of code in the patch.
> 
> Sarah might be able to tell you how to make gmail not do that.

I do know how to get the web interface to let you respond inline in
plain text (click ... and make sure your reply mode is Plain Text rather
than Rich Text), but I can't seem to figure out how you make it not word
wrap replies.

There's a support question that indicates it might be possible with Rich
Text replies (which we _don't_ want, since we don't want to send html
mails to the lists):

http://productforums.google.com/forum/#!topic/gmail/UAhajzTtSU0

Another support question indicates that you can't turn off word wrap for
plain text emails:

http://productforums.google.com/forum/#!topic/gmail/X6X27-3XADg

ISTR that Greg has used the gmail web interface in the past, but I think
he uses the Linux Foundation servers, and those are probably set to not
word-wrap out going mails.  Greg?

What are the other applicants, mentors, and former interns using to
respond to patches?  I'm personally using mutt to reply to emails after
pulling them down locally with mbsync or fetchmail (depending on the
server).

Sarah Sharp
Waskiewicz Jr, Peter P Oct. 28, 2013, 9:52 p.m. UTC | #5
On Mon, 2013-10-28 at 21:39 +0000, Sarah Sharp wrote:
> What are the other applicants, mentors, and former interns using to
> respond to patches?  I'm personally using mutt to reply to emails after
> pulling them down locally with mbsync or fetchmail (depending on the
> server).

I'm mostly using Evolution in Plain Text mode with evolution-ews
(Exchange Web Services).  I was using mutt through imap with the vim
interface (to annotate tab characters), but I've not done that for a few
months now.

-PJ
Greg Kroah-Hartman Oct. 28, 2013, 10:11 p.m. UTC | #6
On Mon, Oct 28, 2013 at 09:39:14PM +0000, Sarah Sharp wrote:
> ISTR that Greg has used the gmail web interface in the past, but I think
> he uses the Linux Foundation servers, and those are probably set to not
> word-wrap out going mails.  Greg?

I only use gmail's SMTP and IMAP servers, never the web interface, so I
don't know how to do this at all, sorry.

greg k-h
Kelley Nielsen Oct. 28, 2013, 10:19 p.m. UTC | #7
Since that happened, I've been using mutt for everything that has any code 
> in it. Better to be safe.
Lisa Nguyen Oct. 28, 2013, 10:26 p.m. UTC | #8
On Mon, Oct 28, 2013 at 3:11 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 28, 2013 at 09:39:14PM +0000, Sarah Sharp wrote:
>> ISTR that Greg has used the gmail web interface in the past, but I think
>> he uses the Linux Foundation servers, and those are probably set to not
>> word-wrap out going mails.  Greg?
>
> I only use gmail's SMTP and IMAP servers, never the web interface, so I
> don't know how to do this at all, sorry.
>
> greg k-h
>

I use the Gmail web interface with the plain text option on. I can
tell when I'm seeing rich text vs. plain text when I'm responding.

Lisa

> You received this message because you are subscribed to the Google Groups "opw-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to opw-kernel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
Zach Brown Oct. 29, 2013, 10:11 p.m. UTC | #9
> What are the other applicants, mentors, and former interns using to
> respond to patches?  I'm personally using mutt to reply to emails after
> pulling them down locally with mbsync or fetchmail (depending on the
> server).

Same here.  I use mutt and fetchmail.

- z

Patch
diff mbox

diff --git a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
index 1a1413d..c5bf482 100644
--- a/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
+++ b/drivers/staging/ft1000/ft1000-usb/ft1000_download.c
@@ -554,6 +554,28 @@  static u32 write_blk_fifo(struct ft1000_usb *ft1000dev, u16 **pUsFile,
 	return Status;
 }
 
+u16 scram_start_dwnld(struct ft1000_usb *ft1000dev, u16 *hshake, u32 *state)
+{
+	u16 status = STATUS_SUCCESS;
+	DEBUG("FT1000:STATE_START_DWNLD\n");
+	if (ft1000dev->usbboot)
+		*hshake = get_handshake_usb(ft1000dev, HANDSHAKE_DSP_BL_READY);
+	else
+		*hshake = get_handshake(ft1000dev, HANDSHAKE_DSP_BL_READY);
+	if (*hshake == HANDSHAKE_DSP_BL_READY) {
+		DEBUG("scram_dnldr: handshake is HANDSHAKE_DSP_BL_READY,");
+		DEBUG("	call put_handshake(HANDSHAKE_DRIVER_READY)\n");
+		put_handshake(ft1000dev, HANDSHAKE_DRIVER_READY);
+	} else {
+		DEBUG
+		    ("FT1000:download:Download error: Handshake failed\n");
+		status = STATUS_FAILURE;
+	}
+	*state = STATE_BOOT_DWNLD;
+	return status;
+}
+
+
 /* Scramble downloader for Harley based ASIC via USB interface */
 u16 scram_dnldr(struct ft1000_usb *ft1000dev, void *pFileStart,
 		u32 FileLength)
@@ -617,29 +639,8 @@  u16 scram_dnldr(struct ft1000_usb *ft1000dev, void *pFileStart,
 	while ((status == STATUS_SUCCESS) && (state != STATE_DONE_FILE)) {
 		switch (state) {
 		case STATE_START_DWNLD:
-			DEBUG("FT1000:STATE_START_DWNLD\n");
-			if (ft1000dev->usbboot)
-				handshake =
-				    get_handshake_usb(ft1000dev,
-						      HANDSHAKE_DSP_BL_READY);
-			else
-				handshake =
-				    get_handshake(ft1000dev,
-						  HANDSHAKE_DSP_BL_READY);
-
-			if (handshake == HANDSHAKE_DSP_BL_READY) {
-				DEBUG
-				    ("scram_dnldr: handshake is HANDSHAKE_DSP_BL_READY, call put_handshake(HANDSHAKE_DRIVER_READY)\n");
-				put_handshake(ft1000dev,
-					      HANDSHAKE_DRIVER_READY);
-			} else {
-				DEBUG
-				    ("FT1000:download:Download error: Handshake failed\n");
-				status = STATUS_FAILURE;
-			}
-
-			state = STATE_BOOT_DWNLD;
-
+			status = scram_start_dwnld(ft1000dev, &handshake,
+						   &state);
 			break;
 
 		case STATE_BOOT_DWNLD: