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

[Jon Hunter <jon-hunter@ti.com>]

On 02/26/2013 06:13 PM, Stephen Warren wrote:
> On 02/26/2013 04:45 PM, Jon Hunter wrote:
>>
>> On 02/26/2013 05:06 PM, Stephen Warren wrote:
>>> On 02/26/2013 04:01 PM, Jon Hunter wrote:
>>>>
>>>> On 02/26/2013 04:44 PM, Stephen Warren wrote:
>>>>> On 02/26/2013 03:40 PM, Jon Hunter wrote:
>>>>>>
>>>>>> On 02/26/2013 04:01 AM, Javier Martinez Canillas wrote:
>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>> I was wondering if the level/edge settings for gpios is working on OMAP.
>>>>>>>
>>>>>>> I'm adding DT support for an SMSC911x ethernet chip connected to the
>>>>>>> GPMC for an OMAP3 SoC based board.
>>>>>>>
>>>>>>> In the smsc911x driver probe function (smsc911x_drv_probe() in
>>>>>>> drivers/net/ethernet/smsc/smsc911x.c), a call to request_irq() with
>>>>>>> the flag IRQF_TRIGGER_LOW is needed because of the wiring on my board.
>>>>>>>
>>>>>>> Reading the gpio-omap.txt documentation it says that #interrupt-cells
>>>>>>> should be <2> and that a value of 8 is "active low level-sensitive".
>>>>>>>
>>>>>>> So I tried this:
>>>>>>>
>>>>>>> &gpmc {
>>>>>>> 	ethernet@5,0 {
>>>>>>> 		pinctrl-names = "default";
>>>>>>> 		pinctrl-0 = <&smsc911x_pins>;
>>>>>>> 		compatible = "smsc,lan9221", "smsc,lan9115";
>>>>>>> 		reg = <5 0 0xff>; /* CS5 */
>>>>>>> 		interrupt-parent = <&gpio6>;
>>>>>>> 		interrupts = <16 8>; /* gpio line 176 */
>>>>>>> 		interrupt-names = "smsc911x irq";
>>>>>>> 		vmmc-supply = <&vddvario>;
>>>>>>> 		vmmc_aux-supply = <&vdd33a>;
>>>>>>> 		reg-io-width = <4>;
>>>>>>>
>>>>>>> 		smsc,save-mac-address;
>>>>>>>       };
>>>>>>> };
>>>>>>
>>>>>> Are you requesting the gpio anywhere? If not then this is not going to
>>>>>> work as-is. This was discussed fairly recently [1] and the conclusion
>>>>>> was that the gpio needs to be requested before we can use as an interrupt.
>>>>>
>>>>> That seems wrong; the GPIO/IRQ driver should handle this internally. The
>>>>> Ethernet driver shouldn't know/care whether the interrupt it's given is
>>>>> some form of dedicated interrupt or a GPIO-based interrupt, and even if
>>>>> it somehow did, there's no irq_to_gpio() any more, so the driver can't
>>>>> tell which GPIO ID it should request, unless it's given yet another
>>>>> property to represent this.
>>>>
>>>> I agree that ideally this should be handled internally. Did you read the
>>>> discussion on the thread that I referenced [1]? If you have any thoughts
>>>> we are open to ideas :-)
>>>>
>>>> Cheers
>>>> Jon
>>>>
>>>> [1] http://comments.gmane.org/gmane.linux.ports.arm.omap/92192
>>>
>>> Oh, when I clicked that link the first time, all I saw was the patch,
>>> not any discussion. I guess it must have timed out finding the other
>>> emails or something.
>>
>> Actually, I sent a slightly different link the 2nd time to ensure you
>> saw the thread. So my fault ;-)
>>
>>> I disagree that the GPIO needs to be requested, and that a custom DT
>>> property and associated code are needed for that; simply requesting the
>>> IRQ should be enough to make everything work.
>>>
>>> In the Tegra GPIO IRQ driver for example, the irq_set_type irq_chip op
>>> goes and configures the base GPIO HW to enable the pin as a GPIO, just
>>> like gpio_request() would. I imagine the OMAP driver can do whatever
>>> similar action it needs.
>>
>> Yes that is similar to what the patch in the thread was attempting to
>> do, but this got shot down.
>>
>> One issue I see is that by not calling gpio_request, then potentially
>> you could have someone request a gpio via gpio_request() and someone
>> trying to use it as an interrupt source via request_irq(). Now obviously
>> that represents a bug because there is only one physical gpio, but I
>> gather it is something we need to protect against.
> 
> I'm not sure it's really that much of an issue, but presumably the
> solution is for a combined GPIO+IRQ driver to simply call gpio_request
> internally from within some irq_chip function. It looks like struct
> irq_chip doesn't have a request/free, but perhaps they could be added to
> solve this?

Yes I was wondering if we could do something like that. That would work,
may be that's what we should propose.

Thanks
Jon