Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752302AbdLDUFe (ORCPT ); Mon, 4 Dec 2017 15:05:34 -0500 Received: from mail-lf0-f54.google.com ([209.85.215.54]:38658 "EHLO mail-lf0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752255AbdLDUF3 (ORCPT ); Mon, 4 Dec 2017 15:05:29 -0500 X-Google-Smtp-Source: AGs4zMbO1y5NTKD/SUsUaYQjMjLmcaS+sI5XGhZfUL9fHv4+IfsONVb+NWGy3QQA2OR/4/IPuxJcKw== Date: Mon, 4 Dec 2017 21:05:25 +0100 From: Simon =?utf-8?Q?Sandstr=C3=B6m?= To: Marcus Wolf Cc: Dan Carpenter , 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: <20171204200524.i2nl7gz6egga4jpt@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> <20171204194240.42rueojkjz7q32hm@kappa.nikanor.nu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 998 Lines: 34 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. > > 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 - Simon