Messages in this thread | | | Subject | Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static | From | Marcus Wolf <> | Date | Sat, 2 Dec 2017 19:15:50 +0200 |
| |
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
| |