From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by ash.osuosl.org (Postfix) with ESMTP id 23C241C0ADB for ; Tue, 15 May 2018 07:42:30 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 20EB486CCD for ; Tue, 15 May 2018 07:42:30 +0000 (UTC) Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id k5+11aIO7yyp for ; Tue, 15 May 2018 07:42:29 +0000 (UTC) Received: from userp2120.oracle.com (userp2120.oracle.com [156.151.31.85]) by hemlock.osuosl.org (Postfix) with ESMTPS id 7517E86A57 for ; Tue, 15 May 2018 07:42:29 +0000 (UTC) Date: Tue, 15 May 2018 10:42:17 +0300 From: Dan Carpenter Subject: Re: [PATCH 03/14] staging: clocking-wizard: Split probe function Message-ID: <20180515074217.ptcemlso2uxi2vvv@mwanda> References: <20180507012040.18187-1-jamespeterkelly@gmail.com> <20180507012040.18187-4-jamespeterkelly@gmail.com> <20180514134726.zdl46z46qe4thx7x@mwanda> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: James Kelly Cc: Greg Kroah-Hartman , driverdev-devel@linuxdriverproject.org, Michal Simek On Tue, May 15, 2018 at 05:30:15AM +1000, James Kelly wrote: > Dan, > > > On 14 May 2018, at 11:47 pm, Dan Carpenter wrote: > > > > On Mon, May 07, 2018 at 11:20:29AM +1000, James Kelly wrote: > >> +static int clk_wzrd_probe(struct platform_device *pdev) > >> +{ > >> + int ret; > >> + struct device *dev = &pdev->dev; > >> + > >> + ret = clk_wzrd_get_device_tree_data(dev); > >> + if (ret) > >> + return ret; > >> + > >> + ret = clk_wzrd_regmap_alloc(dev); > >> + if (ret) > >> + return ret; > >> + > >> + ret = clk_wzrd_register_ccf(dev); > >> + if (ret) > >> + return ret; > >> + > >> + return 0; > > > > The error handling is a terrible layer violation now... Every > > allocation function should have a matching free function. But now > > instead of that if clk_wzrd_register_ccf() fails then it starts cleaning > > up the clk_wzrd->axi_clk that was allocated in > > clk_wzrd_get_device_tree_data(). > > > > regards, > > dan carpenter > > > > > > This gets cleaned up in patch 6. There seemed little point in putting a whole lot of > code in patch 3 only to rip it out again in patch 6. I was trying to keep the patches > simple. I later posted that it's actually buggy so it's not allow to introduce a bug and then fix it. I really quarrel with the word "simple". As a reviewer it's my job to spot bugs being introduced. It took me a long time to review this patch until I spotted the bug. It's better to not take short cuts, but just write error handling in the standard way. regards, dan carpenter _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel