2013-07-03 14:14:10

by Felipe Balbi

[permalink] [raw]

Subject: Re: [PATCH v3 2/8] wlcore: set irq_flags in the board files instead of hiding behind a quirk

Hi,

On Wed, Jul 03, 2013 at 05:03:23PM +0300, Luciano Coelho wrote:
> diff --git a/arch/arm/mach-omap2/board-4430sdp.c b/arch/arm/mach-omap2/board-4430sdp.c
> index 56a9a4f..953f620 100644
> --- a/arch/arm/mach-omap2/board-4430sdp.c
> +++ b/arch/arm/mach-omap2/board-4430sdp.c
> @@ -703,12 +703,30 @@ static void __init omap4_sdp4430_wifi_init(void)
>
> omap4_sdp4430_wifi_mux_init();
> omap4_sdp4430_wlan_data.irq = gpio_to_irq(GPIO_WIFI_IRQ);
> +
> + ret = gpio_request_one(GPIO_WIFI_IRQ, GPIOF_IN, "GPIO_WIFI_IRQ");
> + if (ret) {
> + pr_err("error requesting wl12xx gpio: %d\n", ret);
> + goto out;
> + }
> +
> + ret = irq_set_irq_type(gpio_to_irq(GPIO_WIFI_IRQ), IRQ_TYPE_LEVEL_HIGH);
> + if (ret) {
> + pr_err("error setting wl12xx irq type: %d\n", ret);
> + goto free;
> + }
> +
> ret = wl12xx_set_platform_data(&omap4_sdp4430_wlan_data);
> if (ret)
> pr_err("Error setting wl12xx data: %d\n", ret);
> +
> ret = platform_device_register(&omap_vwlan_device);
> if (ret)
> pr_err("Error registering wl12xx device: %d\n", ret);
> +out:
> + return;
> +free:
> + gpio_free(GPIO_WIFI_IRQ);

actually, you should leave this GPIO requested in order to use it as
IRQ.

ditto for all others

--
balbi

Attachments:

(No filename) (1.20 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-07-03 14:14:44

On Wed, Jul 03, 2013 at 05:03:25PM +0300, Luciano Coelho wrote:
> Instead of defining an enumeration with the FW specific values for the
> different clock rates, use the actual frequency instead. Also add a
> boolean to specify whether the clock is XTAL or not.
>
> Change all board files to reflect this.
>
> Cc: Tony Lindgren <[email protected]>
> Cc: Sekhar Nori <[email protected]>
> Signed-off-by: Luciano Coelho <[email protected]>

Reviewed-by: Felipe Balbi <[email protected]>

--
balbi

Attachments:

(No filename) (486.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-07-03 14:19:15

by Luciano Coelho

On Wed, 2013-07-03 at 17:12 +0200, Javier Martinez Canillas wrote:
> On Wed, Jul 3, 2013 at 4:15 PM, Luciano Coelho <[email protected]> wrote:
> > On Wed, 2013-07-03 at 17:03 +0300, Luciano Coelho wrote:
> >> The platform_quirk element in the platform data was used to change the
> >> way the IRQ is triggered. When set, the EDGE_IRQ quirk would change
> >> the irqflags used and treat edge trigger differently from the rest.
> >>
> >> Instead of hiding this irq flag setting behind the quirk, have the
> >> board files set the flags during initialization. This will be more
> >> meaningful than driver-specific quirks when we switch to DT.
> >>
> >> Additionally, fix missing gpio_request() calls in the boarding files
> >> (so that setting the flags actually works).
> >>
> >> Cc: Tony Lindgren <[email protected]>
> >> Cc: Sekhar Nori <[email protected]>
> >> Signed-off-by: Luciano Coelho <[email protected]>
> >> ---
> >
> > [...]
> >
> >> @@ -5928,16 +5927,21 @@ static void wlcore_nvs_cb(const struct firmware *fw, void *context)
> >> wlcore_adjust_conf(wl);
> >>
> >> wl->irq = platform_get_irq(pdev, 0);
> >> - wl->platform_quirks = pdata->platform_quirks;
> >> wl->if_ops = pdev_data->if_ops;
> >>
> >> - if (wl->platform_quirks & WL12XX_PLATFORM_QUIRK_EDGE_IRQ)
> >> - irqflags = IRQF_TRIGGER_RISING;
> >> - else
> >> - irqflags = IRQF_TRIGGER_HIGH | IRQF_ONESHOT;
> >> + irq_data = irq_get_irq_data(wl->irq);
> >> + if (!irq_data) {
> >> + wl1271_error("couldn't get irq data for irq %d\n", wl->irq);
> >> + ret = -EINVAL;
> >> + goto out_free_nvs;
> >> + }
> >> +
> >> + wl->irq_flags = irqd_get_trigger_type(irq_data);
> >
> > BTW, there seems to be a patch on its way to make reading the flags
> > easier (ie. no need to get the irq_data first):
> >
> > http://mid.gmane.org/[email protected]
> >
> > I'm not sure if this is going to be taken in, but if it does, it would
> > be nice to change the code here to use the new irq_get_trigger_type()
> > function.

> That patch has been already merged in Linus tree as commit 1f6236bf
> ("genirq: Add irq_get_trigger_type() to get IRQ flags").
>
> So yes, it would be better if you can use irq_get_trigger_type()
> instead calling irq_get_irq_data() + irqd_get_trigger_type().

That's great, thanks! I'll make this change as soon as I get your patch
into my tree (ie. after the merge window closes).

--
Luca.