* [PATCH] staging: vt6655: Rename `by_preamble_type` parameter
@ 2021-10-20 8:40 Karolina Drobnik
2021-10-20 8:54 ` [Outreachy kernel] " Julia Lawall
2021-10-20 8:55 ` Greg KH
0 siblings, 2 replies; 6+ messages in thread
From: Karolina Drobnik @ 2021-10-20 8:40 UTC (permalink / raw)
To: outreachy-kernel
Cc: gregkh, forest, linux-staging, linux-kernel, Karolina Drobnik
Drop `by` prefix in the first parameter of `bb_get_frame_time` function.
As the original argument, `byPreambleType`, was renamed to `preamble_type`,
the parameter referring to it is now renamed to match the new naming
convention.
Update `bb_get_frame_time` comment to reflect that change.
This patch is a follow-up work to this commit:
Commit 548b6d7ebfa4 ("staging: vt6655: Rename byPreambleType field")
Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
---
drivers/staging/vt6655/baseband.c | 6 +++---
drivers/staging/vt6655/baseband.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/vt6655/baseband.c b/drivers/staging/vt6655/baseband.c
index 5efca92f1f18..8f9177db6663 100644
--- a/drivers/staging/vt6655/baseband.c
+++ b/drivers/staging/vt6655/baseband.c
@@ -1691,7 +1691,7 @@ static const unsigned short awc_frame_time[MAX_RATE] = {
*
* Parameters:
* In:
- * by_preamble_type - Preamble Type
+ * preamble_type - Preamble Type
* by_pkt_type - PK_TYPE_11A, PK_TYPE_11B, PK_TYPE_11GB, PK_TYPE_11GA
* cb_frame_length - Baseband Type
* tx_rate - Tx Rate
@@ -1700,7 +1700,7 @@ static const unsigned short awc_frame_time[MAX_RATE] = {
* Return Value: FrameTime
*
*/
-unsigned int bb_get_frame_time(unsigned char by_preamble_type,
+unsigned int bb_get_frame_time(unsigned char preamble_type,
unsigned char by_pkt_type,
unsigned int cb_frame_length,
unsigned short tx_rate)
@@ -1717,7 +1717,7 @@ unsigned int bb_get_frame_time(unsigned char by_preamble_type,
rate = (unsigned int)awc_frame_time[rate_idx];
if (rate_idx <= 3) { /* CCK mode */
- if (by_preamble_type == 1) /* Short */
+ if (preamble_type == 1) /* Short */
preamble = 96;
else
preamble = 192;
diff --git a/drivers/staging/vt6655/baseband.h b/drivers/staging/vt6655/baseband.h
index 0a30afaa7cc3..15b2802ed727 100644
--- a/drivers/staging/vt6655/baseband.h
+++ b/drivers/staging/vt6655/baseband.h
@@ -44,7 +44,7 @@
#define TOP_RATE_2M 0x00200000
#define TOP_RATE_1M 0x00100000
-unsigned int bb_get_frame_time(unsigned char by_preamble_type,
+unsigned int bb_get_frame_time(unsigned char preamble_type,
unsigned char by_pkt_type,
unsigned int cb_frame_length,
unsigned short w_rate);
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Outreachy kernel] [PATCH] staging: vt6655: Rename `by_preamble_type` parameter
2021-10-20 8:40 [PATCH] staging: vt6655: Rename `by_preamble_type` parameter Karolina Drobnik
@ 2021-10-20 8:54 ` Julia Lawall
2021-10-20 12:54 ` Karolina Drobnik
2021-10-20 8:55 ` Greg KH
1 sibling, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2021-10-20 8:54 UTC (permalink / raw)
To: Karolina Drobnik
Cc: outreachy-kernel, gregkh, forest, linux-staging, linux-kernel
On Wed, 20 Oct 2021, Karolina Drobnik wrote:
> Drop `by` prefix in the first parameter of `bb_get_frame_time` function.
> As the original argument, `byPreambleType`, was renamed to `preamble_type`,
> the parameter referring to it is now renamed to match the new naming
> convention.
> Update `bb_get_frame_time` comment to reflect that change.
>
> This patch is a follow-up work to this commit:
> Commit 548b6d7ebfa4 ("staging: vt6655: Rename byPreambleType field")
This is not going to be practical. If the previous patch is accepted,
then this it not needed. If the previous patch is not yet accepted, then
there needs to be a vn+1 putting the patches together into a series.
> Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> ---
> drivers/staging/vt6655/baseband.c | 6 +++---
> drivers/staging/vt6655/baseband.h | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/vt6655/baseband.c b/drivers/staging/vt6655/baseband.c
> index 5efca92f1f18..8f9177db6663 100644
> --- a/drivers/staging/vt6655/baseband.c
> +++ b/drivers/staging/vt6655/baseband.c
> @@ -1691,7 +1691,7 @@ static const unsigned short awc_frame_time[MAX_RATE] = {
> *
> * Parameters:
> * In:
> - * by_preamble_type - Preamble Type
> + * preamble_type - Preamble Type
> * by_pkt_type - PK_TYPE_11A, PK_TYPE_11B, PK_TYPE_11GB, PK_TYPE_11GA
In the realm of small cleanups to this driver, the extra space in front of
the - above is a bit annoying.
> * cb_frame_length - Baseband Type
> * tx_rate - Tx Rate
> @@ -1700,7 +1700,7 @@ static const unsigned short awc_frame_time[MAX_RATE] = {
> * Return Value: FrameTime
> *
> */
> -unsigned int bb_get_frame_time(unsigned char by_preamble_type,
> +unsigned int bb_get_frame_time(unsigned char preamble_type,
> unsigned char by_pkt_type,
> unsigned int cb_frame_length,
> unsigned short tx_rate)
> @@ -1717,7 +1717,7 @@ unsigned int bb_get_frame_time(unsigned char by_preamble_type,
> rate = (unsigned int)awc_frame_time[rate_idx];
>
> if (rate_idx <= 3) { /* CCK mode */
> - if (by_preamble_type == 1) /* Short */
> + if (preamble_type == 1) /* Short */
I hope you will get around to replacing the 1 by the appropriate constant
and removing the "Short" comment.
julia
> preamble = 96;
> else
> preamble = 192;
> diff --git a/drivers/staging/vt6655/baseband.h b/drivers/staging/vt6655/baseband.h
> index 0a30afaa7cc3..15b2802ed727 100644
> --- a/drivers/staging/vt6655/baseband.h
> +++ b/drivers/staging/vt6655/baseband.h
> @@ -44,7 +44,7 @@
> #define TOP_RATE_2M 0x00200000
> #define TOP_RATE_1M 0x00100000
>
> -unsigned int bb_get_frame_time(unsigned char by_preamble_type,
> +unsigned int bb_get_frame_time(unsigned char preamble_type,
> unsigned char by_pkt_type,
> unsigned int cb_frame_length,
> unsigned short w_rate);
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20211020084033.414994-1-karolinadrobnik%40gmail.com.
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Outreachy kernel] [PATCH] staging: vt6655: Rename `by_preamble_type` parameter
2021-10-20 8:54 ` [Outreachy kernel] " Julia Lawall
@ 2021-10-20 12:54 ` Karolina Drobnik
2021-10-20 12:59 ` Julia Lawall
0 siblings, 1 reply; 6+ messages in thread
From: Karolina Drobnik @ 2021-10-20 12:54 UTC (permalink / raw)
To: Julia Lawall
Cc: outreachy-kernel, gregkh, forest, linux-staging, linux-kernel
On Wed, 2021-10-20 at 10:54 +0200, Julia Lawall wrote:
> On Wed, 20 Oct 2021, Karolina Drobnik wrote:
>
> > Drop `by` prefix in the first parameter of `bb_get_frame_time`
> > function.
> > As the original argument, `byPreambleType`, was renamed to
> > `preamble_type`,
> > the parameter referring to it is now renamed to match the new
> > naming
> > convention.
> > Update `bb_get_frame_time` comment to reflect that change.
> >
> > This patch is a follow-up work to this commit:
> > Commit 548b6d7ebfa4 ("staging: vt6655: Rename byPreambleType
> > field")
>
> This is not going to be practical. If the previous patch is
> accepted, then this it not needed.
This change was there before but Greg told me to do only one logical
change per patch (which was a struct member rename), so I reverted it.
I believe this is needed because this parameter still uses Hungarian
notation, which is against the LK coding style. Also, it makes sense to
update the name given my previous change.
> there needs to be a vn+1 putting the patches together into a series.
I didn't know that it should be send this way, especially given the
fact that Outreachy applicants should first get 3-5 patches out before
creating a patchset. Or has something changed in this regard?
> > @@ -1691,7 +1691,7 @@ static const unsigned short
> > awc_frame_time[MAX_RATE] = {
> > *
> > * Parameters:
> > * In:
> > - * by_preamble_type - Preamble Type
> > + * preamble_type - Preamble Type
> > * by_pkt_type - PK_TYPE_11A, PK_TYPE_11B,
> > PK_TYPE_11GB, PK_TYPE_11GA
>
> In the realm of small cleanups to this driver, the extra space in
> front of the - above is a bit annoying.
I can add this in but will that still count as a one logical change?
I described the comment update, will that suffice?
> > @@ -1717,7 +1717,7 @@ unsigned int bb_get_frame_time(unsigned char
> > by_preamble_type,
> > rate = (unsigned int)awc_frame_time[rate_idx];
> >
> > if (rate_idx <= 3) { /* CCK mode */
> > - if (by_preamble_type == 1) /* Short */
> > + if (preamble_type == 1) /* Short */
>
> I hope you will get around to replacing the 1 by the appropriate
> constant and removing the "Short" comment.
I plan to do so after correcting the name variable.
On Wed, 2021-10-20 at 10:55 +0200, Greg KH wrote:
> On Wed, Oct 20, 2021 at 09:40:33AM +0100, Karolina Drobnik wrote:
> > Drop `by` prefix in the first parameter of `bb_get_frame_time`
> > function.
> > As the original argument, `byPreambleType`, was renamed to
> > `preamble_type`,
> > the parameter referring to it is now renamed to match the new
> > naming
> > convention.
> > Update `bb_get_frame_time` comment to reflect that change.
> >
> > This patch is a follow-up work to this commit:
> > Commit 548b6d7ebfa4 ("staging: vt6655: Rename byPreambleType
> > field")
>
> There is no need for these two lines in the changelog text. They can
> go
> below --- if you want to have them.
Thank you for clarifying this. I've been following the Submitting
Patches docs[1] and thought this is needed.
Thanks,
Karolina
-------------------------------------------------------------------
[1]:
https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L106
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Outreachy kernel] [PATCH] staging: vt6655: Rename `by_preamble_type` parameter
2021-10-20 12:54 ` Karolina Drobnik
@ 2021-10-20 12:59 ` Julia Lawall
2021-10-20 13:08 ` Karolina Drobnik
0 siblings, 1 reply; 6+ messages in thread
From: Julia Lawall @ 2021-10-20 12:59 UTC (permalink / raw)
To: Karolina Drobnik
Cc: outreachy-kernel, gregkh, forest, linux-staging, linux-kernel
On Wed, 20 Oct 2021, Karolina Drobnik wrote:
> On Wed, 2021-10-20 at 10:54 +0200, Julia Lawall wrote:
> > On Wed, 20 Oct 2021, Karolina Drobnik wrote:
> >
> > > Drop `by` prefix in the first parameter of `bb_get_frame_time`
> > > function.
> > > As the original argument, `byPreambleType`, was renamed to
> > > `preamble_type`,
> > > the parameter referring to it is now renamed to match the new
> > > naming
> > > convention.
> > > Update `bb_get_frame_time` comment to reflect that change.
> > >
> > > This patch is a follow-up work to this commit:
> > > Commit 548b6d7ebfa4 ("staging: vt6655: Rename byPreambleType
> > > field")
> >
> > This is not going to be practical. If the previous patch is
> > accepted, then this it not needed.
>
> This change was there before but Greg told me to do only one logical
> change per patch (which was a struct member rename), so I reverted it.
> I believe this is needed because this parameter still uses Hungarian
> notation, which is against the LK coding style. Also, it makes sense to
> update the name given my previous change.
Sorry, I think I was not clear. It's not practical to explain constraints
on other patches in the log message.
>
> > there needs to be a vn+1 putting the patches together into a series.
>
> I didn't know that it should be send this way, especially given the
> fact that Outreachy applicants should first get 3-5 patches out before
> creating a patchset. Or has something changed in this regard?
I think that the 3-5 rule is not that important. The important thing is
that if you want to make two different changes on the same file, either
the first one has to be accepted before you submit the second one, or they
have to be in a series.
> > > @@ -1691,7 +1691,7 @@ static const unsigned short
> > > awc_frame_time[MAX_RATE] = {
> > > *
> > > * Parameters:
> > > * In:
> > > - * by_preamble_type - Preamble Type
> > > + * preamble_type - Preamble Type
> > > * by_pkt_type - PK_TYPE_11A, PK_TYPE_11B,
> > > PK_TYPE_11GB, PK_TYPE_11GA
> >
> > In the realm of small cleanups to this driver, the extra space in
> > front of the - above is a bit annoying.
>
> I can add this in but will that still count as a one logical change?
No. It's a different change. It's just a small whitespace issue, but
it's not triggered by the other changes you have made.
julia
> I described the comment update, will that suffice?
>
> > > @@ -1717,7 +1717,7 @@ unsigned int bb_get_frame_time(unsigned char
> > > by_preamble_type,
> > > rate = (unsigned int)awc_frame_time[rate_idx];
> > >
> > > if (rate_idx <= 3) { /* CCK mode */
> > > - if (by_preamble_type == 1) /* Short */
> > > + if (preamble_type == 1) /* Short */
> >
> > I hope you will get around to replacing the 1 by the appropriate
> > constant and removing the "Short" comment.
>
> I plan to do so after correcting the name variable.
>
>
> On Wed, 2021-10-20 at 10:55 +0200, Greg KH wrote:
> > On Wed, Oct 20, 2021 at 09:40:33AM +0100, Karolina Drobnik wrote:
> > > Drop `by` prefix in the first parameter of `bb_get_frame_time`
> > > function.
> > > As the original argument, `byPreambleType`, was renamed to
> > > `preamble_type`,
> > > the parameter referring to it is now renamed to match the new
> > > naming
> > > convention.
> > > Update `bb_get_frame_time` comment to reflect that change.
> > >
> > > This patch is a follow-up work to this commit:
> > > Commit 548b6d7ebfa4 ("staging: vt6655: Rename byPreambleType
> > > field")
> >
> > There is no need for these two lines in the changelog text. They can
> > go
> > below --- if you want to have them.
>
> Thank you for clarifying this. I've been following the Submitting
> Patches docs[1] and thought this is needed.
>
>
> Thanks,
> Karolina
> -------------------------------------------------------------------
> [1]:
> https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L106
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Outreachy kernel] [PATCH] staging: vt6655: Rename `by_preamble_type` parameter
2021-10-20 12:59 ` Julia Lawall
@ 2021-10-20 13:08 ` Karolina Drobnik
0 siblings, 0 replies; 6+ messages in thread
From: Karolina Drobnik @ 2021-10-20 13:08 UTC (permalink / raw)
To: Julia Lawall
Cc: outreachy-kernel, gregkh, forest, linux-staging, linux-kernel
On Wed, 2021-10-20 at 14:59 +0200, Julia Lawall wrote:
> On Wed, 20 Oct 2021, Karolina Drobnik wrote:
>
> > On Wed, 2021-10-20 at 10:54 +0200, Julia Lawall wrote:
> > > On Wed, 20 Oct 2021, Karolina Drobnik wrote:
> > >
> > > > Drop `by` prefix in the first parameter of `bb_get_frame_time`
> > > > function.
> > > > As the original argument, `byPreambleType`, was renamed to
> > > > `preamble_type`,
> > > > the parameter referring to it is now renamed to match the new
> > > > naming
> > > > convention.
> > > > Update `bb_get_frame_time` comment to reflect that change.
> > > >
> > > > This patch is a follow-up work to this commit:
> > > > Commit 548b6d7ebfa4 ("staging: vt6655: Rename
> > > > byPreambleType
> > > > field")
> > >
> > > This is not going to be practical. If the previous patch is
> > > accepted, then this it not needed.
> >
> > This change was there before but Greg told me to do only one
> > logical
> > change per patch (which was a struct member rename), so I reverted
> > it.
> > I believe this is needed because this parameter still uses
> > Hungarian
> > notation, which is against the LK coding style. Also, it makes
> > sense to
> > update the name given my previous change.
>
> Sorry, I think I was not clear. It's not practical to explain
> constraints
> on other patches in the log message.
Oh, I see. I thought about this log message as "why" but now, when I
come think of it, just saying it's about the Hungarian notation should
be enough. I'll correct the log message, thank you.
>
> The important thing is
> that if you want to make two different changes on the same file,
> either
> the first one has to be accepted before you submit the second one, or
> they
> have to be in a series.
I see, thanks for clarification.
> > >
> > I can add this in but will that still count as a one logical
> > change?
>
> No. It's a different change. It's just a small whitespace issue,
> but
> it's not triggered by the other changes you have made.
Ok, I'll submit a separate patch for it later on.
Thanks,
Karolina
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging: vt6655: Rename `by_preamble_type` parameter
2021-10-20 8:40 [PATCH] staging: vt6655: Rename `by_preamble_type` parameter Karolina Drobnik
2021-10-20 8:54 ` [Outreachy kernel] " Julia Lawall
@ 2021-10-20 8:55 ` Greg KH
1 sibling, 0 replies; 6+ messages in thread
From: Greg KH @ 2021-10-20 8:55 UTC (permalink / raw)
To: Karolina Drobnik; +Cc: outreachy-kernel, forest, linux-staging, linux-kernel
On Wed, Oct 20, 2021 at 09:40:33AM +0100, Karolina Drobnik wrote:
> Drop `by` prefix in the first parameter of `bb_get_frame_time` function.
> As the original argument, `byPreambleType`, was renamed to `preamble_type`,
> the parameter referring to it is now renamed to match the new naming
> convention.
> Update `bb_get_frame_time` comment to reflect that change.
>
> This patch is a follow-up work to this commit:
> Commit 548b6d7ebfa4 ("staging: vt6655: Rename byPreambleType field")
There is no need for these two lines in the changelog text. They can go
below --- if you want to have them.
But, as the above commit is already in my tree, there's no need for this
information at all, right?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-20 13:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-20 8:40 [PATCH] staging: vt6655: Rename `by_preamble_type` parameter Karolina Drobnik
2021-10-20 8:54 ` [Outreachy kernel] " Julia Lawall
2021-10-20 12:54 ` Karolina Drobnik
2021-10-20 12:59 ` Julia Lawall
2021-10-20 13:08 ` Karolina Drobnik
2021-10-20 8:55 ` Greg KH
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).