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

[Dan Carpenter]

On Tue, Dec 05, 2017 at 01:40:02PM +0100, Marcus Wolf wrote:
> > It's not the greatest, but it's not the worst...  The configuration for
> > ->enable_sync is a bit spread out and it might be nice to move it all to
> > one function?
> > 
> > I liked Simon's naming scheme and I thought it was clear what the
> > rf69_set_sync(spi, false) function would do.
>   ^^^^^^^^^^^^^^^^^^^^^^^^^
> > 

Simon's liked splitting it up but he also proposed this alternative:

rf69_set_sync_operation(spi, true/false);

but I removed the "_operation" because I think it doesn't add anything.

> > Simon, it seems like Marcus and I both are Ok with your style choices.
> > Do whatever seems best when you implement the code.  If it's awkward to
> > break up the functions then don't.
> > 
> > regards,
> > dan carpenter
> > 
> 
> Hi Dan,
> 
> now I am a bit confused.
> 
> My favourit:
> ------------
> rf69_set_sync_enable(spi, false)
> rf69_set_amp_enable(spi, false)
> rf69_set_crc_enable(spi, false)
> 
> I prefer to keep the enable (or comparable), because it shows, what the
> function is doing. For sync, for example, there are several setter:
> size, tolerance, values ... AND enable (or comparable).

To me it's just weird that "_enable" disables anything.  I really
prefer just splitting it up.  I don't think it will bloat the code.
But I'm also fine with:

rf69_set_sync(spi, true/false)
rf69_set_amp(spi, true/false)
rf69_set_crc(spi, true/false)

Anyway, I feel like I'll like whatever Simon does.  Some of these
things, you can't tell how they'll look until the end until you try.
Let's wait until we see a patch before we debate any more.

regards,
dan carpenter