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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id ABC51C433EF for ; Mon, 25 Oct 2021 10:28:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 8538E60E52 for ; Mon, 25 Oct 2021 10:28:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230267AbhJYKag (ORCPT ); Mon, 25 Oct 2021 06:30:36 -0400 Received: from mail.kernel.org ([198.145.29.99]:55432 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230231AbhJYKag (ORCPT ); Mon, 25 Oct 2021 06:30:36 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id E036760E8C; Mon, 25 Oct 2021 10:28:13 +0000 (UTC) Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mexD9-001KvG-HK; Mon, 25 Oct 2021 11:28:11 +0100 Date: Mon, 25 Oct 2021 11:28:11 +0100 Message-ID: <87fsspbdqc.wl-maz@kernel.org> From: Marc Zyngier To: Joey Gouly Cc: , Linus Walleij , Hector Martin , Alyssa Rosenzweig , Sven Peter , , Rob Herring , Mark Kettenis , Subject: Re: [PATCH v4 0/5] pinctrl/GPIO driver for Apple SoCs In-Reply-To: <20211024101838.43107-1-joey.gouly@arm.com> References: <20211024101838.43107-1-joey.gouly@arm.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: joey.gouly@arm.com, linux-gpio@vger.kernel.org, linus.walleij@linaro.org, marcan@marcan.st, alyssa.rosenzweig@collabora.com, sven@svenpeter.dev, devicetree@vger.kernel.org, robh+dt@kernel.org, kettenis@openbsd.org, nd@arm.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-gpio@vger.kernel.org On Sun, 24 Oct 2021 11:18:33 +0100, Joey Gouly wrote: > > Hi all, > > Here is the v4 patchset for the Apple pinctrl/GPIO driver. > > Changes since v3 [1]: > - Applied Marc Zyngier's review/patch (with exception noted below) > - Removed "apple,t8103-pinctrl" compatible from driver > - Applied Acks/Review tags > > > With Marc's changes, the irq_chip was being shared between pinctrl > drivers and I was getting the following warning: > > drivers/gpio/gpiolib.c:1456: > > detected irqchip that is shared with multiple gpiochips: please fix > the driver. > > > So I applied the following diff to Marc's change, to not share the > irq_chips, while still being cleaner overall: > > diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c > b/drivers/pinctrl/pinctrl-apple-gpio.c > index 732c347a2bbc..ce037a5c15c1 100644 > --- a/drivers/pinctrl/pinctrl-apple-gpio.c > +++ b/drivers/pinctrl/pinctrl-apple-gpio.c > @@ -35,6 +35,7 @@ struct apple_gpio_pinctrl { > > struct pinctrl_desc pinctrl_desc; > struct gpio_chip gpio_chip; > + struct irq_chip irq_chip; > u8 irqgrps[0]; > }; > > @@ -369,6 +370,8 @@ static int apple_gpio_gpio_register(struct > apple_gpio_pinctrl *pctl) > return dev_err_probe(pctl->dev, -ENODEV, > "No gpio-controller property\n"); > > + pctl->irq_chip = apple_gpio_irqchip; > + > pctl->gpio_chip.label = dev_name(pctl->dev); > pctl->gpio_chip.request = gpiochip_generic_request; > pctl->gpio_chip.free = gpiochip_generic_free; > @@ -385,7 +388,7 @@ static int apple_gpio_gpio_register(struct > apple_gpio_pinctrl *pctl) > if (girq->num_parents) { > int i; > > - girq->chip = &apple_gpio_irqchip; > + girq->chip = &pctl->irq_chip; > girq->parent_handler = apple_gpio_gpio_irq_handler; > > girq->parents = kmalloc_array(girq->num_parents, > > Marc said that hierarchical IRQ domains should solve this problem, but > I'll let him explain more on the list, maybe that should solved in a different > patch series. The issue I have with the gpiolib code is that it hijacks function pointers from a structure that is not under its control, and that is exactly what the hierarchical IRQ domains/irqchips were supposed to prevent. It isn't obvious to me why this cannot be fixed with a gpiolib domain and irqchip stacked on top of the one exposed by the low-level driver, providing the required hooks in a standard way. Yes, this is even more indirection. It also isn't clear why gpiochip_set_irq_hooks() shouts: if the hooks are already there, move on. Ultimately, this sort of manipulation is what prevents the irq_chip structure from being made 'const' everywhere (ok, there is another nit because of the parent_device field, which I'm looking at getting rid of). Keeping writable function pointers isn't great, overall. Now, given that this is an issue that isn't directly related to the driver at hand, it shouldn't be a blocker for merging it. So for the driver itself: Reviewed-by: Marc Zyngier M. -- Without deviation from the norm, progress is not possible.