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 012A9C433F5 for ; Fri, 14 Jan 2022 09:39:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239817AbiANJjY convert rfc822-to-8bit (ORCPT ); Fri, 14 Jan 2022 04:39:24 -0500 Received: from mail-vk1-f182.google.com ([209.85.221.182]:39619 "EHLO mail-vk1-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229785AbiANJjX (ORCPT ); Fri, 14 Jan 2022 04:39:23 -0500 Received: by mail-vk1-f182.google.com with SMTP id n14so4303363vkk.6; Fri, 14 Jan 2022 01:39:22 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=XIsXuJfmE8JES6Mbtb18c2n3rWZVUZhPWhasZQeRi6Q=; b=B6fVIblE2YvZVeCArGcMUv0EJNHL+C2rkW//YimybgZ4VFOF6qnKGPJBYjiX+q9Dq5 nJVlZ2LbWqXpD7YGOyGSeudPe2r8bTHf3AuHH/E/JTEQch+crQsQwq7Xesx6PJuRoHVG vZLoGUjBK4/tgccEHiWg5NGcjFeo/ZYdd/pZ8EadRA5wmgmCgLLaxqlpFck3zhsk9R3g s5LGB2neA5o/2//SrAax7o4hmZkvRNgswZ5FI80jKNxgcY0VxYt74iZSKZnFkopMg/Ad PWLhMOBCniba9fgx5U5sdkjls0O8hAWjVUS70uDszzCGQvcy7c6UaGPWrEX+dVtkV8jd iOSg== X-Gm-Message-State: AOAM532dOoWuvtPRgS471+cj2DoVR7o0lPqEF04Zasu/K8Gtvx/nMOFM FIhIPsgx3PVt64ddBzE7FJ0b535mFnUbhej7 X-Google-Smtp-Source: ABdhPJyk3O8GzDxpL3/PrxHmPJiIYnb05POlLBdMiX4/E5+LHj71bMHAwA4rtBlCUD6Pjl/GZG0tmg== X-Received: by 2002:a05:6122:208a:: with SMTP id i10mr366113vkd.16.1642153161863; Fri, 14 Jan 2022 01:39:21 -0800 (PST) Received: from mail-ua1-f41.google.com (mail-ua1-f41.google.com. [209.85.222.41]) by smtp.gmail.com with ESMTPSA id x128sm2072722vkx.14.2022.01.14.01.39.19 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 14 Jan 2022 01:39:19 -0800 (PST) Received: by mail-ua1-f41.google.com with SMTP id i5so15890425uaq.10; Fri, 14 Jan 2022 01:39:19 -0800 (PST) X-Received: by 2002:a05:6102:3581:: with SMTP id h1mr3716211vsu.5.1642153159149; Fri, 14 Jan 2022 01:39:19 -0800 (PST) MIME-Version: 1.0 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> In-Reply-To: <20220114092557.jrkfx7ihg26ekzci@pengutronix.de> From: Geert Uytterhoeven Date: Fri, 14 Jan 2022 10:39:07 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional To: =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= Cc: Sergey Shtylyov , Mark Brown , Andrew Lunn , Ulf Hansson , Vignesh Raghavendra , KVM list , "Rafael J. Wysocki" , linux-iio@vger.kernel.org, Linus Walleij , Amit Kucheria , ALSA Development Mailing List , Jaroslav Kysela , Guenter Roeck , Thierry Reding , MTD Maling List , Linux I2C , Miquel Raynal , linux-phy@lists.infradead.org, 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 , "open list:SERIAL DRIVERS" , bcm-kernel-feedback-list , Zhang Rui , platform-driver-x86@vger.kernel.org, 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 , openipmi-developer@lists.sourceforge.net, Andy Shevchenko , Benson Leung , Pengutronix Kernel Team , Linux ARM , linux-edac@vger.kernel.org, Tony Luck , Richard Weinberger , Mun Yew Tham , "open list:GPIO SUBSYSTEM" , netdev@vger.kernel.org, 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?= , linux-mediatek@lists.infradead.org, Brian Norris , "David S. Miller" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org Hi Uwe, On Fri, Jan 14, 2022 at 10:26 AM Uwe Kleine-König wrote: > On Thu, Jan 13, 2022 at 11:35:34PM +0300, Sergey Shtylyov wrote: > > On 1/13/22 12:45 AM, Mark Brown 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(), > 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: > 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) { > ... 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". Please don't make that change. If platform_get_irq() would return 0 on "not found", all existing users have to be changed to: irq = platform_get_irq(...); if (irq < 0) { return irq; } else if (!irq) { return -ENOENT; } else { ... setup irq operation ... } If the IRQ is not optional, there should be an error code when it is not present. This keeps error handling simple. The _optional() difference lies in the zero/NULL vs. error code in case of not present. > Note the win is considerably less compared to gpiod_get_optional vs > gpiod_get however. And then it still lacks the semantic of a dummy irq > which IMHO forfeits the right to call it ..._optional(). > > 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". Usually drivers do have to check if the interrupt was present or not, because they may have to change the operation of the driver, depending on interrupt-based or timer/polling-based processing. Clocks, regulators, and resets are different, as their absence is really a no-op. The absence of an interrupt is not a no-op (except for the separate interrupts vs. a single muxed interrupt case). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds