Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h

[Simon SandstrÃm]

On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote:
> 
> Hi Simon, hi Dan,
> 
> if you both are of the same opinion, for me, it's fine, if we go with two
> functions.
> 
> But I don't get the advantage, if we split approx. 10 functions, to get rid
> of enum optionOnOff.
> 
> Keep in mind, that if you split the functions, in the interface
> implementation you also need more code:
> 
> SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync));
> 
> will have to be converted in something like
> 
> if (rx_cfg->enable_sync)
> 	SET_CHECKED(rf69_set_sync_enbable(dev->spi);
> else
> 	SET_CHECKED(rf69_set_sync_disable(dev->spi);

I think that this makes the code very clear. If the config tells us to
enable the sync then we'll enabled it, otherwise we'll disable it.

> 
> For me, it is important, that the configuration, you'll have to write in the
> user space program (aka fill out the config struct) will be 100%
> non-ambigious and easy to read.
> 
> Cheers,
> Marcus

- Simon