Re: [PATCH 3/5] gpio/omap: Add DT support to GPIO driver

[Javier Martinez Canillas <martinez.javier@gmail.com>]

On Wed, Apr 17, 2013 at 1:14 AM, Jon Hunter <jon-hunter@ti.com> 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 <javier.martinez@collabora.co.uk>
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 <javier.martinez@collabora.co.uk>
---
 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;

-- 
1.7.7.6