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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
