Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755820AbdC1NFd (ORCPT ); Tue, 28 Mar 2017 09:05:33 -0400 Received: from mail-it0-f47.google.com ([209.85.214.47]:36634 "EHLO mail-it0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755737AbdC1NFa (ORCPT ); Tue, 28 Mar 2017 09:05:30 -0400 MIME-Version: 1.0 In-Reply-To: <87vaqtadry.fsf@free-electrons.com> References: <0e60fccd7913b83ee53d2921ce8f297927e8b6f3.1490120798.git-series.gregory.clement@free-electrons.com> <87vaqtadry.fsf@free-electrons.com> From: Linus Walleij Date: Tue, 28 Mar 2017 15:04:39 +0200 Message-ID: Subject: Re: [PATCH v2 5/7] pinctrl: aramda-37xx: Add irqchip support To: Gregory CLEMENT Cc: "linux-gpio@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , Rob Herring , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Nadav Haklai , Victor Gu , Marcin Wojtas , Wilson Ding , Hua Jing , Neta Zur Hershkovits Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3664 Lines: 94 On Tue, Mar 28, 2017 at 12:36 PM, Gregory CLEMENT wrote: > On lun., mars 27 2017, Linus Walleij wrote: >>> + u32 virq = irq_linear_revmap(d, hwirq + >>> + i * GPIO_PER_REG); >> >> Use irq_find_mapping() instead please. > > As we are in the interrupt handler I chose to use this function because > according to its documentation: "This is a fast path alternative to > irq_find_mapping() that can be called directly by irq controller code to > save a handful of instructions". > > The only restriction is "It is always safe to call, but won't find irqs > mapped using the radix tree.". So I think that for this driver it is > okay. So you are relying on the core code in gpiolib to select a linear map. That is implicit semantics, and either all drivers using GPIOLIB_IRQCHIP should be changed to irq_linear_revmap() or all stay with irq_find_mapping(). In this case, I doubt it that you are actually so timing critical that it matters. Please use irq_find_mapping(). >>> + nr_irq_parent = of_irq_count(np); >>> + spin_lock_init(&info->irq_lock); >>> + >>> + if (!nr_irq_parent) { >>> + dev_err(&pdev->dev, "Invalid or no IRQ\n"); >>> + return 0; >>> + } >> >> What if it is > 1? That doesn't seem to work but will pass this >> check silently. > > If we have nr_irq_parent > 1, it will work and it is actually expected. Ah, I get it. Nice. >>> + ret = gpiochip_irqchip_add(gc, irqchip, 0, >>> + handle_level_irq, IRQ_TYPE_NONE); >> >> If you also set up the handler in .set_type() you can assign >> handle_bad_irq() here and let .set_type set the right handler >> as e.g. drivers/gpio/gpio-pl061.c. > > Well the hardware can only manage the edge trigger, so there is no > benefit to modify it each time we want to change the kind of edge we > want (raising or falling). But your comment make me realized that I used > the wrong one, I will move to handle_edge_irq in the v4. Ooops, yeah handle_edge_irq() is what calls the ACK callback. >>> + for (i = 0; i < nrirqs; i++) { >>> + struct irq_data *d = irq_get_irq_data(gc->irq_base + i); >>> + >>> + d->mask = 1 << (i % GPIO_PER_REG); >>> + } >> >> What is this? It looks like a big hack. At least put in a fat >> comment about what is going on and why. > > I can reuse a part of the commit log here: "The only unusual "feature" > is that many interrupts are connected to the parent interrupt > controller. But we do not take advantage of this and use the chained irq > with all of them." What you're doing is mocking around with core irqchip semantics. Is ->mask really supposed to be played around with from the outsid like this? Anyways: BIT(i % GPIO_PER_REG) is nicer. >>> + for (i = 0; i < nr_irq_parent; i++) { >>> + int irq = irq_of_parse_and_map(np, i); >> >> I think gpiochip_irqchip_add() will do this for you already, >> as it calls irq_create_mapping() for all offsets which will call >> irq_of_parse_and_map() am I right? > > After reading the code, it doesn't seem it is the case. At least there > is no irq_of_parse_and_map() call from gpiochip_irqchip_add(). And waht > we need here is to associate each IRQ to the same GPIO handler. > > I can still try without this line to confirm it. It has irq_create_mapping(gpiochip->irqdomain, offset); that get called for every IRQ, and that will eventually call irq_of_parse_and_map() if the IRQs are defined in the device tree. (IIRC) Yours, Linus Walleij