From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6A565E748EE for ; Sun, 1 Oct 2023 09:22:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234700AbjJAJWI (ORCPT ); Sun, 1 Oct 2023 05:22:08 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34660 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234641AbjJAJWF (ORCPT ); Sun, 1 Oct 2023 05:22:05 -0400 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.24]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 09D09AC; Sun, 1 Oct 2023 02:22:03 -0700 (PDT) X-IronPort-AV: E=McAfee;i="6600,9927,10849"; a="385340058" X-IronPort-AV: E=Sophos;i="6.03,191,1694761200"; d="scan'208";a="385340058" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Oct 2023 02:22:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10849"; a="816030252" X-IronPort-AV: E=Sophos;i="6.03,191,1694761200"; d="scan'208";a="816030252" Received: from smile.fi.intel.com ([10.237.72.54]) by fmsmga008.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Oct 2023 02:21:58 -0700 Received: from andy by smile.fi.intel.com with local (Exim 4.97-RC0) (envelope-from ) id 1qmseA-00000001rYi-08n0; Sun, 01 Oct 2023 12:21:54 +0300 Date: Sun, 1 Oct 2023 12:21:53 +0300 From: Andy Shevchenko To: Linus Walleij Cc: Kent Gibson , Duje =?utf-8?Q?Mihanovi=C4=87?= , Daniel Mack , Haojian Zhuang , Robert Jarzmik , Russell King , Alan Stern , Greg Kroah-Hartman , Bartosz Golaszewski , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org, linux-gpio@vger.kernel.org Subject: Re: [PATCH RFC 1/6] ARM: pxa: Convert Spitz OHCI to GPIO descriptors Message-ID: References: <20230924-pxa-gpio-v1-0-2805b87d8894@skole.hr> <20230924-pxa-gpio-v1-1-2805b87d8894@skole.hr> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Oct 01, 2023 at 11:18:41AM +0300, Andy Shevchenko wrote: > On Wed, Sep 27, 2023 at 04:01:58PM +0200, Linus Walleij wrote: > > On Mon, Sep 25, 2023 at 9:30 AM Andy Shevchenko wrote: ... > > > > + if (pxa_ohci->usb_host) > > > > + gpiod_put(pxa_ohci->usb_host); > > > > > > Linus, Bart, do we have misdesigned _optinal() GPIO APIs? > > > > > > In GPIOLIB=n, the above requires that redundant check. Shouldn't we replace > > > gpiod_put() stub to be simply no-op? > > > > You mean the WARN_ON(desc) in gpiod_put() in the static inline > > stub version? > > > > I thought about it for a bit, drafted a patch removing them, and then > > realized the following: > > > > If someone is making the gpiolib optional for a driver, i.e. neither > > DEPENDS ON GPIOLIB nor SELECT GPIOLIB, they are a quite > > narrow segment. I would say in 9 cases out of 10 or more this is > > just a driver that should depend on or select GPIOLIB. > > > > I think such drivers should actually do the NULL checks and not be > > too convenient, the reason is readability: someone reading that > > driver will be thinking gpios are not optional if they can call > > gpiod_set_value(), gpiod_put() etc without any sign that the > > desc is optional. > > > > If the driver uses [devm_]gpiod_get_optional() the library is not > > using the stubs and does the right thing, and it is clear that > > the GPIO is *runtime* optional. > > > > But *compile time* optional, *combined* with runtime optional - > > I'm not so happy if we try to avoid warnings around that. I think > > it leads to confusing configs and code that looks like gpiolib is > > around despite it wasn't selected. > > > > If the code isn't depending on or selecting GPIOLIB and still > > use _optional() calls, it better be ready to do some extra checks, > > because this is a weird combo, it can't be common. > > > > Could be a documentation update making this clear though. > > > > What do you other people think? > > The problem here indeed if the code is not selecting or being dependent on > GPIOLIB and uses _optional() calls. > > I agree that this is quite a niche that should be addressed on the driver side. One more thing, though. I think those warnings are incomplete or actually reversed, and we outta use WARN_ON(IS_ERR(desc)), no? This way it will fix my concerns and your concerns will be satisfied, right? So, if gpiod_get() returns an error pointer and then we are trying to free it with GPIOLIB=n, _then_ we will got a warning and it's obvious that driver has to be prepared for that, otherwise if we have it NULL and call for gpiod_get_optional(), even with GPIOLIB=n, it's fine to free, we don't care. -- With Best Regards, Andy Shevchenko