Received: by 2002:a05:6a10:af89:0:0:0:0 with SMTP id iu9csp723535pxb; Fri, 14 Jan 2022 15:03:20 -0800 (PST) X-Google-Smtp-Source: ABdhPJzSsE32YIjm1NkpkQTTEGF434hQRrt6osj9O1qlz6i0QVrgleIV29Rd1ikLdpHKlQfhXuZJ X-Received: by 2002:a17:902:7617:b0:149:9c02:f260 with SMTP id k23-20020a170902761700b001499c02f260mr11505289pll.30.1642201400465; Fri, 14 Jan 2022 15:03:20 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1642201400; cv=none; d=google.com; s=arc-20160816; b=UDl1qE9v9VIGEWfSSsxGjDh+8w8zOLoHORws4rp2jH7Q7rlv6OupjITTbzr05qD1cP rqqn172/kEAbNzx14RR4CNyaIQ4lnTv7odjiOGsjlYshBYaxjwZrLosWw8dj36pFUm0G wJJ2naBq6hmwe+u0BPVM+NOt0BSp45Dh4NIYaalWl+1sHbgENGGiFxnu+YWMbdBb1MtX cU9pVtIBUxlBtAC4EQmG3FD1Z3ug5BZ86TlpH1qnoG84e+0jYFu2ciEuZB4LV1N8Nk4N 7VUjZl9GpYphZV6oc5u1AVZm4AjGk/y4eT1ViQ6D3lBDXsup0irG/dEztvQrn0sDkQjf 3aXw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:organization :references:cc:to:subject:from:dkim-filter; bh=X/o9mIv6XfWXGgBrYjMBRUDMMcnNTpXgBh+nmx0RnQQ=; b=00FGWab8s6tT597P77z1YbOhPpyIHJw/iGPNUqNJ6XJcu9hQCOR80NLcdYHbBW3/+E 41TD7DRs1LMFpcgfz96V109hNyTpJent+Oojwk9rNuMCgQoSswEjhs2GLrx9rrvQVCfh jR17hs4YSqhn7WD/ZbC6tXe17ts2CKC+ho8xIyUhcrw904zIbLA8Xhx11hkQDpryHnYU DZjbJWjAA1MWqhVoiajdmc8+ELbUJ/JiCVALlH4MPs6ku4Wv9N1RKIGcbtk+tL780lbb YrU+fi3Vgr7E6H7sdv4AZnqvfwmI0dt/tLsb37N/mrLHIV+q4ASq9MAyr9Z6170v9WAs oV+A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id co2si6519818pjb.117.2022.01.14.15.03.08; Fri, 14 Jan 2022 15:03:20 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S244105AbiANTOX (ORCPT + 99 others); Fri, 14 Jan 2022 14:14:23 -0500 Received: from mxout01.lancloud.ru ([45.84.86.81]:38262 "EHLO mxout01.lancloud.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S244089AbiANTOU (ORCPT ); Fri, 14 Jan 2022 14:14:20 -0500 Received: from LanCloud DKIM-Filter: OpenDKIM Filter v2.11.0 mxout01.lancloud.ru 96891205E49F Received: from LanCloud Received: from LanCloud Received: from LanCloud From: Sergey Shtylyov Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional To: =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= CC: Mark Brown , Andrew Lunn , Ulf Hansson , Vignesh Raghavendra , KVM list , "Rafael J. Wysocki" , , Linus Walleij , "Amit Kucheria" , ALSA Development Mailing List , Jaroslav Kysela , "Guenter Roeck" , Thierry Reding , "MTD Maling List" , Linux I2C , Miquel Raynal , , Jiri Slaby , "Linux Kernel Mailing List" , Khuong Dinh , Florian Fainelli , Matthias Schiffer , Kamal Dasu , Greg Kroah-Hartman , "Lee Jones" , Bartosz Golaszewski , "Daniel Lezcano" , Kishon Vijay Abraham I , Geert Uytterhoeven , "open list:SERIAL DRIVERS" , bcm-kernel-feedback-list , Zhang Rui , , Linux PWM List , Robert Richter , "Saravanan Sekar" , Corey Minyard , Linux PM list , Liam Girdwood , "Mauro Carvalho Chehab" , John Garry , Takashi Iwai , Peter Korsgaard , "William Breathitt Gray" , Mark Gross , Hans de Goede , Alex Williamson , Borislav Petkov , Eric Auger , Jakub Kicinski , Matthias Brugger , , "Andy Shevchenko" , Benson Leung , Pengutronix Kernel Team , "Linux ARM" , , Tony Luck , Richard Weinberger , "Mun Yew Tham" , "open list:GPIO SUBSYSTEM" , , Yoshihiro Shimoda , Cornelia Huck , "Linux MMC List" , Joakim Zhang , linux-spi , Linux-Renesas , Vinod Koul , "James Morse" , Zha Qipeng , "Sebastian Reichel" , =?UTF-8?Q?Niklas_S=c3=b6derlund?= , , "Brian Norris" , "David S. Miller" References: <20220110195449.12448-1-s.shtylyov@omp.ru> <20220110195449.12448-2-s.shtylyov@omp.ru> <20220110201014.mtajyrfcfznfhyqm@pengutronix.de> <20220112085009.dbasceh3obfok5dc@pengutronix.de> <20220112213121.5ruae5mxwj6t3qiy@pengutronix.de> <29f0c65d-77f2-e5b2-f6cc-422add8a707d@omp.ru> <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> Organization: Open Mobile Platform Message-ID: <61b80939-357d-14f5-df99-b8d102a4e1a1@omp.ru> Date: Fri, 14 Jan 2022 22:14:10 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> Content-Type: text/plain; charset="windows-1252" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [192.168.11.198] X-ClientProxiedBy: LFEXT01.lancloud.ru (fd00:f066::141) To LFEX1907.lancloud.ru (fd00:f066::207) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 1/14/22 12:25 PM, Uwe Kleine-K?nig wrote: >>>>> To me it sounds much more logical for the driver to check if an >>>>> optional irq is non-zero (available) or zero (not available), than to >>>>> sprinkle around checks for -ENXIO. In addition, you have to remember >>>>> that this one returns -ENXIO, while other APIs use -ENOENT or -ENOSYS >>>>> (or some other error code) to indicate absence. I thought not having >>>>> to care about the actual error code was the main reason behind the >>>>> introduction of the *_optional() APIs. >>> >>>> No, the main benefit of gpiod_get_optional() (and clk_get_optional()) is >>>> that you can handle an absent GPIO (or clk) as if it were available. >> >> Hm, I've just looked at these and must note that they match 1:1 with >> platform_get_irq_optional(). Unfortunately, we can't however behave the >> same way in request_irq() -- because it has to support IRQ0 for the sake >> of i8253 drivers in arch/... > > Let me reformulate your statement to the IMHO equivalent: > > If you set aside the differences between > platform_get_irq_optional() and gpiod_get_optional(), Sorry, I should make it clear this is actually the diff between a would-be platform_get_irq_optional() after my patch, not the current code... > platform_get_irq_optional() is like gpiod_get_optional(). > > The introduction of gpiod_get_optional() made it possible to simplify > the following code: > > reset_gpio = gpiod_get(...) > if IS_ERR(reset_gpio): > error = PTR_ERR(reset_gpio) > if error != -ENDEV: ENODEV? > return error > else: > gpiod_set_direction(reset_gpiod, INACTIVE) > > to > > reset_gpio = gpiod_get_optional(....) > if IS_ERR(reset_gpio): > return reset_gpio > gpiod_set_direction(reset_gpiod, INACTIVE) > > and I never need to actually know if the reset_gpio actually exists. > Either the line is put into its inactive state, or it doesn't exist and > then gpiod_set_direction is a noop. For a regulator or a clk this works > in a similar way. > > However for an interupt this cannot work. You will always have to check > if the irq is actually there or not because if it's not you cannot just > ignore that. So there is no benefit of an optional irq. > > Leaving error message reporting aside, the introduction of > platform_get_irq_optional() allows to change > > irq = platform_get_irq(...); > if (irq < 0 && irq != -ENXIO) { > return irq; > } else if (irq >= 0) { Rather (irq > 0) actually, IRQ0 is considered invalid (but still returned). > ... setup irq operation ... > } else { /* irq == -ENXIO */ > ... setup polling ... > } > > to > > irq = platform_get_irq_optional(...); > if (irq < 0 && irq != -ENXIO) { > return irq; > } else if (irq >= 0) { > ... setup irq operation ... > } else { /* irq == -ENXIO */ > ... setup polling ... > } > > which isn't a win. When changing the return value as you suggest, it can > be changed instead to: > > irq = platform_get_irq_optional(...); > if (irq < 0) { > return irq; > } else if (irq > 0) { > ... setup irq operation ... > } else { /* irq == 0 */ > ... setup polling ... > } > > which is a tad nicer. If that is your goal however I ask you to also > change the semantic of platform_get_irq() to return 0 on "not found". Well, I'm not totally opposed to that... but would there be a considerable win? Anyway, we should 1st stop returning 0 for "valid" IRQs -- this is done by my patch the discussed patch series are atop of. > Note the win is considerably less compared to gpiod_get_optional vs If there's any at all... We'd basically have to touch /all/ platform_get_irq() calls (and get an even larger CC list ;-)). > gpiod_get however. And then it still lacks the semantic of a dummy irq > which IMHO forfeits the right to call it ..._optional(). Not quite grasping it... Why e.g. clk_get() doesn't return 0 for a not found clock? > Now I'm unwilling to continue the discussion unless there pops up a > suggestion that results in a considerable part (say > 10%) of the > drivers using platform_get_irq_optional not having to check if the > return value corresponds to "not found". Note that many actual drivers don't follow the pattern prescribed in the comment to platform_get_irq_optional() and their check for an optional IRQ look like irq < 0 (and, after my patches, irq <= 0). Maybe we shouldn't even bother returning the error codes and just return 0 for all kinds of misfortunes instead? :-) > Best regards > Uwe MBR, Sergey