diff mbox

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

Message ID CAAwP0s0e2x33jZQhFRY+Hoo61SFqYKRgWk=1mL2+DNxSDJHz+A@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Javier Martinez Canillas April 14, 2013, 1:35 a.m. UTC
On Fri, Apr 12, 2013 at 12:47 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/11/2013 04:16 PM, Linus Walleij wrote:
>> On Thu, Apr 11, 2013 at 10:30 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 04/10/2013 03:28 PM, Linus Walleij wrote:
>>
>>>> So the only reason I'm rambing on about this is that it breaks the
>>>
>>> I'm not sure I understand this paragraph; what is "it" in the line above.
>>>
>>> If "it" is this patch, then should "breaks" be re-establishes?
>>
>> No I'm replying to Javier Martinez Canillas mail in this thread:
>> http://marc.info/?l=linux-arm-kernel&m=136334655902407&w=2
>> which is stating a question to grand, and contains the below
>> hunk:
>>
>>> +static int gpio_irq_request(struct irq_data *d)
>>> +{
>>> +       struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>>> +
>>> +       return gpio_request(irq_to_gpio(bank, d->irq), "gpio-irq");
>>> +}
>>
>> irq_to_gpio(). Notice that. not my_funny_driver_irq_to_gpio().
>
> OK, right. sorry for being so confused/confusing.
>
> Yes, that code should certainly call e.g. omap_gpio__irq_to_gpio() not
> irq_to_gpio(). Probably gpio_irq_request() wants renaming to something
> more like omap_gpio__irq_request() too, so the names don't look like
> they'd clash with global functions. (__ added for clarity but probably
> only one _ at a time)

Stephen, Linus,

Is the following inlined patch [1] what you were thinking that would
be the right approach?

With this patch an explicit call to call gpio_request() before a call
to chip->irq_set_type() is needed. I've tested both with DT and
without DT where a explicit call to gpio_request() is made and it
works in both cases. So it shouldn't have a functional change for
non-DT cases as far as I know.

If you agree with [1] then I'll split in two patches (one that adds
the irq_request function pointer to irq_chip and another one that adds
the implementation for gpio-omap) and send as a patch-set. I just
thought that it would be easier for you if I posted here an inlined
patch so you could have context.

Thanks a lot for your feedback and best regards,
Javier

[1]

Comments

Linus Walleij April 14, 2013, 8:53 p.m. UTC | #1
On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
<martinez.javier@gmail.com> wrote:

> Is the following inlined patch [1] what you were thinking that would
> be the right approach?

This looks sort of OK, but I'm still struggling with the question of
what we could do to help other implementations facing the same issue.

This is a pretty hard design pattern to properly replicate in every such
driver is it not?

Hmmmm

Yours,
Linus Walleij
Stephen Warren April 15, 2013, 4:53 p.m. UTC | #2
On 04/13/2013 07:35 PM, Javier Martinez Canillas wrote:
...
> Is the following inlined patch [1] what you were thinking that would
> be the right approach?
...
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
...
> +static int omap_gpio_irq_request(struct irq_data *d)
> +{
> +	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
> +
> +	return omap_gpio_request(&bank->chip, d->hwirq);

If you want the GPIO usage to be known to the GPIO subsystem, then
wouldn't you call gpio_request() here rather than omap_gpio_request()?
The above code will certainly do enough so that the OMAP GPIO HW is
fully enabled as you need, but I thought the idea was to also prevent
some other code successfully running gpio_request() on that same GPIO?
Stephen Warren April 15, 2013, 4:58 p.m. UTC | #3
On 04/14/2013 02:53 PM, Linus Walleij wrote:
> On Sun, Apr 14, 2013 at 3:35 AM, Javier Martinez Canillas
> <martinez.javier@gmail.com> wrote:
> 
>> Is the following inlined patch [1] what you were thinking that would
>> be the right approach?
> 
> This looks sort of OK, but I'm still struggling with the question of
> what we could do to help other implementations facing the same issue.
> 
> This is a pretty hard design pattern to properly replicate in every such
> driver is it not?

Well, instead of adding .request_irq() to the irqchip, and then making
each driver call gpio_request() from the implementation, perhaps you
could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
and if it gets back a non-error response, the IRQ core could call
gpio_request(). That means that the change to each GPIO+IRQ driver is
simply to implement a standalone data transformation function
.irq_to_gpio().

Now, this does re-introduce irq_to_gpio() in some way, but with the
following advantages:

1) The implementation is per-controller, not a single global function,
so isn't introducing any kind of centralized mapping scheme again.

2) This irq-chip-specific .irq_to_gpio() would only be implemented for
IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
Its potential existence doesn't imply that all IRQ chips need implement
this; it would be very specifically be for this one particular case.

So, I think it's reasonable to introduce this.
Hunter, Jon April 15, 2013, 8 p.m. UTC | #4
On 04/15/2013 11:53 AM, Stephen Warren wrote:
> On 04/13/2013 07:35 PM, Javier Martinez Canillas wrote:
> ...
>> Is the following inlined patch [1] what you were thinking that would
>> be the right approach?
> ...
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> ...
>> +static int omap_gpio_irq_request(struct irq_data *d)
>> +{
>> +	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
>> +
>> +	return omap_gpio_request(&bank->chip, d->hwirq);
> 
> If you want the GPIO usage to be known to the GPIO subsystem, then
> wouldn't you call gpio_request() here rather than omap_gpio_request()?
> The above code will certainly do enough so that the OMAP GPIO HW is
> fully enabled as you need, but I thought the idea was to also prevent
> some other code successfully running gpio_request() on that same GPIO?

Also, although omap gpios default to being inputs, we should not assume
that. So may be you should call gpio_request_one() here passing as flags
GPIOF_IN to configure as an input.

Cheers
Jon
Linus Walleij April 26, 2013, 6:59 a.m. UTC | #5
On Mon, Apr 15, 2013 at 6:58 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/14/2013 02:53 PM, Linus Walleij wrote:

>> This is a pretty hard design pattern to properly replicate in every such
>> driver is it not?
>
> Well, instead of adding .request_irq() to the irqchip, and then making
> each driver call gpio_request() from the implementation, perhaps you
> could add a .irq_to_gpio() to the irqchip, have the IRQ core call that,
> and if it gets back a non-error response, the IRQ core could call
> gpio_request(). That means that the change to each GPIO+IRQ driver is
> simply to implement a standalone data transformation function
> .irq_to_gpio().
>
> Now, this does re-introduce irq_to_gpio() in some way, but with the
> following advantages:
>
> 1) The implementation is per-controller, not a single global function,
> so isn't introducing any kind of centralized mapping scheme again.
>
> 2) This irq-chip-specific .irq_to_gpio() would only be implemented for
> IRQ+GPIO chips that actually have a 1:1 mapping between GPIOs and IRQs.
> Its potential existence doesn't imply that all IRQ chips need implement
> this; it would be very specifically be for this one particular case.

I sort of like the idea. It'd have to go past Grant and Thomas G though.

A problem is that this will result in the same kind of dilemma that
the pinctrl subsystem has been facing with mapping the global GPIO
numbers back into local numbers (as is the nature of IRQ numbers).
Such as is done by the GPIO ranges in pinctrl.

The good part is that we now know how to actually contain that, even
how to do it in DT.

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 685e850..e035e64 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -811,6 +811,13 @@  static void gpio_unmask_irq(struct irq_data *d)
 	spin_unlock_irqrestore(&bank->lock, flags);
 }

+static int omap_gpio_irq_request(struct irq_data *d)
+{
+	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
+
+	return omap_gpio_request(&bank->chip, d->hwirq);
+}
+
 static struct irq_chip gpio_irq_chip = {
 	.name		= "GPIO",
 	.irq_shutdown	= gpio_irq_shutdown,
@@ -819,6 +826,7 @@  static struct irq_chip gpio_irq_chip = {
 	.irq_unmask	= gpio_unmask_irq,
 	.irq_set_type	= gpio_irq_type,
 	.irq_set_wake	= gpio_wake_enable,
+	.irq_request    = omap_gpio_irq_request,
 };

 /*---------------------------------------------------------------------*/
@@ -1028,6 +1036,7 @@  omap_mpuio_alloc_gc(struct gpio_bank *bank,
unsigned int irq_start,
 	ct->chip.irq_mask = irq_gc_mask_set_bit;
 	ct->chip.irq_unmask = irq_gc_mask_clr_bit;
 	ct->chip.irq_set_type = gpio_irq_type;
+	ct->chip.irq_request = omap_gpio_irq_request;

 	if (bank->regs->wkup_en)
 		ct->chip.irq_set_wake = gpio_wake_enable,
diff --git a/include/linux/irq.h b/include/linux/irq.h
index 0e8e3a6..85596cc 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -303,6 +303,7 @@  struct irq_chip {
 	void		(*irq_shutdown)(struct irq_data *data);
 	void		(*irq_enable)(struct irq_data *data);
 	void		(*irq_disable)(struct irq_data *data);
+	int             (*irq_request)(struct irq_data *data);

 	void		(*irq_ack)(struct irq_data *data);
 	void		(*irq_mask)(struct irq_data *data);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index fa17855..a4bd4f7 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -588,6 +588,12 @@  int __irq_set_trigger(struct irq_desc *desc,
unsigned int irq,
 			unmask = 1;
 	}

+	if (chip->irq_request) {
+			ret = chip->irq_request(&desc->irq_data);
+			if (ret)
+				return ret;
+	}
+
 	/* caller masked out all except trigger mode flags */
 	ret = chip->irq_set_type(&desc->irq_data, flags);