Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static
From: Marcus Wolf
Date: Sat Dec 02 2017 - 12:15:56 EST
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@xxxxxxxxxxxxxxxxxxxxx>
---
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