[PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static

[PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static

[Marcus Wolf]

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);
-- 
1.7.10.4

Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static

[Greg KH]

On Sat, Dec 02, 2017 at 05:20:22PM +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>

No blank line before signed-off-by :(

Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static

[Marcus Wolf]

Hi!

I am sorry :-/

Can you recover, or do I need to resend?

Cheers,
Marcus

Am 02.12.2017 um 17:56 schrieb Greg KH:
> On Sat, Dec 02, 2017 at 05:20:22PM +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>
> 
> No blank line before signed-off-by :(
> 

-- 
Marcus Wolf

Wolf-Entwicklungen
Helene-Lange-Weg 23
80637 München

Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Sat, Dec 02, 2017 at 06:01:59PM +0200, Marcus Wolf wrote:
> Hi!
> 
> I am sorry :-/
> 
> Can you recover, or do I need to resend?

I do not hand-edit patches, otherwise it would be impossible for me to
keep up with the review workload...

Do what I said earlier, fix all of these, take the time, verify they are
correct and in the correct format, and then send a patch series.

thanks,

greg k-h

Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static

[Joe Perches]

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)

Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static

[Marcus Wolf]

Am 02.12.2017 um 18:46 schrieb Joe Perches:
> 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)
> 

Hi Joe,

that's a really nice idea.

Like I told at some other point in the discussions, rf69.h and parts of 
rf69.c have their origin in generated text, I derived from the 
datasheet. Therefore not everything is optimized for human readability 
and sometimes, code is repeated, though it could be written without 
repetition, if done a little bit more tricky.

But since I am too stupid and my time for the hobby kernel is so small, 
that I even today couldn't integrate the changes, I implemented already 
in July (and already three times tried to submit), for sure I won't 
start further improvment on the driver, now.

I will think about a redesign according to your proposal after I was 
able to submit the stuff, that's already waiting to be applied for monthes.


By the way: Can you give me a hint, how to invoke the checkpatch.pl.
In summer, I generated my patches with diff, then checked them and tried 
to put them into a mail. The result was, that in every mail there was a 
formal problem - espeially due to the mailtool.
I was suggested to send the mails directly via git. So now I setup git 
to be able to send the patches. I use 'git send-email -1 --annotate', so 
the patch is directly derived from the git and put into the mail. Is 
there a trick, to invoke checkpatch in between?

Thanks a lot,

Marcus

Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static

[Joe Perches]

On Sat, 2017-12-02 at 19:15 +0200, Marcus Wolf wrote:
> Can you give me a hint, how to invoke the checkpatch.pl.

There are lots of ways.

I suggest reading the output of $ ./scripts/checkpatch.pl --help

For instance:

[ after the fact checking ]

$ git checkout -b <branch>
  [ commit one or more changes ]
$ git format-patch -<count_of_commits> -o <directory> 
$ ./scripts/checkpatch.pl <directory>/*

[ or pipe git diff output to checkpatch ]

$ git checkout -b <branch>
  [ edit files and save changes ]
$ git diff --stat -p | ./scripts/checkpatch.pl

[ or run checkpatch on a particular commit ]

$ ./scripts/checkpatch -g <commit>

[ or run checkpatch.pl on a range of commits ]

$ ./scripts/checkpatch.pl -g <rev1..rev2>