Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752151AbdLFPLI (ORCPT ); Wed, 6 Dec 2017 10:11:08 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:48236 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751544AbdLFPLF (ORCPT ); Wed, 6 Dec 2017 10:11:05 -0500 Date: Wed, 6 Dec 2017 16:11:11 +0100 From: Greg KH To: Marcus Wolf Cc: Dan Carpenter , devel@driverdev.osuosl.org, linux@Wolf-Entwicklungen.de, linux-kernel@vger.kernel.org, Simon =?iso-8859-1?Q?Sandstr=F6m?= Subject: Re: [PATCH v2 06/11] staging: pi433: Split rf69_set_crc_enabled into two functions Message-ID: <20171206151111.GA29768@kroah.com> References: <20171205220849.5486-1-simon@nikanor.nu> <20171205220849.5486-7-simon@nikanor.nu> <20171206093703.bgs32doff7svvdf5@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2506 Lines: 74 On Wed, Dec 06, 2017 at 12:07:20PM +0200, Marcus Wolf wrote: > > > Am 06.12.2017 um 11:37 schrieb Dan Carpenter: > > On Wed, Dec 06, 2017 at 11:05:22AM +0200, Marcus Wolf wrote: > > > > > > > > > Am 06.12.2017 um 00:08 schrieb Simon Sandstr?m: > > > > Splits rf69_set_crc_enabled(dev, enabled) into > > > > rf69_enable_crc(dev) and rf69_disable_crc(dev). > > > > > > > > Signed-off-by: Simon Sandstr?m > > > > --- > > > > drivers/staging/pi433/pi433_if.c | 22 ++++++++++++++++++++-- > > > > drivers/staging/pi433/rf69.c | 18 ++++++------------ > > > > drivers/staging/pi433/rf69.h | 4 ++-- > > > > 3 files changed, 28 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c > > > > index 2ae19ac565d1..614eec7dd904 100644 > > > > --- a/drivers/staging/pi433/pi433_if.c > > > > +++ b/drivers/staging/pi433/pi433_if.c > > > > @@ -216,7 +216,16 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg) > > > > return ret; > > > > } > > > > SET_CHECKED(rf69_set_adressFiltering(dev->spi, rx_cfg->enable_address_filtering)); > > > > - SET_CHECKED(rf69_set_crc_enable (dev->spi, rx_cfg->enable_crc)); > > > > + > > > > + if (rx_cfg->enable_crc == OPTION_ON) { > > > > + ret = rf69_enable_crc(dev->spi); > > > > + if (ret < 0) > > > > + return ret; > > > > + } else { > > > > + ret = rf69_disable_crc(dev->spi); > > > > + if (ret < 0) > > > > + return ret; > > > > + } > > > > > > Why don't you use SET_CHECKED(...)? > > > > > > > Marcus, please don't introduce new uses of SET_CHECKED(). It has a > > hidden return in it which is against kernel style and introduces very > > predictable and avoidable bugs. For example, in probe(). > > Ah ok. > > Thanks for clarifiytion! > > What a pitty - another bunch of extra lines of code... > > Or is there an other construction, allowing for one line per register > change? Something like > > ret = rf69_set_xyz(...); if (ret) return ret; > ret = rf69_set_abc(...); if (ret) return ret; > > is pretty ugly and voids the style guide... Just spell it out: ret = rf69_set_xyz(); if (ret) goto unwind_xyz; Almost never do you want to instantly return. You should clean up from the error first. But if you do just want to exit, that's fine too, just return. That's the normal way here, don't do funny things in macros (like return from a function), that way lies madness... thanks, greg k-h