Search Linux Wireless

Re: [PATCH v2 24/26] staging: brcm80211: declared global vars in softmac phy as const

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 09/27/2011 04:10 PM, Arend van Spriel wrote:
On 09/27/2011 08:35 PM, Larry Finger wrote:
On 09/27/2011 12:45 PM, Franky Lin wrote:
From: Roland Vossen<rvossen@xxxxxxxxxxxx>

Global variables are undesirable unless they are read only.

Reported-by: Johannes Berg<johannes@xxxxxxxxxxxxxxxx>
Reviewed-by: Pieter-Paul Giesberts<pieterpg@xxxxxxxxxxxx>
Reviewed-by: Arend van Spriel<arend@xxxxxxxxxxxx>
Signed-off-by: Franky Lin<frankyl@xxxxxxxxxxxx>
---
drivers/staging/brcm80211/brcmsmac/phy/phy_cmn.c | 16 +-----
drivers/staging/brcm80211/brcmsmac/phy/phy_int.h | 23 --------
drivers/staging/brcm80211/brcmsmac/phy/phy_lcn.c | 6 +-
drivers/staging/brcm80211/brcmsmac/phy/phy_n.c | 60 ++++++++++----------
.../staging/brcm80211/brcmsmac/phy/phytbl_lcn.c | 2 +-
5 files changed, 36 insertions(+), 71 deletions(-)

diff --git a/drivers/staging/brcm80211/brcmsmac/phy/phy_cmn.c
b/drivers/staging/brcm80211/brcmsmac/phy/phy_cmn.c
index f9702c0..5b81480 100644
--- a/drivers/staging/brcm80211/brcmsmac/phy/phy_cmn.c
+++ b/drivers/staging/brcm80211/brcmsmac/phy/phy_cmn.c
@@ -52,7 +52,7 @@ struct chan_info_basic {
u16 freq;
};

-static struct chan_info_basic chan_info_all[] = {
+static const struct chan_info_basic chan_info_all[] = {
{1, 2412},
{2, 2417},
{3, 2422},
What are you doing? You make this change here in patch 24, then undo it in #25,
and redo it in #26! Are you guys actually thinking about what you are doing?

We reordered the patches a little to move the ones we got feedback about at the
end for rework. My bet is that you see rebase at work although I agree this is
pretty tricky. I am glad the net result is that the const qualifier is added.
There was some thinking involved in what we did today ;-)

This needs to be fixed. In addition, the next time you submit this patch bomb,
change from V2 to V3. That is the standard way to handle resubmissions that are
addressing comments, not by increasing some extra counter in the 00/XX cover
patch. The VX notation helps the maintainer keep track of the patches as he
knows to drop all the V2 ones when V3 arrives. Maintainers are precious and must
be treated very carefully.

And here we were thinking we would have learned all tricks about submitting
patches in our staging year ;-)

So to be sure I have it right: Only the cover letter should say: [PATCH v2
00/20] with the same headline as the orignal. Or should each patch say [PATCH v2
xx/20] even when there are 26 patches?

No, the number of patches should always be correct, but I would go to V2 even if there are a different number of patches in the series than there were with V1.

There are as many ways to do it as there are kernel hackers - perhaps more. The main thing is to make is absolutely clear to the maintainer and any possible reviewers what is happening. Make certain that the patch subject is unique and have the cover patch spell out what is happening in great detail. That material is never included with the git commit, so you can say anything.

It would be best if your brcmsmac and brcmfmac patches are clearly distinguished in the subject line. That was what tripped me. I saw V2 for soft mac and thought I had seen V2 already. I had, but it was for full mac.

It would not have hurt if the cover letter of this series would indicate it
replaces the previous series by refering to its message-id. Would that be an
alternative approach or just a good addition.

That would be a good addition, but I would still keep the other description as well.

Larry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]
  Powered by Linux