Am 04.12.2017 um 21:15 schrieb Dan Carpenter:
On Mon, Dec 04, 2017 at 08:37:51PM +0200, Marcus Wolf wrote:
Am 04.12.2017 um 12:37 schrieb Dan Carpenter:
On Mon, Dec 04, 2017 at 01:17:37PM +0300, Dan Carpenter wrote:
Perhaps choose different function names if you want? You could do it
as several patches:
patch 1: change types to bool
patch 2: sed -e '/ == optionOn//'
patch 3: split the functions into two functions
patch 4: delete optionOnOff enum
patches 1 and 2 could be merged together (your choice).
Markus says that optionOn is used by user space so my you won't be able
to remove these entirely. But as much as possible we should internally.
regards,
dan carpenter
Hi Dan, hi Simon,
I think, it's a pretty nice idea to remove th optionOnOff and replace it by
bool.
<history>
In former times, the variables in the config struct had very different names
- not containing "enable". Therefore optionOnOff was used to make absolutely
clear (in user space), wheter something was switched on, or off.
Now the variable have nice names, so bool is fine, even better now :-)
</history>
I would suggest not to split the amp-functions but to rename them, to also
contain an enable:
rf69_set_amp_X_enable()
That's a bad name, because it doesn't just enable it also disables.
Please split them.
regards,
dan carpenter
Same applies to all other stuff, that's using optionOnOff:
rf69_set_sync_enable(optionOn/Off) enables and disbales sync,
rf69_set_crc_enable(optionOn/Off) enables and disables crc,
...
In my opinion, if we want perfect clarity, we should stay with optionOnOff.
If we are ok, if rf69_set_sync_enable(false) disables sync,
in my opinion, we also have to be ok, if rf69_set_amp_X_enable(false)
disables the amp.
Cheers,
Marcus
