Messages in this thread | | | Date | Tue, 5 Dec 2017 16:03:08 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h |
| |
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
| |