From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brian Norris Subject: Re: [RFC PATCH] pinctrl: rockchip: add irq_shutdown Date: Thu, 29 Jun 2017 15:05:33 -0700 Message-ID: <20170629220532.GA54678@google.com> References: <1498223019-9123-1-git-send-email-jeffy.chen@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1498223019-9123-1-git-send-email-jeffy.chen@rock-chips.com> Sender: linux-kernel-owner@vger.kernel.org To: Jeffy Chen Cc: linux-kernel@vger.kernel.org, tfiga@chromium.org, heiko@sntech.de, dianders@chromium.org, linux-rockchip@lists.infradead.org, Linus Walleij , linux-gpio@vger.kernel.org, linux-arm-kernel@lists.infradead.org List-Id: linux-gpio@vger.kernel.org On Fri, Jun 23, 2017 at 09:03:39PM +0800, Jeffy Chen wrote: > Currently the rockchip pinctrl driver would try to enable/disable the > gpio bank clk when enable/disable an irq. > > So when the irq core trying to shutdown an already disabled irq, it > would result in unbalanced clk disable request: > [ 35.911955] WARNING: at drivers/clk/clk.c:680 > ... > [ 37.272271] Call trace: > [ 37.274729] [] clk_core_disable+0x28/0x194 > [ 37.280395] [] clk_disable+0x34/0x48 > [ 37.285544] [] rockchip_irq_disable+0x30/0x3c > [ 37.291472] [] __irq_disable+0x40/0x64 > [ 37.296791] [] irq_shutdown+0x68/0x8c > [ 37.302023] [] __free_irq+0x110/0x218 > [ 37.307254] [] free_irq+0x54/0x64 > [ 37.312138] [] devm_irq_release+0x24/0x30 > > Add an irq_shutdown callback, and do a sanity check for irq state to > prevent that. IMO, this patch is completely the wrong approach. Either we: (a) follow the current semantics of the irqchip core handling (which tglx described more or less as "no refcounting; no guarantee that enable/disable are balanced") or (b) fix the irqchip core to provide the above guarantee To do (a) properly would be rather trivial too; just keep an extra bitmask in the bank struct to track the "enabled" state of each interrupt. If the mask is non-zero, enable the clock; if zero, disable. (Incidentally, this is pretty similar logic to what you ended up with on your SPI runtime PM / set_cs() patches recently.) This patch does neither (a) nor (b), and so I'd tend to reject it. Brian > Signed-off-by: Jeffy Chen > --- > > drivers/pinctrl/pinctrl-rockchip.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c > index 607f52c..b0e3130 100644 > --- a/drivers/pinctrl/pinctrl-rockchip.c > +++ b/drivers/pinctrl/pinctrl-rockchip.c > @@ -2551,6 +2551,12 @@ static void rockchip_irq_disable(struct irq_data *d) > clk_disable(bank->clk); > } > > +static void rockchip_irq_shutdown(struct irq_data *d) > +{ > + if (!irqd_irq_disabled(d)) > + rockchip_irq_disable(d); > +} > + > static void rockchip_irq_bus_lock(struct irq_data *d) > { > struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d); > @@ -2641,6 +2647,7 @@ static int rockchip_interrupts_register(struct platform_device *pdev, > gc->chip_types[0].chip.irq_unmask = irq_gc_mask_clr_bit; > gc->chip_types[0].chip.irq_enable = rockchip_irq_enable; > gc->chip_types[0].chip.irq_disable = rockchip_irq_disable; > + gc->chip_types[0].chip.irq_shutdown = rockchip_irq_shutdown; > gc->chip_types[0].chip.irq_set_wake = irq_gc_set_wake; > gc->chip_types[0].chip.irq_suspend = rockchip_irq_suspend; > gc->chip_types[0].chip.irq_resume = rockchip_irq_resume; > -- > 2.1.4 > >