[PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[Punit Vara]

This patch is to the rtl871x_ioctl_linux.c that fixes up following
warning reported by checkpatch.pl :

- Comparisons should place the constant on the right side of the test

Signed-off-by: Punit Vara <punitvara@gmail.com>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index 143be0f..6d7f8f7 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -957,7 +957,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
 	if (IS_ERR(ext))
 		return PTR_ERR(ext);
 
-	if (0 == strcasecmp(ext, "RSSI")) {
+	if (strcasecmp(ext, "RSSI") == 0) {
 		/*Return received signal strength indicator in -db for */
 		/* current AP */
 		/*<ssid> Rssi xx */
@@ -974,7 +974,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
 		} else {
 			sprintf(ext, "OK");
 		}
-	} else if (0 == strcasecmp(ext, "LINKSPEED")) {
+	} else if (strcasecmp(ext, "LINKSPEED") == 0) {
 		/*Return link speed in MBPS */
 		/*LinkSpeed xx */
 		union iwreq_data wrqd;
@@ -982,30 +982,30 @@ static int r871x_wx_set_priv(struct net_device *dev,
 		int mbps;
 
 		ret_inner = r8711_wx_get_rate(dev, info, &wrqd, extra);
-		if (0 != ret_inner)
+		if (ret_inner != 0)
 			mbps = 0;
 		else
 			mbps = wrqd.bitrate.value / 1000000;
 		sprintf(ext, "LINKSPEED %d", mbps);
-	} else if (0 == strcasecmp(ext, "MACADDR")) {
+	} else if (strcasecmp(ext, "MACADDR") == 0) {
 		/*Return mac address of the station */
 		/* Macaddr = xx:xx:xx:xx:xx:xx */
 		sprintf(ext, "MACADDR = %pM", dev->dev_addr);
-	} else if (0 == strcasecmp(ext, "SCAN-ACTIVE")) {
+	} else if (strcasecmp(ext, "SCAN-ACTIVE") == 0) {
 		/*Set scan type to active */
 		/*OK if successful */
 		struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
 		pmlmepriv->passive_mode = 1;
 		sprintf(ext, "OK");
-	} else if (0 == strcasecmp(ext, "SCAN-PASSIVE")) {
+	} else if (strcasecmp(ext, "SCAN-PASSIVE") == 0) {
 		/*Set scan type to passive */
 		/*OK if successful */
 		struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 
 		pmlmepriv->passive_mode = 0;
 		sprintf(ext, "OK");
-	} else if (0 == strncmp(ext, "DCE-E", 5)) {
+	} else if (strncmp(ext, "DCE-E", 5) == 0) {
 		/*Set scan type to passive */
 		/*OK if successful */
 		r8712_disconnectCtrlEx_cmd(padapter
@@ -1015,7 +1015,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
 			, 5000 /*u32 firstStageTO */
 		);
 		sprintf(ext, "OK");
-	} else if (0 == strncmp(ext, "DCE-D", 5)) {
+	} else if (strncmp(ext, "DCE-D", 5) == 0) {
 		/*Set scan type to passive */
 		/*OK if successfu */
 		r8712_disconnectCtrlEx_cmd(padapter
-- 
2.5.2

Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[Larry Finger]

On 09/26/2015 11:49 AM, Punit Vara wrote:
> This patch is to the rtl871x_ioctl_linux.c that fixes up following
> warning reported by checkpatch.pl :
>
> - Comparisons should place the constant on the right side of the test
>
> Signed-off-by: Punit Vara <punitvara@gmail.com>

This warning is crap. WTF difference does it make???? The compiler does not 
care, and any reader with any piece of a brain is not going to be confused!

This patch and all others like it are just meaningless source churning!

This author has made such a royal mess of his patches that I recommend that ALL 
of them be dropped. In addition, we should continue to drop his changes until he 
learns how to use git to generate N/M patches, and until he reads the 
documentation on patch submission.

Larry

> ---
>   drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> index 143be0f..6d7f8f7 100644
> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> @@ -957,7 +957,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
>   	if (IS_ERR(ext))
>   		return PTR_ERR(ext);
>
> -	if (0 == strcasecmp(ext, "RSSI")) {
> +	if (strcasecmp(ext, "RSSI") == 0) {
>   		/*Return received signal strength indicator in -db for */
>   		/* current AP */
>   		/*<ssid> Rssi xx */
> @@ -974,7 +974,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
>   		} else {
>   			sprintf(ext, "OK");
>   		}
> -	} else if (0 == strcasecmp(ext, "LINKSPEED")) {
> +	} else if (strcasecmp(ext, "LINKSPEED") == 0) {
>   		/*Return link speed in MBPS */
>   		/*LinkSpeed xx */
>   		union iwreq_data wrqd;
> @@ -982,30 +982,30 @@ static int r871x_wx_set_priv(struct net_device *dev,
>   		int mbps;
>
>   		ret_inner = r8711_wx_get_rate(dev, info, &wrqd, extra);
> -		if (0 != ret_inner)
> +		if (ret_inner != 0)
>   			mbps = 0;
>   		else
>   			mbps = wrqd.bitrate.value / 1000000;
>   		sprintf(ext, "LINKSPEED %d", mbps);
> -	} else if (0 == strcasecmp(ext, "MACADDR")) {
> +	} else if (strcasecmp(ext, "MACADDR") == 0) {
>   		/*Return mac address of the station */
>   		/* Macaddr = xx:xx:xx:xx:xx:xx */
>   		sprintf(ext, "MACADDR = %pM", dev->dev_addr);
> -	} else if (0 == strcasecmp(ext, "SCAN-ACTIVE")) {
> +	} else if (strcasecmp(ext, "SCAN-ACTIVE") == 0) {
>   		/*Set scan type to active */
>   		/*OK if successful */
>   		struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
>
>   		pmlmepriv->passive_mode = 1;
>   		sprintf(ext, "OK");
> -	} else if (0 == strcasecmp(ext, "SCAN-PASSIVE")) {
> +	} else if (strcasecmp(ext, "SCAN-PASSIVE") == 0) {
>   		/*Set scan type to passive */
>   		/*OK if successful */
>   		struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
>
>   		pmlmepriv->passive_mode = 0;
>   		sprintf(ext, "OK");
> -	} else if (0 == strncmp(ext, "DCE-E", 5)) {
> +	} else if (strncmp(ext, "DCE-E", 5) == 0) {
>   		/*Set scan type to passive */
>   		/*OK if successful */
>   		r8712_disconnectCtrlEx_cmd(padapter
> @@ -1015,7 +1015,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
>   			, 5000 /*u32 firstStageTO */
>   		);
>   		sprintf(ext, "OK");
> -	} else if (0 == strncmp(ext, "DCE-D", 5)) {
> +	} else if (strncmp(ext, "DCE-D", 5) == 0) {
>   		/*Set scan type to passive */
>   		/*OK if successfu */
>   		r8712_disconnectCtrlEx_cmd(padapter
>

Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[punit vara]

On Sat, Sep 26, 2015 at 11:15 PM, Larry Finger
<Larry.Finger@lwfinger.net> wrote:
> On 09/26/2015 11:49 AM, Punit Vara wrote:
>>
>> This patch is to the rtl871x_ioctl_linux.c that fixes up following
>> warning reported by checkpatch.pl :
>>
>> - Comparisons should place the constant on the right side of the test
>>
>> Signed-off-by: Punit Vara <punitvara@gmail.com>
>
>
> This warning is crap. WTF difference does it make???? The compiler does not
> care, and any reader with any piece of a brain is not going to be confused!
>
> This patch and all others like it are just meaningless source churning!
>
> This author has made such a royal mess of his patches that I recommend that
> ALL of them be dropped. In addition, we should continue to drop his changes
> until he learns how to use git to generate N/M patches, and until he reads
> the documentation on patch submission.
>
> Larry
>
>
>> ---
>>   drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> index 143be0f..6d7f8f7 100644
>> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
>> @@ -957,7 +957,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
>>         if (IS_ERR(ext))
>>                 return PTR_ERR(ext);
>>
>> -       if (0 == strcasecmp(ext, "RSSI")) {
>> +       if (strcasecmp(ext, "RSSI") == 0) {
>>                 /*Return received signal strength indicator in -db for */
>>                 /* current AP */
>>                 /*<ssid> Rssi xx */
>> @@ -974,7 +974,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
>>                 } else {
>>                         sprintf(ext, "OK");
>>                 }
>> -       } else if (0 == strcasecmp(ext, "LINKSPEED")) {
>> +       } else if (strcasecmp(ext, "LINKSPEED") == 0) {
>>                 /*Return link speed in MBPS */
>>                 /*LinkSpeed xx */
>>                 union iwreq_data wrqd;
>> @@ -982,30 +982,30 @@ static int r871x_wx_set_priv(struct net_device *dev,
>>                 int mbps;
>>
>>                 ret_inner = r8711_wx_get_rate(dev, info, &wrqd, extra);
>> -               if (0 != ret_inner)
>> +               if (ret_inner != 0)
>>                         mbps = 0;
>>                 else
>>                         mbps = wrqd.bitrate.value / 1000000;
>>                 sprintf(ext, "LINKSPEED %d", mbps);
>> -       } else if (0 == strcasecmp(ext, "MACADDR")) {
>> +       } else if (strcasecmp(ext, "MACADDR") == 0) {
>>                 /*Return mac address of the station */
>>                 /* Macaddr = xx:xx:xx:xx:xx:xx */
>>                 sprintf(ext, "MACADDR = %pM", dev->dev_addr);
>> -       } else if (0 == strcasecmp(ext, "SCAN-ACTIVE")) {
>> +       } else if (strcasecmp(ext, "SCAN-ACTIVE") == 0) {
>>                 /*Set scan type to active */
>>                 /*OK if successful */
>>                 struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
>>
>>                 pmlmepriv->passive_mode = 1;
>>                 sprintf(ext, "OK");
>> -       } else if (0 == strcasecmp(ext, "SCAN-PASSIVE")) {
>> +       } else if (strcasecmp(ext, "SCAN-PASSIVE") == 0) {
>>                 /*Set scan type to passive */
>>                 /*OK if successful */
>>                 struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
>>
>>                 pmlmepriv->passive_mode = 0;
>>                 sprintf(ext, "OK");
>> -       } else if (0 == strncmp(ext, "DCE-E", 5)) {
>> +       } else if (strncmp(ext, "DCE-E", 5) == 0) {
>>                 /*Set scan type to passive */
>>                 /*OK if successful */
>>                 r8712_disconnectCtrlEx_cmd(padapter
>> @@ -1015,7 +1015,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
>>                         , 5000 /*u32 firstStageTO */
>>                 );
>>                 sprintf(ext, "OK");
>> -       } else if (0 == strncmp(ext, "DCE-D", 5)) {
>> +       } else if (strncmp(ext, "DCE-D", 5) == 0) {
>>                 /*Set scan type to passive */
>>                 /*OK if successfu */
>>                 r8712_disconnectCtrlEx_cmd(padapter
>>
>

Sorry I have disappointed you .I learned now how to generate N/M
patches .I have recently sent my last 6 patches in that way only.   I
am extremely sorry for previous patches as I could not able to send in
series. Please let me allow to send me those patches again in correct
way. If you would say I will definitely send them in series.Can I send
all the patches in series for rtl8712 directory if you do not mind ?

Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[Joshua Clayton]

On Saturday, September 26, 2015 11:24:06 PM punit vara wrote:
> On Sat, Sep 26, 2015 at 11:15 PM, Larry Finger
> <Larry.Finger@lwfinger.net> wrote:
> > On 09/26/2015 11:49 AM, Punit Vara wrote:
> >>
> >> This patch is to the rtl871x_ioctl_linux.c that fixes up following
> >> warning reported by checkpatch.pl :
> >>
> >> - Comparisons should place the constant on the right side of the test
> >>
> >> Signed-off-by: Punit Vara <punitvara@gmail.com>
> >
> >
> > This warning is crap. WTF difference does it make???? The compiler does not
> > care, and any reader with any piece of a brain is not going to be confused!
> >
> > This patch and all others like it are just meaningless source churning!
> >
> > This author has made such a royal mess of his patches that I recommend that
> > ALL of them be dropped. In addition, we should continue to drop his changes
> > until he learns how to use git to generate N/M patches, and until he reads
> > the documentation on patch submission.
> >
> > Larry
> >
> >
> >> ---
> >>   drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 16 ++++++++--------
> >>   1 file changed, 8 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> index 143be0f..6d7f8f7 100644
> >> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> >> @@ -957,7 +957,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
> >>         if (IS_ERR(ext))
> >>                 return PTR_ERR(ext);
> >>
> >> -       if (0 == strcasecmp(ext, "RSSI")) {
> >> +       if (strcasecmp(ext, "RSSI") == 0) {
> >>                 /*Return received signal strength indicator in -db for */
> >>                 /* current AP */
> >>                 /*<ssid> Rssi xx */
> >> @@ -974,7 +974,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
> >>                 } else {
> >>                         sprintf(ext, "OK");
> >>                 }
> >> -       } else if (0 == strcasecmp(ext, "LINKSPEED")) {
> >> +       } else if (strcasecmp(ext, "LINKSPEED") == 0) {
> >>                 /*Return link speed in MBPS */
> >>                 /*LinkSpeed xx */
> >>                 union iwreq_data wrqd;
> >> @@ -982,30 +982,30 @@ static int r871x_wx_set_priv(struct net_device *dev,
> >>                 int mbps;
> >>
> >>                 ret_inner = r8711_wx_get_rate(dev, info, &wrqd, extra);
> >> -               if (0 != ret_inner)
> >> +               if (ret_inner != 0)
> >>                         mbps = 0;
> >>                 else
> >>                         mbps = wrqd.bitrate.value / 1000000;
> >>                 sprintf(ext, "LINKSPEED %d", mbps);
> >> -       } else if (0 == strcasecmp(ext, "MACADDR")) {
> >> +       } else if (strcasecmp(ext, "MACADDR") == 0) {
> >>                 /*Return mac address of the station */
> >>                 /* Macaddr = xx:xx:xx:xx:xx:xx */
> >>                 sprintf(ext, "MACADDR = %pM", dev->dev_addr);
> >> -       } else if (0 == strcasecmp(ext, "SCAN-ACTIVE")) {
> >> +       } else if (strcasecmp(ext, "SCAN-ACTIVE") == 0) {
> >>                 /*Set scan type to active */
> >>                 /*OK if successful */
> >>                 struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
> >>
> >>                 pmlmepriv->passive_mode = 1;
> >>                 sprintf(ext, "OK");
> >> -       } else if (0 == strcasecmp(ext, "SCAN-PASSIVE")) {
> >> +       } else if (strcasecmp(ext, "SCAN-PASSIVE") == 0) {
> >>                 /*Set scan type to passive */
> >>                 /*OK if successful */
> >>                 struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
> >>
> >>                 pmlmepriv->passive_mode = 0;
> >>                 sprintf(ext, "OK");
> >> -       } else if (0 == strncmp(ext, "DCE-E", 5)) {
> >> +       } else if (strncmp(ext, "DCE-E", 5) == 0) {
> >>                 /*Set scan type to passive */
> >>                 /*OK if successful */
> >>                 r8712_disconnectCtrlEx_cmd(padapter
> >> @@ -1015,7 +1015,7 @@ static int r871x_wx_set_priv(struct net_device *dev,
> >>                         , 5000 /*u32 firstStageTO */
> >>                 );
> >>                 sprintf(ext, "OK");
> >> -       } else if (0 == strncmp(ext, "DCE-D", 5)) {
> >> +       } else if (strncmp(ext, "DCE-D", 5) == 0) {
> >>                 /*Set scan type to passive */
> >>                 /*OK if successfu */
> >>                 r8712_disconnectCtrlEx_cmd(padapter
> >>
> >
> 
> Sorry I have disappointed you .I learned now how to generate N/M
> patches .I have recently sent my last 6 patches in that way only.   I
> am extremely sorry for previous patches as I could not able to send in
> series. Please let me allow to send me those patches again in correct
> way. If you would say I will definitely send them in series.Can I send
> all the patches in series for rtl8712 directory if you do not mind ?
I have not seen your other patches, but I agree with Larry. This is a formatting patch
that  does not improve the readability of the code. Should be dropped.
This particular checkpatch warning might be useful in expressing a preference for new code, I guess.

Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[Dan Carpenter]

Don't worry too much.  I think your patches are basically fine.  I'm a
fairly experience kernel dev but I don't know what an N/M patch is...

Just don't work on staging/rtl* any more.  The rest of staging is fine.

regards,
dan carpenter

Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[Sudip Mukherjee]

On Sat, Sep 26, 2015 at 12:45:46PM -0500, Larry Finger wrote:
> On 09/26/2015 11:49 AM, Punit Vara wrote:
> >This patch is to the rtl871x_ioctl_linux.c that fixes up following
> >warning reported by checkpatch.pl :
> >
> >- Comparisons should place the constant on the right side of the test
> >
> >Signed-off-by: Punit Vara <punitvara@gmail.com>
> 
> This warning is crap. WTF difference does it make???? The compiler
> does not care, and any reader with any piece of a brain is not going
> to be confused!
> 
> This patch and all others like it are just meaningless source churning!
> 
> This author has made such a royal mess of his patches that I
> recommend that ALL of them be dropped. In addition, we should
> continue to drop his changes until he learns how to use git to
> generate N/M patches, and until he reads the documentation on patch
> submission.
Excuse me for my ignorance, but I still can not see what was wrong with
his patch. checkpatch is giving warning and he has fixed it. As far as
sending in series is concerned, he is a newbie and after telling him how
to generate patches in series he has learnt that. I have already told
him that his patches might be dropped as they are not in series and he
is ready to resend in series as soon as Greg confirms that they are
dropped. And as long as the driver is in staging there will be source
churning, isn't it?
If i remember correctly I was told that for a driver to be moved out of
staging the primary thing is that all checkpatch warnings needs to fixed.
So if this driver has to move out of staging someday then these warnings
also has to be fixed by someone.

regards
sudip

Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[punit vara]

On Mon, Sep 28, 2015 at 4:48 PM, Sudip Mukherjee
<sudipm.mukherjee@gmail.com> wrote:
> On Sat, Sep 26, 2015 at 12:45:46PM -0500, Larry Finger wrote:
>> On 09/26/2015 11:49 AM, Punit Vara wrote:
>> >This patch is to the rtl871x_ioctl_linux.c that fixes up following
>> >warning reported by checkpatch.pl :
>> >
>> >- Comparisons should place the constant on the right side of the test
>> >
>> >Signed-off-by: Punit Vara <punitvara@gmail.com>
>>
>> This warning is crap. WTF difference does it make???? The compiler
>> does not care, and any reader with any piece of a brain is not going
>> to be confused!
>>
>> This patch and all others like it are just meaningless source churning!
>>
>> This author has made such a royal mess of his patches that I
>> recommend that ALL of them be dropped. In addition, we should
>> continue to drop his changes until he learns how to use git to
>> generate N/M patches, and until he reads the documentation on patch
>> submission.
> Excuse me for my ignorance, but I still can not see what was wrong with
> his patch. checkpatch is giving warning and he has fixed it. As far as
> sending in series is concerned, he is a newbie and after telling him how
> to generate patches in series he has learnt that. I have already told
> him that his patches might be dropped as they are not in series and he
> is ready to resend in series as soon as Greg confirms that they are
> dropped. And as long as the driver is in staging there will be source
> churning, isn't it?
> If i remember correctly I was told that for a driver to be moved out of
> staging the primary thing is that all checkpatch warnings needs to fixed.
> So if this driver has to move out of staging someday then these warnings
> also has to be fixed by someone.
>
> regards
> sudip

Thank you for understanding my situation @Sudip @Dan Carpenter ..
First I have planned to clean all the warning whatever it may be silly
or some serious. So that by cleaning those I can easily understand the
whole process . I was thinking to dig into TODO list and followed by
writing any driver. Greg please confirm me what should I do ? Do i
touch any rtl* ? It is first time I am working with this much huge
project .So may be I dont know what warnings are priority and what are
not ? I felt proud when my first patch was accepted .Execuse me @Larry
@Joshua if I irritate you by sending so many patches in sequence but I
am happy to clean all the staging driver warning first. So that every
programmer can focus on programming only not on cleanpatch warnings .

Regards,
Punit

Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[Larry Finger]

On 09/28/2015 06:18 AM, Sudip Mukherjee wrote:
> On Sat, Sep 26, 2015 at 12:45:46PM -0500, Larry Finger wrote:
>> On 09/26/2015 11:49 AM, Punit Vara wrote:
>>> This patch is to the rtl871x_ioctl_linux.c that fixes up following
>>> warning reported by checkpatch.pl :
>>>
>>> - Comparisons should place the constant on the right side of the test
>>>
>>> Signed-off-by: Punit Vara <punitvara@gmail.com>
>>
>> This warning is crap. WTF difference does it make???? The compiler
>> does not care, and any reader with any piece of a brain is not going
>> to be confused!
>>
>> This patch and all others like it are just meaningless source churning!
>>
>> This author has made such a royal mess of his patches that I
>> recommend that ALL of them be dropped. In addition, we should
>> continue to drop his changes until he learns how to use git to
>> generate N/M patches, and until he reads the documentation on patch
>> submission.
> Excuse me for my ignorance, but I still can not see what was wrong with
> his patch. checkpatch is giving warning and he has fixed it. As far as
> sending in series is concerned, he is a newbie and after telling him how
> to generate patches in series he has learnt that. I have already told
> him that his patches might be dropped as they are not in series and he
> is ready to resend in series as soon as Greg confirms that they are
> dropped. And as long as the driver is in staging there will be source
> churning, isn't it?
> If i remember correctly I was told that for a driver to be moved out of
> staging the primary thing is that all checkpatch warnings needs to fixed.
> So if this driver has to move out of staging someday then these warnings
> also has to be fixed by someone.

The primary requirement for moving a driver out of staging is that it use 
mac80211. No amount of cosmetic fixing will ever make that change for rtl8712u!

In my opinion, not ALL checkpatch warnings ever need to be heeded.Can you tell 
me why

/**
  * This is a comment
  */

is superior to

/*
   This is a comment also
  */

The difference is not significant, yet checkpatch treats it as though the source 
was horribly flawed. Similarly, satisfying the 80-character requirement can 
leave the code horribly unreadable.

My main complaint is that the OP submitted dozens of patches with similar 
subjects, yet gave no indication is any of these were resubmissions, and 
completely failed to utilize the patch series mechanism of git. This behavior 
makes life difficult for both the maintainer and the reviewer.

Larry

Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[Sudip Mukherjee]

On Mon, Sep 28, 2015 at 09:56:19AM -0500, Larry Finger wrote:
> On 09/28/2015 06:18 AM, Sudip Mukherjee wrote:
> >On Sat, Sep 26, 2015 at 12:45:46PM -0500, Larry Finger wrote:
> >>On 09/26/2015 11:49 AM, Punit Vara wrote:
> >>>This patch is to the rtl871x_ioctl_linux.c that fixes up following
> >>>warning reported by checkpatch.pl :
> >>>
> >>>- Comparisons should place the constant on the right side of the test
> >>>
> >>>Signed-off-by: Punit Vara <punitvara@gmail.com>
> >>
> >>This warning is crap. WTF difference does it make???? The compiler
> >>does not care, and any reader with any piece of a brain is not going
> >>to be confused!
> >>
> >>This patch and all others like it are just meaningless source churning!
> >>
> >>This author has made such a royal mess of his patches that I
> >>recommend that ALL of them be dropped. In addition, we should
> >>continue to drop his changes until he learns how to use git to
> >>generate N/M patches, and until he reads the documentation on patch
> >>submission.
> >Excuse me for my ignorance, but I still can not see what was wrong with
> >his patch. checkpatch is giving warning and he has fixed it. As far as
> >sending in series is concerned, he is a newbie and after telling him how
> >to generate patches in series he has learnt that. I have already told
> >him that his patches might be dropped as they are not in series and he
> >is ready to resend in series as soon as Greg confirms that they are
> >dropped. And as long as the driver is in staging there will be source
> >churning, isn't it?
> >If i remember correctly I was told that for a driver to be moved out of
> >staging the primary thing is that all checkpatch warnings needs to fixed.
> >So if this driver has to move out of staging someday then these warnings
> >also has to be fixed by someone.
> 
> The primary requirement for moving a driver out of staging is that
> it use mac80211. No amount of cosmetic fixing will ever make that
> change for rtl8712u!
Yes, ofcourse. But you will also have to admit that since this is in
staging and newbies are encouraged to first work with staging to fix
checkpatch warnings, so you will get these kind of patches. Outreachy is
starting from tomorrow and I am sure Greg will be flooded with these
kind of patches.
> 
> In my opinion, not ALL checkpatch warnings ever need to be
> heeded.Can you tell me why
> 
> /**
>  * This is a comment
>  */
> 
> is superior to
> 
> /*
>   This is a comment also
>  */
> 
> The difference is not significant, yet checkpatch treats it as
> though the source was horribly flawed. Similarly, satisfying the
> 80-character requirement can leave the code horribly unreadable.
Yes, I admit. Just recently I submited some memory leak patches to another
subsystem and there I had more than 80 char, so just kept a comment that
this warning is not taken care of as it improves readability. But like I
just told this is staging and unless all checkpatch warnings are fixed
someone or the other will continue to send these patches.
> 
> My main complaint is that the OP submitted dozens of patches with
> similar subjects, yet gave no indication is any of these were
> resubmissions, and completely failed to utilize the patch series
> mechanism of git. This behavior makes life difficult for both the
> maintainer and the reviewer.
Yes. True. Even some time he has sent exactly same patch more than one
time, sometimes a new version with no version in subject. That was
confusing. But again he is a newbie. By the time he was informed to send
in series he has already sent lots of patches. I understand your
irritation for that but again he is a newbie. So first few mistakes are
excusable. Isn't it?

regards
sudip

Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[Tillmann Heidsieck]

On Mon, Sep 28, 2015 at 09:56:19AM -0500, Larry Finger wrote:
> The primary requirement for moving a driver out of staging is that it use
> mac80211. No amount of cosmetic fixing will ever make that change for
> rtl8712u!

As I am pretty sure someone has done something like this before, is
there a best practice write-up on this? Or a driver that is recommended
for usage as a template?

Regards
Tillmann

Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[Dan Carpenter]

On Tue, Sep 29, 2015 at 10:24:59PM +0200, Tillmann Heidsieck wrote:
> On Mon, Sep 28, 2015 at 09:56:19AM -0500, Larry Finger wrote:
> > The primary requirement for moving a driver out of staging is that it use
> > mac80211. No amount of cosmetic fixing will ever make that change for
> > rtl8712u!
> 
> As I am pretty sure someone has done something like this before, is
> there a best practice write-up on this? Or a driver that is recommended
> for usage as a template?

That's a good question.  The rtl* drivers are basically a dozen copies
of the same code.  I think we have had a few escape from staging?

regards,
dan carpenter

Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision

[Dan Carpenter]

On Wed, Sep 30, 2015 at 12:34:33PM +0300, Dan Carpenter wrote:
> On Tue, Sep 29, 2015 at 10:24:59PM +0200, Tillmann Heidsieck wrote:
> > On Mon, Sep 28, 2015 at 09:56:19AM -0500, Larry Finger wrote:
> > > The primary requirement for moving a driver out of staging is that it use
> > > mac80211. No amount of cosmetic fixing will ever make that change for
> > > rtl8712u!
> > 
> > As I am pretty sure someone has done something like this before, is
> > there a best practice write-up on this? Or a driver that is recommended
> > for usage as a template?
> 
> That's a good question.  The rtl* drivers are basically a dozen copies
> of the same code.  I think we have had a few escape from staging?
> 

There have several realtek drivers which have escaped staging.  All of
them seem to have been re-writes from scratch.  Three of them were wifi
drivers.

rtl8187se: 5ed0a8e66709 ('staging: delete rtl8187se wireless driver')
rtl8192ee: f823182bc289 ('staging: r8192ee: Remove staging driver')
rtl8821ae: 76272ab3f348 ('staging: rtl8821ae: remove driver')

It feels like cleaning up rtl drivers is a waste of time except for the
churn.  Staging was deliberately designed to encourage churn so that a
lot of newbies could get experience sending patches.  It would have been
easy enough to run new drivers through checkpatch.pl --fix or whatever
but churn was part of the point from square one.

regards,
dan carpenter