[PATCH -next 1/3] staging: vt6655: remove unused variable

[PATCH -next 1/3] staging: vt6655: remove unused variable

[Edmundo Carmona Antoranz]

Since its introduction in 5449c685a4b3 (Staging: Add pristine
upstream vt6655 driver sources, 2009-04-25), the values
stored in variable byData have never been read in the macro
PCAvDelayByIO. By removing it, we are getting rid of a warning:

drivers/staging/vt6655/upc.h:45:16: warning: variable ‘byData’ set but not used [-Wunused-but-set-variable]

Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
---
 drivers/staging/vt6655/upc.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/vt6655/upc.h b/drivers/staging/vt6655/upc.h
index e086ec6e77f7..f00d0fd90003 100644
--- a/drivers/staging/vt6655/upc.h
+++ b/drivers/staging/vt6655/upc.h
@@ -42,14 +42,13 @@
 
 #define PCAvDelayByIO(uDelayUnit)				\
 do {								\
-	unsigned char byData;					\
 	unsigned long ii;					\
 								\
 	if (uDelayUnit <= 50) {					\
 		udelay(uDelayUnit);				\
 	} else {						\
 		for (ii = 0; ii < (uDelayUnit); ii++)		\
-			byData = inb(0x61);			\
+			inb(0x61);				\
 	}							\
 } while (0)
 
-- 
2.30.1

[PATCH -next 2/3] staging: vt6655: correct documentation warnings

[Edmundo Carmona Antoranz]

Both arguments to set_channel have changed their names and their
types. Correct the discrepancy in the function documentation to get
rid of four warnings:

drivers/staging/vt6655/channel.c:165: warning: Function parameter or member 'priv' not described in 'set_channel'
drivers/staging/vt6655/channel.c:165: warning: Function parameter or member 'ch' not described in 'set_channel'
drivers/staging/vt6655/channel.c:165: warning: Excess function parameter 'pDeviceHandler' description in 'set_channel'
drivers/staging/vt6655/channel.c:165: warning: Excess function parameter 'uConnectionChannel' description in 'set_channel'

Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
---
 drivers/staging/vt6655/channel.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/vt6655/channel.c b/drivers/staging/vt6655/channel.c
index 889fc22f19bd..ba92b7259ec6 100644
--- a/drivers/staging/vt6655/channel.c
+++ b/drivers/staging/vt6655/channel.c
@@ -155,8 +155,8 @@ void vnt_init_bands(struct vnt_private *priv)
 /**
  * set_channel() - Set NIC media channel
  *
- * @pDeviceHandler: The adapter to be set
- * @uConnectionChannel: Channel to be set
+ * @priv: The adapter to be set
+ * @ch: Channel to be set
  *
  * Return Value: true if succeeded; false if failed.
  *
-- 
2.30.1

[PATCH -next 3/3] staging: vt6655: remove duplicate code

[Edmundo Carmona Antoranz]

In the definition of vnt_init_bands(), there are two sections of
code that are very similar. Unifying them through a new
function that takes care of initialization of a single band.

Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
---
 drivers/staging/vt6655/channel.c | 38 +++++++++++++++-----------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/vt6655/channel.c b/drivers/staging/vt6655/channel.c
index ba92b7259ec6..cf46ee63681a 100644
--- a/drivers/staging/vt6655/channel.c
+++ b/drivers/staging/vt6655/channel.c
@@ -114,40 +114,38 @@ static struct ieee80211_supported_band vnt_supported_5ghz_band = {
 	.n_bitrates = ARRAY_SIZE(vnt_rates_a),
 };
 
-void vnt_init_bands(struct vnt_private *priv)
+static void vnt_init_band(struct vnt_private *priv,
+			  struct ieee80211_supported_band *supported_band,
+			  enum nl80211_band band)
 {
-	struct ieee80211_channel *ch;
 	int i;
 
+	for (i = 0; i < supported_band->n_channels; i++) {
+		supported_band->channels[i].max_power = 0x3f;
+		supported_band->channels[i].flags =
+			IEEE80211_CHAN_NO_HT40;
+	}
+
+	priv->hw->wiphy->bands[band] = supported_band;
+}
+
+void vnt_init_bands(struct vnt_private *priv)
+{
 	switch (priv->byRFType) {
 	case RF_AIROHA7230:
 	case RF_UW2452:
 	case RF_NOTHING:
 	default:
-		ch = vnt_channels_5ghz;
-
-		for (i = 0; i < ARRAY_SIZE(vnt_channels_5ghz); i++) {
-			ch[i].max_power = 0x3f;
-			ch[i].flags = IEEE80211_CHAN_NO_HT40;
-		}
-
-		priv->hw->wiphy->bands[NL80211_BAND_5GHZ] =
-						&vnt_supported_5ghz_band;
+		vnt_init_band(priv, &vnt_supported_5ghz_band,
+			      NL80211_BAND_5GHZ);
 		fallthrough;
 	case RF_RFMD2959:
 	case RF_AIROHA:
 	case RF_AL2230S:
 	case RF_UW2451:
 	case RF_VT3226:
-		ch = vnt_channels_2ghz;
-
-		for (i = 0; i < ARRAY_SIZE(vnt_channels_2ghz); i++) {
-			ch[i].max_power = 0x3f;
-			ch[i].flags = IEEE80211_CHAN_NO_HT40;
-		}
-
-		priv->hw->wiphy->bands[NL80211_BAND_2GHZ] =
-						&vnt_supported_2ghz_band;
+		vnt_init_band(priv, &vnt_supported_2ghz_band,
+			      NL80211_BAND_2GHZ);
 		break;
 	}
 }
-- 
2.30.1

Re: [PATCH -next 1/3] staging: vt6655: remove unused variable

[Greg KH]

On Sun, Mar 14, 2021 at 08:59:41AM -0600, Edmundo Carmona Antoranz wrote:
> Since its introduction in 5449c685a4b3 (Staging: Add pristine
> upstream vt6655 driver sources, 2009-04-25), the values
> stored in variable byData have never been read in the macro
> PCAvDelayByIO. By removing it, we are getting rid of a warning:
> 
> drivers/staging/vt6655/upc.h:45:16: warning: variable ‘byData’ set but not used [-Wunused-but-set-variable]
> 
> Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
> ---
>  drivers/staging/vt6655/upc.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/upc.h b/drivers/staging/vt6655/upc.h
> index e086ec6e77f7..f00d0fd90003 100644
> --- a/drivers/staging/vt6655/upc.h
> +++ b/drivers/staging/vt6655/upc.h
> @@ -42,14 +42,13 @@
>  
>  #define PCAvDelayByIO(uDelayUnit)				\
>  do {								\
> -	unsigned char byData;					\
>  	unsigned long ii;					\
>  								\
>  	if (uDelayUnit <= 50) {					\
>  		udelay(uDelayUnit);				\
>  	} else {						\
>  		for (ii = 0; ii < (uDelayUnit); ii++)		\
> -			byData = inb(0x61);			\
> +			inb(0x61);				\

Are you sure that the compiler does not make the inb() now go away?

This is being done like this for a very specific reason, the value read
does not matter, but you have to read something.

thanks,

greg k-h

Re: [PATCH -next 1/3] staging: vt6655: remove unused variable

[Edmundo Carmona Antoranz]

On Sun, Mar 14, 2021 at 10:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > -                     byData = inb(0x61);                     \
> > +                     inb(0x61);                              \
>
> Are you sure that the compiler does not make the inb() now go away?

Hey, Greg!

Take what you are about to read with a BIG grain of salt because I'm
just learning about the details of what's going on and the tools at my
disposal.

On v5.11 with an allyesconfig, by changing the value of
CB_DELAY_LOOP_WAIT in mac.h to 100 to force the compiler to include
the `else` path that includes the for with the inb() call, comparing
the resulting srom.o files before/after the patch:

Compiling with only the value in mac.h changed:

$ size drivers/staging/vt6655/srom.o
  text    data     bss     dec     hex filename
   959       0       0     959     3bf drivers/staging/vt6655/srom.o

By removing the variable in the macro:

$ size drivers/staging/vt6655/srom.o
  text    data     bss     dec     hex filename
   959       0       0     959     3bf drivers/staging/vt6655/srom.o

By also removing the inb() call, in other words, keeping an empty for:

$ size drivers/staging/vt6655/srom.o
  text    data     bss     dec     hex filename
   865       0       0     865     361 drivers/staging/vt6655/srom.


That being the case, I think that inb is not fading away by removing
the variable... at least in my environment.

Just in case, it's not like I _want_ the patch to be accepted. I took
it as an exercise and if you think that you want to play safe and
reject it, it's fine. It was still valuable to me because of your
question about the function going away and me trying to find out.

Thanks!

Re: [PATCH -next 1/3] staging: vt6655: remove unused variable

[Greg KH]

On Mon, Mar 15, 2021 at 07:46:35PM -0600, Edmundo Carmona Antoranz wrote:
> On Sun, Mar 14, 2021 at 10:01 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > > -                     byData = inb(0x61);                     \
> > > +                     inb(0x61);                              \
> >
> > Are you sure that the compiler does not make the inb() now go away?
> 
> Hey, Greg!
> 
> Take what you are about to read with a BIG grain of salt because I'm
> just learning about the details of what's going on and the tools at my
> disposal.
> 
> On v5.11 with an allyesconfig, by changing the value of
> CB_DELAY_LOOP_WAIT in mac.h to 100 to force the compiler to include
> the `else` path that includes the for with the inb() call, comparing
> the resulting srom.o files before/after the patch:
> 
> Compiling with only the value in mac.h changed:
> 
> $ size drivers/staging/vt6655/srom.o
>   text    data     bss     dec     hex filename
>    959       0       0     959     3bf drivers/staging/vt6655/srom.o
> 
> By removing the variable in the macro:
> 
> $ size drivers/staging/vt6655/srom.o
>   text    data     bss     dec     hex filename
>    959       0       0     959     3bf drivers/staging/vt6655/srom.o
> 
> By also removing the inb() call, in other words, keeping an empty for:
> 
> $ size drivers/staging/vt6655/srom.o
>   text    data     bss     dec     hex filename
>    865       0       0     865     361 drivers/staging/vt6655/srom.
> 
> 
> That being the case, I think that inb is not fading away by removing
> the variable... at least in my environment.
> 
> Just in case, it's not like I _want_ the patch to be accepted. I took
> it as an exercise and if you think that you want to play safe and
> reject it, it's fine. It was still valuable to me because of your
> question about the function going away and me trying to find out.

Ok, I'll leave this as-is because the inb() needs to happen, so saving
the value is normal for stuff like this.

thanks,

greg k-h

Re: [PATCH -next 1/3] staging: vt6655: remove unused variable

[Dan Carpenter]

On Sun, Mar 14, 2021 at 05:01:24PM +0100, Greg KH wrote:
> On Sun, Mar 14, 2021 at 08:59:41AM -0600, Edmundo Carmona Antoranz wrote:
> > Since its introduction in 5449c685a4b3 (Staging: Add pristine
> > upstream vt6655 driver sources, 2009-04-25), the values
> > stored in variable byData have never been read in the macro
> > PCAvDelayByIO. By removing it, we are getting rid of a warning:
> > 
> > drivers/staging/vt6655/upc.h:45:16: warning: variable ‘byData’ set but not used [-Wunused-but-set-variable]
> > 
> > Signed-off-by: Edmundo Carmona Antoranz <eantoranz@gmail.com>
> > ---
> >  drivers/staging/vt6655/upc.h | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/vt6655/upc.h b/drivers/staging/vt6655/upc.h
> > index e086ec6e77f7..f00d0fd90003 100644
> > --- a/drivers/staging/vt6655/upc.h
> > +++ b/drivers/staging/vt6655/upc.h
> > @@ -42,14 +42,13 @@
> >  
> >  #define PCAvDelayByIO(uDelayUnit)				\
> >  do {								\
> > -	unsigned char byData;					\
> >  	unsigned long ii;					\
> >  								\
> >  	if (uDelayUnit <= 50) {					\
> >  		udelay(uDelayUnit);				\
> >  	} else {						\
> >  		for (ii = 0; ii < (uDelayUnit); ii++)		\
> > -			byData = inb(0x61);			\
> > +			inb(0x61);				\
> 
> Are you sure that the compiler does not make the inb() now go away?
> 

This is safe.  The compiler can't remove it.

regards,
dan carpenter