Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753646Ab3FJVn3 (ORCPT ); Mon, 10 Jun 2013 17:43:29 -0400 Received: from 7of9.schinagl.nl ([88.159.158.68]:41970 "EHLO 7of9.schinagl.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752857Ab3FJVnV (ORCPT ); Mon, 10 Jun 2013 17:43:21 -0400 Message-ID: <51B64877.4040908@schinagl.nl> Date: Mon, 10 Jun 2013 23:43:19 +0200 From: Oliver Schinagl User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130528 Thunderbird/17.0.6 MIME-Version: 1.0 To: Andy Shevchenko CC: maxime.ripard@free-electrons.com, Arnd Bergmann , Greg Kroah-Hartman , "linux-kernel@vger.kernel.org" , linux-arm Mailing List , Oliver Schinagl Subject: Re: [PATCH 1/2] Initial support for Allwinner's Security ID fuses References: <1370185130-15332-1-git-send-email-oliver+list@schinagl.nl> <1370185130-15332-2-git-send-email-oliver+list@schinagl.nl> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1777 Lines: 53 On 06/06/13 21:16, Andy Shevchenko wrote: Thank you andy for your review, I do have a few questions/comments if you don't mind. > On Sun, Jun 2, 2013 at 5:58 PM, Oliver Schinagl wrote: >> From: Oliver Schinagl >> + if (likely((SID_SIZE))) { > > Extra braces. > Use antipattern here. While I accidentally dropped the pointer here, sorry for the confusion, what is antipattern? I have asked around and nobody really knew. Wikipedia mentions it as a software development thing, but you make it sound like it is some sort of tool? >> + if (unlikely(!pdev->dev.of_node)) { >> + dev_err(dev, "No devicetree data available\n"); >> + ret = -EFAULT; >> + goto exit; > > Plain return here and in entire function where it applies. Why? I know there's conflicting preferences here. The general consensus seems, don't return mid function if you don't absolutely have to. Yet, you make it sound, just return wherever. I take it that really is just a preference? I think i see both constructs throughout the kernel. So one review prefers the one method, the next the other? >> + >> + ret = device_create_bin_file(dev, &sid_bin_attr); >> + if (unlikely(ret)) { > > Any benifit of (un)likely in probe()? Does it hurt however in any way though? It's just a compiler optimization isn't it. > > -- > With Best Regards, > Andy Shevchenko > Thank you for your time, it is much appreciated :) Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/