* Re: [Patch] Staging: winbond: Made local functions static
2013-05-30 5:48 [Patch] Staging: winbond: Made local functions static Harsh Kumar
@ 2013-05-30 9:20 ` walter harms
2013-05-30 9:40 ` Pavel Machek
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: walter harms @ 2013-05-30 9:20 UTC (permalink / raw)
To: kernel-janitors
Am 30.05.2013 07:48, schrieb Harsh Kumar:
> Few functions are used only in one file. They are not included in any
> other .h or .c files (I used grep to check). They seem to be local functions.
> So, I have made them static.
> I have also inlined one function as it is a one line function.
>
I would leave that to the compiler (personal taste)
> Signed-off-by: Harsh Kumar <harsh1kumar@gmail.com>
>
> ---
> drivers/staging/winbond/reg.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff -uprN a/drivers/staging/winbond/reg.c b/drivers/staging/winbond/reg.c
> --- a/drivers/staging/winbond/reg.c 2013-05-28 00:52:20.000000000 +0530
> +++ b/drivers/staging/winbond/reg.c 2013-05-30 10:28:48.038634461 +0530
> @@ -920,20 +920,20 @@ void Uxx_power_on_procedure(struct hw_da
> Wb35Reg_WriteSync(pHwData, 0x03f8, 0x7ff);
> }
>
> -void Set_ChanIndep_RfData_al7230_24(struct hw_data *pHwData, u32 *pltmp , char number)
> +static void Set_ChanIndep_RfData_al7230_24(struct hw_data *pHwData, u32 *pltmp,
> + char number)
> {
> u8 i;
> -
> for (i = 0; i < number; i++) {
> pHwData->phy_para[i] = al7230_rf_data_24[i];
> pltmp[i] = (1 << 31) | (0 << 30) | (24 << 24) | (al7230_rf_data_24[i] & 0xffffff);
> }
> }
>
number seems strange, al7230_rf_data_24[] is global, you can use ARRAY_SIZE(al7230_rf_data_24) here.
just my 2 cents
re,
wh
> -void Set_ChanIndep_RfData_al7230_50(struct hw_data *pHwData, u32 *pltmp, char number)
> +static void Set_ChanIndep_RfData_al7230_50(struct hw_data *pHwData, u32 *pltmp,
> + char number)
> {
> u8 i;
> -
> for (i = 0; i < number; i++) {
> pHwData->phy_para[i] = al7230_rf_data_50[i];
> pltmp[i] = (1 << 31) | (0 << 30) | (24 << 24) | (al7230_rf_data_50[i] & 0xffffff);
> @@ -1263,7 +1263,7 @@ void RFSynthesizer_initial(struct hw_dat
> }
> }
>
> -void BBProcessor_AL7230_2400(struct hw_data *pHwData)
> +static void BBProcessor_AL7230_2400(struct hw_data *pHwData)
> {
> struct wb35_reg *reg = &pHwData->reg;
> u32 pltmp[12];
> @@ -1304,7 +1304,7 @@ void BBProcessor_AL7230_2400(struct hw_d
> Wb35Reg_BurstWrite(pHwData, 0x1030, pltmp, 12, AUTO_INCREMENT);
> }
>
> -void BBProcessor_AL7230_5000(struct hw_data *pHwData)
> +static void BBProcessor_AL7230_5000(struct hw_data *pHwData)
> {
> struct wb35_reg *reg = &pHwData->reg;
> u32 pltmp[12];
> @@ -1620,22 +1620,24 @@ void BBProcessor_initial(struct hw_data
> reg->SQ3_filter[i] = 0x2f; /* half of Bit 0 ~ 6 */
> }
>
> -void set_tx_power_per_channel_max2829(struct hw_data *pHwData, struct chan_info Channel)
> +static inline void set_tx_power_per_channel_max2829(struct hw_data *pHwData,
> + struct chan_info Channel)
> {
> RFSynthesizer_SetPowerIndex(pHwData, 100);
> }
>
> -void set_tx_power_per_channel_al2230(struct hw_data *pHwData, struct chan_info Channel)
> +static void set_tx_power_per_channel_al2230(struct hw_data *pHwData,
> + struct chan_info Channel)
> {
> u8 index = 100;
> -
> if (pHwData->TxVgaFor24[Channel.ChanNo - 1] != 0xff)
> index = pHwData->TxVgaFor24[Channel.ChanNo - 1];
>
> RFSynthesizer_SetPowerIndex(pHwData, index);
> }
>
> -void set_tx_power_per_channel_al7230(struct hw_data *pHwData, struct chan_info Channel)
> +static void set_tx_power_per_channel_al7230(struct hw_data *pHwData,
> + struct chan_info Channel)
> {
> u8 i, index = 100;
>
> @@ -1658,7 +1660,8 @@ void set_tx_power_per_channel_al7230(str
> RFSynthesizer_SetPowerIndex(pHwData, index);
> }
>
> -void set_tx_power_per_channel_wb242(struct hw_data *pHwData, struct chan_info Channel)
> +static void set_tx_power_per_channel_wb242(struct hw_data *pHwData,
> + struct chan_info Channel)
> {
> u8 index = 100;
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: Made local functions static
2013-05-30 5:48 [Patch] Staging: winbond: Made local functions static Harsh Kumar
2013-05-30 9:20 ` walter harms
@ 2013-05-30 9:40 ` Pavel Machek
2013-05-30 12:13 ` Dan Carpenter
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Pavel Machek @ 2013-05-30 9:40 UTC (permalink / raw)
To: kernel-janitors
On Thu 2013-05-30 11:18:12, Harsh Kumar wrote:
> Few functions are used only in one file. They are not included in any
> other .h or .c files (I used grep to check). They seem to be local functions.
> So, I have made them static.
> I have also inlined one function as it is a one line function.
>
> Signed-off-by: Harsh Kumar <harsh1kumar@gmail.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Thanks for the changes!
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: Made local functions static
2013-05-30 5:48 [Patch] Staging: winbond: Made local functions static Harsh Kumar
2013-05-30 9:20 ` walter harms
2013-05-30 9:40 ` Pavel Machek
@ 2013-05-30 12:13 ` Dan Carpenter
2013-05-30 12:18 ` Dan Carpenter
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2013-05-30 12:13 UTC (permalink / raw)
To: kernel-janitors
On Thu, May 30, 2013 at 11:18:12AM +0530, Harsh Kumar wrote:
> Few functions are used only in one file. They are not included in any
> other .h or .c files (I used grep to check). They seem to be local functions.
> So, I have made them static.
> I have also inlined one function as it is a one line function.
>
> Signed-off-by: Harsh Kumar <harsh1kumar@gmail.com>
>
> ---
> drivers/staging/winbond/reg.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff -uprN a/drivers/staging/winbond/reg.c b/drivers/staging/winbond/reg.c
> --- a/drivers/staging/winbond/reg.c 2013-05-28 00:52:20.000000000 +0530
> +++ b/drivers/staging/winbond/reg.c 2013-05-30 10:28:48.038634461 +0530
> @@ -920,20 +920,20 @@ void Uxx_power_on_procedure(struct hw_da
> Wb35Reg_WriteSync(pHwData, 0x03f8, 0x7ff);
> }
>
> -void Set_ChanIndep_RfData_al7230_24(struct hw_data *pHwData, u32 *pltmp , char number)
> +static void Set_ChanIndep_RfData_al7230_24(struct hw_data *pHwData, u32 *pltmp,
> + char number)
> {
> u8 i;
> -
Please leave the blank line here between the declarations and the
code. This is firmly established kernel standard, but it's probably
complicated to add it to checkpatch.pl...
Btw, this code uses u8 and char where int would be more appropriate.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: Made local functions static
2013-05-30 5:48 [Patch] Staging: winbond: Made local functions static Harsh Kumar
` (2 preceding siblings ...)
2013-05-30 12:13 ` Dan Carpenter
@ 2013-05-30 12:18 ` Dan Carpenter
2013-05-30 13:08 ` Dan Carpenter
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2013-05-30 12:18 UTC (permalink / raw)
To: kernel-janitors
On Thu, May 30, 2013 at 11:20:34AM +0200, walter harms wrote:
>
>
> Am 30.05.2013 07:48, schrieb Harsh Kumar:
> > Few functions are used only in one file. They are not included in any
> > other .h or .c files (I used grep to check). They seem to be local functions.
> > So, I have made them static.
> > I have also inlined one function as it is a one line function.
> >
> I would leave that to the compiler (personal taste)
>
Yes. Modern compilers basically ignore inline statements anyway.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: Made local functions static
2013-05-30 5:48 [Patch] Staging: winbond: Made local functions static Harsh Kumar
` (3 preceding siblings ...)
2013-05-30 12:18 ` Dan Carpenter
@ 2013-05-30 13:08 ` Dan Carpenter
2013-05-30 13:33 ` Harsh Kumar
2013-05-30 13:58 ` Dan Carpenter
6 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2013-05-30 13:08 UTC (permalink / raw)
To: kernel-janitors
Hi Harsh,
Here are some notes on the devel process.
You should probably include the driver devel list in the CC. I
always include one public development mailing list. Normally I
don't include LKML if I have another devel list CC'd.
In staging we're trying to merge patches as fast as we can so our
standards are slightly lower than things in the mm/ directory for
example.
Your last two patches have had too many unrelated whitespace changes
mixed in. They were small and this is staging so no one commented
on it. But it's best to avoid that.
When someone gives a review comment like my comment on u8 vs int or
Walter's comment on Set_ChanIndep_RfData_al7230_50(), that's not
stuff you introduced so it could be fixed later/ignored.
Walter's comment on "inline" is correct, but it's probably not a
"redo this patch" thing, it's more of a "think about this for later"
thing. In mm/ that kind of thing would be more serious, but this is
staging.
My comment on deleting blank lines is something that the patch
introduced. We'll need to put the blank lines back for this patch
to move out of staging. It would be better if you could send a v2
patch, but Pavel already Acked the patch and Greg may have merged
it.
Anyway, thank you for the patches. I hope you'll stick around. :)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: Made local functions static
2013-05-30 5:48 [Patch] Staging: winbond: Made local functions static Harsh Kumar
` (4 preceding siblings ...)
2013-05-30 13:08 ` Dan Carpenter
@ 2013-05-30 13:33 ` Harsh Kumar
2013-05-30 13:58 ` Dan Carpenter
6 siblings, 0 replies; 8+ messages in thread
From: Harsh Kumar @ 2013-05-30 13:33 UTC (permalink / raw)
To: kernel-janitors
On Thursday 30 May 2013 06:38 PM, Dan Carpenter wrote:
> Hi Harsh,
>
> Here are some notes on the devel process.
>
> You should probably include the driver devel list in the CC. I
> always include one public development mailing list. Normally I
> don't include LKML if I have another devel list CC'd.
>
> In staging we're trying to merge patches as fast as we can so our
> standards are slightly lower than things in the mm/ directory for
> example.
>
> Your last two patches have had too many unrelated whitespace changes
> mixed in. They were small and this is staging so no one commented
> on it. But it's best to avoid that.
>
> When someone gives a review comment like my comment on u8 vs int or
> Walter's comment on Set_ChanIndep_RfData_al7230_50(), that's not
> stuff you introduced so it could be fixed later/ignored.
>
> Walter's comment on "inline" is correct, but it's probably not a
> "redo this patch" thing, it's more of a "think about this for later"
> thing. In mm/ that kind of thing would be more serious, but this is
> staging.
>
Thanks for the guidance, I will take care of this in future.
> My comment on deleting blank lines is something that the patch
> introduced. We'll need to put the blank lines back for this patch
> to move out of staging. It would be better if you could send a v2
> patch, but Pavel already Acked the patch and Greg may have merged
> it.
Thanks for the clarifications Dan, I was not able to decide what
to do - submit a new patch or not.
The v2 patch will be over & above the changes that have been by my previous
patch, right?
Harsh
>
> Anyway, thank you for the patches. I hope you'll stick around. :)
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Patch] Staging: winbond: Made local functions static
2013-05-30 5:48 [Patch] Staging: winbond: Made local functions static Harsh Kumar
` (5 preceding siblings ...)
2013-05-30 13:33 ` Harsh Kumar
@ 2013-05-30 13:58 ` Dan Carpenter
6 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2013-05-30 13:58 UTC (permalink / raw)
To: kernel-janitors
On Thu, May 30, 2013 at 06:51:48PM +0530, Harsh Kumar wrote:
> The v2 patch will be over & above the changes that have been by my
> previous patch, right?
A v2 patch is a replacement for the first patch. There is a formula
to it:
1) Reply to the original patch.
2) The subject changes to [patch v2] Staging: winbond: Made local functions static
3) Use the same changelog as before.
4) After the signed off by line put:
---
v2: whitespace changes
That line gets chopped out of the commit message save in the git
log.
5) The same patch but without the whitespace changes
Btw, once Greg sends you an email that your patch has been applied
then it's too late to send a v2 patch and everything needs to be
done as a separate patch.
I feel like if Greg has already merged the patch then just forget
about it. We'll fix it up later. No worries. Patch review isn't
meant as a punishment for newbie stuff. ;)
regards,
dan carpenter
^ permalink raw reply [flat|nested] 8+ messages in thread