[PATCH] staging: vt6655: Rename `by_preamble_type` parameter

[PATCH] staging: vt6655: Rename `by_preamble_type` parameter

[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

Re: [Outreachy kernel] [PATCH] staging: vt6655: Rename `by_preamble_type` parameter

[Julia Lawall]

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.
>

Re: [PATCH] staging: vt6655: Rename `by_preamble_type` parameter

[Greg KH]

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

Re: [Outreachy kernel] [PATCH] staging: vt6655: Rename `by_preamble_type` parameter

[Karolina Drobnik]

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

Re: [Outreachy kernel] [PATCH] staging: vt6655: Rename `by_preamble_type` parameter

[Julia Lawall]

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
>
>
>

Re: [Outreachy kernel] [PATCH] staging: vt6655: Rename `by_preamble_type` parameter

[Karolina Drobnik]

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