Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752583AbdLEMQu (ORCPT ); Tue, 5 Dec 2017 07:16:50 -0500 Received: from userp2130.oracle.com ([156.151.31.86]:52416 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752042AbdLEMQs (ORCPT ); Tue, 5 Dec 2017 07:16:48 -0500 Date: Tue, 5 Dec 2017 15:16:29 +0300 From: Dan Carpenter To: Marcus Wolf Cc: Simon =?iso-8859-1?Q?Sandstr=F6m?= , 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: <20171205103008.h7evql7onlaygczi@mwanda> 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/20170609 (1.8.3) X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=8735 signatures=668637 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1712050176 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2021 Lines: 64 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