[RFC PATCH] pinctrl: rockchip: add irq_shutdown
From: briannorris@chromium.org (Brian Norris)
Date: 2017-06-29 22:05:38
Also in:
linux-gpio, linux-rockchip, lkml
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] [<ffffffc0007ac270>] clk_core_disable+0x28/0x194 [ 37.280395] [<ffffffc0007ac6a8>] clk_disable+0x34/0x48 [ 37.285544] [<ffffffc0004f3bf0>] rockchip_irq_disable+0x30/0x3c [ 37.291472] [<ffffffc00027a7e0>] __irq_disable+0x40/0x64 [ 37.296791] [<ffffffc00027a86c>] irq_shutdown+0x68/0x8c [ 37.302023] [<ffffffc0002777fc>] __free_irq+0x110/0x218 [ 37.307254] [<ffffffc0002779a8>] free_irq+0x54/0x64 [ 37.312138] [<ffffffc00027ba24>] 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
quoted hunk ↗ jump to hunk
Signed-off-by: Jeffy Chen <redacted> --- 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