From mboxrd@z Thu Jan 1 00:00:00 1970 From: Grant Likely Subject: Re: [PATCH 6/9] gpiolib: use descriptors internally Date: Sat, 09 Feb 2013 09:17:29 +0000 Message-ID: <20130209091729.978C73E1A18@localhost> References: <1359822572-26009-1-git-send-email-acourbot@nvidia.com> <1359822572-26009-8-git-send-email-acourbot@nvidia.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Alexandre Courbot , Linus Walleij Cc: linux-arch , Alexandre Courbot , "linux-arm-kernel@lists.infradead.org" , Arnd Bergmann , Linux Kernel Mailing List List-Id: linux-arch.vger.kernel.org On Thu, 7 Feb 2013 15:57:32 +0900, Alexandre Courbot wrote: > On Wed, Feb 6, 2013 at 2:53 AM, Linus Walleij wrote: > > On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot wrote: > >> +/** > >> + * Convert a GPIO number to its descriptor > >> + */ > >> +static struct gpio_desc *gpio_to_desc(unsigned gpio) > >> +{ > >> + if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio)) > >> + return NULL; > > > > Don't we want to return ERR_PTR(-EINVAL); here? > > > > Then you can use IS_ERR() on the pointers later. > > > > This is the approach taken by the external API for clk > > and pins. > > Yes, that completely makes sense. > No, it does not. The ERR_PTR()/IS_ERR() is a horrible pattern for code readability because it breaks the expectations that programmers have for what is and is not a bad pointer. There are decades of history where the test for a bad pointer is 'if (!ptr)'. Not only does ERR_PTR make make that test not work, but the compiler won't tell you when you get it wrong. There are places where ERR_PTR makes sense. Particularly when communicating with userspace where error codes have very specific meanings, but I don't want it in the GPIO subsystem. g. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-we0-f173.google.com ([74.125.82.173]:46570 "EHLO mail-we0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752567Ab3BIKKF (ORCPT ); Sat, 9 Feb 2013 05:10:05 -0500 Received: by mail-we0-f173.google.com with SMTP id r5so3658214wey.32 for ; Sat, 09 Feb 2013 02:10:04 -0800 (PST) From: Grant Likely Subject: Re: [PATCH 6/9] gpiolib: use descriptors internally In-Reply-To: References: <1359822572-26009-1-git-send-email-acourbot@nvidia.com> <1359822572-26009-8-git-send-email-acourbot@nvidia.com> Date: Sat, 09 Feb 2013 09:17:29 +0000 Message-ID: <20130209091729.978C73E1A18@localhost> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Alexandre Courbot , Linus Walleij Cc: Arnd Bergmann , linux-arch , "linux-arm-kernel@lists.infradead.org" , Linux Kernel Mailing List , Alexandre Courbot Message-ID: <20130209091729.Db6wM0tM4wj96rMV7sv4HLTSwBxtrK54kAGXMMxQ4GQ@z> On Thu, 7 Feb 2013 15:57:32 +0900, Alexandre Courbot wrote: > On Wed, Feb 6, 2013 at 2:53 AM, Linus Walleij wrote: > > On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot wrote: > >> +/** > >> + * Convert a GPIO number to its descriptor > >> + */ > >> +static struct gpio_desc *gpio_to_desc(unsigned gpio) > >> +{ > >> + if (WARN(!gpio_is_valid(gpio), "invalid GPIO %d\n", gpio)) > >> + return NULL; > > > > Don't we want to return ERR_PTR(-EINVAL); here? > > > > Then you can use IS_ERR() on the pointers later. > > > > This is the approach taken by the external API for clk > > and pins. > > Yes, that completely makes sense. > No, it does not. The ERR_PTR()/IS_ERR() is a horrible pattern for code readability because it breaks the expectations that programmers have for what is and is not a bad pointer. There are decades of history where the test for a bad pointer is 'if (!ptr)'. Not only does ERR_PTR make make that test not work, but the compiler won't tell you when you get it wrong. There are places where ERR_PTR makes sense. Particularly when communicating with userspace where error codes have very specific meanings, but I don't want it in the GPIO subsystem. g.