On Wed, Mar 19, 2014 at 6:09 PM, Tien Hock Loh <[email protected]> wrote:
> On Fri, Mar 7, 2014 at 11:14 PM, Josh Cartwright <[email protected]> wrote:
>> On Mon, Mar 03, 2014 at 06:27:43PM +0800, [email protected] wrote:
>>> From: Tien Hock Loh <[email protected]>
>>>
>>> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
>>> Tested on Altera CV SoC board using dipsw and LED using LED framework.
>>>
>>> Signed-off-by: Tien Hock Loh <[email protected]>
>>> ---
>>
>>> + return -ENOMEM;
>>> + }
>>> + altera_gc->domain = 0;
>>> +
>>> + spin_lock_init(&altera_gc->gpio_lock);
>>> +
>>> + id = pdev->id;
>>> +
>>> + if (of_property_read_u32(node, "altr,gpio-bank-width", ®))
>>> + /*By default assume full GPIO controller*/
>>> + altera_gc->mmchip.gc.ngpio = 32;
>>> + else
>>> + altera_gc->mmchip.gc.ngpio = reg;
>>> +
>>> + if (altera_gc->mmchip.gc.ngpio > 32) {
>>> + dev_warn(&pdev->dev,
>>> + "ngpio is greater than 32, defaulting to 32\n");
>>> + altera_gc->mmchip.gc.ngpio = 32;
>>> + }
>>> +
>>> + altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input;
>>> + altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
>>> + altera_gc->mmchip.gc.get = altera_gpio_get;
>>> + altera_gc->mmchip.gc.set = altera_gpio_set;
>>> + altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
>>> + altera_gc->mmchip.gc.owner = THIS_MODULE;
>>> +
>>> + ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
>>> + if (ret) {
>>> + dev_err(&pdev->dev, "Failed adding memory mapped gpiochip\n");
>>> + return ret;
>>> + }
>>> +
>>> + platform_set_drvdata(pdev, altera_gc);
>>> +
>>> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
>>>
>>
>> platform_get_irq(pdev, 0);
>>
> OK.
>
platform_get_irq doesn't create the irq mapping which is needed by the
driver. Since this driver is targeted at using of, should I be using
irq_of_parse_and_map or should I still redo the codes with
platform_get_irq and irq_create_mapping? I think the latter would be
introducing code redundancy. Please advice.
Thanks.
On Mon, Apr 07, 2014 at 04:00:43PM +0800, Tien Hock Loh wrote:
> On Wed, Mar 19, 2014 at 6:09 PM, Tien Hock Loh <[email protected]> wrote:
> > On Fri, Mar 7, 2014 at 11:14 PM, Josh Cartwright <[email protected]> wrote:
> >> On Mon, Mar 03, 2014 at 06:27:43PM +0800, [email protected] wrote:
> >>> From: Tien Hock Loh <[email protected]>
[..]
> >>> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
> >>>
> >>
> >> platform_get_irq(pdev, 0);
> >>
> > OK.
> >
>
> platform_get_irq doesn't create the irq mapping which is needed by the
> driver. Since this driver is targeted at using of, should I be using
> irq_of_parse_and_map or should I still redo the codes with
> platform_get_irq and irq_create_mapping? I think the latter would be
> introducing code redundancy. Please advice.
Yes, it is technically true that platform_get_irq() doesn't do the
mapping directly, but that's because the mapping is setup earlier, when
of_device_alloc() (drivers/of/platform.c) allocates resources for your
platform device.
Calling irq_of_parse_and_map() should be unnecessary.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
>>>>> Add driver support for Altera GPIO soft IP, including interrupts and I/O.
>>>>> Tested on Altera CV SoC board using dipsw and LED using LED framework.
>>>>>
>>>>> Signed-off-by: Tien Hock Loh <[email protected]>
>>>>> ---
>>>>
>>>>> + return -ENOMEM;
>>>>> + }
>>>>> + altera_gc->domain = 0;
>>>>> +
>>>>> + spin_lock_init(&altera_gc->gpio_lock);
>>>>> +
>>>>> + id = pdev->id;
>>>>> +
>>>>> + if (of_property_read_u32(node, "altr,gpio-bank-width", ®))
>>>>> + /*By default assume full GPIO controller*/
>>>>> + altera_gc->mmchip.gc.ngpio = 32;
>>>>> + else
>>>>> + altera_gc->mmchip.gc.ngpio = reg;
>>>>> +
>>>>> + if (altera_gc->mmchip.gc.ngpio > 32) {
>>>>> + dev_warn(&pdev->dev,
>>>>> + "ngpio is greater than 32, defaulting to 32\n");
>>>>> + altera_gc->mmchip.gc.ngpio = 32;
>>>>> + }
>>>>> +
>>>>> + altera_gc->mmchip.gc.direction_input = altera_gpio_direction_input;
>>>>> + altera_gc->mmchip.gc.direction_output = altera_gpio_direction_output;
>>>>> + altera_gc->mmchip.gc.get = altera_gpio_get;
>>>>> + altera_gc->mmchip.gc.set = altera_gpio_set;
>>>>> + altera_gc->mmchip.gc.to_irq = altera_gpio_to_irq;
>>>>> + altera_gc->mmchip.gc.owner = THIS_MODULE;
>>>>> +
>>>>> + ret = of_mm_gpiochip_add(node, &altera_gc->mmchip);
>>>>> + if (ret) {
>>>>> + dev_err(&pdev->dev, "Failed adding memory mapped gpiochip\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + platform_set_drvdata(pdev, altera_gc);
>>>>> +
>>>>> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
>>>>>
>>>>
>>>> platform_get_irq(pdev, 0);
>>>>
>>> OK.
>>>
>>
>> platform_get_irq doesn't create the irq mapping which is needed by the
>> driver. Since this driver is targeted at using of, should I be using
>> irq_of_parse_and_map or should I still redo the codes with
>> platform_get_irq and irq_create_mapping? I think the latter would be
>> introducing code redundancy. Please advice.
>
> Yes, it is technically true that platform_get_irq() doesn't do the mapping directly, but that's
> because the mapping is setup earlier, when
> of_device_alloc() (drivers/of/platform.c) allocates resources for your platform device.
>
> Calling irq_of_parse_and_map() should be unnecessary.
I checked and tried running the without irq_create_mapping but it
seems the mapping is not done.
What I've seen other GPIO driver is doing is to create the mapping
during the gpio_to_irq call. However Linus suggested we are avoiding
that route, thus the use of irq_of_parse_and_map. Do you agree with my
findings?
>>
>> Thanks.
On Isn, 2014-04-07 at 12:11 -0500, Josh Cartwright wrote:
> On Mon, Apr 07, 2014 at 04:00:43PM +0800, Tien Hock Loh wrote:
> > On Wed, Mar 19, 2014 at 6:09 PM, Tien Hock Loh <[email protected]> wrote:
> > > On Fri, Mar 7, 2014 at 11:14 PM, Josh Cartwright <[email protected]> wrote:
> > >> On Mon, Mar 03, 2014 at 06:27:43PM +0800, [email protected] wrote:
> > >>> From: Tien Hock Loh <[email protected]>
> [..]
> > >>> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
> > >>>
> > >>
> > >> platform_get_irq(pdev, 0);
> > >>
> > > OK.
> > >
> >
> > platform_get_irq doesn't create the irq mapping which is needed by the
> > driver. Since this driver is targeted at using of, should I be using
> > irq_of_parse_and_map or should I still redo the codes with
> > platform_get_irq and irq_create_mapping? I think the latter would be
> > introducing code redundancy. Please advice.
>
> Yes, it is technically true that platform_get_irq() doesn't do the
> mapping directly, but that's because the mapping is setup earlier, when
> of_device_alloc() (drivers/of/platform.c) allocates resources for your
> platform device.
>
> Calling irq_of_parse_and_map() should be unnecessary.
I checked and tried running the without irq_create_mapping but it seems
the mapping is not done. What I've seen other GPIO driver is doing is to
create the mapping during the gpio_to_irq call. However Linus suggested
we are avoiding that route, thus the use of irq_of_parse_and_map.
Do you agree with my findings?
>
________________________________
Confidentiality Notice.
This message may contain information that is confidential or otherwise protected from disclosure. If you are not the intended recipient, you are hereby notified that any use, disclosure, dissemination, distribution, or copying of this message, or any attachments, is strictly prohibited. If you have received this message in error, please advise the sender by reply e-mail, and delete the message and any attachments. Thank you.
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, 7 Apr 2014 12:11:15 -0500, Josh Cartwright <[email protected]> wrote:
> On Mon, Apr 07, 2014 at 04:00:43PM +0800, Tien Hock Loh wrote:
> > On Wed, Mar 19, 2014 at 6:09 PM, Tien Hock Loh <[email protected]> wrote:
> > > On Fri, Mar 7, 2014 at 11:14 PM, Josh Cartwright <[email protected]> wrote:
> > >> On Mon, Mar 03, 2014 at 06:27:43PM +0800, [email protected] wrote:
> > >>> From: Tien Hock Loh <[email protected]>
> [..]
> > >>> + altera_gc->mapped_irq = irq_of_parse_and_map(node, 0);
> > >>>
> > >>
> > >> platform_get_irq(pdev, 0);
> > >>
> > > OK.
> > >
> >
> > platform_get_irq doesn't create the irq mapping which is needed by the
> > driver. Since this driver is targeted at using of, should I be using
> > irq_of_parse_and_map or should I still redo the codes with
> > platform_get_irq and irq_create_mapping? I think the latter would be
> > introducing code redundancy. Please advice.
>
> Yes, it is technically true that platform_get_irq() doesn't do the
> mapping directly, but that's because the mapping is setup earlier, when
> of_device_alloc() (drivers/of/platform.c) allocates resources for your
> platform device.
>
> Calling irq_of_parse_and_map() should be unnecessary.
In the latest mainline, platform_get_irq() now does the mapping when
using an IRQ specified in the device tree.
g.