[PATCH v2 0/2] staging: vt6656: Refactor the vnt_get_phy_field function

[PATCH v2 0/2] staging: vt6656: Refactor the vnt_get_phy_field function

[Oscar Carter]

This patch series makes a refactor of the vnt_get_phy_field function
through two patches.

The first one refactors the assignment of the "phy->signal" variable
using a constant array with the correct values for every rate.

The second patch removes duplicate code for the assignment of the
"phy->service" variable by putting it outside the if-else statement due
to it's the same for the two branches.

Changelog v1 -> v2:
- Remove one dimension from the constant array for the "phy->signal"
  values and use an OR mask instead of the second array dimension as
  Malcolm Priestley has suggested.

Oscar Carter (2):
  staging: vt6656: Refactor the assignment of the phy->signal variable
  staging: vt6656: Remove duplicate code for the phy->service assignment

 drivers/staging/vt6656/baseband.c | 108 ++++++++----------------------
 1 file changed, 27 insertions(+), 81 deletions(-)

--
2.20.1

[PATCH v2 1/2] staging: vt6656: Refactor the assignment of the phy->signal variable

[Oscar Carter]

Create a constant array with the values of the "phy->signal" for every
rate. Remove all "phy->signal" assignments inside the switch statement
and replace these with a single reading from the new vnt_phy_signal
array.

The constant array can be of one dimension because the OR mask with
BIT(3) or BIT(4) allow obtain a second value according to the rate,
the preamble_type and the pkt_type.

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
 drivers/staging/vt6656/baseband.c | 105 ++++++++----------------------
 1 file changed, 26 insertions(+), 79 deletions(-)

diff --git a/drivers/staging/vt6656/baseband.c b/drivers/staging/vt6656/baseband.c
index a19a563d8bcc..05cc4797df52 100644
--- a/drivers/staging/vt6656/baseband.c
+++ b/drivers/staging/vt6656/baseband.c
@@ -115,6 +115,21 @@ static const u16 vnt_frame_time[MAX_RATE] = {
 	10, 20, 55, 110, 24, 36, 48, 72, 96, 144, 192, 216
 };

+static const u8 vnt_phy_signal[] = {
+	0x00,	/* RATE_1M  */
+	0x01,	/* RATE_2M  */
+	0x02,	/* RATE_5M  */
+	0x03,	/* RATE_11M */
+	0x8b,	/* RATE_6M  */
+	0x8f,	/* RATE_9M  */
+	0x8a,	/* RATE_12M */
+	0x8e,	/* RATE_18M */
+	0x89,	/* RATE_24M */
+	0x8d,	/* RATE_36M */
+	0x88,	/* RATE_48M */
+	0x8c	/* RATE_54M */
+};
+
 /*
  * Description: Calculate data frame transmitting time
  *
@@ -183,6 +198,8 @@ void vnt_get_phy_field(struct vnt_private *priv, u32 frame_length,
 	u32 count = 0;
 	u32 tmp;
 	int ext_bit;
+	int i;
+	u8 mask = 0;
 	u8 preamble_type = priv->preamble_type;

 	bit_count = frame_length * 8;
@@ -191,27 +208,12 @@ void vnt_get_phy_field(struct vnt_private *priv, u32 frame_length,
 	switch (tx_rate) {
 	case RATE_1M:
 		count = bit_count;
-
-		phy->signal = 0x00;
-
 		break;
 	case RATE_2M:
 		count = bit_count / 2;
-
-		if (preamble_type == 1)
-			phy->signal = 0x09;
-		else
-			phy->signal = 0x01;
-
 		break;
 	case RATE_5M:
 		count = DIV_ROUND_UP(bit_count * 10, 55);
-
-		if (preamble_type == 1)
-			phy->signal = 0x0a;
-		else
-			phy->signal = 0x02;
-
 		break;
 	case RATE_11M:
 		count = bit_count / 11;
@@ -224,75 +226,20 @@ void vnt_get_phy_field(struct vnt_private *priv, u32 frame_length,
 				ext_bit = true;
 		}

-		if (preamble_type == 1)
-			phy->signal = 0x0b;
-		else
-			phy->signal = 0x03;
-
-		break;
-	case RATE_6M:
-		if (pkt_type == PK_TYPE_11A)
-			phy->signal = 0x9b;
-		else
-			phy->signal = 0x8b;
-
 		break;
-	case RATE_9M:
-		if (pkt_type == PK_TYPE_11A)
-			phy->signal = 0x9f;
-		else
-			phy->signal = 0x8f;
-
-		break;
-	case RATE_12M:
-		if (pkt_type == PK_TYPE_11A)
-			phy->signal = 0x9a;
-		else
-			phy->signal = 0x8a;
-
-		break;
-	case RATE_18M:
-		if (pkt_type == PK_TYPE_11A)
-			phy->signal = 0x9e;
-		else
-			phy->signal = 0x8e;
-
-		break;
-	case RATE_24M:
-		if (pkt_type == PK_TYPE_11A)
-			phy->signal = 0x99;
-		else
-			phy->signal = 0x89;
-
-		break;
-	case RATE_36M:
-		if (pkt_type == PK_TYPE_11A)
-			phy->signal = 0x9d;
-		else
-			phy->signal = 0x8d;
-
-		break;
-	case RATE_48M:
-		if (pkt_type == PK_TYPE_11A)
-			phy->signal = 0x98;
-		else
-			phy->signal = 0x88;
+	}

-		break;
-	case RATE_54M:
+	if (tx_rate > RATE_11M) {
 		if (pkt_type == PK_TYPE_11A)
-			phy->signal = 0x9c;
-		else
-			phy->signal = 0x8c;
-		break;
-	default:
-		if (pkt_type == PK_TYPE_11A)
-			phy->signal = 0x9c;
-		else
-			phy->signal = 0x8c;
-		break;
+			mask = BIT(4);
+	} else if (tx_rate > RATE_1M) {
+		if (preamble_type == PREAMBLE_SHORT)
+			mask = BIT(3);
 	}

+	i = tx_rate > RATE_54M ? RATE_54M : tx_rate;
+	phy->signal = vnt_phy_signal[i] | mask;
+
 	if (pkt_type == PK_TYPE_11B) {
 		phy->service = 0x00;
 		if (ext_bit)
--
2.20.1

Re: [PATCH v2 1/2] staging: vt6656: Refactor the assignment of the phy->signal variable

[Greg Kroah-Hartman]

On Sat, Apr 11, 2020 at 02:26:09PM +0200, Oscar Carter wrote:
> Create a constant array with the values of the "phy->signal" for every
> rate. Remove all "phy->signal" assignments inside the switch statement
> and replace these with a single reading from the new vnt_phy_signal
> array.
> 
> The constant array can be of one dimension because the OR mask with
> BIT(3) or BIT(4) allow obtain a second value according to the rate,
> the preamble_type and the pkt_type.
> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> ---
>  drivers/staging/vt6656/baseband.c | 105 ++++++++----------------------
>  1 file changed, 26 insertions(+), 79 deletions(-)

This series did not apply to my tree, please rebase and resend.

thanks,

greg k-h

Re: [PATCH v2 1/2] staging: vt6656: Refactor the assignment of the phy->signal variable

[Oscar Carter]

On Mon, Apr 13, 2020 at 02:56:16PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Apr 11, 2020 at 02:26:09PM +0200, Oscar Carter wrote:
> > Create a constant array with the values of the "phy->signal" for every
> > rate. Remove all "phy->signal" assignments inside the switch statement
> > and replace these with a single reading from the new vnt_phy_signal
> > array.
> >
> > The constant array can be of one dimension because the OR mask with
> > BIT(3) or BIT(4) allow obtain a second value according to the rate,
> > the preamble_type and the pkt_type.
> >
> > Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> > ---
> >  drivers/staging/vt6656/baseband.c | 105 ++++++++----------------------
> >  1 file changed, 26 insertions(+), 79 deletions(-)
>
> This series did not apply to my tree, please rebase and resend.

Rebase the patchs is a normal process in the development or am I doing something
wrong ?

>
> thanks,
>
> greg k-h

thanks,

oscar carter

Re: [PATCH v2 1/2] staging: vt6656: Refactor the assignment of the phy->signal variable

[Greg Kroah-Hartman]

On Mon, Apr 13, 2020 at 04:25:17PM +0200, Oscar Carter wrote:
> On Mon, Apr 13, 2020 at 02:56:16PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Apr 11, 2020 at 02:26:09PM +0200, Oscar Carter wrote:
> > > Create a constant array with the values of the "phy->signal" for every
> > > rate. Remove all "phy->signal" assignments inside the switch statement
> > > and replace these with a single reading from the new vnt_phy_signal
> > > array.
> > >
> > > The constant array can be of one dimension because the OR mask with
> > > BIT(3) or BIT(4) allow obtain a second value according to the rate,
> > > the preamble_type and the pkt_type.
> > >
> > > Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> > > ---
> > >  drivers/staging/vt6656/baseband.c | 105 ++++++++----------------------
> > >  1 file changed, 26 insertions(+), 79 deletions(-)
> >
> > This series did not apply to my tree, please rebase and resend.
> 
> Rebase the patchs is a normal process in the development or am I doing something
> wrong ?

It's normal when multiple people are working on the same area with lots
of patches flying around.  Not a problem, it doesn't bother me at all :)

thanks,

greg k-h

Re: [PATCH v2 1/2] staging: vt6656: Refactor the assignment of the phy->signal variable

[Oscar Carter]

On Mon, Apr 13, 2020 at 04:32:58PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 13, 2020 at 04:25:17PM +0200, Oscar Carter wrote:
> > On Mon, Apr 13, 2020 at 02:56:16PM +0200, Greg Kroah-Hartman wrote:
> > > On Sat, Apr 11, 2020 at 02:26:09PM +0200, Oscar Carter wrote:
> > > > Create a constant array with the values of the "phy->signal" for every
> > > > rate. Remove all "phy->signal" assignments inside the switch statement
> > > > and replace these with a single reading from the new vnt_phy_signal
> > > > array.
> > > >
> > > > The constant array can be of one dimension because the OR mask with
> > > > BIT(3) or BIT(4) allow obtain a second value according to the rate,
> > > > the preamble_type and the pkt_type.
> > > >
> > > > Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> > > > ---
> > > >  drivers/staging/vt6656/baseband.c | 105 ++++++++----------------------
> > > >  1 file changed, 26 insertions(+), 79 deletions(-)
> > >
> > > This series did not apply to my tree, please rebase and resend.
> >
> > Rebase the patchs is a normal process in the development or am I doing something
> > wrong ?
>
> It's normal when multiple people are working on the same area with lots
> of patches flying around.  Not a problem, it doesn't bother me at all :)

Thanks for the clarification about this theme.

>
> thanks,
>
> greg k-h

thanks,

oscar carter

[PATCH v2 2/2] staging: vt6656: Remove duplicate code for the phy->service assignment

[Oscar Carter]

Take out the "phy->service" assignment from the if-else statement due to
it's the same for the two branches.

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
 drivers/staging/vt6656/baseband.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/vt6656/baseband.c b/drivers/staging/vt6656/baseband.c
index 05cc4797df52..c8b3cc84da6c 100644
--- a/drivers/staging/vt6656/baseband.c
+++ b/drivers/staging/vt6656/baseband.c
@@ -239,14 +239,13 @@ void vnt_get_phy_field(struct vnt_private *priv, u32 frame_length,

 	i = tx_rate > RATE_54M ? RATE_54M : tx_rate;
 	phy->signal = vnt_phy_signal[i] | mask;
+	phy->service = 0x00;

 	if (pkt_type == PK_TYPE_11B) {
-		phy->service = 0x00;
 		if (ext_bit)
 			phy->service |= 0x80;
 		phy->len = cpu_to_le16((u16)count);
 	} else {
-		phy->service = 0x00;
 		phy->len = cpu_to_le16((u16)frame_length);
 	}
 }
--
2.20.1