From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751728AbcGSSzY (ORCPT ); Tue, 19 Jul 2016 14:55:24 -0400 Received: from down.free-electrons.com ([37.187.137.238]:55702 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750915AbcGSSzX (ORCPT ); Tue, 19 Jul 2016 14:55:23 -0400 Date: Tue, 19 Jul 2016 20:55:21 +0200 From: Boris Brezillon To: Andrey Smirnov Cc: Brian Norris , linux-mtd@lists.infradead.org, Richard Weinberger , David Woodhouse , linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] mtd: nand: Get rid of needless 'goto' Message-ID: <20160719205521.3a816b19@bbrezillon> In-Reply-To: References: <1468942904-26464-1-git-send-email-andrew.smirnov@gmail.com> <1468942904-26464-2-git-send-email-andrew.smirnov@gmail.com> <20160719183047.GC85399@google.com> Organization: Free Electrons X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 19 Jul 2016 11:48:04 -0700 Andrey Smirnov wrote: > On Tue, Jul 19, 2016 at 11:30 AM, Brian Norris > wrote: > > On Tue, Jul 19, 2016 at 08:41:44AM -0700, Andrey Smirnov wrote: > >> Using "goto" in that "switch" statement only makes it harder to follow > >> control flow and doesn't bring any advantages. Rewrite the code to avoid > >> using "goto". > >> > >> Signed-off-by: Andrey Smirnov > >> --- > >> drivers/mtd/nand/nand_base.c | 13 +++++-------- > >> 1 file changed, 5 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > >> index 57043a6..8fa5536 100644 > >> --- a/drivers/mtd/nand/nand_base.c > >> +++ b/drivers/mtd/nand/nand_base.c > >> @@ -2139,18 +2139,15 @@ static int nand_read_oob(struct mtd_info *mtd, loff_t from, > >> case MTD_OPS_PLACE_OOB: > >> case MTD_OPS_AUTO_OOB: > >> case MTD_OPS_RAW: > >> + if (!ops->datbuf) > >> + ret = nand_do_read_oob(mtd, from, ops); > >> + else > >> + ret = nand_do_read_ops(mtd, from, ops); > >> break; > >> - > >> default: > >> - goto out; > >> + break; > >> } > >> > >> - if (!ops->datbuf) > >> - ret = nand_do_read_oob(mtd, from, ops); > >> - else > >> - ret = nand_do_read_ops(mtd, from, ops); > >> - > >> -out: > >> nand_release_device(mtd); > >> return ret; > >> } > > > > The default case is really just a catch-all error case. We don't > > actually even need the nand_get_device() call for that... can we just > > do this instead? > > Sure, although, if you don't mind, I'd rather you used: > > if (ops->mode != MTD_OPS_PLACE_OOB && > ops->mode != MTD_OPS_AUTO_OOB && > ops->mode != MTD_OPS_RAW) > return -ENOTSUPP; Or just if (ops->mode < MTD_OPS_PLACE_OOB || ops->mode > MTD_OPS_RAW) return -ENOTSUPP; Anyway, I'm fine with all different versions as long as you don't take the nand lock if the mode is incorrect, so I'll let Brian decide. > > instead of the switch statement, IMHO, this way it is more obvious > that this codepath is just arguments correctness checking. > > Andrey