* [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
@ 2015-09-26 16:49 Punit Vara
2015-09-26 17:45 ` Larry Finger
0 siblings, 1 reply; 12+ messages in thread
From: Punit Vara @ 2015-09-26 16:49 UTC (permalink / raw)
To: Larry.Finger
Cc: florian.c.schilhabel, gregkh, sudipm.mukherjee, devel,
linux-kernel, dogukan.ergun, stillcompiling, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
2015-09-26 16:49 [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision Punit Vara
@ 2015-09-26 17:45 ` Larry Finger
2015-09-26 17:54 ` punit vara
2015-09-28 11:18 ` Sudip Mukherjee
0 siblings, 2 replies; 12+ messages in thread
From: Larry Finger @ 2015-09-26 17:45 UTC (permalink / raw)
To: Punit Vara
Cc: florian.c.schilhabel, gregkh, sudipm.mukherjee, devel,
linux-kernel, dogukan.ergun, stillcompiling, Joe Perches
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
2015-09-26 17:45 ` Larry Finger
@ 2015-09-26 17:54 ` punit vara
2015-09-26 18:11 ` Joshua Clayton
2015-09-28 8:35 ` Dan Carpenter
2015-09-28 11:18 ` Sudip Mukherjee
1 sibling, 2 replies; 12+ messages in thread
From: punit vara @ 2015-09-26 17:54 UTC (permalink / raw)
To: Larry Finger
Cc: Flo Schil, Greg KH, Sudip Mukherjee, devel, linux-kernel,
dogukan.ergun, stillcompiling, Joe Perches
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 ?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
2015-09-26 17:54 ` punit vara
@ 2015-09-26 18:11 ` Joshua Clayton
2015-09-28 8:35 ` Dan Carpenter
1 sibling, 0 replies; 12+ messages in thread
From: Joshua Clayton @ 2015-09-26 18:11 UTC (permalink / raw)
To: punit vara
Cc: Larry Finger, Flo Schil, Greg KH, Sudip Mukherjee, devel,
linux-kernel, dogukan.ergun, Joe Perches
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
2015-09-26 17:54 ` punit vara
2015-09-26 18:11 ` Joshua Clayton
@ 2015-09-28 8:35 ` Dan Carpenter
1 sibling, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2015-09-28 8:35 UTC (permalink / raw)
To: punit vara
Cc: Larry Finger, devel, Flo Schil, stillcompiling, Greg KH,
linux-kernel, Sudip Mukherjee, Joe Perches, dogukan.ergun
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
2015-09-26 17:45 ` Larry Finger
2015-09-26 17:54 ` punit vara
@ 2015-09-28 11:18 ` Sudip Mukherjee
2015-09-28 13:46 ` punit vara
2015-09-28 14:56 ` Larry Finger
1 sibling, 2 replies; 12+ messages in thread
From: Sudip Mukherjee @ 2015-09-28 11:18 UTC (permalink / raw)
To: Larry Finger
Cc: Punit Vara, florian.c.schilhabel, gregkh, devel, linux-kernel,
dogukan.ergun, stillcompiling, Joe Perches
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
2015-09-28 11:18 ` Sudip Mukherjee
@ 2015-09-28 13:46 ` punit vara
2015-09-28 14:56 ` Larry Finger
1 sibling, 0 replies; 12+ messages in thread
From: punit vara @ 2015-09-28 13:46 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: Larry Finger, Flo Schil, Greg KH, devel, linux-kernel,
Dogukan Ergun, Joshua Clayton, Joe Perches
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
2015-09-28 11:18 ` Sudip Mukherjee
2015-09-28 13:46 ` punit vara
@ 2015-09-28 14:56 ` Larry Finger
2015-09-28 15:19 ` Sudip Mukherjee
2015-09-29 20:24 ` Tillmann Heidsieck
1 sibling, 2 replies; 12+ messages in thread
From: Larry Finger @ 2015-09-28 14:56 UTC (permalink / raw)
To: Sudip Mukherjee
Cc: Punit Vara, florian.c.schilhabel, gregkh, devel, linux-kernel,
dogukan.ergun, stillcompiling, Joe Perches
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
2015-09-28 14:56 ` Larry Finger
@ 2015-09-28 15:19 ` Sudip Mukherjee
2015-09-29 20:24 ` Tillmann Heidsieck
1 sibling, 0 replies; 12+ messages in thread
From: Sudip Mukherjee @ 2015-09-28 15:19 UTC (permalink / raw)
To: Larry Finger
Cc: Punit Vara, florian.c.schilhabel, gregkh, devel, linux-kernel,
dogukan.ergun, stillcompiling, Joe Perches
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
2015-09-28 14:56 ` Larry Finger
2015-09-28 15:19 ` Sudip Mukherjee
@ 2015-09-29 20:24 ` Tillmann Heidsieck
2015-09-30 9:34 ` Dan Carpenter
1 sibling, 1 reply; 12+ messages in thread
From: Tillmann Heidsieck @ 2015-09-29 20:24 UTC (permalink / raw)
To: Larry Finger
Cc: Sudip Mukherjee, Punit Vara, florian.c.schilhabel, gregkh, devel,
linux-kernel, dogukan.ergun, stillcompiling, Joe Perches
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
2015-09-29 20:24 ` Tillmann Heidsieck
@ 2015-09-30 9:34 ` Dan Carpenter
2015-10-09 14:54 ` Dan Carpenter
0 siblings, 1 reply; 12+ messages in thread
From: Dan Carpenter @ 2015-09-30 9:34 UTC (permalink / raw)
To: Tillmann Heidsieck
Cc: Larry Finger, devel, florian.c.schilhabel, Punit Vara,
stillcompiling, gregkh, dogukan.ergun, linux-kernel, Joe Perches,
Sudip Mukherjee
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision
2015-09-30 9:34 ` Dan Carpenter
@ 2015-10-09 14:54 ` Dan Carpenter
0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2015-10-09 14:54 UTC (permalink / raw)
To: Tillmann Heidsieck
Cc: devel, florian.c.schilhabel, Punit Vara, stillcompiling, gregkh,
linux-kernel, Sudip Mukherjee, Joe Perches, dogukan.ergun,
Larry Finger
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-10-09 14:54 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-26 16:49 [PATCH] Staging: rtl8712: rtl871x_ioctl_linux.c Move constant to right side of the comparision Punit Vara
2015-09-26 17:45 ` Larry Finger
2015-09-26 17:54 ` punit vara
2015-09-26 18:11 ` Joshua Clayton
2015-09-28 8:35 ` Dan Carpenter
2015-09-28 11:18 ` Sudip Mukherjee
2015-09-28 13:46 ` punit vara
2015-09-28 14:56 ` Larry Finger
2015-09-28 15:19 ` Sudip Mukherjee
2015-09-29 20:24 ` Tillmann Heidsieck
2015-09-30 9:34 ` Dan Carpenter
2015-10-09 14:54 ` Dan Carpenter
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).