From patchwork Wed Apr 17 00:41:02 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Javier Martinez Canillas X-Patchwork-Id: 2451691 Return-Path: X-Original-To: patchwork-linux-omap@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 68A2DDFF66 for ; Wed, 17 Apr 2013 00:41:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755221Ab3DQAlZ (ORCPT ); Tue, 16 Apr 2013 20:41:25 -0400 Received: from mail-ie0-f182.google.com ([209.85.223.182]:46515 "EHLO mail-ie0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755034Ab3DQAlW (ORCPT ); Tue, 16 Apr 2013 20:41:22 -0400 Received: by mail-ie0-f182.google.com with SMTP id at1so1304641iec.41 for ; Tue, 16 Apr 2013 17:41:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; bh=wkdr4e4lmpvEsn46F/ovybnLrWdeZF9EQm0yM0bkU5I=; b=LHWAeofPbrjpypcQ7+xtCEYwmft1qQRQJUewIQfoQDS5L7x17S4/VLZxqQtWLANyvY pa3Rq7EjldasdxAh3gZMKlfIjjHeC9YF+YKS4RGArIjMWgrVH2xXiKO4xHSROR3ei2le OIzg2N1uuMzDGn3z2jDX36ECTqkus8NR6Y6o8vZZKMib37zk+NNLthdNCIbqVRyasjmT 9kYIo+pRyAFiSeMNLRMUjYy8Ou5MyG5YtZ3igRzsb74nqFQ/ArkgBj4tcbr9IBTfRStz w2gSdlROeuS8Jl15sblenOeABqI368DRFM8BJiOazPzLe7vKghBXe0yyckvhYbmQdsmN DkUg== X-Received: by 10.50.107.106 with SMTP id hb10mr9405547igb.25.1366159282329; Tue, 16 Apr 2013 17:41:22 -0700 (PDT) MIME-Version: 1.0 Received: by 10.64.71.2 with HTTP; Tue, 16 Apr 2013 17:41:02 -0700 (PDT) In-Reply-To: <516DDB4D.9020500@ti.com> References: <1329321854-24490-1-git-send-email-b-cousson@ti.com> <514C79E1.4090106@wwwdotorg.org> <514CE0AB.6060207@ti.com> <515319D5.20105@wwwdotorg.org> <5155C902.7080207@wwwdotorg.org> <5165CB9D.1090202@wwwdotorg.org> <51671D7B.5060303@wwwdotorg.org> <51673D70.3010503@wwwdotorg.org> <516C31C3.9040505@wwwdotorg.org> <516C7C43.3040105@wwwdotorg.org> <516C8760.2050500@ti.com> <516D9B05.1000501@wwwdotorg.org> <516DA60A.5070000@ti.com> <516DCCA8.3070108@wwwdotorg.org> <516DDB4D.9020500@ti.com> From: Javier Martinez Canillas Date: Wed, 17 Apr 2013 02:41:02 +0200 Message-ID: Subject: Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver To: Jon Hunter Cc: Stephen Warren , Linus Walleij , Grant Likely , Alexandre Courbot , Stephen Warren , Kevin Hilman , "devicetree-discuss@lists.ozlabs.org" , "linux-omap@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" Sender: linux-omap-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-omap@vger.kernel.org On Wed, Apr 17, 2013 at 1:14 AM, Jon Hunter wrote: > > On 04/16/2013 05:11 PM, Stephen Warren wrote: >> On 04/16/2013 01:27 PM, Jon Hunter wrote: >>> >>> On 04/16/2013 01:40 PM, Stephen Warren wrote: >>>> On 04/15/2013 05:04 PM, Jon Hunter wrote: >> ... >>>>> If some driver is calling gpio_request() directly, then they will most >>>>> likely just call gpio_to_irq() when requesting the interrupt and so the >>>>> xlate function would not be called in this case (mmc drivers are a good >>>>> example). So I don't see that as being a problem. In fact that's the >>>>> benefit of this approach as AFAICT it solves this problem. >>>> >>>> Oh. That assumption seems very fragile. What about drivers that actually >>>> do have platform data (or DT bindings) that provide both the IRQ and >>>> GPIO IDs, and hence don't use gpio_to_irq()? That's entirely possible. >>> >>> Right. In the DT case though, if someone does provide the IRQ and GPIO >>> IDs then at least they would use a different xlate function. Another >>> option to consider would be defining the #interrupt-cells = <3> where we >>> would have ... >>> >>> cell-#1 --> IRQ domain ID >>> cell-#2 --> Trigger type >>> cell-#3 --> GPIO ID >>> >>> Then we could have a generic xlate for 3 cells that would also request >>> the GPIO. Again not sure if people are against a gpio being requested in >>> the xlate but just an idea. Or given that irq_of_parse_and_map() calls >>> the xlate, we could have this function call gpio_request() if the >>> interrupt controller is a gpio and there are 3 cells. >> >> I rather dislike this approach since: >> >> a) It requires changes to the DT bindings, which are already defined. >> Admittedly it's backwards-compatible, but still. >> >> b) There isn't really any need for the DT to represent this; the >> GPIO+IRQ driver itself already knows which IRQ ID is which GPIO ID and >> vice-versa (if the HW has such a concept), so there's no need for the DT >> to contain this information. This seems like pushing Linux's internal >> requirements into the design of the DT binding. > > Yes, so the only alternative is to use irq_to_gpio to avoid this. > >> c) I have the feeling that hooking the of_xlate function for this is a >> bit of an abuse of the function. > > I was wondering about that. So I was grep'ing through the various xlate > implementations and found this [1]. Also you may recall that in the > of_dma_simple_xlate() we call the dma_request_channel() to allocate the > channel, which is very similar. However, I don't wish to get a > reputation as abusing APIs so would be good to know if this really is > abuse or not ;-) > > Cheers > Jon > > [1] http://permalink.gmane.org/gmane.linux.ports.arm.kernel/195124 I was looking at [1] shared by Jon and come up with the following patch that does something similar for OMAP GPIO. This has the advantage that is local to gpio-omap instead changing gpiolib-of and also doesn't require DT changes But I don't want to get a reputation for abusing APIs neither :-) Best regards, Javier From 23368eb72b125227fcf4b22be19ea70b4ab94556 Mon Sep 17 00:00:00 2001 From: Javier Martinez Canillas Date: Wed, 17 Apr 2013 02:03:08 +0200 Subject: [PATCH 1/1] gpio/omap: add custom xlate function handler Signed-off-by: Javier Martinez Canillas --- drivers/gpio/gpio-omap.c | 29 ++++++++++++++++++++++++++++- 1 files changed, 28 insertions(+), 1 deletions(-) diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 8524ce5..77216f9 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -1097,6 +1097,33 @@ static void omap_gpio_chip_init(struct gpio_bank *bank) static const struct of_device_id omap_gpio_match[]; static void omap_gpio_init_context(struct gpio_bank *p); +static int omap_gpio_irq_domain_xlate(struct irq_domain *d, + struct device_node *ctrlr, + const u32 *intspec, unsigned int intsize, + irq_hw_number_t *out_hwirq, + unsigned int *out_type) +{ + int ret; + struct gpio_bank *bank = d->host_data; + int gpio = bank->chip.base + intspec[0]; + + if (WARN_ON(intsize < 2)) + return -EINVAL; + + ret = gpio_request_one(gpio, GPIOF_IN, ctrlr->full_name); + if (ret) + return ret; + + *out_hwirq = intspec[0]; + *out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE; + + return 0; +} + +static struct irq_domain_ops omap_gpio_irq_ops = { + .xlate = omap_gpio_irq_domain_xlate, +}; + static int omap_gpio_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -1144,7 +1171,7 @@ static int omap_gpio_probe(struct platform_device *pdev) bank->domain = irq_domain_add_linear(node, bank->width, - &irq_domain_simple_ops, NULL); + &omap_gpio_irq_ops, bank); if (!bank->domain) return -ENODEV;