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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
