Date: Mon, 4 Dec 2017 20:42:41 +0100
From: Simon =?utf-8?Q?Sandstr=C3=B6m?= <simon@nikanor.nu>
To: Marcus Wolf <marcus.wolf@smarthome-wolf.de>
Cc: Dan Carpenter <dan.carpenter@oracle.com>, devel@driverdev.osuosl.org,
        gregkh@linuxfoundation.org, linux@Wolf-Entwicklungen.de,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in
 rf69_enum.h
Message-ID: <20171204194240.42rueojkjz7q32hm@kappa.nikanor.nu>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9e438a22-4371-551d-5e71-9803e3f06a44@smarthome-wolf.de>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1100
Lines: 39

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