Message-ID: <1512233175.6321.11.camel@perches.com>
Subject: Re: [PATCH] staging: pi433: Make
 rf69_set_dc_cut_off_frequency_intern static
From: Joe Perches <joe@perches.com>
To: Marcus Wolf <linux@wolf-entwicklungen.de>, gregkh@linuxfoundation.org,
        dan.carpenter@oracle.com, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
Date: Sat, 02 Dec 2017 08:46:15 -0800
In-Reply-To: <1512228022-22599-1-git-send-email-linux@wolf-entwicklungen.de>
References: <1512228022-22599-1-git-send-email-linux@wolf-entwicklungen.de>
Content-Type: text/plain; charset="ISO-8859-1"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4812
Lines: 108

On Sat, 2017-12-02 at 17:20 +0200, Marcus Wolf wrote:
> rf69_set_dc_cut_off_frequency_intern is used by rf69.c internally only.
> Therefore removed the function from header and declared it staic in
> the implemtation.
> Signed-off-by: Marcus Wolf <linux@wolf-entwicklungen.de>
> ---
>  drivers/staging/pi433/rf69.c |    2 +-
>  drivers/staging/pi433/rf69.h |    1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
> index ec4b540..90ccf4e 100644
> --- a/drivers/staging/pi433/rf69.c
> +++ b/drivers/staging/pi433/rf69.c
> @@ -442,7 +442,7 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
>  	}
>  }
>  
> -int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 reg, enum dccPercent dccPercent)
> +static int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 reg, enum dccPercent dccPercent)
>  {
>  	switch (dccPercent) {
>  	case dcc16Percent:	return rmw(spi, reg, MASK_BW_DCC_FREQ, BW_DCC_16_PERCENT);
> diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
> index 645c8df..7f580e9 100644
> --- a/drivers/staging/pi433/rf69.h
> +++ b/drivers/staging/pi433/rf69.h
> @@ -36,7 +36,6 @@
>  int rf69_set_antenna_impedance(struct spi_device *spi, enum antennaImpedance antennaImpedance);
>  int rf69_set_lna_gain(struct spi_device *spi, enum lnaGain lnaGain);
>  enum lnaGain rf69_get_lna_gain(struct spi_device *spi);
> -int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 reg, enum dccPercent dccPercent);
>  int rf69_set_dc_cut_off_frequency(struct spi_device *spi, enum dccPercent dccPercent);
>  int rf69_set_dc_cut_off_frequency_during_afc(struct spi_device *spi, enum dccPercent dccPercent);
>  int rf69_set_bandwidth(struct spi_device *spi, enum mantisse mantisse, u8 exponent);

Beyond the basics of learning to submit patches by
shutting up checkpatch messages, please always keep
in mind how to actually improve the logic and code
clarity for human readers.

The rf69_set_dc_cut_off_frequency_intern function
is actually pretty poorly written.

It's repeated logic that could be simplified and
code size reduced quite a bit.

For instance, the patch below makes it more obvious
what the function does and reduces boiler-plate
copy/paste to a single line.

And I don't particularly care that it is not
checkpatch 'clean'.  checkpatch is only a stupid,
mindless style checker.  Always try to be better
than a mindless script.

	and you -really- want to make it checkpatch clean,
	a new #define could be used like this below

	#define case_dcc_percent(val, dcc, DCC)	\
		case dcc: val = DCC; break;

Anyway:

$ size drivers/staging/pi433/rf69.o*
   text	   data	    bss	    dec	    hex	
filename
  35820	   5600	      0	  41420	   a1cc	
drivers/staging/pi433/rf69.o.new
  36968	   5600	      0	  42568	   a648	
drivers/staging/pi433/rf69.o.old
---
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a2153c999..9e40f162ac77 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -423,19 +423,23 @@ enum lnaGain rf69_get_lna_gain(struct spi_device *spi)
 
 int rf69_set_dc_cut_off_frequency_intern(struct spi_device *spi, u8 reg, enum dccPercent dccPercent)
 {
+	int val;
+
 	switch (dccPercent) {
-	case dcc16Percent:	return WRITE_REG(reg, ((READ_REG(reg) & ~MASK_BW_DCC_FREQ) | BW_DCC_16_PERCENT));
-	case dcc8Percent:	return WRITE_REG(reg, ((READ_REG(reg) & ~MASK_BW_DCC_FREQ) | BW_DCC_8_PERCENT));
-	case dcc4Percent:	return WRITE_REG(reg, ((READ_REG(reg) & ~MASK_BW_DCC_FREQ) | BW_DCC_4_PERCENT));
-	case dcc2Percent:	return WRITE_REG(reg, ((READ_REG(reg) & ~MASK_BW_DCC_FREQ) | BW_DCC_2_PERCENT));
-	case dcc1Percent:	return WRITE_REG(reg, ((READ_REG(reg) & ~MASK_BW_DCC_FREQ) | BW_DCC_1_PERCENT));
-	case dcc0_5Percent:	return WRITE_REG(reg, ((READ_REG(reg) & ~MASK_BW_DCC_FREQ) | BW_DCC_0_5_PERCENT));
-	case dcc0_25Percent:	return WRITE_REG(reg, ((READ_REG(reg) & ~MASK_BW_DCC_FREQ) | BW_DCC_0_25_PERCENT));
-	case dcc0_125Percent:	return WRITE_REG(reg, ((READ_REG(reg) & ~MASK_BW_DCC_FREQ) | BW_DCC_0_125_PERCENT));
+	case dcc16Percent:	val = BW_DCC_16_PERCENT; break;
+	case dcc8Percent:	val = BW_DCC_8_PERCENT; break;
+	case dcc4Percent:	val = BW_DCC_4_PERCENT; break;
+	case dcc2Percent:	val = BW_DCC_2_PERCENT; break;
+	case dcc1Percent:	val = BW_DCC_1_PERCENT; break;
+	case dcc0_5Percent:	val = BW_DCC_0_5_PERCENT; break;
+	case dcc0_25Percent:	val = BW_DCC_0_25_PERCENT; break;
+	case dcc0_125Percent:	val = BW_DCC_0_125_PERCENT; break;
 	default:
 		dev_dbg(&spi->dev, "set: illegal input param");
 		return -EINVAL;
 	}
+
+	return WRITE_REG(reg, (READ_REG(reg) & ~MASK_BW_DCC_FREQ) | val);
 }
 
 int rf69_set_dc_cut_off_frequency(struct spi_device *spi, enum dccPercent dccPercent)