Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751793AbdLDT7N (ORCPT ); Mon, 4 Dec 2017 14:59:13 -0500 Received: from dd39320.kasserver.com ([85.13.155.146]:41676 "EHLO dd39320.kasserver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751436AbdLDT7M (ORCPT ); Mon, 4 Dec 2017 14:59:12 -0500 Subject: Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in rf69_enum.h To: =?UTF-8?Q?Simon_Sandstr=c3=b6m?= Cc: Dan Carpenter , 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> From: Marcus Wolf Message-ID: Date: Mon, 4 Dec 2017 21:59:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171204194240.42rueojkjz7q32hm@kappa.nikanor.nu> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: de-DE Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1931 Lines: 71 Am 04.12.2017 um 21:42 schrieb Simon Sandström: > On Mon, Dec 04, 2017 at 09:22:06PM +0200, Marcus Wolf wrote: >> >> >> Am 04.12.2017 um 21:15 schrieb Dan Carpenter: >>> >>> That's a bad name, because it doesn't just enable it also disables. >>> Please split them. >>> >>> regards, >>> dan carpenter >>> >>> >> >> Same applies to all other stuff, that's using optionOnOff: >> rf69_set_sync_enable(optionOn/Off) enables and disbales sync, >> rf69_set_crc_enable(optionOn/Off) enables and disables crc, >> ... >> >> In my opinion, if we want perfect clarity, we should stay with optionOnOff. >> If we are ok, if rf69_set_sync_enable(false) disables sync, >> in my opinion, we also have to be ok, if rf69_set_amp_X_enable(false) >> disables the amp. >> >> Cheers, >> Marcus > > Hi, > > I agree with Dan. rf69_enable_sync() / rf69_disable_sync() is clear. If > there are more functions like this (e.g. for crc) then we'll just split > those functions as well. > > If you really want one single function for enabling/disabling then I > think that you need to find a better name. Something like > rf69_set_sync_operation(bool), rf69_set_crc_operation(bool), etc. > > > Regards, > Simon > 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); For me, it is important, that the configuration, you'll have to write in the user space program (aka fill out the config struct) will be 100% non-ambigious and easy to read. Cheers, Marcus