| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 2 Dec 2017 19:15:50 +0200
From: Marcus Wolf <marcus.wolf@...rthome-wolf.de>
To: Joe Perches <joe@...ches.com>,
Marcus Wolf <linux@...f-entwicklungen.de>,
gregkh@...uxfoundation.org, dan.carpenter@...cle.com,
devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern
static
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@...f-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
Powered by blists - more mailing lists