Am 03.12.2017 um 11:56 schrieb Marcin Ciupak:
On Sat, Dec 02, 2017 at 08:46:15AM -0800, Joe Perches wrote:
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 at 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)
I was going to propose sth similar that on Friday (but was waiting
for changes from Marcus) and additionally I wanted to go a step further
i.e. instead of using enum and #define merge it and use one more
compact solution as follows:
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index e69a2153c999..49f853124e9a 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -45,11 +45,12 @@ int rf69_set_mode(struct spi_device *spi, enum mode mode)
#endif
switch (mode) {
- case transmit: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & ~MASK_OPMODE_MODE) | OPMODE_MODE_TRANSMIT);
- case receive: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & ~MASK_OPMODE_MODE) | OPMODE_MODE_RECEIVE);
- case synthesizer: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & ~MASK_OPMODE_MODE) | OPMODE_MODE_SYNTHESIZER);
- case standby: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & ~MASK_OPMODE_MODE) | OPMODE_MODE_STANDBY);
- case mode_sleep: return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & ~MASK_OPMODE_MODE) | OPMODE_MODE_SLEEP);
+ case OPMODE_MODE_TRANSMIT:
+ case OPMODE_MODE_RECEIVE:
+ case OPMODE_MODE_SYNTHESIZER:
+ case OPMODE_MODE_STANDBY:
+ case OPMODE_MODE_SLEEP:
+ return WRITE_REG(REG_OPMODE, (READ_REG(REG_OPMODE) & ~MASK_OPMODE_MODE) | mode);
default:
dev_dbg(&spi->dev, "set: illegal input param");
return -EINVAL;
diff --git a/drivers/staging/pi433/rf69_enum.h b/drivers/staging/pi433/rf69_enum.h
index 86429aa66ad1..abf6bb9d8447 100644
--- a/drivers/staging/pi433/rf69_enum.h
+++ b/drivers/staging/pi433/rf69_enum.h
@@ -24,11 +24,11 @@ enum optionOnOff {
};
enum mode {
- mode_sleep,
- standby,
- synthesizer,
- transmit,
- receive
+ OPMODE_MODE_SLEEP = 0x00,
+ OPMODE_MODE_STANDBY = 0x04, /* default */
+ OPMODE_MODE_SYNTHESIZER = 0x08,
+ OPMODE_MODE_TRANSMIT = 0x0C,
+ OPMODE_MODE_RECEIVE = 0x10
};
enum dataMode {
This is a change in other function, but idea here is the same. The
advantage is that there is no need to store #define value in local
variable and instead just pass directly value of enum which already has
the correct value.
I would like to hear any comments before touching 80% of rf69.c code and
got it rejected ;)
Thanks,
Marcin
Hi Marcin,
looks nice.
I see three disadvantages:
* You are touching the enums, that are used in user space as well. Thus
you will need to increase or change the version number of the (old
fashioned) IOCTL-Interface (according to TODO-list subject to be
changed)and therefore render all apps, already available, useless.
But since driver is young, most probably just me and maybe three/four of
the nerds, who bought a Pi433 will suffer from that.
* You are increasing the readability of the code, for everybody, that is
having a "theoretical" (don't know a better word) look on the code. For
someone, who will practically work with the code, things get a little
bit worse, since you pull apart some of the register based values
(some will stay in rf69_regs.h, and some will go to rf69_enums.h). So in
future you need to keep track of two files, if you want to extend or
modify the register access (by far not all registers are abstracted so
far - see the huge list of commeted out text in rf69_regs).
* My first plan was to write a generic driver, able to support more,
then yust the rf69 chip, but also other chips of Hope RF.
Together with the pi433-Interface, the current driver is a kind of
abstraction of the rfm69-433 (integrated module of Hope RF). I plan to
also support the rfm69-868. That can be done with your change without a
problem, if we take care to place defines in the right files (see my
rejected patch from yesterday).
But in future there might be even the wish, to support the rf12 or one
of the new 433/868 chips (don't remeber the numbers) of Hope RF. They
all have a similar interface and a similar internal logic. But not
identical, just similar. In that case, it might be (never checked it),
that the modulation type OOK needs an other bit mask for one of the new
cips, then it neededs for the rf69.
Therfore, I distingwished between "labels" for the user and defines
containing the values, needed for chip acces.
I really like your short and lovely code. In addition, it complies a lot
better with the formal rules for long lines.
Please have a look on my rejected patch, that replaces the READ_REG and
WRITE_REG defines against some higher-level inline functions (setBit,
rstBit and rmw).
Me and some other fellows are of the opinion, that it is also a nice
enhancement - on compile time and on readability of the code.
Maybe you want to adopt the idea, since my patch is rendered useless, if
your changes will aply.
Result:
-------
It's pretty hard for me to decide, whether the disadvantages mentioned
above are that important, that you/we should stop such kind of optimization.
Further info about my plans (past and future)and my implementations:
--------------------------------------------------------------------
I already prepared code snippets, that could be used for supporting
other rf chips (something like rfxx.c) and I had a look on supporting
the rfm12. But there was no one interested in having such support. I
even tried three times, to involve the producer Hope RF. But Hope RF was
of the opinion, that the typical customer isn't interested in a driver
for linux, but wants to write his own driver.
With no one being interested in such a driver an no one supporting me
finanically, all this stays just hobby.
Since I am too stupid (or barriers for persons, just submitting one
patch a month are too high), I wasn't able to place a single patch,
after the initial version of my driver was submited. So all my time and
motivation for improving the driver was taken for learning how to not
have a space too much or a new line to few in a patch or a an upset
person, because I used top-posting. To be honest: I am using email for
25 years now, never used bottom-posting and there never was any
complaint about that. Maybe that's not only realted to the person,
writing a mail, but also to the style, how a person is reading an email...
Never the less: There was no time and motiviation left. Technical
development or even extention of the driver was completely left behind
for half a year now - though I even have Raspi shields with the other
modules here, since I tested a lot, before I decided to use a rfm69-433
on Pi433.
Hope some of the information is helping you and others in taking the
best decision, how to proceed with your proposal.
Finally a big thank you for your interesd in the driver!
If you need a Pi433 for testing, pls. send me a PN.
Marcus
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
