From patchwork Fri Mar 24 08:00:06 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= X-Patchwork-Id: 743097 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3vqGbN39Fpz9s83 for ; Fri, 24 Mar 2017 19:21:20 +1100 (AEDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753717AbdCXIBe (ORCPT ); Fri, 24 Mar 2017 04:01:34 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:53511 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756359AbdCXIAh (ORCPT ); Fri, 24 Mar 2017 04:00:37 -0400 Received: from pty.hi.pengutronix.de ([2001:67c:670:100:1d::c5]) by metis.ext.pengutronix.de with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1crK8m-0004C1-KN; Fri, 24 Mar 2017 09:00:08 +0100 Received: from ukl by pty.hi.pengutronix.de with local (Exim 4.84_2) (envelope-from ) id 1crK8k-000129-3C; Fri, 24 Mar 2017 09:00:06 +0100 Date: Fri, 24 Mar 2017 09:00:06 +0100 From: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= To: Dmitry Torokhov Cc: Linus Walleij , Geert Uytterhoeven , Richard Genoud , Greg Kroah-Hartman , Geert Uytterhoeven , Nicolas Ferre , Boris Brezillon , "linux-arm-kernel@lists.infradead.org" , "linux-serial@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Janusz Uzycki , "linux-gpio@vger.kernel.org" Subject: Re: [PATCH 4/4] tty/serial: sh-sci: remove uneeded IS_ERR_OR_NULL calls Message-ID: <20170324080006.tbhyqlgk35xybsna@pengutronix.de> References: <20170323101045.u3uigdu5xfwjmjc7@pengutronix.de> <20170323111106.7ogh6g2oa3m4cqc6@pengutronix.de> <20170323123437.uqdwhfmmsjke3f7s@pengutronix.de> <20170323144325.GA952@dtor-ws> <20170323154441.GA9460@dtor-ws> <20170323191020.nrstp2cfh7cuqvf3@pengutronix.de> <20170323195804.GA2502@dtor-ws> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170323195804.GA2502@dtor-ws> User-Agent: Mutt/1.6.2-neo (2016-06-11) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c5 X-SA-Exim-Mail-From: ukl@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-gpio@vger.kernel.org Sender: linux-gpio-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org Hello Dmitry, On Thu, Mar 23, 2017 at 12:58:04PM -0700, Dmitry Torokhov wrote: > On Thu, Mar 23, 2017 at 08:10:20PM +0100, Uwe Kleine-König wrote: > > On Thu, Mar 23, 2017 at 08:44:41AM -0700, Dmitry Torokhov wrote: > > > On Thu, Mar 23, 2017 at 07:43:25AM -0700, Dmitry Torokhov wrote: > > > > On Thu, Mar 23, 2017 at 02:41:53PM +0100, Linus Walleij wrote: > > > > > On Thu, Mar 23, 2017 at 1:34 PM, Uwe Kleine-König > > > > > wrote: > > > > > > > > > > > Maybe we can make gpiod_get_optional look like this: > > > > > > > > > > > > if (!dev->of_node && isnt_a_acpi_device(dev) && !IS_ENABLED(GPIOLIB)) > > > > > > return NULL; > > > > > > else > > > > > > return -ENOSYS; > > > > > > > > > > > > I don't know how isnt_a_acpi_device looks like, probably it involves > > > > > > CONFIG_ACPI and/or dev->acpi_node. > > > > > > > > > > > > This should be safe and still comfortable for legacy platforms, isn't it? > > > > > > > > > > I like the looks of this. > > > > > > > > > > Can we revert Dmitry's patch and apply something like this instead? > > > > > > > > > > Dmitry, how do you feel about this? > > > > > > > > I frankly do not see the point. It still makes driver code more complex > > > > Note that this code is in the gpio header, and not in driver code. So > > the driver just does > > > > gpiod = gpiod_get_optional(...) > > if (IS_ERR(gpiod)) > > return PTR_ERR(gpiod); > > > > (as it is supposed to do now). I think that's nice. > > Except that it breaks if !CONFIG_GPIOLIB which is legitimate config in > many cases. Can I have a DT platform or ACPI platform that does not > expose any GPIOs for kernel to control and thus want to disable GPIOLIB? > I can't see why not. > > > > > > > for no good reason. I also think that not having optional GPIO is not an > > > > error, so returning value from error space is not correct. NULL is value > > > > from another space altogether. > > > > It seems you didn't understand my concern. > > Likewise. OK, lets agree that we don't agree then. You think that if someone disabled GPIOLIB it should be safe to assume that there is no GPIO, I think that's wrong. Still I think it should be possible that we agree on my suggestion to return NULL in some cases only. Here is a prototype (not even compile tested without GPIOLIB): ------->8-------- From: Uwe Kleine-König Subject: [PATCH] gpiod: let get_optional return NULL in some cases with GPIOLIB disabled People disagree if gpiod_get_optional should return NULL or ERR_PTR(-ENOSYS) if GPIOLIB is disabled. The argument for NULL is that the person who decided to disable GPIOLIB is assumed to know that there is no GPIO. The reason to stick to ERR_PTR(-ENOSYS) is that it might introduce hard to debug problems if that decision is wrong. So this patch introduces a compromise and let gpiod_get_optional (and its variants) return NULL if the device in question cannot have an associated GPIO because it is neither instantiated by a device tree nor by ACPI. This should handle most cases that are argued about. Signed-off-by: Uwe Kleine-König --- include/linux/gpio/consumer.h | 55 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 9 deletions(-) Best regards Uwe diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index fb0fde686cb1..0ca29889290d 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -161,20 +161,48 @@ gpiod_get_index(struct device *dev, return ERR_PTR(-ENOSYS); } -static inline struct gpio_desc *__must_check -gpiod_get_optional(struct device *dev, const char *con_id, - enum gpiod_flags flags) +static inline bool __gpiod_no_optional_possible(struct device *dev) { - return ERR_PTR(-ENOSYS); + /* + * gpiod_get_optional et al can only provide a GPIO if at least one of + * the backends for specifing a GPIO is available. These are device + * tree, ACPI and gpiolib's lookup tables. The latter isn't available if + * GPIOLIB is disabled (which is the case here). + * So if the provided device is unrelated to device tree and ACPI, we + * can be sure that there is no optional GPIO and let gpiod_get_optional + * safely return NULL. + * Otherwise there is still a chance that there is no GPIO but we cannot + * be sure without having to enable a part of GPIOLIB (i.e. the lookup + * part). So lets play safe and return an error. (Though there are also + * arguments that returning NULL then would be beneficial.) + */ + + if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) + return false; + + if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_COMPANION(dev)) + return false; + + return true; } static inline struct gpio_desc *__must_check gpiod_get_index_optional(struct device *dev, const char *con_id, unsigned int index, enum gpiod_flags flags) { + if (__gpiod_no_optional_possible(dev)) + return NULL; + return ERR_PTR(-ENOSYS); } +static inline struct gpio_desc *__must_check +gpiod_get_optional(struct device *dev, const char *con_id, + enum gpiod_flags flags) +{ + return gpiod_get_index_optional(dev, con_id, 0, flags); +} + static inline struct gpio_descs *__must_check gpiod_get_array(struct device *dev, const char *con_id, enum gpiod_flags flags) @@ -186,6 +214,9 @@ static inline struct gpio_descs *__must_check gpiod_get_array_optional(struct device *dev, const char *con_id, enum gpiod_flags flags) { + if (__gpiod_no_optional_possible(dev)) + return NULL; + return ERR_PTR(-ENOSYS); } @@ -223,17 +254,20 @@ devm_gpiod_get_index(struct device *dev, } static inline struct gpio_desc *__must_check -devm_gpiod_get_optional(struct device *dev, const char *con_id, - enum gpiod_flags flags) +devm_gpiod_get_index_optional(struct device *dev, const char *con_id, + unsigned int index, enum gpiod_flags flags) { + if (__gpiod_no_optional_possible(dev)) + return NULL; + return ERR_PTR(-ENOSYS); } static inline struct gpio_desc *__must_check -devm_gpiod_get_index_optional(struct device *dev, const char *con_id, - unsigned int index, enum gpiod_flags flags) +devm_gpiod_get_optional(struct device *dev, const char *con_id, + enum gpiod_flags flags) { - return ERR_PTR(-ENOSYS); + return devm_gpiod_get_index_optional(dev, con_id, 0, flags); } static inline struct gpio_descs *__must_check @@ -247,6 +281,9 @@ static inline struct gpio_descs *__must_check devm_gpiod_get_array_optional(struct device *dev, const char *con_id, enum gpiod_flags flags) { + if (__gpiod_no_optional_possible(dev)) + return NULL; + return ERR_PTR(-ENOSYS); }