Re: [PATCH 2/3] ASoC: OMAP4: Add support for McPDM

[Felipe Balbi <felipe.balbi@nokia.com>]

Hi,

On Fri, Dec 18, 2009 at 06:01:19PM +0100, ext Candelaria Villareal, Jorge wrote:
>Mmm... But it _does_ have some breaks. Besides, I am still unsure that
>if structure should be used here. Code would be duplicated, for example,
>DN_IRQ_FULL and DN_IRQ_EMPTY share the same procedure to acknowledge
>the request.

quoting your switch for irq here:

> +       switch (irq_status) {
> +       case DN_IRQ_FULL:
> +       case DN_IRQ_EMTPY:
> +               dev_err(mcpdm_irq->dev, "DN FIFO error %x\n", irq_status);
> +               omap_mcpdm_reset(MCPDM_DOWNLINK, 1);
> +               omap_mcpdm_set_downlink(mcpdm_irq->downlink);
> +               omap_mcpdm_reset(MCPDM_DOWNLINK, 0);
> +               break;
> +       case DN_IRQ:
> +               dev_dbg(mcpdm_irq->dev, "DN write request\n");
> +               break;
> +       case UP_IRQ_FULL:
> +       case UP_IRQ_EMPTY:
> +               dev_err(mcpdm_irq->dev, "UP FIFO error %x\n", irq_status);
> +               omap_mcpdm_reset(MCPDM_UPLINK, 1);
> +               omap_mcpdm_set_uplink(mcpdm_irq->uplink);
> +               omap_mcpdm_reset(MCPDM_UPLINK, 0);
> +               break;
> +       case UP_IRQ:
> +               dev_dbg(mcpdm_irq->dev, "UP write request\n");
> +               break;
> +       }

what happens if you have both DN_IRQ_FULL and DN_IRQ_EMPTY at the same 
time ?

irq_status == DN_IRQ_FULL will evaluate to false and
irq_status == DN_IRQ_EMPTY will also evaluate to false so none of those 
case statements will execute. Similarly to other case statements.

if you have to execute the same piece of code for two different irqs you 
can always:

if ((irq_status & DN_IRQ_FULL) || (irq_status & DN_IRQ_EMPTY))
	ack_those_irqs();

this code might be working now only out of luck, simply because you 
didn't have two irqs hapenning at the same time. Do not use switch() on 
bitmasks, it won't work always.

-- 
balbi