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 E00D7ECAAA1 for ; Sat, 17 Sep 2022 16:27:09 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229452AbiIQQ1J (ORCPT ); Sat, 17 Sep 2022 12:27:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:53002 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229450AbiIQQ1I (ORCPT ); Sat, 17 Sep 2022 12:27:08 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 47CFA2DA83 for ; Sat, 17 Sep 2022 09:27:07 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id D4E2EB80CB0 for ; Sat, 17 Sep 2022 16:27:05 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8EE5CC433C1; Sat, 17 Sep 2022 16:27:04 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1663432024; bh=sRIEE4FwU5J55QsQ/ik3Xi1XJ7TkddR7kXzDtvs2uUw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=qItp7BHQOfd9Bct+vUGrPwgk7YjYqrwnhUvyE4UHVp868hRUcen7K/2NUYRBzcRVr aAvJ+Kex55KhB/SqQagUvU+QZhfSWzaoAFsoSr2A7mMvXTKBIK/fjVUPTZd6u0XbLW X8oOmEn/yENxjDivTR3TOi996Tyu+gKgQhOaaA4RgrMYNT1tX8zutJmX0aCeLVZGLh K4rE7z4aLmZICnOBrUmLeXBZmiF5YMEBWUtgoUNLVrxIg4b+2vu5Avs2o5gCvd5Bg7 0eG3IWEloCyV8dhmwXHc0oi/aIUXlla9aX9Ud1zMWTKMsU5VWCCl9A3dfgrPvTbbj4 NJlKgDiO778uw== Received: from 185-176-101-241.host.sccbroadband.ie ([185.176.101.241] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1oZaek-00AnuN-9P; Sat, 17 Sep 2022 17:27:02 +0100 Date: Sat, 17 Sep 2022 17:27:01 +0100 Message-ID: <87o7vekluy.wl-maz@kernel.org> From: Marc Zyngier To: Linus Walleij Cc: linux-gpio@vger.kernel.org Subject: Re: [PATCH] pinctrl: nomadik: Make gpio irqchip immutable In-Reply-To: <20220917131907.86899-1-linus.walleij@linaro.org> References: <20220917131907.86899-1-linus.walleij@linaro.org> 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.176.101.241 X-SA-Exim-Rcpt-To: linus.walleij@linaro.org, linux-gpio@vger.kernel.org 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 Sat, 17 Sep 2022 14:19:07 +0100, Linus Walleij wrote: > > This makes the Nomadik GPIO irqchip immutable. > > Tested on the Samsung Galaxy SIII mini GT-I8190. > > Cc: Marc Zyngier > Signed-off-by: Linus Walleij > --- > drivers/pinctrl/nomadik/pinctrl-nomadik.c | 51 +++++++++++------------ > 1 file changed, 24 insertions(+), 27 deletions(-) > > diff --git a/drivers/pinctrl/nomadik/pinctrl-nomadik.c b/drivers/pinctrl/nomadik/pinctrl-nomadik.c > index f5014d09d81a..1ee3b45dd6bc 100644 > --- a/drivers/pinctrl/nomadik/pinctrl-nomadik.c > +++ b/drivers/pinctrl/nomadik/pinctrl-nomadik.c > @@ -244,7 +244,6 @@ enum nmk_gpio_slpm { > > struct nmk_gpio_chip { > struct gpio_chip chip; > - struct irq_chip irqchip; > void __iomem *addr; > struct clk *clk; > unsigned int bank; > @@ -675,15 +674,11 @@ static void __nmk_gpio_set_wake(struct nmk_gpio_chip *nmk_chip, > __nmk_gpio_irq_modify(nmk_chip, offset, WAKE, on); > } > > -static int nmk_gpio_irq_maskunmask(struct irq_data *d, bool enable) > +static void nmk_gpio_irq_maskunmask(struct nmk_gpio_chip *nmk_chip, > + struct irq_data *d, bool enable) > { > - struct nmk_gpio_chip *nmk_chip; > unsigned long flags; > > - nmk_chip = irq_data_get_irq_chip_data(d); > - if (!nmk_chip) > - return -EINVAL; > - > clk_enable(nmk_chip->clk); > spin_lock_irqsave(&nmk_gpio_slpm_lock, flags); > spin_lock(&nmk_chip->lock); > @@ -696,18 +691,22 @@ static int nmk_gpio_irq_maskunmask(struct irq_data *d, bool enable) > spin_unlock(&nmk_chip->lock); > spin_unlock_irqrestore(&nmk_gpio_slpm_lock, flags); > clk_disable(nmk_chip->clk); > - > - return 0; > } > > static void nmk_gpio_irq_mask(struct irq_data *d) > { > - nmk_gpio_irq_maskunmask(d, false); > + struct nmk_gpio_chip *nmk_chip = irq_data_get_irq_chip_data(d); As far as I can tell, the per-chip data for a GPIO chip points to the gpio_chip, and not the driver-specific view that contains the generic structure. Here you're lucky that they are the same address as the gpio_chip is the first one. This is the same bug as the one fixed by d1e972ace423 ("gpio: tegra186: Fix chip_data type confusion"). > + > + nmk_gpio_irq_maskunmask(nmk_chip, d, false); > + gpiochip_disable_irq(&nmk_chip->chip, irqd_to_hwirq(d)); > } > > static void nmk_gpio_irq_unmask(struct irq_data *d) > { > - nmk_gpio_irq_maskunmask(d, true); > + struct nmk_gpio_chip *nmk_chip = irq_data_get_irq_chip_data(d); > + > + gpiochip_enable_irq(&nmk_chip->chip, irqd_to_hwirq(d)); > + nmk_gpio_irq_maskunmask(nmk_chip, d, true); > } > > static int nmk_gpio_irq_set_wake(struct irq_data *d, unsigned int on) > @@ -1078,13 +1077,25 @@ static struct nmk_gpio_chip *nmk_gpio_populate_chip(struct device_node *np, > return nmk_chip; > } > > +static const struct irq_chip nmk_irq_chip = { > + .name = "nomadik-gpio", Maybe not, see below. > + .irq_ack = nmk_gpio_irq_ack, > + .irq_mask = nmk_gpio_irq_mask, > + .irq_unmask = nmk_gpio_irq_unmask, > + .irq_set_type = nmk_gpio_irq_set_type, > + .irq_set_wake = nmk_gpio_irq_set_wake, > + .irq_startup = nmk_gpio_irq_startup, > + .irq_shutdown = nmk_gpio_irq_shutdown, > + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_IMMUTABLE, > + GPIOCHIP_IRQ_RESOURCE_HELPERS, > +}; > + > static int nmk_gpio_probe(struct platform_device *dev) > { > struct device_node *np = dev->dev.of_node; > struct nmk_gpio_chip *nmk_chip; > struct gpio_chip *chip; > struct gpio_irq_chip *girq; > - struct irq_chip *irqchip; > bool supports_sleepmode; > int irq; > int ret; > @@ -1125,22 +1136,8 @@ static int nmk_gpio_probe(struct platform_device *dev) > chip->can_sleep = false; > chip->owner = THIS_MODULE; > > - irqchip = &nmk_chip->irqchip; > - irqchip->irq_ack = nmk_gpio_irq_ack; > - irqchip->irq_mask = nmk_gpio_irq_mask; > - irqchip->irq_unmask = nmk_gpio_irq_unmask; > - irqchip->irq_set_type = nmk_gpio_irq_set_type; > - irqchip->irq_set_wake = nmk_gpio_irq_set_wake; > - irqchip->irq_startup = nmk_gpio_irq_startup; > - irqchip->irq_shutdown = nmk_gpio_irq_shutdown; > - irqchip->flags = IRQCHIP_MASK_ON_SUSPEND; > - irqchip->name = kasprintf(GFP_KERNEL, "nmk%u-%u-%u", > - dev->id, > - chip->base, > - chip->base + chip->ngpio - 1); > - Dropping this is unfortunately a userspace visible change. I'm 99% sure nobody will ever care, but we've been burned by the remaining 1%, and I'd rather you keep it using the irq_print_chip() callback with something like: static void nmk_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p) { struct gpio_chip *chip = irq_data_get_irq_chip_data(d); struct nmk_gpio_chip *nmk_chip = container_of(chip, struct nmk_gpio_chip, chip); seq_printf(p, "nmk%u-%u-%u", nmk_chip->bank, chip->base, chip->base + chip->ngpio - 1); } Thanks, M. -- Without deviation from the norm, progress is not possible.