Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1752736AbdLEND2 (ORCPT <rfc822;w@1wt.eu>);
        Tue, 5 Dec 2017 08:03:28 -0500
Received: from aserp1040.oracle.com ([141.146.126.69]:16836 "EHLO
        aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752042AbdLENDZ (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 5 Dec 2017 08:03:25 -0500
Date: Tue, 5 Dec 2017 16:03:08 +0300
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Marcus Wolf <marcus.wolf@smarthome-wolf.de>
Cc: devel@driverdev.osuosl.org, gregkh@linuxfoundation.org,
        linux@Wolf-Entwicklungen.de, linux-kernel@vger.kernel.org,
        Simon =?iso-8859-1?Q?Sandstr=F6m?= <simon@nikanor.nu>
Subject: Re: [PATCH 4/6] staging: pi433: Rename enum optionOnOff in
 rf69_enum.h
Message-ID: <20171205130308.ifapjzngjisfx4p2@mwanda>
References: <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>
 <b33f2952-f169-eeb2-f322-e6c4b22358db@smarthome-wolf.de>
 <20171205103008.h7evql7onlaygczi@mwanda>
 <56c0844b-5797-1fc5-551d-f464963fbc43@smarthome-wolf.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <56c0844b-5797-1fc5-551d-f464963fbc43@smarthome-wolf.de>
User-Agent: NeoMutt/20170609 (1.8.3)
X-Source-IP: aserv0021.oracle.com [141.146.126.233]
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1665
Lines: 54

On Tue, Dec 05, 2017 at 01:40:02PM +0100, Marcus Wolf wrote:
> > 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's liked splitting it up but he also proposed this alternative:

rf69_set_sync_operation(spi, true/false);

but I removed the "_operation" because I think it doesn't add anything.

> > 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).

To me it's just weird that "_enable" disables anything.  I really
prefer just splitting it up.  I don't think it will bloat the code.
But I'm also fine with:

rf69_set_sync(spi, true/false)
rf69_set_amp(spi, true/false)
rf69_set_crc(spi, true/false)

Anyway, I feel like I'll like whatever Simon does.  Some of these
things, you can't tell how they'll look until the end until you try.
Let's wait until we see a patch before we debate any more.

regards,
dan carpenter