Am 04.12.2017 um 21:05 schrieb 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. > Hi Simon, I thought about it a while. Yes, you are right, it makes it very clear - but doesn't tell a lot extra. The only thing, you can see extra, is, that there is the option, to turn on and to turn off. You even can't see, whether it is turend on or off. The info, that there is an option of en- or disabling - at least from my side - is visible, too, if there is just one function that takes a bool or optionOnOff as argument. When you 'll redesign every setter, that now deals with optionOnOff in that way, you will introduce something like 50 extra lines of code without bringing any extra functionality to the driver. >From my point of view, that's bad: The longer the text, the harder to understand everything entierly, the more chances for bugs and a lot harder to service. So from my point of view such a redesign is nice for optics but bad for further development. I would really prefer a solution, like Marcin Ciupak offered. He is not blowing up the code, but even tries to reduce the calls to READ_REG and WRITE_REG. That increases readbility, too, but also reduces code size and chances for bugs. But regardless of the arguments above, I don't mind. If it's a benefit for you and lot of others in the community, I won't blame you for such a change. If you would add 50 lines of code with new funcitonality (e. g. support the AES feature of the module), I would be a lot happier - and most useres for sure, too. Cheers, Marcus _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
