[PATCH] staging: pi433: Make rf69_set_dc_cut_off_frequency_intern static

Marcus Wolf marcus.wolf at smarthome-wolf.de
Sun Dec 3 11:21:51 UTC 2017


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


More information about the devel mailing list