Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752562AbdLEMkF (ORCPT ); Tue, 5 Dec 2017 07:40:05 -0500 Received: from dd39320.kasserver.com ([85.13.155.146]:36438 "EHLO dd39320.kasserver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752126AbdLEMkE (ORCPT ); Tue, 5 Dec 2017 07:40:04 -0500 Subject: Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h To: Dan Carpenter Cc: =?UTF-8?Q?Simon_Sandstr=c3=b6m?= , devel@driverdev.osuosl.org, gregkh@linuxfoundation.org, linux@Wolf-Entwicklungen.de, linux-kernel@vger.kernel.org References: <20171203151726.16639-1-simon@nikanor.nu> <20171203151726.16639-5-simon@nikanor.nu> <20171204101737.xv4eyglsrbmiagtn@mwanda> <20171204103719.7talcb3dqu3yfns2@mwanda> <401fb0b7-48a9-dfc9-481b-d4e31f0ed9b5@smarthome-wolf.de> <20171204191522.dcwclu7v7fig65nc@mwanda> <9e438a22-4371-551d-5e71-9803e3f06a44@smarthome-wolf.de> <20171204194240.42rueojkjz7q32hm@kappa.nikanor.nu> <20171205103008.h7evql7onlaygczi@mwanda> From: Marcus Wolf Message-ID: <56c0844b-5797-1fc5-551d-f464963fbc43@smarthome-wolf.de> Date: Tue, 5 Dec 2017 13:40:02 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20171205103008.h7evql7onlaygczi@mwanda> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3139 Lines: 108 Am 05.12.2017 um 13:16 schrieb Dan Carpenter: > On Mon, Dec 04, 2017 at 09:59:02PM +0200, Marcus Wolf wrote: >> 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); >> > > Here's what the code looks like right now: > > 198 /* packet config */ > 199 /* enable */ > 200 SET_CHECKED(rf69_set_sync_enable(dev->spi, rx_cfg->enable_sync)); > 201 if (rx_cfg->enable_sync == optionOn) > 202 { > 203 SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt)); > 204 } > 205 else > 206 { > 207 SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always)); > 208 } > > That's for the rx_cfg. We have related but different code for the > tx_cfg. It's strange to me that we can enable sync for rx and not for > tx... How does that work when the setting ends up getting stored in the > same register? > > The new code would look like this: > > if (rx_cfg->enable_sync) { > ret = rf69_enable_sync(spi); ^^^^^^^^^^^^^^^^^^^^^ > if (ret) > return ret; > ret = rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt); > if (ret) > return ret; > } else { > ret = rf69_disable_sync(dev->spi); > if (ret) > return ret; > ret = rf69_set_fifo_fill_condition(dev->spi, always); > if (ret) > return ret; > } > > 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, 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). All functions in rf69.c should start with rf69_get or rf69_set. Simons proposal: ---------------- rf69_set_sync_enable(spi) rf69_set_sync_disable(spi) rf69_set_amp_enable(spi) rf69_set_amp_disable(spi) rf69_set_crc_enable(spi) rf69_set_crc_disable(spi) I don't like that, because it's blowing up the code without bringing extra feature (see my last mail). In your code example, you use Simons proposal, but in your explaination below, you show the original style - although you refer to Simons sheme... Cheers, Marcus