Subject: Re: [PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern
 static
To: Joe Perches <joe@perches.com>,
        Marcus Wolf <linux@wolf-entwicklungen.de>, gregkh@linuxfoundation.org,
        dan.carpenter@oracle.com, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
References: <1512228022-22599-1-git-send-email-linux@wolf-entwicklungen.de>
 <1512233175.6321.11.camel@perches.com>
From: Marcus Wolf <marcus.wolf@smarthome-wolf.de>
Message-ID: <0b0afaef-aa5e-5b45-e570-8ce6a100225a@smarthome-wolf.de>
Date: Sat, 2 Dec 2017 19:15:50 +0200
User-Agent: Mozilla/5.0 (X11; Linux i686; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <1512233175.6321.11.camel@perches.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: de-DE
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6394
Lines: 144


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