public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Tresca Agustin <treskita@gmail.com>
Cc: gregkh@suse.de, wfp5p@virginia.edu, devel@driverdev.osuosl.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] I checked with chechpatch.pl the archive in drivers/staging/bcm/CmHost.c , and cleanup the Errors and warnings, I didn't change any function, only code style.
Date: Thu, 10 Nov 2011 00:00:56 +0300	[thread overview]
Message-ID: <20111109210056.GV4751@mwanda> (raw)
In-Reply-To: <1320710143-2675-1-git-send-email-a.tresca@hotmail.com>

[-- Attachment #1: Type: text/plain, Size: 3244 bytes --]

Hi Tresca,

Welcome.  :)

1) This patch combines way too many things into one patch.  Only do
one logical thing per patch.

2) When you're writing the changelog write a one line summary at the
top, leave a blank line, and then write the changelog.  You didn't
have the summary so the Subject of the email was messed up.

3) No signed-off-by line.

On Mon, Nov 07, 2011 at 08:55:43PM -0300, Tresca Agustin wrote:
> From: Tresca Agustin <atresca@atresca-system.(none)>
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The email address is wrong here.

>
> ---
>  drivers/staging/bcm/CmHost.c | 3794 ++++++++++++++++++++++++-----------------
>  1 files changed, 2221 insertions(+), 1573 deletions(-)
> 
> diff --git a/drivers/staging/bcm/CmHost.c b/drivers/staging/bcm/CmHost.c
> index c0ee95a..8819c52 100644
> --- a/drivers/staging/bcm/CmHost.c
> +++ b/drivers/staging/bcm/CmHost.c
> @@ -4,18 +4,17 @@
>  *	Management.
>  ************************************************************/
>  
> -//#define CONN_MSG
> +/*#define CONN_MSG*/
>  #include "headers.h"
>  
> -typedef enum _E_CLASSIFIER_ACTION
> -{
> -	eInvalidClassifierAction,
> -	eAddClassifier,
> -	eReplaceClassifier,
> -	eDeleteClassifier
> -}E_CLASSIFIER_ACTION;
> +enum _E_CLASSIFIER_ACTION {
> +	eInvalidClsfrAction,
> +	eAddClsfr,
> +	eRplcClsfr,
> +	eDelClsfr
> +}; /*_E_CLASSIFIER_ACTION*/

These names are even worse than the original.

>  
> -static ULONG GetNextTargetBufferLocation(PMINI_ADAPTER Adapter,B_UINT16 tid);
> +static ULONG GetNextTargetBufferLocation(PMINI_ADAPTER Adapter, B_UINT16 tid);
>  
>  /************************************************************
>  * Function	  -	SearchSfid
> @@ -24,18 +23,18 @@ static ULONG GetNextTargetBufferLocation(PMINI_ADAPTER Adapter,B_UINT16 tid);
>  *				specified SFID as input parameter.
>  *
>  * Parameters  -	Adapter: Pointer to the Adapter structure
> -*			   	uiSfid : Given SFID for matching
> +*				uiSfid : Given SFID for matching
>  *
>  * Returns	  - Queue index for this SFID(If matched)
>  				Else Invalid Queue Index(If Not matched)
>  ************************************************************/
> -INT SearchSfid(PMINI_ADAPTER Adapter,UINT uiSfid)
> +INT SearchSfid(PMINI_ADAPTER Adapter, UINT uiSfid)
>  {
> -	INT 	iIndex=0;
> -	for(iIndex=(NO_OF_QUEUES-1); iIndex>=0; iIndex--)
> -		if(Adapter->PackInfo[iIndex].ulSFID==uiSfid)
> +	INT iIndex = 0;

blank line goes here.
INT should be changed to "int"
No need to initialize iIndex.

> +	for (iIndex = (NO_OF_QUEUES - 1); iIndex >= 0; iIndex--)
> +		if (Adapter->PackInfo[iIndex].ulSFID == uiSfid)
>  			return iIndex;

But curly braces around multi-line indent blocks for style reasons
even though it's not needed for semantic reasons.

> -	return NO_OF_QUEUES+1;
> +	return NO_OF_QUEUES + 1;

Anyway, this whole thing needs to be broken up into smaller logical
patches and resent.  Like maybe change fix add spaces around the
operators in one patch or something like that.  Fix one class of
checkpatch.pl warning at a time.

Don't worry that your first patch was not accepted.  Everyone's first
patch gets rejected.  :P

regards,
dan carpenter


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

      reply	other threads:[~2011-11-09 21:00 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-11-07 23:55 [PATCH] I checked with chechpatch.pl the archive in drivers/staging/bcm/CmHost.c , and cleanup the Errors and warnings, I didn't change any function, only code style Tresca Agustin
2011-11-09 21:00 ` Dan Carpenter [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=20111109210056.GV4751@mwanda \
    --to=dan.carpenter@oracle.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@suse.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=treskita@gmail.com \
    --cc=wfp5p@virginia.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